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

Ákos Uzonyi [off-list ref] writes:

From: Uzonyi Ákos <redacted>

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.

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.

Hi,

Thanks for your detailed review.

On Fri, 25 Sep 2020 at 19:30, Junio C Hamano [off-list ref] wrote:

Ákos Uzonyi [off-list ref] writes:

quoted

From: Uzonyi Ákos <redacted>

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.

quoted

Signed-off-by: Ákos Uzonyi <redacted>
---
 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.

quoted

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