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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 21, 2022, 22:39 UTC
Message-ID: <xmqqa6cedpt3.fsf@gitster.g>
In-Reply-To: <patch-4.4-7698c0f11a8-20220421T200733Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 11 quoted lines

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