git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Duy Nguyen <pclouds@gmail.com>
Cc: "Denton Liu" <liu.denton@gmail.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Git Mailing List" <git@vger.kernel.org>,
	"Anmol Mago" <anmolmago@gmail.com>,
	briankyho@gmail.com, david.lu97@outlook.com,
	shirui.wang@hotmail.com
Subject: Re: [PATCH v2] completion: use builtin completion for format-patch
Date: Sat, 03 Nov 2018 19:20:38 +0900	[thread overview]
Message-ID: <xmqqmuqqmphl.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <CACsJy8B4vrtsBu79J2kYNmcNQfRkgetdbi8XOPjm7j1kNDJ6Yg@mail.gmail.com> (Duy Nguyen's message of "Sat, 3 Nov 2018 09:29:31 +0100")

Duy Nguyen <pclouds@gmail.com> writes:

>> Would it make sense to make send-email's completion helper print these
>> out directly? That way, if someone were to modify send-email in the
>> future, they'd only have to look through one file instead of both
>> send-email and the completions script.
>
> I did think about that and decided not to do it (in fact the first
> revision of this patch did exactly that).
>
> If we have to maintain this list manually, we might as well leave to
> the place that matters: the completion script. I don't think the
> person who updates send-email.perl would be always interested in
> completion support. And the one that is interested usually only looks
> at the completion script. Putting this list in send-email.perl just
> makes it harder to find.

I do not necessarily disagree with the conclusion, but I am not sure
if I agree with the last paragraph.  If the definition used to list
completable options was in the send-email command, it is more likely
that a patch to send-email.perl that would add/modify an option
without making a matching change to the definition in the same file
gets noticed, whether the developer who is doing the feature is or
is not interested in maintaining the completion script working, no?
Similarly, if one notices that an option the command supports that
ought to get completed does not get completed, and gets motivated
enough to try finding where in the completion script other existing
options that do get completed are handled, wouldn't that lead one to
the right solution, i.e. discovery of the definition in the
send-email script?  

Quite honestly, I would expect that our developers and user base are
much more competent than one who

 - wants to add completion of the option Y to the command A, which
   has known-to-be-working completion of the option X, and yet

 - fails to imagine that it could be a possible good first step to
   figure out how the option X is completed, so that a new support
   for the option Y might be able to emulate it.

Now, once we start going in the direction of having both the
implementation of options *and* the definition of the list of
completable options in send-email.perl script, I would agree with
your initial assessment in a message much earlier in the thread.  It
would be very tempting to use the data we feed Getopt::Long as the
source of the list of completable options to reduce the longer-term
maintenance load, which would mean it will involve more work.  And
in order to avoid having to invest more work upfront (which I do not
think is necessarily a bad thing), having the definition in the
completion script might be easier to manage---it is closer to the
status quo, especially the state before you taught parse-options API
to give the list of completable options.

Thanks.

      reply	other threads:[~2018-11-03 10:20 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-29 17:57 [PATCH] completion: use builtin completion for format-patch Denton Liu
2018-10-30  2:20 ` Junio C Hamano
2018-10-30  3:50   ` Denton Liu
2018-10-30  6:38   ` [PATCH v2] " Denton Liu
2018-10-30  7:33     ` Junio C Hamano
2018-10-30 15:29     ` Duy Nguyen
2018-11-01  1:42       ` Junio C Hamano
2018-11-01 15:40         ` Duy Nguyen
2018-11-01 23:52           ` Junio C Hamano
2018-11-03  6:03             ` Duy Nguyen
2018-11-03  7:59               ` Denton Liu
2018-11-03  8:29                 ` Duy Nguyen
2018-11-03 10:20                   ` Junio C Hamano [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=xmqqmuqqmphl.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=anmolmago@gmail.com \
    --cc=briankyho@gmail.com \
    --cc=david.lu97@outlook.com \
    --cc=git@vger.kernel.org \
    --cc=liu.denton@gmail.com \
    --cc=pclouds@gmail.com \
    --cc=shirui.wang@hotmail.com \
    --cc=szeder.dev@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).