git-completion.bash: improvements to _git_stash()

This series modernises the _git_stash() completion handler by letting it take advantage of __gitcomp_builtin(). Also, it fixes a bug with how it offers completions when arguments are provided to the main git command.

Denton Liu (3):
git-completion.bash: extract from else in _git_stash() git-completion.bash: fix git <args>... stash branch bug git-completion.bash: use __gitcomp_builtin() in _git_stash()

 contrib/completion/git-completion.bash | 103 +++++++++++++------------
 1 file changed, 52 insertions(+), 51 deletions(-)

2.31.0.rc2.261.g7f71774620

To save a level of indentation, perform an early return in the "if" arm so we can move the "else" code out of the block.

---
 contrib/completion/git-completion.bash | 73 +++++++++++++-------------
 1 file changed, 37 insertions(+), 36 deletions(-)

index 7dc6cd8eb8..fe79f6b71c 100644
@@ -3035,44 +3035,45 @@ _git_stash ()
 			fi
 			;;
 		esac
-	else
-		case "$subcommand,$cur" in
-		push,--*)
-			__gitcomp "$save_opts --message"
-			;;
-		save,--*)
-			__gitcomp "$save_opts"
-			;;
-		apply,--*|pop,--*)
-			__gitcomp "--index --quiet"
-			;;
-		drop,--*)
-			__gitcomp "--quiet"
-			;;
-		list,--*)
-			__gitcomp "--name-status --oneline --patch-with-stat"
-			;;
-		show,--*)
-			__gitcomp "$__git_diff_common_options"
-			;;
-		branch,--*)
-			;;
-		branch,*)
-			if [ $cword -eq 3 ]; then
-				__git_complete_refs
-			else
-				__gitcomp_nl "$(__git stash list \
-						| sed -n -e 's/:.*//p')"
-			fi
-			;;
-		show,*|apply,*|drop,*|pop,*)
+		return
+	fi
+
+	case "$subcommand,$cur" in
+	push,--*)
+		__gitcomp "$save_opts --message"
+		;;
+	save,--*)
+		__gitcomp "$save_opts"
+		;;
+	apply,--*|pop,--*)
+		__gitcomp "--index --quiet"
+		;;
+	drop,--*)
+		__gitcomp "--quiet"
+		;;
+	list,--*)
+		__gitcomp "--name-status --oneline --patch-with-stat"
+		;;
+	show,--*)
+		__gitcomp "$__git_diff_common_options"
+		;;
+	branch,--*)
+		;;
+	branch,*)
+		if [ $cword -eq 3 ]; then
+			__git_complete_refs
+		else
 			__gitcomp_nl "$(__git stash list \
 					| sed -n -e 's/:.*//p')"
-			;;
-		*)
-			;;
-		esac
-	fi
+		fi
+		;;
+	show,*|apply,*|drop,*|pop,*)
+		__gitcomp_nl "$(__git stash list \
+				| sed -n -e 's/:.*//p')"
+		;;
+	*)
+		;;
+	esac
 }
 
 _git_submodule ()

2.31.0.rc2.261.g7f71774620

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.

---
 contrib/completion/git-completion.bash | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index fe79f6b71c..da46f46e3c 100644
@@ -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 \

2.31.0.rc2.261.g7f71774620

The completion for 'git stash' has not changed in a major way since it was converted from shell script to builtin. Now that it's a builtin, we can take advantage of the groundwork laid out by parse-options and use the generated options.

Rewrite _git_stash() to take use __gitcomp_builtin() to generate completions for subcommands.

The main git stash command does not take any arguments directly. If no subcommand is given, it automatically defaults to git stash push. This means that we can simplify the logic for when no subcommands have been given yet. We only have to offer subcommand completions when we're completing the word after "stash". Unfortunately, this does not work 100% correctly. For example, in the case of something like git -C stash stash<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.

One area that this patch could improve upon is that the git stash list command accepts log-options. It would be nice if the completion for this were unified with that of _git_log() and _git_show() which would allow completions to be provided for options such as --pretty but that is outside the scope of this patch.

---
 contrib/completion/git-completion.bash | 41 ++++++++++++--------------
 1 file changed, 19 insertions(+), 22 deletions(-)

index da46f46e3c..83b6415b13 100644
@@ -3013,29 +3013,21 @@ _git_sparse_checkout ()
 
 _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
 	if [ -z "$subcommand" ]; then
-		case "$cur" in
-		--*)
-			__gitcomp "$save_opts"
+		case "$((cword - stash_idx)),$cur" in
+		*,--*)
+			__gitcomp_builtin stash_push
 			;;
-		sa*)
-			if [ -z "$(__git_find_on_cmdline "$save_opts")" ]; then
-				__gitcomp "save"
-			fi
+		1,sa*)
+			__gitcomp "save"
 			;;
-		*)
-			if [ -z "$(__git_find_on_cmdline "$save_opts")" ]; then
-				__gitcomp "$subcommands"
-			fi
+		1,*)
+			__gitcomp "$subcommands"
 			;;
 		esac
 		return

@@ -3043,24 +3035,29 @@ _git_stash ()
 
 	case "$subcommand,$cur" in
 	push,--*)
-		__gitcomp "$save_opts --message"
+		__gitcomp_builtin stash_push
 		;;
 	save,--*)
-		__gitcomp "$save_opts"
+		__gitcomp_builtin stash_save
 		;;
-	apply,--*|pop,--*)
-		__gitcomp "--index --quiet"
+	pop,--*)
+		__gitcomp_builtin stash_pop
+		;;
+	apply,--*)
+		__gitcomp_builtin stash_apply
 		;;
 	drop,--*)
-		__gitcomp "--quiet"
+		__gitcomp_builtin stash_drop
 		;;
 	list,--*)
-		__gitcomp "--name-status --oneline --patch-with-stat"
+		# NEEDSWORK: can we somehow unify this with the options in _git_log() and _git_show()
+		__gitcomp_builtin stash_list "$__git_log_common_options $__git_diff_common_options"
 		;;
 	show,--*)
-		__gitcomp "$__git_diff_common_options"
+		__gitcomp_builtin stash_show "$__git_diff_common_options"
 		;;
 	branch,--*)
+		__gitcomp_builtin stash_branch
 		;;
 	branch,*)
 		if [ $((cword - stash_idx)) -eq 2 ]; then

2.31.0.rc2.261.g7f71774620

This series modernises the _git_stash() completion handler by letting it take advantage of __gitcomp_builtin(). Also, it fixes a bug with how it offers completions when arguments are provided to the main git command.

Denton Liu (3):
git-completion.bash: extract from else in _git_stash() git-completion.bash: fix git <args>... stash branch bug git-completion.bash: use __gitcomp_builtin() in _git_stash()

 contrib/completion/git-completion.bash | 103 +++++++++++++------------
 1 file changed, 52 insertions(+), 51 deletions(-)

2.31.0.rc2.261.g7f71774620

To save a level of indentation, perform an early return in the "if" arm so we can move the "else" code out of the block.

---
 contrib/completion/git-completion.bash | 73 +++++++++++++-------------
 1 file changed, 37 insertions(+), 36 deletions(-)

index 7dc6cd8eb8..fe79f6b71c 100644
@@ -3035,44 +3035,45 @@ _git_stash ()
 			fi
 			;;
 		esac
-	else
-		case "$subcommand,$cur" in
-		push,--*)
-			__gitcomp "$save_opts --message"
-			;;
-		save,--*)
-			__gitcomp "$save_opts"
-			;;
-		apply,--*|pop,--*)
-			__gitcomp "--index --quiet"
-			;;
-		drop,--*)
-			__gitcomp "--quiet"
-			;;
-		list,--*)
-			__gitcomp "--name-status --oneline --patch-with-stat"
-			;;
-		show,--*)
-			__gitcomp "$__git_diff_common_options"
-			;;
-		branch,--*)
-			;;
-		branch,*)
-			if [ $cword -eq 3 ]; then
-				__git_complete_refs
-			else
-				__gitcomp_nl "$(__git stash list \
-						| sed -n -e 's/:.*//p')"
-			fi
-			;;
-		show,*|apply,*|drop,*|pop,*)
+		return
+	fi
+
+	case "$subcommand,$cur" in
+	push,--*)
+		__gitcomp "$save_opts --message"
+		;;
+	save,--*)
+		__gitcomp "$save_opts"
+		;;
+	apply,--*|pop,--*)
+		__gitcomp "--index --quiet"
+		;;
+	drop,--*)
+		__gitcomp "--quiet"
+		;;
+	list,--*)
+		__gitcomp "--name-status --oneline --patch-with-stat"
+		;;
+	show,--*)
+		__gitcomp "$__git_diff_common_options"
+		;;
+	branch,--*)
+		;;
+	branch,*)
+		if [ $cword -eq 3 ]; then
+			__git_complete_refs
+		else
 			__gitcomp_nl "$(__git stash list \
 					| sed -n -e 's/:.*//p')"
-			;;
-		*)
-			;;
-		esac
-	fi
+		fi
+		;;
+	show,*|apply,*|drop,*|pop,*)
+		__gitcomp_nl "$(__git stash list \
+				| sed -n -e 's/:.*//p')"
+		;;
+	*)
+		;;
+	esac
 }
 
 _git_submodule ()

2.31.0.rc2.261.g7f71774620

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.

---
 contrib/completion/git-completion.bash | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index fe79f6b71c..da46f46e3c 100644
@@ -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 \

2.31.0.rc2.261.g7f71774620

The completion for 'git stash' has not changed in a major way since it was converted from shell script to builtin. Now that it's a builtin, we can take advantage of the groundwork laid out by parse-options and use the generated options.

Rewrite _git_stash() to take use __gitcomp_builtin() to generate completions for subcommands.

The main git stash command does not take any arguments directly. If no subcommand is given, it automatically defaults to git stash push. This means that we can simplify the logic for when no subcommands have been given yet. We only have to offer subcommand completions when we're completing the word after "stash". Unfortunately, this does not work 100% correctly. For example, in the case of something like git -C stash stash<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.

One area that this patch could improve upon is that the git stash list command accepts log-options. It would be nice if the completion for this were unified with that of _git_log() and _git_show() which would allow completions to be provided for options such as --pretty but that is outside the scope of this patch.

---
 contrib/completion/git-completion.bash | 41 ++++++++++++--------------
 1 file changed, 19 insertions(+), 22 deletions(-)

index da46f46e3c..83b6415b13 100644
@@ -3013,29 +3013,21 @@ _git_sparse_checkout ()
 
 _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
 	if [ -z "$subcommand" ]; then
-		case "$cur" in
-		--*)
-			__gitcomp "$save_opts"
+		case "$((cword - stash_idx)),$cur" in
+		*,--*)
+			__gitcomp_builtin stash_push
 			;;
-		sa*)
-			if [ -z "$(__git_find_on_cmdline "$save_opts")" ]; then
-				__gitcomp "save"
-			fi
+		1,sa*)
+			__gitcomp "save"
 			;;
-		*)
-			if [ -z "$(__git_find_on_cmdline "$save_opts")" ]; then
-				__gitcomp "$subcommands"
-			fi
+		1,*)
+			__gitcomp "$subcommands"
 			;;
 		esac
 		return

@@ -3043,24 +3035,29 @@ _git_stash ()
 
 	case "$subcommand,$cur" in
 	push,--*)
-		__gitcomp "$save_opts --message"
+		__gitcomp_builtin stash_push
 		;;
 	save,--*)
-		__gitcomp "$save_opts"
+		__gitcomp_builtin stash_save
 		;;
-	apply,--*|pop,--*)
-		__gitcomp "--index --quiet"
+	pop,--*)
+		__gitcomp_builtin stash_pop
+		;;
+	apply,--*)
+		__gitcomp_builtin stash_apply
 		;;
 	drop,--*)
-		__gitcomp "--quiet"
+		__gitcomp_builtin stash_drop
 		;;
 	list,--*)
-		__gitcomp "--name-status --oneline --patch-with-stat"
+		# NEEDSWORK: can we somehow unify this with the options in _git_log() and _git_show()
+		__gitcomp_builtin stash_list "$__git_log_common_options $__git_diff_common_options"
 		;;
 	show,--*)
-		__gitcomp "$__git_diff_common_options"
+		__gitcomp_builtin stash_show "$__git_diff_common_options"
 		;;
 	branch,--*)
+		__gitcomp_builtin stash_branch
 		;;
 	branch,*)
 		if [ $((cword - stash_idx)) -eq 2 ]; then

2.31.0.rc2.261.g7f71774620

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?

Hmph, this comflicts with your own "stash show --include-untracked and --only-untracked" completion patch d3c7bf73bdb67, it seems. How ready is that topic for 'master'?

Hi Junio,

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

Hi Junio,

Ah, sorry I forgot to mention that it conflicts in the cover letter. The resolution can be done by just taking these changes. If you'd like, I can also rebase this series on top of 'dl/stash-show-untracked'.

In any case, unless you have any other concerns, I would declare 'dl/stash-show-untracked' ready for 'master'.

Thanks,
Denton

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.

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.

I've read dl/stash-show-untracked a few times already and agree that it would be OK after the "fixes" graduate for 'master' and also for 2.31.x maintenance track at the same time.

It would be nicer if we just rebuild on top, perhaps making the "use __gitcomp_builtin" as the first of these "improvements" patches.

Thanks.

This series modernises the _git_stash() completion handler by letting it take advantage of __gitcomp_builtin(). Also, it fixes a bug with how completions are offered when arguments are provided to the main git command.

Changes since v1:

* Fix the hardcoded $cword comparisons in a more generic way

Denton Liu (3):
git-completion.bash: pass $__git_subcommand_idx from __git_main() git-completion.bash: extract from else in _git_stash() git-completion.bash: use __gitcomp_builtin() in _git_stash()

 contrib/completion/git-completion.bash | 122 ++++++++++++-------------
 1 file changed, 60 insertions(+), 62 deletions(-)

Range-diff against v1:

-:  ---------- > 1:  e4aa3e8cd7 git-completion.bash: pass $__git_subcommand_idx from __git_main()
1:  a2d9bc4a66 ! 2:  430d5acf97 git-completion.bash: extract from else in _git_stash()
    @@ contrib/completion/git-completion.bash: _git_stash ()
     -		branch,--*)
     -			;;
     -		branch,*)
    --			if [ $cword -eq 3 ]; then
    +-			if [ $cword -eq $((__git_subcommand_idx+2)) ]; then
     -				__git_complete_refs
     -			else
     -				__gitcomp_nl "$(__git stash list \
    @@ contrib/completion/git-completion.bash: _git_stash ()
     +	branch,--*)
     +		;;
     +	branch,*)
    -+		if [ $cword -eq 3 ]; then
    ++		if [ $cword -eq $((__git_subcommand_idx+2)) ]; then
     +			__git_complete_refs
     +		else
      			__gitcomp_nl "$(__git stash list \
2:  be727d0171 < -:  ---------- git-completion.bash: fix `git <args>... stash branch` bug
3:  d6deaecc1f ! 3:  680f3a3146 git-completion.bash: use __gitcomp_builtin() in _git_stash()

@@ Commit message
subcommand is given, it automatically defaults to git stash push. This means that we can simplify the logic for when no subcommands have been given yet. We only have to offer subcommand completions when we're
- completing the word after "stash". Unfortunately, this does not work
- 100% correctly. For example, in the case of something like `git -C stash
- stash<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.
+ completing a non-option after "stash".

One area that this patch could improve upon is that the git stash list command accepts log-options. It would be nice if the completion for this
@@ contrib/completion/git-completion.bash: _git_sparse_checkout ()
- 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
++
if [ -z "$subcommand" ]; then
- case "$cur" in
- --*)
- __gitcomp "$save_opts"
-+ case "$((cword - stash_idx)),$cur" in
++ case "$((cword - __git_subcommand_idx)),$cur" in
+ *,--*)
+ __gitcomp_builtin stash_push
;;
@@ contrib/completion/git-completion.bash: _git_stash ()
+ __gitcomp_builtin stash_branch
;;
branch,*)
- if [ $((cword - stash_idx)) -eq 2 ]; then
+ if [ $cword -eq $((__git_subcommand_idx+2)) ]; then

2.31.0.rc2.261.g7f71774620

Many completion functions perform hardcoded comparisons with $cword. This fails in the case where the main git command is given arguments (e.g. git -C . bundle<TAB> would fail to complete its subcommands).

Even _git_worktree(), which uses __git_find_on_cmdline(), could still fail. With something like git -C add worktree move<TAB>, the subcommand would be incorrectly identified as "add" instead of "move".

Assign $__git_subcommand_idx in __git_main(), where the git subcommand is actually found and the corresponding completion function is called. Use this variable to replace hardcoded comparisons with $cword.

---
 contrib/completion/git-completion.bash | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

index 7dc6cd8eb8..a2f1b5e916 100644
@@ -1474,12 +1474,12 @@ _git_branch ()
 
 _git_bundle ()
 {
-	local cmd="${words[2]}"
+	local cmd="${words[__git_subcommand_idx+1]}"
 	case "$cword" in
-	2)
+	$((__git_subcommand_idx+1)))
 		__gitcomp "create list-heads verify unbundle"
 		;;
-	3)
+	$((__git_subcommand_idx+2)))
 		# looking for a file
 		;;
 	*)

@@ -1894,7 +1894,7 @@ _git_grep ()
 	esac
 
 	case "$cword,$prev" in
-	2,*|*,-*)
+	$((__git_subcommand_idx+1)),*|*,-*)
 		__git_complete_symbol && return
 		;;
 	esac

@@ -3058,7 +3058,7 @@ _git_stash ()
 		branch,--*)
 			;;
 		branch,*)
-			if [ $cword -eq 3 ]; then
+			if [ $cword -eq $((__git_subcommand_idx+2)) ]; then
 				__git_complete_refs
 			else
 				__gitcomp_nl "$(__git stash list \

@@ -3277,11 +3277,9 @@ __git_complete_worktree_paths ()
 _git_worktree ()
 {
 	local subcommands="add list lock move prune remove unlock"
-	local subcommand subcommand_idx
+	local subcommand
 
-	subcommand="$(__git_find_on_cmdline --show-idx "$subcommands")"
-	subcommand_idx="${subcommand% *}"
-	subcommand="${subcommand#* }"
+	subcommand="$(__git_find_on_cmdline "$subcommands")"
 
 	case "$subcommand,$cur" in
 	,*)

@@ -3306,7 +3304,7 @@ _git_worktree ()
 			# be either the 'add' subcommand, the unstuck
 			# argument of an option (e.g. branch for -b|-B), or
 			# the path for the new worktree.
-			if [ $cword -eq $((subcommand_idx+1)) ]; then
+			if [ $cword -eq $((__git_subcommand_idx+2)) ]; then
 				# Right after the 'add' subcommand: have to
 				# complete the path, so fall back to Bash
 				# filename completion.

@@ -3330,7 +3328,7 @@ _git_worktree ()
 		__git_complete_worktree_paths
 		;;
 	move,*)
-		if [ $cword -eq $((subcommand_idx+1)) ]; then
+		if [ $cword -eq $((__git_subcommand_idx+2)) ]; then
 			# The first parameter must be an existing working
 			# tree to be moved.
 			__git_complete_worktree_paths

@@ -3398,6 +3396,7 @@ __git_main ()
 {
 	local i c=1 command __git_dir __git_repo_path
 	local __git_C_args C_args_count=0
+	local __git_subcommand_idx
 
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"

@@ -3412,7 +3411,7 @@ __git_main ()
 			__git_C_args[C_args_count++]="${words[c]}"
 			;;
 		-*) ;;
-		*) command="$i"; break ;;
+		*) command="$i"; __git_subcommand_idx="$c"; break ;;
 		esac
 		((c++))
 	done

2.31.0.rc2.261.g7f71774620

To save a level of indentation, perform an early return in the "if" arm so we can move the "else" code out of the block.

---
 contrib/completion/git-completion.bash | 73 +++++++++++++-------------
 1 file changed, 37 insertions(+), 36 deletions(-)

index a2f1b5e916..8d4d8cc0fe 100644
@@ -3035,44 +3035,45 @@ _git_stash ()
 			fi
 			;;
 		esac
-	else
-		case "$subcommand,$cur" in
-		push,--*)
-			__gitcomp "$save_opts --message"
-			;;
-		save,--*)
-			__gitcomp "$save_opts"
-			;;
-		apply,--*|pop,--*)
-			__gitcomp "--index --quiet"
-			;;
-		drop,--*)
-			__gitcomp "--quiet"
-			;;
-		list,--*)
-			__gitcomp "--name-status --oneline --patch-with-stat"
-			;;
-		show,--*)
-			__gitcomp "$__git_diff_common_options"
-			;;
-		branch,--*)
-			;;
-		branch,*)
-			if [ $cword -eq $((__git_subcommand_idx+2)) ]; then
-				__git_complete_refs
-			else
-				__gitcomp_nl "$(__git stash list \
-						| sed -n -e 's/:.*//p')"
-			fi
-			;;
-		show,*|apply,*|drop,*|pop,*)
+		return
+	fi
+
+	case "$subcommand,$cur" in
+	push,--*)
+		__gitcomp "$save_opts --message"
+		;;
+	save,--*)
+		__gitcomp "$save_opts"
+		;;
+	apply,--*|pop,--*)
+		__gitcomp "--index --quiet"
+		;;
+	drop,--*)
+		__gitcomp "--quiet"
+		;;
+	list,--*)
+		__gitcomp "--name-status --oneline --patch-with-stat"
+		;;
+	show,--*)
+		__gitcomp "$__git_diff_common_options"
+		;;
+	branch,--*)
+		;;
+	branch,*)
+		if [ $cword -eq $((__git_subcommand_idx+2)) ]; then
+			__git_complete_refs
+		else
 			__gitcomp_nl "$(__git stash list \
 					| sed -n -e 's/:.*//p')"
-			;;
-		*)
-			;;
-		esac
-	fi
+		fi
+		;;
+	show,*|apply,*|drop,*|pop,*)
+		__gitcomp_nl "$(__git stash list \
+				| sed -n -e 's/:.*//p')"
+		;;
+	*)
+		;;
+	esac
 }
 
 _git_submodule ()

2.31.0.rc2.261.g7f71774620

The completion for 'git stash' has not changed in a major way since it was converted from shell script to builtin. Now that it's a builtin, we can take advantage of the groundwork laid out by parse-options and use the generated options.

Rewrite _git_stash() to take use __gitcomp_builtin() to generate completions for subcommands.

The main git stash command does not take any arguments directly. If no subcommand is given, it automatically defaults to git stash push. This means that we can simplify the logic for when no subcommands have been given yet. We only have to offer subcommand completions when we're completing a non-option after "stash".

One area that this patch could improve upon is that the git stash list command accepts log-options. It would be nice if the completion for this were unified with that of _git_log() and _git_show() which would allow completions to be provided for options such as --pretty but that is outside the scope of this patch.

---
 contrib/completion/git-completion.bash | 42 ++++++++++++--------------
 1 file changed, 20 insertions(+), 22 deletions(-)

index 8d4d8cc0fe..c926ca26c6 100644
@@ -3013,26 +3013,19 @@ _git_sparse_checkout ()
 
 _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")"
-	if [ -z "$subcommand" -a -n "$(__git_find_on_cmdline "-p")" ]; then
-		subcommand="push"
-	fi
+
 	if [ -z "$subcommand" ]; then
-		case "$cur" in
-		--*)
-			__gitcomp "$save_opts"
+		case "$((cword - __git_subcommand_idx)),$cur" in
+		*,--*)
+			__gitcomp_builtin stash_push
 			;;
-		sa*)
-			if [ -z "$(__git_find_on_cmdline "$save_opts")" ]; then
-				__gitcomp "save"
-			fi
+		1,sa*)
+			__gitcomp "save"
 			;;
-		*)
-			if [ -z "$(__git_find_on_cmdline "$save_opts")" ]; then
-				__gitcomp "$subcommands"
-			fi
+		1,*)
+			__gitcomp "$subcommands"
 			;;
 		esac
 		return

@@ -3040,24 +3033,29 @@ _git_stash ()
 
 	case "$subcommand,$cur" in
 	push,--*)
-		__gitcomp "$save_opts --message"
+		__gitcomp_builtin stash_push
 		;;
 	save,--*)
-		__gitcomp "$save_opts"
+		__gitcomp_builtin stash_save
 		;;
-	apply,--*|pop,--*)
-		__gitcomp "--index --quiet"
+	pop,--*)
+		__gitcomp_builtin stash_pop
+		;;
+	apply,--*)
+		__gitcomp_builtin stash_apply
 		;;
 	drop,--*)
-		__gitcomp "--quiet"
+		__gitcomp_builtin stash_drop
 		;;
 	list,--*)
-		__gitcomp "--name-status --oneline --patch-with-stat"
+		# NEEDSWORK: can we somehow unify this with the options in _git_log() and _git_show()
+		__gitcomp_builtin stash_list "$__git_log_common_options $__git_diff_common_options"
 		;;
 	show,--*)
-		__gitcomp "$__git_diff_common_options"
+		__gitcomp_builtin stash_show "$__git_diff_common_options"
 		;;
 	branch,--*)
+		__gitcomp_builtin stash_branch
 		;;
 	branch,*)
 		if [ $cword -eq $((__git_subcommand_idx+2)) ]; then

2.31.0.rc2.261.g7f71774620

Nit: I don't like the word "pass" in the subject line, because you don't actually "pass" that variable as a parameter, but simly set it, and it will be visible in all called functions, because that's how shell variables work.

It's not just the hardcoded comparison with $cword, but the hardcoded indices into the $words array that causes these problems:

In 'git -C add worktree move this there' we invoke the 'worktree' command's 'move' subcommand. Therefore, this variable should be called $__git_command_idx. Or perhaps $__git_cmd_idx, to spare some keystrokes without sacrificing readability?

This patch leaves a couple of accesses to $words and $cword unchanged, though they still suffer from the same issues and should be changed, e.g.:

__git_complete_remote_or_refspec() assumes that ${words[1]} is the command and starts looking for options starting at index 2, so e.g. 'git fetch <TAB>' lists configured remotes, but 'git -C . fetch <TAB>' doesn't.

_git_branch() is curious, because, just like the "main" 'git' command, 'git branch' has '-c' and '-C' options, and as a result 'git branch o<TAB>' lists branches from 'origin', but 'git -c foo.bar=baz -C . branch o<TAB>' doesn't.

It's debatable whether __git_find_on_cmdline() and its friends should be changed. If we only look at the function's name, then no, because it implicitly implies that it searches through the whole command line. However, if we look at how we actually use it, then we'll find that we only use it to check for the presence of subcommands or certain options of a command or subcommand. This means that we only want to search the words following the command, but since it starts its scan at ${words[1]}, it leads to that issue with 'git worktree' that you described in the commit message, but it affects all other commands with subcommands as well.

I haven't looked closely at the other cases, but I'm inclinened to think that all _git_cmd() functions and any helper functions invoked by them should only concern themselves with words after the git command.