{"thread":{"id":"64021","subject":"[PATCH] fetch-pack: re-scan when double-checking graph objects","startedAt":"2025-08-24T05:00:48Z","lastAt":"2025-08-24T05:00:48Z","messageCount":1,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"524803","messageId":"20250824050040.GA228050@coredump.intra.peff.net","threadId":"64021","inReplyTo":null,"subject":"[PATCH] fetch-pack: re-scan when double-checking graph objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-24T05:00:40Z","receivedAt":"2025-08-24T05:00:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The fetch code tries to avoid asking the remote side for an object we\nalready have. It does this by traversing recent commits reachable from\nour refs looking for matches. Commit 5d4cc78f72 (fetch-pack: die if in\ncommit graph but not obj db, 2024-11-05) introduced an extra check\nthere: if we think we have an object because it's in the commit graph,\nwe double-check that we actually have it in our object database with a\ncall to odb_has_object().\n\nBut that call does not pass any flags, and so the function won't call\nreprepared_packed_git() if it does not find the object. That opens us up\nto the usual race against some other process repacking the odb:\n\n  1. We scan the list of packs in objects/pack but haven't yet opened them.\n\n  2. Somebody else packs the object into a new pack (which we don't know\n     about), and deletes the old pack it was in.\n\n  3. Our odb_has_object() calls tries to open that old pack, but finds it\n     is gone. We declare that we don't have the object.\n\nAnd this causes us to erroneously complain and abort the fetch, thinking\nour commit-graph and object database are out of sync. Instead, we should\npass HAS_OBJECT_RECHECK_PACKED, which will add a new step:\n\n  4. We re-scan the pack directory again, find the new pack, and locate\n     the object.\n\nOften the fetch code tries to avoid these kinds of re-scans if it's\nlikely that we won't have the object. If the other side has told us\nabout object X and we want to know if we have it, we'll skip the re-scan\n(to avoid spending a lot of effort when there are many such objects). We\ncan accept the racy false negative in that case because the worst case\nis that we ask the other side to send us the object.\n\nBut this is not one of those cases. These are objects which are\naccessible from _our_ refs, and which we already found in the commit\ngraph file. We should have them, and if we don't, we'll die()\nimmediately. So the performance impact is negligible, and getting the\nright answer is important.\n\nThere's no test here because it's inherently racy. In fact, I had\ntrouble even developing a minimal test. The problem seen in the wild can\nbe produced like this:\n\n  # Any git.git mirror which supports partial clones; I think this\n  # should work with any repo that contains submodules, but note that\n  # $obj below is specific to this repo\n  url=https://github.com/git/git.git\n\n  # This is a commit that is not at the tip of any branches (so after\n  # we have it, we'll still have some commits to fetch).\n  obj=cf6f63ea6bf35173e02e18bdc6a4ba41288acff9\n\n  git init\n  git fetch --filter=tree:0 $url $obj:refs/heads/foo\n  git checkout foo\n  git commit-graph write --reachable\n  git fetch $url\n\nWhat happens here is that the initial fetch grabs that older commit (and\nits ancestors) but no trees or blobs, and the subsequent checkout grabs\nthe necessary trees and blobs just for that commit. The final fetch\nspawns a long sequence of child fetches due to fetch_submodules(), which\nwants to check whether there have been any gitlink modifications which\nshould trigger a fetch of the related submodule (we'll leave aside the\nirony that we did not even check out any submodules yet).\n\nThat series of fetches causes us to accumulate packs, which eventually\ntriggers background maintenance to run. That repacks all-into-one, and\nthe pack containing $obj goes away in favor of a new pack. And then the\nfetch eventually fails with:\n\n  fatal: You are attempting to fetch cf6f63ea6bf35173e02e18bdc6a4ba41288acff9, which is in the commit graph file but\nnot in the object database.\n\nIn the scenario above, the race becomes likely because of the long\nseries of quick fetches. But I _think_ the bug is independent of partial\nclones entirely, and you could run into the same thing with a single\nfetch, some other process running \"git repack\" simultaneously, and a bit\nof bad luck. I haven't been able to reproduce, though. I'm not sure if\nthat's because there's some mis-analysis above, or if the race window is\njust small enough that it's hard to trigger.\n\nAt any rate, re-scanning here seems like an obviously correct thing to\ndo with no downside, and it does fix the partial-clone case shown above.\n\nReported-by: Дилян Палаузов <dilyan.palauzov@aegee.org>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nOriginally discussed in:\n\n  https://lore.kernel.org/git/e7a2fdff63d9a90ef4dc1341fa642fff5197b64a.camel@aegee.org/\n\n fetch-pack.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 20e5533b21..6ed5662951 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -143,7 +143,8 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n \tcommit = lookup_commit_in_graph(the_repository, oid);\n \tif (commit) {\n \t\tif (mark_tags_complete_and_check_obj_db) {\n-\t\t\tif (!odb_has_object(the_repository->objects, oid, 0))\n+\t\t\tif (!odb_has_object(the_repository->objects, oid,\n+\t\t\t\t\t    HAS_OBJECT_RECHECK_PACKED))\n \t\t\t\tdie_in_commit_graph_only(oid);\n \t\t}\n \t\treturn commit;\n-- \n2.51.0.364.gbc6ae1dd20\n"}]}