completion: avoid panic when completing non-normal paths #7770
Conversation
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.
jgreitemann
force-pushed
the
jg/xwxkuvqprpst
branch
from
5be92a7 to
0fd4b73
Compare
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.
jgreitemann
force-pushed
the
jg/xwxkuvqprpst
branch
from
0fd4b73 to
e0df00b
Compare
| path.split_at(path.len() - remainder.len() + next.len() + 1) | ||
| .0 | ||
| }) | ||
| fn is_path_separator(c: char) -> bool { |
There was a problem hiding this comment.
It might be better to use std::path::is_separator instead of manually defining this function
|
Maybe it's easier to normalize input to |
|
Easier in the sense that RepoPath uses If it's just Git Bash (and other POSIX shells in MinGW et al. environments), I'd lean towards sticking with the native path separator in general and fix the Git Bash issue separately by using |
Easier because the command output is also normalized (to repo-relative paths.) Post-processing would be simple if both inputs and outputs are in the canonical form. We can convert them back to backslash paths if needed. |
3 participants
Fixes #6861
Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)