# [PATCH 0/5] git-completion.bash: fixes on top of 'dl/complete-stash'

18 messages from 2021-04-20 to 2021-04-22. Participants: Denton Liu, Ævar Arnfjörð Bjarmason, Junio C Hamano, SZEDER Gábor.
Thread: https://gitlist.dev/t/55527

## Denton Liu, 2021-04-20 09:19

Subject: [PATCH 0/5] git-completion.bash: fixes on top of 'dl/complete-stash'
Message-ID: <cover.1618910364.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/cover.1618910364.git.liu.denton%40gmail.com

```
Gábor pointed out some potential fixes what's currently queued in
'dl/complete-stash'[0][1][2][3]. This series addresses those concerns.

Sorry for the long wait, I've been quite busy over the past month.

[0]: https://lore.kernel.org/git/20210327183554.GD2271@szeder.dev/
[1]: https://lore.kernel.org/git/20210328103134.GF2271@szeder.dev/
[2]: https://lore.kernel.org/git/20210328103057.GE2271@szeder.dev/
[3]: https://lore.kernel.org/git/20210328110427.GG2271@szeder.dev/

Denton Liu (5):
  git-completion.bash: separate some commands onto their own line
  git-completion.bash: rename to $__git_cmd_idx
  git-completion.bash: use $__git_cmd_idx in more places
  git-completion.bash: consolidate cases in _git_stash()
  git-completion.bash: consolidate no-subcommand case for _git_stash()

 contrib/completion/git-completion.bash | 118 +++++++++++++------------
 t/t9902-completion.sh                  |  19 ++++
 2 files changed, 80 insertions(+), 57 deletions(-)

-- 
2.31.1.424.g95a8dafae5


```

## Denton Liu, 2021-04-20 09:19

Subject: [PATCH 1/5] git-completion.bash: separate some commands onto their own line
Message-ID: <65c485ea0c1726c96d292d7d7385def8d2f19f25.1618910364.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/65c485ea0c1726c96d292d7d7385def8d2f19f25.1618910364.git.liu.denton%40gmail.com
In-Reply-To: <cover.1618910364.git.liu.denton@gmail.com>

```
In e94fb44042 (git-completion.bash: pass $__git_subcommand_idx from
__git_main(), 2021-03-24), a line was introduced which contained
multiple statements. This is difficult to read so break it into multiple
lines.

While we're at it, follow this convention for the rest of the
__git_main() and break up lines that contain multiple statements.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 contrib/completion/git-completion.bash | 37 +++++++++++++++++++-------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c926ca26c6..1dedb14b47 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3400,17 +3400,35 @@ __git_main ()
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
 		case "$i" in
-		--git-dir=*) __git_dir="${i#--git-dir=}" ;;
-		--git-dir)   ((c++)) ; __git_dir="${words[c]}" ;;
-		--bare)      __git_dir="." ;;
-		--help) command="help"; break ;;
-		-c|--work-tree|--namespace) ((c++)) ;;
-		-C)	__git_C_args[C_args_count++]=-C
+		--git-dir=*)
+			__git_dir="${i#--git-dir=}"
+			;;
+		--git-dir)
+			((c++))
+			__git_dir="${words[c]}"
+			;;
+		--bare)
+			__git_dir="."
+			;;
+		--help)
+			command="help"
+			break
+			;;
+		-c|--work-tree|--namespace)
+			((c++))
+			;;
+		-C)
+			__git_C_args[C_args_count++]=-C
 			((c++))
 			__git_C_args[C_args_count++]="${words[c]}"
 			;;
-		-*) ;;
-		*) command="$i"; __git_subcommand_idx="$c"; break ;;
+		-*)
+			;;
+		*)
+			command="$i"
+			__git_subcommand_idx="$c"
+			break
+			;;
 		esac
 		((c++))
 	done
@@ -3432,7 +3450,8 @@ __git_main ()
 			;;
 		esac
 		case "$cur" in
-		--*)   __gitcomp "
+		--*)
+			__gitcomp "
 			--paginate
 			--no-pager
 			--git-dir=
-- 
2.31.1.424.g95a8dafae5


```

## Denton Liu, 2021-04-20 09:19

Subject: [PATCH 2/5] git-completion.bash: rename to $__git_cmd_idx
Message-ID: <7c7d6de380d1aaa860e86627778bef6c0c78ca08.1618910364.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/7c7d6de380d1aaa860e86627778bef6c0c78ca08.1618910364.git.liu.denton%40gmail.com
In-Reply-To: <cover.1618910364.git.liu.denton@gmail.com>

```
In e94fb44042 (git-completion.bash: pass $__git_subcommand_idx from
__git_main(), 2021-03-24), the $__git_subcommand_idx variable was
introduced. Naming it after the index of the subcommand is flat-out
wrong as this variable really holds the index of the git comand (e.g.
"stash").

Rename this variable so that it's obvious it's about git commands. While
we're at it, shorten up its name so that it's still readable without
being a handful to type.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 contrib/completion/git-completion.bash | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1dedb14b47..c29c129f87 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1474,12 +1474,12 @@ _git_branch ()
 
 _git_bundle ()
 {
-	local cmd="${words[__git_subcommand_idx+1]}"
+	local cmd="${words[__git_cmd_idx+1]}"
 	case "$cword" in
-	$((__git_subcommand_idx+1)))
+	$((__git_cmd_idx+1)))
 		__gitcomp "create list-heads verify unbundle"
 		;;
-	$((__git_subcommand_idx+2)))
+	$((__git_cmd_idx+2)))
 		# looking for a file
 		;;
 	*)
@@ -1894,7 +1894,7 @@ _git_grep ()
 	esac
 
 	case "$cword,$prev" in
-	$((__git_subcommand_idx+1)),*|*,-*)
+	$((__git_cmd_idx+1)),*|*,-*)
 		__git_complete_symbol && return
 		;;
 	esac
@@ -3017,7 +3017,7 @@ _git_stash ()
 	local subcommand="$(__git_find_on_cmdline "$subcommands save")"
 
 	if [ -z "$subcommand" ]; then
-		case "$((cword - __git_subcommand_idx)),$cur" in
+		case "$((cword - __git_cmd_idx)),$cur" in
 		*,--*)
 			__gitcomp_builtin stash_push
 			;;
@@ -3058,7 +3058,7 @@ _git_stash ()
 		__gitcomp_builtin stash_branch
 		;;
 	branch,*)
-		if [ $cword -eq $((__git_subcommand_idx+2)) ]; then
+		if [ $cword -eq $((__git_cmd_idx+2)) ]; then
 			__git_complete_refs
 		else
 			__gitcomp_nl "$(__git stash list \
@@ -3303,7 +3303,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 $((__git_subcommand_idx+2)) ]; then
+			if [ $cword -eq $((__git_cmd_idx+2)) ]; then
 				# Right after the 'add' subcommand: have to
 				# complete the path, so fall back to Bash
 				# filename completion.
@@ -3327,7 +3327,7 @@ _git_worktree ()
 		__git_complete_worktree_paths
 		;;
 	move,*)
-		if [ $cword -eq $((__git_subcommand_idx+2)) ]; then
+		if [ $cword -eq $((__git_cmd_idx+2)) ]; then
 			# The first parameter must be an existing working
 			# tree to be moved.
 			__git_complete_worktree_paths
@@ -3395,7 +3395,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
+	local __git_cmd_idx
 
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
@@ -3426,7 +3426,7 @@ __git_main ()
 			;;
 		*)
 			command="$i"
-			__git_subcommand_idx="$c"
+			__git_cmd_idx="$c"
 			break
 			;;
 		esac
-- 
2.31.1.424.g95a8dafae5


```

## Denton Liu, 2021-04-20 09:19

Subject: [PATCH 3/5] git-completion.bash: use $__git_cmd_idx in more places
Message-ID: <63a699258545d55e47d9f6fcf6e70055a76e8949.1618910364.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/63a699258545d55e47d9f6fcf6e70055a76e8949.1618910364.git.liu.denton%40gmail.com
In-Reply-To: <cover.1618910364.git.liu.denton@gmail.com>

```
With the introduction of the $__git_cmd_idx variable in e94fb44042
(git-completion.bash: pass $__git_subcommand_idx from __git_main(),
2021-03-24), completion functions were able to know the index at which
the git command is listed, allowing them to skip options that are given
to the underlying git itself, not the corresponding command (e.g.
`-C asdf` in `git -C asdf branch`).

While most of the changes here are self-explanatory, some bear further
explanation.

For the __git_find_on_cmdline() and __git_find_last_on_cmdline() pair of
functions, these functions are only ever called in the context of a git
command completion function. These functions will only care about words
after the command so we can safely ignore the words before this.

For _git_worktree(), this change is technically a no-op (once the
__git_find_last_on_cmdline change is also applied). It was in poor style
to have hard-coded on the index right after `worktree`. In case
`git worktree` were to ever learn to accept options, the current
situation would be inflexible.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 contrib/completion/git-completion.bash | 26 ++++++++++++++------------
 t/t9902-completion.sh                  | 19 +++++++++++++++++++
 2 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c29c129f87..30c9a97616 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1006,8 +1006,8 @@ __git_complete_revlist ()
 
 __git_complete_remote_or_refspec ()
 {
-	local cur_="$cur" cmd="${words[1]}"
-	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
+	local cur_="$cur" cmd="${words[__git_cmd_idx]}"
+	local i c=$((__git_cmd_idx+1)) remote="" pfx="" lhs=1 no_complete_refspec=0
 	if [ "$cmd" = "remote" ]; then
 		((c++))
 	fi
@@ -1176,7 +1176,7 @@ __git_aliased_command ()
 # --show-idx: Optionally show the index of the found word in the $words array.
 __git_find_on_cmdline ()
 {
-	local word c=1 show_idx
+	local word c="$__git_cmd_idx" show_idx
 
 	while test $# -gt 1; do
 		case "$1" in
@@ -1221,7 +1221,7 @@ __git_find_last_on_cmdline ()
 	done
 	local wordlist="$1"
 
-	while [ $c -gt 1 ]; do
+	while [ $c -gt "$__git_cmd_idx" ]; do
 		((c--))
 		for word in $wordlist; do
 			if [ "$word" = "${words[c]}" ]; then
@@ -1306,7 +1306,7 @@ __git_count_arguments ()
 	local word i c=0
 
 	# Skip "git" (first argument)
-	for ((i=1; i < ${#words[@]}; i++)); do
+	for ((i="$__git_cmd_idx"; i < ${#words[@]}; i++)); do
 		word="${words[i]}"
 
 		case "$word" in
@@ -1442,7 +1442,7 @@ __git_ref_fieldlist="refname objecttype objectsize objectname upstream push HEAD
 
 _git_branch ()
 {
-	local i c=1 only_local_ref="n" has_r="n"
+	local i c="$__git_cmd_idx" only_local_ref="n" has_r="n"
 
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
@@ -2474,7 +2474,7 @@ _git_switch ()
 __git_config_get_set_variables ()
 {
 	local prevword word config_file= c=$cword
-	while [ $c -gt 1 ]; do
+	while [ $c -gt "$__git_cmd_idx" ]; do
 		word="${words[c]}"
 		case "$word" in
 		--system|--global|--local|--file=*)
@@ -3224,7 +3224,7 @@ _git_svn ()
 
 _git_tag ()
 {
-	local i c=1 f=0
+	local i c="$__git_cmd_idx" f=0
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
 		case "$i" in
@@ -3276,9 +3276,11 @@ __git_complete_worktree_paths ()
 _git_worktree ()
 {
 	local subcommands="add list lock move prune remove unlock"
-	local subcommand
+	local subcommand subcommand_idx
 
-	subcommand="$(__git_find_on_cmdline "$subcommands")"
+	subcommand="$(__git_find_on_cmdline --show-idx "$subcommands")"
+	subcommand_idx="${subcommand% *}"
+	subcommand="${subcommand#* }"
 
 	case "$subcommand,$cur" in
 	,*)
@@ -3303,7 +3305,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 $((__git_cmd_idx+2)) ]; then
+			if [ $cword -eq $((subcommand_idx+1)) ]; then
 				# Right after the 'add' subcommand: have to
 				# complete the path, so fall back to Bash
 				# filename completion.
@@ -3327,7 +3329,7 @@ _git_worktree ()
 		__git_complete_worktree_paths
 		;;
 	move,*)
-		if [ $cword -eq $((__git_cmd_idx+2)) ]; then
+		if [ $cword -eq $((subcommand_idx+1)) ]; then
 			# The first parameter must be an existing working
 			# tree to be moved.
 			__git_complete_worktree_paths
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 04ce884ef5..9439fec8f0 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1879,6 +1879,7 @@ test_expect_success '__git_find_on_cmdline - single match' '
 	(
 		words=(git command --opt list) &&
 		cword=${#words[@]} &&
+		__git_cmd_idx=1 &&
 		__git_find_on_cmdline "add list remove" >actual
 	) &&
 	test_cmp expect actual
@@ -1889,6 +1890,7 @@ test_expect_success '__git_find_on_cmdline - multiple matches' '
 	(
 		words=(git command -o --opt remove list add) &&
 		cword=${#words[@]} &&
+		__git_cmd_idx=1 &&
 		__git_find_on_cmdline "add list remove" >actual
 	) &&
 	test_cmp expect actual
@@ -1898,6 +1900,7 @@ test_expect_success '__git_find_on_cmdline - no match' '
 	(
 		words=(git command --opt branch) &&
 		cword=${#words[@]} &&
+		__git_cmd_idx=1 &&
 		__git_find_on_cmdline "add list remove" >actual
 	) &&
 	test_must_be_empty actual
@@ -1908,6 +1911,7 @@ test_expect_success '__git_find_on_cmdline - single match with index' '
 	(
 		words=(git command --opt list) &&
 		cword=${#words[@]} &&
+		__git_cmd_idx=1 &&
 		__git_find_on_cmdline --show-idx "add list remove" >actual
 	) &&
 	test_cmp expect actual
@@ -1918,6 +1922,7 @@ test_expect_success '__git_find_on_cmdline - multiple matches with index' '
 	(
 		words=(git command -o --opt remove list add) &&
 		cword=${#words[@]} &&
+		__git_cmd_idx=1 &&
 		__git_find_on_cmdline --show-idx "add list remove" >actual
 	) &&
 	test_cmp expect actual
@@ -1927,11 +1932,23 @@ test_expect_success '__git_find_on_cmdline - no match with index' '
 	(
 		words=(git command --opt branch) &&
 		cword=${#words[@]} &&
+		__git_cmd_idx=1 &&
 		__git_find_on_cmdline --show-idx "add list remove" >actual
 	) &&
 	test_must_be_empty actual
 '
 
+test_expect_success '__git_find_on_cmdline - ignores matches before command with index' '
+	echo "6 remove" >expect &&
+	(
+		words=(git -C remove command -o --opt remove list add) &&
+		cword=${#words[@]} &&
+		__git_cmd_idx=3 &&
+		__git_find_on_cmdline --show-idx "add list remove" >actual
+	) &&
+	test_cmp expect actual
+'
+
 test_expect_success '__git_get_config_variables' '
 	cat >expect <<-EOF &&
 	name-1
@@ -2275,6 +2292,7 @@ do
 		(
 			words=(git push '$flag' other ma) &&
 			cword=${#words[@]} cur=${words[cword-1]} &&
+			__git_cmd_idx=1 &&
 			__git_complete_remote_or_refspec &&
 			print_comp
 		) &&
@@ -2288,6 +2306,7 @@ do
 		(
 			words=(git push other '$flag' ma) &&
 			cword=${#words[@]} cur=${words[cword-1]} &&
+			__git_cmd_idx=1 &&
 			__git_complete_remote_or_refspec &&
 			print_comp
 		) &&
-- 
2.31.1.424.g95a8dafae5


```

## Denton Liu, 2021-04-20 09:19

Subject: [PATCH 4/5] git-completion.bash: consolidate cases in _git_stash()
Message-ID: <4f8d015d54376af277883f57e8b4cf2c63ed8a03.1618910364.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/4f8d015d54376af277883f57e8b4cf2c63ed8a03.1618910364.git.liu.denton%40gmail.com
In-Reply-To: <cover.1618910364.git.liu.denton@gmail.com>

```
The $subcommand case statement in _git_stash() is quite repetitive.
Consolidate the cases together into one catch-all case to reduce the
repetition.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 contrib/completion/git-completion.bash | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 30c9a97616..7bce9a0112 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3032,21 +3032,6 @@ _git_stash ()
 	fi
 
 	case "$subcommand,$cur" in
-	push,--*)
-		__gitcomp_builtin stash_push
-		;;
-	save,--*)
-		__gitcomp_builtin stash_save
-		;;
-	pop,--*)
-		__gitcomp_builtin stash_pop
-		;;
-	apply,--*)
-		__gitcomp_builtin stash_apply
-		;;
-	drop,--*)
-		__gitcomp_builtin stash_drop
-		;;
 	list,--*)
 		# 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"
@@ -3054,8 +3039,8 @@ _git_stash ()
 	show,--*)
 		__gitcomp_builtin stash_show "$__git_diff_common_options"
 		;;
-	branch,--*)
-		__gitcomp_builtin stash_branch
+	*,--*)
+		__gitcomp_builtin "stash_$subcommand"
 		;;
 	branch,*)
 		if [ $cword -eq $((__git_cmd_idx+2)) ]; then
@@ -3069,8 +3054,6 @@ _git_stash ()
 		__gitcomp_nl "$(__git stash list \
 				| sed -n -e 's/:.*//p')"
 		;;
-	*)
-		;;
 	esac
 }
 
-- 
2.31.1.424.g95a8dafae5


```

## Denton Liu, 2021-04-20 09:19

Subject: [PATCH 5/5] git-completion.bash: consolidate no-subcommand case for _git_stash()
Message-ID: <b4a9b0afa7ab28b701499982f5a8fc66eb7e19e8.1618910364.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/b4a9b0afa7ab28b701499982f5a8fc66eb7e19e8.1618910364.git.liu.denton%40gmail.com
In-Reply-To: <cover.1618910364.git.liu.denton@gmail.com>

```
We have a separate if case for when no subcommand is given. It is
simpler to just consolidate this logic into the case statement below.

It would be nice to complete remove the magic that deals with indices
and replace it with what was originally there,

	if [ -z "$subcommand" -a -n "$(__git_find_on_cmdline "-p")" ]; then
	        subcommand="push"
	fi

but this gives a slightly incorrect completion. In the case where we're
attempting to complete `git stash -a <TAB>` we will get the subcommands
back as a respose instead of the completions for `git stash push`, which
is what we'd expect. We could potentially hardcode all of the short
options but that would be too much work to maintain so we stick with the
index solution.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 contrib/completion/git-completion.bash | 30 +++++++++++++-------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 7bce9a0112..060adc0ed7 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3016,22 +3016,22 @@ _git_stash ()
 	local subcommands='push list show apply clear drop pop create branch'
 	local subcommand="$(__git_find_on_cmdline "$subcommands save")"
 
-	if [ -z "$subcommand" ]; then
-		case "$((cword - __git_cmd_idx)),$cur" in
-		*,--*)
-			__gitcomp_builtin stash_push
-			;;
-		1,sa*)
-			__gitcomp "save"
-			;;
-		1,*)
-			__gitcomp "$subcommands"
-			;;
-		esac
-		return
-	fi
-
 	case "$subcommand,$cur" in
+	,--*)
+		__gitcomp_builtin stash_save
+		;;
+	,sa*)
+		__git_init_builtin_opts stash_save
+		if ((cword - __git_cmd_idx == 1)); then
+			__gitcomp "save"
+		fi
+		;;
+	,*)
+		__git_init_builtin_opts stash_save
+		if ((cword - __git_cmd_idx == 1)); then
+			__gitcomp "$subcommands"
+		fi
+		;;
 	list,--*)
 		# 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"
-- 
2.31.1.424.g95a8dafae5


```

## Ævar Arnfjörð Bjarmason, 2021-04-20 10:44

Subject: Re: [PATCH 4/5] git-completion.bash: consolidate cases in _git_stash()
Message-ID: <877dkx44ma.fsf@evledraar.gmail.com>
URL: https://gitlist.dev/e/877dkx44ma.fsf%40evledraar.gmail.com
In-Reply-To: <4f8d015d54376af277883f57e8b4cf2c63ed8a03.1618910364.git.liu.denton@gmail.com>

```

On Tue, Apr 20 2021, Denton Liu wrote:

> The $subcommand case statement in _git_stash() is quite repetitive.
> Consolidate the cases together into one catch-all case to reduce the
> repetition.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 21 ++-------------------
>  1 file changed, 2 insertions(+), 19 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 30c9a97616..7bce9a0112 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -3032,21 +3032,6 @@ _git_stash ()
>  	fi
>  
>  	case "$subcommand,$cur" in
> -	push,--*)
> -		__gitcomp_builtin stash_push
> -		;;
> -	save,--*)
> -		__gitcomp_builtin stash_save
> -		;;
> -	pop,--*)
> -		__gitcomp_builtin stash_pop
> -		;;
> -	apply,--*)
> -		__gitcomp_builtin stash_apply
> -		;;
> -	drop,--*)
> -		__gitcomp_builtin stash_drop
> -		;;
>  	list,--*)
>  		# 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"
> @@ -3054,8 +3039,8 @@ _git_stash ()
>  	show,--*)
>  		__gitcomp_builtin stash_show "$__git_diff_common_options"
>  		;;
> -	branch,--*)
> -		__gitcomp_builtin stash_branch
> +	*,--*)
> +		__gitcomp_builtin "stash_$subcommand"
>  		;;
>  	branch,*)
>  		if [ $cword -eq $((__git_cmd_idx+2)) ]; then
> @@ -3069,8 +3054,6 @@ _git_stash ()
>  		__gitcomp_nl "$(__git stash list \
>  				| sed -n -e 's/:.*//p')"
>  		;;
> -	*)
> -		;;
>  	esac
>  }

One might think that this introduces a logic error in "git stash
doesnotexist" now dispatching to a non-existing "stash_doesnotexist" or
something, bu tI see that earlier (omitted from context) in the function
there's an exhaustive lit of push/save/pop etc. which guards against
this, is is that correct?

```

## Junio C Hamano, 2021-04-20 20:50

Subject: Re: [PATCH 2/5] git-completion.bash: rename to $__git_cmd_idx
Message-ID: <xmqqr1j44r5g.fsf@gitster.g>
URL: https://gitlist.dev/e/xmqqr1j44r5g.fsf%40gitster.g
In-Reply-To: <7c7d6de380d1aaa860e86627778bef6c0c78ca08.1618910364.git.liu.denton@gmail.com>

```
Denton Liu <liu.denton@gmail.com> writes:

> In e94fb44042 (git-completion.bash: pass $__git_subcommand_idx from
> __git_main(), 2021-03-24), the $__git_subcommand_idx variable was
> introduced. Naming it after the index of the subcommand is flat-out
> wrong as this variable really holds the index of the git comand (e.g.

comand -> command

> "stash").
>
> Rename this variable so that it's obvious it's about git commands. While
> we're at it, shorten up its name so that it's still readable without
> being a handful to type.

As the patch has already written, I won't complain too much, but to
many people "git" is a command and "add", "commit" etc. are
subcommands of "git", so I do not see git_subcommand_idx so wrong
that it needs to be renamed.  I do understand that it is a bit too
long and it may be easier to type if renamed to say git_cmd_idx,
though ;-)

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 1dedb14b47..c29c129f87 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1474,12 +1474,12 @@ _git_branch ()
>  
>  _git_bundle ()
>  {
> -	local cmd="${words[__git_subcommand_idx+1]}"
> +	local cmd="${words[__git_cmd_idx+1]}"
>  	case "$cword" in
> -	$((__git_subcommand_idx+1)))
> +	$((__git_cmd_idx+1)))
>  		__gitcomp "create list-heads verify unbundle"
>  		;;
> -	$((__git_subcommand_idx+2)))
> +	$((__git_cmd_idx+2)))
>  		# looking for a file
>  		;;
>  	*)
> @@ -1894,7 +1894,7 @@ _git_grep ()
>  	esac
>  
>  	case "$cword,$prev" in
> -	$((__git_subcommand_idx+1)),*|*,-*)
> +	$((__git_cmd_idx+1)),*|*,-*)
>  		__git_complete_symbol && return
>  		;;
>  	esac
> @@ -3017,7 +3017,7 @@ _git_stash ()
>  	local subcommand="$(__git_find_on_cmdline "$subcommands save")"
>  
>  	if [ -z "$subcommand" ]; then
> -		case "$((cword - __git_subcommand_idx)),$cur" in
> +		case "$((cword - __git_cmd_idx)),$cur" in
>  		*,--*)
>  			__gitcomp_builtin stash_push
>  			;;
> @@ -3058,7 +3058,7 @@ _git_stash ()
>  		__gitcomp_builtin stash_branch
>  		;;
>  	branch,*)
> -		if [ $cword -eq $((__git_subcommand_idx+2)) ]; then
> +		if [ $cword -eq $((__git_cmd_idx+2)) ]; then
>  			__git_complete_refs
>  		else
>  			__gitcomp_nl "$(__git stash list \
> @@ -3303,7 +3303,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 $((__git_subcommand_idx+2)) ]; then
> +			if [ $cword -eq $((__git_cmd_idx+2)) ]; then
>  				# Right after the 'add' subcommand: have to
>  				# complete the path, so fall back to Bash
>  				# filename completion.
> @@ -3327,7 +3327,7 @@ _git_worktree ()
>  		__git_complete_worktree_paths
>  		;;
>  	move,*)
> -		if [ $cword -eq $((__git_subcommand_idx+2)) ]; then
> +		if [ $cword -eq $((__git_cmd_idx+2)) ]; then
>  			# The first parameter must be an existing working
>  			# tree to be moved.
>  			__git_complete_worktree_paths
> @@ -3395,7 +3395,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
> +	local __git_cmd_idx
>  
>  	while [ $c -lt $cword ]; do
>  		i="${words[c]}"
> @@ -3426,7 +3426,7 @@ __git_main ()
>  			;;
>  		*)
>  			command="$i"
> -			__git_subcommand_idx="$c"
> +			__git_cmd_idx="$c"
>  			break
>  			;;
>  		esac

```

## Junio C Hamano, 2021-04-20 21:10

Subject: Re: [PATCH 5/5] git-completion.bash: consolidate no-subcommand case for _git_stash()
Message-ID: <xmqqk0ow4q7k.fsf@gitster.g>
URL: https://gitlist.dev/e/xmqqk0ow4q7k.fsf%40gitster.g
In-Reply-To: <b4a9b0afa7ab28b701499982f5a8fc66eb7e19e8.1618910364.git.liu.denton@gmail.com>

```
Denton Liu <liu.denton@gmail.com> writes:

> We have a separate if case for when no subcommand is given. It is
> simpler to just consolidate this logic into the case statement below.

Hmph, I am not quite sure if the removal of the first case is making
the code easier to follow.  Is this supposed to be a no-op clean-up,
or is it fixing some bugs?

> It would be nice to complete remove the magic that deals with indices
> and replace it with what was originally there,
>
> 	if [ -z "$subcommand" -a -n "$(__git_find_on_cmdline "-p")" ]; then
> 	        subcommand="push"
> 	fi
>
> but this gives a slightly incorrect completion. In the case where we're
> attempting to complete `git stash -a <TAB>` we will get the subcommands
> back as a respose instead of the completions for `git stash push`, which
> is what we'd expect. We could potentially hardcode all of the short
> options but that would be too much work to maintain so we stick with the
> index solution.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 30 +++++++++++++-------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 7bce9a0112..060adc0ed7 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -3016,22 +3016,22 @@ _git_stash ()
>  	local subcommands='push list show apply clear drop pop create branch'
>  	local subcommand="$(__git_find_on_cmdline "$subcommands save")"
>  
> -	if [ -z "$subcommand" ]; then
> -		case "$((cword - __git_cmd_idx)),$cur" in
> -		*,--*)
> -			__gitcomp_builtin stash_push
> -			;;
> -		1,sa*)
> -			__gitcomp "save"
> -			;;
> -		1,*)
> -			__gitcomp "$subcommands"
> -			;;
> -		esac
> -		return
> -	fi
> -
>  	case "$subcommand,$cur" in
> +	,--*)
> +		__gitcomp_builtin stash_save
> +		;;
> +	,sa*)
> +		__git_init_builtin_opts stash_save
> +		if ((cword - __git_cmd_idx == 1)); then
> +			__gitcomp "save"
> +		fi
> +		;;
> +	,*)
> +		__git_init_builtin_opts stash_save
> +		if ((cword - __git_cmd_idx == 1)); then
> +			__gitcomp "$subcommands"
> +		fi
> +		;;
>  	list,--*)
>  		# 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"

```

## SZEDER Gábor, 2021-04-20 21:14

Subject: Re: [PATCH 2/5] git-completion.bash: rename to $__git_cmd_idx
Message-ID: <20210420211432.GA3842406@szeder.dev>
URL: https://gitlist.dev/e/20210420211432.GA3842406%40szeder.dev
In-Reply-To: <xmqqr1j44r5g.fsf@gitster.g>

```
On Tue, Apr 20, 2021 at 01:50:19PM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > In e94fb44042 (git-completion.bash: pass $__git_subcommand_idx from
> > __git_main(), 2021-03-24), the $__git_subcommand_idx variable was
> > introduced. Naming it after the index of the subcommand is flat-out
> > wrong as this variable really holds the index of the git comand (e.g.
> 
> comand -> command
> 
> > "stash").
> >
> > Rename this variable so that it's obvious it's about git commands. While
> > we're at it, shorten up its name so that it's still readable without
> > being a handful to type.
> 
> As the patch has already written, I won't complain too much, but to
> many people "git" is a command and "add", "commit" etc. are
> subcommands of "git", so I do not see git_subcommand_idx so wrong
> that it needs to be renamed.  I do understand that it is a bit too
> long and it may be easier to type if renamed to say git_cmd_idx,
> though ;-)

The completion functions for git commands having subcommands usually
start like this:

    _git_remote ()
    {
        local subcommands="
                add rename remove set-head set-branches
                get-url set-url show prune update
                "
        local subcommand="$(__git_find_on_cmdline "$subcommands")"
        if [ -z "$subcommand" ]; then

__git_subcommand_idx holds the index of the word "remote", not the
index of "add/rename/etc.", so in the context of the completion script
that name is misleading.


```

## Junio C Hamano, 2021-04-20 22:31

Subject: Re: [PATCH 2/5] git-completion.bash: rename to $__git_cmd_idx
Message-ID: <xmqqo8e837vw.fsf@gitster.g>
URL: https://gitlist.dev/e/xmqqo8e837vw.fsf%40gitster.g
In-Reply-To: <20210420211432.GA3842406@szeder.dev>

```
SZEDER Gábor <szeder.dev@gmail.com> writes:

> The completion functions for git commands having subcommands usually
> start like this:
>
>     _git_remote ()
>     {
>         local subcommands="
>                 add rename remove set-head set-branches
>                 get-url set-url show prune update
>                 "
>         local subcommand="$(__git_find_on_cmdline "$subcommands")"
>         if [ -z "$subcommand" ]; then
>
> __git_subcommand_idx holds the index of the word "remote", not the
> index of "add/rename/etc.", so in the context of the completion script
> that name is misleading.

OK.  As subsubcommands would be a mouthful, calling "stash" etc. as
"cmd" would be fine.

```

## Denton Liu, 2021-04-21 04:03

Subject: Re: [PATCH 4/5] git-completion.bash: consolidate cases in _git_stash()
Message-ID: <YH+kHGLBLpPVW+88@generichostname>
URL: https://gitlist.dev/e/YH%2BkHGLBLpPVW%2B88%40generichostname
In-Reply-To: <877dkx44ma.fsf@evledraar.gmail.com>

```
Hi Ævar,

On Tue, Apr 20, 2021 at 12:44:45PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Apr 20 2021, Denton Liu wrote:
> 
> > The $subcommand case statement in _git_stash() is quite repetitive.
> > Consolidate the cases together into one catch-all case to reduce the
> > repetition.
> >
> > Signed-off-by: Denton Liu <liu.denton@gmail.com>
> > ---
> >  contrib/completion/git-completion.bash | 21 ++-------------------
> >  1 file changed, 2 insertions(+), 19 deletions(-)
> >
> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> > index 30c9a97616..7bce9a0112 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -3032,21 +3032,6 @@ _git_stash ()
> >  	fi
> >  
> >  	case "$subcommand,$cur" in
> > -	push,--*)
> > -		__gitcomp_builtin stash_push
> > -		;;
> > -	save,--*)
> > -		__gitcomp_builtin stash_save
> > -		;;
> > -	pop,--*)
> > -		__gitcomp_builtin stash_pop
> > -		;;
> > -	apply,--*)
> > -		__gitcomp_builtin stash_apply
> > -		;;
> > -	drop,--*)
> > -		__gitcomp_builtin stash_drop
> > -		;;
> >  	list,--*)
> >  		# 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"
> > @@ -3054,8 +3039,8 @@ _git_stash ()
> >  	show,--*)
> >  		__gitcomp_builtin stash_show "$__git_diff_common_options"
> >  		;;
> > -	branch,--*)
> > -		__gitcomp_builtin stash_branch
> > +	*,--*)
> > +		__gitcomp_builtin "stash_$subcommand"
> >  		;;
> >  	branch,*)
> >  		if [ $cword -eq $((__git_cmd_idx+2)) ]; then
> > @@ -3069,8 +3054,6 @@ _git_stash ()
> >  		__gitcomp_nl "$(__git stash list \
> >  				| sed -n -e 's/:.*//p')"
> >  		;;
> > -	*)
> > -		;;
> >  	esac
> >  }
> 
> One might think that this introduces a logic error in "git stash
> doesnotexist" now dispatching to a non-existing "stash_doesnotexist" or
> something, bu tI see that earlier (omitted from context) in the function
> there's an exhaustive lit of push/save/pop etc. which guards against
> this, is is that correct?

Yep, that's exactly correct.

```

## Denton Liu, 2021-04-21 04:08

Subject: Re: [PATCH 5/5] git-completion.bash: consolidate no-subcommand case for _git_stash()
Message-ID: <YH+lMZCg5cqniPaU@generichostname>
URL: https://gitlist.dev/e/YH%2BlMZCg5cqniPaU%40generichostname
In-Reply-To: <xmqqk0ow4q7k.fsf@gitster.g>

```
Hi Junio,

On Tue, Apr 20, 2021 at 02:10:39PM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > We have a separate if case for when no subcommand is given. It is
> > simpler to just consolidate this logic into the case statement below.
> 
> Hmph, I am not quite sure if the removal of the first case is making
> the code easier to follow.  Is this supposed to be a no-op clean-up,
> or is it fixing some bugs?

This is simply a no-op clean-up. I am on the fence about doing this
change as well so I can drop it on the next reroll unless someone has
objections.

> > It would be nice to complete remove the magic that deals with indices
> > and replace it with what was originally there,
> >
> > 	if [ -z "$subcommand" -a -n "$(__git_find_on_cmdline "-p")" ]; then
> > 	        subcommand="push"
> > 	fi
> >
> > but this gives a slightly incorrect completion. In the case where we're
> > attempting to complete `git stash -a <TAB>` we will get the subcommands
> > back as a respose instead of the completions for `git stash push`, which
> > is what we'd expect. We could potentially hardcode all of the short
> > options but that would be too much work to maintain so we stick with the
> > index solution.
> >
> > Signed-off-by: Denton Liu <liu.denton@gmail.com>
> > ---
> >  contrib/completion/git-completion.bash | 30 +++++++++++++-------------
> >  1 file changed, 15 insertions(+), 15 deletions(-)
> >
> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> > index 7bce9a0112..060adc0ed7 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -3016,22 +3016,22 @@ _git_stash ()
> >  	local subcommands='push list show apply clear drop pop create branch'
> >  	local subcommand="$(__git_find_on_cmdline "$subcommands save")"
> >  
> > -	if [ -z "$subcommand" ]; then
> > -		case "$((cword - __git_cmd_idx)),$cur" in
> > -		*,--*)
> > -			__gitcomp_builtin stash_push
> > -			;;
> > -		1,sa*)
> > -			__gitcomp "save"
> > -			;;
> > -		1,*)
> > -			__gitcomp "$subcommands"
> > -			;;
> > -		esac
> > -		return
> > -	fi
> > -
> >  	case "$subcommand,$cur" in
> > +	,--*)
> > +		__gitcomp_builtin stash_save
> > +		;;
> > +	,sa*)
> > +		__git_init_builtin_opts stash_save

Also, I just noticed upon re-reading this patch that this is some
leftover cruft. But moot point since I'll be dropping this patch.

> > +		if ((cword - __git_cmd_idx == 1)); then
> > +			__gitcomp "save"
> > +		fi
> > +		;;
> > +	,*)
> > +		__git_init_builtin_opts stash_save
> > +		if ((cword - __git_cmd_idx == 1)); then
> > +			__gitcomp "$subcommands"
> > +		fi
> > +		;;
> >  	list,--*)
> >  		# 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"

```

## Denton Liu, 2021-04-22 10:00

Subject: [PATCH v2 0/4] git-completion.bash: fixes on top of 'dl/complete-stash'
Message-ID: <cover.1619085387.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/cover.1619085387.git.liu.denton%40gmail.com
In-Reply-To: <cover.1618910364.git.liu.denton@gmail.com>

```
Gábor pointed out some potential fixes what's currently queued in
'dl/complete-stash'[0][1][2][3]. This series addresses those concerns.

Changes since v1:

* Make the commit message for "git-completion.bash: rename to
  $__git_cmd_idx" descriptive and accurate

* Drop "git-completion.bash: consolidate no-subcommand case for
  _git_stash()"

[0]: https://lore.kernel.org/git/20210327183554.GD2271@szeder.dev/
[1]: https://lore.kernel.org/git/20210328103134.GF2271@szeder.dev/
[2]: https://lore.kernel.org/git/20210328103057.GE2271@szeder.dev/
[3]: https://lore.kernel.org/git/20210328110427.GG2271@szeder.dev/

Denton Liu (4):
  git-completion.bash: separate some commands onto their own line
  git-completion.bash: rename to $__git_cmd_idx
  git-completion.bash: use $__git_cmd_idx in more places
  git-completion.bash: consolidate cases in _git_stash()

 contrib/completion/git-completion.bash | 98 ++++++++++++++------------
 t/t9902-completion.sh                  | 19 +++++
 2 files changed, 70 insertions(+), 47 deletions(-)

Range-diff against v1:
1:  65c485ea0c = 1:  65c485ea0c git-completion.bash: separate some commands onto their own line
2:  7c7d6de380 ! 2:  76328e3123 git-completion.bash: rename to $__git_cmd_idx
    @@ Commit message
     
         In e94fb44042 (git-completion.bash: pass $__git_subcommand_idx from
         __git_main(), 2021-03-24), the $__git_subcommand_idx variable was
    -    introduced. Naming it after the index of the subcommand is flat-out
    -    wrong as this variable really holds the index of the git comand (e.g.
    -    "stash").
    +    introduced. Naming it after the index of the subcommand is needlessly
    +    confusing as, when this variable is used, it is in the completion
    +    functions for commands (e.g. _git_remote()) where for `git remote add`,
    +    the `remote` is referred to as the command and `add` is referred to as
    +    the subcommand.
     
         Rename this variable so that it's obvious it's about git commands. While
         we're at it, shorten up its name so that it's still readable without
3:  63a6992585 = 3:  70fda62db1 git-completion.bash: use $__git_cmd_idx in more places
4:  4f8d015d54 = 4:  103d38e293 git-completion.bash: consolidate cases in _git_stash()
5:  b4a9b0afa7 < -:  ---------- git-completion.bash: consolidate no-subcommand case for _git_stash()
-- 
2.31.1.499.g90b4fd31cd


```

## Denton Liu, 2021-04-22 10:00

Subject: [PATCH v2 1/4] git-completion.bash: separate some commands onto their own line
Message-ID: <65c485ea0c1726c96d292d7d7385def8d2f19f25.1619085387.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/65c485ea0c1726c96d292d7d7385def8d2f19f25.1619085387.git.liu.denton%40gmail.com
In-Reply-To: <cover.1619085387.git.liu.denton@gmail.com>

```
In e94fb44042 (git-completion.bash: pass $__git_subcommand_idx from
__git_main(), 2021-03-24), a line was introduced which contained
multiple statements. This is difficult to read so break it into multiple
lines.

While we're at it, follow this convention for the rest of the
__git_main() and break up lines that contain multiple statements.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 contrib/completion/git-completion.bash | 37 +++++++++++++++++++-------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c926ca26c6..1dedb14b47 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3400,17 +3400,35 @@ __git_main ()
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
 		case "$i" in
-		--git-dir=*) __git_dir="${i#--git-dir=}" ;;
-		--git-dir)   ((c++)) ; __git_dir="${words[c]}" ;;
-		--bare)      __git_dir="." ;;
-		--help) command="help"; break ;;
-		-c|--work-tree|--namespace) ((c++)) ;;
-		-C)	__git_C_args[C_args_count++]=-C
+		--git-dir=*)
+			__git_dir="${i#--git-dir=}"
+			;;
+		--git-dir)
+			((c++))
+			__git_dir="${words[c]}"
+			;;
+		--bare)
+			__git_dir="."
+			;;
+		--help)
+			command="help"
+			break
+			;;
+		-c|--work-tree|--namespace)
+			((c++))
+			;;
+		-C)
+			__git_C_args[C_args_count++]=-C
 			((c++))
 			__git_C_args[C_args_count++]="${words[c]}"
 			;;
-		-*) ;;
-		*) command="$i"; __git_subcommand_idx="$c"; break ;;
+		-*)
+			;;
+		*)
+			command="$i"
+			__git_subcommand_idx="$c"
+			break
+			;;
 		esac
 		((c++))
 	done
@@ -3432,7 +3450,8 @@ __git_main ()
 			;;
 		esac
 		case "$cur" in
-		--*)   __gitcomp "
+		--*)
+			__gitcomp "
 			--paginate
 			--no-pager
 			--git-dir=
-- 
2.31.1.499.g90b4fd31cd


```

## Denton Liu, 2021-04-22 10:00

Subject: [PATCH v2 2/4] git-completion.bash: rename to $__git_cmd_idx
Message-ID: <76328e3123bd670435632244cbdd0e68db815804.1619085387.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/76328e3123bd670435632244cbdd0e68db815804.1619085387.git.liu.denton%40gmail.com
In-Reply-To: <cover.1619085387.git.liu.denton@gmail.com>

```
In e94fb44042 (git-completion.bash: pass $__git_subcommand_idx from
__git_main(), 2021-03-24), the $__git_subcommand_idx variable was
introduced. Naming it after the index of the subcommand is needlessly
confusing as, when this variable is used, it is in the completion
functions for commands (e.g. _git_remote()) where for `git remote add`,
the `remote` is referred to as the command and `add` is referred to as
the subcommand.

Rename this variable so that it's obvious it's about git commands. While
we're at it, shorten up its name so that it's still readable without
being a handful to type.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 contrib/completion/git-completion.bash | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1dedb14b47..c29c129f87 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1474,12 +1474,12 @@ _git_branch ()
 
 _git_bundle ()
 {
-	local cmd="${words[__git_subcommand_idx+1]}"
+	local cmd="${words[__git_cmd_idx+1]}"
 	case "$cword" in
-	$((__git_subcommand_idx+1)))
+	$((__git_cmd_idx+1)))
 		__gitcomp "create list-heads verify unbundle"
 		;;
-	$((__git_subcommand_idx+2)))
+	$((__git_cmd_idx+2)))
 		# looking for a file
 		;;
 	*)
@@ -1894,7 +1894,7 @@ _git_grep ()
 	esac
 
 	case "$cword,$prev" in
-	$((__git_subcommand_idx+1)),*|*,-*)
+	$((__git_cmd_idx+1)),*|*,-*)
 		__git_complete_symbol && return
 		;;
 	esac
@@ -3017,7 +3017,7 @@ _git_stash ()
 	local subcommand="$(__git_find_on_cmdline "$subcommands save")"
 
 	if [ -z "$subcommand" ]; then
-		case "$((cword - __git_subcommand_idx)),$cur" in
+		case "$((cword - __git_cmd_idx)),$cur" in
 		*,--*)
 			__gitcomp_builtin stash_push
 			;;
@@ -3058,7 +3058,7 @@ _git_stash ()
 		__gitcomp_builtin stash_branch
 		;;
 	branch,*)
-		if [ $cword -eq $((__git_subcommand_idx+2)) ]; then
+		if [ $cword -eq $((__git_cmd_idx+2)) ]; then
 			__git_complete_refs
 		else
 			__gitcomp_nl "$(__git stash list \
@@ -3303,7 +3303,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 $((__git_subcommand_idx+2)) ]; then
+			if [ $cword -eq $((__git_cmd_idx+2)) ]; then
 				# Right after the 'add' subcommand: have to
 				# complete the path, so fall back to Bash
 				# filename completion.
@@ -3327,7 +3327,7 @@ _git_worktree ()
 		__git_complete_worktree_paths
 		;;
 	move,*)
-		if [ $cword -eq $((__git_subcommand_idx+2)) ]; then
+		if [ $cword -eq $((__git_cmd_idx+2)) ]; then
 			# The first parameter must be an existing working
 			# tree to be moved.
 			__git_complete_worktree_paths
@@ -3395,7 +3395,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
+	local __git_cmd_idx
 
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
@@ -3426,7 +3426,7 @@ __git_main ()
 			;;
 		*)
 			command="$i"
-			__git_subcommand_idx="$c"
+			__git_cmd_idx="$c"
 			break
 			;;
 		esac
-- 
2.31.1.499.g90b4fd31cd


```

## Denton Liu, 2021-04-22 10:00

Subject: [PATCH v2 4/4] git-completion.bash: consolidate cases in _git_stash()
Message-ID: <103d38e29394d7e4d1bf01817795aed5be568673.1619085387.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/103d38e29394d7e4d1bf01817795aed5be568673.1619085387.git.liu.denton%40gmail.com
In-Reply-To: <cover.1619085387.git.liu.denton@gmail.com>

```
The $subcommand case statement in _git_stash() is quite repetitive.
Consolidate the cases together into one catch-all case to reduce the
repetition.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 contrib/completion/git-completion.bash | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 30c9a97616..7bce9a0112 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3032,21 +3032,6 @@ _git_stash ()
 	fi
 
 	case "$subcommand,$cur" in
-	push,--*)
-		__gitcomp_builtin stash_push
-		;;
-	save,--*)
-		__gitcomp_builtin stash_save
-		;;
-	pop,--*)
-		__gitcomp_builtin stash_pop
-		;;
-	apply,--*)
-		__gitcomp_builtin stash_apply
-		;;
-	drop,--*)
-		__gitcomp_builtin stash_drop
-		;;
 	list,--*)
 		# 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"
@@ -3054,8 +3039,8 @@ _git_stash ()
 	show,--*)
 		__gitcomp_builtin stash_show "$__git_diff_common_options"
 		;;
-	branch,--*)
-		__gitcomp_builtin stash_branch
+	*,--*)
+		__gitcomp_builtin "stash_$subcommand"
 		;;
 	branch,*)
 		if [ $cword -eq $((__git_cmd_idx+2)) ]; then
@@ -3069,8 +3054,6 @@ _git_stash ()
 		__gitcomp_nl "$(__git stash list \
 				| sed -n -e 's/:.*//p')"
 		;;
-	*)
-		;;
 	esac
 }
 
-- 
2.31.1.499.g90b4fd31cd


```

## Denton Liu, 2021-04-22 10:00

Subject: [PATCH v2 3/4] git-completion.bash: use $__git_cmd_idx in more places
Message-ID: <70fda62db198ad4a25702861565a47ee8f1dd62e.1619085387.git.liu.denton@gmail.com>
URL: https://gitlist.dev/e/70fda62db198ad4a25702861565a47ee8f1dd62e.1619085387.git.liu.denton%40gmail.com
In-Reply-To: <cover.1619085387.git.liu.denton@gmail.com>

```
With the introduction of the $__git_cmd_idx variable in e94fb44042
(git-completion.bash: pass $__git_subcommand_idx from __git_main(),
2021-03-24), completion functions were able to know the index at which
the git command is listed, allowing them to skip options that are given
to the underlying git itself, not the corresponding command (e.g.
`-C asdf` in `git -C asdf branch`).

While most of the changes here are self-explanatory, some bear further
explanation.

For the __git_find_on_cmdline() and __git_find_last_on_cmdline() pair of
functions, these functions are only ever called in the context of a git
command completion function. These functions will only care about words
after the command so we can safely ignore the words before this.

For _git_worktree(), this change is technically a no-op (once the
__git_find_last_on_cmdline change is also applied). It was in poor style
to have hard-coded on the index right after `worktree`. In case
`git worktree` were to ever learn to accept options, the current
situation would be inflexible.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 contrib/completion/git-completion.bash | 26 ++++++++++++++------------
 t/t9902-completion.sh                  | 19 +++++++++++++++++++
 2 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c29c129f87..30c9a97616 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1006,8 +1006,8 @@ __git_complete_revlist ()
 
 __git_complete_remote_or_refspec ()
 {
-	local cur_="$cur" cmd="${words[1]}"
-	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
+	local cur_="$cur" cmd="${words[__git_cmd_idx]}"
+	local i c=$((__git_cmd_idx+1)) remote="" pfx="" lhs=1 no_complete_refspec=0
 	if [ "$cmd" = "remote" ]; then
 		((c++))
 	fi
@@ -1176,7 +1176,7 @@ __git_aliased_command ()
 # --show-idx: Optionally show the index of the found word in the $words array.
 __git_find_on_cmdline ()
 {
-	local word c=1 show_idx
+	local word c="$__git_cmd_idx" show_idx
 
 	while test $# -gt 1; do
 		case "$1" in
@@ -1221,7 +1221,7 @@ __git_find_last_on_cmdline ()
 	done
 	local wordlist="$1"
 
-	while [ $c -gt 1 ]; do
+	while [ $c -gt "$__git_cmd_idx" ]; do
 		((c--))
 		for word in $wordlist; do
 			if [ "$word" = "${words[c]}" ]; then
@@ -1306,7 +1306,7 @@ __git_count_arguments ()
 	local word i c=0
 
 	# Skip "git" (first argument)
-	for ((i=1; i < ${#words[@]}; i++)); do
+	for ((i="$__git_cmd_idx"; i < ${#words[@]}; i++)); do
 		word="${words[i]}"
 
 		case "$word" in
@@ -1442,7 +1442,7 @@ __git_ref_fieldlist="refname objecttype objectsize objectname upstream push HEAD
 
 _git_branch ()
 {
-	local i c=1 only_local_ref="n" has_r="n"
+	local i c="$__git_cmd_idx" only_local_ref="n" has_r="n"
 
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
@@ -2474,7 +2474,7 @@ _git_switch ()
 __git_config_get_set_variables ()
 {
 	local prevword word config_file= c=$cword
-	while [ $c -gt 1 ]; do
+	while [ $c -gt "$__git_cmd_idx" ]; do
 		word="${words[c]}"
 		case "$word" in
 		--system|--global|--local|--file=*)
@@ -3224,7 +3224,7 @@ _git_svn ()
 
 _git_tag ()
 {
-	local i c=1 f=0
+	local i c="$__git_cmd_idx" f=0
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
 		case "$i" in
@@ -3276,9 +3276,11 @@ __git_complete_worktree_paths ()
 _git_worktree ()
 {
 	local subcommands="add list lock move prune remove unlock"
-	local subcommand
+	local subcommand subcommand_idx
 
-	subcommand="$(__git_find_on_cmdline "$subcommands")"
+	subcommand="$(__git_find_on_cmdline --show-idx "$subcommands")"
+	subcommand_idx="${subcommand% *}"
+	subcommand="${subcommand#* }"
 
 	case "$subcommand,$cur" in
 	,*)
@@ -3303,7 +3305,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 $((__git_cmd_idx+2)) ]; then
+			if [ $cword -eq $((subcommand_idx+1)) ]; then
 				# Right after the 'add' subcommand: have to
 				# complete the path, so fall back to Bash
 				# filename completion.
@@ -3327,7 +3329,7 @@ _git_worktree ()
 		__git_complete_worktree_paths
 		;;
 	move,*)
-		if [ $cword -eq $((__git_cmd_idx+2)) ]; then
+		if [ $cword -eq $((subcommand_idx+1)) ]; then
 			# The first parameter must be an existing working
 			# tree to be moved.
 			__git_complete_worktree_paths
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 04ce884ef5..9439fec8f0 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1879,6 +1879,7 @@ test_expect_success '__git_find_on_cmdline - single match' '
 	(
 		words=(git command --opt list) &&
 		cword=${#words[@]} &&
+		__git_cmd_idx=1 &&
 		__git_find_on_cmdline "add list remove" >actual
 	) &&
 	test_cmp expect actual
@@ -1889,6 +1890,7 @@ test_expect_success '__git_find_on_cmdline - multiple matches' '
 	(
 		words=(git command -o --opt remove list add) &&
 		cword=${#words[@]} &&
+		__git_cmd_idx=1 &&
 		__git_find_on_cmdline "add list remove" >actual
 	) &&
 	test_cmp expect actual
@@ -1898,6 +1900,7 @@ test_expect_success '__git_find_on_cmdline - no match' '
 	(
 		words=(git command --opt branch) &&
 		cword=${#words[@]} &&
+		__git_cmd_idx=1 &&
 		__git_find_on_cmdline "add list remove" >actual
 	) &&
 	test_must_be_empty actual
@@ -1908,6 +1911,7 @@ test_expect_success '__git_find_on_cmdline - single match with index' '
 	(
 		words=(git command --opt list) &&
 		cword=${#words[@]} &&
+		__git_cmd_idx=1 &&
 		__git_find_on_cmdline --show-idx "add list remove" >actual
 	) &&
 	test_cmp expect actual
@@ -1918,6 +1922,7 @@ test_expect_success '__git_find_on_cmdline - multiple matches with index' '
 	(
 		words=(git command -o --opt remove list add) &&
 		cword=${#words[@]} &&
+		__git_cmd_idx=1 &&
 		__git_find_on_cmdline --show-idx "add list remove" >actual
 	) &&
 	test_cmp expect actual
@@ -1927,11 +1932,23 @@ test_expect_success '__git_find_on_cmdline - no match with index' '
 	(
 		words=(git command --opt branch) &&
 		cword=${#words[@]} &&
+		__git_cmd_idx=1 &&
 		__git_find_on_cmdline --show-idx "add list remove" >actual
 	) &&
 	test_must_be_empty actual
 '
 
+test_expect_success '__git_find_on_cmdline - ignores matches before command with index' '
+	echo "6 remove" >expect &&
+	(
+		words=(git -C remove command -o --opt remove list add) &&
+		cword=${#words[@]} &&
+		__git_cmd_idx=3 &&
+		__git_find_on_cmdline --show-idx "add list remove" >actual
+	) &&
+	test_cmp expect actual
+'
+
 test_expect_success '__git_get_config_variables' '
 	cat >expect <<-EOF &&
 	name-1
@@ -2275,6 +2292,7 @@ do
 		(
 			words=(git push '$flag' other ma) &&
 			cword=${#words[@]} cur=${words[cword-1]} &&
+			__git_cmd_idx=1 &&
 			__git_complete_remote_or_refspec &&
 			print_comp
 		) &&
@@ -2288,6 +2306,7 @@ do
 		(
 			words=(git push other '$flag' ma) &&
 			cword=${#words[@]} cur=${words[cword-1]} &&
+			__git_cmd_idx=1 &&
 			__git_complete_remote_or_refspec &&
 			print_comp
 		) &&
-- 
2.31.1.499.g90b4fd31cd


```