| Message ID | 20200925142552.1656596-1-uzonyi.akos@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | completion: complete refs after 'git restore -s' | expand |
Á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.
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".
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 }