René Scharfe <l.s.r@xxxxxx> 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@xxxxxx>
> ---
> 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
