From: "Ákos Uzonyi" <uzonyi.akos@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Subject: Re: [PATCH] completion: complete refs after 'git restore -s'
Date: Fri, 25 Sep 2020 21:59:49 +0200 [thread overview]
Message-ID: <CAJrAtOWH8Upwov5dL4OfHPAb9v3NynmooC8mr3hAVA+Yo9Yi5w@mail.gmail.com> (raw)
In-Reply-To: <xmqqlfgxlq6o.fsf@gitster.c.googlers.com>
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".
prev parent reply other threads:[~2020-09-25 20:22 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-25 14:25 [PATCH] completion: complete refs after 'git restore -s' Ákos Uzonyi
2020-09-25 17:30 ` Junio C Hamano
2020-09-25 19:59 ` Ákos Uzonyi [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJrAtOWH8Upwov5dL4OfHPAb9v3NynmooC8mr3hAVA+Yo9Yi5w@mail.gmail.com \
--to=uzonyi.akos@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=pclouds@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).