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

[Junio C Hamano <gitster@pobox.com>]

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