git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
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;


  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 \
    --subject='Re: [RFC PATCH 1/2] parse-options: add --git-completion-helper-all' \
    /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

Code repositories for project(s) associated with this 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).