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

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

[Ákos Uzonyi]

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

Currently only the long version (--source=) supports completion.

Add completion support to the short (-s) option too.

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]}"
+
+		case "$prevword" in
+			-s)
+				__git_complete_refs
+				return
+				;;
+			*)
+				;;
+		esac
+		;;
 	esac
 }
 
-- 
2.28.0

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

[Junio C Hamano]

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

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

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

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

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

[Ákos Uzonyi]

Hi,

Thanks for your detailed review.

On Fri, 25 Sep 2020 at 19:30, Junio C Hamano <gitster@pobox.com> wrote:
> Ákos Uzonyi <uzonyi.akos@gmail.com> writes:
>
> > 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.

There are multiple commands already having this kind of short-long
option completion. The "-c" options of commit, switch and checkout
each have longer counterparts, and both the short and long versions
have completion support for their arguments.

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

I think these cases are not too frequent, so it doesn't seem to be a
big scaling problem.

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

Thanks, I'll fix it.

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

Thanks, I'll fix it.

By the way, I copied this piece of code from _git_switch (it's also
there in _git_checkout), so these problems have to be fixed there as
well.

Also, reading _git_commit it looks that we already have a "$prev"
variable, so I'll use that instead of "$prevword".