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

From: Junio C Hamano <[email-masked]>
Date: Thu, 21 Apr 2022 15:39:20 -0700

Ævar Arnfjörð Bjarmason  <[email-masked]> 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.

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.