Re: [PATCH 1/2] parse-options: support --git-completion-helper

From: Duy Nguyen <pclouds@gmail.com>
Date: Jan 17, 2018, 00:27 UTC
Message-ID: <CACsJy8C1-sjeEgFBRx45zHeqNo2ZcgDvoRMs0mKtJLUqmtQsAg@mail.gmail.com>
In-Reply-To: <xmqq4lnltlia.fsf@gitster.mtv.corp.google.com>

On Wed, Jan 17, 2018 at 6:46 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 26 quoted lines

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> This option is designed to be used by git-completion.bash. For many
>> simple cases, what we do in there is usually
>>
>>     __gitcomp "lots of completion options"
>>
>> which has to be manually updated when a new user-visible option is
>> added. With support from parse-options, we can write
>>
>>     __gitcomp "$(git command --git-completion-helper)"
>>
>> and get that list directly from the parser for free. Dangerous/Unpopular
>> options could be hidden with the new "NO_GITCOMP" flag.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  parse-options.c | 37 +++++++++++++++++++++++++++++++++++++
>>  parse-options.h |  6 ++++--
>>  2 files changed, 41 insertions(+), 2 deletions(-)
>
> I do not know if you need PARSE_OPT_NO_INTERNAL_GITCOMP.  The only
> way to trigger this is by passing a rather long option (intended to
> be used only by scripts), so unlike PARSE_OPT_NO_INTERNAL_HELP which
> was an escape hatch to free a short-and-sweet "-h" to be retargetted
> by a selected few commands, there may not be need for it.

It's a copy/paste from internal_help. Because I was asking for comments, not really aiming to submit these changes to git.git (yet), I didn't bother to check further.

Show 7 quoted lines

> Some day when everybody uses parse-options, it would be trivially
> correct to declare that this is the right approach ;-) I am not sure
> how much of the current completion script's hardcoded option list
> can be reduced with this approach with today's code, given that the
> bigger ones (diff and log family) are not using parse-options API at
> all, but for the ones that already do use parse-options, I think
> this makes sense.

Yeah yeah the horrible handle_revision_opt() and diff_opt_parse(). I think I attempted to convert them two parse-options at some point, not sure why I dropped it, maybe because target variables are in dynamically allocated memory ('struct diff_options *' and 'struct rev_info *') which does not fit well to how we usually initialize 'struct options[]'.

-- 
Duy