{"thread":{"id":"60987","subject":"[PATCH 2/6] parse-options: set arg of abbreviated option lazily","startedAt":"2024-02-24T21:31:24Z","lastAt":"2024-03-13T18:47:57Z","messageCount":23,"participants":["René Scharfe","Junio C Hamano","Josh Steadmon"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"489284","messageId":"20240224212953.44026-3-l.s.r@web.de","threadId":"60987","inReplyTo":"20240224212953.44026-1-l.s.r@web.de","subject":"[PATCH 2/6] parse-options: set arg of abbreviated option lazily","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-24T21:29:49Z","receivedAt":"2024-02-24T21:31:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Postpone setting the opt pointer until we're about to call get_value(),\nwhich uses it.  There's no point in setting it eagerly for every\nabbreviated candidate option, which may turn out to be ambiguous.\nRemoving this assignment from the loop doesn't noticeably improve the\nperformance, but allows further simplification.\n---\n parse-options.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex e4ce33ea48..056c6b30e9 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -391,8 +391,6 @@ static enum parse_opt_result parse_long_opt(\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 (*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@@ -441,8 +439,11 @@ static enum parse_opt_result parse_long_opt(\n \t\t\tabbrev_option->long_name);\n \t\treturn PARSE_OPT_HELP;\n \t}\n-\tif (abbrev_option)\n+\tif (abbrev_option) {\n+\t\tif (*arg_end)\n+\t\t\tp->opt = arg_end + 1;\n \t\treturn get_value(p, abbrev_option, abbrev_flags);\n+\t}\n \treturn PARSE_OPT_UNKNOWN;\n }\n\n--\n2.44.0\n\n"},{"id":"489285","messageId":"20240224212953.44026-4-l.s.r@web.de","threadId":"60987","inReplyTo":"20240224212953.44026-1-l.s.r@web.de","subject":"[PATCH 3/6] parse-options: factor out register_abbrev() and struct parsed_option","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-24T21:29:50Z","receivedAt":"2024-02-24T21:31:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a function, register_abbrev(), for storing the necessary details for\nremembering an abbreviated and thus potentially ambiguous option.  Call\nit instead of sharing the code using goto, to make the control flow more\nexplicit.\n\nConveniently collect these details in the new struct parsed_option to\nreduce the number of necessary function arguments.\n---\n parse-options.c | 83 +++++++++++++++++++++++++++++--------------------\n 1 file changed, 49 insertions(+), 34 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 056c6b30e9..398ebaef14 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -350,14 +350,40 @@ static int is_alias(struct parse_opt_ctx_t *ctx,\n \treturn 0;\n }\n\n+struct parsed_option {\n+\tconst struct option *option;\n+\tenum opt_parsed flags;\n+};\n+\n+static void register_abbrev(struct parse_opt_ctx_t *p,\n+\t\t\t    const struct option *option, enum opt_parsed flags,\n+\t\t\t    struct parsed_option *abbrev,\n+\t\t\t    struct parsed_option *ambiguous)\n+{\n+\tif (p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT)\n+\t\treturn;\n+\tif (abbrev->option &&\n+\t    !is_alias(p, abbrev->option, option)) {\n+\t\t/*\n+\t\t * If this is abbreviated, it is\n+\t\t * ambiguous. So when there is no\n+\t\t * exact match later, we need to\n+\t\t * error out.\n+\t\t */\n+\t\tambiguous->option = abbrev->option;\n+\t\tambiguous->flags = abbrev->flags;\n+\t}\n+\tabbrev->option = option;\n+\tabbrev->flags = flags;\n+}\n+\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+\tstruct parsed_option abbrev = { .option = NULL, .flags = OPT_LONG };\n+\tstruct parsed_option ambiguous = { .option = NULL, .flags = OPT_LONG };\n\n \tfor (; options->type != OPTION_END; options++) {\n \t\tconst char *rest, *long_name = options->long_name;\n@@ -377,31 +403,20 @@ static enum parse_opt_result parse_long_opt(\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\tabbrev_option = options;\n-\t\t\t\tabbrev_flags = flags ^ opt_flags;\n+\t\t\tif (!strncmp(long_name, arg, arg_end - arg)) {\n+\t\t\t\tregister_abbrev(p, options, flags ^ opt_flags,\n+\t\t\t\t\t\t&abbrev, &ambiguous);\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\tif (starts_with(\"no-\", arg)) {\n \t\t\t\tflags |= OPT_UNSET;\n-\t\t\t\tgoto is_abbreviated;\n+\t\t\t\tregister_abbrev(p, options, flags ^ opt_flags,\n+\t\t\t\t\t\t&abbrev, &ambiguous);\n+\t\t\t\tcontinue;\n \t\t\t}\n \t\t\t/* negated? */\n \t\t\tif (!starts_with(arg, \"no-\"))\n@@ -409,12 +424,12 @@ static enum parse_opt_result parse_long_opt(\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    !strncmp(long_name, arg + 3,\n+\t\t\t\tif (!strncmp(long_name, arg + 3,\n \t\t\t\t\t     arg_end - 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\t\tregister_abbrev(p, options,\n+\t\t\t\t\t\t\tflags ^ opt_flags,\n+\t\t\t\t\t\t\t&abbrev, &ambiguous);\n+\t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n \t\tif (*rest) {\n@@ -425,24 +440,24 @@ static enum parse_opt_result parse_long_opt(\n \t\treturn get_value(p, options, flags ^ opt_flags);\n \t}\n\n-\tif (disallow_abbreviated_options && (ambiguous_option || abbrev_option))\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+\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\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+\tif (abbrev.option) {\n \t\tif (*arg_end)\n \t\t\tp->opt = arg_end + 1;\n-\t\treturn get_value(p, abbrev_option, abbrev_flags);\n+\t\treturn get_value(p, abbrev.option, abbrev.flags);\n \t}\n \treturn PARSE_OPT_UNKNOWN;\n }\n--\n2.44.0\n\n"},{"id":"489286","messageId":"20240224212953.44026-1-l.s.r@web.de","threadId":"60987","inReplyTo":null,"subject":"[PATCH 0/6] parse-options: long option parsing fixes and cleanup","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-24T21:29:47Z","receivedAt":"2024-02-24T21:31:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Fix two corner cases in patches 1 and 4, refactor the code in patches 2\nand 3 to prepare the second fix, bonus patches 5 and 6 simplify the code\nand improve readability.\n\n  parse-options: recognize abbreviated negated option with arg\n  parse-options: set arg of abbreviated option lazily\n  parse-options: factor out register_abbrev() and struct parsed_option\n  parse-options: detect ambiguous self-negation\n  parse-options: normalize arg and long_name before comparison\n  parse-options: rearrange long_name matching code\n\n parse-options.c               | 137 ++++++++++++++++++----------------\n t/t0040-parse-options.sh      |  16 ++++\n t/t1502-rev-parse-parseopt.sh |  11 +++\n 3 files changed, 100 insertions(+), 64 deletions(-)\n\n--\n2.44.0\n\n"},{"id":"489288","messageId":"20240224212953.44026-2-l.s.r@web.de","threadId":"60987","inReplyTo":"20240224212953.44026-1-l.s.r@web.de","subject":"[PATCH 1/6] parse-options: recognize abbreviated negated option with arg","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-24T21:29:48Z","receivedAt":"2024-02-24T21:31:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Giving an argument to an option that doesn't take one causes Git to\nreport that error specifically:\n\n   $ git rm --dry-run=bogus\n   error: option `dry-run' takes no value\n\nThe same is true when the option is negated or abbreviated:\n\n   $ git rm --no-dry-run=bogus\n   error: option `no-dry-run' takes no value\n\n   $ git rm --dry=bogus\n   error: option `dry-run' takes no value\n\nNot so when doing both, though:\n\n   $ git rm --no-dry=bogus\n   error: unknown option `no-dry=bogus'\n   usage: git rm [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch]\n\n(Rest of the usage message omitted.)\n\nImprove consistency and usefulness of the error message by recognizing\nabbreviated negated options even if they have a (most likely bogus)\nargument.  With this patch we get:\n\n   $ git rm --no-dry=bogus\n   error: option `no-dry-run' takes no value\n---\n parse-options.c          |  5 +++--\n t/t0040-parse-options.sh | 16 ++++++++++++++++\n 2 files changed, 19 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 63a99dea6e..e4ce33ea48 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -391,7 +391,7 @@ static enum parse_opt_result parse_long_opt(\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\tif (*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@@ -412,7 +412,8 @@ static enum parse_opt_result parse_long_opt(\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    !strncmp(long_name, arg + 3,\n+\t\t\t\t\t     arg_end - arg - 3))\n \t\t\t\t\tgoto is_abbreviated;\n \t\t\t\telse\n \t\t\t\t\tcontinue;\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex ec974867e4..8bb2a8b453 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -210,6 +210,22 @@ test_expect_success 'superfluous value provided: boolean' '\n \ttest_cmp expect actual\n '\n\n+test_expect_success 'superfluous value provided: boolean, abbreviated' '\n+\tcat >expect <<-\\EOF &&\n+\terror: option `yes'\\'' takes no value\n+\tEOF\n+\ttest_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n+\ttest-tool parse-options --ye=hi 2>actual &&\n+\ttest_cmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\terror: option `no-yes'\\'' takes no value\n+\tEOF\n+\ttest_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n+\ttest-tool parse-options --no-ye=hi 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'superfluous value provided: cmdmode' '\n \tcat >expect <<-\\EOF &&\n \terror: option `mode1'\\'' takes no value\n--\n2.44.0\n\n"},{"id":"489287","messageId":"20240224212953.44026-7-l.s.r@web.de","threadId":"60987","inReplyTo":"20240224212953.44026-1-l.s.r@web.de","subject":"[PATCH 6/6] parse-options: rearrange long_name matching code","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-24T21:29:53Z","receivedAt":"2024-02-24T21:31:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Move the code for handling a full match of long_name first and get rid\nof negations.  Reduce the indent of the code for matching abbreviations\nand remove unnecessary curly braces.  Combine the checks for whether\nnegation is allowed and whether arg is \"n\", \"no\" or \"no-\" because they\nbelong together and avoid a continue statement.  The result is shorter,\nmore readable code.\n---\n parse-options.c | 37 +++++++++++++++----------------------\n 1 file changed, 15 insertions(+), 22 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex d45efa4e5c..30b9e68f8a 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -413,30 +413,23 @@ static enum parse_opt_result parse_long_opt(\n \t\tif (((flags ^ opt_flags) & OPT_UNSET) && !allow_unset)\n \t\t\tcontinue;\n\n-\t\tif (!skip_prefix(arg_start, long_name, &rest))\n-\t\t\trest = NULL;\n-\t\tif (!rest) {\n-\t\t\t/* abbreviated? */\n-\t\t\tif (!strncmp(long_name, arg_start, arg_end - arg_start)) {\n-\t\t\t\tregister_abbrev(p, options, flags ^ opt_flags,\n-\t\t\t\t\t\t&abbrev, &ambiguous);\n-\t\t\t}\n-\t\t\t/* negation allowed? */\n-\t\t\tif (options->flags & PARSE_OPT_NONEG)\n+\t\tif (skip_prefix(arg_start, long_name, &rest)) {\n+\t\t\tif (*rest == '=')\n+\t\t\t\tp->opt = rest + 1;\n+\t\t\telse if (*rest)\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\t\tregister_abbrev(p, options, OPT_UNSET ^ opt_flags,\n-\t\t\t\t\t\t&abbrev, &ambiguous);\n-\t\t\t}\n-\t\t\tcontinue;\n+\t\t\treturn get_value(p, options, flags ^ opt_flags);\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+\n+\t\t/* abbreviated? */\n+\t\tif (!strncmp(long_name, arg_start, arg_end - arg_start))\n+\t\t\tregister_abbrev(p, options, flags ^ opt_flags,\n+\t\t\t\t\t&abbrev, &ambiguous);\n+\n+\t\t/* negated and abbreviated very much? */\n+\t\tif (allow_unset && starts_with(\"no-\", arg))\n+\t\t\tregister_abbrev(p, options, OPT_UNSET ^ opt_flags,\n+\t\t\t\t\t&abbrev, &ambiguous);\n \t}\n\n \tif (disallow_abbreviated_options && (ambiguous.option || abbrev.option))\n--\n2.44.0\n\n"},{"id":"489289","messageId":"20240224212953.44026-6-l.s.r@web.de","threadId":"60987","inReplyTo":"20240224212953.44026-1-l.s.r@web.de","subject":"[PATCH 5/6] parse-options: normalize arg and long_name before comparison","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-24T21:29:52Z","receivedAt":"2024-02-24T21:31:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Strip \"no-\" from arg and long_name before comparing them.  This way we\nno longer have to repeat the comparison with an offset of 3 for negated\narguments.\n\nNote that we must not modify the \"flags\" value, which tracks whether arg\nis negated, inside the loop.  When registering \"--n\", \"--no\" or \"--no-\"\nas abbreviation for any negative option, we used to OR it with OPT_UNSET\nand end the loop.  We can simply hard-code OPT_UNSET and leave flags\nunchanged instead.\n---\n parse-options.c | 44 ++++++++++++++++++++++----------------------\n 1 file changed, 22 insertions(+), 22 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 008c0f32cf..d45efa4e5c 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -382,28 +382,42 @@ static enum parse_opt_result parse_long_opt(\n \tconst struct option *options)\n {\n \tconst char *arg_end = strchrnul(arg, '=');\n+\tconst char *arg_start = arg;\n+\tenum opt_parsed flags = OPT_LONG;\n+\tint arg_starts_with_no_no = 0;\n \tstruct parsed_option abbrev = { .option = NULL, .flags = OPT_LONG };\n \tstruct parsed_option ambiguous = { .option = NULL, .flags = OPT_LONG };\n\n+\tif (skip_prefix(arg_start, \"no-\", &arg_start)) {\n+\t\tif (skip_prefix(arg_start, \"no-\", &arg_start))\n+\t\t\targ_starts_with_no_no = 1;\n+\t\telse\n+\t\t\tflags |= OPT_UNSET;\n+\t}\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+\t\tenum opt_parsed opt_flags = OPT_LONG;\n+\t\tint allow_unset = !(options->flags & PARSE_OPT_NONEG);\n\n \t\tif (options->type == OPTION_SUBCOMMAND)\n \t\t\tcontinue;\n \t\tif (!long_name)\n \t\t\tcontinue;\n\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\tif (skip_prefix(long_name, \"no-\", &long_name))\n \t\t\topt_flags |= OPT_UNSET;\n+\t\telse if (arg_starts_with_no_no)\n+\t\t\tcontinue;\n+\n+\t\tif (((flags ^ opt_flags) & OPT_UNSET) && !allow_unset)\n+\t\t\tcontinue;\n\n-\t\tif (!skip_prefix(arg, long_name, &rest))\n+\t\tif (!skip_prefix(arg_start, long_name, &rest))\n \t\t\trest = NULL;\n \t\tif (!rest) {\n \t\t\t/* abbreviated? */\n-\t\t\tif (!strncmp(long_name, arg, arg_end - arg)) {\n+\t\t\tif (!strncmp(long_name, arg_start, arg_end - arg_start)) {\n \t\t\t\tregister_abbrev(p, options, flags ^ opt_flags,\n \t\t\t\t\t\t&abbrev, &ambiguous);\n \t\t\t}\n@@ -412,24 +426,10 @@ static enum parse_opt_result parse_long_opt(\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\t\tflags |= OPT_UNSET;\n-\t\t\t\tregister_abbrev(p, options, flags ^ opt_flags,\n+\t\t\t\tregister_abbrev(p, options, OPT_UNSET ^ opt_flags,\n \t\t\t\t\t\t&abbrev, &ambiguous);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\t/* negated? */\n-\t\t\tif (!starts_with(arg, \"no-\"))\n-\t\t\t\tcontinue;\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 (!strncmp(long_name, arg + 3,\n-\t\t\t\t\t     arg_end - arg - 3))\n-\t\t\t\t\tregister_abbrev(p, options,\n-\t\t\t\t\t\t\tflags ^ opt_flags,\n-\t\t\t\t\t\t\t&abbrev, &ambiguous);\n-\t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tcontinue;\n \t\t}\n \t\tif (*rest) {\n \t\t\tif (*rest != '=')\n--\n2.44.0\n\n"},{"id":"489290","messageId":"20240224212953.44026-5-l.s.r@web.de","threadId":"60987","inReplyTo":"20240224212953.44026-1-l.s.r@web.de","subject":"[PATCH 4/6] parse-options: detect ambiguous self-negation","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-24T21:29:51Z","receivedAt":"2024-02-24T21:31:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Git currently does not detect the ambiguity of an option that starts\nwith \"no\" like --notes and its negated form if given just --n or --no.\nAll Git commands with such options have other negatable options, and\nwe detect the ambiguity with them, so that's currently only a potential\nproblem for scripts that use git rev-parse --parseopt.\n\nLet's fix it nevertheless, as there's no need for that confusion.  To\ndetect the ambiguity we have to loosen the check in register_abbrev(),\nas an option is considered an alias of itself.  Add non-matching\nnegation flags as a criterion to recognize an option being ambiguous\nwith its negated form.\n\nAnd we need to keep going after finding a non-negated option as an\nabbreviated candidate and perform the negation checks in the same\nloop.\n---\n parse-options.c               |  3 +--\n t/t1502-rev-parse-parseopt.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 398ebaef14..008c0f32cf 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -363,7 +363,7 @@ static void register_abbrev(struct parse_opt_ctx_t *p,\n \tif (p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT)\n \t\treturn;\n \tif (abbrev->option &&\n-\t    !is_alias(p, abbrev->option, option)) {\n+\t    !(abbrev->flags == flags && is_alias(p, abbrev->option, option))) {\n \t\t/*\n \t\t * If this is abbreviated, it is\n \t\t * ambiguous. So when there is no\n@@ -406,7 +406,6 @@ static enum parse_opt_result parse_long_opt(\n \t\t\tif (!strncmp(long_name, arg, arg_end - arg)) {\n \t\t\t\tregister_abbrev(p, options, flags ^ opt_flags,\n \t\t\t\t\t\t&abbrev, &ambiguous);\n-\t\t\t\tcontinue;\n \t\t\t}\n \t\t\t/* negation allowed? */\n \t\t\tif (options->flags & PARSE_OPT_NONEG)\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex f0737593c3..b754b9fd74 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -322,4 +322,15 @@ check_invalid_long_option optionspec-neg --no-positive-only\n check_invalid_long_option optionspec-neg --negative\n check_invalid_long_option optionspec-neg --no-no-negative\n\n+test_expect_success 'ambiguous: --no matches both --noble and --no-noble' '\n+\tcat >spec <<-\\EOF &&\n+\tsome-command [options]\n+\t--\n+\tnoble The feudal switch.\n+\tEOF\n+\ttest_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n+\tgit rev-parse --parseopt -- <spec 2>err --no &&\n+\tgrep \"error: ambiguous option: no (could be --noble or --no-noble)\" err\n+'\n+\n test_done\n--\n2.44.0\n\n"},{"id":"489291","messageId":"c3982abd-58ca-42e9-b34e-48b03b8f8caa@web.de","threadId":"60987","inReplyTo":"20240224212953.44026-1-l.s.r@web.de","subject":"Re: [PATCH 0/6] parse-options: long option parsing fixes and cleanup","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-24T21:42:27Z","receivedAt":"2024-02-24T21:42:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 24.02.24 um 22:29 schrieb René Scharfe:\n> Fix two corner cases in patches 1 and 4, refactor the code in patches 2\n> and 3 to prepare the second fix, bonus patches 5 and 6 simplify the code\n> and improve readability.\n\nForgot to sign-off, d'oh!  Will wait for feedback and sign-off in v2..\n\n>\n>   parse-options: recognize abbreviated negated option with arg\n>   parse-options: set arg of abbreviated option lazily\n>   parse-options: factor out register_abbrev() and struct parsed_option\n>   parse-options: detect ambiguous self-negation\n>   parse-options: normalize arg and long_name before comparison\n>   parse-options: rearrange long_name matching code\n>\n>  parse-options.c               | 137 ++++++++++++++++++----------------\n>  t/t0040-parse-options.sh      |  16 ++++\n>  t/t1502-rev-parse-parseopt.sh |  11 +++\n>  3 files changed, 100 insertions(+), 64 deletions(-)\n>\n> --\n> 2.44.0\n>\n"},{"id":"489297","messageId":"xmqqttlxjw1u.fsf@gitster.g","threadId":"60987","inReplyTo":"20240224212953.44026-4-l.s.r@web.de","subject":"Re: [PATCH 3/6] parse-options: factor out register_abbrev() and struct parsed_option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-24T23:13:01Z","receivedAt":"2024-02-24T23:13:13Z","isPatch":true,"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>  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> +\tstruct parsed_option abbrev = { .option = NULL, .flags = OPT_LONG };\n> +\tstruct parsed_option ambiguous = { .option = NULL, .flags = OPT_LONG };\n\nThere is this \"allow_abbrev\" thing ...\n\n> ...\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    !strncmp(long_name, arg + 3,\n> +\t\t\t\tif (!strncmp(long_name, arg + 3,\n>  \t\t\t\t\t     arg_end - 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\t\tregister_abbrev(p, options,\n> +\t\t\t\t\t\t\tflags ^ opt_flags,\n> +\t\t\t\t\t\t\t&abbrev, &ambiguous);\n> +\t\t\t\tcontinue;\n>  \t\t\t}\n>  \t\t}\n\n... whose use goes away completely from the loop.  We still call\nregister_abbrev(), but the helper function becomes no-op when\np->flags had KEEP_UNKNOWN_OPT bit set, so everything cancels out.\n\nOK.\n"},{"id":"489812","messageId":"20240303121944.20627-7-l.s.r@web.de","threadId":"60987","inReplyTo":"20240303121944.20627-1-l.s.r@web.de","subject":"[PATCH v2 6/6] parse-options: rearrange long_name matching code","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-03-03T12:19:43Z","receivedAt":"2024-03-03T12:19:56Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Move the code for handling a full match of long_name first and get rid\nof negations.  Reduce the indent of the code for matching abbreviations\nand remove unnecessary curly braces.  Combine the checks for whether\nnegation is allowed and whether arg is \"n\", \"no\" or \"no-\" because they\nbelong together and avoid a continue statement.  The result is shorter,\nmore readable code.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n parse-options.c | 37 +++++++++++++++----------------------\n 1 file changed, 15 insertions(+), 22 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex d45efa4e5c..30b9e68f8a 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -413,30 +413,23 @@ static enum parse_opt_result parse_long_opt(\n \t\tif (((flags ^ opt_flags) & OPT_UNSET) && !allow_unset)\n \t\t\tcontinue;\n\n-\t\tif (!skip_prefix(arg_start, long_name, &rest))\n-\t\t\trest = NULL;\n-\t\tif (!rest) {\n-\t\t\t/* abbreviated? */\n-\t\t\tif (!strncmp(long_name, arg_start, arg_end - arg_start)) {\n-\t\t\t\tregister_abbrev(p, options, flags ^ opt_flags,\n-\t\t\t\t\t\t&abbrev, &ambiguous);\n-\t\t\t}\n-\t\t\t/* negation allowed? */\n-\t\t\tif (options->flags & PARSE_OPT_NONEG)\n+\t\tif (skip_prefix(arg_start, long_name, &rest)) {\n+\t\t\tif (*rest == '=')\n+\t\t\t\tp->opt = rest + 1;\n+\t\t\telse if (*rest)\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\t\tregister_abbrev(p, options, OPT_UNSET ^ opt_flags,\n-\t\t\t\t\t\t&abbrev, &ambiguous);\n-\t\t\t}\n-\t\t\tcontinue;\n+\t\t\treturn get_value(p, options, flags ^ opt_flags);\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+\n+\t\t/* abbreviated? */\n+\t\tif (!strncmp(long_name, arg_start, arg_end - arg_start))\n+\t\t\tregister_abbrev(p, options, flags ^ opt_flags,\n+\t\t\t\t\t&abbrev, &ambiguous);\n+\n+\t\t/* negated and abbreviated very much? */\n+\t\tif (allow_unset && starts_with(\"no-\", arg))\n+\t\t\tregister_abbrev(p, options, OPT_UNSET ^ opt_flags,\n+\t\t\t\t\t&abbrev, &ambiguous);\n \t}\n\n \tif (disallow_abbreviated_options && (ambiguous.option || abbrev.option))\n--\n2.44.0\n\n"},{"id":"489810","messageId":"20240303121944.20627-6-l.s.r@web.de","threadId":"60987","inReplyTo":"20240303121944.20627-1-l.s.r@web.de","subject":"[PATCH v2 5/6] parse-options: normalize arg and long_name before comparison","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-03-03T12:19:42Z","receivedAt":"2024-03-03T12:19:57Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Strip \"no-\" from arg and long_name before comparing them.  This way we\nno longer have to repeat the comparison with an offset of 3 for negated\narguments.\n\nNote that we must not modify the \"flags\" value, which tracks whether arg\nis negated, inside the loop.  When registering \"--n\", \"--no\" or \"--no-\"\nas abbreviation for any negative option, we used to OR it with OPT_UNSET\nand end the loop.  We can simply hard-code OPT_UNSET and leave flags\nunchanged instead.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n parse-options.c | 44 ++++++++++++++++++++++----------------------\n 1 file changed, 22 insertions(+), 22 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 008c0f32cf..d45efa4e5c 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -382,28 +382,42 @@ static enum parse_opt_result parse_long_opt(\n \tconst struct option *options)\n {\n \tconst char *arg_end = strchrnul(arg, '=');\n+\tconst char *arg_start = arg;\n+\tenum opt_parsed flags = OPT_LONG;\n+\tint arg_starts_with_no_no = 0;\n \tstruct parsed_option abbrev = { .option = NULL, .flags = OPT_LONG };\n \tstruct parsed_option ambiguous = { .option = NULL, .flags = OPT_LONG };\n\n+\tif (skip_prefix(arg_start, \"no-\", &arg_start)) {\n+\t\tif (skip_prefix(arg_start, \"no-\", &arg_start))\n+\t\t\targ_starts_with_no_no = 1;\n+\t\telse\n+\t\t\tflags |= OPT_UNSET;\n+\t}\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+\t\tenum opt_parsed opt_flags = OPT_LONG;\n+\t\tint allow_unset = !(options->flags & PARSE_OPT_NONEG);\n\n \t\tif (options->type == OPTION_SUBCOMMAND)\n \t\t\tcontinue;\n \t\tif (!long_name)\n \t\t\tcontinue;\n\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\tif (skip_prefix(long_name, \"no-\", &long_name))\n \t\t\topt_flags |= OPT_UNSET;\n+\t\telse if (arg_starts_with_no_no)\n+\t\t\tcontinue;\n+\n+\t\tif (((flags ^ opt_flags) & OPT_UNSET) && !allow_unset)\n+\t\t\tcontinue;\n\n-\t\tif (!skip_prefix(arg, long_name, &rest))\n+\t\tif (!skip_prefix(arg_start, long_name, &rest))\n \t\t\trest = NULL;\n \t\tif (!rest) {\n \t\t\t/* abbreviated? */\n-\t\t\tif (!strncmp(long_name, arg, arg_end - arg)) {\n+\t\t\tif (!strncmp(long_name, arg_start, arg_end - arg_start)) {\n \t\t\t\tregister_abbrev(p, options, flags ^ opt_flags,\n \t\t\t\t\t\t&abbrev, &ambiguous);\n \t\t\t}\n@@ -412,24 +426,10 @@ static enum parse_opt_result parse_long_opt(\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\t\tflags |= OPT_UNSET;\n-\t\t\t\tregister_abbrev(p, options, flags ^ opt_flags,\n+\t\t\t\tregister_abbrev(p, options, OPT_UNSET ^ opt_flags,\n \t\t\t\t\t\t&abbrev, &ambiguous);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\t/* negated? */\n-\t\t\tif (!starts_with(arg, \"no-\"))\n-\t\t\t\tcontinue;\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 (!strncmp(long_name, arg + 3,\n-\t\t\t\t\t     arg_end - arg - 3))\n-\t\t\t\t\tregister_abbrev(p, options,\n-\t\t\t\t\t\t\tflags ^ opt_flags,\n-\t\t\t\t\t\t\t&abbrev, &ambiguous);\n-\t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tcontinue;\n \t\t}\n \t\tif (*rest) {\n \t\t\tif (*rest != '=')\n--\n2.44.0\n\n"},{"id":"489811","messageId":"20240303121944.20627-3-l.s.r@web.de","threadId":"60987","inReplyTo":"20240303121944.20627-1-l.s.r@web.de","subject":"[PATCH v2 2/6] parse-options: set arg of abbreviated option lazily","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-03-03T12:19:39Z","receivedAt":"2024-03-03T12:19:57Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Postpone setting the opt pointer until we're about to call get_value(),\nwhich uses it.  There's no point in setting it eagerly for every\nabbreviated candidate option, which may turn out to be ambiguous.\nRemoving this assignment from the loop doesn't noticeably improve the\nperformance, but allows further simplification.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n parse-options.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex e4ce33ea48..056c6b30e9 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -391,8 +391,6 @@ static enum parse_opt_result parse_long_opt(\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 (*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@@ -441,8 +439,11 @@ static enum parse_opt_result parse_long_opt(\n \t\t\tabbrev_option->long_name);\n \t\treturn PARSE_OPT_HELP;\n \t}\n-\tif (abbrev_option)\n+\tif (abbrev_option) {\n+\t\tif (*arg_end)\n+\t\t\tp->opt = arg_end + 1;\n \t\treturn get_value(p, abbrev_option, abbrev_flags);\n+\t}\n \treturn PARSE_OPT_UNKNOWN;\n }\n\n--\n2.44.0\n\n"},{"id":"489813","messageId":"20240303121944.20627-2-l.s.r@web.de","threadId":"60987","inReplyTo":"20240303121944.20627-1-l.s.r@web.de","subject":"[PATCH v2 1/6] parse-options: recognize abbreviated negated option with arg","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-03-03T12:19:38Z","receivedAt":"2024-03-03T12:20:00Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Giving an argument to an option that doesn't take one causes Git to\nreport that error specifically:\n\n   $ git rm --dry-run=bogus\n   error: option `dry-run' takes no value\n\nThe same is true when the option is negated or abbreviated:\n\n   $ git rm --no-dry-run=bogus\n   error: option `no-dry-run' takes no value\n\n   $ git rm --dry=bogus\n   error: option `dry-run' takes no value\n\nNot so when doing both, though:\n\n   $ git rm --no-dry=bogus\n   error: unknown option `no-dry=bogus'\n   usage: git rm [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch]\n\n(Rest of the usage message omitted.)\n\nImprove consistency and usefulness of the error message by recognizing\nabbreviated negated options even if they have a (most likely bogus)\nargument.  With this patch we get:\n\n   $ git rm --no-dry=bogus\n   error: option `no-dry-run' takes no value\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n parse-options.c          |  5 +++--\n t/t0040-parse-options.sh | 16 ++++++++++++++++\n 2 files changed, 19 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 63a99dea6e..e4ce33ea48 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -391,7 +391,7 @@ static enum parse_opt_result parse_long_opt(\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\tif (*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@@ -412,7 +412,8 @@ static enum parse_opt_result parse_long_opt(\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    !strncmp(long_name, arg + 3,\n+\t\t\t\t\t     arg_end - arg - 3))\n \t\t\t\t\tgoto is_abbreviated;\n \t\t\t\telse\n \t\t\t\t\tcontinue;\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex ec974867e4..8bb2a8b453 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -210,6 +210,22 @@ test_expect_success 'superfluous value provided: boolean' '\n \ttest_cmp expect actual\n '\n\n+test_expect_success 'superfluous value provided: boolean, abbreviated' '\n+\tcat >expect <<-\\EOF &&\n+\terror: option `yes'\\'' takes no value\n+\tEOF\n+\ttest_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n+\ttest-tool parse-options --ye=hi 2>actual &&\n+\ttest_cmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\terror: option `no-yes'\\'' takes no value\n+\tEOF\n+\ttest_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n+\ttest-tool parse-options --no-ye=hi 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'superfluous value provided: cmdmode' '\n \tcat >expect <<-\\EOF &&\n \terror: option `mode1'\\'' takes no value\n--\n2.44.0\n\n"},{"id":"489814","messageId":"20240303121944.20627-1-l.s.r@web.de","threadId":"60987","inReplyTo":"20240224212953.44026-1-l.s.r@web.de","subject":"[PATCH v2 0/6] parse-options: long option parsing fixes and cleanup","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-03-03T12:19:37Z","receivedAt":"2024-03-03T12:20:00Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Fix two corner cases in patches 1 and 4, refactor the code in patches 2\nand 3 to prepare the second fix, bonus patches 5 and 6 simplify the code\nand improve readability.\n\nChanges since v1:\n* Added sign-off.\n\n  parse-options: recognize abbreviated negated option with arg\n  parse-options: set arg of abbreviated option lazily\n  parse-options: factor out register_abbrev() and struct parsed_option\n  parse-options: detect ambiguous self-negation\n  parse-options: normalize arg and long_name before comparison\n  parse-options: rearrange long_name matching code\n\n parse-options.c               | 137 ++++++++++++++++++----------------\n t/t0040-parse-options.sh      |  16 ++++\n t/t1502-rev-parse-parseopt.sh |  11 +++\n 3 files changed, 100 insertions(+), 64 deletions(-)\n\n--\n2.44.0\n\n"},{"id":"489815","messageId":"20240303121944.20627-5-l.s.r@web.de","threadId":"60987","inReplyTo":"20240303121944.20627-1-l.s.r@web.de","subject":"[PATCH v2 4/6] parse-options: detect ambiguous self-negation","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-03-03T12:19:41Z","receivedAt":"2024-03-03T12:20:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Git currently does not detect the ambiguity of an option that starts\nwith \"no\" like --notes and its negated form if given just --n or --no.\nAll Git commands with such options have other negatable options, and\nwe detect the ambiguity with them, so that's currently only a potential\nproblem for scripts that use git rev-parse --parseopt.\n\nLet's fix it nevertheless, as there's no need for that confusion.  To\ndetect the ambiguity we have to loosen the check in register_abbrev(),\nas an option is considered an alias of itself.  Add non-matching\nnegation flags as a criterion to recognize an option being ambiguous\nwith its negated form.\n\nAnd we need to keep going after finding a non-negated option as an\nabbreviated candidate and perform the negation checks in the same\nloop.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n parse-options.c               |  3 +--\n t/t1502-rev-parse-parseopt.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 398ebaef14..008c0f32cf 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -363,7 +363,7 @@ static void register_abbrev(struct parse_opt_ctx_t *p,\n \tif (p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT)\n \t\treturn;\n \tif (abbrev->option &&\n-\t    !is_alias(p, abbrev->option, option)) {\n+\t    !(abbrev->flags == flags && is_alias(p, abbrev->option, option))) {\n \t\t/*\n \t\t * If this is abbreviated, it is\n \t\t * ambiguous. So when there is no\n@@ -406,7 +406,6 @@ static enum parse_opt_result parse_long_opt(\n \t\t\tif (!strncmp(long_name, arg, arg_end - arg)) {\n \t\t\t\tregister_abbrev(p, options, flags ^ opt_flags,\n \t\t\t\t\t\t&abbrev, &ambiguous);\n-\t\t\t\tcontinue;\n \t\t\t}\n \t\t\t/* negation allowed? */\n \t\t\tif (options->flags & PARSE_OPT_NONEG)\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex f0737593c3..b754b9fd74 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -322,4 +322,15 @@ check_invalid_long_option optionspec-neg --no-positive-only\n check_invalid_long_option optionspec-neg --negative\n check_invalid_long_option optionspec-neg --no-no-negative\n\n+test_expect_success 'ambiguous: --no matches both --noble and --no-noble' '\n+\tcat >spec <<-\\EOF &&\n+\tsome-command [options]\n+\t--\n+\tnoble The feudal switch.\n+\tEOF\n+\ttest_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n+\tgit rev-parse --parseopt -- <spec 2>err --no &&\n+\tgrep \"error: ambiguous option: no (could be --noble or --no-noble)\" err\n+'\n+\n test_done\n--\n2.44.0\n\n"},{"id":"489816","messageId":"20240303121944.20627-4-l.s.r@web.de","threadId":"60987","inReplyTo":"20240303121944.20627-1-l.s.r@web.de","subject":"[PATCH v2 3/6] parse-options: factor out register_abbrev() and struct parsed_option","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-03-03T12:19:40Z","receivedAt":"2024-03-03T12:20:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a function, register_abbrev(), for storing the necessary details for\nremembering an abbreviated and thus potentially ambiguous option.  Call\nit instead of sharing the code using goto, to make the control flow more\nexplicit.\n\nConveniently collect these details in the new struct parsed_option to\nreduce the number of necessary function arguments.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n parse-options.c | 83 +++++++++++++++++++++++++++++--------------------\n 1 file changed, 49 insertions(+), 34 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 056c6b30e9..398ebaef14 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -350,14 +350,40 @@ static int is_alias(struct parse_opt_ctx_t *ctx,\n \treturn 0;\n }\n\n+struct parsed_option {\n+\tconst struct option *option;\n+\tenum opt_parsed flags;\n+};\n+\n+static void register_abbrev(struct parse_opt_ctx_t *p,\n+\t\t\t    const struct option *option, enum opt_parsed flags,\n+\t\t\t    struct parsed_option *abbrev,\n+\t\t\t    struct parsed_option *ambiguous)\n+{\n+\tif (p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT)\n+\t\treturn;\n+\tif (abbrev->option &&\n+\t    !is_alias(p, abbrev->option, option)) {\n+\t\t/*\n+\t\t * If this is abbreviated, it is\n+\t\t * ambiguous. So when there is no\n+\t\t * exact match later, we need to\n+\t\t * error out.\n+\t\t */\n+\t\tambiguous->option = abbrev->option;\n+\t\tambiguous->flags = abbrev->flags;\n+\t}\n+\tabbrev->option = option;\n+\tabbrev->flags = flags;\n+}\n+\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+\tstruct parsed_option abbrev = { .option = NULL, .flags = OPT_LONG };\n+\tstruct parsed_option ambiguous = { .option = NULL, .flags = OPT_LONG };\n\n \tfor (; options->type != OPTION_END; options++) {\n \t\tconst char *rest, *long_name = options->long_name;\n@@ -377,31 +403,20 @@ static enum parse_opt_result parse_long_opt(\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\tabbrev_option = options;\n-\t\t\t\tabbrev_flags = flags ^ opt_flags;\n+\t\t\tif (!strncmp(long_name, arg, arg_end - arg)) {\n+\t\t\t\tregister_abbrev(p, options, flags ^ opt_flags,\n+\t\t\t\t\t\t&abbrev, &ambiguous);\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\tif (starts_with(\"no-\", arg)) {\n \t\t\t\tflags |= OPT_UNSET;\n-\t\t\t\tgoto is_abbreviated;\n+\t\t\t\tregister_abbrev(p, options, flags ^ opt_flags,\n+\t\t\t\t\t\t&abbrev, &ambiguous);\n+\t\t\t\tcontinue;\n \t\t\t}\n \t\t\t/* negated? */\n \t\t\tif (!starts_with(arg, \"no-\"))\n@@ -409,12 +424,12 @@ static enum parse_opt_result parse_long_opt(\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    !strncmp(long_name, arg + 3,\n+\t\t\t\tif (!strncmp(long_name, arg + 3,\n \t\t\t\t\t     arg_end - 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\t\tregister_abbrev(p, options,\n+\t\t\t\t\t\t\tflags ^ opt_flags,\n+\t\t\t\t\t\t\t&abbrev, &ambiguous);\n+\t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n \t\tif (*rest) {\n@@ -425,24 +440,24 @@ static enum parse_opt_result parse_long_opt(\n \t\treturn get_value(p, options, flags ^ opt_flags);\n \t}\n\n-\tif (disallow_abbreviated_options && (ambiguous_option || abbrev_option))\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+\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\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+\tif (abbrev.option) {\n \t\tif (*arg_end)\n \t\t\tp->opt = arg_end + 1;\n-\t\treturn get_value(p, abbrev_option, abbrev_flags);\n+\t\treturn get_value(p, abbrev.option, abbrev.flags);\n \t}\n \treturn PARSE_OPT_UNKNOWN;\n }\n--\n2.44.0\n\n"},{"id":"490507","messageId":"ZfDMWoRuftBcg-19@google.com","threadId":"60987","inReplyTo":"20240303121944.20627-2-l.s.r@web.de","subject":"Re: [PATCH v2 1/6] parse-options: recognize abbreviated negated option with arg","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-03-12T21:42:50Z","receivedAt":"2024-03-12T21:42:56Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.03.03 13:19, René Scharfe wrote:\n> Giving an argument to an option that doesn't take one causes Git to\n> report that error specifically:\n> \n>    $ git rm --dry-run=bogus\n>    error: option `dry-run' takes no value\n> \n> The same is true when the option is negated or abbreviated:\n> \n>    $ git rm --no-dry-run=bogus\n>    error: option `no-dry-run' takes no value\n> \n>    $ git rm --dry=bogus\n>    error: option `dry-run' takes no value\n> \n> Not so when doing both, though:\n> \n>    $ git rm --no-dry=bogus\n>    error: unknown option `no-dry=bogus'\n>    usage: git rm [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch]\n> \n> (Rest of the usage message omitted.)\n> \n> Improve consistency and usefulness of the error message by recognizing\n> abbreviated negated options even if they have a (most likely bogus)\n> argument.  With this patch we get:\n> \n>    $ git rm --no-dry=bogus\n>    error: option `no-dry-run' takes no value\n> \n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  parse-options.c          |  5 +++--\n>  t/t0040-parse-options.sh | 16 ++++++++++++++++\n>  2 files changed, 19 insertions(+), 2 deletions(-)\n> \n> diff --git a/parse-options.c b/parse-options.c\n> index 63a99dea6e..e4ce33ea48 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -391,7 +391,7 @@ static enum parse_opt_result parse_long_opt(\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\tif (*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\nSo this part allows us to recognize when we've attached an option onto a\nnegated flag (like \"--no-foo=bar\"). Looks good.\n\n\n> @@ -412,7 +412,8 @@ static enum parse_opt_result parse_long_opt(\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    !strncmp(long_name, arg + 3,\n> +\t\t\t\t\t     arg_end - arg - 3))\n>  \t\t\t\t\tgoto is_abbreviated;\n>  \t\t\t\telse\n>  \t\t\t\t\tcontinue;\n\nAnd now we fix the abbreviation match. So if arg=\"no-foo=bar\", (arg+3) =\n\"foo=bar\" and thus never matches a long_name. We fix that by only\ncomparing up to the first equal sign. Also looks good.\n\n> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> index ec974867e4..8bb2a8b453 100755\n> --- a/t/t0040-parse-options.sh\n> +++ b/t/t0040-parse-options.sh\n> @@ -210,6 +210,22 @@ test_expect_success 'superfluous value provided: boolean' '\n>  \ttest_cmp expect actual\n>  '\n> \n> +test_expect_success 'superfluous value provided: boolean, abbreviated' '\n> +\tcat >expect <<-\\EOF &&\n> +\terror: option `yes'\\'' takes no value\n> +\tEOF\n> +\ttest_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n> +\ttest-tool parse-options --ye=hi 2>actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\terror: option `no-yes'\\'' takes no value\n> +\tEOF\n> +\ttest_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n> +\ttest-tool parse-options --no-ye=hi 2>actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_expect_success 'superfluous value provided: cmdmode' '\n>  \tcat >expect <<-\\EOF &&\n>  \terror: option `mode1'\\'' takes no value\n> --\n> 2.44.0\n\nTest looks good as well.\n"},{"id":"490508","messageId":"ZfDMhJ4r5g-g-B3n@google.com","threadId":"60987","inReplyTo":"20240303121944.20627-4-l.s.r@web.de","subject":"Re: [PATCH v2 3/6] parse-options: factor out register_abbrev() and struct parsed_option","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-03-12T21:43:32Z","receivedAt":"2024-03-12T21:43:37Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"I found this change to be hard to follow, although I'm not sure anything\nactually needs to be changed. Thinking aloud below, apologies for being\nverbose.\n\nOn 2024.03.03 13:19, René Scharfe wrote:\n> Add a function, register_abbrev(), for storing the necessary details for\n> remembering an abbreviated and thus potentially ambiguous option.  Call\n> it instead of sharing the code using goto, to make the control flow more\n> explicit.\n> \n> Conveniently collect these details in the new struct parsed_option to\n> reduce the number of necessary function arguments.\n> \n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  parse-options.c | 83 +++++++++++++++++++++++++++++--------------------\n>  1 file changed, 49 insertions(+), 34 deletions(-)\n> \n> diff --git a/parse-options.c b/parse-options.c\n> index 056c6b30e9..398ebaef14 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -350,14 +350,40 @@ static int is_alias(struct parse_opt_ctx_t *ctx,\n>  \treturn 0;\n>  }\n> \n> +struct parsed_option {\n> +\tconst struct option *option;\n> +\tenum opt_parsed flags;\n> +};\n\nWe're bundling up the state for previously-examined options that \"arg\"\nmight expand to. Looks good.\n\n\n> +static void register_abbrev(struct parse_opt_ctx_t *p,\n> +\t\t\t    const struct option *option, enum opt_parsed flags,\n> +\t\t\t    struct parsed_option *abbrev,\n> +\t\t\t    struct parsed_option *ambiguous)\n> +{\n\nHere we're defining a function to separate out the logic that we\npreviously jumped to with \"goto is_abbreviated;\". Looks good.\n\n\n> +\tif (p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT)\n> +\t\treturn;\n\nThis is the (negation of the) allow_abbrev condition that was removed\nbelow. Looks good.\n\n> +\tif (abbrev->option &&\n> +\t    !is_alias(p, abbrev->option, option)) {\n> +\t\t/*\n> +\t\t * If this is abbreviated, it is\n> +\t\t * ambiguous. So when there is no\n> +\t\t * exact match later, we need to\n> +\t\t * error out.\n> +\t\t */\n> +\t\tambiguous->option = abbrev->option;\n> +\t\tambiguous->flags = abbrev->flags;\n\nCouldn't this just be \"*ambiguous = *abbrev;\" ?\n\n\n> +\t}\n> +\tabbrev->option = option;\n> +\tabbrev->flags = flags;\n> +}\n\nSo, we've found a candidate that matches our abbreviation. If this is\nthe second (or later) candidate, then we've got an ambiguous\nabbreviation, which we'll need to warn about later. Since we're\noverwriting both \"ambiguous\" and \"abbrev\", we'll only refer to the last\ntwo candidates seen, but that seems fine.\n\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> +\tstruct parsed_option abbrev = { .option = NULL, .flags = OPT_LONG };\n> +\tstruct parsed_option ambiguous = { .option = NULL, .flags = OPT_LONG };\n> \n>  \tfor (; options->type != OPTION_END; options++) {\n>  \t\tconst char *rest, *long_name = options->long_name;\n> @@ -377,31 +403,20 @@ static enum parse_opt_result parse_long_opt(\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\tabbrev_option = options;\n> -\t\t\t\tabbrev_flags = flags ^ opt_flags;\n> +\t\t\tif (!strncmp(long_name, arg, arg_end - arg)) {\n> +\t\t\t\tregister_abbrev(p, options, flags ^ opt_flags,\n> +\t\t\t\t\t\t&abbrev, &ambiguous);\n\nThis part I found hard to follow; the change itself is a simple\nreplacement of the original logic with our new function, but I don't\nunderstand the original logic in the first place. Why do we XOR flags\nand opt_flags here?\n\nThese are both bitflags which can hold a combination of OPT_SHORT and/or\nOPT_UNSET (i.e., we've passed --no-foo). Since we're only looking at\nargs that we already know are in long form, the only one we should need\nto worry about is whether OPT_UNSET is true or false. And indeed, we\nnever set OPT_SHORT in this function.\n\nIIUC, \"flags\" corresponds to \"arg\", the flag we're trying to parse, and\n\"opt_flags\" corresponds to \"options\", the current item in the list of\nall options that we're trying to match against.\n\nSo OPT_UNSET will be true in \"flags\" if either \"arg\" starts with \"no-\"\nor if it is a prefix of \"no-\".\n\nOPT_UNSET will be true in \"opt_flags\" if all of the following are true:\n* \"arg\" does not start with \"no-\"\n* PARSE_OPT_NONEG is not set in options->flags\n* options->long_name starts with \"no-\"\n\nSo OPT_UNSET can never be set at the same time in both \"flags\" and\n\"opt_flags\", and thus the XOR makes a bit more sense: either the\nargument we're parsing or the canonical form of the candidate that we're\nmatching against is negated. We don't care which one, but we need to\nknow about the negation later, to either set the value properly, or to\nreport potential ambiguities with the \"no-\" prefix.\n\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\tif (starts_with(\"no-\", arg)) {\n>  \t\t\t\tflags |= OPT_UNSET;\n> -\t\t\t\tgoto is_abbreviated;\n> +\t\t\t\tregister_abbrev(p, options, flags ^ opt_flags,\n> +\t\t\t\t\t\t&abbrev, &ambiguous);\n> +\t\t\t\tcontinue;\n\nThis is a slight change: we might set OPT_UNSET in flags where before we\nwere prevented by the \"allow_abbrev\" condition, but register_abbrev will\nstill be a no-op in that case, so I don't think this changes any\nbehavior.\n\nAll the other changes in this patch are straightforward. So, despite\nhaving to puzzle out the original logic, everything here looks good.\n"},{"id":"490509","messageId":"ZfDMn-3dKDQwYx_v@google.com","threadId":"60987","inReplyTo":"20240303121944.20627-6-l.s.r@web.de","subject":"Re: [PATCH v2 5/6] parse-options: normalize arg and long_name before comparison","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-03-12T21:43:59Z","receivedAt":"2024-03-12T21:44:04Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.03.03 13:19, René Scharfe wrote:\n> Strip \"no-\" from arg and long_name before comparing them.  This way we\n> no longer have to repeat the comparison with an offset of 3 for negated\n> arguments.\n> \n> Note that we must not modify the \"flags\" value, which tracks whether arg\n> is negated, inside the loop.  When registering \"--n\", \"--no\" or \"--no-\"\n> as abbreviation for any negative option, we used to OR it with OPT_UNSET\n> and end the loop.  We can simply hard-code OPT_UNSET and leave flags\n> unchanged instead.\n> \n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  parse-options.c | 44 ++++++++++++++++++++++----------------------\n>  1 file changed, 22 insertions(+), 22 deletions(-)\n> \n> diff --git a/parse-options.c b/parse-options.c\n> index 008c0f32cf..d45efa4e5c 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -382,28 +382,42 @@ static enum parse_opt_result parse_long_opt(\n>  \tconst struct option *options)\n>  {\n>  \tconst char *arg_end = strchrnul(arg, '=');\n> +\tconst char *arg_start = arg;\n> +\tenum opt_parsed flags = OPT_LONG;\n> +\tint arg_starts_with_no_no = 0;\n>  \tstruct parsed_option abbrev = { .option = NULL, .flags = OPT_LONG };\n>  \tstruct parsed_option ambiguous = { .option = NULL, .flags = OPT_LONG };\n> \n> +\tif (skip_prefix(arg_start, \"no-\", &arg_start)) {\n> +\t\tif (skip_prefix(arg_start, \"no-\", &arg_start))\n> +\t\t\targ_starts_with_no_no = 1;\n> +\t\telse\n> +\t\t\tflags |= OPT_UNSET;\n> +\t}\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> +\t\tenum opt_parsed opt_flags = OPT_LONG;\n> +\t\tint allow_unset = !(options->flags & PARSE_OPT_NONEG);\n> \n>  \t\tif (options->type == OPTION_SUBCOMMAND)\n>  \t\t\tcontinue;\n>  \t\tif (!long_name)\n>  \t\t\tcontinue;\n> \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\tif (skip_prefix(long_name, \"no-\", &long_name))\n>  \t\t\topt_flags |= OPT_UNSET;\n> +\t\telse if (arg_starts_with_no_no)\n> +\t\t\tcontinue;\n\nWe only allow double negation (\"--no-no-foo\") if the canonical form\nstarts with \"--no-\". Makes sense.\n\n\n> +\t\tif (((flags ^ opt_flags) & OPT_UNSET) && !allow_unset)\n> +\t\t\tcontinue;\n\nI don't think the \"& OPT_UNSET\" is required here, as we never set any\nflags other than OPT_UNSET, but I think it's fine to keep it for\nclarity.\n\n\n> -\t\tif (!skip_prefix(arg, long_name, &rest))\n> +\t\tif (!skip_prefix(arg_start, long_name, &rest))\n>  \t\t\trest = NULL;\n>  \t\tif (!rest) {\n>  \t\t\t/* abbreviated? */\n> -\t\t\tif (!strncmp(long_name, arg, arg_end - arg)) {\n> +\t\t\tif (!strncmp(long_name, arg_start, arg_end - arg_start)) {\n>  \t\t\t\tregister_abbrev(p, options, flags ^ opt_flags,\n>  \t\t\t\t\t\t&abbrev, &ambiguous);\n>  \t\t\t}\n> @@ -412,24 +426,10 @@ static enum parse_opt_result parse_long_opt(\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\t\tflags |= OPT_UNSET;\n> -\t\t\t\tregister_abbrev(p, options, flags ^ opt_flags,\n> +\t\t\t\tregister_abbrev(p, options, OPT_UNSET ^ opt_flags,\n>  \t\t\t\t\t\t&abbrev, &ambiguous);\n> -\t\t\t\tcontinue;\n> -\t\t\t}\n> -\t\t\t/* negated? */\n> -\t\t\tif (!starts_with(arg, \"no-\"))\n> -\t\t\t\tcontinue;\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 (!strncmp(long_name, arg + 3,\n> -\t\t\t\t\t     arg_end - arg - 3))\n> -\t\t\t\t\tregister_abbrev(p, options,\n> -\t\t\t\t\t\t\tflags ^ opt_flags,\n> -\t\t\t\t\t\t\t&abbrev, &ambiguous);\n> -\t\t\t\tcontinue;\n>  \t\t\t}\n> +\t\t\tcontinue;\n>  \t\t}\n>  \t\tif (*rest) {\n>  \t\t\tif (*rest != '=')\n> --\n> 2.44.0\n> \n> \n"},{"id":"490510","messageId":"ZfDMr8rRftxXmTXf@google.com","threadId":"60987","inReplyTo":"20240303121944.20627-7-l.s.r@web.de","subject":"Re: [PATCH v2 6/6] parse-options: rearrange long_name matching code","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-03-12T21:44:15Z","receivedAt":"2024-03-12T21:44:21Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.03.03 13:19, René Scharfe wrote:\n> Move the code for handling a full match of long_name first and get rid\n> of negations.  Reduce the indent of the code for matching abbreviations\n> and remove unnecessary curly braces.  Combine the checks for whether\n> negation is allowed and whether arg is \"n\", \"no\" or \"no-\" because they\n> belong together and avoid a continue statement.  The result is shorter,\n> more readable code.\n> \n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  parse-options.c | 37 +++++++++++++++----------------------\n>  1 file changed, 15 insertions(+), 22 deletions(-)\n\nThis version is much more readable, thanks for the cleanup!\n"},{"id":"490511","messageId":"ZfDM5Or3EKw7Q9SA@google.com","threadId":"60987","inReplyTo":"20240303121944.20627-1-l.s.r@web.de","subject":"Re: [PATCH v2 0/6] parse-options: long option parsing fixes and cleanup","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-03-12T21:45:08Z","receivedAt":"2024-03-12T21:45:14Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.03.03 13:19, René Scharfe wrote:\n> Fix two corner cases in patches 1 and 4, refactor the code in patches 2\n> and 3 to prepare the second fix, bonus patches 5 and 6 simplify the code\n> and improve readability.\n> \n> Changes since v1:\n> * Added sign-off.\n> \n>   parse-options: recognize abbreviated negated option with arg\n>   parse-options: set arg of abbreviated option lazily\n>   parse-options: factor out register_abbrev() and struct parsed_option\n>   parse-options: detect ambiguous self-negation\n>   parse-options: normalize arg and long_name before comparison\n>   parse-options: rearrange long_name matching code\n> \n>  parse-options.c               | 137 ++++++++++++++++++----------------\n>  t/t0040-parse-options.sh      |  16 ++++\n>  t/t1502-rev-parse-parseopt.sh |  11 +++\n>  3 files changed, 100 insertions(+), 64 deletions(-)\n> \n> --\n> 2.44.0\n> \n> \n\nThis series is a good cleanup of some code that at least to me was hard\nto figure out on first read. Thanks for the series!\n\nReviewed-by: Josh Steadmon <steadmon@google.com>\n"},{"id":"490560","messageId":"xmqqil1qc9b6.fsf@gitster.g","threadId":"60987","inReplyTo":"ZfDMhJ4r5g-g-B3n@google.com","subject":"Re: [PATCH v2 3/6] parse-options: factor out register_abbrev() and struct parsed_option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-13T17:48:13Z","receivedAt":"2024-03-13T17:48:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon <steadmon@google.com> writes:\n\n> I found this change to be hard to follow, although I'm not sure anything\n> actually needs to be changed. Thinking aloud below, apologies for being\n> verbose.\n\nThanks for carefully following the code.  It unfortunately has to\nget long, but this is the kind of review that I would appreciate\nmost if I were the author, pointing out what is easy to understand\nand more importantly what is harder to follow.\n"},{"id":"490572","messageId":"e820c026-4897-434d-8c17-b7febf3d07a0@web.de","threadId":"60987","inReplyTo":"ZfDMhJ4r5g-g-B3n@google.com","subject":"Re: [PATCH v2 3/6] parse-options: factor out register_abbrev() and struct parsed_option","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-03-13T18:47:48Z","receivedAt":"2024-03-13T18:47:57Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 12.03.24 um 22:43 schrieb Josh Steadmon:\n> I found this change to be hard to follow, although I'm not sure anything\n> actually needs to be changed. Thinking aloud below, apologies for being\n> verbose.\n\nThank you for taking the time to review and share!\n\n> On 2024.03.03 13:19, René Scharfe wrote:\n>> Add a function, register_abbrev(), for storing the necessary details for\n>> remembering an abbreviated and thus potentially ambiguous option.  Call\n>> it instead of sharing the code using goto, to make the control flow more\n>> explicit.\n>>\n>> Conveniently collect these details in the new struct parsed_option to\n>> reduce the number of necessary function arguments.\n>>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>> ---\n>>  parse-options.c | 83 +++++++++++++++++++++++++++++--------------------\n>>  1 file changed, 49 insertions(+), 34 deletions(-)\n>>\n>> diff --git a/parse-options.c b/parse-options.c\n>> index 056c6b30e9..398ebaef14 100644\n>> --- a/parse-options.c\n>> +++ b/parse-options.c\n>> @@ -350,14 +350,40 @@ static int is_alias(struct parse_opt_ctx_t *ctx,\n>>  \treturn 0;\n>>  }\n>>\n>> +struct parsed_option {\n>> +\tconst struct option *option;\n>> +\tenum opt_parsed flags;\n>> +};\n>\n> We're bundling up the state for previously-examined options that \"arg\"\n> might expand to. Looks good.\n>\n>\n>> +static void register_abbrev(struct parse_opt_ctx_t *p,\n>> +\t\t\t    const struct option *option, enum opt_parsed flags,\n>> +\t\t\t    struct parsed_option *abbrev,\n>> +\t\t\t    struct parsed_option *ambiguous)\n>> +{\n>\n> Here we're defining a function to separate out the logic that we\n> previously jumped to with \"goto is_abbreviated;\". Looks good.\n>\n>\n>> +\tif (p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT)\n>> +\t\treturn;\n>\n> This is the (negation of the) allow_abbrev condition that was removed\n> below. Looks good.\n>\n>> +\tif (abbrev->option &&\n>> +\t    !is_alias(p, abbrev->option, option)) {\n>> +\t\t/*\n>> +\t\t * If this is abbreviated, it is\n>> +\t\t * ambiguous. So when there is no\n>> +\t\t * exact match later, we need to\n>> +\t\t * error out.\n>> +\t\t */\n>> +\t\tambiguous->option = abbrev->option;\n>> +\t\tambiguous->flags = abbrev->flags;\n>\n> Couldn't this just be \"*ambiguous = *abbrev;\" ?\n\nIt could, but I wanted to keep it as close to the original as possible.\nI was hoping the changes made here would be mechanical enough to be\nreviewable without requiring a deeper understanding of what the code\nactually does, but that didn't quite work out it seems.  Glad you made\nit through, though! :)\n\n>\n>\n>> +\t}\n>> +\tabbrev->option = option;\n>> +\tabbrev->flags = flags;\n>> +}\n>\n> So, we've found a candidate that matches our abbreviation. If this is\n> the second (or later) candidate, then we've got an ambiguous\n> abbreviation, which we'll need to warn about later. Since we're\n> overwriting both \"ambiguous\" and \"abbrev\", we'll only refer to the last\n> two candidates seen, but that seems fine.\n>\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>> +\tstruct parsed_option abbrev = { .option = NULL, .flags = OPT_LONG };\n>> +\tstruct parsed_option ambiguous = { .option = NULL, .flags = OPT_LONG };\n>>\n>>  \tfor (; options->type != OPTION_END; options++) {\n>>  \t\tconst char *rest, *long_name = options->long_name;\n>> @@ -377,31 +403,20 @@ static enum parse_opt_result parse_long_opt(\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\tabbrev_option = options;\n>> -\t\t\t\tabbrev_flags = flags ^ opt_flags;\n>> +\t\t\tif (!strncmp(long_name, arg, arg_end - arg)) {\n>> +\t\t\t\tregister_abbrev(p, options, flags ^ opt_flags,\n>> +\t\t\t\t\t\t&abbrev, &ambiguous);\n>\n> This part I found hard to follow; the change itself is a simple\n> replacement of the original logic with our new function, but I don't\n> understand the original logic in the first place. Why do we XOR flags\n> and opt_flags here?\n>\n> These are both bitflags which can hold a combination of OPT_SHORT and/or\n> OPT_UNSET (i.e., we've passed --no-foo). Since we're only looking at\n> args that we already know are in long form, the only one we should need\n> to worry about is whether OPT_UNSET is true or false. And indeed, we\n> never set OPT_SHORT in this function.\n\nRight.  We do set OPT_LONG as well, but its value is 0, so that's just\nfor show.\n\n> IIUC, \"flags\" corresponds to \"arg\", the flag we're trying to parse, and\n> \"opt_flags\" corresponds to \"options\", the current item in the list of\n> all options that we're trying to match against.\n>\n> So OPT_UNSET will be true in \"flags\" if either \"arg\" starts with \"no-\"\n> or if it is a prefix of \"no-\".\n>\n> OPT_UNSET will be true in \"opt_flags\" if all of the following are true:\n> * \"arg\" does not start with \"no-\"\n> * PARSE_OPT_NONEG is not set in options->flags\n> * options->long_name starts with \"no-\"\n\nRight.\n\n> So OPT_UNSET can never be set at the same time in both \"flags\" and\n> \"opt_flags\",\n\nIt can if arg is \"no\" or \"n\" (the \"negated and abbreviated very much?\"\npath), PARSE_OPT_NONEG is unset and long_name starts with \"no-\".\n\n> and thus the XOR makes a bit more sense: either the\n> argument we're parsing or the canonical form of the candidate that we're\n> matching against is negated. We don't care which one, but we need to\n> know about the negation later, to either set the value properly, or to\n> report potential ambiguities with the \"no-\" prefix.\n\nRight.  The flags track whether the user negated an option, but with\nthe twist that both arguments given by the user and long names in the\noption definition can start with \"no-\":\n\narg        option       negated\n---------- ------------ -------\n--index     --index     no\n--index     --no-index  yes\n--no-index  --index     yes\n--no-index  --no-index  no\n\n\"negated\" means \"the opposite meaning of the defined option\" here,\nnot \"presence of no- somewhere\".  --index asks for the opposite\neffect of what an option defined as --no-index would do.\n\nIf OPT_UNSET is set in both, the XOR cancels it out.  E.g. \"--no\"\ndirectly matches \"--no-index\" as an abbreviation, without negating it.\n(It also matches all other negatable options, so that's only usable\nfor commands that have a single one of them..)\n\n>\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\tif (starts_with(\"no-\", arg)) {\n>>  \t\t\t\tflags |= OPT_UNSET;\n>> -\t\t\t\tgoto is_abbreviated;\n>> +\t\t\t\tregister_abbrev(p, options, flags ^ opt_flags,\n>> +\t\t\t\t\t\t&abbrev, &ambiguous);\n>> +\t\t\t\tcontinue;\n>\n> This is a slight change: we might set OPT_UNSET in flags where before we\n> were prevented by the \"allow_abbrev\" condition, but register_abbrev will\n> still be a no-op in that case, so I don't think this changes any\n> behavior.\n\nIndeed.  The continue statement starts the loop over and initializes\nflags anew, and the allow_abbrev condition is checked at the top of\nregister_abbrev(), as you noted.\n\n> All the other changes in this patch are straightforward. So, despite\n> having to puzzle out the original logic, everything here looks good.\n"}]}