From 1eccd3430fb1d0fe2b7b2d748dc6cebc702234d2 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Thu, 27 Oct 2022 20:30:44 -0700 Subject: [PATCH] Make the choice and arguments of mergetool configurable Also makes the merge/diff tool binary default to the tool's name (thus changing the diff editor behavior slightly) --- src/diff_edit.rs | 146 ++++++++++++++++++++++++++-------- tests/test_restore_command.rs | 5 +- tests/test_touchup_command.rs | 5 +- 3 files changed, 121 insertions(+), 35 deletions(-) diff --git a/src/diff_edit.rs b/src/diff_edit.rs index c20a5aeb4..d30403de3 100644 --- a/src/diff_edit.rs +++ b/src/diff_edit.rs @@ -42,14 +42,23 @@ pub enum ExternalToolError { ConfigError(#[from] ConfigError), #[error("Error setting up temporary directory: {0:?}")] SetUpDirError(#[source] std::io::Error), - #[error("Error executing '{tool_binary}': {source}")] + #[error("Error executing '{tool_binary}' with args {args:?}: {source}")] FailedToExecute { tool_binary: String, + args: Vec, #[source] source: std::io::Error, }, - #[error("Tool exited with a non-zero code.")] - ToolAborted, + #[error( + "Tool exited with a non-zero code.\n Details: tool '{tool_binary}' was \ + executed with args {args:?}. Exit code: {}.", + exit_status.code().map(|c| c.to_string()).unwrap_or_else(|| "".to_string()) + )] + ToolAborted { + tool_binary: String, + args: Vec, + exit_status: std::process::ExitStatus, + }, #[error("I/O error: {0:?}")] IoError(#[from] std::io::Error), } @@ -134,7 +143,7 @@ fn set_readonly_recursively(path: &Path) -> Result<(), std::io::Error> { } pub fn run_mergetool( - _ui: &mut Ui, + ui: &mut Ui, tree: &Tree, repo_path: &RepoPath, ) -> Result { @@ -205,20 +214,27 @@ pub fn run_mergetool( .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"]) + let editor = get_merge_tool_from_settings(ui)?; + // TODO: Error if `editor.merge_args` is empty + let args = interpolate_mergetool_filename_patterns(&editor.merge_args, &paths); + let args_str = args + .iter() + .map(|p| p.to_string_lossy().into_owned()) + .collect_vec(); + let exit_status = Command::new(&editor.program) + .args(args) .status() .map_err(|e| ExternalToolError::FailedToExecute { - tool_binary: progname.to_string(), + tool_binary: editor.program.clone(), + args: args_str.clone(), source: e, })?; if !exit_status.success() { - return Err(ConflictResolveError::from(ExternalToolError::ToolAborted)); + return Err(ConflictResolveError::from(ExternalToolError::ToolAborted { + tool_binary: editor.program, + args: args_str, + exit_status, + })); } let output_file_contents: Vec = std::fs::read(paths.get("output").unwrap())?; @@ -240,6 +256,27 @@ pub fn run_mergetool( Ok(tree_builder.write_tree()) } +fn interpolate_mergetool_filename_patterns>( + merge_args: &[String], + paths: &HashMap<&str, PathBuf>, +) -> Vec +where + Vec: FromIterator, +{ + merge_args + .iter() + .map(|arg| { + // TODO: Match all instances of `\$\w+` pattern and replace them + // so that portions of args can be replaced, and so that file paths + // that include the '$' character are processed correctly. + arg.strip_prefix('$') + .and_then(|p| paths.get(p)) + .and_then(|p| From::from(p.clone())) + .unwrap_or_else(|| From::from(arg.clone())) + }) + .collect() +} + pub fn edit_diff( ui: &mut Ui, left_tree: &Tree, @@ -291,21 +328,10 @@ pub fn edit_diff( .map_err(ExternalToolError::SetUpDirError)?; } - // TODO: Make this configuration have a table of possible editors and detect the - // best one here. - let editor_name = match ui.settings().config().get_string("ui.diff-editor") { - Ok(editor_binary) => editor_binary, - Err(_) => { - let default_editor = "meld".to_string(); - ui.write_hint(format!( - "Using default editor '{}'; you can change this by setting ui.diff-editor\n", - default_editor - )) - .map_err(ExternalToolError::IoError)?; - default_editor - } - }; - let editor = get_tool(ui.settings(), &editor_name).map_err(ExternalToolError::ConfigError)?; + let editor = get_diff_editor_from_settings(ui)?; + let mut args_str = editor.edit_args.clone(); + args_str.push(left_wc_dir.display().to_string()); + args_str.push(right_wc_dir.display().to_string()); // Start a diff editor on the two directories. let exit_status = Command::new(&editor.program) .args(&editor.edit_args) @@ -314,10 +340,15 @@ pub fn edit_diff( .status() .map_err(|e| ExternalToolError::FailedToExecute { tool_binary: editor.program.clone(), + args: args_str.clone(), source: e, })?; if !exit_status.success() { - return Err(DiffEditError::from(ExternalToolError::ToolAborted)); + return Err(DiffEditError::from(ExternalToolError::ToolAborted { + tool_binary: editor.program, + args: args_str, + exit_status, + })); } if add_instructions { std::fs::remove_file(instructions_path).ok(); @@ -331,11 +362,20 @@ pub fn edit_diff( #[derive(Clone, Debug, serde::Deserialize)] #[serde(rename_all = "kebab-case")] struct MergeTool { - /// Program to execute. + /// Program to execute. Must be defined; defaults to the tool name + /// if not specified in the config. + #[serde(default)] pub program: String, /// Arguments to pass to the program when editing diffs. #[serde(default)] pub edit_args: Vec, + /// Arguments to pass to the program when resolving 3-way conflicts. + /// `$left`, `$right`, `$base`, and `$output` are replaced with + /// paths to the corresponding files. + /// TODO: Currently, the entire argument has to match one of these 4 + /// strings to be substituted. + #[serde(default)] + pub merge_args: Vec, } impl MergeTool { @@ -343,13 +383,14 @@ impl MergeTool { MergeTool { program: program.to_owned(), edit_args: vec![], + merge_args: vec![], } } } /// Loads merge tool options from `[merge-tools.]`. The given name is used /// as an executable name if no configuration found for that name. -fn get_tool(settings: &UserSettings, name: &str) -> Result { +fn get_tool_config(settings: &UserSettings, name: &str) -> Result { const TABLE_KEY: &str = "merge-tools"; let tools_table = match settings.config().get_table(TABLE_KEY) { Ok(table) => table, @@ -357,11 +398,50 @@ fn get_tool(settings: &UserSettings, name: &str) -> Result return Err(err), }; if let Some(v) = tools_table.get(name) { - v.clone() + let mut result: MergeTool = v + .clone() .try_deserialize() // add config key, deserialize error is otherwise unclear - .map_err(|e| ConfigError::Message(format!("{TABLE_KEY}.{name}: {e}"))) + .map_err(|e| ConfigError::Message(format!("{TABLE_KEY}.{name}: {e}")))?; + + if result.program.is_empty() { + result.program.clone_from(&name.to_string()); + }; + Ok(result) } else { Ok(MergeTool::with_program(name)) } } + +fn get_diff_editor_from_settings(ui: &mut Ui) -> Result { + _get_editor_from_settings_helper(ui, "diff") +} + +fn get_merge_tool_from_settings(ui: &mut Ui) -> Result { + _get_editor_from_settings_helper(ui, "merge") +} + +/// Finds the appropriate tool for diff editing or merges +fn _get_editor_from_settings_helper( + ui: &mut Ui, + diff_or_merge: &str, +) -> Result { + // TODO: Make this configuration have a table of possible editors and detect the + // best one here. + let editor_name = match ui + .settings() + .config() + .get_string(&format!("ui.{diff_or_merge}-editor")) + { + Ok(editor_binary) => editor_binary, + Err(_) => { + let default_editor = "meld".to_string(); + ui.write_hint(format!( + "Using default editor '{default_editor}'; you can change this by setting \ + ui.{diff_or_merge}-editor\n" + ))?; + default_editor + } + }; + Ok(get_tool_config(ui.settings(), &editor_name)?) +} diff --git a/tests/test_restore_command.rs b/tests/test_restore_command.rs index 04f186bb2..be3378e88 100644 --- a/tests/test_restore_command.rs +++ b/tests/test_restore_command.rs @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +use regex::Regex; + use crate::common::TestEnvironment; pub mod common; @@ -132,8 +134,9 @@ fn test_restore_interactive() { // Nothing happens if the diff-editor exits with an error 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###" + insta::assert_snapshot!(Regex::new(r"Details: [^\n]+").unwrap().replace(&stderr, "Details: "), @r###" Error: Failed to edit diff: Tool exited with a non-zero code. + Details: "###); 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 8c488bafa..ad63f2913 100644 --- a/tests/test_touchup_command.rs +++ b/tests/test_touchup_command.rs @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +use regex::Regex; + use crate::common::TestEnvironment; pub mod common; @@ -50,8 +52,9 @@ fn test_touchup() { // Nothing happens if the diff-editor exits with an error std::fs::write(&edit_script, "rm file2\0fail").unwrap(); let stderr = test_env.jj_cmd_failure(&repo_path, &["touchup"]); - insta::assert_snapshot!(stderr, @r###" + insta::assert_snapshot!(Regex::new(r"Details: [^\n]+").unwrap().replace(&stderr, "Details: "), @r###" Error: Failed to edit diff: Tool exited with a non-zero code. + Details: "###); let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]); insta::assert_snapshot!(stdout, @r###"