"git diff --no-rename A B"
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?
Hello,
Quoted 4 lines · 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?
Unless I'm missing something obvious, an error condition should be generated in general when an unknown command-line option is specified.
Quoted 4 lines · 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?
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.c +10 -5
@@ -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
Am 20.01.24 um 02:18 schrieb Jeff King:
Quoted 7 lines · Junio C Hamano
Quoted 5 lines
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?
Quoted 7 lines
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.
---
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(-)
parse-options.c +5 -3
@@ -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 ; }
t/t4013-diff-various.sh +6 -0
@@ -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
Quoted 8 lines · René Scharfe
Because
diff_opt_parse()passesPARSE_OPT_KEEP_UNKNOWN_OPT, which makesparse_long_opt()skip abbreviation detection. Which it does sincebaa4adc66a(parse-options: disable option abbreviation withPARSE_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.
Quoted 141 lines
--- >8 ---
Subject: [PATCH] parse-options: fully disable option abbreviation withPARSE_OPT_KEEP_UNKNOWN
baa4adc66a(parse-options: disable option abbreviation withPARSE_OPT_KEEP_UNKNOWN, 2019-01-27) turned off support for abbreviated options when the flagPARSE_OPT_KEEP_UNKNOWNis 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.
baa4adc66aonly blocked the first way. Add the condition to the other ones as well.Signed-off-by René Scharfe <[email protected]>---
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(-)parse-options.c +5
-3@@ -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 ; }t/t4013-diff-various.sh +6
-1@@ -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
Am 20.01.24 um 18:55 schrieb Junio C Hamano:
Quoted 3 lines
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.
---
Patch formatted with --function-context for easier review.
parse-options.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
parse-options.c +6 -7
@@ -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
Quoted 9 lines · René Scharfe
Quoted 4 lines
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()passesPARSE_OPT_KEEP_UNKNOWN_OPT, which makesparse_long_opt()skip abbreviation detection. Which it does sincebaa4adc66a(parse-options: disable option abbreviation withPARSE_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:
t/helper/test-parse-options.c +1 -0
@@ -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
Quoted 8 lines · Jeff King
then running:
t/helper/test-tool parse-options --docorrectly 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