From: Derrick Stolee <derrickstolee@github.com>
To: Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH v2 4/4] object-file: fix a unpack_loose_header() regression in 3b6a8db3b03
Date: Mon, 16 May 2022 10:59:27 -0400 [thread overview]
Message-ID: <377be0e9-8a0f-4a86-0a66-3b08c0284dae@github.com> (raw)
In-Reply-To: <xmqqo802codb.fsf@gitster.g>
On 5/12/2022 7:39 PM, Junio C Hamano wrote:
> 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
next prev parent reply other threads:[~2022-05-16 14:59 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-21 20:14 [PATCH 0/4] Fix issues and a regression noted by valgrind Ævar Arnfjörð Bjarmason
2022-04-21 20:14 ` [PATCH 1/4] tests: make RUNTIME_PREFIX compatible with --valgrind Ævar Arnfjörð Bjarmason
2022-04-21 22:22 ` Junio C Hamano
2022-04-21 20:14 ` [PATCH 2/4] log test: skip a failing mkstemp() test under valgrind Ævar Arnfjörð Bjarmason
2022-04-21 20:14 ` [PATCH 3/4] commit-graph.c: don't assume that stat() succeeds Ævar Arnfjörð Bjarmason
2022-04-21 22:29 ` Junio C Hamano
2022-04-21 20:14 ` [PATCH 4/4] object-file: fix a unpack_loose_header() regression in 3b6a8db3b03 Ævar Arnfjörð Bjarmason
2022-04-21 22:39 ` Junio C Hamano
2022-04-22 8:21 ` Ævar Arnfjörð Bjarmason
2022-05-12 22:32 ` [PATCH v2 0/4] test fixes around valgrind Junio C Hamano
2022-05-12 22:32 ` [PATCH v2 1/4] tests: using custom GIT_EXEC_PATH breaks --valgrind tests Junio C Hamano
2022-05-12 22:32 ` [PATCH v2 2/4] log test: skip a failing mkstemp() test under valgrind Junio C Hamano
2022-05-12 22:32 ` [PATCH v2 3/4] commit-graph.c: don't assume that stat() succeeds Junio C Hamano
2022-05-12 22:32 ` [PATCH v2 4/4] object-file: fix a unpack_loose_header() regression in 3b6a8db3b03 Junio C Hamano
2022-05-12 23:39 ` Junio C Hamano
2022-05-16 14:59 ` Derrick Stolee [this message]
2022-05-19 20:09 ` [RFC PATCH 0/2] Alternate ab/valgrind-fixes fix-up Ævar Arnfjörð Bjarmason
2022-05-19 20:09 ` [RFC PATCH 1/2] object-file API: fix obscure unpack_loose_header() return Ævar Arnfjörð Bjarmason
2022-05-19 20:09 ` [RFC PATCH 2/2] object-file API: have unpack_loose_header() return "int" again Ævar Arnfjörð Bjarmason
2022-05-20 4:27 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=377be0e9-8a0f-4a86-0a66-3b08c0284dae@github.com \
--to=derrickstolee@github.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).