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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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

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

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