| Message ID | be727d0171b16e488a357a959176e60bf9210d40.1616060793.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | git-completion.bash: improvements to _git_stash() | expand |
Denton Liu <liu.denton@gmail.com> writes: > When completions are offered for `git stash branch<TAB>`, the user is > supposed to receive refs. This works in the case where the main git > command is called without arguments but if options are provided, such as > `git -C dir stash branch<TAB>`, then the `$cword -eq 3` provides > incorrect results. > > Count the words relative to the first instance of "stash" so that we > ignore arguments to the main git command. > > Unfortunately, this still does not work 100% correctly. For example, in > the case of something like `git -C stash stash branch<TAB>`, this will > incorrectly identify the first "stash" as the command. This seems to be > an edge-case that we can ignore, though, as other functions, such as > _git_worktree(), suffer from the same problem. I am not familiar with how the completion support works, but doing this inside _git_stash() and still not being able to tell which "stash" on the command line is supposed to be the git subcommand smells quite fishy to me. How did the caller decide to invoke _git_stash helper function in the first place? When it is given "git -C push --paginate stash branch<TAB>", it must have parsed the command line, past the options given to the "git" potty, to find "stash" on the command line that it is _git_stash and not _git_push that needs to be called, no? If it were possible to propagate that information without losing it, then we do not have to recompute where the subcommand name is at all, do we? > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > contrib/completion/git-completion.bash | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index fe79f6b71c..da46f46e3c 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -3016,6 +3016,9 @@ _git_stash () > local save_opts='--all --keep-index --no-keep-index --quiet --patch --include-untracked' > local subcommands='push list show apply clear drop pop create branch' > local subcommand="$(__git_find_on_cmdline "$subcommands save")" > + local stash_idx="$(__git_find_on_cmdline --show-idx stash)" > + stash_idx="${stash_idx% *}" > + > if [ -z "$subcommand" -a -n "$(__git_find_on_cmdline "-p")" ]; then > subcommand="push" > fi > @@ -3060,7 +3063,7 @@ _git_stash () > branch,--*) > ;; > branch,*) > - if [ $cword -eq 3 ]; then > + if [ $((cword - stash_idx)) -eq 2 ]; then > __git_complete_refs > else > __gitcomp_nl "$(__git stash list \
Hi Junio, On Thu, Mar 18, 2021 at 01:30:38PM -0700, Junio C Hamano wrote: > Denton Liu <liu.denton@gmail.com> writes: > > > When completions are offered for `git stash branch<TAB>`, the user is > > supposed to receive refs. This works in the case where the main git > > command is called without arguments but if options are provided, such as > > `git -C dir stash branch<TAB>`, then the `$cword -eq 3` provides > > incorrect results. > > > > Count the words relative to the first instance of "stash" so that we > > ignore arguments to the main git command. > > > > Unfortunately, this still does not work 100% correctly. For example, in > > the case of something like `git -C stash stash branch<TAB>`, this will > > incorrectly identify the first "stash" as the command. This seems to be > > an edge-case that we can ignore, though, as other functions, such as > > _git_worktree(), suffer from the same problem. > > I am not familiar with how the completion support works, but doing > this inside _git_stash() and still not being able to tell which > "stash" on the command line is supposed to be the git subcommand > smells quite fishy to me. > > How did the caller decide to invoke _git_stash helper function in > the first place? > > When it is given "git -C push --paginate stash branch<TAB>", it must > have parsed the command line, past the options given to the "git" > potty, to find "stash" on the command line that it is _git_stash and > not _git_push that needs to be called, no? If it were possible to > propagate that information without losing it, then we do not have to > recompute where the subcommand name is at all, do we? Good observation. _git_stash() is called in the body of __git_complete_command() which is called by __git_main(). There is currently no mechanism by which to pass the index of the command over to _git_*() completion functions. That being said, passing in the index to all functions would definitely be doable. I can work on a series in the future that passes in the index of the command so that working with $cword is more robust but I'd prefer if that were handled outside this series to keep it focused. Thanks, Denton
Denton Liu <liu.denton@gmail.com> writes: > Good observation. _git_stash() is called in the body of > __git_complete_command() which is called by __git_main(). There is > currently no mechanism by which to pass the index of the command over to > _git_*() completion functions. I think, given that "set | grep _git" tells us we use many globals already, it would be OK to introduce another variable, call it $__git_subcmd_pos, and assign to it when the command dispatcher discovers which token on the command line is the subcommand name and decides to call the subcommand specific completion helper function. Or does the command dispatcher not exactly know the position (e.g. iterates with "for w" and knows $w==stash in the current iteration but it is not counting the position in the array)? If so, then we'd need a surgery larger than that. But if we only need to set a variable, we won't have to change the calling convention of these helpers (well, we shouldn't be changing the arguments to completion functions lightly anyway---third-party completion functions can be called from __git_complete_command, if I am reading the code correctly, and we cannot update them all even if we wanted to). And most subcommands that do not care where on the command line the subcommand name is won't have to change anything. > That being said, passing in the index to all functions would definitely > be doable. I can work on a series in the future that passes in the index > of the command so that working with $cword is more robust but I'd prefer > if that were handled outside this series to keep it focused. If the breakage of "stash branch" were a serious show-stopper bug that needs to be fixed right away, I would agree that a band-aid solution that would work most of the time would be fine, but I didn't get an impression that it is so urgent and we can afford to fix it right this time, together with the other completion that share the same problem (you mentioned _git_worktree IIRC). Thanks.
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index fe79f6b71c..da46f46e3c 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -3016,6 +3016,9 @@ _git_stash () local save_opts='--all --keep-index --no-keep-index --quiet --patch --include-untracked' local subcommands='push list show apply clear drop pop create branch' local subcommand="$(__git_find_on_cmdline "$subcommands save")" + local stash_idx="$(__git_find_on_cmdline --show-idx stash)" + stash_idx="${stash_idx% *}" + if [ -z "$subcommand" -a -n "$(__git_find_on_cmdline "-p")" ]; then subcommand="push" fi @@ -3060,7 +3063,7 @@ _git_stash () branch,--*) ;; branch,*) - if [ $cword -eq 3 ]; then + if [ $((cword - stash_idx)) -eq 2 ]; then __git_complete_refs else __gitcomp_nl "$(__git stash list \
When completions are offered for `git stash branch<TAB>`, the user is supposed to receive refs. This works in the case where the main git command is called without arguments but if options are provided, such as `git -C dir stash branch<TAB>`, then the `$cword -eq 3` provides incorrect results. Count the words relative to the first instance of "stash" so that we ignore arguments to the main git command. Unfortunately, this still does not work 100% correctly. For example, in the case of something like `git -C stash stash branch<TAB>`, this will incorrectly identify the first "stash" as the command. This seems to be an edge-case that we can ignore, though, as other functions, such as _git_worktree(), suffer from the same problem. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- contrib/completion/git-completion.bash | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)