diff --git a/src/cli_util.rs b/src/cli_util.rs index 40e7a6e8b..29ed24ad5 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -52,7 +52,7 @@ use jujutsu_lib::workspace::{Workspace, WorkspaceInitError, WorkspaceLoadError}; use jujutsu_lib::{dag_walk, file_util, git, revset}; use crate::config::read_config; -use crate::diff_edit::DiffEditError; +use crate::diff_edit::{ConflictResolveError, DiffEditError}; use crate::formatter::Formatter; use crate::templater::TemplateFormatter; use crate::ui::{ColorChoice, Ui}; @@ -154,6 +154,12 @@ impl From for CommandError { } } +impl From for CommandError { + fn from(err: ConflictResolveError) -> Self { + user_error(format!("Failed to use external tool to resolve: {err}")) + } +} + impl From for CommandError { fn from(err: git2::Error) -> Self { user_error(format!("Git operation failed: {err}")) @@ -756,14 +762,33 @@ impl WorkspaceCommandHelper { Ok(()) } + pub fn run_mergetool( + &self, + ui: &mut Ui, + tree: &Tree, + path: &str, + ) -> Result { + Ok(crate::diff_edit::run_mergetool( + ui, + tree, + &self.parse_file_path(path)?, + )?) + } + pub fn edit_diff( &self, ui: &mut Ui, left_tree: &Tree, right_tree: &Tree, instructions: &str, - ) -> Result { - crate::diff_edit::edit_diff(ui, left_tree, right_tree, instructions, self.base_ignores()) + ) -> Result { + Ok(crate::diff_edit::edit_diff( + ui, + left_tree, + right_tree, + instructions, + self.base_ignores(), + )?) } pub fn select_diff( diff --git a/src/commands.rs b/src/commands.rs index 9f6da7a1b..5bc723334 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -91,6 +91,7 @@ enum Commands { Unsquash(UnsquashArgs), Restore(RestoreArgs), Touchup(TouchupArgs), + Resolve(ResolveArgs), Split(SplitArgs), /// Merge work from multiple branches /// @@ -495,6 +496,35 @@ struct UnsquashArgs { interactive: bool, } +/// Resolve a conflicted file with an external merge tool +/// +/// Only conflicts that can be resolved with a 3-way merge are supported. See +/// docs for merge tool configuration instructions. +/// +/// Note that conflicts can also be resolved without using this command. You may +/// edit the conflict markers in the conflicted file directly with a text +/// editor. +// TODOs: +// - `jj resolve --list` to list conflicts +// - `jj resolve --editor` to resolve a conflict in the default text editor. Should work for +// conflicts with 3+ adds. Useful to resolve conflicts in a commit other than the current one. +// - Make the `path` argument optional and/or repeatable. If a specific file is not specified, +// either pick an arbitrary file with a conflict (e.g. first one `jj resolve --list` shows), +// offer a UI, and/or loop over all the conflicted files. +// - A way to help split commits with conflicts that are too complicated (more than two sides) +// into commits with simpler conflicts. In case of a tree with many merges, we could for example +// point to existing commits with simpler conflicts where resolving those conflicts would help +// simplify the present one. +#[derive(clap::Args, Clone, Debug)] +struct ResolveArgs { + #[arg(long, short, default_value = "@")] + revision: String, + /// The path to a file with conflicts. You can use `jj status` to find such + /// files + #[arg(value_hint = clap::ValueHint::AnyPath)] + path: String, +} + /// Restore paths from another revision /// /// That means that the paths get the same content in the destination (`--to`) @@ -2828,6 +2858,25 @@ aborted. Ok(()) } +fn cmd_resolve( + ui: &mut Ui, + command: &CommandHelper, + args: &ResolveArgs, +) -> Result<(), CommandError> { + let mut workspace_command = command.workspace_helper(ui)?; + let commit = workspace_command.resolve_single_rev(&args.revision)?; + workspace_command.check_rewriteable(&commit)?; + let mut tx = workspace_command.start_transaction(&format!( + "Resolve conflicts in commit {}", + commit.id().hex() + )); + let new_tree_id = workspace_command.run_mergetool(ui, &commit.tree(), &args.path)?; + CommitBuilder::for_rewrite_from(ui.settings(), &commit) + .set_tree(new_tree_id) + .write_to_repo(tx.mut_repo()); + workspace_command.finish_transaction(ui, tx) +} + fn cmd_restore( ui: &mut Ui, command: &CommandHelper, @@ -4651,6 +4700,7 @@ pub fn run_command( Commands::Merge(sub_args) => cmd_merge(ui, command_helper, sub_args), Commands::Rebase(sub_args) => cmd_rebase(ui, command_helper, sub_args), Commands::Backout(sub_args) => cmd_backout(ui, command_helper, sub_args), + Commands::Resolve(sub_args) => cmd_resolve(ui, command_helper, sub_args), Commands::Branch(sub_args) => cmd_branch(ui, command_helper, sub_args), Commands::Undo(sub_args) => cmd_op_undo(ui, command_helper, sub_args), Commands::Operation(sub_args) => cmd_operation(ui, command_helper, sub_args), diff --git a/src/diff_edit.rs b/src/diff_edit.rs index 528b2d64f..c20a5aeb4 100644 --- a/src/diff_edit.rs +++ b/src/diff_edit.rs @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::collections::HashMap; use std::fs::File; use std::io::Write; use std::path::{Path, PathBuf}; @@ -20,7 +21,10 @@ use std::sync::Arc; use config::ConfigError; use itertools::Itertools; -use jujutsu_lib::backend::TreeId; +use jujutsu_lib::backend::{TreeId, TreeValue}; +use jujutsu_lib::conflicts::{ + describe_conflict, extract_file_conflict_as_single_hunk, materialize_merge_result, +}; use jujutsu_lib::gitignore::GitIgnoreFile; use jujutsu_lib::matchers::EverythingMatcher; use jujutsu_lib::repo_path::RepoPath; @@ -33,36 +37,69 @@ use thiserror::Error; use crate::ui::Ui; #[derive(Debug, Error)] -pub enum DiffEditError { +pub enum ExternalToolError { #[error("Invalid config: {0}")] ConfigError(#[from] ConfigError), - #[error("The diff tool exited with a non-zero code")] - DifftoolAborted, - #[error("Failed to write directories to diff: {0:?}")] - CheckoutError(CheckoutError), #[error("Error setting up temporary directory: {0:?}")] SetUpDirError(#[source] std::io::Error), - #[error("Error executing editor '{editor_binary}': {source}")] - ExecuteEditorError { - editor_binary: String, + #[error("Error executing '{tool_binary}': {source}")] + FailedToExecute { + tool_binary: String, #[source] source: std::io::Error, }, + #[error("Tool exited with a non-zero code.")] + ToolAborted, #[error("I/O error: {0:?}")] - IoError(#[source] std::io::Error), - #[error("Failed to snapshot changes: {0:?}")] - SnapshotError(SnapshotError), + IoError(#[from] std::io::Error), } -impl From for DiffEditError { - fn from(err: CheckoutError) -> Self { - DiffEditError::CheckoutError(err) +#[derive(Debug, Error)] +pub enum DiffEditError { + #[error("{0}")] + ExternalToolError(#[from] ExternalToolError), + #[error("Failed to write directories to diff: {0:?}")] + CheckoutError(#[from] CheckoutError), + #[error("Failed to snapshot changes: {0:?}")] + SnapshotError(#[from] SnapshotError), +} + +#[derive(Debug, Error)] +pub enum ConflictResolveError { + #[error("{0}")] + ExternalToolError(#[from] ExternalToolError), + #[error("Couldn't find the path {0:?} in this revision")] + PathNotFoundError(RepoPath), + #[error("Couldn't find any conflicts at {0:?} in this revision")] + NotAConflictError(RepoPath), + #[error( + "Only conflicts that involve normal files (not symlinks, not executable, etc.) are \ + supported. Conflict summary:\n {1}" + )] + NotNormalFilesError(RepoPath, String), + #[error( + "The conflict at {path:?} has {removes} removes and {adds} adds.\nAt most 1 remove and 2 \ + adds are supported." + )] + ConflictTooComplicatedError { + path: RepoPath, + removes: usize, + adds: usize, + }, + #[error("The output file is either unchanged or empty after the editor quit.")] + EmptyOrUnchanged, + #[error("Backend error: {0:?}")] + BackendError(#[from] jujutsu_lib::backend::BackendError), +} + +impl From for DiffEditError { + fn from(err: std::io::Error) -> Self { + DiffEditError::ExternalToolError(ExternalToolError::from(err)) } } - -impl From for DiffEditError { - fn from(err: SnapshotError) -> Self { - DiffEditError::SnapshotError(err) +impl From for ConflictResolveError { + fn from(err: std::io::Error) -> Self { + ConflictResolveError::ExternalToolError(ExternalToolError::from(err)) } } @@ -73,8 +110,8 @@ fn check_out( tree: &Tree, sparse_patterns: Vec, ) -> Result { - std::fs::create_dir(&wc_dir).map_err(DiffEditError::SetUpDirError)?; - std::fs::create_dir(&state_dir).map_err(DiffEditError::SetUpDirError)?; + std::fs::create_dir(&wc_dir).map_err(ExternalToolError::SetUpDirError)?; + std::fs::create_dir(&state_dir).map_err(ExternalToolError::SetUpDirError)?; let mut tree_state = TreeState::init(store, wc_dir, state_dir); tree_state.set_sparse_patterns(sparse_patterns)?; tree_state.check_out(tree)?; @@ -96,6 +133,113 @@ fn set_readonly_recursively(path: &Path) -> Result<(), std::io::Error> { } } +pub fn run_mergetool( + _ui: &mut Ui, + tree: &Tree, + repo_path: &RepoPath, +) -> Result { + let conflict_id = match tree.path_value(repo_path) { + Some(TreeValue::Conflict(id)) => id, + Some(_) => return Err(ConflictResolveError::NotAConflictError(repo_path.clone())), + None => return Err(ConflictResolveError::PathNotFoundError(repo_path.clone())), + }; + let conflict = tree.store().read_conflict(repo_path, &conflict_id)?; + let mut content = match extract_file_conflict_as_single_hunk(tree.store(), repo_path, &conflict) + { + Some(c) => c, + _ => { + let mut summary_bytes: Vec = vec![]; + describe_conflict(&conflict, &mut summary_bytes) + .expect("Writing to an in-memory buffer should never fail"); + return Err(ConflictResolveError::NotNormalFilesError( + repo_path.clone(), + String::from_utf8_lossy(summary_bytes.as_slice()).to_string(), + )); + } + }; + // The usual case is 1 `removes` and 2 `adds`. 0 `removes` means the file did + // not exist in the conflict base. Only 1 `adds` may exist for an + // edit-delete conflict. + if content.removes.len() > 1 || content.adds.len() > 2 { + return Err(ConflictResolveError::ConflictTooComplicatedError { + path: repo_path.clone(), + removes: content.removes.len(), + adds: content.adds.len(), + }); + }; + + let mut materialized_conflict: Vec = vec![]; + materialize_merge_result(&content, &mut materialized_conflict) + .expect("Writing to an in-memory buffer should never fail"); + let materialized_conflict = materialized_conflict; + + let files: HashMap<&str, _> = maplit::hashmap! { + "base" => content.removes.pop().unwrap_or_default(), + "right" => content.adds.pop().unwrap_or_default(), + "left" => content.adds.pop().unwrap_or_default(), + "output" => materialized_conflict.clone(), + }; + + let temp_dir = tempfile::Builder::new() + .prefix("jj-resolve-") + .tempdir() + .map_err(ExternalToolError::SetUpDirError)?; + let suffix = repo_path + .components() + .last() + .map(|filename| format!("_{}", filename.as_str())) + // The default case below should never actually trigger, but we support it just in case + // resolving the root path ever makes sense. + .unwrap_or_default(); + let paths: Result, ConflictResolveError> = files + .iter() + .map(|(role, contents)| { + let path = temp_dir.path().join(format!("{role}{suffix}")); + std::fs::write(&path, contents).map_err(ExternalToolError::SetUpDirError)?; + if *role != "output" { + // TODO: Should actually ignore the error here, or have a warning. + set_readonly_recursively(&path).map_err(ExternalToolError::SetUpDirError)?; + } + Ok((*role, path)) + }) + .collect(); + let paths = paths?; + + let progname = "vimdiff"; + let exit_status = Command::new(progname) + .args(["-f", "-d"]) + .arg(paths.get("output").unwrap()) + .arg("-M") + .args(["left", "base", "right"].map(|n| paths.get(n).unwrap())) + .args(["-c", "wincmd J", "-c", "setl modifiable write"]) + .status() + .map_err(|e| ExternalToolError::FailedToExecute { + tool_binary: progname.to_string(), + source: e, + })?; + if !exit_status.success() { + return Err(ConflictResolveError::from(ExternalToolError::ToolAborted)); + } + + let output_file_contents: Vec = std::fs::read(paths.get("output").unwrap())?; + if output_file_contents.is_empty() || output_file_contents == materialized_conflict { + return Err(ConflictResolveError::EmptyOrUnchanged); + } + // TODO: parse any remaining conflicts (done in followup commit) + let new_file_id = tree + .store() + .write_file(repo_path, &mut File::open(paths.get("output").unwrap())?)?; + let mut tree_builder = tree.store().tree_builder(tree.id().clone()); + tree_builder.set( + repo_path.clone(), + TreeValue::File { + id: new_file_id, + executable: false, + }, + ); + Ok(tree_builder.write_tree()) +} + pub fn edit_diff( ui: &mut Ui, left_tree: &Tree, @@ -114,7 +258,7 @@ pub fn edit_diff( let temp_dir = tempfile::Builder::new() .prefix("jj-diff-edit-") .tempdir() - .map_err(DiffEditError::SetUpDirError)?; + .map_err(ExternalToolError::SetUpDirError)?; let left_wc_dir = temp_dir.path().join("left"); let left_state_dir = temp_dir.path().join("left_state"); let right_wc_dir = temp_dir.path().join("right"); @@ -126,7 +270,7 @@ pub fn edit_diff( left_tree, changed_files.clone(), )?; - set_readonly_recursively(&left_wc_dir).map_err(DiffEditError::SetUpDirError)?; + set_readonly_recursively(&left_wc_dir).map_err(ExternalToolError::SetUpDirError)?; let mut right_tree_state = check_out( store.clone(), right_wc_dir.clone(), @@ -139,9 +283,12 @@ pub fn edit_diff( // not get any instructions. let add_instructions = !instructions.is_empty() && !instructions_path.exists(); if add_instructions { - let mut file = File::create(&instructions_path).map_err(DiffEditError::SetUpDirError)?; + // TODO: This can be replaced with std::fs::write. Is this used in other places + // as well? + let mut file = + File::create(&instructions_path).map_err(ExternalToolError::SetUpDirError)?; file.write_all(instructions.as_bytes()) - .map_err(DiffEditError::SetUpDirError)?; + .map_err(ExternalToolError::SetUpDirError)?; } // TODO: Make this configuration have a table of possible editors and detect the @@ -154,23 +301,23 @@ pub fn edit_diff( "Using default editor '{}'; you can change this by setting ui.diff-editor\n", default_editor )) - .map_err(DiffEditError::IoError)?; + .map_err(ExternalToolError::IoError)?; default_editor } }; - let editor = get_tool(ui.settings(), &editor_name)?; + let editor = get_tool(ui.settings(), &editor_name).map_err(ExternalToolError::ConfigError)?; // Start a diff editor on the two directories. let exit_status = Command::new(&editor.program) .args(&editor.edit_args) .arg(&left_wc_dir) .arg(&right_wc_dir) .status() - .map_err(|e| DiffEditError::ExecuteEditorError { - editor_binary: editor.program, + .map_err(|e| ExternalToolError::FailedToExecute { + tool_binary: editor.program.clone(), source: e, })?; if !exit_status.success() { - return Err(DiffEditError::DifftoolAborted); + return Err(DiffEditError::from(ExternalToolError::ToolAborted)); } if add_instructions { std::fs::remove_file(instructions_path).ok(); diff --git a/tests/test_restore_command.rs b/tests/test_restore_command.rs index 2c4e69f3c..04f186bb2 100644 --- a/tests/test_restore_command.rs +++ b/tests/test_restore_command.rs @@ -133,7 +133,7 @@ fn test_restore_interactive() { std::fs::write(&edit_script, "rm file2\0fail").unwrap(); let stderr = test_env.jj_cmd_failure(&repo_path, &["restore", "-i"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to edit diff: The diff tool exited with a non-zero code + Error: Failed to edit diff: Tool exited with a non-zero code. "###); let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]); insta::assert_snapshot!(stdout, @r###" diff --git a/tests/test_touchup_command.rs b/tests/test_touchup_command.rs index 03c11ddbc..8c488bafa 100644 --- a/tests/test_touchup_command.rs +++ b/tests/test_touchup_command.rs @@ -51,7 +51,7 @@ fn test_touchup() { std::fs::write(&edit_script, "rm file2\0fail").unwrap(); let stderr = test_env.jj_cmd_failure(&repo_path, &["touchup"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to edit diff: The diff tool exited with a non-zero code + Error: Failed to edit diff: Tool exited with a non-zero code. "###); let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]); insta::assert_snapshot!(stdout, @r###"