run_merge_tool, edit_diff: Print command args with -v instead of errors

src/merge_tools.rs

@@ -43,21 +43,22 @@ pub enum ExternalToolError {
     ConfigError(#[from] ConfigError),
     #[error("Error setting up temporary directory: {0:?}")]
     SetUpDirError(#[source] std::io::Error),
-    #[error("Error executing '{tool_binary}' with args {args:?}: {source}")]
+    // TODO: Remove the "(run with --verbose to see the exact invocation)"
+    // from this and other errors. Print it as a hint but only if --verbose is *not* set.
+    #[error(
+        "Error executing '{tool_binary}' (run with --verbose to see the exact invocation). \
+         {source}"
+    )]
     FailedToExecute {
         tool_binary: String,
-        args: Vec<String>,
         #[source]
         source: std::io::Error,
     },
     #[error(
-        "Tool exited with a non-zero code.\n Details: tool '{tool_binary}' was \
-         executed with args {args:?}. Exit code: {}.",
+        "Tool exited with a non-zero code (run with --verbose to see the exact invocation). Exit code: {}.",
          exit_status.code().map(|c| c.to_string()).unwrap_or_else(|| "<unknown>".to_string())
     )]
     ToolAborted {
-        tool_binary: String,
-        args: Vec<String>,
         exit_status: std::process::ExitStatus,
     },
     #[error("I/O error: {0:?}")]
@@ -101,7 +102,10 @@ pub enum ConflictResolveError {
         removes: usize,
         adds: usize,
     },
-    #[error("The output file is either unchanged or empty after the editor quit.")]
+    #[error(
+        "The output file is either unchanged or empty after the editor quit (run with --verbose \
+         to see the exact invocation)."
+    )]
     EmptyOrUnchanged,
     #[error("Backend error: {0:?}")]
     BackendError(#[from] jujutsu_lib::backend::BackendError),
@@ -227,22 +231,17 @@ pub fn run_mergetool(
         .try_collect()?;
 
     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)
+    let mut cmd = Command::new(&editor.program);
+    cmd.args(args);
+    tracing::debug!(?cmd, "Invoking the external merge tool:");
+    let exit_status = cmd
         .status()
         .map_err(|e| ExternalToolError::FailedToExecute {
             tool_binary: editor.program.clone(),
-            args: args_str.clone(),
             source: e,
         })?;
     if !exit_status.success() {
         return Err(ConflictResolveError::from(ExternalToolError::ToolAborted {
-            tool_binary: editor.program,
-            args: args_str,
             exit_status,
         }));
     }
@@ -351,24 +350,20 @@ pub fn edit_diff(
     }
 
     let editor = get_diff_editor_from_settings(ui, settings)?;
-    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)
+    let mut cmd = Command::new(&editor.program);
+    cmd.args(&editor.edit_args)
         .arg(&left_wc_dir)
-        .arg(&right_wc_dir)
+        .arg(&right_wc_dir);
+    tracing::debug!(?cmd, "Invoking the external diff editor:");
+    let exit_status = cmd
         .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 {
-            tool_binary: editor.program,
-            args: args_str,
             exit_status,
         }));
     }

tests/test_diffedit_command.rs

@@ -12,8 +12,6 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use regex::Regex;
-
 use crate::common::TestEnvironment;
 
 pub mod common;
@@ -52,10 +50,8 @@ fn test_diffedit() {
 
     // 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, &["diffedit"]);
-    insta::assert_snapshot!(Regex::new(r"Details: [^\n]+").unwrap().replace(&stderr, "Details: <OS-Dependent>"), @r###"
-    Error: Failed to edit diff: Tool exited with a non-zero code.
-     Details: <OS-Dependent>
+    insta::assert_snapshot!(&test_env.jj_cmd_failure(&repo_path, &["diffedit"]), @r###"
+    Error: Failed to edit diff: Tool exited with a non-zero code (run with --verbose to see the exact invocation). Exit code: 1.
     "###);
     let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]);
     insta::assert_snapshot!(stdout, @r###"
@@ -202,10 +198,8 @@ fn test_diffedit_old_restore_interactive_tests() {
 
     // 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, &["diffedit", "--from", "@-"]);
-    insta::assert_snapshot!(Regex::new(r"Details: [^\n]+").unwrap().replace(&stderr, "Details: <OS-Dependent>"), @r###"
-    Error: Failed to edit diff: Tool exited with a non-zero code.
-     Details: <OS-Dependent>
+    insta::assert_snapshot!(&test_env.jj_cmd_failure(&repo_path, &["diffedit", "--from", "@-"]), @r###"
+    Error: Failed to edit diff: Tool exited with a non-zero code (run with --verbose to see the exact invocation). Exit code: 1.
     "###);
     let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]);
     insta::assert_snapshot!(stdout, @r###"

tests/test_resolve_command.rs

@@ -275,17 +275,15 @@ fn check_resolve_produces_input_file(
     std::fs::write(editor_script, format!("expect\n{expected_content}")).unwrap();
 
     let merge_arg_config = format!(r#"merge-tools.fake-editor.merge-args = ["${role}"]"#);
-    let error =
-        test_env.jj_cmd_failure(repo_path, &["resolve", "--config-toml", &merge_arg_config]);
     // This error means that fake-editor exited successfully but did not modify the
     // output file.
     // We cannot use `insta::assert_snapshot!` here after insta 1.22 due to
     // https://github.com/mitsuhiko/insta/commit/745b45b. Hopefully, this will again become possible
     // in the future. See also https://github.com/mitsuhiko/insta/issues/313.
     assert_eq!(
-        error,
+        &test_env.jj_cmd_failure(repo_path, &["resolve", "--config-toml", &merge_arg_config]),
         "Error: Failed to use external tool to resolve: The output file is either unchanged or \
-         empty after the editor quit.\n"
+         empty after the editor quit (run with --verbose to see the exact invocation).\n"
     );
 }