From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Derrick Stolee <derrickstolee@github.com>
Cc: Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org
Subject: Re: [PATCH] revision: mark blobs needed for resolve-undo as reachable
Date: Tue, 14 Jun 2022 02:24:12 +0200 [thread overview]
Message-ID: <220614.86czfcytlz.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <7cd41846-e6ef-7a24-0426-6031a529360f@github.com>
On Mon, Jun 13 2022, Derrick Stolee wrote:
> On 6/9/2022 7:44 PM, Junio C Hamano wrote:
>
>> + struct string_list *resolve_undo = istate->resolve_undo;
>> +
>> + if (!resolve_undo)
>> + return 0;
>> +
>> + for_each_string_list_item(item, resolve_undo) {
>
> I see this is necessary since for_each_string_list_item() does
> not handle NULL lists. After attempting to allow it to handle
> NULL lists, I see that the compiler complains about the cases
> where it would _never_ be NULL, so that change appears to be
> impossible.
>
> The patch looks good. I liked the comments for the three phases
> of the test.
I think it's probably good to keep for_each_string_list_item()
implemented the way it is, given that all existing callers of it feed
non-NULL lists to it.
But why is it impossible to make it handle NULL lists? This works for
me, and passes the tests:
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 4b17ccc3f44..4160bb50ad0 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -763,9 +763,6 @@ static int fsck_resolve_undo(struct index_state *istate)
struct string_list_item *item;
struct string_list *resolve_undo = istate->resolve_undo;
- if (!resolve_undo)
- return 0;
-
for_each_string_list_item(item, resolve_undo) {
const char *path = item->string;
struct resolve_undo_info *ru = item->util;
diff --git a/string-list.h b/string-list.h
index d5a744e1438..aa15aea8c2b 100644
--- a/string-list.h
+++ b/string-list.h
@@ -143,7 +143,7 @@ int for_each_string_list(struct string_list *list,
/** Iterate over each item, as a macro. */
#define for_each_string_list_item(item,list) \
- for (item = (list)->items; \
+ for (item = (((list) && (list)->items) ? ((list)->items) : NULL); \
item && item < (list)->items + (list)->nr; \
++item)
Perhaps you tried to convert it to a do/while macro with an "if", which
won't work as we need the "for" to be used for the subsequent {} (or a
single statement)..
But under the hood this is all just syntax sugar for "goto", so if we
can avoid dereferencing "list" we're good to go...
next prev parent reply other threads:[~2022-06-14 0:30 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-09 23:44 [PATCH] revision: mark blobs needed for resolve-undo as reachable Junio C Hamano
2022-06-13 15:15 ` Derrick Stolee
2022-06-13 20:11 ` Junio C Hamano
2022-06-14 0:24 ` Ævar Arnfjörð Bjarmason [this message]
2022-06-14 14:35 ` Derrick Stolee
2022-06-15 2:02 ` Taylor Blau
2022-06-15 3:48 ` Jeff King
2022-06-15 20:47 ` Taylor Blau
2022-06-15 17:11 ` Junio C Hamano
2022-06-16 14:10 ` Ævar Arnfjörð Bjarmason
2022-06-14 2:49 ` Taylor Blau
2022-07-11 8:19 ` fsck segfault (was: Re: [PATCH] revision: mark blobs needed for resolve-undo as reachable) SZEDER Gábor
2022-07-11 19:39 ` fsck segfault Junio C Hamano
2022-07-11 23:25 ` [PATCH 2/1] fsck: do not dereference NULL while checking resolve-undo data 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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=220614.86czfcytlz.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=derrickstolee@github.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.
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 NNTP newsgroup(s).