{"thread":{"id":"60757","subject":"[Bug?] \"git diff --no-rename A B\"","startedAt":"2024-01-18T01:07:19Z","lastAt":"2024-01-23T00:28:25Z","messageCount":8,"participants":["Junio C Hamano","Dragan Simic","Jeff King","René Scharfe"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"486948","messageId":"xmqq34uvtpob.fsf@gitster.g","threadId":"60757","inReplyTo":null,"subject":"[Bug?] \"git diff --no-rename A B\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-18T01:07:16Z","receivedAt":"2024-01-18T01:07:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When the user misspells \"--no-renames\" as \"--no-rename\", it seems\nthat the rename detection (which is ont by default these days) still\nkicks in, which means the misspelt option is silently ignored.\nShould we show an error message instead?\n"},{"id":"486956","messageId":"b14b9ed73398505f44e1c38434a78041@manjaro.org","threadId":"60757","inReplyTo":"xmqq34uvtpob.fsf@gitster.g","subject":"Re: [Bug?] \"git diff --no-rename A B\"","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-01-18T06:26:22Z","receivedAt":"2024-01-18T06:26:24Z","isPatch":false,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Hello,\n\nOn 2024-01-18 02:07, Junio C Hamano wrote:\n> When the user misspells \"--no-renames\" as \"--no-rename\", it seems\n> that the rename detection (which is ont by default these days) still\n> kicks in, which means the misspelt option is silently ignored.\n> Should we show an error message instead?\n\nUnless I'm missing something obvious, an error condition should be\ngenerated in general when an unknown command-line option is specified.\n"},{"id":"487125","messageId":"20240120011800.GF117170@coredump.intra.peff.net","threadId":"60757","inReplyTo":"xmqq34uvtpob.fsf@gitster.g","subject":"Re: [Bug?] \"git diff --no-rename A B\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-20T01:18:00Z","receivedAt":"2024-01-20T01:18:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 17, 2024 at 05:07:16PM -0800, Junio C Hamano wrote:\n\n> When the user misspells \"--no-renames\" as \"--no-rename\", it seems\n> that the rename detection (which is ont by default these days) still\n> kicks in, which means the misspelt option is silently ignored.\n> Should we show an error message instead?\n\nI wondered if \"--no-foo\" complained, and it does. I think this is a\nsubtle corner case in parse-options.\n\nThe issue is that we have \"--rename-empty\", which of course also\nprovides \"--no-rename-empty\". And parse-options is happy to let you\nabbreviate names as long as they are unambiguous. But \"--no-rename\" _is_\nambiguous with \"--no-renames\". Why don't we catch it?\n\nI'd guess it is because we do not have \"--renames\" as an option, but\nexplicitly generate an entry for \"--no-renames\" (since the non-negative\nversion is actually \"--find-renames\"). I know there is some special code\nto handle these pre-negated cases, but I would not be surprised if the\nambiguity checker does not.\n\nSo I think it's likely just a bug in parse-options which should be\nfixed.\n\nWe could also work around it by providing --renames. ;) E.g., if we let\nthe find-renames callback handle negation, then --renames becomes a\nsynonym, like so:\n\ndiff --git a/diff.c b/diff.c\nindex a89a6a6128..cdec9bfbd9 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5292,7 +5292,11 @@ static int diff_opt_find_renames(const struct option *opt,\n {\n \tstruct diff_options *options = opt->value;\n \n-\tBUG_ON_OPT_NEG(unset);\n+\tif (unset) {\n+\t\toptions->detect_rename = 0;\n+\t\treturn 0;\n+\t}\n+\n \tif (!arg)\n \t\targ = \"\";\n \toptions->rename_score = parse_rename_score(&arg);\n@@ -5686,7 +5690,7 @@ struct option *add_diff_options(const struct option *opts,\n \t\t\t       diff_opt_break_rewrites),\n \t\tOPT_CALLBACK_F('M', \"find-renames\", options, N_(\"<n>\"),\n \t\t\t       N_(\"detect renames\"),\n-\t\t\t       PARSE_OPT_NONEG | PARSE_OPT_OPTARG,\n+\t\t\t       PARSE_OPT_OPTARG,\n \t\t\t       diff_opt_find_renames),\n \t\tOPT_SET_INT_F('D', \"irreversible-delete\", &options->irreversible_delete,\n \t\t\t      N_(\"omit the preimage for deletes\"),\n@@ -5697,9 +5701,10 @@ struct option *add_diff_options(const struct option *opts,\n \t\t\t       diff_opt_find_copies),\n \t\tOPT_BOOL(0, \"find-copies-harder\", &options->flags.find_copies_harder,\n \t\t\t N_(\"use unmodified files as source to find copies\")),\n-\t\tOPT_SET_INT_F(0, \"no-renames\", &options->detect_rename,\n-\t\t\t      N_(\"disable rename detection\"),\n-\t\t\t      0, PARSE_OPT_NONEG),\n+\t\tOPT_CALLBACK_F(0, \"renames\", options, N_(\"<n>\"),\n+\t\t\t       N_(\"synonym for --find-renames\"),\n+\t\t\t       PARSE_OPT_OPTARG,\n+\t\t\t       diff_opt_find_renames),\n \t\tOPT_BOOL(0, \"rename-empty\", &options->flags.rename_empty,\n \t\t\t N_(\"use empty blobs as rename source\")),\n \t\tOPT_CALLBACK_F(0, \"follow\", options, NULL,\n\nAnd you get the expected output:\n\n  $ git show f920b0289b --oneline --raw --no-rename\n  error: ambiguous option: no-rename (could be --no-renames or --no-rename-empty)\n\nAnd as a bonus, now \"--renames\" works. :) It might pollute the output of\n\"-h\" more, but I am not sure if we ever actually show these diff options\nvia \"-h\" (they are parsed quite indirectly, and \"-h\" is handled by the\nmain command's parse-options list).\n\nStill, it seems like it's worth fixing the parse-options bug.\n\n-Peff\n"},{"id":"487136","messageId":"579fd5bc-3bfd-427f-b22d-dab5e7e56eb2@web.de","threadId":"60757","inReplyTo":"20240120011800.GF117170@coredump.intra.peff.net","subject":"Re: [Bug?] \"git diff --no-rename A B\"","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-01-20T14:39:38Z","receivedAt":"2024-01-20T14:45:13Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 20.01.24 um 02:18 schrieb Jeff King:\n> On Wed, Jan 17, 2024 at 05:07:16PM -0800, Junio C Hamano wrote:\n>\n>> When the user misspells \"--no-renames\" as \"--no-rename\", it seems\n>> that the rename detection (which is ont by default these days) still\n>> kicks in, which means the misspelt option is silently ignored.\n>> Should we show an error message instead?\n>\n> I wondered if \"--no-foo\" complained, and it does. I think this is a\n> subtle corner case in parse-options.\n>\n> The issue is that we have \"--rename-empty\", which of course also\n> provides \"--no-rename-empty\". And parse-options is happy to let you\n> abbreviate names as long as they are unambiguous. But \"--no-rename\" _is_\n> ambiguous with \"--no-renames\". Why don't we catch it?\n\nBecause diff_opt_parse() passes PARSE_OPT_KEEP_UNKNOWN_OPT, which makes\nparse_long_opt() skip abbreviation detection.  Which it does since\nbaa4adc66a (parse-options: disable option abbreviation with\nPARSE_OPT_KEEP_UNKNOWN, 2019-01-27).\n\nIt added a condition only to the if.  Its body can be reached via goto\nfrom two other places, though.  So abbreviated options are effectively\nallowed through the back door.\n\n--- >8 ---\nSubject: [PATCH] parse-options: fully disable option abbreviation with PARSE_OPT_KEEP_UNKNOWN\n\nbaa4adc66a (parse-options: disable option abbreviation with\nPARSE_OPT_KEEP_UNKNOWN, 2019-01-27) turned off support for abbreviated\noptions when the flag PARSE_OPT_KEEP_UNKNOWN is given, as any shortened\noption could also be an abbreviation for one of the unknown options.\n\nThe code for handling abbreviated options is guarded by an if, but it\ncan also be reached via goto.  baa4adc66a only blocked the first way.\nAdd the condition to the other ones as well.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nFormatted with --function-context for easier review.\nThe code is a quite tangled, any ideas how to structure it better?\n\n parse-options.c         | 8 +++++---\n t/t4013-diff-various.sh | 6 ++++++\n 2 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 4ce2b7ca16..92e96ca6cd 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -353,93 +353,95 @@ static int is_alias(struct parse_opt_ctx_t *ctx,\n static enum parse_opt_result parse_long_opt(\n \tstruct parse_opt_ctx_t *p, const char *arg,\n \tconst struct option *options)\n {\n \tconst char *arg_end = strchrnul(arg, '=');\n \tconst struct option *abbrev_option = NULL, *ambiguous_option = NULL;\n \tenum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG;\n+\tint allow_abbrev = !(p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT);\n\n \tfor (; options->type != OPTION_END; options++) {\n \t\tconst char *rest, *long_name = options->long_name;\n \t\tenum opt_parsed flags = OPT_LONG, opt_flags = OPT_LONG;\n\n \t\tif (options->type == OPTION_SUBCOMMAND)\n \t\t\tcontinue;\n \t\tif (!long_name)\n \t\t\tcontinue;\n\n again:\n \t\tif (!skip_prefix(arg, long_name, &rest))\n \t\t\trest = NULL;\n \t\tif (!rest) {\n \t\t\t/* abbreviated? */\n-\t\t\tif (!(p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT) &&\n+\t\t\tif (allow_abbrev &&\n \t\t\t    !strncmp(long_name, arg, arg_end - arg)) {\n is_abbreviated:\n \t\t\t\tif (abbrev_option &&\n \t\t\t\t    !is_alias(p, abbrev_option, options)) {\n \t\t\t\t\t/*\n \t\t\t\t\t * If this is abbreviated, it is\n \t\t\t\t\t * ambiguous. So when there is no\n \t\t\t\t\t * exact match later, we need to\n \t\t\t\t\t * error out.\n \t\t\t\t\t */\n \t\t\t\t\tambiguous_option = abbrev_option;\n \t\t\t\t\tambiguous_flags = abbrev_flags;\n \t\t\t\t}\n \t\t\t\tif (!(flags & OPT_UNSET) && *arg_end)\n \t\t\t\t\tp->opt = arg_end + 1;\n \t\t\t\tabbrev_option = options;\n \t\t\t\tabbrev_flags = flags ^ opt_flags;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\t/* negation allowed? */\n \t\t\tif (options->flags & PARSE_OPT_NONEG)\n \t\t\t\tcontinue;\n \t\t\t/* negated and abbreviated very much? */\n-\t\t\tif (starts_with(\"no-\", arg)) {\n+\t\t\tif (allow_abbrev && starts_with(\"no-\", arg)) {\n \t\t\t\tflags |= OPT_UNSET;\n \t\t\t\tgoto is_abbreviated;\n \t\t\t}\n \t\t\t/* negated? */\n \t\t\tif (!starts_with(arg, \"no-\")) {\n \t\t\t\tif (skip_prefix(long_name, \"no-\", &long_name)) {\n \t\t\t\t\topt_flags |= OPT_UNSET;\n \t\t\t\t\tgoto again;\n \t\t\t\t}\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tflags |= OPT_UNSET;\n \t\t\tif (!skip_prefix(arg + 3, long_name, &rest)) {\n \t\t\t\t/* abbreviated and negated? */\n-\t\t\t\tif (starts_with(long_name, arg + 3))\n+\t\t\t\tif (allow_abbrev &&\n+\t\t\t\t    starts_with(long_name, arg + 3))\n \t\t\t\t\tgoto is_abbreviated;\n \t\t\t\telse\n \t\t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n \t\tif (*rest) {\n \t\t\tif (*rest != '=')\n \t\t\t\tcontinue;\n \t\t\tp->opt = rest + 1;\n \t\t}\n \t\treturn get_value(p, options, flags ^ opt_flags);\n \t}\n\n \tif (disallow_abbreviated_options && (ambiguous_option || abbrev_option))\n \t\tdie(\"disallowed abbreviated or ambiguous option '%.*s'\",\n \t\t    (int)(arg_end - arg), arg);\n\n \tif (ambiguous_option) {\n \t\terror(_(\"ambiguous option: %s \"\n \t\t\t\"(could be --%s%s or --%s%s)\"),\n \t\t\targ,\n \t\t\t(ambiguous_flags & OPT_UNSET) ?  \"no-\" : \"\",\n \t\t\tambiguous_option->long_name,\n \t\t\t(abbrev_flags & OPT_UNSET) ?  \"no-\" : \"\",\n \t\t\tabbrev_option->long_name);\n \t\treturn PARSE_OPT_HELP;\n \t}\n \tif (abbrev_option)\n \t\treturn get_value(p, abbrev_option, abbrev_flags);\n \treturn PARSE_OPT_UNKNOWN;\n }\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex cb094241ec..1e3b2dbea4 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -663,4 +663,10 @@ test_expect_success 'diff --default-prefix overrides diff.mnemonicprefix' '\n \tcheck_prefix actual a/file0 b/file0\n '\n\n+test_expect_success 'diff --no-renames cannot be abbreviated' '\n+\ttest_expect_code 129 git diff --no-rename >actual 2>error &&\n+\ttest_must_be_empty actual &&\n+\tgrep \"invalid option: --no-rename\" error\n+'\n+\n test_done\n--\n2.43.0\n"},{"id":"487141","messageId":"xmqqmsszga9m.fsf@gitster.g","threadId":"60757","inReplyTo":"579fd5bc-3bfd-427f-b22d-dab5e7e56eb2@web.de","subject":"Re: [Bug?] \"git diff --no-rename A B\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-20T17:55:17Z","receivedAt":"2024-01-20T17:55:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Because diff_opt_parse() passes PARSE_OPT_KEEP_UNKNOWN_OPT, which makes\n> parse_long_opt() skip abbreviation detection.  Which it does since\n> baa4adc66a (parse-options: disable option abbreviation with\n> PARSE_OPT_KEEP_UNKNOWN, 2019-01-27).\n>\n> It added a condition only to the if.  Its body can be reached via goto\n> from two other places, though.  So abbreviated options are effectively\n> allowed through the back door.\n\nWow, that is nasty.  Thanks for spotting.\n\nI agree with you that the structure of that loop and the processing\nin it does look confusing.\n\n> --- >8 ---\n> Subject: [PATCH] parse-options: fully disable option abbreviation with PARSE_OPT_KEEP_UNKNOWN\n>\n> baa4adc66a (parse-options: disable option abbreviation with\n> PARSE_OPT_KEEP_UNKNOWN, 2019-01-27) turned off support for abbreviated\n> options when the flag PARSE_OPT_KEEP_UNKNOWN is given, as any shortened\n> option could also be an abbreviation for one of the unknown options.\n>\n> The code for handling abbreviated options is guarded by an if, but it\n> can also be reached via goto.  baa4adc66a only blocked the first way.\n> Add the condition to the other ones as well.\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n> Formatted with --function-context for easier review.\n> The code is a quite tangled, any ideas how to structure it better?\n>\n>  parse-options.c         | 8 +++++---\n>  t/t4013-diff-various.sh | 6 ++++++\n>  2 files changed, 11 insertions(+), 3 deletions(-)\n>\n> diff --git a/parse-options.c b/parse-options.c\n> index 4ce2b7ca16..92e96ca6cd 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -353,93 +353,95 @@ static int is_alias(struct parse_opt_ctx_t *ctx,\n>  static enum parse_opt_result parse_long_opt(\n>  \tstruct parse_opt_ctx_t *p, const char *arg,\n>  \tconst struct option *options)\n>  {\n>  \tconst char *arg_end = strchrnul(arg, '=');\n>  \tconst struct option *abbrev_option = NULL, *ambiguous_option = NULL;\n>  \tenum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG;\n> +\tint allow_abbrev = !(p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT);\n>\n>  \tfor (; options->type != OPTION_END; options++) {\n>  \t\tconst char *rest, *long_name = options->long_name;\n>  \t\tenum opt_parsed flags = OPT_LONG, opt_flags = OPT_LONG;\n>\n>  \t\tif (options->type == OPTION_SUBCOMMAND)\n>  \t\t\tcontinue;\n>  \t\tif (!long_name)\n>  \t\t\tcontinue;\n>\n>  again:\n>  \t\tif (!skip_prefix(arg, long_name, &rest))\n>  \t\t\trest = NULL;\n>  \t\tif (!rest) {\n>  \t\t\t/* abbreviated? */\n> -\t\t\tif (!(p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT) &&\n> +\t\t\tif (allow_abbrev &&\n>  \t\t\t    !strncmp(long_name, arg, arg_end - arg)) {\n>  is_abbreviated:\n>  \t\t\t\tif (abbrev_option &&\n>  \t\t\t\t    !is_alias(p, abbrev_option, options)) {\n>  \t\t\t\t\t/*\n>  \t\t\t\t\t * If this is abbreviated, it is\n>  \t\t\t\t\t * ambiguous. So when there is no\n>  \t\t\t\t\t * exact match later, we need to\n>  \t\t\t\t\t * error out.\n>  \t\t\t\t\t */\n>  \t\t\t\t\tambiguous_option = abbrev_option;\n>  \t\t\t\t\tambiguous_flags = abbrev_flags;\n>  \t\t\t\t}\n>  \t\t\t\tif (!(flags & OPT_UNSET) && *arg_end)\n>  \t\t\t\t\tp->opt = arg_end + 1;\n>  \t\t\t\tabbrev_option = options;\n>  \t\t\t\tabbrev_flags = flags ^ opt_flags;\n>  \t\t\t\tcontinue;\n>  \t\t\t}\n>  \t\t\t/* negation allowed? */\n>  \t\t\tif (options->flags & PARSE_OPT_NONEG)\n>  \t\t\t\tcontinue;\n>  \t\t\t/* negated and abbreviated very much? */\n> -\t\t\tif (starts_with(\"no-\", arg)) {\n> +\t\t\tif (allow_abbrev && starts_with(\"no-\", arg)) {\n>  \t\t\t\tflags |= OPT_UNSET;\n>  \t\t\t\tgoto is_abbreviated;\n>  \t\t\t}\n>  \t\t\t/* negated? */\n>  \t\t\tif (!starts_with(arg, \"no-\")) {\n>  \t\t\t\tif (skip_prefix(long_name, \"no-\", &long_name)) {\n>  \t\t\t\t\topt_flags |= OPT_UNSET;\n>  \t\t\t\t\tgoto again;\n>  \t\t\t\t}\n>  \t\t\t\tcontinue;\n>  \t\t\t}\n>  \t\t\tflags |= OPT_UNSET;\n>  \t\t\tif (!skip_prefix(arg + 3, long_name, &rest)) {\n>  \t\t\t\t/* abbreviated and negated? */\n> -\t\t\t\tif (starts_with(long_name, arg + 3))\n> +\t\t\t\tif (allow_abbrev &&\n> +\t\t\t\t    starts_with(long_name, arg + 3))\n>  \t\t\t\t\tgoto is_abbreviated;\n>  \t\t\t\telse\n>  \t\t\t\t\tcontinue;\n>  \t\t\t}\n>  \t\t}\n>  \t\tif (*rest) {\n>  \t\t\tif (*rest != '=')\n>  \t\t\t\tcontinue;\n>  \t\t\tp->opt = rest + 1;\n>  \t\t}\n>  \t\treturn get_value(p, options, flags ^ opt_flags);\n>  \t}\n>\n>  \tif (disallow_abbreviated_options && (ambiguous_option || abbrev_option))\n>  \t\tdie(\"disallowed abbreviated or ambiguous option '%.*s'\",\n>  \t\t    (int)(arg_end - arg), arg);\n>\n>  \tif (ambiguous_option) {\n>  \t\terror(_(\"ambiguous option: %s \"\n>  \t\t\t\"(could be --%s%s or --%s%s)\"),\n>  \t\t\targ,\n>  \t\t\t(ambiguous_flags & OPT_UNSET) ?  \"no-\" : \"\",\n>  \t\t\tambiguous_option->long_name,\n>  \t\t\t(abbrev_flags & OPT_UNSET) ?  \"no-\" : \"\",\n>  \t\t\tabbrev_option->long_name);\n>  \t\treturn PARSE_OPT_HELP;\n>  \t}\n>  \tif (abbrev_option)\n>  \t\treturn get_value(p, abbrev_option, abbrev_flags);\n>  \treturn PARSE_OPT_UNKNOWN;\n>  }\n> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n> index cb094241ec..1e3b2dbea4 100755\n> --- a/t/t4013-diff-various.sh\n> +++ b/t/t4013-diff-various.sh\n> @@ -663,4 +663,10 @@ test_expect_success 'diff --default-prefix overrides diff.mnemonicprefix' '\n>  \tcheck_prefix actual a/file0 b/file0\n>  '\n>\n> +test_expect_success 'diff --no-renames cannot be abbreviated' '\n> +\ttest_expect_code 129 git diff --no-rename >actual 2>error &&\n> +\ttest_must_be_empty actual &&\n> +\tgrep \"invalid option: --no-rename\" error\n> +'\n> +\n>  test_done\n> --\n> 2.43.0\n"},{"id":"487180","messageId":"fb3c679a-5f00-4934-b028-6b2d081cd5b2@web.de","threadId":"60757","inReplyTo":"xmqqmsszga9m.fsf@gitster.g","subject":"Re: [Bug?] \"git diff --no-rename A B\"","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-01-21T17:56:39Z","receivedAt":"2024-01-21T17:56:57Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 20.01.24 um 18:55 schrieb Junio C Hamano:\n>\n> I agree with you that the structure of that loop and the processing\n> in it does look confusing.\n\nHere's a small simplification.\n\n--- >8 ---\nSubject: [PATCH 2/1] parse-options: simplify positivation handling\n\nWe accept the positive version of options whose long name starts with\n\"no-\" and are defined without the flag PARSE_OPT_NONEG.  E.g. git clone\nhas an explicitly defined --no-checkout option and also implicitly\naccepts --checkout to override it.\n\nparse_long_opt() handles that by restarting the option matching with the\npositive version when it finds that only the current option definition\nstarts with \"no-\", but not the user-supplied argument.  This code is\nlocated almost at the end of the matching logic.\n\nAvoid the need for a restart by moving the code up.  We don't have to\ncheck the positive arg against the negative long_name at all -- the\n\"no-\" prefix of the latter makes a match impossible.  Skip it and toggle\nOPT_UNSET right away to simplify the control flow.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nPatch formatted with --function-context for easier review.\n\n parse-options.c | 13 ++++++-------\n 1 file changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 92e96ca6cd..63a99dea6e 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -353,95 +353,94 @@ static int is_alias(struct parse_opt_ctx_t *ctx,\n static enum parse_opt_result parse_long_opt(\n \tstruct parse_opt_ctx_t *p, const char *arg,\n \tconst struct option *options)\n {\n \tconst char *arg_end = strchrnul(arg, '=');\n \tconst struct option *abbrev_option = NULL, *ambiguous_option = NULL;\n \tenum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG;\n \tint allow_abbrev = !(p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT);\n\n \tfor (; options->type != OPTION_END; options++) {\n \t\tconst char *rest, *long_name = options->long_name;\n \t\tenum opt_parsed flags = OPT_LONG, opt_flags = OPT_LONG;\n\n \t\tif (options->type == OPTION_SUBCOMMAND)\n \t\t\tcontinue;\n \t\tif (!long_name)\n \t\t\tcontinue;\n\n-again:\n+\t\tif (!starts_with(arg, \"no-\") &&\n+\t\t    !(options->flags & PARSE_OPT_NONEG) &&\n+\t\t    skip_prefix(long_name, \"no-\", &long_name))\n+\t\t\topt_flags |= OPT_UNSET;\n+\n \t\tif (!skip_prefix(arg, long_name, &rest))\n \t\t\trest = NULL;\n \t\tif (!rest) {\n \t\t\t/* abbreviated? */\n \t\t\tif (allow_abbrev &&\n \t\t\t    !strncmp(long_name, arg, arg_end - arg)) {\n is_abbreviated:\n \t\t\t\tif (abbrev_option &&\n \t\t\t\t    !is_alias(p, abbrev_option, options)) {\n \t\t\t\t\t/*\n \t\t\t\t\t * If this is abbreviated, it is\n \t\t\t\t\t * ambiguous. So when there is no\n \t\t\t\t\t * exact match later, we need to\n \t\t\t\t\t * error out.\n \t\t\t\t\t */\n \t\t\t\t\tambiguous_option = abbrev_option;\n \t\t\t\t\tambiguous_flags = abbrev_flags;\n \t\t\t\t}\n \t\t\t\tif (!(flags & OPT_UNSET) && *arg_end)\n \t\t\t\t\tp->opt = arg_end + 1;\n \t\t\t\tabbrev_option = options;\n \t\t\t\tabbrev_flags = flags ^ opt_flags;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\t/* negation allowed? */\n \t\t\tif (options->flags & PARSE_OPT_NONEG)\n \t\t\t\tcontinue;\n \t\t\t/* negated and abbreviated very much? */\n \t\t\tif (allow_abbrev && starts_with(\"no-\", arg)) {\n \t\t\t\tflags |= OPT_UNSET;\n \t\t\t\tgoto is_abbreviated;\n \t\t\t}\n \t\t\t/* negated? */\n-\t\t\tif (!starts_with(arg, \"no-\")) {\n-\t\t\t\tif (skip_prefix(long_name, \"no-\", &long_name)) {\n-\t\t\t\t\topt_flags |= OPT_UNSET;\n-\t\t\t\t\tgoto again;\n-\t\t\t\t}\n+\t\t\tif (!starts_with(arg, \"no-\"))\n \t\t\t\tcontinue;\n-\t\t\t}\n \t\t\tflags |= OPT_UNSET;\n \t\t\tif (!skip_prefix(arg + 3, long_name, &rest)) {\n \t\t\t\t/* abbreviated and negated? */\n \t\t\t\tif (allow_abbrev &&\n \t\t\t\t    starts_with(long_name, arg + 3))\n \t\t\t\t\tgoto is_abbreviated;\n \t\t\t\telse\n \t\t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n \t\tif (*rest) {\n \t\t\tif (*rest != '=')\n \t\t\t\tcontinue;\n \t\t\tp->opt = rest + 1;\n \t\t}\n \t\treturn get_value(p, options, flags ^ opt_flags);\n \t}\n\n \tif (disallow_abbreviated_options && (ambiguous_option || abbrev_option))\n \t\tdie(\"disallowed abbreviated or ambiguous option '%.*s'\",\n \t\t    (int)(arg_end - arg), arg);\n\n \tif (ambiguous_option) {\n \t\terror(_(\"ambiguous option: %s \"\n \t\t\t\"(could be --%s%s or --%s%s)\"),\n \t\t\targ,\n \t\t\t(ambiguous_flags & OPT_UNSET) ?  \"no-\" : \"\",\n \t\t\tambiguous_option->long_name,\n \t\t\t(abbrev_flags & OPT_UNSET) ?  \"no-\" : \"\",\n \t\t\tabbrev_option->long_name);\n \t\treturn PARSE_OPT_HELP;\n \t}\n \tif (abbrev_option)\n \t\treturn get_value(p, abbrev_option, abbrev_flags);\n \treturn PARSE_OPT_UNKNOWN;\n }\n--\n2.43.0\n"},{"id":"487233","messageId":"20240122230051.GB814674@coredump.intra.peff.net","threadId":"60757","inReplyTo":"579fd5bc-3bfd-427f-b22d-dab5e7e56eb2@web.de","subject":"Re: [Bug?] \"git diff --no-rename A B\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-22T23:00:51Z","receivedAt":"2024-01-22T23:00:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 20, 2024 at 03:39:38PM +0100, René Scharfe wrote:\n\n> > The issue is that we have \"--rename-empty\", which of course also\n> > provides \"--no-rename-empty\". And parse-options is happy to let you\n> > abbreviate names as long as they are unambiguous. But \"--no-rename\" _is_\n> > ambiguous with \"--no-renames\". Why don't we catch it?\n> \n> Because diff_opt_parse() passes PARSE_OPT_KEEP_UNKNOWN_OPT, which makes\n> parse_long_opt() skip abbreviation detection.  Which it does since\n> baa4adc66a (parse-options: disable option abbreviation with\n> PARSE_OPT_KEEP_UNKNOWN, 2019-01-27).\n\nOK, it makes sense to me that we should avoid abbreviation entirely with\nKEEP_UNKNOWN_OPT, for the reasons given in that commit. But if adding\n--rename fixed it, is there another bug lurking? That is, would we do\nthe wrong thing on a case without KEEP_UNKNOWN_OPT but which had\n\"--renames\" and \"--no-rename\" defined? Or was it simply the\ninconsistency in how KEEP_UNKNOWN_OPT is being applied?\n\nI think it might just be the latter. If I do this:\n\ndiff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\nindex ded8116cc5..e908c7386d 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -124,6 +124,7 @@ int cmd__parse_options(int argc, const char **argv)\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"yes\", &boolean, \"get a boolean\"),\n \t\tOPT_BOOL('D', \"no-doubt\", &boolean, \"begins with 'no-'\"),\n+\t\tOPT_BOOL(0, \"do-it\", &boolean, \"'do' ambiguous with 'doubt'\"),\n \t\t{ OPTION_SET_INT, 'B', \"no-fear\", &boolean, NULL,\n \t\t  \"be brave\", PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },\n \t\tOPT_COUNTUP('b', \"boolean\", &boolean, \"increment by one\"),\n\nthen running:\n\n  t/helper/test-tool parse-options --do\n\ncorrectly complains about the ambiguity (though amusingly it mentions\n\"--no-no-doubt\" in the error message). And if I add KEEP_UNKNOWN_OPT,\nthen it gives the wrong behavior. But curiously it does so even with\nyour patch applied. So I think there may be further fixes needed.\n\n-Peff\n"},{"id":"487239","messageId":"20240123002824.GA827890@coredump.intra.peff.net","threadId":"60757","inReplyTo":"20240122230051.GB814674@coredump.intra.peff.net","subject":"Re: [Bug?] \"git diff --no-rename A B\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-23T00:28:24Z","receivedAt":"2024-01-23T00:28:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 22, 2024 at 06:00:51PM -0500, Jeff King wrote:\n\n> then running:\n> \n>   t/helper/test-tool parse-options --do\n> \n> correctly complains about the ambiguity (though amusingly it mentions\n> \"--no-no-doubt\" in the error message). And if I add KEEP_UNKNOWN_OPT,\n> then it gives the wrong behavior. But curiously it does so even with\n> your patch applied. So I think there may be further fixes needed.\n\nOh sorry, I'm stupid. Of course it does not complain with\nKEEP_UNKNOWN_OPT. It treats it as unknown and keeps it!\n\nSo with the patch I showed (and adding in KEEP_UNKNOWN_OPT), even\nwithout your patch \"--do\" is handled correctly (it is unknown and left\nin argv[0]). It is only \"--no-do\" which is broken, and your patch fixes\nthat.\n\nSorry for the noise. :)\n\n-Peff\n"}]}