From a8c35db43d7b80ea28e664e45aec63bd4275e919 Mon Sep 17 00:00:00 2001 From: Remo Senekowitsch Date: Sun, 17 Nov 2024 09:47:31 +0100 Subject: [PATCH] completion: suggest file paths incrementally If there are multiple files in a subdirectory that are candidates for completion, only complete the common directory prefix to reduce the number of completion candidates shown at once. This matches the normal shell completion of file paths. --- Cargo.lock | 1 + cli/Cargo.toml | 1 + cli/src/commands/commit.rs | 4 +- cli/src/commands/diff.rs | 3 +- cli/src/commands/interdiff.rs | 3 +- cli/src/commands/resolve.rs | 3 +- cli/src/commands/restore.rs | 3 +- cli/src/commands/split.rs | 3 +- cli/src/commands/squash.rs | 3 +- cli/src/complete.rs | 117 +++++++++++++++++++++++++--------- cli/tests/test_completion.rs | 67 +++++++++++++++---- 11 files changed, 159 insertions(+), 49 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2f4f2eb3bf..3f6b7851d4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1826,6 +1826,7 @@ dependencies = [ "futures 0.3.31", "git2", "gix", + "glob", "indexmap", "indoc", "insta", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 57be178226..a2d3400a91 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -64,6 +64,7 @@ dunce = { workspace = true } futures = { workspace = true } git2 = { workspace = true } gix = { workspace = true } +glob = { workspace = true } indexmap = { workspace = true } indoc = { workspace = true } itertools = { workspace = true } diff --git a/cli/src/commands/commit.rs b/cli/src/commands/commit.rs index 2e79432958..52f75a4441 100644 --- a/cli/src/commands/commit.rs +++ b/cli/src/commands/commit.rs @@ -12,7 +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; use jj_lib::repo::Repo; @@ -44,7 +44,7 @@ pub(crate) struct CommitArgs { /// Put these paths in the first commit #[arg( value_hint = clap::ValueHint::AnyPath, - add = ArgValueCandidates::new(complete::modified_files), + add = ArgValueCompleter::new(complete::modified_files), )] paths: Vec, /// Reset the author to the configured user diff --git a/cli/src/commands/diff.rs b/cli/src/commands/diff.rs index 0f79aa707c..b3a1c28409 100644 --- a/cli/src/commands/diff.rs +++ b/cli/src/commands/diff.rs @@ -13,6 +13,7 @@ // limitations under the License. use clap_complete::ArgValueCandidates; +use clap_complete::ArgValueCompleter; use itertools::Itertools; use jj_lib::copies::CopyRecords; use jj_lib::repo::Repo; @@ -59,7 +60,7 @@ pub(crate) struct DiffArgs { /// Restrict the diff to these paths #[arg( value_hint = clap::ValueHint::AnyPath, - add = ArgValueCandidates::new(complete::modified_revision_or_range_files), + add = ArgValueCompleter::new(complete::modified_revision_or_range_files), )] paths: Vec, #[command(flatten)] diff --git a/cli/src/commands/interdiff.rs b/cli/src/commands/interdiff.rs index dbe2db66b3..d577e22bdf 100644 --- a/cli/src/commands/interdiff.rs +++ b/cli/src/commands/interdiff.rs @@ -16,6 +16,7 @@ use std::slice; use clap::ArgGroup; use clap_complete::ArgValueCandidates; +use clap_complete::ArgValueCompleter; use tracing::instrument; use crate::cli_util::CommandHelper; @@ -44,7 +45,7 @@ pub(crate) struct InterdiffArgs { /// Restrict the diff to these paths #[arg( value_hint = clap::ValueHint::AnyPath, - add = ArgValueCandidates::new(complete::interdiff_files), + add = ArgValueCompleter::new(complete::interdiff_files), )] paths: Vec, #[command(flatten)] diff --git a/cli/src/commands/resolve.rs b/cli/src/commands/resolve.rs index 76bd8315a3..bd29479ab1 100644 --- a/cli/src/commands/resolve.rs +++ b/cli/src/commands/resolve.rs @@ -15,6 +15,7 @@ use std::io::Write; use clap_complete::ArgValueCandidates; +use clap_complete::ArgValueCompleter; use itertools::Itertools; use jj_lib::object_id::ObjectId; use tracing::instrument; @@ -64,7 +65,7 @@ pub(crate) struct ResolveArgs { // TODO: Find the conflict we can resolve even if it's not the first one. #[arg( value_hint = clap::ValueHint::AnyPath, - add = ArgValueCandidates::new(complete::revision_conflicted_files), + add = ArgValueCompleter::new(complete::revision_conflicted_files), )] paths: Vec, } diff --git a/cli/src/commands/restore.rs b/cli/src/commands/restore.rs index 11ec1bfdf7..5262dd6e3d 100644 --- a/cli/src/commands/restore.rs +++ b/cli/src/commands/restore.rs @@ -15,6 +15,7 @@ use std::io::Write; use clap_complete::ArgValueCandidates; +use clap_complete::ArgValueCompleter; use jj_lib::object_id::ObjectId; use jj_lib::rewrite::restore_tree; use tracing::instrument; @@ -47,7 +48,7 @@ pub(crate) struct RestoreArgs { /// Restore only these paths (instead of all paths) #[arg( value_hint = clap::ValueHint::AnyPath, - add = ArgValueCandidates::new(complete::modified_range_files), + add = ArgValueCompleter::new(complete::modified_range_files), )] paths: Vec, /// Revision to restore from (source) diff --git a/cli/src/commands/split.rs b/cli/src/commands/split.rs index e750af8705..25d2f01cf3 100644 --- a/cli/src/commands/split.rs +++ b/cli/src/commands/split.rs @@ -14,6 +14,7 @@ use std::io::Write; use clap_complete::ArgValueCandidates; +use clap_complete::ArgValueCompleter; use jj_lib::object_id::ObjectId; use jj_lib::repo::Repo; use tracing::instrument; @@ -68,7 +69,7 @@ pub(crate) struct SplitArgs { /// Put these paths in the first commit #[arg( value_hint = clap::ValueHint::AnyPath, - add = ArgValueCandidates::new(complete::modified_revision_files), + add = ArgValueCompleter::new(complete::modified_revision_files), )] paths: Vec, } diff --git a/cli/src/commands/squash.rs b/cli/src/commands/squash.rs index 79f13fba31..462fec1e8c 100644 --- a/cli/src/commands/squash.rs +++ b/cli/src/commands/squash.rs @@ -13,6 +13,7 @@ // limitations under the License. use clap_complete::ArgValueCandidates; +use clap_complete::ArgValueCompleter; use itertools::Itertools as _; use jj_lib::commit::Commit; use jj_lib::commit::CommitIteratorExt; @@ -93,7 +94,7 @@ pub(crate) struct SquashArgs { #[arg( conflicts_with_all = ["interactive", "tool"], value_hint = clap::ValueHint::AnyPath, - add = ArgValueCandidates::new(complete::squash_revision_files), + add = ArgValueCompleter::new(complete::squash_revision_files), )] paths: Vec, /// The source revision will not be abandoned diff --git a/cli/src/complete.rs b/cli/src/complete.rs index ed829a50b2..bad1e81504 100644 --- a/cli/src/complete.rs +++ b/cli/src/complete.rs @@ -448,7 +448,23 @@ pub fn leaf_config_keys() -> Vec { config_keys_impl(true) } -fn all_files_from_rev(rev: String) -> Vec { +fn dir_prefix_from<'a>(path: &'a str, current: &str) -> Option<&'a str> { + path[current.len()..] + .split_once(std::path::MAIN_SEPARATOR) + .map(|(next, _)| path.split_at(current.len() + next.len() + 1).0) +} + +fn current_prefix_to_fileset(current: &str) -> String { + let cur_esc = glob::Pattern::escape(current); + let dir_pat = format!("{cur_esc}*/**"); + let path_pat = format!("{cur_esc}*"); + format!("glob:{dir_pat:?} | glob:{path_pat:?}") +} + +fn all_files_from_rev(rev: String, current: &std::ffi::OsStr) -> Vec { + let Some(current) = current.to_str() else { + return Vec::new(); + }; with_jj(|jj, _| { let mut child = jj .build() @@ -456,6 +472,9 @@ fn all_files_from_rev(rev: String) -> Vec { .arg("list") .arg("--revision") .arg(rev) + .arg("--config-toml") + .arg("ui.allow-filesets = true") + .arg(current_prefix_to_fileset(current)) .stdout(std::process::Stdio::piped()) .spawn() .map_err(user_error)?; @@ -465,7 +484,13 @@ fn all_files_from_rev(rev: String) -> Vec { .lines() .take(1_000) .map_while(Result::ok) - .map(CompletionCandidate::new) + .map(|path| { + if let Some(dir_path) = dir_prefix_from(&path, current) { + return CompletionCandidate::new(dir_path); + } + CompletionCandidate::new(path) + }) + .dedup() // directories may occur multiple times .collect()) }) } @@ -473,8 +498,16 @@ fn all_files_from_rev(rev: String) -> Vec { fn modified_files_from_rev_with_jj_cmd( rev: (String, Option), mut cmd: std::process::Command, + current: &std::ffi::OsStr, ) -> Result, CommandError> { - cmd.arg("diff").arg("--summary"); + let Some(current) = current.to_str() else { + return Ok(Vec::new()); + }; + cmd.arg("diff") + .arg("--summary") + .arg("--config-toml") + .arg("ui.allow-filesets = true") + .arg(current_prefix_to_fileset(current)); match rev { (rev, None) => cmd.arg("--revision").arg(rev), (from, Some(to)) => cmd.arg("--from").arg(from).arg("--to").arg(to), @@ -488,6 +521,11 @@ fn modified_files_from_rev_with_jj_cmd( let (mode, path) = line .split_once(' ') .expect("diff --summary should contain a space between mode and path"); + + if let Some(dir_path) = dir_prefix_from(path, current) { + return CompletionCandidate::new(dir_path); + } + let help = match mode { "M" => "Modified".into(), "D" => "Deleted".into(), @@ -498,14 +536,21 @@ fn modified_files_from_rev_with_jj_cmd( }; CompletionCandidate::new(path).help(Some(help.into())) }) + .dedup() // directories may occur multiple times .collect()) } -fn modified_files_from_rev(rev: (String, Option)) -> Vec { - with_jj(|jj, _| modified_files_from_rev_with_jj_cmd(rev, jj.build())) +fn modified_files_from_rev( + rev: (String, Option), + current: &std::ffi::OsStr, +) -> Vec { + with_jj(|jj, _| modified_files_from_rev_with_jj_cmd(rev, jj.build(), current)) } -fn conflicted_files_from_rev(rev: &str) -> Vec { +fn conflicted_files_from_rev(rev: &str, current: &std::ffi::OsStr) -> Vec { + let Some(current) = current.to_str() else { + return Vec::new(); + }; with_jj(|jj, _| { let output = jj .build() @@ -513,59 +558,69 @@ fn conflicted_files_from_rev(rev: &str) -> Vec { .arg("--list") .arg("--revision") .arg(rev) + .arg("--config-toml") + .arg("ui.allow-filesets = true") + .arg(current_prefix_to_fileset(current)) .output() .map_err(user_error)?; let stdout = String::from_utf8_lossy(&output.stdout); Ok(stdout .lines() - .filter_map(|line| line.split_whitespace().next()) - .map(CompletionCandidate::new) + .map(|line| { + let path = line + .split_whitespace() + .next() + .expect("resolve --list should contain whitespace after path"); + + if let Some(dir_path) = dir_prefix_from(path, current) { + return CompletionCandidate::new(dir_path); + } + CompletionCandidate::new(path) + }) + .dedup() // directories may occur multiple times .collect()) }) } -pub fn modified_files() -> Vec { - modified_files_from_rev(("@".into(), None)) +pub fn modified_files(current: &std::ffi::OsStr) -> Vec { + modified_files_from_rev(("@".into(), None), current) } pub fn all_revision_files(current: &std::ffi::OsStr) -> Vec { - // TODO: Use `current` once `jj file list` gains the ability to list only - // the content of the "current" directory. - let _ = current; - all_files_from_rev(parse::revision_or_wc()) + all_files_from_rev(parse::revision_or_wc(), current) } -pub fn modified_revision_files() -> Vec { - modified_files_from_rev((parse::revision_or_wc(), None)) +pub fn modified_revision_files(current: &std::ffi::OsStr) -> Vec { + modified_files_from_rev((parse::revision_or_wc(), None), current) } -pub fn modified_range_files() -> Vec { +pub fn modified_range_files(current: &std::ffi::OsStr) -> Vec { match parse::range() { - Some((from, to)) => modified_files_from_rev((from, Some(to))), - None => modified_files_from_rev(("@".into(), None)), + Some((from, to)) => modified_files_from_rev((from, Some(to)), current), + None => modified_files_from_rev(("@".into(), None), current), } } -pub fn modified_revision_or_range_files() -> Vec { +pub fn modified_revision_or_range_files(current: &std::ffi::OsStr) -> Vec { if let Some(rev) = parse::revision() { - return modified_files_from_rev((rev, None)); + return modified_files_from_rev((rev, None), current); } - modified_range_files() + modified_range_files(current) } -pub fn revision_conflicted_files() -> Vec { - conflicted_files_from_rev(&parse::revision_or_wc()) +pub fn revision_conflicted_files(current: &std::ffi::OsStr) -> Vec { + conflicted_files_from_rev(&parse::revision_or_wc(), current) } /// Specific function for completing file paths for `jj squash` -pub fn squash_revision_files() -> Vec { +pub fn squash_revision_files(current: &std::ffi::OsStr) -> Vec { let rev = parse::squash_revision().unwrap_or_else(|| "@".into()); - modified_files_from_rev((rev, None)) + modified_files_from_rev((rev, None), current) } /// Specific function for completing file paths for `jj interdiff` -pub fn interdiff_files() -> Vec { +pub fn interdiff_files(current: &std::ffi::OsStr) -> Vec { let Some((from, to)) = parse::range() else { return Vec::new(); }; @@ -573,8 +628,12 @@ pub fn interdiff_files() -> Vec { // files that are the same in both, which is a false positive. This approach // is more lightweight than actually doing a temporary rebase here. with_jj(|jj, _| { - let mut res = modified_files_from_rev_with_jj_cmd((from, None), jj.build())?; - res.extend(modified_files_from_rev_with_jj_cmd((to, None), jj.build())?); + let mut res = modified_files_from_rev_with_jj_cmd((from, None), jj.build(), current)?; + res.extend(modified_files_from_rev_with_jj_cmd( + (to, None), + jj.build(), + current, + )?); Ok(res) }) } diff --git a/cli/tests/test_completion.rs b/cli/tests/test_completion.rs index 409a3f691f..6f8da88795 100644 --- a/cli/tests/test_completion.rs +++ b/cli/tests/test_completion.rs @@ -550,6 +550,9 @@ fn create_commit( test_env.jj_cmd_ok(repo_path, &args); } for (name, content) in files { + if let Some((dir, _)) = name.rsplit_once('/') { + std::fs::create_dir_all(repo_path.join(dir)).unwrap(); + } match content { Some(content) => std::fs::write(repo_path.join(name), content).unwrap(), None => std::fs::remove_file(repo_path.join(name)).unwrap(), @@ -564,6 +567,10 @@ fn test_files() { test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); let repo_path = test_env.env_root().join("repo"); + // Completions for files use filesets internally. + // Ensure they still work if the user has them disabled. + test_env.add_config("ui.allow-filesets = false"); + create_commit( &test_env, &repo_path, @@ -588,6 +595,9 @@ fn test_files() { ("f_renamed", Some("renamed\n")), ("f_deleted", None), ("f_added", Some("added\n")), + ("f_dir/dir_file_1", Some("foo\n")), + ("f_dir/dir_file_2", Some("foo\n")), + ("f_dir/dir_file_3", Some("foo\n")), ], ); @@ -600,6 +610,9 @@ fn test_files() { &[ ("f_modified", Some("modified_again\n")), ("f_added_2", Some("added_2\n")), + ("f_dir/dir_file_1", Some("bar\n")), + ("f_dir/dir_file_2", Some("bar\n")), + ("f_dir/dir_file_3", Some("bar\n")), ], ); test_env.jj_cmd_ok(&repo_path, &["rebase", "-r=@", "-d=first"]); @@ -639,20 +652,26 @@ fn test_files() { ); let stdout = test_env.jj_cmd_success(&repo_path, &["log", "-r", "all()", "--summary"]); - insta::assert_snapshot!(stdout, @r" - @ wqnwkozp test.user@example.com 2001-02-03 08:05:20 working_copy 89d772f3 + insta::assert_snapshot!(stdout.replace('\\', "/"), @r" + @ wqnwkozp test.user@example.com 2001-02-03 08:05:20 working_copy 45c3a621 │ working_copy │ A f_added_2 │ M f_modified - ○ zsuskuln test.user@example.com 2001-02-03 08:05:11 second 12ffc2f7 + ○ zsuskuln test.user@example.com 2001-02-03 08:05:11 second 77a99380 │ second │ A f_added │ D f_deleted + │ A f_dir/dir_file_1 + │ A f_dir/dir_file_2 + │ A f_dir/dir_file_3 │ M f_modified │ A f_renamed - │ × royxmykx test.user@example.com 2001-02-03 08:05:14 conflicted 14453858 conflict + │ × royxmykx test.user@example.com 2001-02-03 08:05:14 conflicted 23eb154d conflict ├─╯ conflicted │ A f_added_2 + │ A f_dir/dir_file_1 + │ A f_dir/dir_file_2 + │ A f_dir/dir_file_3 │ M f_modified ○ rlvkpnrz test.user@example.com 2001-02-03 08:05:09 first 2a2f433c │ first @@ -676,9 +695,10 @@ fn test_files() { let test_env = test_env; let stdout = test_env.jj_cmd_success(&repo_path, &["--", "jj", "file", "show", "f_"]); - insta::assert_snapshot!(stdout, @r" + insta::assert_snapshot!(stdout.replace('\\', "/"), @r" f_added f_added_2 + f_dir/ f_modified f_not_yet_renamed f_renamed @@ -687,27 +707,47 @@ fn test_files() { let stdout = test_env.jj_cmd_success(&repo_path, &["--", "jj", "file", "annotate", "-r@-", "f_"]); - insta::assert_snapshot!(stdout, @r" + insta::assert_snapshot!(stdout.replace('\\', "/"), @r" f_added + f_dir/ f_modified f_not_yet_renamed f_renamed f_unchanged "); - let stdout = test_env.jj_cmd_success(&repo_path, &["--", "jj", "diff", "-r", "@-", "f_"]); - insta::assert_snapshot!(stdout, @r" + insta::assert_snapshot!(stdout.replace('\\', "/"), @r" f_added Added f_deleted Deleted + f_dir/ f_modified Modified f_renamed Added "); + + let stdout = test_env.jj_cmd_success( + &repo_path, + &[ + "--", + "jj", + "diff", + "-r", + "@-", + &format!("f_dir{}", std::path::MAIN_SEPARATOR), + ], + ); + insta::assert_snapshot!(stdout.replace('\\', "/"), @r" + f_dir/dir_file_1 Added + f_dir/dir_file_2 Added + f_dir/dir_file_3 Added + "); + let stdout = test_env.jj_cmd_success( &repo_path, &["--", "jj", "diff", "--from", "root()", "--to", "@-", "f_"], ); - insta::assert_snapshot!(stdout, @r" + insta::assert_snapshot!(stdout.replace('\\', "/"), @r" f_added Added + f_dir/ f_modified Added f_not_yet_renamed Added f_renamed Added @@ -726,7 +766,7 @@ fn test_files() { "f_", ], ); - insta::assert_snapshot!(stdout, @r" + insta::assert_snapshot!(stdout.replace('\\', "/"), @r" f_interdiff_only_from Added f_interdiff_same Added f_interdiff_only_to Added @@ -735,7 +775,7 @@ fn test_files() { // squash has a different behavior with --from and --to flags let stdout = test_env.jj_cmd_success(&repo_path, &["--", "jj", "squash", "-f=first", "f_"]); - insta::assert_snapshot!(stdout, @r" + insta::assert_snapshot!(stdout.replace('\\', "/"), @r" f_deleted Added f_modified Added f_not_yet_renamed Added @@ -744,5 +784,8 @@ fn test_files() { let stdout = test_env.jj_cmd_success(&repo_path, &["--", "jj", "resolve", "-r=conflicted", "f_"]); - insta::assert_snapshot!(stdout, @"f_modified"); + insta::assert_snapshot!(stdout.replace('\\', "/"), @r" + f_dir/ + f_modified + "); }