From b8cfb1a8c6e2d80ab5dab5c788e180881825c28b Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Mon, 31 Mar 2025 23:11:50 -0700 Subject: [PATCH] cli completion: complete `--tool` args for merge tools Includes diff tools, diff editors, and merge editors. --- cli/src/commands/commit.rs | 7 ++++- cli/src/commands/diffedit.rs | 6 +++- cli/src/commands/resolve.rs | 7 ++++- cli/src/commands/restore.rs | 6 +++- cli/src/commands/split.rs | 6 +++- cli/src/commands/squash.rs | 6 +++- cli/src/complete.rs | 43 ++++++++++++++++++++++++++++ cli/src/diff_util.rs | 6 +++- cli/src/merge_tools/mod.rs | 28 ++++++++++++++---- cli/tests/test_completion.rs | 55 ++++++++++++++++++++++++++++++++++++ 10 files changed, 157 insertions(+), 13 deletions(-) diff --git a/cli/src/commands/commit.rs b/cli/src/commands/commit.rs index a6b81c03c..05bedbc3b 100644 --- a/cli/src/commands/commit.rs +++ b/cli/src/commands/commit.rs @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +use clap_complete::ArgValueCandidates; use clap_complete::ArgValueCompleter; use jj_lib::backend::Signature; use jj_lib::object_id::ObjectId as _; @@ -35,7 +36,11 @@ pub(crate) struct CommitArgs { #[arg(short, long)] interactive: bool, /// Specify diff editor to be used (implies --interactive) - #[arg(long, value_name = "NAME")] + #[arg( + long, + value_name = "NAME", + add = ArgValueCandidates::new(complete::diff_editors), + )] tool: Option, /// The change description to use (don't open editor) #[arg(long = "message", short, value_name = "MESSAGE")] diff --git a/cli/src/commands/diffedit.rs b/cli/src/commands/diffedit.rs index d07b3521d..f3c38f193 100644 --- a/cli/src/commands/diffedit.rs +++ b/cli/src/commands/diffedit.rs @@ -78,7 +78,11 @@ pub(crate) struct DiffeditArgs { )] to: Option, /// Specify diff editor to be used - #[arg(long, value_name = "NAME")] + #[arg( + long, + value_name = "NAME", + add = ArgValueCandidates::new(complete::diff_editors), + )] tool: Option, /// Preserve the content (not the diff) when rebasing descendants /// diff --git a/cli/src/commands/resolve.rs b/cli/src/commands/resolve.rs index 40b6c5f2c..4e151b895 100644 --- a/cli/src/commands/resolve.rs +++ b/cli/src/commands/resolve.rs @@ -62,7 +62,12 @@ pub(crate) struct ResolveArgs { /// /// The built-in merge tools `:ours` and `:theirs` can be used to choose /// side #1 and side #2 of the conflict respectively. - #[arg(long, conflicts_with = "list", value_name = "NAME")] + #[arg( + long, + conflicts_with = "list", + value_name = "NAME", + add = ArgValueCandidates::new(complete::merge_tools), + )] tool: Option, /// Only resolve conflicts in these paths. You can use the `--list` argument /// to find paths to use here. diff --git a/cli/src/commands/restore.rs b/cli/src/commands/restore.rs index 69fd3498b..1dbbe4e04 100644 --- a/cli/src/commands/restore.rs +++ b/cli/src/commands/restore.rs @@ -97,7 +97,11 @@ pub(crate) struct RestoreArgs { #[arg(long, short)] interactive: bool, /// Specify diff editor to be used (implies --interactive) - #[arg(long, value_name = "NAME")] + #[arg( + long, + value_name = "NAME", + add = ArgValueCandidates::new(complete::diff_editors), + )] tool: Option, /// Preserve the content (not the diff) when rebasing descendants #[arg(long)] diff --git a/cli/src/commands/split.rs b/cli/src/commands/split.rs index e5cc5362e..e62280409 100644 --- a/cli/src/commands/split.rs +++ b/cli/src/commands/split.rs @@ -59,7 +59,11 @@ pub(crate) struct SplitArgs { #[arg(long, short)] interactive: bool, /// Specify diff editor to be used (implies --interactive) - #[arg(long, value_name = "NAME")] + #[arg( + long, + value_name = "NAME", + add = ArgValueCandidates::new(complete::diff_editors), + )] tool: Option, /// The revision to split #[arg( diff --git a/cli/src/commands/squash.rs b/cli/src/commands/squash.rs index 1987c9011..a34e01e57 100644 --- a/cli/src/commands/squash.rs +++ b/cli/src/commands/squash.rs @@ -99,7 +99,11 @@ pub(crate) struct SquashArgs { #[arg(long, short)] interactive: bool, /// Specify diff editor to be used (implies --interactive) - #[arg(long, value_name = "NAME")] + #[arg( + long, + value_name = "NAME", + add = ArgValueCandidates::new(complete::diff_editors), + )] tool: Option, /// Move only changes to these paths (instead of all paths) #[arg( diff --git a/cli/src/complete.rs b/cli/src/complete.rs index 3fbb7fb10..191d213b5 100644 --- a/cli/src/complete.rs +++ b/cli/src/complete.rs @@ -34,6 +34,8 @@ use crate::config::default_config_layers; use crate::config::ConfigArgKind; use crate::config::ConfigEnv; use crate::config::CONFIG_SCHEMA; +use crate::merge_tools::configured_merge_tools; +use crate::merge_tools::MergeEditor; use crate::revset_util::load_revset_aliases; use crate::ui::Ui; @@ -412,6 +414,47 @@ pub fn workspaces() -> Vec { .collect()) }) } +pub fn merge_tools() -> Vec { + with_jj(|_, settings| { + Ok([":builtin", ":ours", ":theirs"] + .into_iter() + .chain( + configured_merge_tools(settings) + .filter(|name| MergeEditor::dummy_with_name(name, settings).is_ok()), + ) + .map(CompletionCandidate::new) + .collect()) + }) +} + +/// Approximate list of known diff editors +/// +/// Diff tools can be used without configuration. Some merge tools that are +/// configured for 3-way merging may not work for diffing/diff editing, and we +/// can't tell which these are. So, this not reliable, but probably good enough +/// for command-line completion. +pub fn diff_editors() -> Vec { + with_jj(|_, settings| { + Ok(std::iter::once(":builtin") + .chain(configured_merge_tools(settings)) + .map(CompletionCandidate::new) + .collect()) + }) +} + +/// Approximate list of known diff tools +/// +/// Diff tools can be used without configuration. Some merge tools that are +/// configured for 3-way merging may not work for diffing/diff editing, and we +/// can't tell which these are. So, this not reliable, but probably good enough +/// for command-line completion. +pub fn diff_tools() -> Vec { + with_jj(|_, settings| { + Ok(configured_merge_tools(settings) + .map(CompletionCandidate::new) + .collect()) + }) +} fn config_keys_rec( prefix: ConfigNamePathBuf, diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index b32ae0aa1..21cfe4cba 100644 --- a/cli/src/diff_util.rs +++ b/cli/src/diff_util.rs @@ -23,6 +23,7 @@ use std::path::PathBuf; use bstr::BStr; use bstr::BString; +use clap_complete::ArgValueCandidates; use futures::executor::block_on_stream; use futures::stream::BoxStream; use futures::StreamExt as _; @@ -117,7 +118,10 @@ pub struct DiffFormatArgs { #[arg(long)] pub color_words: bool, /// Generate diff by external command - #[arg(long)] + #[arg( + long, + add = ArgValueCandidates::new(crate::complete::diff_tools), + )] pub tool: Option, /// Number of lines of context to show #[arg(long)] diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index 5f8b0c00f..6dc11f7fb 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -204,6 +204,11 @@ fn editor_args_from_settings( } } +/// List configured merge tools (diff editors, diff tools, merge editors) +pub fn configured_merge_tools(settings: &UserSettings) -> impl Iterator { + settings.table_keys("merge-tools") +} + /// Loads external diff/merge tool options from `[merge-tools.]`. pub fn get_external_tool_config( settings: &UserSettings, @@ -376,6 +381,22 @@ impl MergeEditor { Self::new_inner(name, tool, path_converter, conflict_marker_style) } + /// For the purposes of testing or checking basic config + pub fn dummy_with_name( + name: &str, + settings: &UserSettings, + ) -> Result { + Self::with_name( + name, + settings, + RepoPathUiConverter::Fs { + cwd: "".into(), + base: "".into(), + }, + ConflictMarkerStyle::Diff, + ) + } + /// Loads the default 3-way merge editor from the settings. pub fn from_settings( ui: &Ui, @@ -762,12 +783,7 @@ mod tests { let get = |name, config_text| { let config = config_from_string(config_text); let settings = UserSettings::from_config(config).unwrap(); - let path_converter = RepoPathUiConverter::Fs { - cwd: "".into(), - base: "".into(), - }; - MergeEditor::with_name(name, &settings, path_converter, ConflictMarkerStyle::Diff) - .map(|editor| editor.tool) + MergeEditor::dummy_with_name(name, &settings).map(|editor| editor.tool) }; insta::assert_debug_snapshot!(get(":builtin", "").unwrap(), @"Builtin"); diff --git a/cli/tests/test_completion.rs b/cli/tests/test_completion.rs index 4fbad45ae..1d6cfa417 100644 --- a/cli/tests/test_completion.rs +++ b/cli/tests/test_completion.rs @@ -772,6 +772,61 @@ fn test_template_alias() { "); } +#[test] +fn test_merge_tools() { + let mut test_env = TestEnvironment::default(); + test_env.add_env_var("COMPLETE", "fish"); + let dir = test_env.env_root(); + + let output = test_env.run_jj_in(dir, ["--", "jj", "diff", "--tool", ""]); + insta::assert_snapshot!(output, @r" + diffedit3 + diffedit3-ssh + difft + kdiff3 + meld + meld-3 + mergiraf + smerge + vimdiff + vscode + vscodium + [EOF] + "); + // Includes :builtin + let output = test_env.run_jj_in(dir, ["--", "jj", "diffedit", "--tool", ""]); + insta::assert_snapshot!(output, @r" + :builtin + diffedit3 + diffedit3-ssh + difft + kdiff3 + meld + meld-3 + mergiraf + smerge + vimdiff + vscode + vscodium + [EOF] + "); + // Only includes configured merge editors + let output = test_env.run_jj_in(dir, ["--", "jj", "resolve", "--tool", ""]); + insta::assert_snapshot!(output, @r" + :builtin + :ours + :theirs + kdiff3 + meld + mergiraf + smerge + vimdiff + vscode + vscodium + [EOF] + "); +} + fn create_commit( work_dir: &TestWorkDir, name: &str,