[PATCH 0/3] git-completion.bash: improvements to _git_stash()

[PATCH 0/3] git-completion.bash: improvements to _git_stash()

[Denton Liu]

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

[PATCH 1/3] git-completion.bash: extract from else in _git_stash()

[Denton Liu]

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.

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 7dc6cd8eb8..fe79f6b71c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -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

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

[Denton Liu]

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(-)

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 \
-- 
2.31.0.rc2.261.g7f71774620

[PATCH 3/3] git-completion.bash: use __gitcomp_builtin() in _git_stash()

[Denton Liu]

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.

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index da46f46e3c..83b6415b13 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -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

[RESEND PATCH 0/3] git-completion.bash: improvements to _git_stash()

[Denton Liu]

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

[RESEND PATCH 1/3] git-completion.bash: extract from else in _git_stash()

[Denton Liu]

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.

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 7dc6cd8eb8..fe79f6b71c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -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

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

[Denton Liu]

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(-)

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 \
-- 
2.31.0.rc2.261.g7f71774620

[RESEND PATCH 3/3] git-completion.bash: use __gitcomp_builtin() in _git_stash()

[Denton Liu]

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.

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index da46f46e3c..83b6415b13 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -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

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

[Junio C Hamano]

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 \

Re: [RESEND PATCH 0/3] git-completion.bash: improvements to _git_stash()

[Junio C Hamano]

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

> 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(-)

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'?

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

[Denton Liu]

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

Re: [RESEND PATCH 0/3] git-completion.bash: improvements to _git_stash()

[Denton Liu]

Hi Junio,

On Thu, Mar 18, 2021 at 02:58:34PM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > 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(-)
> 
> 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'?

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

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

[Junio C Hamano]

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.

Re: [RESEND PATCH 0/3] git-completion.bash: improvements to _git_stash()

[Junio C Hamano]

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

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

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.

[PATCH v2 0/3] git-completion.bash: improvements to _git_stash()

[Denton Liu]

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

[PATCH v2 1/3] git-completion.bash: pass $__git_subcommand_idx from __git_main()

[Denton Liu]

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.

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 7dc6cd8eb8..a2f1b5e916 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -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

[PATCH v2 2/3] git-completion.bash: extract from else in _git_stash()

[Denton Liu]

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.

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index a2f1b5e916..8d4d8cc0fe 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -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

[PATCH v2 3/3] git-completion.bash: use __gitcomp_builtin() in _git_stash()

[Denton Liu]

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.

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

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 8d4d8cc0fe..c926ca26c6 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -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

Re: [PATCH v2 1/3] git-completion.bash: pass $__git_subcommand_idx from __git_main()

[SZEDER Gábor]

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.

On Wed, Mar 24, 2021 at 01:36:27AM -0700, Denton Liu wrote:
> 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).

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

> 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

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?

> is actually found and the corresponding completion function is called.
> Use this variable to replace hardcoded comparisons with $cword.
> 
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)

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.


> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 7dc6cd8eb8..a2f1b5e916 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -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

I don't like these changes to _git_worktree(), because they implicitly
assume that 'git worktree' doesn't have any --options, and it would
then start to misbehave if we added one.

And these changes wouldn't be necessary if __git_find_on_cmdline()
started its search at $__git_cmd_idx instead of at ${words[1]}.

> @@ -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 ;;

See what variable is $i assigned to?  $command, not $subcommand.

>  		esac
>  		((c++))
>  	done
> -- 
> 2.31.0.rc2.261.g7f71774620
> 

Re: [PATCH v2 2/3] git-completion.bash: extract from else in _git_stash()

[SZEDER Gábor]

On Wed, Mar 24, 2021 at 01:36:28AM -0700, Denton Liu wrote:
> 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.
> 
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 73 +++++++++++++-------------
>  1 file changed, 37 insertions(+), 36 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index a2f1b5e916..8d4d8cc0fe 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -3035,44 +3035,45 @@ _git_stash ()

That "if arm" mentioned in the commit message outside the patch
context looks like this:

        if [ -z "$subcommand" ]; then
                case "$cur" in
                [ handle the cases when there is no subcommand]

So we could simplify this further dropping that "if" completely, and
unify the two case blocks from the if and else branches like this:

        case "$subcommand,$cur" in
        [ handle the cases without a subcommand ]
        ,--*)  [ ... ]
        [ ... ]
        [ handle the casese with a subcommand, just like you did in
          this patch ]
        esac

I think this would also make the thid patch a bit simpler.

>  			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
> 

Re: [PATCH v2 1/3] git-completion.bash: pass $__git_subcommand_idx from __git_main()

[SZEDER Gábor]

On Wed, Mar 24, 2021 at 01:36:27AM -0700, Denton Liu wrote:
> @@ -3412,7 +3411,7 @@ __git_main ()
>  			__git_C_args[C_args_count++]="${words[c]}"
>  			;;
>  		-*) ;;
> -		*) command="$i"; break ;;
> +		*) command="$i"; __git_subcommand_idx="$c"; break ;;

Please put each of these statements on separate lines.

>  		esac
>  		((c++))
>  	done
> -- 
> 2.31.0.rc2.261.g7f71774620
> 

Re: [PATCH v2 3/3] git-completion.bash: use __gitcomp_builtin() in _git_stash()

[SZEDER Gábor]

On Wed, Mar 24, 2021 at 01:36:29AM -0700, Denton Liu wrote:
> 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.
> 
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 42 ++++++++++++--------------
>  1 file changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 8d4d8cc0fe..c926ca26c6 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -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

I think it would be better to keep this condition ...

> +
>  	if [ -z "$subcommand" ]; then
> -		case "$cur" in
> -		--*)
> -			__gitcomp "$save_opts"
> +		case "$((cword - __git_subcommand_idx)),$cur" in

... and not bring in such magic with the indices here and ...

> +		*,--*)
> +			__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"

... here in these two case arms, but instead handle the cases both
with and without a subcommand in a unified case statement as I
suggested in reply to the previous patch.

>  			;;
>  		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
>  		;;

These case arms are still quite repetitive, and could be handled by a
single arm like this:

        *,--*)
                __gitcomp_builtin stash_$subcommand
                ;;

Of course the more specific 'list,--*' and 'show,--*' cases should be
handled before.

The end result would look something like this (taken from a WIP patch
series of mine, which has been in a WIP state for about a year and a
half now...):

  https://github.com/szeder/git/commit/83a0e138767040280750062c5c0d43886796fcb1


>  	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
>