From: Ryan Zoeller <rtzoeller@rtzoeller.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [RFC PATCH 1/2] parse-options: add --git-completion-helper-all
Date: Wed, 19 Aug 2020 20:51:11 +0000 [thread overview]
Message-ID: <a51ff9bb-47df-63ce-0b7f-508e064f4e3f@rtzoeller.com> (raw)
In-Reply-To: <xmqqeeo2togj.fsf@gitster.c.googlers.com>
On 8/19/20 2:39 PM, Junio C Hamano wrote:
>
> Ryan Zoeller <rtzoeller@rtzoeller.com> writes:
>
>> --git-completion-helper excludes hidden options, such as --allow-empty
>> for git commit. This is typically helpful, but occasionally we want
>> auto-completion for obscure flags.
>
> Hits from "git grep -B2 OPT_NOCOMPLETE" tells me that these are
> mostly unsafe options. Those who accept the risk by saying
> "complete all" should be allowed to see them.
>
> The same with OPT_HIDDEN (including OPT_HIDDEN_<TYPE>) gives us a
> mixed bag. Many are unsafe, some are uncommon and the rest are
> discouraged, or old synonym to some other option that does get
> completed. I am not sure if letting them be completed is an overall
> win or makes the output from "git cmd --<TAB><TAB>" too noisy.
If options marked OPT_HIDDEN are considered too internal to
meaningfully expose, I'm happy to hide them. I defaulted to
"show everything", and backing off from that is easy enough.
>
>> --git-completion-helper-all returns
>> all options, even if they are marked as hidden or nocomplete.
>
> If it is "occasinally", why is the removal of OPT_HIDDEN in
> show_negated_gitcomp() unconditional?
It shouldn't have been. I visually clumped the calls to it
as being inside the for loop, and assumed they were being skipped
over as part of the continue.
>
>> Signed-off-by: Ryan Zoeller <rtzoeller@rtzoeller.com>
>> ---
>> parse-options.c | 18 ++++++++++++------
>> 1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/parse-options.c b/parse-options.c
>> index c57618d537..cc7239e1c6 100644
>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -535,8 +535,9 @@ static void show_negated_gitcomp(const struct option *opts, int nr_noopts)
>>
>> if (!opts->long_name)
>> continue;
>> - if (opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE))
>> - continue;
>> + /* Don't check PARSE_OPT_HIDDEN or PARSE_OPT_NOCOMPLETE,
>> + * we expect the caller to handle these appropriately.
>> + */
>
> /*
> * Style: our multi-line comments should begin with
> * slash-asterisk alone on its own line, and end with
> * asterisk-slash also on its own line, like this.
> */
>
> We do not run around and fix existing style violations that would
> only raise the patch noise, but we try not to introduce new
> violators.
Will fix.
>
> I am not sure what the new comment says is justifiable. The only
> caller of show_negated_gitcomp() is in show_gitcomp() where the
> function looped over the options and showed the options normally,
> honoring these two flags, but then the original list of options
> are passed to this function without filtering any option element
> on the list that are marked with these bits, so "the caller"
> apparently is not interested in handling the elements with these
> bits when making the decision to show "--no-<option>" form itself;
> it farms it out to show_negated_gitcomp() and expects the function
> to do "the right thing". Am I misleading the code?
>
>
You're not misreading it; I apparently neglected to test the completion
for '--no-' options with '--git-completion-helper', only
'--git-completion-helper-all'. I'll apply the same show_all logic
to this function.
>> if (opts->flags & PARSE_OPT_NONEG)
>> continue;
>>
>> @@ -572,7 +573,7 @@ static void show_negated_gitcomp(const struct option *opts, int nr_noopts)
>> }
>> }
>>
>> -static int show_gitcomp(const struct option *opts)
>> +static int show_gitcomp(const struct option *opts, int show_all)
>> {
>> const struct option *original_opts = opts;
>> int nr_noopts = 0;
>> @@ -582,7 +583,8 @@ static int show_gitcomp(const struct option *opts)
>>
>> if (!opts->long_name)
>> continue;
>> - if (opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE))
>> + if (!show_all &&
>> + (opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE)))
>> continue;
>>
>> switch (opts->type) {
>> @@ -723,9 +725,13 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
>> if (internal_help && ctx->total == 1 && !strcmp(arg + 1, "h"))
>> goto show_usage;
>>
>> - /* lone --git-completion-helper is asked by git-completion.bash */
>> + /* lone --git-completion-helper and --git-completion-helper-all
>> + * are asked by git-completion.bash
>> + */
>
> Ditto.
>
Will fix.
>> if (ctx->total == 1 && !strcmp(arg + 1, "-git-completion-helper"))
>> - return show_gitcomp(options);
>> + return show_gitcomp(options, 0);
>> + if (ctx->total == 1 && !strcmp(arg + 1, "-git-completion-helper-all"))
>> + return show_gitcomp(options, 1);
>
> This is not your fault, but the micro-optimization to avoid
> comparison between *arg (which already is known to be "-") and "-"
> is not worth the reduced readability.
>
> if (ctx->total == 1 && !strcmp(arg, "--git-completion-helper"))
> return show_gitcomp(options, 0);
> if (ctx->total == 1 && !strcmp(arg, "--git-completion-helper-all"))
> return show_gitcomp(options, 1);
>
> would be much clearer for readers to know what is going on.
>
I completely agree, and will clean these up.
> With an extra "const char *rest" variable in the same scope as
> "const char *arg",
>
> if (ctx->total == 1 &&
> !skip_prefix(arg, "--git-completion-helper", &rest) &&
> (!*rest || !strcmp(rest, "-all")))
> return show_gitcomp(options, *rest);
>
> would further avoid repetitions, but some folks may find it a bit
> too dense. I dunno.
I'm inclined to be repetitive in order to keep
'--git-completion-helper-all' intact, e.g. for grepping.
>
>>
>> if (arg[1] != '-') {
>> ctx->opt = arg + 1;
next prev parent reply other threads:[~2020-08-19 20:51 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-19 17:50 [RFC PATCH 0/2] Support enabling bash completion of all options Ryan Zoeller
2020-08-19 17:51 ` [RFC PATCH 1/2] parse-options: add --git-completion-helper-all Ryan Zoeller
2020-08-19 19:39 ` Junio C Hamano
2020-08-19 20:51 ` Ryan Zoeller [this message]
2020-08-19 21:05 ` Junio C Hamano
2020-08-19 17:51 ` [RFC PATCH 2/2] completion: add GIT_COMPLETION_SHOW_ALL env var Ryan Zoeller
2020-08-19 23:06 ` [PATCH v2 0/2] Support enabling bash completion of all options Ryan Zoeller
2020-08-19 23:06 ` [PATCH v2 1/2] parse-options: add --git-completion-helper-all Ryan Zoeller
2020-08-19 23:06 ` [PATCH v2 2/2] completion: add GIT_COMPLETION_SHOW_ALL env var Ryan Zoeller
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=a51ff9bb-47df-63ce-0b7f-508e064f4e3f@rtzoeller.com \
--to=rtzoeller@rtzoeller.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.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).