Re: [PATCH 4/4] object-file: fix a unpack_loose_header() regression in 3b6a8db3b03

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 22, 2022, 08:21 UTC
Message-ID: <220422.86levx5xuc.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqa6cedpt3.fsf@gitster.g>

On Thu, Apr 21 2022, Junio C Hamano wrote:

Show 17 quoted lines

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> -	if (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),
>> -				NULL) < 0) {
>> +	switch (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),
>> +				    NULL)) {
>> +	case ULHR_OK:
>> +		break;
>> +	case ULHR_BAD:
>> +	case ULHR_TOO_LONG:
>>  		error(_("unable to unpack header of %s"), path);
>>  		goto out;
>>  	}
>
> Sigh, well spotted.  This is why I hate the application of "enum is
> better, let's rewrite the 'negative is error, 0 is good' with it"
> and other dogmatic "clean-up" that touch everywhere in the codebase.

While this is squarely my fault, I'm FWIW not as dogmatic on that point as you think. I initially made a new error state a -2, and got feedback on the series that that was too magical, then ended up turning it into an enum and missed this callsite.

I think it's less that enums are bad in this case, as it's probably sensible to consistently use negative values for error states.

Show 10 quoted lines

> Now because it is ULHR_OK or everything else that is an error, I think
> the fix should be
>
> 	if (unpack_loose_header(...) != ULHR_OK) {
> 		error(...);
> 		goto out;
> 	}
>
> It would also be much closer in spirit to the original code before
> the "enum" change broke it.

We have two other callers of the API using the exhaustive enumeration pattern, so by doing this we'd have the compiler miss this callsite if another label is added.

It could be refactored etc., but I think this change as-is is the most minimal & least invasive. I.e. it just adjusts this one caller to match the other ones, we could also refactor the interface & pattern we use to call it.

But if we're doing that I don't see the benefit of doing it for just one caller, and if we're doing it for all of them surely that's better as some follow-up cleanup...