{"thread":{"id":"58404","subject":"[PATCH 0/2] bug with rev-list --verify-objects and commit-graph","startedAt":"2022-09-06T20:59:11Z","lastAt":"2022-09-07T16:38:21Z","messageCount":7,"participants":["Jeff King","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"462666","messageId":"Yxe0k++LA/UfFLF/@coredump.intra.peff.net","threadId":"58404","inReplyTo":null,"subject":"[PATCH 0/2] bug with rev-list --verify-objects and commit-graph","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-06T20:58:59Z","receivedAt":"2022-09-06T20:59:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"While looking at something unrelated, I noticed that \"git rev-list\n--verify-objects\" will not actually notice corruptions of commits that\nare found in a commit graph. This fixes it.\n\nThe first one is a cleanup that is not strictly related, but is needed\nfor the tests in the second to work reliably (and is a good idea\nanyway).\n\nThe second is the fix. I don't think it's super-important, as we do not\nuse --verify-objects for anything, since d21c463d55 (fetch/receive:\nremove over-pessimistic connectivity check, 2012-03-15). And it's not\neven documented, so perhaps we should just consider getting rid of it.\nBut in the meantime, it was easy enough to correct.\n\n [1/2]: lookup_commit_in_graph(): use prepare_commit_graph() to check for graph\n [2/2]: rev-list: disable commit graph with --verify-objects\n\n commit-graph.c | 2 +-\n revision.c | 1 +\n t/t1450-fsck.sh | 28 ++++++++++++++++++++++++++++\n 3 files changed, 30 insertions(+), 1 deletion(-)\n\n-Peff\n"},{"id":"462667","messageId":"Yxe1VbyYovwprPgQ@coredump.intra.peff.net","threadId":"58404","inReplyTo":"Yxe0k++LA/UfFLF/@coredump.intra.peff.net","subject":"[PATCH 1/2] lookup_commit_in_graph(): use prepare_commit_graph() to check for graph","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-06T21:02:13Z","receivedAt":"2022-09-06T21:02:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We exit early from lookup_commit_in_graph() if the commit_graph pointer\nis NULL, under the assumption that we don't have a graph to look at. But\nthe graph pointer is lazy-loaded; if no other code happens to have\ncalled prepare_commit_graph(), we'll incorrectly assume that one isn't\navailable at all.\n\nThis has a pretty small performance impact in practice, because the\nfallback will generally be to call parse_object() instead. That ends up\nin parse_commit_buffer(), which loads the graph data itself. So the\nfirst commit we see won't use the graph, but subsequent ones will. Since\nusing the graph is just an optimization there's generally no\nuser-visible difference, but if you instrument rev-list like so:\n\n diff --git a/revision.c b/revision.c\n index ee702e498a..63c488ffb6 100644\n --- a/revision.c\n +++ b/revision.c\n @@ -381,6 +381,9 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n * parsing commit data from disk.\n */\n commit = lookup_commit_in_graph(revs->repo, oid);\n + warning(\"%s %s in commit graph\",\n + commit ? \"found\" : \"did not find\",\n + name);\n if (commit)\n object = &commit->object;\n else\n\nand run (in git.git):\n\n git commit-graph write --reachable\n git rev-list origin/master origin/next >/dev/null\n\nyou'll see that we fail to find the first one:\n\n warning: did not find origin/master in commit graph\n warning: found origin/next in commit graph\n\nAfter this patch, you'll see that we find both:\n\n warning: found origin/master in commit graph\n warning: found origin/next in commit graph\n\nEven though the performance implication is small here, there are two\nimportant reasons to do this:\n\n - it's downright confusing if you are hunting a bug triggered by the\n use of the commit graph. It may or may not trigger depending on the\n number and ordering of tips you ask for.\n\n - prepare_commit_graph() has other policy logic, too. In particular,\n if we've loaded a commit graph and then disabled the graph via\n disable_commit_graph(), that should take precedence.\n\n I'm not sure if this can trigger bad behavior in practice. The only\n caller there is upload-pack's deepen_by_rev_list(), which should be\n avoiding the commit graph for its traversal tips, but probably\n wasn't before this patch. Whether you could come up with a case\n where that mattered is unclear. Still, this is obviously the right\n thing to be doing.\n\nSigned-off-by: Jeff King \n---\nI couldn't find any other reason to avoid calling prepare_commit_graph()\nhere (especially since it ends up lazy-loaded as discussed above). The\ncost of the call should not be high; after the first call, it is\nsimplified down to a few integer checks.\n\n commit-graph.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex f2a36032f8..aef076e145 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -901,7 +901,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje\n \tstruct commit *commit;\n \tuint32_t pos;\n \n-\tif (!repo->objects->commit_graph)\n+\tif (!prepare_commit_graph(repo))\n \t\treturn NULL;\n \tif (!search_commit_pos_in_graph(id, repo->objects->commit_graph, &pos))\n \t\treturn NULL;\n-- \n2.37.3.1134.gfd534b3986\n\n"},{"id":"462668","messageId":"Yxe1422xPYoMNlkG@coredump.intra.peff.net","threadId":"58404","inReplyTo":"Yxe0k++LA/UfFLF/@coredump.intra.peff.net","subject":"[PATCH 2/2] rev-list: disable commit graph with --verify-objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-06T21:04:35Z","receivedAt":"2022-09-06T21:04:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since the point of --verify-objects is to actually load and checksum the\nbytes of each object, optimizing out reads using the commit graph runs\ncontrary to our goal.\n\nThe most targeted way to implement this would be for the revision\ntraversal code to check revs->verify_objects and avoid using the commit\ngraph. But it's difficult to be sure we've hit all of the correct spots.\nFor instance, I started this patch by writing the first of the included\ntest cases, where the corrupted commit is directly on rev-list's command\nline. And that is easy to fix by teaching get_reference() to check\nrevs->verify_objects before calling lookup_commit_in_graph().\n\nBut that doesn't cover the second test case: when we traverse to a\ncorrupted commit, we'd parse the parent in process_parents(). So we'd\nneed to check there, too. And it keeps going. In handle_commit() we\nsometimes parses commits, too, though I couldn't figure out a way to\ntrigger it that did not already parse via get_reference() or tag\npeeling. And try_to_simplify_commit() has its own parse call, and so on.\n\nSo it seems like the safest thing is to just disable the commit graph\nfor the whole process when we see the --verify-objects option. We can do\nthat either in builtin/rev-list.c, where we use the option, or in\nrevision.c, where we parse it. There are some subtleties:\n\n - putting it in rev-list.c is less surprising in some ways, because\n there we know we are just doing a single traversal. In a command\n which does multiple traversals in a single process, it's rather\n unexpected to globally disable the commit graph.\n\n - putting it in revision.c is less surprising in some ways, because\n the caller does not have to remember to disable the graph\n themselves. But this is already tricky! The verify_objects flag in\n rev_info doesn't do anything by itself. The caller has to provide an\n object callback which does the right thing.\n\n - for that reason, in practice nobody but rev-list uses this option in\n the first place. So the distinction is probably not important either\n way. Arguably it should just be an option of rev-list, and not the\n general revision machinery; right now you can run \"git log\n --verify-objects\", but it does not actually do anything useful.\n\n - checking for a parsed revs.verify_objects flag in rev-list.c is too\n late. By that time we've already passed the arguments to\n setup_revisions(), which will have parsed the commits using the\n graph.\n\nSo this commit disables the graph as soon as we see the option in\nrevision.c. That's a pretty broad hammer, but it does what we want, and\nin practice nobody but rev-list is using this flag anyway.\n\nThe tests cover both the \"tip\" and \"parent\" cases. Obviously our hammer\nhits them both in this case, but it's good to check both in case\nsomebody later tries the more focused approach.\n\nSigned-off-by: Jeff King \n---\n revision.c | 1 +\n t/t1450-fsck.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 29 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex ee702e498a..00f9c7943b 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2426,6 +2426,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->tree_objects = 1;\n \t\trevs->blob_objects = 1;\n \t\trevs->verify_objects = 1;\n+\t\tdisable_commit_graph(revs->repo);\n \t} else if (!strcmp(arg, \"--unpacked\")) {\n \t\trevs->unpacked = 1;\n \t} else if (starts_with(arg, \"--unpacked=\")) {\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 53c2aa10b7..f9a1bc5de7 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -507,6 +507,34 @@ test_expect_success 'rev-list --verify-objects with bad sha1' '\n \ttest_i18ngrep -q \"error: hash mismatch $(dirname $new)$(test_oid ff_2)\" out\n '\n \n+test_expect_success 'set up repository with commit-graph' '\n+\tgit init corrupt-graph &&\n+\t(\n+\t\tcd corrupt-graph &&\n+\t\ttest_commit one &&\n+\t\ttest_commit two &&\n+\t\tgit commit-graph write --reachable\n+\t)\n+'\n+\n+corrupt_graph_obj () {\n+\toid=$(git -C corrupt-graph rev-parse \"$1\") &&\n+\tobj=corrupt-graph/.git/objects/$(test_oid_to_path $oid) &&\n+\ttest_when_finished 'mv backup $obj' &&\n+\tmv $obj backup &&\n+\techo garbage >$obj\n+}\n+\n+test_expect_success 'rev-list --verify-objects with commit graph (tip)' '\n+\tcorrupt_graph_obj HEAD &&\n+\ttest_must_fail git -C corrupt-graph rev-list --verify-objects HEAD\n+'\n+\n+test_expect_success 'rev-list --verify-objects with commit graph (parent)' '\n+\tcorrupt_graph_obj HEAD^ &&\n+\ttest_must_fail git -C corrupt-graph rev-list --verify-objects HEAD\n+'\n+\n test_expect_success 'force fsck to ignore double author' '\n \tgit cat-file commit HEAD >basis &&\n \tsed \"s/^author .*/&,&/\" multiple-authors &&\n-- \n2.37.3.1134.gfd534b3986\n"},{"id":"462669","messageId":"Yxe4xxF/yWTLAtI3@nand.local","threadId":"58404","inReplyTo":"Yxe1VbyYovwprPgQ@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] lookup_commit_in_graph(): use prepare_commit_graph() to check for graph","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-09-06T21:16:55Z","receivedAt":"2022-09-06T21:17:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 06, 2022 at 05:02:13PM -0400, Jeff King wrote:\n> I couldn't find any other reason to avoid calling prepare_commit_graph()\n> here (especially since it ends up lazy-loaded as discussed above). The\n> cost of the call should not be high; after the first call, it is\n> simplified down to a few integer checks.\n\nNeither could I. Obviously f559d6d45e (revision: avoid hitting packfiles\nwhen commits are in commit-graph, 2021-08-09) is adding a call to look\nfor a commit by object ID in the commit-graph where there wasn't one\nbefore, so there isn't anything to compare to there.\n\nBut the next-closest function `load_commit_graph_info()` calls\n`prepare_commit_graph()` (via `repo_find_commit_pos_in_graph()`).\n\nAnd that all matches my understanding that `r->objects->commit_graph` is\nlazily loaded. Perhaps it should be made more difficult to access via\nthe struct member, and instead done behind a function like\n`prepare_commit_graph()` (modified to return the `struct commit_graph*`\nif one was found).\n\nBut that's for another day :-). This is obviously correct in the\nmeantime.\n\nThanks,\nTaylor\n"},{"id":"462670","messageId":"Yxe5qwO/GzsEXMjb@nand.local","threadId":"58404","inReplyTo":"Yxe1422xPYoMNlkG@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] rev-list: disable commit graph with --verify-objects","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-09-06T21:20:43Z","receivedAt":"2022-09-06T21:20:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 06, 2022 at 05:04:35PM -0400, Jeff King wrote:\n> So it seems like the safest thing is to just disable the commit graph\n> for the whole process when we see the --verify-objects option. We can do\n> that either in builtin/rev-list.c, where we use the option, or in\n> revision.c, where we parse it. There are some subtleties:\n\nAgreed that putting it in the rev-list code makes sense.\n\nI was wondering whether disabling the commit graph would survive\na `raw_object_store_clear()` or `close_commit_graph()`. But it does,\nsince we're not actually touching the `commit_graph` pointer itself, but\na second variable `commit_graph_disabled`, which isn't ever set back to\n0.\n\nThanks,\nTaylor\n"},{"id":"462671","messageId":"Yxe5uVmrJ9d8vp1Y@nand.local","threadId":"58404","inReplyTo":"Yxe0k++LA/UfFLF/@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] bug with rev-list --verify-objects and commit-graph","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-09-06T21:20:57Z","receivedAt":"2022-09-06T21:21:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 06, 2022 at 04:58:59PM -0400, Jeff King wrote:\n> While looking at something unrelated, I noticed that \"git rev-list\n> --verify-objects\" will not actually notice corruptions of commits that\n> are found in a commit graph. This fixes it.\n\nI reviewed both of these patches, and they look good to me. Thanks!\n\nThanks,\nTaylor\n"},{"id":"462734","messageId":"xmqq35d3ywaw.fsf@gitster.g","threadId":"58404","inReplyTo":"Yxe1422xPYoMNlkG@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] rev-list: disable commit graph with --verify-objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-07T16:38:15Z","receivedAt":"2022-09-07T16:38:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"> diff --git a/revision.c b/revision.c\n> index ee702e498a..00f9c7943b 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2426,6 +2426,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n\nIt is consistent with the description in the proposed log message ...\n\n> \t\trevs->tree_objects = 1;\n> \t\trevs->blob_objects = 1;\n> \t\trevs->verify_objects = 1;\n> +\t\tdisable_commit_graph(revs->repo);\n\n... to disable commit graph when \"--verify-objects\" is requested.\n\n> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\n> index 53c2aa10b7..f9a1bc5de7 100755\n> --- a/t/t1450-fsck.sh\n> +++ b/t/t1450-fsck.sh\n> @@ -507,6 +507,34 @@ test_expect_success 'rev-list --verify-objects with bad sha1' '\n> \ttest_i18ngrep -q \"error: hash mismatch $(dirname $new)$(test_oid ff_2)\" out\n> '\n> \n> +test_expect_success 'set up repository with commit-graph' '\n> +\tgit init corrupt-graph &&\n> +\t(\n> +\t\tcd corrupt-graph &&\n> +\t\ttest_commit one &&\n> +\t\ttest_commit two &&\n> +\t\tgit commit-graph write --reachable\n> +\t)\n> +'\n> +\n> +corrupt_graph_obj () {\n> +\toid=$(git -C corrupt-graph rev-parse \"$1\") &&\n> +\tobj=corrupt-graph/.git/objects/$(test_oid_to_path $oid) &&\n> +\ttest_when_finished 'mv backup $obj' &&\n\nOK. I missed this when-finished thing in my first read and wondered\nwho takes care of the 'backup' file. We obviously cannot use it in\ntests that do not follow the \"break single loose object and inspect\nthe command behaviour in the repository\" pattern, but that is OK.\n\n> +\tmv $obj backup &&\n> +\techo garbage >$obj\n> +}\n> +\n> +test_expect_success 'rev-list --verify-objects with commit graph (tip)' '\n> +\tcorrupt_graph_obj HEAD &&\n> +\ttest_must_fail git -C corrupt-graph rev-list --verify-objects HEAD\n> +'\n> +\n> +test_expect_success 'rev-list --verify-objects with commit graph (parent)' '\n> +\tcorrupt_graph_obj HEAD^ &&\n> +\ttest_must_fail git -C corrupt-graph rev-list --verify-objects HEAD\n> +'\n> +\n\nLooks good, thanks.\n\n> test_expect_success 'force fsck to ignore double author' '\n> \tgit cat-file commit HEAD >basis &&\n> \tsed \"s/^author .*/&,&/\" multiple-authors &&\n"}]}