From: Jeff King <peff@peff.net>
Date: Sat, 28 Feb 2026 10:58:49 GMT
Subject: Re: [PATCH v2 2/2] parseopt: check for duplicate long names and numerical options
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.

> +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)