{"thread":{"id":"57978","subject":"[PATCH] revision: mark blobs needed for resolve-undo as reachable","startedAt":"2022-06-09T23:44:28Z","lastAt":"2022-07-11T23:25:31Z","messageCount":14,"participants":["Junio C Hamano","Derrick Stolee","Ævar Arnfjörð Bjarmason","Taylor Blau","Jeff King","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"456967","messageId":"xmqqfskdieqz.fsf@gitster.g","threadId":"57978","inReplyTo":null,"subject":"[PATCH] revision: mark blobs needed for resolve-undo as reachable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-09T23:44:20Z","receivedAt":"2022-06-09T23:44:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The resolve-undo extension was added to the index in cfc5789a\n(resolve-undo: record resolved conflicts in a new index extension\nsection, 2009-12-25).  This extension records the blob object names\nand their modes of conflicted paths when the path gets resolved\n(e.g. with \"git add\"), to allow \"undoing\" the resolution with\n\"checkout -m path\".  These blob objects should be guarded from\ngarbage-collection while we have the resolve-undo information in the\nindex (otherwise unresolve operation may try to use a blob object\nthat has already been pruned away).\n\nBut the code called from mark_reachable_objects() for the index\nforgets to do so.  Teach add_index_objects_to_pending() helper to\nalso add objects referred to by the resolve-undo extension.\n\nAlso make matching changes to \"fsck\", which has code that is fairly\nsimilar to the reachability stuff, but have parallel implementations\nfor all these stuff, which may (or may not) someday want to be unified.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/fsck.c            | 38 +++++++++++++++++++++\n revision.c                | 36 ++++++++++++++++++++\n t/t2030-unresolve-info.sh | 71 +++++++++++++++++++++++++++++++++++++++\n 3 files changed, 145 insertions(+)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 9e54892311..4b17ccc3f4 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -19,6 +19,7 @@\n #include \"decorate.h\"\n #include \"packfile.h\"\n #include \"object-store.h\"\n+#include \"resolve-undo.h\"\n #include \"run-command.h\"\n #include \"worktree.h\"\n \n@@ -757,6 +758,42 @@ static int fsck_cache_tree(struct cache_tree *it)\n \treturn err;\n }\n \n+static int fsck_resolve_undo(struct index_state *istate)\n+{\n+\tstruct string_list_item *item;\n+\tstruct string_list *resolve_undo = istate->resolve_undo;\n+\n+\tif (!resolve_undo)\n+\t\treturn 0;\n+\n+\tfor_each_string_list_item(item, resolve_undo) {\n+\t\tconst char *path = item->string;\n+\t\tstruct resolve_undo_info *ru = item->util;\n+\t\tint i;\n+\n+\t\tif (!ru)\n+\t\t\tcontinue;\n+\t\tfor (i = 0; i < 3; i++) {\n+\t\t\tstruct object *obj;\n+\n+\t\t\tif (!ru->mode[i] || !S_ISREG(ru->mode[i]))\n+\t\t\t\tcontinue;\n+\n+\t\t\tobj = parse_object(the_repository, &ru->oid[i]);\n+\t\t\tif (!obj) {\n+\t\t\t\terror(_(\"%s: invalid sha1 pointer in resolve-undo\"),\n+\t\t\t\t      oid_to_hex(&ru->oid[i]));\n+\t\t\t\terrors_found |= ERROR_REFS;\n+\t\t\t}\n+\t\t\tobj->flags |= USED;\n+\t\t\tfsck_put_object_name(&fsck_walk_options, &ru->oid[i],\n+\t\t\t\t\t     \":(%d):%s\", i, path);\n+\t\t\tmark_object_reachable(obj);\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n static void mark_object_for_connectivity(const struct object_id *oid)\n {\n \tstruct object *obj = lookup_unknown_object(the_repository, oid);\n@@ -938,6 +975,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tif (active_cache_tree)\n \t\t\tfsck_cache_tree(active_cache_tree);\n+\t\tfsck_resolve_undo(&the_index);\n \t}\n \n \tcheck_connectivity();\ndiff --git a/revision.c b/revision.c\nindex 7d435f8048..cd78627b20 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -33,6 +33,7 @@\n #include \"bloom.h\"\n #include \"json-writer.h\"\n #include \"list-objects-filter-options.h\"\n+#include \"resolve-undo.h\"\n \n volatile show_early_output_fn_t show_early_output;\n \n@@ -1690,6 +1691,39 @@ static void add_cache_tree(struct cache_tree *it, struct rev_info *revs,\n \n }\n \n+static void add_resolve_undo_to_pending(struct index_state *istate, struct rev_info *revs)\n+{\n+\tstruct string_list_item *item;\n+\tstruct string_list *resolve_undo = istate->resolve_undo;\n+\n+\tif (!resolve_undo)\n+\t\treturn;\n+\n+\tfor_each_string_list_item(item, resolve_undo) {\n+\t\tconst char *path = item->string;\n+\t\tstruct resolve_undo_info *ru = item->util;\n+\t\tint i;\n+\n+\t\tif (!ru)\n+\t\t\tcontinue;\n+\t\tfor (i = 0; i < 3; i++) {\n+\t\t\tstruct blob *blob;\n+\n+\t\t\tif (!ru->mode[i] || !S_ISREG(ru->mode[i]))\n+\t\t\t\tcontinue;\n+\n+\t\t\tblob = lookup_blob(revs->repo, &ru->oid[i]);\n+\t\t\tif (!blob) {\n+\t\t\t\twarning(_(\"resolve-undo records `%s` which is missing\"),\n+\t\t\t\t\toid_to_hex(&ru->oid[i]));\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tadd_pending_object_with_path(revs, &blob->object, \"\",\n+\t\t\t\t\t\t     ru->mode[i], path);\n+\t\t}\n+\t}\n+}\n+\n static void do_add_index_objects_to_pending(struct rev_info *revs,\n \t\t\t\t\t    struct index_state *istate,\n \t\t\t\t\t    unsigned int flags)\n@@ -1718,6 +1752,8 @@ static void do_add_index_objects_to_pending(struct rev_info *revs,\n \t\tadd_cache_tree(istate->cache_tree, revs, &path, flags);\n \t\tstrbuf_release(&path);\n \t}\n+\n+\tadd_resolve_undo_to_pending(istate, revs);\n }\n \n void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)\ndiff --git a/t/t2030-unresolve-info.sh b/t/t2030-unresolve-info.sh\nindex f691e6d903..2d8c70b03a 100755\n--- a/t/t2030-unresolve-info.sh\n+++ b/t/t2030-unresolve-info.sh\n@@ -194,4 +194,75 @@ test_expect_success 'rerere forget (add-add conflict)' '\n \ttest_i18ngrep \"no remembered\" actual\n '\n \n+test_expect_success 'resolve-undo keeps blobs from gc' '\n+\tgit checkout -f main &&\n+\n+\t# First make sure we do not have any cruft left in the object store\n+\tgit repack -a -d &&\n+\tgit prune --expire=now &&\n+\tgit prune-packed &&\n+\tgit gc --prune=now &&\n+\tgit fsck --unreachable >cruft &&\n+\ttest_must_be_empty cruft &&\n+\n+\t# Now add three otherwise unreferenced blob objects to the index\n+\tgit reset --hard &&\n+\tB1=$(echo \"resolve undo test data 1\" | git hash-object -w --stdin) &&\n+\tB2=$(echo \"resolve undo test data 2\" | git hash-object -w --stdin) &&\n+\tB3=$(echo \"resolve undo test data 3\" | git hash-object -w --stdin) &&\n+\tgit update-index --add --index-info <<-EOF &&\n+\t100644 $B1 1\tfrotz\n+\t100644 $B2 2\tfrotz\n+\t100644 $B3 3\tfrotz\n+\tEOF\n+\n+\t# These three blob objects are reachable (only) from the index\n+\tgit fsck --unreachable >cruft &&\n+\ttest_must_be_empty cruft &&\n+\t# and they should be protected from GC\n+\tgit gc --prune=now &&\n+\tgit cat-file -e $B1 &&\n+\tgit cat-file -e $B2 &&\n+\tgit cat-file -e $B3 &&\n+\n+\t# Now resolve the conflicted path\n+\tB0=$(echo \"resolve undo test data 0\" | git hash-object -w --stdin) &&\n+\tgit update-index --add --cacheinfo 100644,$B0,frotz &&\n+\n+\t# These three blob objects are now reachable only from the resolve-undo\n+\tgit fsck --unreachable >cruft &&\n+\ttest_must_be_empty cruft &&\n+\n+\t# and they should survive GC\n+\tgit gc --prune=now &&\n+\tgit cat-file -e $B0 &&\n+\tgit cat-file -e $B1 &&\n+\tgit cat-file -e $B2 &&\n+\tgit cat-file -e $B3 &&\n+\n+\t# Now we switch away, which nukes resolve-undo, and\n+\t# blobs B0..B3 would become dangling.  fsck should\n+\t# notice that they are now unreachable.\n+\tgit checkout -f side &&\n+\tgit fsck --unreachable >cruft &&\n+\tsort cruft >actual &&\n+\tsort <<-EOF >expect &&\n+\tunreachable blob $B0\n+\tunreachable blob $B1\n+\tunreachable blob $B2\n+\tunreachable blob $B3\n+\tEOF\n+\ttest_cmp expect actual &&\n+\n+\t# And they should go away when gc runs.\n+\tgit gc --prune=now &&\n+\tgit fsck --unreachable >cruft &&\n+\ttest_must_be_empty cruft &&\n+\n+\ttest_must_fail git cat-file -e $B0 &&\n+\ttest_must_fail git cat-file -e $B1 &&\n+\ttest_must_fail git cat-file -e $B2 &&\n+\ttest_must_fail git cat-file -e $B3\n+'\n+\n test_done\n-- \n2.36.1-502-gf5055183a0\n\n"},{"id":"457106","messageId":"7cd41846-e6ef-7a24-0426-6031a529360f@github.com","threadId":"57978","inReplyTo":"xmqqfskdieqz.fsf@gitster.g","subject":"Re: [PATCH] revision: mark blobs needed for resolve-undo as reachable","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-13T15:15:03Z","receivedAt":"2022-06-13T18:45:46Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/9/2022 7:44 PM, Junio C Hamano wrote:\n\n> +\tstruct string_list *resolve_undo = istate->resolve_undo;\n> +\n> +\tif (!resolve_undo)\n> +\t\treturn 0;\n> +\n> +\tfor_each_string_list_item(item, resolve_undo) {\n\nI see this is necessary since for_each_string_list_item() does\nnot handle NULL lists. After attempting to allow it to handle\nNULL lists, I see that the compiler complains about the cases\nwhere it would _never_ be NULL, so that change appears to be\nimpossible.\n \nThe patch looks good. I liked the comments for the three phases\nof the test.\n\nThanks,\n-Stolee\n"},{"id":"457120","messageId":"xmqqczfc5nob.fsf@gitster.g","threadId":"57978","inReplyTo":"7cd41846-e6ef-7a24-0426-6031a529360f@github.com","subject":"Re: [PATCH] revision: mark blobs needed for resolve-undo as reachable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-13T20:11:16Z","receivedAt":"2022-06-13T20:53:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <derrickstolee@github.com> writes:\n\n> On 6/9/2022 7:44 PM, Junio C Hamano wrote:\n>\n>> +\tstruct string_list *resolve_undo = istate->resolve_undo;\n>> +\n>> +\tif (!resolve_undo)\n>> +\t\treturn 0;\n>> +\n>> +\tfor_each_string_list_item(item, resolve_undo) {\n>\n> I see this is necessary since for_each_string_list_item() does not\n> handle NULL lists.  After attempting to allow it to handle NULL\n> lists, I see that the compiler complains about the cases where it\n> would _never_ be NULL, so that change appears to be impossible.\n\nHeh, no such deep thought went into this.  I just copied what is\ndone in builtin/ls-files.c::show_ru_info() that grabs these blobs\nand does something interesting and the only difference is this does\nsomething else interesting.\n\nWe _could_ refactor these to into \"take a callback and iterate over\nresolve-undo information\" shell plus two callback functions, one to\nprint them and the other to mark them still reachable, but the\niterator being relatively short, I doubt that it is worth it.\n\n> The patch looks good. I liked the comments for the three phases\n> of the test.\n\nThanks.\n"},{"id":"457156","messageId":"220614.86czfcytlz.gmgdl@evledraar.gmail.com","threadId":"57978","inReplyTo":"7cd41846-e6ef-7a24-0426-6031a529360f@github.com","subject":"Re: [PATCH] revision: mark blobs needed for resolve-undo as reachable","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-14T00:24:12Z","receivedAt":"2022-06-14T00:30:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 13 2022, Derrick Stolee wrote:\n\n> On 6/9/2022 7:44 PM, Junio C Hamano wrote:\n>\n>> +\tstruct string_list *resolve_undo = istate->resolve_undo;\n>> +\n>> +\tif (!resolve_undo)\n>> +\t\treturn 0;\n>> +\n>> +\tfor_each_string_list_item(item, resolve_undo) {\n>\n> I see this is necessary since for_each_string_list_item() does\n> not handle NULL lists. After attempting to allow it to handle\n> NULL lists, I see that the compiler complains about the cases\n> where it would _never_ be NULL, so that change appears to be\n> impossible.\n>  \n> The patch looks good. I liked the comments for the three phases\n> of the test.\n\nI think it's probably good to keep for_each_string_list_item()\nimplemented the way it is, given that all existing callers of it feed\nnon-NULL lists to it.\n\nBut why is it impossible to make it handle NULL lists? This works for\nme, and passes the tests:\n\t\n\tdiff --git a/builtin/fsck.c b/builtin/fsck.c\n\tindex 4b17ccc3f44..4160bb50ad0 100644\n\t--- a/builtin/fsck.c\n\t+++ b/builtin/fsck.c\n\t@@ -763,9 +763,6 @@ static int fsck_resolve_undo(struct index_state *istate)\n\t \tstruct string_list_item *item;\n\t \tstruct string_list *resolve_undo = istate->resolve_undo;\n\t \n\t-\tif (!resolve_undo)\n\t-\t\treturn 0;\n\t-\n\t \tfor_each_string_list_item(item, resolve_undo) {\n\t \t\tconst char *path = item->string;\n\t \t\tstruct resolve_undo_info *ru = item->util;\n\tdiff --git a/string-list.h b/string-list.h\n\tindex d5a744e1438..aa15aea8c2b 100644\n\t--- a/string-list.h\n\t+++ b/string-list.h\n\t@@ -143,7 +143,7 @@ int for_each_string_list(struct string_list *list,\n\t \n\t /** Iterate over each item, as a macro. */\n\t #define for_each_string_list_item(item,list)            \\\n\t-\tfor (item = (list)->items;                      \\\n\t+\tfor (item = (((list) && (list)->items) ? ((list)->items) : NULL); \\\n\t \t     item && item < (list)->items + (list)->nr; \\\n\t \t     ++item)\n\t \n\nPerhaps you tried to convert it to a do/while macro with an \"if\", which\nwon't work as we need the \"for\" to be used for the subsequent {} (or a\nsingle statement)..\n\nBut under the hood this is all just syntax sugar for \"goto\", so if we\ncan avoid dereferencing \"list\" we're good to go...\n"},{"id":"457167","messageId":"Yqf3OT9vSvWETVCJ@nand.local","threadId":"57978","inReplyTo":"xmqqfskdieqz.fsf@gitster.g","subject":"Re: [PATCH] revision: mark blobs needed for resolve-undo as reachable","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-14T02:49:29Z","receivedAt":"2022-06-14T02:58:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jun 09, 2022 at 04:44:20PM -0700, Junio C Hamano wrote:\n> The resolve-undo extension was added to the index in cfc5789a\n> (resolve-undo: record resolved conflicts in a new index extension\n> section, 2009-12-25).  This extension records the blob object names\n> and their modes of conflicted paths when the path gets resolved\n> (e.g. with \"git add\"), to allow \"undoing\" the resolution with\n> \"checkout -m path\".  These blob objects should be guarded from\n> garbage-collection while we have the resolve-undo information in the\n> index (otherwise unresolve operation may try to use a blob object\n> that has already been pruned away).\n>\n> But the code called from mark_reachable_objects() for the index\n> forgets to do so.  Teach add_index_objects_to_pending() helper to\n> also add objects referred to by the resolve-undo extension.\n\nNice find!\n\n> Also make matching changes to \"fsck\", which has code that is fairly\n> similar to the reachability stuff, but have parallel implementations\n> for all these stuff, which may (or may not) someday want to be unified.\n\nI wasn't sure what the change in fsck was when skimming the diffstat\nbefore reading your patch message, but makes sense. I'm glad that you\nincluded this, too.\n\n> +static void add_resolve_undo_to_pending(struct index_state *istate, struct rev_info *revs)\n> +{\n> +\tstruct string_list_item *item;\n> +\tstruct string_list *resolve_undo = istate->resolve_undo;\n> +\n> +\tif (!resolve_undo)\n> +\t\treturn;\n> +\n> +\tfor_each_string_list_item(item, resolve_undo) {\n> +\t\tconst char *path = item->string;\n> +\t\tstruct resolve_undo_info *ru = item->util;\n> +\t\tint i;\n> +\n> +\t\tif (!ru)\n> +\t\t\tcontinue;\n> +\t\tfor (i = 0; i < 3; i++) {\n> +\t\t\tstruct blob *blob;\n> +\n> +\t\t\tif (!ru->mode[i] || !S_ISREG(ru->mode[i]))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tblob = lookup_blob(revs->repo, &ru->oid[i]);\n> +\t\t\tif (!blob) {\n> +\t\t\t\twarning(_(\"resolve-undo records `%s` which is missing\"),\n> +\t\t\t\t\toid_to_hex(&ru->oid[i]));\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t\tadd_pending_object_with_path(revs, &blob->object, \"\",\n> +\t\t\t\t\t\t     ru->mode[i], path);\n> +\t\t}\n> +\t}\n> +}\n\nThis implementation looks good to my eyes.\n\n> @@ -1718,6 +1752,8 @@ static void do_add_index_objects_to_pending(struct rev_info *revs,\n>  \t\tadd_cache_tree(istate->cache_tree, revs, &path, flags);\n>  \t\tstrbuf_release(&path);\n>  \t}\n> +\n> +\tadd_resolve_undo_to_pending(istate, revs);\n>  }\n\nGreat; this fixes the bug for cruft packs, too, whose reachable pack is\ngenerated with `--indexed-objects`, so the cruft pack will no longer\ncontain the resolve-undo objects.\n\n> +test_expect_success 'resolve-undo keeps blobs from gc' '\n\nVery thorough. Thanks!\n\nTaylor\n"},{"id":"457192","messageId":"31f406b1-b4e8-5da2-40af-5747938de634@github.com","threadId":"57978","inReplyTo":"220614.86czfcytlz.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] revision: mark blobs needed for resolve-undo as reachable","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-14T14:35:10Z","receivedAt":"2022-06-14T14:35:15Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/13/2022 8:24 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Jun 13 2022, Derrick Stolee wrote:\n> \n>> On 6/9/2022 7:44 PM, Junio C Hamano wrote:\n>>\n>>> +\tstruct string_list *resolve_undo = istate->resolve_undo;\n>>> +\n>>> +\tif (!resolve_undo)\n>>> +\t\treturn 0;\n>>> +\n>>> +\tfor_each_string_list_item(item, resolve_undo) {\n>>\n>> I see this is necessary since for_each_string_list_item() does\n>> not handle NULL lists. After attempting to allow it to handle\n>> NULL lists, I see that the compiler complains about the cases\n>> where it would _never_ be NULL, so that change appears to be\n>> impossible.\n>>  \n>> The patch looks good. I liked the comments for the three phases\n>> of the test.\n> \n> I think it's probably good to keep for_each_string_list_item()\n> implemented the way it is, given that all existing callers of it feed\n> non-NULL lists to it.\n\nWe are talking right now about an example where it would be cleaner to\nallow a NULL value.\n\nThis guarded example also exists in http.c (we would still need to guard\non NULL options):\n\n\t/* Add additional headers here */\n\tif (options && options->extra_headers) {\n\t\tconst struct string_list_item *item;\n\t\tfor_each_string_list_item(item, options->extra_headers) {\n\t\t\theaders = curl_slist_append(headers, item->string);\n\t\t}\n\t}\n\nThese guarded examples in ref_filter_match() would be greatly simplified:\n\n\tif (exclude_patterns && exclude_patterns->nr) {\n\t\tfor_each_string_list_item(item, exclude_patterns) {\n\t\t\tif (match_ref_pattern(refname, item))\n\t\t\t\treturn 0;\n\t\t}\n\t}\n\n\tif (include_patterns && include_patterns->nr) {\n\t\tfor_each_string_list_item(item, include_patterns) {\n\t\t\tif (match_ref_pattern(refname, item))\n\t\t\t\treturn 1;\n\t\t}\n\t\treturn 0;\n\t}\n\n\tif (exclude_patterns_config && exclude_patterns_config->nr) {\n\t\tfor_each_string_list_item(item, exclude_patterns_config) {\n\t\t\tif (match_ref_pattern(refname, item))\n\t\t\t\treturn 0;\n\t\t}\n\t}\n\n(The include_patterns check would still be needed for that extra\nreturn 0; in the middle.)\n\nThere are more examples, but I'll stop listing them here.\n\n> But why is it impossible to make it handle NULL lists? This works for\n> me, and passes the tests:\n\n> \t /** Iterate over each item, as a macro. */\n> \t #define for_each_string_list_item(item,list)            \\\n> \t-\tfor (item = (list)->items;                      \\\n> \t+\tfor (item = (((list) && (list)->items) ? ((list)->items) : NULL); \\\n\nI thinks I had something like\n\n\tfor ((list) && item = (list)->items; (list) && item && ...\n\nbut even with your suggestion, I get this compiler error:\n\nIn file included from convert.h:8,\n                 from cache.h:10,\n                 from apply.c:10:\napply.c: In function ‘write_out_results’:\nstring-list.h:146:22: error: the address of ‘cpath’ will always evaluate as ‘true’ [-Werror=address]\n  146 |         for (item = ((list) && (list)->items) ? (list)->items : NULL;     \\\n      |                      ^\napply.c:4652:25: note: in expansion of macro ‘for_each_string_list_item’\n 4652 |                         for_each_string_list_item(item, &cpath)\n      |   \n\n(along with many other examples).\n\nJunio is right that we would need to convert this into a method with a\nfunction pointer instead of a for_each_* macro. That's quite a big lift\nfor some small convenience for the callers.\n\nThanks,\n-Stolee\n"},{"id":"457240","messageId":"Yqk9uMS5kxHD6o7l@nand.local","threadId":"57978","inReplyTo":"31f406b1-b4e8-5da2-40af-5747938de634@github.com","subject":"Re: [PATCH] revision: mark blobs needed for resolve-undo as reachable","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-15T02:02:32Z","receivedAt":"2022-06-15T02:02:54Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 14, 2022 at 10:35:10AM -0400, Derrick Stolee wrote:\n> I thinks I had something like\n>\n> \tfor ((list) && item = (list)->items; (list) && item && ...\n\nI wrote something like this:\n\n--- >8 ---\n\ndiff --git a/string-list.h b/string-list.h\nindex d5a744e143..425abc55f4 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -143,7 +143,7 @@ int for_each_string_list(struct string_list *list,\n\n /** Iterate over each item, as a macro. */\n #define for_each_string_list_item(item,list)            \\\n-\tfor (item = (list)->items;                      \\\n+\tfor (item = (list) ? (list)->items : NULL;      \\\n \t     item && item < (list)->items + (list)->nr; \\\n \t     ++item)\n\n--- 8< ---\n\n> but even with your suggestion, I get this compiler error:\n\n...so did I. Though I'm not sure I understand the compiler's warning\nhere. Surely the thing being passed as list in the macro expansion\n_won't_ always evaluate to non-NULL, will it?\n\nThanks,\nTaylor\n"},{"id":"457246","messageId":"YqlWhKDFX3KESY0h@coredump.intra.peff.net","threadId":"57978","inReplyTo":"Yqk9uMS5kxHD6o7l@nand.local","subject":"Re: [PATCH] revision: mark blobs needed for resolve-undo as reachable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-06-15T03:48:20Z","receivedAt":"2022-06-15T03:48:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 14, 2022 at 10:02:32PM -0400, Taylor Blau wrote:\n\n> --- >8 ---\n> \n> diff --git a/string-list.h b/string-list.h\n> index d5a744e143..425abc55f4 100644\n> --- a/string-list.h\n> +++ b/string-list.h\n> @@ -143,7 +143,7 @@ int for_each_string_list(struct string_list *list,\n> \n>  /** Iterate over each item, as a macro. */\n>  #define for_each_string_list_item(item,list)            \\\n> -\tfor (item = (list)->items;                      \\\n> +\tfor (item = (list) ? (list)->items : NULL;      \\\n>  \t     item && item < (list)->items + (list)->nr; \\\n>  \t     ++item)\n> \n> --- 8< ---\n> \n> > but even with your suggestion, I get this compiler error:\n> \n> ...so did I. Though I'm not sure I understand the compiler's warning\n> here. Surely the thing being passed as list in the macro expansion\n> _won't_ always evaluate to non-NULL, will it?\n\nIn the general case, no, but in this specific expansion of the macro, it\nis passing the address of a local variable (&cpath), which will never be\nNULL. The compiler is overeager here; the check is indeed pointless in\nthis expansion, but warning on useless macro-expanded code isn't\nhelpful, since other macro users need it.\n\nHiding it in a function seems to work, even with -O2 inlining, like:\n\ndiff --git a/string-list.h b/string-list.h\nindex d5a744e143..b28b135e11 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -141,9 +141,14 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c\n int for_each_string_list(struct string_list *list,\n \t\t\t string_list_each_func_t func, void *cb_data);\n \n+static inline struct string_list_item *string_list_first_item(const struct string_list *list)\n+{\n+\treturn list ? list->items : NULL;\n+}\n+\n /** Iterate over each item, as a macro. */\n #define for_each_string_list_item(item,list)            \\\n-\tfor (item = (list)->items;                      \\\n+\tfor (item = string_list_first_item(list);       \\\n \t     item && item < (list)->items + (list)->nr; \\\n \t     ++item)\n \n\n-Peff\n"},{"id":"457298","messageId":"xmqqedzp26oe.fsf@gitster.g","threadId":"57978","inReplyTo":"31f406b1-b4e8-5da2-40af-5747938de634@github.com","subject":"Re: [PATCH] revision: mark blobs needed for resolve-undo as reachable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-15T17:11:13Z","receivedAt":"2022-06-15T17:11:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <derrickstolee@github.com> writes:\n\n> Junio is right that we would need to convert this into a method with a\n> function pointer instead of a for_each_* macro. That's quite a big lift\n> for some small convenience for the callers.\n\nHeh, I never said we would need to.  I only said we _could_ do such\na change but I do not see the need to.\n\n"},{"id":"457316","messageId":"YqpFXoKPu2E1j+mr@nand.local","threadId":"57978","inReplyTo":"YqlWhKDFX3KESY0h@coredump.intra.peff.net","subject":"Re: [PATCH] revision: mark blobs needed for resolve-undo as reachable","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-15T20:47:26Z","receivedAt":"2022-06-15T20:47:33Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 14, 2022 at 11:48:20PM -0400, Jeff King wrote:\n> On Tue, Jun 14, 2022 at 10:02:32PM -0400, Taylor Blau wrote:\n>\n> > --- >8 ---\n> >\n> > diff --git a/string-list.h b/string-list.h\n> > index d5a744e143..425abc55f4 100644\n> > --- a/string-list.h\n> > +++ b/string-list.h\n> > @@ -143,7 +143,7 @@ int for_each_string_list(struct string_list *list,\n> >\n> >  /** Iterate over each item, as a macro. */\n> >  #define for_each_string_list_item(item,list)            \\\n> > -\tfor (item = (list)->items;                      \\\n> > +\tfor (item = (list) ? (list)->items : NULL;      \\\n> >  \t     item && item < (list)->items + (list)->nr; \\\n> >  \t     ++item)\n> >\n> > --- 8< ---\n> >\n> > > but even with your suggestion, I get this compiler error:\n> >\n> > ...so did I. Though I'm not sure I understand the compiler's warning\n> > here. Surely the thing being passed as list in the macro expansion\n> > _won't_ always evaluate to non-NULL, will it?\n>\n> In the general case, no, but in this specific expansion of the macro, it\n> is passing the address of a local variable (&cpath), which will never be\n> NULL. The compiler is overeager here; the check is indeed pointless in\n> this expansion, but warning on useless macro-expanded code isn't\n> helpful, since other macro users need it.\n\nAh, that makes sense. The compiler is warning us that the macro-expanded\nversion of for_each_string_list_item() has a ternary expression that\nwill never evaluate its right-hand side in cases where it can prove the\nsecond argument to the macro is non-NULL.\n\n> Hiding it in a function seems to work, even with -O2 inlining, like:\n>\n> diff --git a/string-list.h b/string-list.h\n> index d5a744e143..b28b135e11 100644\n> --- a/string-list.h\n> +++ b/string-list.h\n> @@ -141,9 +141,14 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c\n>  int for_each_string_list(struct string_list *list,\n>  \t\t\t string_list_each_func_t func, void *cb_data);\n>\n> +static inline struct string_list_item *string_list_first_item(const struct string_list *list)\n> +{\n> +\treturn list ? list->items : NULL;\n> +}\n> +\n>  /** Iterate over each item, as a macro. */\n>  #define for_each_string_list_item(item,list)            \\\n> -\tfor (item = (list)->items;                      \\\n> +\tfor (item = string_list_first_item(list);       \\\n>  \t     item && item < (list)->items + (list)->nr; \\\n>  \t     ++item)\n\nThat works, nice. I don't really want to mess up the tree too much this\nclose to a release, but this sort of clean-up seems good to do. I know\nStolee identified a handful of spots that would benefit from it. Some\ngood #leftoverbits, I guess :-).\n\nThanks,\nTaylor\n"},{"id":"457382","messageId":"220616.86bkuswuh5.gmgdl@evledraar.gmail.com","threadId":"57978","inReplyTo":"31f406b1-b4e8-5da2-40af-5747938de634@github.com","subject":"Re: [PATCH] revision: mark blobs needed for resolve-undo as reachable","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-16T14:10:12Z","receivedAt":"2022-06-16T14:31:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 14 2022, Derrick Stolee wrote:\n\n> On 6/13/2022 8:24 PM, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Mon, Jun 13 2022, Derrick Stolee wrote:\n>> \n>>> On 6/9/2022 7:44 PM, Junio C Hamano wrote:\n>>>\n>>>> +\tstruct string_list *resolve_undo = istate->resolve_undo;\n>>>> +\n>>>> +\tif (!resolve_undo)\n>>>> +\t\treturn 0;\n>>>> +\n>>>> +\tfor_each_string_list_item(item, resolve_undo) {\n>>>\n>>> I see this is necessary since for_each_string_list_item() does\n>>> not handle NULL lists. After attempting to allow it to handle\n>>> NULL lists, I see that the compiler complains about the cases\n>>> where it would _never_ be NULL, so that change appears to be\n>>> impossible.\n>>>  \n>>> The patch looks good. I liked the comments for the three phases\n>>> of the test.\n>> \n>> I think it's probably good to keep for_each_string_list_item()\n>> implemented the way it is, given that all existing callers of it feed\n>> non-NULL lists to it.\n>\n> We are talking right now about an example where it would be cleaner to\n> allow a NULL value.\n\nFirst, I've read the thread and I see the compile error you ran into\nbelow, and Jeff King's downthread workaround, so we could do this, and\nI'm not at all opposed...\n\n> This guarded example also exists in http.c (we would still need to guard\n> on NULL options):\n>\n> \t/* Add additional headers here */\n> \tif (options && options->extra_headers) {\n> \t\tconst struct string_list_item *item;\n> \t\tfor_each_string_list_item(item, options->extra_headers) {\n> \t\t\theaders = curl_slist_append(headers, item->string);\n> \t\t}\n> \t}\n\nI think this and probably many other examples you can find are ones\nwhere we should just stop using a maybe-NULL \"struct string_list\", as in\nthe WIP (but segfaulting) patch below, but I think you get the\nidea. I.e. in that case we're passing an \"options\" struct that can be\nNULL, but could just have an initializer.\n\nI.e. we should probably just have a non-NULL options with sensible\ndefaults, which would also allow for changing the adjacent \"options &&\noptions->no_cache\" etc. code to just \"options->no_cache\".\n\n> These guarded examples in ref_filter_match() would be greatly simplified:\n>\n> \tif (exclude_patterns && exclude_patterns->nr) {\n> \t\tfor_each_string_list_item(item, exclude_patterns) {\n> \t\t\tif (match_ref_pattern(refname, item))\n> \t\t\t\treturn 0;\n> \t\t}\n> \t}\n>\n> \tif (include_patterns && include_patterns->nr) {\n> \t\tfor_each_string_list_item(item, include_patterns) {\n> \t\t\tif (match_ref_pattern(refname, item))\n> \t\t\t\treturn 1;\n> \t\t}\n> \t\treturn 0;\n> \t}\n>\n> \tif (exclude_patterns_config && exclude_patterns_config->nr) {\n> \t\tfor_each_string_list_item(item, exclude_patterns_config) {\n> \t\t\tif (match_ref_pattern(refname, item))\n> \t\t\t\treturn 0;\n> \t\t}\n> \t}\n\nFirst, regardless of any bigger change, I think all of those except the\nmiddle one can drop the \"nr\" check.\n\nBut more generally, isn't something like [2] below a better change than\nchasing the case of \"what if it's NULL?\".\n\nVery WIP of course, and I just checked if it compiled, there's probably\nmore lurking bugs, but I think you get the idea.\n\nOne commit (of mine) on \"master\" that goes in that direction is\n0000e81811b (builtin/remote.c: add and use SHOW_INFO_INIT, 2021-10-01).\n\n> (The include_patterns check would still be needed for that extra\n> return 0; in the middle.)\n>\n> There are more examples, but I'll stop listing them here.\n\nMaybe there's better ones, but from my own past spelunking into \"struct\nstring_list\" users I've mostly found cases like 0000e81811b and the\nbelow.\n\nI.e. sure, handling NULL was a hassle, but the underlying problem was\nusually *why is it NULL*. I.e. can't we just have the list be 0..N, why\ndo we need NULL, 0..N?\n\n1.\n\ndiff --git a/http.c b/http.c\nindex 11c6f69facd..c2fa2b78db8 100644\n--- a/http.c\n+++ b/http.c\n@@ -1808,6 +1808,7 @@ static int http_request(const char *url,\n \tstruct strbuf buf = STRBUF_INIT;\n \tconst char *accept_language;\n \tint ret;\n+\tconst struct string_list_item *item;\n \n \tslot = get_active_slot();\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n@@ -1844,12 +1845,8 @@ static int http_request(const char *url,\n \theaders = curl_slist_append(headers, buf.buf);\n \n \t/* Add additional headers here */\n-\tif (options && options->extra_headers) {\n-\t\tconst struct string_list_item *item;\n-\t\tfor_each_string_list_item(item, options->extra_headers) {\n-\t\t\theaders = curl_slist_append(headers, item->string);\n-\t\t}\n-\t}\n+\tfor_each_string_list_item(item, &options->extra_headers)\n+\t\theaders = curl_slist_append(headers, item->string);\n \n \tcurl_easy_setopt(slot->curl, CURLOPT_URL, url);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n@@ -2055,7 +2052,7 @@ static char *fetch_pack_index(unsigned char *hash, const char *base_url)\n \tstrbuf_addf(&buf, \"%s.temp\", sha1_pack_index_name(hash));\n \ttmp = strbuf_detach(&buf, NULL);\n \n-\tif (http_get_file(url, tmp, NULL) != HTTP_OK) {\n+\tif (http_get_file(url, tmp, NULL /* segfaults due to this */) != HTTP_OK) {\n \t\terror(\"Unable to get pack index %s\", url);\n \t\tFREE_AND_NULL(tmp);\n \t}\ndiff --git a/http.h b/http.h\nindex ba303cfb372..41596fbf443 100644\n--- a/http.h\n+++ b/http.h\n@@ -144,7 +144,7 @@ struct http_get_options {\n \t * request. The strings in the list must not be freed until after the\n \t * request has completed.\n \t */\n-\tstruct string_list *extra_headers;\n+\tstruct string_list extra_headers;\n };\n \n /* Return values for http_get_*() */\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 67f178b1120..30235577487 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -449,7 +449,6 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \tstruct strbuf refs_url = STRBUF_INIT;\n \tstruct strbuf effective_url = STRBUF_INIT;\n \tstruct strbuf protocol_header = STRBUF_INIT;\n-\tstruct string_list extra_headers = STRING_LIST_INIT_DUP;\n \tstruct discovery *last = last_discovery;\n \tint http_ret, maybe_smart = 0;\n \tstruct http_get_options http_options;\n@@ -478,16 +477,18 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \tif (version == protocol_v2 && !strcmp(\"git-receive-pack\", service))\n \t\tversion = protocol_v0;\n \n+\t/* antipattern, we should have an initializer for \"struct http_get_options\"... */\n+\tmemset(&http_options, 0, sizeof(http_options));\n+\tstring_list_init_nodup(&http_options.extra_headers);\n+\n \t/* Add the extra Git-Protocol header */\n \tif (get_protocol_http_header(version, &protocol_header))\n-\t\tstring_list_append(&extra_headers, protocol_header.buf);\n+\t\tstring_list_append(&http_options.extra_headers, protocol_header.buf);\n \n-\tmemset(&http_options, 0, sizeof(http_options));\n \thttp_options.content_type = &type;\n \thttp_options.charset = &charset;\n \thttp_options.effective_url = &effective_url;\n \thttp_options.base_url = &url;\n-\thttp_options.extra_headers = &extra_headers;\n \thttp_options.initial_request = 1;\n \thttp_options.no_cache = 1;\n \n@@ -538,7 +539,8 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \tstrbuf_release(&effective_url);\n \tstrbuf_release(&buffer);\n \tstrbuf_release(&protocol_header);\n-\tstring_list_clear(&extra_headers, 0);\n+\t/*... and a proper destructor... */\n+\tstring_list_clear(&http_options.extra_headers, 0);\n \tlast_discovery = last;\n \treturn last;\n }\n\n2.\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 88a5e98875a..4af1503887e 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -168,12 +168,11 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \tstruct userformat_want w;\n \tint quiet = 0, source = 0, mailmap;\n \tstatic struct line_opt_callback_data line_cb = {NULL, NULL, STRING_LIST_INIT_DUP};\n-\tstatic struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;\n-\tstatic struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;\n-\tstatic struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;\n-\tstruct decoration_filter decoration_filter = {&decorate_refs_include,\n-\t\t\t\t\t\t      &decorate_refs_exclude,\n-\t\t\t\t\t\t      &decorate_refs_exclude_config};\n+\tstruct decoration_filter decoration_filter = {\n+\t\t.include_ref_pattern = STRING_LIST_INIT_NODUP,\n+\t\t.exclude_ref_pattern = STRING_LIST_INIT_NODUP,\n+\t\t.exclude_ref_config_pattern = STRING_LIST_INIT_NODUP,\n+\t};\n \tstatic struct revision_sources revision_sources;\n \n \tconst struct option builtin_log_options[] = {\n@@ -181,9 +180,9 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \t\tOPT_BOOL(0, \"source\", &source, N_(\"show source\")),\n \t\tOPT_BOOL(0, \"use-mailmap\", &mailmap, N_(\"use mail map file\")),\n \t\tOPT_ALIAS(0, \"mailmap\", \"use-mailmap\"),\n-\t\tOPT_STRING_LIST(0, \"decorate-refs\", &decorate_refs_include,\n+\t\tOPT_STRING_LIST(0, \"decorate-refs\", &decoration_filter.include_ref_pattern,\n \t\t\t\tN_(\"pattern\"), N_(\"only decorate refs that match <pattern>\")),\n-\t\tOPT_STRING_LIST(0, \"decorate-refs-exclude\", &decorate_refs_exclude,\n+\t\tOPT_STRING_LIST(0, \"decorate-refs-exclude\", &decoration_filter.exclude_ref_pattern,\n \t\t\t\tN_(\"pattern\"), N_(\"do not decorate refs that match <pattern>\")),\n \t\tOPT_CALLBACK_F(0, \"decorate\", NULL, NULL, N_(\"decorate options\"),\n \t\t\t       PARSE_OPT_OPTARG, decorate_callback),\n@@ -272,7 +271,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \t\tif (config_exclude) {\n \t\t\tstruct string_list_item *item;\n \t\t\tfor_each_string_list_item(item, config_exclude)\n-\t\t\t\tstring_list_append(&decorate_refs_exclude_config,\n+\t\t\t\tstring_list_append(&decoration_filter.exclude_ref_config_pattern,\n \t\t\t\t\t\t   item->string);\n \t\t}\n \ndiff --git a/log-tree.c b/log-tree.c\nindex d0ac0a6327a..f7d475f652f 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -104,32 +104,23 @@ static int ref_filter_match(const char *refname,\n \t\t\t    const struct decoration_filter *filter)\n {\n \tstruct string_list_item *item;\n-\tconst struct string_list *exclude_patterns = filter->exclude_ref_pattern;\n-\tconst struct string_list *include_patterns = filter->include_ref_pattern;\n-\tconst struct string_list *exclude_patterns_config =\n-\t\t\t\tfilter->exclude_ref_config_pattern;\n \n-\tif (exclude_patterns && exclude_patterns->nr) {\n-\t\tfor_each_string_list_item(item, exclude_patterns) {\n-\t\t\tif (match_ref_pattern(refname, item))\n-\t\t\t\treturn 0;\n-\t\t}\n-\t}\n+\tfor_each_string_list_item(item, &filter->exclude_ref_pattern)\n+\t\tif (match_ref_pattern(refname, item))\n+\t\t\treturn 0;\n \n-\tif (include_patterns && include_patterns->nr) {\n-\t\tfor_each_string_list_item(item, include_patterns) {\n+\t\n+\tif (filter->include_ref_pattern.nr) {\n+\t\tfor_each_string_list_item(item, &filter->include_ref_pattern) {\n \t\t\tif (match_ref_pattern(refname, item))\n \t\t\t\treturn 1;\n+\t\t\treturn 0;\n \t\t}\n-\t\treturn 0;\n \t}\n \n-\tif (exclude_patterns_config && exclude_patterns_config->nr) {\n-\t\tfor_each_string_list_item(item, exclude_patterns_config) {\n-\t\t\tif (match_ref_pattern(refname, item))\n-\t\t\t\treturn 0;\n-\t\t}\n-\t}\n+\tfor_each_string_list_item(item, &filter->exclude_ref_config_pattern)\n+\t\tif (match_ref_pattern(refname, item))\n+\t\t\treturn 0;\n \n \treturn 1;\n }\n@@ -202,13 +193,13 @@ void load_ref_decorations(struct decoration_filter *filter, int flags)\n \tif (!decoration_loaded) {\n \t\tif (filter) {\n \t\t\tstruct string_list_item *item;\n-\t\t\tfor_each_string_list_item(item, filter->exclude_ref_pattern) {\n+\t\t\tfor_each_string_list_item(item, &filter->exclude_ref_pattern) {\n \t\t\t\tnormalize_glob_ref(item, NULL, item->string);\n \t\t\t}\n-\t\t\tfor_each_string_list_item(item, filter->include_ref_pattern) {\n+\t\t\tfor_each_string_list_item(item, &filter->include_ref_pattern) {\n \t\t\t\tnormalize_glob_ref(item, NULL, item->string);\n \t\t\t}\n-\t\t\tfor_each_string_list_item(item, filter->exclude_ref_config_pattern) {\n+\t\t\tfor_each_string_list_item(item, &filter->exclude_ref_config_pattern) {\n \t\t\t\tnormalize_glob_ref(item, NULL, item->string);\n \t\t\t}\n \t\t}\n"},{"id":"458756","messageId":"20220711081956.GB2038@szeder.dev","threadId":"57978","inReplyTo":"xmqqfskdieqz.fsf@gitster.g","subject":"fsck segfault (was: Re: [PATCH] revision: mark blobs needed for resolve-undo as reachable)","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-07-11T08:19:56Z","receivedAt":"2022-07-11T08:20:10Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Jun 09, 2022 at 04:44:20PM -0700, Junio C Hamano wrote:\n> +static int fsck_resolve_undo(struct index_state *istate)\n> +{\n> +\tstruct string_list_item *item;\n> +\tstruct string_list *resolve_undo = istate->resolve_undo;\n> +\n> +\tif (!resolve_undo)\n> +\t\treturn 0;\n> +\n> +\tfor_each_string_list_item(item, resolve_undo) {\n> +\t\tconst char *path = item->string;\n> +\t\tstruct resolve_undo_info *ru = item->util;\n> +\t\tint i;\n> +\n> +\t\tif (!ru)\n> +\t\t\tcontinue;\n> +\t\tfor (i = 0; i < 3; i++) {\n> +\t\t\tstruct object *obj;\n> +\n> +\t\t\tif (!ru->mode[i] || !S_ISREG(ru->mode[i]))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tobj = parse_object(the_repository, &ru->oid[i]);\n\nparse_object() can return NULL ...\n\n> +\t\t\tif (!obj) {\n\n... and here is the if statement to show an error in that case ...\n\n> +\t\t\t\terror(_(\"%s: invalid sha1 pointer in resolve-undo\"),\n> +\t\t\t\t      oid_to_hex(&ru->oid[i]));\n> +\t\t\t\terrors_found |= ERROR_REFS;\n> +\t\t\t}\n> +\t\t\tobj->flags |= USED;\n\n... but then there is this line which might dereference that NULL\npointer.\n\nPerhaps all we would need is a 'continue' at the end of that 'if\n(!obj)' block, or an else block for the last three statements, which\nshould result in the same control flow?  Dunno.\n\n> +\t\t\tfsck_put_object_name(&fsck_walk_options, &ru->oid[i],\n> +\t\t\t\t\t     \":(%d):%s\", i, path);\n> +\t\t\tmark_object_reachable(obj);\n> +\t\t}\n> +\t}\n> +\treturn 0;\n> +}\n"},{"id":"458813","messageId":"xmqqo7xvo2vv.fsf@gitster.g","threadId":"57978","inReplyTo":"20220711081956.GB2038@szeder.dev","subject":"Re: fsck segfault","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-11T19:39:16Z","receivedAt":"2022-07-11T19:39:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n>> +\t\tfor (i = 0; i < 3; i++) {\n>> +\t\t\tstruct object *obj;\n>> +\n>> +\t\t\tif (!ru->mode[i] || !S_ISREG(ru->mode[i]))\n>> +\t\t\t\tcontinue;\n>> +\n>> +\t\t\tobj = parse_object(the_repository, &ru->oid[i]);\n>\n> parse_object() can return NULL ...\n>\n>> +\t\t\tif (!obj) {\n>\n> ... and here is the if statement to show an error in that case ...\n>\n>> +\t\t\t\terror(_(\"%s: invalid sha1 pointer in resolve-undo\"),\n>> +\t\t\t\t      oid_to_hex(&ru->oid[i]));\n>> +\t\t\t\terrors_found |= ERROR_REFS;\n>> +\t\t\t}\n>> +\t\t\tobj->flags |= USED;\n>\n> ... but then there is this line which might dereference that NULL\n> pointer.\n>\n> Perhaps all we would need is a 'continue' at the end of that 'if\n> (!obj)' block, or an else block for the last three statements, which\n> should result in the same control flow?  Dunno.\n\nThanks for spotting.  Looking at how fsck_cache_tree() and\nfsck_walk_tree() handles missing object, it sounds like the right\napproach to continue after setting the errors_found bit.\n\n>> +\t\t\tfsck_put_object_name(&fsck_walk_options, &ru->oid[i],\n>> +\t\t\t\t\t     \":(%d):%s\", i, path);\n>> +\t\t\tmark_object_reachable(obj);\n>> +\t\t}\n>> +\t}\n>> +\treturn 0;\n>> +}\n"},{"id":"458837","messageId":"xmqq35f7kzad.fsf@gitster.g","threadId":"57978","inReplyTo":"xmqqfskdieqz.fsf@gitster.g","subject":"[PATCH 2/1] fsck: do not dereference NULL while checking resolve-undo data","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-11T23:25:14Z","receivedAt":"2022-07-11T23:25:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When we found an invalid object recorded in the resolve-undo data,\nwe would have ended up dereferencing NULL while fsck.  Reporting the\nproblem and going on to the next object is the right thing to do\nhere.\n\nNoticed by SZEDER Gábor <szeder.dev@gmail.com>.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/fsck.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 4b17ccc3f4..6c73092f10 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -784,6 +784,7 @@ static int fsck_resolve_undo(struct index_state *istate)\n \t\t\t\terror(_(\"%s: invalid sha1 pointer in resolve-undo\"),\n \t\t\t\t      oid_to_hex(&ru->oid[i]));\n \t\t\t\terrors_found |= ERROR_REFS;\n+\t\t\t\tcontinue;\n \t\t\t}\n \t\t\tobj->flags |= USED;\n \t\t\tfsck_put_object_name(&fsck_walk_options, &ru->oid[i],\n-- \n2.37.0-248-g2505070554\n\n"}]}