On 5/12/2022 7:39 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> 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@xxxxxxxxxx>
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@xxxxxxxxx>
Signed-off-by: Derrick Stolee <derrickstolee@xxxxxxxxxx>
---
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
