From c956261460c03386b9ffb15cde617805d9da40fe Mon Sep 17 00:00:00 2001 From: Jonas Greitemann Date: Sat, 18 Oct 2025 18:04:53 +0200 Subject: [PATCH 1/2] completion: avoid panic when completing non-normal paths Fixes #6861. `current` might not be a prefix of the (normal) completion candidate `path` if `current` itself is non-normal. In case `current` is longer than the candidate `path`, the code previously panicked. The tests have been extended to trigger this panic. The panic is avoided by using `strip_prefix()` instead of direct slicing. However, in case `current` does not prefix `path`, the path may be truncated too early. Additionally, fish discards any completion candidates that do not start with `current`, even if the completion script writes them to stdout. Thus, this commit is the minimal change to avoid the panic, but it does not yet handle completion of non-normal paths correctly. --- cli/src/complete.rs | 8 ++++++-- cli/tests/test_completion.rs | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/cli/src/complete.rs b/cli/src/complete.rs index 34162c79e5..5369d1249f 100644 --- a/cli/src/complete.rs +++ b/cli/src/complete.rs @@ -740,9 +740,13 @@ pub fn branch_name_equals_any_revision(current: &std::ffi::OsStr) -> Vec(path: &'a str, current: &str) -> Option<&'a str> { - path[current.len()..] + let remainder = path.strip_prefix(current).unwrap_or(path); + remainder .split_once(std::path::MAIN_SEPARATOR) - .map(|(next, _)| path.split_at(current.len() + next.len() + 1).0) + .map(|(next, _)| { + path.split_at(path.len() - remainder.len() + next.len() + 1) + .0 + }) } fn current_prefix_to_fileset(current: &str) -> String { diff --git a/cli/tests/test_completion.rs b/cli/tests/test_completion.rs index a0bf6c08fc..7baf795665 100644 --- a/cli/tests/test_completion.rs +++ b/cli/tests/test_completion.rs @@ -1400,6 +1400,20 @@ fn test_files() { [EOF] "); + let output = work_dir.complete_fish(["file", "show", "f_dir/../"]); + insta::assert_snapshot!(output.normalize_backslash(), @r" + f_added + f_added_2 + f_another_renamed_2 + f_copied + f_dir/ + f_modified + f_not_yet_copied + f_renamed + f_unchanged + [EOF] + "); + let output = work_dir.complete_fish(["file", "annotate", "-r@-", "f_"]); insta::assert_snapshot!(output.normalize_backslash(), @r" f_added From e0df00b2ad4e03f40182062c5ad216cf2aaea620 Mon Sep 17 00:00:00 2001 From: Jonas Greitemann Date: Tue, 22 Jul 2025 21:52:58 +0200 Subject: [PATCH 2/2] completion: handle non-normalized paths Rewrites how the file-path completion is determined from a potentially non-normal `current_prefix`. The basic idea is to normalize the `current_prefix` first to obtain an actual prefix of the (normal) `path`. The remainder can then be spliced onto the original `current_prefix` to form a non-normalized completion path that is prefixed by `current_prefix`, allowing the shell to accept it. This requires a different API for the helper than what `dir_prefix_from` provided. The latter assumed that when `None` was returned, `path` could be completed as-is; a `Some` value indicated a partial completion of `path` to the next directory. This is no longer sufficient as we need to potentially return a different, non-normal path in either case. Since we want to ignore the `mode` for directory completions, the helper must also return the type of completion (partial/directory or full/file). To avoid propagating this information, I instead pass the `mode` into the helper and have it return finished `CompletionCandidate`s. This also yields cleaner code on the caller side. --- CHANGELOG.md | 4 ++ cli/src/complete.rs | 112 +++++++++++++++++++++-------------- cli/tests/test_completion.rs | 72 +++++++++++++++++++--- 3 files changed, 133 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fc19e69a62..202aacf5eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -84,6 +84,10 @@ edits the change twice in some cases. * Fixed parsing of `files(expr)` revset expression including parentheses. [#7747](https://github.com/jj-vcs/jj/issues/7747) +* Shell completion now works with non‑normalized paths, fixing the previous + panic and allowing prefixes containing `.` or `..` to be completed correctly. + [#6861](https://github.com/jj-vcs/jj/issues/6861) + ## [0.34.0] - 2025-10-01 ### Release highlights diff --git a/cli/src/complete.rs b/cli/src/complete.rs index 5369d1249f..5d9e454dea 100644 --- a/cli/src/complete.rs +++ b/cli/src/complete.rs @@ -22,6 +22,7 @@ use clap_complete::CompletionCandidate; use indoc::indoc; use itertools::Itertools as _; use jj_lib::config::ConfigNamePathBuf; +use jj_lib::file_util::normalize_path; use jj_lib::settings::UserSettings; use jj_lib::workspace::DefaultWorkspaceLoaderFactory; use jj_lib::workspace::WorkspaceLoaderFactory as _; @@ -739,14 +740,57 @@ pub fn branch_name_equals_any_revision(current: &std::ffi::OsStr) -> Vec(path: &'a str, current: &str) -> Option<&'a str> { - let remainder = path.strip_prefix(current).unwrap_or(path); - remainder - .split_once(std::path::MAIN_SEPARATOR) - .map(|(next, _)| { - path.split_at(path.len() - remainder.len() + next.len() + 1) - .0 - }) +fn is_path_separator(c: char) -> bool { + c == '/' || c == std::path::MAIN_SEPARATOR +} + +fn path_completion_candidate_from( + current_prefix: &str, + path: &str, + mode: Option<&str>, +) -> Option { + // The user-provided completion string might not prefix `path` unless cast in + // normal form. + let normalized_prefix_path = normalize_path(Path::new(current_prefix)); + let normalized_prefix = match normalized_prefix_path.to_str() { + None => current_prefix, + Some(".") => "", // `.` cannot be normalized further, but doesn't prefix `path`. + Some(normalized_prefix) => normalized_prefix, + }; + + let mut remainder = path.strip_prefix(normalized_prefix)?; + + // Trailing slash might have been normalized away in which case we need to strip + // the leading slash in the remainder away, or else the slash would appear + // twice. + if current_prefix.ends_with(is_path_separator) { + remainder = remainder + .strip_prefix(is_path_separator) + .unwrap_or(remainder); + } + + let (completion, help) = match remainder.split_inclusive(is_path_separator).at_most_one() { + // Completed component is the final component in `path`, so we're completing the file to + // which `mode` refers. + Ok(file_completion) => ( + file_completion?, + mode.map(|mode| match mode { + "modified" => "Modified".into(), + "removed" => "Deleted".into(), + "added" => "Added".into(), + "renamed" => "Renamed".into(), + "copied" => "Copied".into(), + _ => format!("unknown mode: '{mode}'").into(), + }), + ), + + // Omit `mode` when completing only up to the next directory. + Err(mut components) => (components.next().unwrap(), None), + }; + + // Completion candidate splices normalized completion onto the user-provided + // prefix. + Some(CompletionCandidate::new(format!("{current_prefix}{completion}")).help(help)) } fn current_prefix_to_fileset(current: &str) -> String { @@ -778,12 +822,7 @@ fn all_files_from_rev(rev: String, current: &std::ffi::OsStr) -> Vec CompletionCandidate { - if let Some(dir_path) = dir_prefix_from(path, current) { - return CompletionCandidate::new(dir_path); + let mut candidates: Vec<_> = stdout + .lines() + .filter_map(|line| line.split_once(' ')) + .filter_map(|(mode, path)| { + if mode == "renamed.source" { + path_completion_candidate_from(current, path, Some("renamed")) + .inspect(|_| include_renames |= true) + } else { + path_completion_candidate_from(current, path, Some(mode)) } + }) + .collect(); - let help = match mode { - "modified" => "Modified".into(), - "removed" => "Deleted".into(), - "added" => "Added".into(), - "renamed" => "Renamed".into(), - "copied" => "Copied".into(), - _ => format!("unknown mode: '{mode}'"), - }; - CompletionCandidate::new(path).help(Some(help.into())) - } - - if mode == "renamed.source" { - if !path.starts_with(current) { - continue; - } - candidates.push(path_to_candidate(current, "renamed", path)); - include_renames |= true; - } else { - candidates.push(path_to_candidate(current, mode, path)); - } - } if include_renames { candidates.sort_unstable_by(|a, b| Path::new(a.get_value()).cmp(Path::new(b.get_value()))); } @@ -878,16 +901,13 @@ fn conflicted_files_from_rev(rev: &str, current: &std::ffi::OsStr) -> Vec