Skip to content

completion: avoid panic when completing non-normal paths #7770

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+138 −42
Open

completion: avoid panic when completing non-normal paths #7770

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c956261
completion: avoid panic when completing non-normal paths
jgreitemann Oct 18, 2025
e0df00b
completion: handle non-normalized paths
jgreitemann Jul 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
112 changes: 66 additions & 46 deletions cli/src/complete.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 _;
Expand Down Expand Up @@ -739,14 +740,57 @@ pub fn branch_name_equals_any_revision(current: &std::ffi::OsStr) -> Vec<Complet
.collect()
}

fn dir_prefix_from<'a>(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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to use std::path::is_separator instead of manually defining this function

c == '/' || c == std::path::MAIN_SEPARATOR
}

fn path_completion_candidate_from(
current_prefix: &str,
path: &str,
mode: Option<&str>,
) -> Option<CompletionCandidate> {
// 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 {
Expand Down Expand Up @@ -778,12 +822,7 @@ fn all_files_from_rev(rev: String, current: &std::ffi::OsStr) -> Vec<CompletionC
.lines()
.take(1_000)
.map_while(Result::ok)
.map(|path| {
if let Some(dir_path) = dir_prefix_from(&path, current) {
return CompletionCandidate::new(dir_path);
}
CompletionCandidate::new(path)
})
.filter_map(|path| path_completion_candidate_from(current, &path, None))
.dedup() // directories may occur multiple times
.collect())
})
Expand Down Expand Up @@ -815,36 +854,20 @@ fn modified_files_from_rev_with_jj_cmd(
let output = cmd.output().map_err(user_error)?;
let stdout = String::from_utf8_lossy(&output.stdout);

let mut candidates = Vec::new();
let mut include_renames = false;

for (mode, path) in stdout.lines().filter_map(|line| line.split_once(' ')) {
fn path_to_candidate(current: &str, mode: &str, path: &str) -> 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())));
}
Expand Down Expand Up @@ -878,16 +901,13 @@ fn conflicted_files_from_rev(rev: &str, current: &std::ffi::OsStr) -> Vec<Comple

Ok(stdout
.lines()
.map(|line| {
.filter_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)
path_completion_candidate_from(current, path, None)
})
.dedup() // directories may occur multiple times
.collect())
Expand Down
72 changes: 63 additions & 9 deletions cli/tests/test_completion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1400,17 +1400,55 @@ fn test_files() {
[EOF]
");

let output = work_dir.complete_fish(["file", "show", "f_dir/../"]);
let output = work_dir.complete_fish(["file", "show", "./f_"]);
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", "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", "show", "f_dir/"]);
insta::assert_snapshot!(output.normalize_backslash(), @r"
f_dir/dir_file_1
f_dir/dir_file_2
f_dir/dir_file_3
f_dir/f_renamed_3
[EOF]
");

let output = work_dir.complete_fish(["file", "show", "f_dir/../"]);
insta::assert_snapshot!(output.normalize_backslash(), @r"
f_dir/../f_added
f_dir/../f_added_2
f_dir/../f_another_renamed_2
f_dir/../f_copied
f_dir/../f_dir/
f_dir/../f_modified
f_dir/../f_not_yet_copied
f_dir/../f_renamed
f_dir/../f_unchanged
[EOF]
");

let output = work_dir.complete_fish(["file", "show", "f_dir/../f_dir/"]);
insta::assert_snapshot!(output.normalize_backslash(), @r"
f_dir/../f_dir/dir_file_1
f_dir/../f_dir/dir_file_2
f_dir/../f_dir/dir_file_3
f_dir/../f_dir/f_renamed_3
[EOF]
");

Expand Down Expand Up @@ -1443,6 +1481,22 @@ fn test_files() {
[EOF]
");

let output = work_dir.complete_fish(["diff", "-r", "@-", "f_dir/../"]);
insta::assert_snapshot!(output.normalize_backslash(), @r"
f_dir/../f_added Added
f_dir/../f_another_renamed_2 Renamed
f_dir/../f_copied Copied
f_dir/../f_deleted Deleted
f_dir/../f_dir/
f_dir/../f_modified Modified
f_dir/../f_not_yet_copied Modified
f_dir/../f_not_yet_renamed Renamed
f_dir/../f_not_yet_renamed_2 Renamed
f_dir/../f_not_yet_renamed_3 Renamed
f_dir/../f_renamed Renamed
[EOF]
");

let output = work_dir.complete_fish([
"diff",
"-r",
Expand Down