[Bug?] "git diff --no-rename A B"

[Bug?] "git diff --no-rename A B"

[Junio C Hamano]

When the user misspells "--no-renames" as "--no-rename", it seems
that the rename detection (which is ont by default these days) still
kicks in, which means the misspelt option is silently ignored.
Should we show an error message instead?

Re: [Bug?] "git diff --no-rename A B"

[Dragan Simic]

Hello,

On 2024-01-18 02:07, Junio C Hamano wrote:
> When the user misspells "--no-renames" as "--no-rename", it seems
> that the rename detection (which is ont by default these days) still
> kicks in, which means the misspelt option is silently ignored.
> Should we show an error message instead?

Unless I'm missing something obvious, an error condition should be
generated in general when an unknown command-line option is specified.

Re: [Bug?] "git diff --no-rename A B"

[Jeff King]

On Wed, Jan 17, 2024 at 05:07:16PM -0800, Junio C Hamano wrote:

> When the user misspells "--no-renames" as "--no-rename", it seems
> that the rename detection (which is ont by default these days) still
> kicks in, which means the misspelt option is silently ignored.
> Should we show an error message instead?

I wondered if "--no-foo" complained, and it does. I think this is a
subtle corner case in parse-options.

The issue is that we have "--rename-empty", which of course also
provides "--no-rename-empty". And parse-options is happy to let you
abbreviate names as long as they are unambiguous. But "--no-rename" _is_
ambiguous with "--no-renames". Why don't we catch it?

I'd guess it is because we do not have "--renames" as an option, but
explicitly generate an entry for "--no-renames" (since the non-negative
version is actually "--find-renames"). I know there is some special code
to handle these pre-negated cases, but I would not be surprised if the
ambiguity checker does not.

So I think it's likely just a bug in parse-options which should be
fixed.

We could also work around it by providing --renames. ;) E.g., if we let
the find-renames callback handle negation, then --renames becomes a
synonym, like so:

diff --git a/diff.c b/diff.c
index a89a6a6128..cdec9bfbd9 100644
--- a/diff.c
+++ b/diff.c
@@ -5292,7 +5292,11 @@ static int diff_opt_find_renames(const struct option *opt,
 {
 	struct diff_options *options = opt->value;
 
-	BUG_ON_OPT_NEG(unset);
+	if (unset) {
+		options->detect_rename = 0;
+		return 0;
+	}
+
 	if (!arg)
 		arg = "";
 	options->rename_score = parse_rename_score(&arg);
@@ -5686,7 +5690,7 @@ struct option *add_diff_options(const struct option *opts,
 			       diff_opt_break_rewrites),
 		OPT_CALLBACK_F('M', "find-renames", options, N_("<n>"),
 			       N_("detect renames"),
-			       PARSE_OPT_NONEG | PARSE_OPT_OPTARG,
+			       PARSE_OPT_OPTARG,
 			       diff_opt_find_renames),
 		OPT_SET_INT_F('D', "irreversible-delete", &options->irreversible_delete,
 			      N_("omit the preimage for deletes"),
@@ -5697,9 +5701,10 @@ struct option *add_diff_options(const struct option *opts,
 			       diff_opt_find_copies),
 		OPT_BOOL(0, "find-copies-harder", &options->flags.find_copies_harder,
 			 N_("use unmodified files as source to find copies")),
-		OPT_SET_INT_F(0, "no-renames", &options->detect_rename,
-			      N_("disable rename detection"),
-			      0, PARSE_OPT_NONEG),
+		OPT_CALLBACK_F(0, "renames", options, N_("<n>"),
+			       N_("synonym for --find-renames"),
+			       PARSE_OPT_OPTARG,
+			       diff_opt_find_renames),
 		OPT_BOOL(0, "rename-empty", &options->flags.rename_empty,
 			 N_("use empty blobs as rename source")),
 		OPT_CALLBACK_F(0, "follow", options, NULL,

And you get the expected output:

  $ git show f920b0289b --oneline --raw --no-rename
  error: ambiguous option: no-rename (could be --no-renames or --no-rename-empty)

And as a bonus, now "--renames" works. :) It might pollute the output of
"-h" more, but I am not sure if we ever actually show these diff options
via "-h" (they are parsed quite indirectly, and "-h" is handled by the
main command's parse-options list).

Still, it seems like it's worth fixing the parse-options bug.

-Peff

Re: [Bug?] "git diff --no-rename A B"

[René Scharfe]

Am 20.01.24 um 02:18 schrieb Jeff King:
> On Wed, Jan 17, 2024 at 05:07:16PM -0800, Junio C Hamano wrote:
>
>> When the user misspells "--no-renames" as "--no-rename", it seems
>> that the rename detection (which is ont by default these days) still
>> kicks in, which means the misspelt option is silently ignored.
>> Should we show an error message instead?
>
> I wondered if "--no-foo" complained, and it does. I think this is a
> subtle corner case in parse-options.
>
> The issue is that we have "--rename-empty", which of course also
> provides "--no-rename-empty". And parse-options is happy to let you
> abbreviate names as long as they are unambiguous. But "--no-rename" _is_
> ambiguous with "--no-renames". Why don't we catch it?

Because diff_opt_parse() passes PARSE_OPT_KEEP_UNKNOWN_OPT, which makes
parse_long_opt() skip abbreviation detection.  Which it does since
baa4adc66a (parse-options: disable option abbreviation with
PARSE_OPT_KEEP_UNKNOWN, 2019-01-27).

It added a condition only to the if.  Its body can be reached via goto
from two other places, though.  So abbreviated options are effectively
allowed through the back door.

--- >8 ---
Subject: [PATCH] parse-options: fully disable option abbreviation with PARSE_OPT_KEEP_UNKNOWN

baa4adc66a (parse-options: disable option abbreviation with
PARSE_OPT_KEEP_UNKNOWN, 2019-01-27) turned off support for abbreviated
options when the flag PARSE_OPT_KEEP_UNKNOWN is given, as any shortened
option could also be an abbreviation for one of the unknown options.

The code for handling abbreviated options is guarded by an if, but it
can also be reached via goto.  baa4adc66a only blocked the first way.
Add the condition to the other ones as well.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
Formatted with --function-context for easier review.
The code is a quite tangled, any ideas how to structure it better?

 parse-options.c         | 8 +++++---
 t/t4013-diff-various.sh | 6 ++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 4ce2b7ca16..92e96ca6cd 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -353,93 +353,95 @@ static int is_alias(struct parse_opt_ctx_t *ctx,
 static enum parse_opt_result parse_long_opt(
 	struct parse_opt_ctx_t *p, const char *arg,
 	const struct option *options)
 {
 	const char *arg_end = strchrnul(arg, '=');
 	const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
 	enum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG;
+	int allow_abbrev = !(p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT);

 	for (; options->type != OPTION_END; options++) {
 		const char *rest, *long_name = options->long_name;
 		enum opt_parsed flags = OPT_LONG, opt_flags = OPT_LONG;

 		if (options->type == OPTION_SUBCOMMAND)
 			continue;
 		if (!long_name)
 			continue;

 again:
 		if (!skip_prefix(arg, long_name, &rest))
 			rest = NULL;
 		if (!rest) {
 			/* abbreviated? */
-			if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT) &&
+			if (allow_abbrev &&
 			    !strncmp(long_name, arg, arg_end - arg)) {
 is_abbreviated:
 				if (abbrev_option &&
 				    !is_alias(p, abbrev_option, options)) {
 					/*
 					 * If this is abbreviated, it is
 					 * ambiguous. So when there is no
 					 * exact match later, we need to
 					 * error out.
 					 */
 					ambiguous_option = abbrev_option;
 					ambiguous_flags = abbrev_flags;
 				}
 				if (!(flags & OPT_UNSET) && *arg_end)
 					p->opt = arg_end + 1;
 				abbrev_option = options;
 				abbrev_flags = flags ^ opt_flags;
 				continue;
 			}
 			/* negation allowed? */
 			if (options->flags & PARSE_OPT_NONEG)
 				continue;
 			/* negated and abbreviated very much? */
-			if (starts_with("no-", arg)) {
+			if (allow_abbrev && starts_with("no-", arg)) {
 				flags |= OPT_UNSET;
 				goto is_abbreviated;
 			}
 			/* negated? */
 			if (!starts_with(arg, "no-")) {
 				if (skip_prefix(long_name, "no-", &long_name)) {
 					opt_flags |= OPT_UNSET;
 					goto again;
 				}
 				continue;
 			}
 			flags |= OPT_UNSET;
 			if (!skip_prefix(arg + 3, long_name, &rest)) {
 				/* abbreviated and negated? */
-				if (starts_with(long_name, arg + 3))
+				if (allow_abbrev &&
+				    starts_with(long_name, arg + 3))
 					goto is_abbreviated;
 				else
 					continue;
 			}
 		}
 		if (*rest) {
 			if (*rest != '=')
 				continue;
 			p->opt = rest + 1;
 		}
 		return get_value(p, options, flags ^ opt_flags);
 	}

 	if (disallow_abbreviated_options && (ambiguous_option || abbrev_option))
 		die("disallowed abbreviated or ambiguous option '%.*s'",
 		    (int)(arg_end - arg), arg);

 	if (ambiguous_option) {
 		error(_("ambiguous option: %s "
 			"(could be --%s%s or --%s%s)"),
 			arg,
 			(ambiguous_flags & OPT_UNSET) ?  "no-" : "",
 			ambiguous_option->long_name,
 			(abbrev_flags & OPT_UNSET) ?  "no-" : "",
 			abbrev_option->long_name);
 		return PARSE_OPT_HELP;
 	}
 	if (abbrev_option)
 		return get_value(p, abbrev_option, abbrev_flags);
 	return PARSE_OPT_UNKNOWN;
 }
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index cb094241ec..1e3b2dbea4 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -663,4 +663,10 @@ test_expect_success 'diff --default-prefix overrides diff.mnemonicprefix' '
 	check_prefix actual a/file0 b/file0
 '

+test_expect_success 'diff --no-renames cannot be abbreviated' '
+	test_expect_code 129 git diff --no-rename >actual 2>error &&
+	test_must_be_empty actual &&
+	grep "invalid option: --no-rename" error
+'
+
 test_done
--
2.43.0

Re: [Bug?] "git diff --no-rename A B"

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Because diff_opt_parse() passes PARSE_OPT_KEEP_UNKNOWN_OPT, which makes
> parse_long_opt() skip abbreviation detection.  Which it does since
> baa4adc66a (parse-options: disable option abbreviation with
> PARSE_OPT_KEEP_UNKNOWN, 2019-01-27).
>
> It added a condition only to the if.  Its body can be reached via goto
> from two other places, though.  So abbreviated options are effectively
> allowed through the back door.

Wow, that is nasty.  Thanks for spotting.

I agree with you that the structure of that loop and the processing
in it does look confusing.

> --- >8 ---
> Subject: [PATCH] parse-options: fully disable option abbreviation with PARSE_OPT_KEEP_UNKNOWN
>
> baa4adc66a (parse-options: disable option abbreviation with
> PARSE_OPT_KEEP_UNKNOWN, 2019-01-27) turned off support for abbreviated
> options when the flag PARSE_OPT_KEEP_UNKNOWN is given, as any shortened
> option could also be an abbreviation for one of the unknown options.
>
> The code for handling abbreviated options is guarded by an if, but it
> can also be reached via goto.  baa4adc66a only blocked the first way.
> Add the condition to the other ones as well.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Formatted with --function-context for easier review.
> The code is a quite tangled, any ideas how to structure it better?
>
>  parse-options.c         | 8 +++++---
>  t/t4013-diff-various.sh | 6 ++++++
>  2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index 4ce2b7ca16..92e96ca6cd 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -353,93 +353,95 @@ static int is_alias(struct parse_opt_ctx_t *ctx,
>  static enum parse_opt_result parse_long_opt(
>  	struct parse_opt_ctx_t *p, const char *arg,
>  	const struct option *options)
>  {
>  	const char *arg_end = strchrnul(arg, '=');
>  	const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
>  	enum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG;
> +	int allow_abbrev = !(p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT);
>
>  	for (; options->type != OPTION_END; options++) {
>  		const char *rest, *long_name = options->long_name;
>  		enum opt_parsed flags = OPT_LONG, opt_flags = OPT_LONG;
>
>  		if (options->type == OPTION_SUBCOMMAND)
>  			continue;
>  		if (!long_name)
>  			continue;
>
>  again:
>  		if (!skip_prefix(arg, long_name, &rest))
>  			rest = NULL;
>  		if (!rest) {
>  			/* abbreviated? */
> -			if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT) &&
> +			if (allow_abbrev &&
>  			    !strncmp(long_name, arg, arg_end - arg)) {
>  is_abbreviated:
>  				if (abbrev_option &&
>  				    !is_alias(p, abbrev_option, options)) {
>  					/*
>  					 * If this is abbreviated, it is
>  					 * ambiguous. So when there is no
>  					 * exact match later, we need to
>  					 * error out.
>  					 */
>  					ambiguous_option = abbrev_option;
>  					ambiguous_flags = abbrev_flags;
>  				}
>  				if (!(flags & OPT_UNSET) && *arg_end)
>  					p->opt = arg_end + 1;
>  				abbrev_option = options;
>  				abbrev_flags = flags ^ opt_flags;
>  				continue;
>  			}
>  			/* negation allowed? */
>  			if (options->flags & PARSE_OPT_NONEG)
>  				continue;
>  			/* negated and abbreviated very much? */
> -			if (starts_with("no-", arg)) {
> +			if (allow_abbrev && starts_with("no-", arg)) {
>  				flags |= OPT_UNSET;
>  				goto is_abbreviated;
>  			}
>  			/* negated? */
>  			if (!starts_with(arg, "no-")) {
>  				if (skip_prefix(long_name, "no-", &long_name)) {
>  					opt_flags |= OPT_UNSET;
>  					goto again;
>  				}
>  				continue;
>  			}
>  			flags |= OPT_UNSET;
>  			if (!skip_prefix(arg + 3, long_name, &rest)) {
>  				/* abbreviated and negated? */
> -				if (starts_with(long_name, arg + 3))
> +				if (allow_abbrev &&
> +				    starts_with(long_name, arg + 3))
>  					goto is_abbreviated;
>  				else
>  					continue;
>  			}
>  		}
>  		if (*rest) {
>  			if (*rest != '=')
>  				continue;
>  			p->opt = rest + 1;
>  		}
>  		return get_value(p, options, flags ^ opt_flags);
>  	}
>
>  	if (disallow_abbreviated_options && (ambiguous_option || abbrev_option))
>  		die("disallowed abbreviated or ambiguous option '%.*s'",
>  		    (int)(arg_end - arg), arg);
>
>  	if (ambiguous_option) {
>  		error(_("ambiguous option: %s "
>  			"(could be --%s%s or --%s%s)"),
>  			arg,
>  			(ambiguous_flags & OPT_UNSET) ?  "no-" : "",
>  			ambiguous_option->long_name,
>  			(abbrev_flags & OPT_UNSET) ?  "no-" : "",
>  			abbrev_option->long_name);
>  		return PARSE_OPT_HELP;
>  	}
>  	if (abbrev_option)
>  		return get_value(p, abbrev_option, abbrev_flags);
>  	return PARSE_OPT_UNKNOWN;
>  }
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index cb094241ec..1e3b2dbea4 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -663,4 +663,10 @@ test_expect_success 'diff --default-prefix overrides diff.mnemonicprefix' '
>  	check_prefix actual a/file0 b/file0
>  '
>
> +test_expect_success 'diff --no-renames cannot be abbreviated' '
> +	test_expect_code 129 git diff --no-rename >actual 2>error &&
> +	test_must_be_empty actual &&
> +	grep "invalid option: --no-rename" error
> +'
> +
>  test_done
> --
> 2.43.0

Re: [Bug?] "git diff --no-rename A B"

[René Scharfe]

Am 20.01.24 um 18:55 schrieb Junio C Hamano:
>
> I agree with you that the structure of that loop and the processing
> in it does look confusing.

Here's a small simplification.

--- >8 ---
Subject: [PATCH 2/1] parse-options: simplify positivation handling

We accept the positive version of options whose long name starts with
"no-" and are defined without the flag PARSE_OPT_NONEG.  E.g. git clone
has an explicitly defined --no-checkout option and also implicitly
accepts --checkout to override it.

parse_long_opt() handles that by restarting the option matching with the
positive version when it finds that only the current option definition
starts with "no-", but not the user-supplied argument.  This code is
located almost at the end of the matching logic.

Avoid the need for a restart by moving the code up.  We don't have to
check the positive arg against the negative long_name at all -- the
"no-" prefix of the latter makes a match impossible.  Skip it and toggle
OPT_UNSET right away to simplify the control flow.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
Patch formatted with --function-context for easier review.

 parse-options.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 92e96ca6cd..63a99dea6e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -353,95 +353,94 @@ static int is_alias(struct parse_opt_ctx_t *ctx,
 static enum parse_opt_result parse_long_opt(
 	struct parse_opt_ctx_t *p, const char *arg,
 	const struct option *options)
 {
 	const char *arg_end = strchrnul(arg, '=');
 	const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
 	enum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG;
 	int allow_abbrev = !(p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT);

 	for (; options->type != OPTION_END; options++) {
 		const char *rest, *long_name = options->long_name;
 		enum opt_parsed flags = OPT_LONG, opt_flags = OPT_LONG;

 		if (options->type == OPTION_SUBCOMMAND)
 			continue;
 		if (!long_name)
 			continue;

-again:
+		if (!starts_with(arg, "no-") &&
+		    !(options->flags & PARSE_OPT_NONEG) &&
+		    skip_prefix(long_name, "no-", &long_name))
+			opt_flags |= OPT_UNSET;
+
 		if (!skip_prefix(arg, long_name, &rest))
 			rest = NULL;
 		if (!rest) {
 			/* abbreviated? */
 			if (allow_abbrev &&
 			    !strncmp(long_name, arg, arg_end - arg)) {
 is_abbreviated:
 				if (abbrev_option &&
 				    !is_alias(p, abbrev_option, options)) {
 					/*
 					 * If this is abbreviated, it is
 					 * ambiguous. So when there is no
 					 * exact match later, we need to
 					 * error out.
 					 */
 					ambiguous_option = abbrev_option;
 					ambiguous_flags = abbrev_flags;
 				}
 				if (!(flags & OPT_UNSET) && *arg_end)
 					p->opt = arg_end + 1;
 				abbrev_option = options;
 				abbrev_flags = flags ^ opt_flags;
 				continue;
 			}
 			/* negation allowed? */
 			if (options->flags & PARSE_OPT_NONEG)
 				continue;
 			/* negated and abbreviated very much? */
 			if (allow_abbrev && starts_with("no-", arg)) {
 				flags |= OPT_UNSET;
 				goto is_abbreviated;
 			}
 			/* negated? */
-			if (!starts_with(arg, "no-")) {
-				if (skip_prefix(long_name, "no-", &long_name)) {
-					opt_flags |= OPT_UNSET;
-					goto again;
-				}
+			if (!starts_with(arg, "no-"))
 				continue;
-			}
 			flags |= OPT_UNSET;
 			if (!skip_prefix(arg + 3, long_name, &rest)) {
 				/* abbreviated and negated? */
 				if (allow_abbrev &&
 				    starts_with(long_name, arg + 3))
 					goto is_abbreviated;
 				else
 					continue;
 			}
 		}
 		if (*rest) {
 			if (*rest != '=')
 				continue;
 			p->opt = rest + 1;
 		}
 		return get_value(p, options, flags ^ opt_flags);
 	}

 	if (disallow_abbreviated_options && (ambiguous_option || abbrev_option))
 		die("disallowed abbreviated or ambiguous option '%.*s'",
 		    (int)(arg_end - arg), arg);

 	if (ambiguous_option) {
 		error(_("ambiguous option: %s "
 			"(could be --%s%s or --%s%s)"),
 			arg,
 			(ambiguous_flags & OPT_UNSET) ?  "no-" : "",
 			ambiguous_option->long_name,
 			(abbrev_flags & OPT_UNSET) ?  "no-" : "",
 			abbrev_option->long_name);
 		return PARSE_OPT_HELP;
 	}
 	if (abbrev_option)
 		return get_value(p, abbrev_option, abbrev_flags);
 	return PARSE_OPT_UNKNOWN;
 }
--
2.43.0

Re: [Bug?] "git diff --no-rename A B"

[Jeff King]

On Sat, Jan 20, 2024 at 03:39:38PM +0100, René Scharfe wrote:

> > The issue is that we have "--rename-empty", which of course also
> > provides "--no-rename-empty". And parse-options is happy to let you
> > abbreviate names as long as they are unambiguous. But "--no-rename" _is_
> > ambiguous with "--no-renames". Why don't we catch it?
> 
> Because diff_opt_parse() passes PARSE_OPT_KEEP_UNKNOWN_OPT, which makes
> parse_long_opt() skip abbreviation detection.  Which it does since
> baa4adc66a (parse-options: disable option abbreviation with
> PARSE_OPT_KEEP_UNKNOWN, 2019-01-27).

OK, it makes sense to me that we should avoid abbreviation entirely with
KEEP_UNKNOWN_OPT, for the reasons given in that commit. But if adding
--rename fixed it, is there another bug lurking? That is, would we do
the wrong thing on a case without KEEP_UNKNOWN_OPT but which had
"--renames" and "--no-rename" defined? Or was it simply the
inconsistency in how KEEP_UNKNOWN_OPT is being applied?

I think it might just be the latter. If I do this:

diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index ded8116cc5..e908c7386d 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -124,6 +124,7 @@ int cmd__parse_options(int argc, const char **argv)
 	struct option options[] = {
 		OPT_BOOL(0, "yes", &boolean, "get a boolean"),
 		OPT_BOOL('D', "no-doubt", &boolean, "begins with 'no-'"),
+		OPT_BOOL(0, "do-it", &boolean, "'do' ambiguous with 'doubt'"),
 		{ OPTION_SET_INT, 'B', "no-fear", &boolean, NULL,
 		  "be brave", PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },
 		OPT_COUNTUP('b', "boolean", &boolean, "increment by one"),

then running:

  t/helper/test-tool parse-options --do

correctly complains about the ambiguity (though amusingly it mentions
"--no-no-doubt" in the error message). And if I add KEEP_UNKNOWN_OPT,
then it gives the wrong behavior. But curiously it does so even with
your patch applied. So I think there may be further fixes needed.

-Peff

Re: [Bug?] "git diff --no-rename A B"

[Jeff King]

On Mon, Jan 22, 2024 at 06:00:51PM -0500, Jeff King wrote:

> then running:
> 
>   t/helper/test-tool parse-options --do
> 
> correctly complains about the ambiguity (though amusingly it mentions
> "--no-no-doubt" in the error message). And if I add KEEP_UNKNOWN_OPT,
> then it gives the wrong behavior. But curiously it does so even with
> your patch applied. So I think there may be further fixes needed.

Oh sorry, I'm stupid. Of course it does not complain with
KEEP_UNKNOWN_OPT. It treats it as unknown and keeps it!

So with the patch I showed (and adding in KEEP_UNKNOWN_OPT), even
without your patch "--do" is handled correctly (it is unknown and left
in argv[0]). It is only "--no-do" which is broken, and your patch fixes
that.

Sorry for the noise. :)

-Peff