{"thread":{"id":"65084","subject":"[Bug] duplicated long-form options go unnoticed","startedAt":"2026-02-27T00:13:19Z","lastAt":"2026-03-02T18:24:04Z","messageCount":12,"participants":["Junio C Hamano","René Scharfe","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"537259","messageId":"xmqq5x7jujqb.fsf@gitster.g","threadId":"65084","inReplyTo":null,"subject":"[Bug] duplicated long-form options go unnoticed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-27T00:13:16Z","receivedAt":"2026-02-27T00:13:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"If you make this stupid change to builtin/cat-file.c, rebuild your\ngit and run \"git cat-file --batch-check\", without anybody helping\nyou notice that your change to add a duplicated long command to the\noptions table is a nonsense.  There should be some way to help the\ndeveloper.\n\nThe most expensive would be a run-time check in parse_options_check(),\nwhich is not very advisable, but it may be OK to have one hidden behind\na conditional debugging option (like exporting GIT_PARSEOPT_PARFNOID\nvariable).\n\n\n builtin/cat-file.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git i/builtin/cat-file.c w/builtin/cat-file.c\nindex b6f12f41d6..eaa53b2b29 100644\n--- i/builtin/cat-file.c\n+++ w/builtin/cat-file.c\n@@ -1091,6 +1091,7 @@ int cmd_cat_file(int argc,\n \t\t\tN_(\"like --batch, but don't emit <contents>\"),\n \t\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n \t\t\tbatch_option_callback),\n+\t\tOPT_BOOL(0, \"batch-check\", &batch, N_(\"batch\")),\n \t\tOPT_BOOL_F('z', NULL, &input_nul_terminated, N_(\"stdin is NUL-terminated\"),\n \t\t\tPARSE_OPT_HIDDEN),\n \t\tOPT_BOOL('Z', NULL, &nul_terminated, N_(\"stdin and stdout is NUL-terminated\")),\n"},{"id":"537321","messageId":"7693799a-91a2-480a-ae3e-29f8eed5b55a@web.de","threadId":"65084","inReplyTo":"xmqq5x7jujqb.fsf@gitster.g","subject":"[PATCH 2/2] parseopt: check for duplicate long names and numerical options","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2026-02-27T19:27:02Z","receivedAt":"2026-02-27T19:27:04Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"We already check for duplicate short names.  Check for and report\nduplicate long names and numerical options as well.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nThe check clearly has a cost, but I have a hard time measuring it.\nWe already do lots of (kinda cheap) checks.  Turning them on only\nin DEVELOPER builds (and ideally demonstrating a speedup) left as\nan exercise for interested readers (with stronger benchmark-fu)..\n\n parse-options.c | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex c9cafc21b9..51b72eee11 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -5,6 +5,7 @@\n #include \"gettext.h\"\n #include \"strbuf.h\"\n #include \"string-list.h\"\n+#include \"strmap.h\"\n #include \"utf8.h\"\n \n static int disallow_abbreviated_options;\n@@ -641,6 +642,8 @@ static void check_typos(const char *arg, const struct option *options)\n static void parse_options_check(const struct option *opts)\n {\n \tchar short_opts[128];\n+\tstruct strset long_names = STRSET_INIT;\n+\tbool saw_number_option = false;\n \tvoid *subcommand_value = NULL;\n \n \tmemset(short_opts, '\\0', sizeof(short_opts));\n@@ -655,6 +658,16 @@ static void parse_options_check(const struct option *opts)\n \t\t\telse if (short_opts[opts->short_name]++)\n \t\t\t\toptbug(opts, \"short name already used\");\n \t\t}\n+\t\tif (opts->long_name) {\n+\t\t\tif (strset_contains(&long_names, opts->long_name))\n+\t\t\t\toptbug(opts, \"long name already used\");\n+\t\t\tstrset_add(&long_names, opts->long_name);\n+\t\t}\n+\t\tif (opts->type == OPTION_NUMBER) {\n+\t\t\tif (saw_number_option)\n+\t\t\t\toptbug(opts, \"duplicate numerical option\");\n+\t\t\tsaw_number_option = true;\n+\t\t}\n \t\tif (opts->flags & PARSE_OPT_NODASH &&\n \t\t    ((opts->flags & PARSE_OPT_OPTARG) ||\n \t\t     !(opts->flags & PARSE_OPT_NOARG) ||\n@@ -712,6 +725,7 @@ static void parse_options_check(const struct option *opts)\n \t\t\toptbug(opts, \"multi-word argh should use dash to separate words\");\n \t}\n \tBUG_if_bug(\"invalid 'struct option'\");\n+\tstrset_clear(&long_names);\n }\n \n static int has_subcommands(const struct option *options)\n-- \n2.53.0\n"},{"id":"537322","messageId":"1e7de0f7-a712-465f-b3c9-5dbe78132d3f@web.de","threadId":"65084","inReplyTo":"xmqq5x7jujqb.fsf@gitster.g","subject":"[PATCH 1/2] pack-objects: remove duplicate --stdin-packs definition","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2026-02-27T19:27:00Z","receivedAt":"2026-02-27T19:27:08Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"cd846bacc7 (pack-objects: introduce '--stdin-packs=follow', 2025-06-23)\nadded a new definition of the option --stdin-packs that accepts an\nargument.  It kept the old definition, which still shows up in the short\nhelp, but is shadowed by the new one.  Remove it.\n\nHinted-at-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/pack-objects.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex cfb03d4c09..1ea823f1fb 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4939,8 +4939,6 @@ int cmd_pack_objects(int argc,\n \t\tOPT_CALLBACK_F(0, \"stdin-packs\", &stdin_packs, N_(\"mode\"),\n \t\t\t     N_(\"read packs from stdin\"),\n \t\t\t     PARSE_OPT_OPTARG, parse_stdin_packs_mode),\n-\t\tOPT_BOOL(0, \"stdin-packs\", &stdin_packs,\n-\t\t\t N_(\"read packs from stdin\")),\n \t\tOPT_BOOL(0, \"stdout\", &pack_to_stdout,\n \t\t\t N_(\"output pack to stdout\")),\n \t\tOPT_BOOL(0, \"include-tag\", &include_tag,\n-- \n2.53.0\n"},{"id":"537357","messageId":"20260227225055.GC2956443@coredump.intra.peff.net","threadId":"65084","inReplyTo":"7693799a-91a2-480a-ae3e-29f8eed5b55a@web.de","subject":"Re: [PATCH 2/2] parseopt: check for duplicate long names and numerical options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-27T22:50:55Z","receivedAt":"2026-02-27T22:50:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 27, 2026 at 08:27:02PM +0100, René Scharfe wrote:\n\n> The check clearly has a cost, but I have a hard time measuring it.\n> We already do lots of (kinda cheap) checks.  Turning them on only\n> in DEVELOPER builds (and ideally demonstrating a speedup) left as\n> an exercise for interested readers (with stronger benchmark-fu)..\n\nI agree it is probably not introducing a measurable slowdown. If we were\nto make it conditional, I'd suggest a run-time toggle (so we could turn\nit on for all test scripts, but not regular use).\n\nThat said...\n\n> @@ -655,6 +658,16 @@ static void parse_options_check(const struct option *opts)\n>  \t\t\telse if (short_opts[opts->short_name]++)\n>  \t\t\t\toptbug(opts, \"short name already used\");\n>  \t\t}\n> +\t\tif (opts->long_name) {\n> +\t\t\tif (strset_contains(&long_names, opts->long_name))\n> +\t\t\t\toptbug(opts, \"long name already used\");\n> +\t\t\tstrset_add(&long_names, opts->long_name);\n> +\t\t}\n\n...if you want to micro-optimize, note that the return value of\nstrset_add() tells you whether the item was already in the set. That can\nsave one hash of the string.\n\nProbably the allocation for each element is the dominating cost, though,\nand it doesn't help with that.\n\n-Peff\n"},{"id":"537358","messageId":"20260227230822.GA2965111@coredump.intra.peff.net","threadId":"65084","inReplyTo":"20260227225055.GC2956443@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] parseopt: check for duplicate long names and numerical options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-27T23:08:22Z","receivedAt":"2026-02-27T23:08:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 27, 2026 at 05:50:56PM -0500, Jeff King wrote:\n\n> On Fri, Feb 27, 2026 at 08:27:02PM +0100, René Scharfe wrote:\n> \n> > The check clearly has a cost, but I have a hard time measuring it.\n> > We already do lots of (kinda cheap) checks.  Turning them on only\n> > in DEVELOPER builds (and ideally demonstrating a speedup) left as\n> > an exercise for interested readers (with stronger benchmark-fu)..\n> \n> I agree it is probably not introducing a measurable slowdown. If we were\n> to make it conditional, I'd suggest a run-time toggle (so we could turn\n> it on for all test scripts, but not regular use).\n\nJust for fun, I was going to write a script that generated a test-tool\nparse-options list with 100k entries. But then I realized we already\nhave something like that!\n\nIf you do this:\n\n  (\n    echo usage\n    echo --\n    for i in $(seq 100000); do\n      echo \"opt$i option $i\"\n    done\n  ) >input\n\nthen hyperfine reports (before and after your patches):\n\n  Benchmark 1: ./git.old rev-parse --parseopt -- --opt42 <input\n    Time (mean ± σ):      22.2 ms ±   0.4 ms    [User: 16.6 ms, System: 5.6 ms]\n    Range (min … max):    21.5 ms …  23.9 ms    127 runs\n  \n  Benchmark 2: ./git.new rev-parse --parseopt -- --opt42 <input\n    Time (mean ± σ):      32.5 ms ±   0.5 ms    [User: 23.8 ms, System: 8.6 ms]\n    Range (min … max):    31.7 ms …  34.8 ms    89 runs\n  \n  Summary\n    ./git.old rev-parse --parseopt -- --opt42 <input ran\n      1.46 ± 0.03 times faster than ./git.new rev-parse --parseopt -- --opt42 <input\n\nSo it is measurable (even with the extra per-option costs to generate\nthe option structs in the first place). Looks like on the order of 10ms\nfor 100k options, or about 100ns per option. If you imagine that most\noption lists are smaller than 100, we're talking about probably the\nequivalent of 50-100 syscalls. If we are really looking to\nmicro-optimize startup time, I suspect there's pretty low-hanging fruit\nto be found of that magnitude.\n\n> > +\t\tif (opts->long_name) {\n> > +\t\t\tif (strset_contains(&long_names, opts->long_name))\n> > +\t\t\t\toptbug(opts, \"long name already used\");\n> > +\t\t\tstrset_add(&long_names, opts->long_name);\n> > +\t\t}\n> \n> ...if you want to micro-optimize, note that the return value of\n> strset_add() tells you whether the item was already in the set. That can\n> save one hash of the string.\n> \n> Probably the allocation for each element is the dominating cost, though,\n> and it doesn't help with that.\n\nDoing this:\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 51b72eee11..f056a4471e 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -659,9 +659,8 @@ static void parse_options_check(const struct option *opts)\n \t\t\t\toptbug(opts, \"short name already used\");\n \t\t}\n \t\tif (opts->long_name) {\n-\t\t\tif (strset_contains(&long_names, opts->long_name))\n+\t\t\tif (!strset_add(&long_names, opts->long_name))\n \t\t\t\toptbug(opts, \"long name already used\");\n-\t\t\tstrset_add(&long_names, opts->long_name);\n \t\t}\n \t\tif (opts->type == OPTION_NUMBER) {\n \t\t\tif (saw_number_option)\n\nseems to shave off ~1% of my benchmark. Not that exciting, but hey, it's\none line shorter to boot.\n\n-Peff\n"},{"id":"537359","messageId":"xmqqpl5pojg6.fsf@gitster.g","threadId":"65084","inReplyTo":"20260227230822.GA2965111@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] parseopt: check for duplicate long names and numerical options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-27T23:28:09Z","receivedAt":"2026-02-27T23:28:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Doing this:\n>\n> diff --git a/parse-options.c b/parse-options.c\n> index 51b72eee11..f056a4471e 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -659,9 +659,8 @@ static void parse_options_check(const struct option *opts)\n>  \t\t\t\toptbug(opts, \"short name already used\");\n>  \t\t}\n>  \t\tif (opts->long_name) {\n> -\t\t\tif (strset_contains(&long_names, opts->long_name))\n> +\t\t\tif (!strset_add(&long_names, opts->long_name))\n>  \t\t\t\toptbug(opts, \"long name already used\");\n> -\t\t\tstrset_add(&long_names, opts->long_name);\n>  \t\t}\n>  \t\tif (opts->type == OPTION_NUMBER) {\n>  \t\t\tif (saw_number_option)\n>\n> seems to shave off ~1% of my benchmark. Not that exciting, but hey, it's\n> one line shorter to boot.\n\nYeah, it is the right thing not to hash the same thing twice which\nis totally unnecessary.\n"},{"id":"537383","messageId":"dc882c28-0846-41e3-a9e8-1a4bc44a1ebc@web.de","threadId":"65084","inReplyTo":"20260227230822.GA2965111@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] parseopt: check for duplicate long names and numerical options","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2026-02-28T09:19:11Z","receivedAt":"2026-02-28T09:19:16Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 2/28/26 12:08 AM, Jeff King wrote:\n> On Fri, Feb 27, 2026 at 05:50:56PM -0500, Jeff King wrote:\n> \n>> On Fri, Feb 27, 2026 at 08:27:02PM +0100, René Scharfe wrote:\n>>\n>>> The check clearly has a cost, but I have a hard time measuring it.\n>>> We already do lots of (kinda cheap) checks.  Turning them on only\n>>> in DEVELOPER builds (and ideally demonstrating a speedup) left as\n>>> an exercise for interested readers (with stronger benchmark-fu)..\n>>\n>> I agree it is probably not introducing a measurable slowdown. If we were\n>> to make it conditional, I'd suggest a run-time toggle (so we could turn\n>> it on for all test scripts, but not regular use).\n\nGood idea.  We could piggy-back on -h.\n\n> Just for fun, I was going to write a script that generated a test-tool\n> parse-options list with 100k entries. But then I realized we already\n> have something like that!\n> \n> If you do this:\n> \n>   (\n>     echo usage\n>     echo --\n>     for i in $(seq 100000); do\n>       echo \"opt$i option $i\"\n>     done\n>   ) >input\n> \n> then hyperfine reports (before and after your patches):\n> \n>   Benchmark 1: ./git.old rev-parse --parseopt -- --opt42 <input\n>     Time (mean ± σ):      22.2 ms ±   0.4 ms    [User: 16.6 ms, System: 5.6 ms]\n>     Range (min … max):    21.5 ms …  23.9 ms    127 runs\n>   \n>   Benchmark 2: ./git.new rev-parse --parseopt -- --opt42 <input\n>     Time (mean ± σ):      32.5 ms ±   0.5 ms    [User: 23.8 ms, System: 8.6 ms]\n>     Range (min … max):    31.7 ms …  34.8 ms    89 runs\n>   \n>   Summary\n>     ./git.old rev-parse --parseopt -- --opt42 <input ran\n>       1.46 ± 0.03 times faster than ./git.new rev-parse --parseopt -- --opt42 <input\n> \n> So it is measurable (even with the extra per-option costs to generate\n> the option structs in the first place). Looks like on the order of 10ms\n> for 100k options, or about 100ns per option. If you imagine that most\n> option lists are smaller than 100, we're talking about probably the\n> equivalent of 50-100 syscalls. If we are really looking to\n> micro-optimize startup time, I suspect there's pretty low-hanging fruit\n> to be found of that magnitude.\n\nInteresting.  I don't like this percentage.  We won't have that many\noptions, ever, but we'd pay that small cost on every git invocation,\nwhich add up.  The beneficiaries are just a handful of developers who\nduplicate options, which seems like a bad deal.\n\n>>> +\t\tif (opts->long_name) {\n>>> +\t\t\tif (strset_contains(&long_names, opts->long_name))\n>>> +\t\t\t\toptbug(opts, \"long name already used\");\n>>> +\t\t\tstrset_add(&long_names, opts->long_name);\n>>> +\t\t}\n>>\n>> ...if you want to micro-optimize, note that the return value of\n>> strset_add() tells you whether the item was already in the set. That can\n>> save one hash of the string.\n\nMakes sense, good call.\n\n>> Probably the allocation for each element is the dominating cost, though,\n>> and it doesn't help with that.\n\nMy knee-jerk reaction is to use a fixed-size array and sort.  Gets rid\nof allocations, needs some more CPU cycles and memory accesses.  That\nwould then either bug out on experiments like yours or detect\nduplicates only in the first N long name options.  Not sure if it's\nworth the limitations.\n\nRené\n\n"},{"id":"537384","messageId":"6b674316-9a6e-4f57-b32c-f1824869ba7e@web.de","threadId":"65084","inReplyTo":"7693799a-91a2-480a-ae3e-29f8eed5b55a@web.de","subject":"[PATCH v2 2/2] parseopt: check for duplicate long names and numerical options","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2026-02-28T09:19:16Z","receivedAt":"2026-02-28T09:19:21Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"We already check for duplicate short names.  Check for and report\nduplicate long names and numerical options as well.\n\nPerform the slightly expensive string duplicate check only when showing\nthe usage to keep the cost of normal invocations low.  t0012-help.sh\ncovers it.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nChanges since v1:\n- Removed strset_contains() call.\n- Only check for duplicate long names when showing the usage.\n\n parse-options.c | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex c9cafc21b9..0214c106d4 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -5,6 +5,7 @@\n #include \"gettext.h\"\n #include \"strbuf.h\"\n #include \"string-list.h\"\n+#include \"strmap.h\"\n #include \"utf8.h\"\n \n static int disallow_abbreviated_options;\n@@ -641,6 +642,7 @@ static void check_typos(const char *arg, const struct option *options)\n static void parse_options_check(const struct option *opts)\n {\n \tchar short_opts[128];\n+\tbool saw_number_option = false;\n \tvoid *subcommand_value = NULL;\n \n \tmemset(short_opts, '\\0', sizeof(short_opts));\n@@ -655,6 +657,11 @@ static void parse_options_check(const struct option *opts)\n \t\t\telse if (short_opts[opts->short_name]++)\n \t\t\t\toptbug(opts, \"short name already used\");\n \t\t}\n+\t\tif (opts->type == OPTION_NUMBER) {\n+\t\t\tif (saw_number_option)\n+\t\t\t\toptbug(opts, \"duplicate numerical option\");\n+\t\t\tsaw_number_option = true;\n+\t\t}\n \t\tif (opts->flags & PARSE_OPT_NODASH &&\n \t\t    ((opts->flags & PARSE_OPT_OPTARG) ||\n \t\t     !(opts->flags & PARSE_OPT_NOARG) ||\n@@ -714,6 +721,19 @@ static void parse_options_check(const struct option *opts)\n \tBUG_if_bug(\"invalid 'struct option'\");\n }\n \n+static void parse_options_check_harder(const struct option *opts)\n+{\n+\tstruct strset long_names = STRSET_INIT;\n+\tfor (; opts->type != OPTION_END; opts++) {\n+\t\tif (opts->long_name) {\n+\t\t\tif (!strset_add(&long_names, opts->long_name))\n+\t\t\t\toptbug(opts, \"long name already used\");\n+\t\t}\n+\t}\n+\tBUG_if_bug(\"invalid 'struct option'\");\n+\tstrset_clear(&long_names);\n+}\n+\n static int has_subcommands(const struct option *options)\n {\n \tfor (; options->type != OPTION_END; options++)\n@@ -1339,6 +1359,8 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n \tconst char *prefix = usage_prefix;\n \tint saw_empty_line = 0;\n \n+\tparse_options_check_harder(opts);\n+\n \tif (!usagestr)\n \t\treturn PARSE_OPT_HELP;\n \n-- \n2.53.0\n"},{"id":"537389","messageId":"20260228105849.GA3626520@coredump.intra.peff.net","threadId":"65084","inReplyTo":"6b674316-9a6e-4f57-b32c-f1824869ba7e@web.de","subject":"Re: [PATCH v2 2/2] parseopt: check for duplicate long names and numerical options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-28T10:58:49Z","receivedAt":"2026-02-28T10:58:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 28, 2026 at 10:19:16AM +0100, René Scharfe wrote:\n\n> Perform the slightly expensive string duplicate check only when showing\n> the usage to keep the cost of normal invocations low.  t0012-help.sh\n> covers it.\n\nNice, this seems like the perfect compromise to me. We get a runtime\nswitch that kicks in at the moment we want, and we don't even have to\npollute the world with a new switch or environment variable.\n\n> +static void parse_options_check_harder(const struct option *opts)\n> +{\n> +\tstruct strset long_names = STRSET_INIT;\n> +\tfor (; opts->type != OPTION_END; opts++) {\n> +\t\tif (opts->long_name) {\n> +\t\t\tif (!strset_add(&long_names, opts->long_name))\n> +\t\t\t\toptbug(opts, \"long name already used\");\n> +\t\t}\n> +\t}\n> +\tBUG_if_bug(\"invalid 'struct option'\");\n> +\tstrset_clear(&long_names);\n> +}\n\nI confirmed on my silly pathological case that invoking rev-parse with a\nreal option shows no slowdown, and we now pay the same 10ms cost to show\n\"-h\".\n\nYour other email made me wonder how the sorted-array solution might\nperform (patch below). It shaves off 2ms of those 10. Probably not worth\ncaring about for \"-h\" output (which is already spending another 5-10ms\nto generate the output, versus a normal parse).\n\n-Peff\n\n-- >8 --\ndiff --git a/parse-options.c b/parse-options.c\nindex 0214c106d4..1ea7efd5a3 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -721,17 +721,39 @@ static void parse_options_check(const struct option *opts)\n \tBUG_if_bug(\"invalid 'struct option'\");\n }\n \n+static int qsort_strcmp(const void *va, const void *vb)\n+{\n+\tconst char *a = *(const char **)va;\n+\tconst char *b = *(const char **)vb;\n+\treturn strcmp(a, b);\n+}\n+\n static void parse_options_check_harder(const struct option *opts)\n {\n-\tstruct strset long_names = STRSET_INIT;\n-\tfor (; opts->type != OPTION_END; opts++) {\n-\t\tif (opts->long_name) {\n-\t\t\tif (!strset_add(&long_names, opts->long_name))\n-\t\t\t\toptbug(opts, \"long name already used\");\n-\t\t}\n+\tconst struct option *p;\n+\tconst char **long_names;\n+\tsize_t i, len;\n+\n+\tlen = 0;\n+\tfor (p = opts; p->type != OPTION_END; p++) {\n+\t\tif (p->long_name)\n+\t\t\tlen++;\n \t}\n-\tBUG_if_bug(\"invalid 'struct option'\");\n-\tstrset_clear(&long_names);\n+\n+\tALLOC_ARRAY(long_names, len);\n+\ti = 0;\n+\tfor (p = opts; p->type != OPTION_END; p++) {\n+\t\tif (p->long_name)\n+\t\t\tlong_names[i++] = p->long_name;\n+\t}\n+\n+\tQSORT(long_names, len, qsort_strcmp);\n+\tfor (i = 1; i < len; i++) {\n+\t\tif (!strcmp(long_names[i], long_names[i-1]))\n+\t\t\tBUG(\"long name %s used twice\", long_names[i]);\n+\t}\n+\n+\tfree(long_names);\n }\n \n static int has_subcommands(const struct option *options)\n"},{"id":"537391","messageId":"7c221132-c2ac-4c6f-9d89-72677a74beb5@web.de","threadId":"65084","inReplyTo":"20260228105849.GA3626520@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] parseopt: check for duplicate long names and numerical options","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2026-02-28T11:28:39Z","receivedAt":"2026-02-28T11:28:47Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 2/28/26 11:58 AM, Jeff King wrote:\n> On Sat, Feb 28, 2026 at 10:19:16AM +0100, René Scharfe wrote:\n> \n>> +static void parse_options_check_harder(const struct option *opts)\n>> +{\n>> +\tstruct strset long_names = STRSET_INIT;\n>> +\tfor (; opts->type != OPTION_END; opts++) {\n>> +\t\tif (opts->long_name) {\n>> +\t\t\tif (!strset_add(&long_names, opts->long_name))\n>> +\t\t\t\toptbug(opts, \"long name already used\");\n>> +\t\t}\n>> +\t}\n>> +\tBUG_if_bug(\"invalid 'struct option'\");\n>> +\tstrset_clear(&long_names);\n>> +}\n> \n> I confirmed on my silly pathological case that invoking rev-parse with a\n> real option shows no slowdown, and we now pay the same 10ms cost to show\n> \"-h\".\n> \n> Your other email made me wonder how the sorted-array solution might\n> perform (patch below). It shaves off 2ms of those 10. Probably not worth\n> caring about for \"-h\" output (which is already spending another 5-10ms\n> to generate the output, versus a normal parse).\nCurious; sorting performs worse on my machine (Apple M1, 1 is 2cc719175,\n2 is patch 2 v2, 3 is your patch on top):\n\nBenchmark 1: ./git_main rev-parse --parseopt -- -h <input\n  Time (mean ± σ):      77.5 ms ±   0.4 ms    [User: 73.1 ms, System: 3.5 ms]\n  Range (min … max):    76.8 ms …  78.5 ms    37 runs\n\n  Warning: Ignoring non-zero exit code.\n\nBenchmark 2: ./git_strset rev-parse --parseopt -- -h <input\n  Time (mean ± σ):      82.6 ms ±   0.3 ms    [User: 77.7 ms, System: 3.9 ms]\n  Range (min … max):    82.1 ms …  83.7 ms    34 runs\n\n  Warning: Ignoring non-zero exit code.\n\nBenchmark 3: ./git_qsort rev-parse --parseopt -- -h <input\n  Time (mean ± σ):      85.6 ms ±   0.2 ms    [User: 81.2 ms, System: 3.5 ms]\n  Range (min … max):    85.3 ms …  86.5 ms    33 runs\n\n  Warning: Ignoring non-zero exit code.\n\nSummary\n  ./git_main rev-parse --parseopt -- -h <input ran\n    1.07 ± 0.01 times faster than ./git_strset rev-parse --parseopt -- -h <input\n    1.10 ± 0.01 times faster than ./git_qsort rev-parse --parseopt -- -h <input\n\n"},{"id":"537444","messageId":"xmqqldgb62n6.fsf@gitster.g","threadId":"65084","inReplyTo":"20260228105849.GA3626520@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] parseopt: check for duplicate long names and numerical options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-01T14:33:01Z","receivedAt":"2026-03-01T14:33:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sat, Feb 28, 2026 at 10:19:16AM +0100, René Scharfe wrote:\n>\n>> Perform the slightly expensive string duplicate check only when showing\n>> the usage to keep the cost of normal invocations low.  t0012-help.sh\n>> covers it.\n>\n> Nice, this seems like the perfect compromise to me. We get a runtime\n> switch that kicks in at the moment we want, and we don't even have to\n> pollute the world with a new switch or environment variable.\n\nYes, absolutely.  This is very nice.\n"},{"id":"537575","messageId":"20260302182402.GH28275@coredump.intra.peff.net","threadId":"65084","inReplyTo":"7c221132-c2ac-4c6f-9d89-72677a74beb5@web.de","subject":"Re: [PATCH v2 2/2] parseopt: check for duplicate long names and numerical options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-02T18:24:02Z","receivedAt":"2026-03-02T18:24:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 28, 2026 at 12:28:39PM +0100, René Scharfe wrote:\n\n> > Your other email made me wonder how the sorted-array solution might\n> > perform (patch below). It shaves off 2ms of those 10. Probably not worth\n> > caring about for \"-h\" output (which is already spending another 5-10ms\n> > to generate the output, versus a normal parse).\n> Curious; sorting performs worse on my machine (Apple M1, 1 is 2cc719175,\n> 2 is patch 2 v2, 3 is your patch on top):\n\nInteresting. Different architectures, I guess (mine's an i9). It makes\nme feel better about not trying to micro-optimize the last couple\nnanoseconds, though. ;)\n\n> Benchmark 1: ./git_main rev-parse --parseopt -- -h <input\n>   Time (mean ± σ):      77.5 ms ±   0.4 ms    [User: 73.1 ms, System: 3.5 ms]\n>   Range (min … max):    76.8 ms …  78.5 ms    37 runs\n> \n>   Warning: Ignoring non-zero exit code.\n> \n> Benchmark 2: ./git_strset rev-parse --parseopt -- -h <input\n>   Time (mean ± σ):      82.6 ms ±   0.3 ms    [User: 77.7 ms, System: 3.9 ms]\n>   Range (min … max):    82.1 ms …  83.7 ms    34 runs\n> \n>   Warning: Ignoring non-zero exit code.\n\nInteresting that your absolute times are much higher than mine (by a\nfactor of 4), but the absolute cost of the strset addition is smaller.\nI'm not sure if that's another architecture difference, or maybe just\nthe other unrelated parts of the process startup are more expensive on\nmacOS (syscalls, filesystem access, etc).\n\nAnyway, now that it is only used for \"-h\" I don't think we need to care\nthat much.\n\n-Peff\n"}]}