From 7ce25f84081f8965af9ac4c89d66ba99712a09ab Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 1 Mar 2024 23:49:22 +0900 Subject: [PATCH] cli: add --tool= 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 | 4 +++ cli/src/cli_util.rs | 44 +++++++++++++++++++++++------- cli/src/commands/commit.rs | 6 +++- cli/src/commands/diffedit.rs | 5 +++- cli/src/commands/move.rs | 8 ++++-- cli/src/commands/resolve.rs | 5 +++- cli/src/commands/split.rs | 10 +++++-- cli/src/commands/squash.rs | 8 ++++-- cli/src/commands/unsquash.rs | 7 +++-- cli/tests/cli-reference@.md.snap | 7 +++++ cli/tests/test_commit_command.rs | 21 ++++++++++++++ cli/tests/test_diffedit_command.rs | 38 +++++++++++++++++++++----- cli/tests/test_global_opts.rs | 1 + cli/tests/test_resolve_command.rs | 43 ++++++++++++++++++++++++++--- cli/tests/test_unsquash_command.rs | 32 ++++++++++++++++++++++ 15 files changed, 207 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a1a6b98ef5..71a37397fe 100644 --- a/CHANGELOG.md +++ b/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=` 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 diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index ff4ca176a9..d06261e9f3 100644 --- a/cli/src/cli_util.rs +++ b/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 { + /// + /// 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 { 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 { - 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 { + 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::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 { + 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 { diff --git a/cli/src/commands/commit.rs b/cli/src/commands/commit.rs index 5fa566afea..7fb2fb6b8d 100644 --- a/cli/src/commands/commit.rs +++ b/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, /// The change description to use (don't open editor) #[arg(long = "message", short, value_name = "MESSAGE")] message_paragraphs: Vec, @@ -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!( diff --git a/cli/src/commands/diffedit.rs b/cli/src/commands/diffedit.rs index 1345667c75..5999988966 100644 --- a/cli/src/commands/diffedit.rs +++ b/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, + /// Specify diff editor to be used + #[arg(long, value_name = "NAME")] + pub tool: Option, } #[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!( "\ diff --git a/cli/src/commands/move.rs b/cli/src/commands/move.rs index bfae6f06c4..7124119a66 100644 --- a/cli/src/commands/move.rs +++ b/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, /// 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, } @@ -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()?; diff --git a/cli/src/commands/resolve.rs b/cli/src/commands/resolve.rs index 3512e01893..c12cf7def9 100644 --- a/cli/src/commands/resolve.rs +++ b/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, /// 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: {}", diff --git a/cli/src/commands/split.rs b/cli/src/commands/split.rs index 21c8a091f2..b887b3c2c0 100644 --- a/cli/src/commands/split.rs +++ b/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, /// 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())?; diff --git a/cli/src/commands/squash.rs b/cli/src/commands/squash.rs index 1098741e40..4cf93615cb 100644 --- a/cli/src/commands/squash.rs +++ b/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, /// 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, } @@ -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!( "\ diff --git a/cli/src/commands/unsquash.rs b/cli/src/commands/unsquash.rs index 5dc10452b2..75ab0806e7 100644 --- a/cli/src/commands/unsquash.rs +++ b/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, } #[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 }; diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index 19a23e5bef..82a27bfc24 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/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 ` — Specify diff editor to be used (implies --interactive) * `-m`, `--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 ` — The revision to touch up. Defaults to @ if neither --to nor --from are specified * `--from ` — Show changes from this revision. Defaults to @ if --to is specified * `--to ` — Edit changes in this revision. Defaults to @ if --from is specified +* `--tool ` — 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 ` — 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 ` — 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 ` — Specify diff editor to be used (implies --interactive) * `-r`, `--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 ` — 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 ` — Specify diff editor to be used (implies --interactive) diff --git a/cli/tests/test_commit_command.rs b/cli/tests/test_commit_command.rs index 233859e998..686752de4b 100644 --- a/cli/tests/test_commit_command.rs +++ b/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=, 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] diff --git a/cli/tests/test_diffedit_command.rs b/cli/tests/test_diffedit_command.rs index 69619b49f7..891a3f2ffe 100644 --- a/cli/tests/test_diffedit_command.rs +++ b/cli/tests/test_diffedit_command.rs @@ -63,6 +63,30 @@ fn test_diffedit() { M file2 "###); + // Try again with --tool= + 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 "###); diff --git a/cli/tests/test_global_opts.rs b/cli/tests/test_global_opts.rs index d2dc8dc6f7..3596460ed9 100644 --- a/cli/tests/test_global_opts.rs +++ b/cli/tests/test_global_opts.rs @@ -440,6 +440,7 @@ fn test_help() { specified --from Show changes from this revision. Defaults to @ if --to is specified --to Edit changes in this revision. Defaults to @ if --from is specified + --tool Specify diff editor to be used -h, --help Print help (see more with '--help') Global Options: diff --git a/cli/tests/test_resolve_command.rs b/cli/tests/test_resolve_command.rs index c48b39de69..232f16c1ed 100644 --- a/cli/tests/test_resolve_command.rs +++ b/cli/tests/test_resolve_command.rs @@ -111,10 +111,45 @@ fn test_resolution() { Error: No conflicts found at this revision "###); + // Try again with --tool= + 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: <<<<<<>>>>>> + "###); + 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 diff --git a/cli/tests/test_unsquash_command.rs b/cli/tests/test_unsquash_command.rs index a6b160625c..f130c070e1 100644 --- a/cli/tests/test_unsquash_command.rs +++ b/cli/tests/test_unsquash_command.rs @@ -205,6 +205,38 @@ fn test_unsquash_partial() { insta::assert_snapshot!(stdout, @r###" c "###); + + // Try again with --tool=, 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 {