On 8/19/20 2:39 PM, Junio C Hamano wrote:
>
> Ryan Zoeller <rtzoeller@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> ---
>> 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;
