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

[Junio C Hamano <gitster@pobox.com>]

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

> +/* 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".

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