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