completion: fix bash and zsh shell completion when completing aliases of multiple arguments #6407
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
steadmon
approved these changes
Apr 24, 2025
yuja
reviewed
Apr 25, 2025
2 tasks
jgreitemann
force-pushed
the
push-rxquwlluvknr
branch
2 times, most recently
from
277e965 to
42cab3f
Compare
yuja
reviewed
Apr 28, 2025
jgreitemann
force-pushed
the
push-rxquwlluvknr
branch
from
42cab3f to
c7b33f4
Compare
yuja
reviewed
Apr 29, 2025
jgreitemann
force-pushed
the
push-rxquwlluvknr
branch
from
c7b33f4 to
fadb0c6
Compare
The completion mechanism works differently in different shells: For example, when the command line `jj aaa bb ccc` is completed at the end of the `bb` token, bash and zsh pass the completer the whole line `-- jj aaa bb ccc` and an index of 2 which refers to the `bb` token; they are then expected to complete `bb`. Meanwhile, fish and Powershell only pass the command up to the completion point, so `-- jj aaa bb`; the completer is always expected to complete the last token. In all cases, the shell ultimately decides what to do with the completions, e.g. to complete up to a common prefix (bash), to show an interactive picker (zsh, fish), or to insert the completion if it is the only one (all shells). Remaining tokens (`ccc`) are also always appended by the shell and not part of the completion. While this is mostly handled by the clap_complete crate, we do expand aliases and present clap_complete with a fake view of the real command line, thereby reaching into its internals by wrapping the interface between the completion shell script that is provided by clap_complete and its Rust code in `CommandEnv::try_complete()`. If we get this wrong, completion might yield unexpected results, so it is worth testing completion for both flavors of shells whenever aliases are potentially in the mix. To avoid redundancy, the shell-specific invocation of `jj` is factored into a `complete_at()` function of the test fixture. The `test-case` crate is then used to instantiate each test case for different values of clap_complete's `Shell` enum. filter bash/zsh specific behavior move impl
…n bash and zsh This also adds a test case for the completion of arguments following multi-argument aliases, to cover the bug reported in issue #5377. The default command is like a special kind of alias, one which is expanded from zero tokens. Consequently, it also triggers the bug #5377 on bash/zsh, even if the `default-command` is just a single token. The fix is along the lines sketched out by the "TODO" comment. Bash and Zsh don't behave identical, so the padding ([""]) still needs to be applied (and removed) conditionally in a disciplined manner.
jgreitemann
force-pushed
the
push-rxquwlluvknr
branch
from
fadb0c6 to
2b85bd1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #5377.
The fix ended up as exactly like @yuja mentioned in the issue and what the
TODOcomment in the code was already proposing. Still, it took a fair bit of fiddling around with the different shells to understand where they are differing in behavior exactly.I've gotten a wee bit carried away refactoring the existing tests to cover multiple shells (where sensible) without duplicating too much of the test code. Even so, insta needs a
matchover the different shell flavors to snapshot their output individually. For a while, I've entertained the idea of adding a.normalize_shell_completions(shell)function on the test output to basically strip away the comment (because bash doesn't have that) such that it would produce the same (normalized) output for each shell and we could do away with thematches. Obviously, that would lose the comment snapshots, so I decided against it for now. Let me know if that's something you want me to revisit.Checklist
If applicable:
CHANGELOG.md