Re: [PATCH v2 2/2] parseopt: check for duplicate long names and numerical options
- From
Jeff King <peff@peff.net>
- Date
- Feb 28, 2026, 10:58 UTC
- Message-ID
- <20260228105849.GA3626520@coredump.intra.peff.net>
- In-Reply-To
- <6b674316-9a6e-4f57-b32c-f1824869ba7e@web.de>
On Sat, Feb 28, 2026 at 10:19:16AM +0100, René Scharfe wrote:
> Perform the slightly expensive string duplicate check only when showing > the usage to keep the cost of normal invocations low. t0012-help.sh > covers it.
Nice, this seems like the perfect compromise to me. We get a runtime switch that kicks in at the moment we want, and we don't even have to pollute the world with a new switch or environment variable.
Show 12 quoted lines
> +static void parse_options_check_harder(const struct option *opts)
> +{
> + struct strset long_names = STRSET_INIT;
> + for (; opts->type != OPTION_END; opts++) {
> + if (opts->long_name) {
> + if (!strset_add(&long_names, opts->long_name))
> + optbug(opts, "long name already used");
> + }
> + }
> + BUG_if_bug("invalid 'struct option'");
> + strset_clear(&long_names);
> +}I confirmed on my silly pathological case that invoking rev-parse with a real option shows no slowdown, and we now pay the same 10ms cost to show "-h".
Your other email made me wonder how the sorted-array solution might perform (patch below). It shaves off 2ms of those 10. Probably not worth caring about for "-h" output (which is already spending another 5-10ms to generate the output, versus a normal parse).
-Peff
-- >8 --
diff --git a/parse-options.c b/parse-options.c index 0214c106d4..1ea7efd5a3 100644 --- a/parse-options.c +++ b/parse-options.c @@ -721,17 +721,39 @@ static void parse_options_check(const struct option *opts) BUG_if_bug("invalid 'struct option'"); } +static int qsort_strcmp(const void *va, const void *vb) +{ + const char *a = *(const char **)va; + const char *b = *(const char **)vb; + return strcmp(a, b); +} + static void parse_options_check_harder(const struct option *opts) { - struct strset long_names = STRSET_INIT; - for (; opts->type != OPTION_END; opts++) { - if (opts->long_name) { - if (!strset_add(&long_names, opts->long_name)) - optbug(opts, "long name already used"); - } + const struct option *p; + const char **long_names; + size_t i, len; + + len = 0; + for (p = opts; p->type != OPTION_END; p++) { + if (p->long_name) + len++; } - BUG_if_bug("invalid 'struct option'"); - strset_clear(&long_names); + + ALLOC_ARRAY(long_names, len); + i = 0; + for (p = opts; p->type != OPTION_END; p++) { + if (p->long_name) + long_names[i++] = p->long_name; + } + + QSORT(long_names, len, qsort_strcmp); + for (i = 1; i < len; i++) { + if (!strcmp(long_names[i], long_names[i-1])) + BUG("long name %s used twice", long_names[i]); + } + + free(long_names); } static int has_subcommands(const struct option *options)