Re: [PATCH 4/4] dir.c: avoid "exceeds maximum object size" error with GCC v12.x

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

Derrick Stolee <derrickstolee@github.com> writes:

> On 5/24/2022 5:05 PM, Johannes Schindelin wrote:> On Tue, 24 May 2022, Ævar Arnfjörð Bjarmason wrote:
>>> On Tue, May 24 2022, Johannes Schindelin via GitGitGadget wrote:
>>>> +	/*
>>>> +	 * It should not be possible to overflow `ptrdiff_t` by passing in an
>>>> +	 * insanely long URL, but GCC does not know that and will complain
>>>> +	 * without this check.
>>>> +	 */
>>>> +	if (end - start < 0)
>>>> +		die(_("No directory name could be guessed.\n"
>>>
>>> This should start with a lower-case letter, see CodingGuidelines.
>> 
>> This message is copied from existing code later in the same function.
>> Since it is a translateable message, I do not want to edit it because that
>> would cause unnecessary work of the translators. Especially given that we
>> do not even expect this message to be shown, ever, but we only add this
>> hunk for GCC's benefit.
>
> Perhaps this should be a BUG() statement, then? Without any
> translation?

Yeah, both are good.  If somehow the caller managed to pass such a
long URL then it can be considered a data error at runtime, and not
that the user detected a bug in our code, so in that sense die()
would be appropriate.  It is like xmalloc() running out of memory.

On the other hand, the "should not be possible to overflow" in the
comment implicitly assumes that it is impossible to pass insanely
long URL to trigger the condition from places we think of offhand,
like the command line, where the input is limited to a much shorter
string.  As "we detected a situation that should not happen unless
there is a programming or design bug" is what BUG() means, it is
also good here---our assumption that this should not be possible
turned out to be faulty, so we noticed a design bug.

I wonder if we can add a separate macro to add more to the
documentation value, though.  With something like

    #define FALSE_WARNING(expression, message) \
	do { if (expression) { BUG(message); } while (0)

the above would just become

	FALSE_WARNING(end - start < 0, "ptrdiff_t would not overflow here");

without a need for a big comment before it.  We might even be able
to optimize it out when building with compilers that do not need the
workaround.