git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Duy Nguyen <pclouds@gmail.com>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Git Mailing List" <git@vger.kernel.org>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Denton Liu" <liu.denton@gmail.com>
Subject: Re: [PATCH v2] parse-options: don't emit "ambiguous option" for aliases
Date: Fri, 19 Apr 2019 13:39:15 +0900
Message-ID: <xmqq1s1yfw24.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20190418092940.GA13484@ash> (Duy Nguyen's message of "Thu, 18 Apr 2019 16:29:41 +0700")

Duy Nguyen <pclouds@gmail.com> writes:

> So an alternative is simply outsource the ambiguity decision back to
> git-clone. If the same situation appears again elsewhere, we'll need
> to sit back and fix it for real. But this way we don't potentially
> introduce any new traps.

Sounds like a sensibly safe approach.

> +/*
> + * Avoid ambiguation error between --recursive and --recurse-submodule
> + * because they are the same. --recurs can be expanded to any of them
> + * and it still works.
> + */
> +static int is_abbrev_ambiguous(const struct option *prev,
> +			       const struct option *next)
> +{
> +	const struct option *opts[] = { prev, next };

By looking at its caller, I think the caller keeps track of the
candidate it saw so far, and ask this function to see if the one it
is looking at right now (i.e. "this one") should be flagged as
conflicting with the other one (or "the other one").  So it probably
makes more sense to call them <it, the_other> or <it, prev> [*1*].

	Side note: *1* I would have used "this" instead of "it", but
	I vaguely recall there are those who want to use C++ aware
	static checkers and avoiding the identifier "this" is easy
	enough so ...

> +	int i, found = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(opts); i++) {
> +		if (!opts[i]->long_name)
> +			continue;
> +		if (!strcmp(opts[i]->long_name, "recursive"))
> +			found |= 1 << 0;
> +		if (!strcmp(opts[i]->long_name, "recurse-submodules"))
> +			found |= 1 << 1;
> +	}
> +
> +	return found != 3;
> +}

For any two options that share the prefix, unless they are
"recursive" and "recurse-submodules" pair, we say "that's
ambiguous".  But when they are these two specifically singled out,
we say it is OK.  Makes sense.

The above may be sufficient for this purpose, but I would have
expected that these groups of aliases would be the ones in the
table, not the <it, the_other> pair.  IOW, with helpers like these:

	static const char *recurse_submodules[] = {
		"recurse-submodules", "recursive", NULL,
	};

	static struct {
		const char **aliases;
	} disamb[] = {
		recurse_submodules,
	};
		
	static int has_string(const char *it, const char **array)
	{
		while (*array)
			if (!strcmp(it, *(array++)))
				return 1;
		return 0;
	}

the body would look something like:

	int j;

	for (j = 0; j < ARRAY_SIZE(disamb); j++) {
		/* it and other are from the same family? */
		if (it->long_name &&
		    has_string(it->long_name, disamb[j].aliases) &&
		    other->long_name &&
		    has_string(other->long_name, disamb[j].aliases))
			return 0;
	}
	return 1;

Perhaps?

I suspect that renaming the function (and the field in the context
struct) to .is_alias() and reverse the polarity of its return value
may make the flow of the logic at the caller side easier to follow?

> diff --git a/parse-options.c b/parse-options.c
> index cec74522e5..c0354e5a92 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -294,7 +294,8 @@ static enum parse_opt_result parse_long_opt(
>  			if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&
>  			    !strncmp(long_name, arg, arg_end - arg)) {
>  is_abbreviated:
> -				if (abbrev_option) {
> +				if (abbrev_option &&
> +				    p->is_abbrev_ambiguous(abbrev_option, options)) {

The above suggestion would make the guard here to

			if (abbrev_option &&
			    !p->is_alias(abbrev_option, options)) {

That is, at this point, we know that the user may have used the
given string to name the "abbrev_option" we earlier saw (and made
sure it prefix matches the string), and now we are looking at
another one, options[0], that also prefix matches the string.  We
usually flag this case as a problematic ambiguity inside the body of
this if statement, but if one is an alias to the other, we do not do
so.

>  					/*
>  					 * If this is abbreviated, it is
>  					 * ambiguous. So when there is no

  reply	other threads:[~2019-04-19 19:48 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-25 18:14 [PATCH 0/8] Do not use abbreviated options in tests Johannes Schindelin via GitGitGadget
2019-03-25 18:14 ` [PATCH 1/8] tests (rebase): spell out the `--keep-empty` option Johannes Schindelin via GitGitGadget
2019-03-25 18:14 ` [PATCH 3/8] t7810: do not abbreviate `--no-exclude-standard` nor `--invert-match` Johannes Schindelin via GitGitGadget
2019-03-25 18:14 ` [PATCH 2/8] tests (rebase): spell out the `--force-rebase` option Johannes Schindelin via GitGitGadget
2019-03-25 18:14 ` [PATCH 4/8] t5531: avoid using an abbreviated option Johannes Schindelin via GitGitGadget
2019-03-25 18:14 ` [PATCH 5/8] tests (push): do not abbreviate the `--follow-tags` option Johannes Schindelin via GitGitGadget
2019-03-25 18:14 ` [PATCH 6/8] tests (status): spell out the `--find-renames` option in full Johannes Schindelin via GitGitGadget
2019-03-25 18:14 ` [PATCH 7/8] tests (pack-objects): use the full, unabbreviated `--revs` option Johannes Schindelin via GitGitGadget
2019-03-25 18:14 ` [PATCH 8/8] tests: disallow the use of abbreviated options (by default) Johannes Schindelin via GitGitGadget
2019-03-25 18:35   ` Denton Liu
2019-03-25 20:26     ` Ævar Arnfjörð Bjarmason
2019-04-12  8:48       ` Johannes Schindelin
2019-04-12  8:50     ` Johannes Schindelin
2019-03-25 19:47   ` Ævar Arnfjörð Bjarmason
2019-04-12  8:59     ` Johannes Schindelin
2019-03-25 20:23 ` [PATCH 0/2] allow for configuring option abbreviation + fix Ævar Arnfjörð Bjarmason
2019-03-25 20:23 ` [PATCH 1/2] parse-options: allow for configuring option abbreviation Ævar Arnfjörð Bjarmason
2019-03-25 21:23   ` Eric Sunshine
2019-03-25 22:47     ` Ævar Arnfjörð Bjarmason
2019-03-26  4:14       ` Duy Nguyen
2019-03-26  6:28         ` Ævar Arnfjörð Bjarmason
2019-03-26  7:13           ` Duy Nguyen
2019-03-26 11:00             ` Ævar Arnfjörð Bjarmason
2019-04-01 10:47     ` Junio C Hamano
2019-04-12  9:06       ` Johannes Schindelin
2019-03-25 20:23 ` [PATCH 2/2] parse-options: don't emit "ambiguous option" for aliases Ævar Arnfjörð Bjarmason
2019-04-17 12:44   ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2019-04-17 16:04     ` Duy Nguyen
2019-04-18  0:48       ` Junio C Hamano
2019-04-18  9:29         ` Duy Nguyen
2019-04-19  4:39           ` Junio C Hamano [this message]
2019-04-22 12:22             ` [PATCH] " Nguyễn Thái Ngọc Duy
2019-04-22 12:34               ` Duy Nguyen
2019-04-29 10:05               ` [PATCH v2] " Nguyễn Thái Ngọc Duy
2019-05-07  3:43                 ` Junio C Hamano
2019-05-07 11:58                   ` Duy Nguyen
2019-04-02  0:58 ` [PATCH 0/8] Do not use abbreviated options in tests Junio C Hamano
2019-04-12  9:37 ` [PATCH v2 " Johannes Schindelin via GitGitGadget
2019-04-12  9:37   ` [PATCH v2 1/8] tests (rebase): spell out the `--keep-empty` option Johannes Schindelin via GitGitGadget
2019-04-12  9:37   ` [PATCH v2 2/8] tests (rebase): spell out the `--force-rebase` option Johannes Schindelin via GitGitGadget
2019-04-12  9:37   ` [PATCH v2 3/8] t7810: do not abbreviate `--no-exclude-standard` nor `--invert-match` Johannes Schindelin via GitGitGadget
2019-04-12  9:37   ` [PATCH v2 4/8] t5531: avoid using an abbreviated option Johannes Schindelin via GitGitGadget
2019-04-12  9:37   ` [PATCH v2 5/8] tests (push): do not abbreviate the `--follow-tags` option Johannes Schindelin via GitGitGadget
2019-04-12  9:37   ` [PATCH v2 6/8] tests (status): spell out the `--find-renames` option in full Johannes Schindelin via GitGitGadget
2019-04-12  9:37   ` [PATCH v2 7/8] tests (pack-objects): use the full, unabbreviated `--revs` option Johannes Schindelin via GitGitGadget
2019-04-12  9:37   ` [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default) Johannes Schindelin via GitGitGadget
2019-04-14  2:35     ` Junio C Hamano
2019-04-15  2:55       ` Junio C Hamano
2019-04-15 13:09         ` Johannes Schindelin
2019-04-15 13:45           ` Junio C Hamano
2019-04-17 12:07             ` Johannes Schindelin
2019-04-15 13:08       ` Johannes Schindelin

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=xmqq1s1yfw24.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=liu.denton@gmail.com \
    --cc=pclouds@gmail.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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git