From: Ryan Zoeller <rtzoeller@rtzoeller.com>
Date: Wed, 19 Aug 2020 20:51:11 GMT
Subject: Re: [RFC PATCH 1/2] parse-options: add --git-completion-helper-all
Message-ID: <a51ff9bb-47df-63ce-0b7f-508e064f4e3f@rtzoeller.com>
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;