Re: [PATCH] completion: complete refs after 'git restore -s'

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 25, 2020, 17:30 UTC
Message-ID: <xmqqlfgxlq6o.fsf@gitster.c.googlers.com>
In-Reply-To: <20200925142552.1656596-1-uzonyi.akos@gmail.com>

Ákos Uzonyi <uzonyi.akos@gmail.com> writes:

Show 5 quoted lines

> From: Uzonyi Ákos <uzonyi.akos@gmail.com>
>
> Currently only the long version (--source=) supports completion.
>
> Add completion support to the short (-s) option too.

I am not too familiar with the completion library, but what makes the "-s" option of restore so special? I've scanned the entire file and did not find that many special cases for short options that have their longer counterpart supported already.

I do not know if the "feature" this wants to bring in is a good idea---we may want to try to be more systematic (e.g. perhaps it involves teaching the parse-options subsystem about equivalence of short and long options, so that we can reuse existing support for the the long option "--source=<TAB>" to complete "-s <TAB>"), if we were to do something like this. Singling out "-s" of "restore" smells not quite right, as the approach would not scale well.

Show 15 quoted lines

> Signed-off-by: Ákos Uzonyi <uzonyi.akos@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 8be4a0316e..50e6e82157 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2853,6 +2853,18 @@ _git_restore ()
>  	--*)
>  		__gitcomp_builtin restore
>  		;;
> +	*)
> +		local prevword prevword="${words[cword-1]}"

Why duplicated prevword here?  Did you mean

	local prevword=${words[cword-1]}

instead?

Show 10 quoted lines

> +
> +		case "$prevword" in
> +			-s)
> +				__git_complete_refs
> +				return
> +				;;
> +			*)
> +				;;
> +		esac
> +		;;

Wrong indentation. In this file, as can be seen on the line "*)" you added at the top of this hunk, the case arms like "-s)" and "*)" must align with "case" and "esac" in this file.

>  	esac
>  }

Thanks.