From 81eb10d13916ddb1cb844aa46b5217fb3d8515e0 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. --- 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 | 4 +- cli/src/complete.rs | 109 ++++++++++++++++++++++++---------- cli/tests/test_completion.rs | 63 ++++++++++++++++---- 9 files changed, 144 insertions(+), 51 deletions(-) diff --git a/cli/src/commands/commit.rs b/cli/src/commands/commit.rs index 2e794329580..52f75a44415 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 0f79aa707c8..b3a1c284090 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 dbe2db66b36..d577e22bdf0 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 76bd8315a34..bd29479ab18 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 11ec1bfdf7b..5262dd6e3d0 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 e750af8705d..25d2f01cf38 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 79f13fba311..281b662b7a7 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; @@ -92,8 +93,7 @@ pub(crate) struct SquashArgs { /// Move only changes to these paths (instead of all paths) #[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 ed829a50b24..2112e0a677f 100644 --- a/cli/src/complete.rs +++ b/cli/src/complete.rs @@ -448,7 +448,16 @@ 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.strip_prefix(current)? + .split_once(std::path::MAIN_SEPARATOR) + .map(|(next, _)| path.split_at(current.len() + next.len() + 1).0) +} + +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() @@ -465,7 +474,16 @@ fn all_files_from_rev(rev: String) -> Vec { .lines() .take(1_000) .map_while(Result::ok) - .map(CompletionCandidate::new) + .filter_map(|path| { + if !path.starts_with(current) { + return None; + } + if let Some(dir_path) = dir_prefix_from(&path, current) { + return Some(CompletionCandidate::new(dir_path)); + } + Some(CompletionCandidate::new(path)) + }) + .dedup() // directories may occur multiple times .collect()) }) } @@ -473,7 +491,11 @@ 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> { + let Some(current) = current.to_str() else { + return Ok(Vec::new()); + }; cmd.arg("diff").arg("--summary"); match rev { (rev, None) => cmd.arg("--revision").arg(rev), @@ -484,10 +506,18 @@ fn modified_files_from_rev_with_jj_cmd( Ok(stdout .lines() - .map(|line| { + .filter_map(|line| { let (mode, path) = line .split_once(' ') .expect("diff --summary should contain a space between mode and path"); + + if !path.starts_with(current) { + return None; + } + if let Some(dir_path) = dir_prefix_from(path, current) { + return Some(CompletionCandidate::new(dir_path)); + } + let help = match mode { "M" => "Modified".into(), "D" => "Deleted".into(), @@ -496,16 +526,23 @@ fn modified_files_from_rev_with_jj_cmd( "C" => "Copied".into(), _ => format!("unknown mode: '{mode}'"), }; - CompletionCandidate::new(path).help(Some(help.into())) + Some(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() @@ -519,53 +556,61 @@ fn conflicted_files_from_rev(rev: &str) -> Vec { Ok(stdout .lines() - .filter_map(|line| line.split_whitespace().next()) - .map(CompletionCandidate::new) + .filter_map(|line| { + let path = line.split_whitespace().next()?; + + if !path.starts_with(current) { + return None; + } + if let Some(dir_path) = dir_prefix_from(path, current) { + return Some(CompletionCandidate::new(dir_path)); + } + + Some(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 +618,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 409a3f691f1..bb8f631e136 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(), @@ -588,6 +591,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 +606,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 +648,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 +691,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 +703,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 +762,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 +771,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 +780,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 + "); }