cli: don't ignore 'diff --tool=:builtin', report error

Before, --tool=:builtin argument was ignored and the tool was loaded from
"ui.diff.tool" option. Since there is no single builtin diff format, :builtin
doesn't make sense here. Maybe we can translate ":<format>" to the internal
diff format instead, but that will also mean "ui.diff.tool" and ".format" can
be merged.

This partially reverts 409356fa5b "merge_tools: enable `:builtin` as default
diff/merge editor."

cli/src/diff_util.rs

@@ -40,7 +40,7 @@ use unicode_width::UnicodeWidthStr as _;
 use crate::cli_util::{CommandError, WorkspaceCommandHelper};
 use crate::config::CommandNameAndArgs;
 use crate::formatter::Formatter;
-use crate::merge_tools::{self, ExternalMergeTool, MergeTool};
+use crate::merge_tools::{self, ExternalMergeTool};
 use crate::text_util;
 use crate::ui::Ui;
 
@@ -129,14 +129,9 @@ fn diff_formats_from_args(
     .filter_map(|(arg, format)| arg.then_some(format))
     .collect_vec();
     if let Some(name) = &args.tool {
-        let tool = merge_tools::get_tool_config(settings, name)?
+        let tool = merge_tools::get_external_tool_config(settings, name)?
-            .unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_program(name)));
+            .unwrap_or_else(|| ExternalMergeTool::with_program(name));
-        match tool {
+        formats.push(DiffFormat::Tool(Box::new(tool)));
-            MergeTool::Builtin => {}
-            MergeTool::External(tool) => {
-                formats.push(DiffFormat::Tool(Box::new(tool)));
-            }
-        }
     }
     Ok(formats)
 }
@@ -146,17 +141,12 @@ fn default_diff_format(settings: &UserSettings) -> Result<DiffFormat, config::Co
     if let Some(args) = config.get("ui.diff.tool").optional()? {
         // External "tool" overrides the internal "format" option.
         let tool = if let CommandNameAndArgs::String(name) = &args {
-            merge_tools::get_tool_config(settings, name)?
+            merge_tools::get_external_tool_config(settings, name)?
         } else {
             None
         }
-        .unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_diff_args(&args)));
+        .unwrap_or_else(|| ExternalMergeTool::with_diff_args(&args));
-        match tool {
+        return Ok(DiffFormat::Tool(Box::new(tool)));
-            MergeTool::Builtin => {}
-            MergeTool::External(tool) => {
-                return Ok(DiffFormat::Tool(Box::new(tool)));
-            }
-        }
     }
     let name = if let Some(name) = config.get_string("ui.diff.format").optional()? {
         name

cli/src/merge_tools/mod.rs

@@ -185,10 +185,7 @@ fn editor_args_from_settings(
 
 /// Resolves builtin merge tool name or loads external tool options from
 /// `[merge-tools.<name>]`.
-pub fn get_tool_config(
+fn get_tool_config(settings: &UserSettings, name: &str) -> Result<Option<MergeTool>, ConfigError> {
-    settings: &UserSettings,
-    name: &str,
-) -> Result<Option<MergeTool>, ConfigError> {
     if name == BUILTIN_EDITOR_NAME {
         Ok(Some(MergeTool::Builtin))
     } else {

cli/tests/test_diff_command.rs

@@ -14,7 +14,7 @@
 
 use itertools::Itertools;
 
-use crate::common::{escaped_fake_diff_editor_path, TestEnvironment};
+use crate::common::{escaped_fake_diff_editor_path, strip_last_line, TestEnvironment};
 
 #[test]
 fn test_diff_basic() {
@@ -753,6 +753,14 @@ fn test_diff_external_tool() {
     insta::assert_snapshot!(stderr, @r###"
     Tool exited with a non-zero code (run with --debug to see the exact invocation). Exit code: 1.
     "###);
+
+    // --tool=:builtin shouldn't be ignored
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["diff", "--tool=:builtin"]);
+    insta::assert_snapshot!(strip_last_line(&stderr), @r###"
+    Error: Failed to generate diff
+    Caused by:
+    1: Error executing ':builtin' (run with --debug to see the exact invocation)
+    "###);
 }
 
 #[cfg(unix)]