cli merge tools: allow setting diff-args=[] to disable diff formatting with a tool

Not setting `diff-args` is equivalent to `diff-args=["$left",
"$right"]`, which I also documented here.

I couldn't decide whether the new error should be part of
`DiffRenderError`, `DiffGenerateError`, or `MergeToolError`. Since the
treatment of diff formatters is already very different from other merge
tools, I just made it a CommandError for now.

CHANGELOG.md

@@ -54,7 +54,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   [`Operation`](docs/templates.md#operation-type) type in the templating
   language.
 
-* Merge tools config can now explicitly forbid using them as diff editors.
+* Merge tools config can now explicitly forbid using them as diff editors or
+  diff formatters.
 
 ### Fixed bugs
 

cli/src/diff_util.rs

@@ -270,6 +270,21 @@ pub fn all_builtin_diff_format_names() -> Vec<String> {
         .collect()
 }
 
+fn diff_formatter_tool(
+    settings: &UserSettings,
+    name: &str,
+) -> Result<Option<ExternalMergeTool>, CommandError> {
+    let maybe_tool = merge_tools::get_external_tool_config(settings, name)?;
+    if let Some(tool) = &maybe_tool {
+        if tool.diff_args.is_empty() {
+            return Err(cli_error(format!(
+                "The tool `{name}` cannot be used for diff formatting"
+            )));
+        };
+    };
+    Ok(maybe_tool)
+}
+
 /// Returns a list of requested diff formats, which will never be empty.
 pub fn diff_formats_for(
     settings: &UserSettings,
@@ -334,9 +349,9 @@ fn diff_formats_from_args(
                 long_format = Some(format);
             }
         } else {
-            let tool = merge_tools::get_external_tool_config(settings, name)?
-                .unwrap_or_else(|| ExternalMergeTool::with_program(name));
             ensure_new(long_kind)?;
+            let tool = diff_formatter_tool(settings, name)?
+                .unwrap_or_else(|| ExternalMergeTool::with_program(name));
             long_format = Some(DiffFormat::Tool(Box::new(tool)));
         }
     }
@@ -346,19 +361,19 @@ fn diff_formats_from_args(
 fn default_diff_format(
     settings: &UserSettings,
     args: &DiffFormatArgs,
-) -> Result<DiffFormat, ConfigGetError> {
+) -> Result<DiffFormat, CommandError> {
     let tool_args: CommandNameAndArgs = settings.get("ui.diff-formatter")?;
     if let Some(name) = tool_args.as_str().and_then(|s| s.strip_prefix(':')) {
-        BuiltinFormatKind::from_name(name)
+        Ok(BuiltinFormatKind::from_name(name)
             .map_err(|err| ConfigGetError::Type {
                 name: "ui.diff-formatter".to_owned(),
                 error: err.into(),
                 source_path: None,
             })?
-            .to_format(settings, args)
+            .to_format(settings, args)?)
     } else {
         let tool = if let Some(name) = tool_args.as_str() {
-            merge_tools::get_external_tool_config(settings, name)?
+            diff_formatter_tool(settings, name)?
         } else {
             None
         }

cli/tests/test_diff_command.rs

@@ -3041,9 +3041,12 @@ fn test_diff_external_tool() {
     // nonzero exit codes should print a warning
     std::fs::write(&edit_script, "fail").unwrap();
     let output = work_dir.run_jj(["diff", "--config=ui.diff-formatter=fake-diff-editor"]);
-    let mut insta_settings = insta::Settings::clone_current();
-    insta_settings.add_filter("exit (status|code)", "<exit status>");
-    insta_settings.bind(|| {
+    let insta_portable_exit_status = {
+        let mut settings = insta::Settings::clone_current();
+        settings.add_filter("exit (status|code)", "<exit status>");
+        settings
+    };
+    insta_portable_exit_status.bind(|| {
         insta::assert_snapshot!(output, @r"
         ------- stderr -------
         Warning: Tool exited with <exit status>: 1 (run with --debug to see the exact invocation)
@@ -3077,6 +3080,31 @@ fn test_diff_external_tool() {
     [EOF]
     ");
 
+    // diff with unset edit-args
+    insta::assert_snapshot!(work_dir.run_jj(["diff", "--tool=fake-diff-editor",
+        "--config=merge-tools.fake-diff-editor.edit-args=[]",
+    ]), @r"
+    file1
+    file2
+    --
+    file2
+    file3
+    [EOF]
+    ");
+
+    // diff with explicitly unset edit-args and diff-args
+    insta_portable_exit_status.bind(|| {
+        insta::assert_snapshot!(work_dir.run_jj(["diff", "--tool=fake-diff-editor",
+        "--config=merge-tools.fake-diff-editor.edit-args=[]",
+        "--config=merge-tools.fake-diff-editor.diff-args=[]",
+    ]), @r"
+        ------- stderr -------
+        Error: The tool `fake-diff-editor` cannot be used for diff formatting
+        [EOF]
+        [<exit status>: 2]
+        ");
+    });
+
     // diff with file patterns
     insta::assert_snapshot!(work_dir.run_jj(["diff", "--tool=fake-diff-editor", "file1"]), @r"
     file1

docs/config.md

@@ -374,6 +374,10 @@ diff-args = ["--color=always", "$left", "$right"]
 
 - If `diff-args` is not specified, `["$left", "$right"]` will be used by default.
 
+- If `diff-args = []`, `jj` will refuse to use this tool for diff formatting.
+  This is a way to explicitly state that a certain tool (e.g. `mergiraf`) does
+  not work for viewing diffs.
+
 By default `jj` will invoke external tools with a directory containing the left
 and right sides.  The `diff-invocation-mode` config can change this to file by file
 invocations as follows: