-
Notifications
You must be signed in to change notification settings - Fork 754
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.
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
base: main
Are you sure you want to change the base?
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.
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.
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.
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
|
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. |
Fixes #6861
Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)