git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Ryan Zoeller <rtzoeller@rtzoeller.com>
Cc: git@vger.kernel.org
Subject: Re: [RFC PATCH 1/2] parse-options: add --git-completion-helper-all
Date: Wed, 19 Aug 2020 12:39:24 -0700	[thread overview]
Message-ID: <xmqqeeo2togj.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200819175047.692962-2-rtzoeller@rtzoeller.com> (Ryan Zoeller's message of "Wed, 19 Aug 2020 17:51:08 +0000")

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.

> --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?

> 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.

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?


>  		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.

>  		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.

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.

>  
>  		if (arg[1] != '-') {
>  			ctx->opt = arg + 1;

  reply	other threads:[~2020-08-19 19:39 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 [this message]
2020-08-19 20:51     ` Ryan Zoeller
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=xmqqeeo2togj.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=rtzoeller@rtzoeller.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).