cli: add --tool=<name> option to diff/merge editing commands

I didn't add e2e tests to all commands, but the added tests should cover
diff_editor/diff_selector/merge_editor() calls.

Closes #2575

CHANGELOG.md

@@ -75,6 +75,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * `jj move --from/--to` can now be abbreviated to `jj move -f/-t`
 
+* `jj commit`/`diffedit`/`move`/`resolve`/`split`/`squash`/`unsquash` now accept
+  `--tool=<NAME>` option to override the default.
+   [#2575](https://github.com/martinvonz/jj/issues/2575)
+
 * Added completions for [Nushell](https://nushell.sh) to `jj util completion`
 
 * `jj branch list` now supports a `--tracked/-t` option which can be used to

cli/src/cli_util.rs

@@ -671,26 +671,50 @@ impl WorkspaceCommandHelper {
     }
 
     /// Loads diff editor from the settings.
-    // TODO: override settings by --tool= option (#2575)
-    pub fn diff_editor(&self, ui: &Ui) -> Result<DiffEditor, CommandError> {
+    ///
+    /// If the `tool_name` isn't specified, the default editor will be returned.
+    pub fn diff_editor(
+        &self,
+        ui: &Ui,
+        tool_name: Option<&str>,
+    ) -> Result<DiffEditor, CommandError> {
         let base_ignores = self.base_ignores()?;
-        Ok(DiffEditor::from_settings(ui, &self.settings, base_ignores)?)
+        if let Some(name) = tool_name {
+            Ok(DiffEditor::with_name(name, &self.settings, base_ignores)?)
+        } else {
+            Ok(DiffEditor::from_settings(ui, &self.settings, base_ignores)?)
+        }
     }
 
     /// Conditionally loads diff editor from the settings.
-    // TODO: override settings by --tool= option (#2575)
-    pub fn diff_selector(&self, ui: &Ui, interactive: bool) -> Result<DiffSelector, CommandError> {
-        if interactive {
-            Ok(DiffSelector::Interactive(self.diff_editor(ui)?))
+    ///
+    /// If the `tool_name` is specified, interactive session is implied.
+    pub fn diff_selector(
+        &self,
+        ui: &Ui,
+        tool_name: Option<&str>,
+        force_interactive: bool,
+    ) -> Result<DiffSelector, CommandError> {
+        if tool_name.is_some() || force_interactive {
+            Ok(DiffSelector::Interactive(self.diff_editor(ui, tool_name)?))
         } else {
             Ok(DiffSelector::NonInteractive)
         }
     }
 
     /// Loads 3-way merge editor from the settings.
-    // TODO: override settings by --tool= option (#2575)
-    pub fn merge_editor(&self, ui: &Ui) -> Result<MergeEditor, MergeToolConfigError> {
-        MergeEditor::from_settings(ui, &self.settings)
+    ///
+    /// If the `tool_name` isn't specified, the default editor will be returned.
+    pub fn merge_editor(
+        &self,
+        ui: &Ui,
+        tool_name: Option<&str>,
+    ) -> Result<MergeEditor, MergeToolConfigError> {
+        if let Some(name) = tool_name {
+            MergeEditor::with_name(name, &self.settings)
+        } else {
+            MergeEditor::from_settings(ui, &self.settings)
+        }
     }
 
     pub fn resolve_single_op(&self, op_str: &str) -> Result<Operation, OpsetEvaluationError> {

cli/src/commands/commit.rs

@@ -29,6 +29,9 @@ pub(crate) struct CommitArgs {
     /// Interactively choose which changes to include in the first commit
     #[arg(short, long)]
     interactive: bool,
+    /// Specify diff editor to be used (implies --interactive)
+    #[arg(long, value_name = "NAME")]
+    pub tool: Option<String>,
     /// The change description to use (don't open editor)
     #[arg(long = "message", short, value_name = "MESSAGE")]
     message_paragraphs: Vec<String>,
@@ -50,7 +53,8 @@ pub(crate) fn cmd_commit(
         .ok_or_else(|| user_error("This command requires a working copy"))?;
     let commit = workspace_command.repo().store().get_commit(commit_id)?;
     let matcher = workspace_command.matcher_from_values(&args.paths)?;
-    let diff_selector = workspace_command.diff_selector(ui, args.interactive)?;
+    let diff_selector =
+        workspace_command.diff_selector(ui, args.tool.as_deref(), args.interactive)?;
     let mut tx = workspace_command.start_transaction();
     let base_tree = merge_commit_trees(tx.repo(), &commit.parents())?;
     let instructions = format!(

cli/src/commands/diffedit.rs

@@ -51,6 +51,9 @@ pub(crate) struct DiffeditArgs {
     /// Edit changes in this revision. Defaults to @ if --from is specified.
     #[arg(long, conflicts_with = "revision")]
     to: Option<RevisionArg>,
+    /// Specify diff editor to be used
+    #[arg(long, value_name = "NAME")]
+    pub tool: Option<String>,
 }
 
 #[instrument(skip_all)]
@@ -78,7 +81,7 @@ pub(crate) fn cmd_diffedit(
     };
     workspace_command.check_rewritable([&target_commit])?;
 
-    let diff_editor = workspace_command.diff_editor(ui)?;
+    let diff_editor = workspace_command.diff_editor(ui, args.tool.as_deref())?;
     let mut tx = workspace_command.start_transaction();
     let instructions = format!(
         "\

cli/src/commands/move.rs

@@ -50,8 +50,11 @@ pub(crate) struct MoveArgs {
     /// Interactively choose which parts to move
     #[arg(long, short)]
     interactive: bool,
+    /// Specify diff editor to be used (implies --interactive)
+    #[arg(long, value_name = "NAME")]
+    pub tool: Option<String>,
     /// Move only changes to these paths (instead of all paths)
-    #[arg(conflicts_with = "interactive", value_hint = clap::ValueHint::AnyPath)]
+    #[arg(conflicts_with_all = ["interactive", "tool"], value_hint = clap::ValueHint::AnyPath)]
     paths: Vec<String>,
 }
 
@@ -70,7 +73,8 @@ pub(crate) fn cmd_move(
     }
     workspace_command.check_rewritable([&source, &destination])?;
     let matcher = workspace_command.matcher_from_values(&args.paths)?;
-    let diff_selector = workspace_command.diff_selector(ui, args.interactive)?;
+    let diff_selector =
+        workspace_command.diff_selector(ui, args.tool.as_deref(), args.interactive)?;
     let mut tx = workspace_command.start_transaction();
     let parent_tree = merge_commit_trees(tx.repo(), &source.parents())?;
     let source_tree = source.tree()?;

cli/src/commands/resolve.rs

@@ -55,6 +55,9 @@ pub(crate) struct ResolveArgs {
     /// conflict
     #[arg(long, short, conflicts_with = "list")]
     quiet: bool,
+    /// Specify 3-way merge tool to be used
+    #[arg(long, conflicts_with = "list", value_name = "NAME")]
+    pub tool: Option<String>,
     /// Restrict to these paths when searching for a conflict to resolve. We
     /// will attempt to resolve the first conflict we can find. You can use
     /// the `--list` argument to find paths to use here.
@@ -97,7 +100,7 @@ pub(crate) fn cmd_resolve(
 
     let (repo_path, _) = conflicts.first().unwrap();
     workspace_command.check_rewritable([&commit])?;
-    let merge_editor = workspace_command.merge_editor(ui)?;
+    let merge_editor = workspace_command.merge_editor(ui, args.tool.as_deref())?;
     writeln!(
         ui.stderr(),
         "Resolving conflicts in: {}",

cli/src/commands/split.rs

@@ -41,6 +41,9 @@ pub(crate) struct SplitArgs {
     /// paths are provided.
     #[arg(long, short)]
     interactive: bool,
+    /// Specify diff editor to be used (implies --interactive)
+    #[arg(long, value_name = "NAME")]
+    pub tool: Option<String>,
     /// The revision to split
     #[arg(long, short, default_value = "@")]
     revision: RevisionArg,
@@ -59,8 +62,11 @@ pub(crate) fn cmd_split(
     let commit = workspace_command.resolve_single_rev(&args.revision)?;
     workspace_command.check_rewritable([&commit])?;
     let matcher = workspace_command.matcher_from_values(&args.paths)?;
-    let diff_selector =
-        workspace_command.diff_selector(ui, args.interactive || args.paths.is_empty())?;
+    let diff_selector = workspace_command.diff_selector(
+        ui,
+        args.tool.as_deref(),
+        args.interactive || args.paths.is_empty(),
+    )?;
     let mut tx = workspace_command.start_transaction();
     let end_tree = commit.tree()?;
     let base_tree = merge_commit_trees(tx.repo(), &commit.parents())?;

cli/src/commands/squash.rs

@@ -46,8 +46,11 @@ pub(crate) struct SquashArgs {
     /// Interactively choose which parts to squash
     #[arg(long, short)]
     interactive: bool,
+    /// Specify diff editor to be used (implies --interactive)
+    #[arg(long, value_name = "NAME")]
+    pub tool: Option<String>,
     /// Move only changes to these paths (instead of all paths)
-    #[arg(conflicts_with = "interactive", value_hint = clap::ValueHint::AnyPath)]
+    #[arg(conflicts_with_all = ["interactive", "tool"], value_hint = clap::ValueHint::AnyPath)]
     paths: Vec<String>,
 }
 
@@ -67,7 +70,8 @@ pub(crate) fn cmd_squash(
     let parent = &parents[0];
     workspace_command.check_rewritable(&parents[..1])?;
     let matcher = workspace_command.matcher_from_values(&args.paths)?;
-    let diff_selector = workspace_command.diff_selector(ui, args.interactive)?;
+    let diff_selector =
+        workspace_command.diff_selector(ui, args.tool.as_deref(), args.interactive)?;
     let mut tx = workspace_command.start_transaction();
     let instructions = format!(
         "\

cli/src/commands/unsquash.rs

@@ -45,6 +45,9 @@ pub(crate) struct UnsquashArgs {
     // the default.
     #[arg(long, short)]
     interactive: bool,
+    /// Specify diff editor to be used (implies --interactive)
+    #[arg(long, value_name = "NAME")]
+    pub tool: Option<String>,
 }
 
 #[instrument(skip_all)]
@@ -62,8 +65,8 @@ pub(crate) fn cmd_unsquash(
     }
     let parent = &parents[0];
     workspace_command.check_rewritable(&parents[..1])?;
-    let interactive_editor = if args.interactive {
-        Some(workspace_command.diff_editor(ui)?)
+    let interactive_editor = if args.tool.is_some() || args.interactive {
+        Some(workspace_command.diff_editor(ui, args.tool.as_deref())?)
     } else {
         None
     };

cli/tests/cli-reference@.md.snap

@@ -436,6 +436,7 @@ Update the description and create a new change on top
 
   Possible values: `true`, `false`
 
+* `--tool <NAME>` — Specify diff editor to be used (implies --interactive)
 * `-m`, `--message <MESSAGE>` — The change description to use (don't open editor)
 
 
@@ -677,6 +678,7 @@ See `jj restore` if you want to move entire files from one revision to another.
 * `-r`, `--revision <REVISION>` — The revision to touch up. Defaults to @ if neither --to nor --from are specified
 * `--from <FROM>` — Show changes from this revision. Defaults to @ if --to is specified
 * `--to <TO>` — Edit changes in this revision. Defaults to @ if --from is specified
+* `--tool <NAME>` — Specify diff editor to be used
 
 
 
@@ -1078,6 +1080,7 @@ If a working-copy commit gets abandoned, it will be given a new, empty commit. T
 
   Possible values: `true`, `false`
 
+* `--tool <NAME>` — Specify diff editor to be used (implies --interactive)
 
 
 
@@ -1514,6 +1517,7 @@ Note that conflicts can also be resolved without using this command. You may edi
 
   Possible values: `true`, `false`
 
+* `--tool <NAME>` — Specify 3-way merge tool to be used
 
 
 
@@ -1665,6 +1669,7 @@ If the change you split had a description, you will be asked to enter a change d
 
   Possible values: `true`, `false`
 
+* `--tool <NAME>` — Specify diff editor to be used (implies --interactive)
 * `-r`, `--revision <REVISION>` — The revision to split
 
   Default value: `@`
@@ -1697,6 +1702,7 @@ If a working-copy commit gets abandoned, it will be given a new, empty commit. T
 
   Possible values: `true`, `false`
 
+* `--tool <NAME>` — Specify diff editor to be used (implies --interactive)
 
 
 
@@ -1884,6 +1890,7 @@ If a working-copy commit gets abandoned, it will be given a new, empty commit. T
 
   Possible values: `true`, `false`
 
+* `--tool <NAME>` — Specify diff editor to be used (implies --interactive)
 
 
 

cli/tests/test_commit_command.rs

@@ -100,6 +100,27 @@ fn test_commit_interactive() {
 
     JJ: Lines starting with "JJ: " (like this one) will be removed.
     "###);
+
+    // Try again with --tool=<name>, which implies --interactive
+    test_env.jj_cmd_ok(&workspace_path, &["undo"]);
+    test_env.jj_cmd_ok(
+        &workspace_path,
+        &[
+            "commit",
+            "--config-toml=ui.diff-editor='false'",
+            "--tool=fake-diff-editor",
+        ],
+    );
+
+    insta::assert_snapshot!(
+        std::fs::read_to_string(test_env.env_root().join("editor")).unwrap(), @r###"
+    add files
+
+    JJ: This commit contains the following changes:
+    JJ:     A file1
+
+    JJ: Lines starting with "JJ: " (like this one) will be removed.
+    "###);
 }
 
 #[test]

cli/tests/test_diffedit_command.rs

@@ -63,6 +63,30 @@ fn test_diffedit() {
     M file2
     "###);
 
+    // Try again with --tool=<name>
+    std::fs::write(
+        &edit_script,
+        "files-before file1 file2\0files-after JJ-INSTRUCTIONS file2",
+    )
+    .unwrap();
+    let (stdout, stderr) = test_env.jj_cmd_ok(
+        &repo_path,
+        &[
+            "diffedit",
+            "--config-toml=ui.diff-editor='false'",
+            "--tool=fake-diff-editor",
+        ],
+    );
+    insta::assert_snapshot!(stdout, @"");
+    insta::assert_snapshot!(stderr, @r###"
+    Nothing changed.
+    "###);
+    let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]);
+    insta::assert_snapshot!(stdout, @r###"
+    D file1
+    M file2
+    "###);
+
     // Nothing happens if the diff-editor exits with an error
     std::fs::write(&edit_script, "rm file2\0fail").unwrap();
     insta::assert_snapshot!(&test_env.jj_cmd_failure(&repo_path, &["diffedit"]), @r###"
@@ -80,8 +104,8 @@ fn test_diffedit() {
     let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["diffedit"]);
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
-    Created kkmpptxz ae84b067 (no description set)
-    Working copy now at: kkmpptxz ae84b067 (no description set)
+    Created kkmpptxz cc387f43 (no description set)
+    Working copy now at: kkmpptxz cc387f43 (no description set)
     Parent commit      : rlvkpnrz 613028a4 (no description set)
     Added 0 files, modified 1 files, removed 0 files
     "###);
@@ -96,10 +120,10 @@ fn test_diffedit() {
     let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["diffedit", "-r", "@-"]);
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
-    Created rlvkpnrz 51a4f74c (no description set)
+    Created rlvkpnrz d842b979 (no description set)
     Rebased 1 descendant commits
-    Working copy now at: kkmpptxz 574af856 (no description set)
-    Parent commit      : rlvkpnrz 51a4f74c (no description set)
+    Working copy now at: kkmpptxz bc2b2dd6 (no description set)
+    Parent commit      : rlvkpnrz d842b979 (no description set)
     Added 0 files, modified 1 files, removed 0 files
     "###);
     let contents = String::from_utf8(std::fs::read(repo_path.join("file3")).unwrap()).unwrap();
@@ -117,8 +141,8 @@ fn test_diffedit() {
     let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["diffedit", "--from", "@--"]);
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
-    Created kkmpptxz cd638328 (no description set)
-    Working copy now at: kkmpptxz cd638328 (no description set)
+    Created kkmpptxz d78a207f (no description set)
+    Working copy now at: kkmpptxz d78a207f (no description set)
     Parent commit      : rlvkpnrz 613028a4 (no description set)
     Added 0 files, modified 0 files, removed 1 files
     "###);

cli/tests/test_global_opts.rs

@@ -440,6 +440,7 @@ fn test_help() {
                                  specified
           --from <FROM>          Show changes from this revision. Defaults to @ if --to is specified
           --to <TO>              Edit changes in this revision. Defaults to @ if --from is specified
+          --tool <NAME>          Specify diff editor to be used
       -h, --help                 Print help (see more with '--help')
 
     Global Options:

cli/tests/test_resolve_command.rs

@@ -111,10 +111,45 @@ fn test_resolution() {
     Error: No conflicts found at this revision
     "###);
 
+    // Try again with --tool=<name>
+    test_env.jj_cmd_ok(&repo_path, &["undo"]);
+    std::fs::write(&editor_script, "write\nresolution\n").unwrap();
+    let (stdout, stderr) = test_env.jj_cmd_ok(
+        &repo_path,
+        &[
+            "resolve",
+            "--config-toml=ui.merge-editor='false'",
+            "--tool=fake-editor",
+        ],
+    );
+    insta::assert_snapshot!(stdout, @"");
+    insta::assert_snapshot!(stderr, @r###"
+    Resolving conflicts in: file
+    Working copy now at: vruxwmqv 1a70c7c6 conflict | conflict
+    Parent commit      : zsuskuln aa493daf a | a
+    Parent commit      : royxmykx db6a4daf b | b
+    Added 0 files, modified 1 files, removed 0 files
+    "###);
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]),
+    @r###"
+    Resolved conflict in file:
+       1    1: <<<<<<<resolution
+       2     : %%%%%%%
+       3     : -base
+       4     : +a
+       5     : +++++++
+       6     : b
+       7     : >>>>>>>
+    "###);
+    insta::assert_snapshot!(test_env.jj_cmd_cli_error(&repo_path, &["resolve", "--list"]),
+    @r###"
+    Error: No conflicts found at this revision
+    "###);
+
     // Check that the output file starts with conflict markers if
     // `merge-tool-edits-conflict-markers=true`
     test_env.jj_cmd_ok(&repo_path, &["undo"]);
-    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), 
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]),
     @"");
     std::fs::write(
         &editor_script,
@@ -185,13 +220,13 @@ conflict
     insta::assert_snapshot!(stderr, @r###"
     Resolving conflicts in: file
     New conflicts appeared in these commits:
-      vruxwmqv ff4e8c6b conflict | (conflict) conflict
+      vruxwmqv 23991847 conflict | (conflict) conflict
     To resolve the conflicts, start by updating to it:
       jj new vruxwmqvtpmx
     Then use `jj resolve`, or edit the conflict markers in the file directly.
     Once the conflicts are resolved, you may want inspect the result with `jj diff`.
     Then run `jj squash` to move the resolution into the conflicted commit.
-    Working copy now at: vruxwmqv ff4e8c6b conflict | (conflict) conflict
+    Working copy now at: vruxwmqv 23991847 conflict | (conflict) conflict
     Parent commit      : zsuskuln aa493daf a | a
     Parent commit      : royxmykx db6a4daf b | b
     Added 0 files, modified 1 files, removed 0 files
@@ -252,7 +287,7 @@ conflict
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
     Resolving conflicts in: file
-    Working copy now at: vruxwmqv 95418cb8 conflict | conflict
+    Working copy now at: vruxwmqv 3166dfd2 conflict | conflict
     Parent commit      : zsuskuln aa493daf a | a
     Parent commit      : royxmykx db6a4daf b | b
     Added 0 files, modified 1 files, removed 0 files

cli/tests/test_unsquash_command.rs

@@ -205,6 +205,38 @@ fn test_unsquash_partial() {
     insta::assert_snapshot!(stdout, @r###"
     c
     "###);
+
+    // Try again with --tool=<name>, which implies --interactive
+    test_env.jj_cmd_ok(&repo_path, &["undo"]);
+    let (stdout, stderr) = test_env.jj_cmd_ok(
+        &repo_path,
+        &[
+            "unsquash",
+            "--config-toml=ui.diff-editor='false'",
+            "--tool=fake-diff-editor",
+        ],
+    );
+    insta::assert_snapshot!(stdout, @"");
+    insta::assert_snapshot!(stderr, @r###"
+    Working copy now at: mzvwutvl 1c82d27c c | (no description set)
+    Parent commit      : kkmpptxz b9d23fd8 b | (no description set)
+    "###);
+    let stdout = test_env.jj_cmd_success(&repo_path, &["print", "file1", "-r", "b"]);
+    insta::assert_snapshot!(stdout, @r###"
+    a
+    "###);
+    let stdout = test_env.jj_cmd_success(&repo_path, &["print", "file2", "-r", "b"]);
+    insta::assert_snapshot!(stdout, @r###"
+    b
+    "###);
+    let stdout = test_env.jj_cmd_success(&repo_path, &["print", "file1", "-r", "c"]);
+    insta::assert_snapshot!(stdout, @r###"
+    c
+    "###);
+    let stdout = test_env.jj_cmd_success(&repo_path, &["print", "file2", "-r", "c"]);
+    insta::assert_snapshot!(stdout, @r###"
+    c
+    "###);
 }
 
 fn get_log_output(test_env: &TestEnvironment, repo_path: &Path) -> String {