Re: [PATCH 5/5] git-completion.bash: consolidate no-subcommand case for _git_stash()

From: Denton Liu <liu.denton@gmail.com>
Date: Apr 21, 2021, 04:08 UTC
Message-ID: <YH+lMZCg5cqniPaU@generichostname>
In-Reply-To: <xmqqk0ow4q7k.fsf@gitster.g>

Hi Junio,

On Tue, Apr 20, 2021 at 02:10:39PM -0700, Junio C Hamano wrote:

Show 8 quoted lines

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

Show 48 quoted lines

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

Show 13 quoted lines

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