-
Notifications
You must be signed in to change notification settings - Fork 749
Description
Description
When invoking dynamic completion on any fileset command line argument, the current implementation assumes that the file path that is being completed is a prefix of all the candidates found through jj file list
. For instance, in the jj repo, typing CHANGELOG
<TAB>, will produce CHANGELOG.md
as the only matching path, and CHANGELOG.md
indeed starts with CHANGELOG
.
However, this assumption breaks down once the fileset argument is not normalized. For example, typing ./CHANGELOG
<TAB> does not produce any completion candidates because jj file list
resolves the glob to the normalized CHANGELOG.md
which does not start with ./CHANGELOG
and is discarded.
Worse still, when the non-normalized fileset is longer than the completed path it resolves to, it produces a panic due to the unchecked slicing in this function:
Lines 628 to 632 in 34b0961
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) | |
} |
This panic could be easily avoided by using
str::strip_prefix()
instead of slicing which would spell out this assumption and additionally verify that we're slicing away the expected current.len()
bytes. However, this would still lead to the legitimate completion candidate being discarded.
A more thorough solution would probably need to use something like the normalize-path crate. In contrast, std::fs::canonicalize()
requires files to exist to resolve symlinks which precludes it from being used on revisions other than @
.
Steps to Reproduce the Problem
Taking the jj repo as an example
jj file show ./CHANGELOG
<TAB> → no completion resultsjj file show ./CHANGELOG.md
<TAB> panics
thread 'main' panicked at cli/src/complete.rs:629:9:
byte index 14 is out of bounds of `CHANGELOG.md`
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
RUST_BACKTRACE=1 jj file show cli/../CHANG
<TAB> panics
thread 'main' panicked at cli/src/complete.rs:629:9:
byte index 13 is out of bounds of `CHANGELOG.md`
stack backtrace:
0: __rustc::rust_begin_unwind
1: core::panicking::panic_fmt
2: core::str::slice_error_fail_rt
3: core::str::slice_error_fail
4: jj_cli::complete::dir_prefix_from
5: <itertools::adaptors::coalesce::CoalesceBy<I,F,C> as core::iter::traits::iterator::Iterator>::next
6: jj_cli::complete::all_files_from_rev
7: <F as clap_complete::engine::custom::ValueCompleter>::complete
8: clap_complete::engine::complete::complete_arg_value
9: clap_complete::engine::complete::complete_arg
10: clap_complete::engine::complete::complete
11: <clap_complete::env::shells::Fish as clap_complete::env::EnvCompleter>::write_complete
12: jj_cli::cli_util::CliRunner::run
13: jj::main
Expected Behavior
Ideally, triggering completion on non-normalized paths should produce candidates which preserve the typed path, i.e. completing to ./CHANGELOG.md
in the first two cases and to cli/../CHANGELOG.md
in the third.
Alternatively, I guess it would be acceptable to use the path as returned by jj file list
which would replace the argument with CHANGELOG.md
in all three cases.
Specifications
- Platform: reproduced on macOS & Linux; probably Windows affected, too. (Possibly, Windows accepting both
/
and\
as path separators may introduce another failure mode, but I cannot check at the moment.) - Version: reproduced with jj 0.30.0 and jj 0.28.2.