Re: [RESEND PATCH 2/3] git-completion.bash: fix `git <args>... stash branch` bug

2 messages, 2 authors, 2021-03-19 · open the first message on its own page

Re: [RESEND PATCH 2/3] git-completion.bash: fix `git <args>... stash branch` bug

From: Junio C Hamano <hidden>
Date: 2021-03-18 20:31:46

Denton Liu [off-list ref] 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?
quoted hunk
Signed-off-by: Denton Liu <redacted>
---
 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 \

Re: [RESEND PATCH 2/3] git-completion.bash: fix `git <args>... stash branch` bug

From: Denton Liu <hidden>
Date: 2021-03-19 08:06:02

Hi Junio,

On Thu, Mar 18, 2021 at 01:30:38PM -0700, Junio C Hamano wrote:
Denton Liu [off-list ref] writes:
quoted
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help