Re: [PATCH v2 4/4] object-file: fix a unpack_loose_header() regression in 3b6a8db3b03
- From
Derrick Stolee <derrickstolee@github.com>
- Date
- May 16, 2022, 14:59 UTC
- Message-ID
- <377be0e9-8a0f-4a86-0a66-3b08c0284dae@github.com>
- In-Reply-To
- <xmqqo802codb.fsf@gitster.g>
On 5/12/2022 7:39 PM, Junio C Hamano wrote:
Show 45 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> diff --git a/object-file.c b/object-file.c
>> index 5ffbf3d4fd..b5d1d12b68 100644
>> --- a/object-file.c
>> +++ b/object-file.c
>> @@ -2623,8 +2623,12 @@ int read_loose_object(const char *path,
>> goto out;
>> }
>>
>> - 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;
>> }
>
> Regarding this hunk, since we only care about a single "did we get
> any error, or did we unpack OK" bit, I think this should be more
> like
>
> if (unpack_loose_header(...) != ULHR_OK) {
> error(_("unable to..."), path);
> goto out;
> }
>
> It is true, as Ævar mentioned, that there is another place in the
> same file that uses switch() in loose_object_info(), and it should
> remain to be switch() on the returned enum because it wants to
> behave differnetly depending on the kind of error it gets. But that
> is not a reason to make this part that only cares about a single
> "did it fail?" into a switch and force future developers to add a
> useless case arm.
>
> I left it there as posted in the previous round because I was too
> lazy ;-) and also it is something we can clean up with a follow up
> patch outside the series. As my today's focus has been to reduce
> the number of topics waiting for a reroll, I'd rather leave things
> that are not outright broken but needs clean up as they are for the
> sake of expediency.Taking a look at your new version, I agree that this use of 'switch' is out of place and can make things more confusing in the future.
Here is a patch doing exactly what you recommended, which you can choose to add or squash. I made you co-author, but I expect you to add your sign-off after mine.
-- >8 --
From 85cd37b4f23e06980ea95311067d735144fe932f Mon Sep 17 00:00:00 2001 From: Derrick Stolee <derrickstolee@github.com> Date: Mon, 16 May 2022 10:53:27 -0400 Subject: [PATCH] object-file: convert 'switch' back to 'if'
This switch statement was recently added to make it clear that unpack_loose_header() returns an enum value, not an int. This adds complications for future developers if that enum gains new values, since that developer would need to add a case statement to this switch for little real value.
Instead, we can revert back to an 'if' statement, but make the enum explicit by using "!= ULHR_OK" instead of assuming it has the numerical value zero.
Co-authored-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Derrick Stolee <derrickstolee@github.com> ---
object-file.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/object-file.c b/object-file.c index b5d1d12b68a..52e4ae1b5f0 100644 --- a/object-file.c +++ b/object-file.c @@ -2623,12 +2623,8 @@ int read_loose_object(const char *path, goto out; } - switch (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr), - NULL)) { - case ULHR_OK: - break; - case ULHR_BAD: - case ULHR_TOO_LONG: + if (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr), + NULL) != ULHR_OK) { error(_("unable to unpack header of %s"), path); goto out; }
-- 2.35.3.vfs.0.0