Re: [PATCH v5 2/2] fetch: add fetch.submoduleErrors to make submodule fetch errors non-fatal

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 15, 2026, 19:13 UTC
Message-ID: <xmqq7bmwm5g6.fsf@gitster.g>
In-Reply-To: <20260715103518.526326-3-paulius.zaleckas@gmail.com>

Paulius Zaleckas <paulius.zaleckas@gmail.com> writes:

Show 13 quoted lines

> +/* really private - use accessors below to parse and format */
> +static const char *submodule_errors_names[] = {
> +	[SUBMODULE_ERRORS_FAIL] = "fail",
> +	[SUBMODULE_ERRORS_WARN] = "warn",
> +};
> +
> +static const char *submodule_errors_to_string(int mode)
> +{
> +	if (mode < 0 || (size_t)mode >= ARRAY_SIZE(submodule_errors_names))
> +		BUG("invalid submodule errors mode %d", mode);
> +	return submodule_errors_names[mode];
> +}
> +

I am ranting here, and it is not entirely your fault, but I have to mention that this is the kind of bad code that "-Wsign-compare" forces on us. We know that 'mode' is a small integer used to index into the submodule_errors_names[] array. Theoretically, an array might contain as many elements as (size_t)(-1), but we know nobody needs to feed us a number that does not fit in a platform-natural "int".

	Side note: submodule_errors_names[] is a horrible name.
	It should be submodule_error_name[].  Look for "Array names"
	in the CodingGuidelines document.

Working around "-Wsign-compare" has forced an unnecessary cast on us here. If anything, we could have just done:

	static const char *submodule_errors_to_string(unsigned mode)

and

	if (ARRAY_SIZE(submodule_error_names) <= mode)
		BUG(...);

which would have been vastly more readable. To me, a plain "int" is also fine, but if we must squelch "-Wsign-compare", using "unsigned" is much saner than turning everything into "size_t".

Show 9 quoted lines

> +static int parse_submodule_errors(const char *name)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < ARRAY_SIZE(submodule_errors_names); i++)
> +		if (!strcmp(submodule_errors_names[i], name))
> +			return i;
> +	return -1;
> +}

And there is no sensible way to justify "size_t i" here. Using a platform-natural "unsigned" would have been much easier to understand.

It is a disease to bend our code only to appease the compiler's warnings; we should resist such temptation.

Also worth reading:

https://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/

Thanks.