{"thread":{"id":"62250","subject":"[RFC PATCH] promisor-remote: always JIT fetch with --refetch","startedAt":"2024-10-03T22:35:56Z","lastAt":"2024-11-06T03:13:01Z","messageCount":38,"participants":["Emily Shaffer","Junio C Hamano","Robert Coup","Taylor Blau","Jonathan Tan","Josh Steadmon","Han Xin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"504060","messageId":"20241003223546.1935471-1-emilyshaffer@google.com","threadId":"62250","inReplyTo":null,"subject":"[RFC PATCH] promisor-remote: always JIT fetch with --refetch","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2024-10-03T22:35:46Z","receivedAt":"2024-10-03T22:35:56Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"By the time we decide we need to do a partial clone fetch, we already\nknow the object is missing, even if the_repository->parsed_objects\nthinks it exists. But --refetch bypasses the local object check, so we\ncan guarantee that a JIT fetch will fix incorrect local caching.\n\nThis manifested at $DAYJOB in a repo with the following features:\n * blob-filtered partial clone enabled\n * commit graph enabled\n * ref Foo pointing to commit object 6aaaca\n * object 6aaaca missing[a]\n\nWith these prerequisites, we noticed that `git fetch` in the repo\nproduced an infinite loop:\n1. `git fetch` tries to fetch, but thinks it has all objects, so it\n noops.\n2. At the end of cmd_fetch(), we try to write_commit_graph_reachable().\n3. write_commit_graph_reachable() does a reachability walk, including\n starting from Foo\n4. The reachability walk tries to peel Foo, and notices it's missing\n 6aaaca.\n5. The partial clone machinery asks for a per-object JIT fetch of\n 6aaaca.\n6. `git fetch` (child process) is asked to fetch 6aaaca.\n7. We put together the_repository->parsed_objects, adding all commit IDs\n reachable from local refs to it\n (fetch-pack.c:mark_complete_and_common_refs(), trace region\n mark_complete_local_refs). We see Foo, so we add 6aaaca to\n the_repository->parsed_objects and mark it as COMPLETE.\n8. cmd_fetch notices that the object ID it was asked for is already\n known, so does not fetch anything new.\n9. GOTO 2.\n\nThe culprit is that we're assuming all local refs already must have\nobjects in place. Using --refetch means we ignore that assumption during\nJIT fetch.\n\nSigned-off-by: Emily Shaffer \n\n---\n\nThere are a few alternative approaches for this issue that I talked\nabout with some folks at $DAYJOB:\n\ni. Just disabling the commit graph rewrite allows this to fall\neventually into a path where the fetch actually succeeds. I didn't like\nthis solution - it's just whack-a-mole - so I didn't look too hard into\nwhy it succeeds that way. It *could* make sense to disable commit graph\nrewrite when we do a JIT fetch with blob or tree filter provided - but\nif later we want to implement commit filter (something we've talked\nabout at Google) then I'd worry about this situation coming up again.\n\nii. We could decide not to mark local refs (and commits reachable from\nthem) as COMPLETE in the_repository->parsed_objects. I didn't try this\nsolution out, and I'm not sure what the performance implications are,\nbut Jonathan Tan likes this solution, so I may try it out and see what\nbreaks shortly.\n\niii. We could do all the JIT fetches with --refetch. In my opinion, this\nis the safest/most self-healing solution; the JIT fetch only happens\nwhen we really know we're missing the object, so it doesn't make sense\nfor that fetch to be canceled by any cache. It doesn't have performance\nimplications as far as I can guess (except that I think we still build\nthe parsed_objects hash even though we are going to ignore it, but we\nalready were doing that anyway). Of course, that's what this patch does.\n\niv. We could do nothing; when cmd_fetch gets a fetch-by-object-id but\ndecides there is nothing more to do, it could terminate with an error.\nThat should stop the infinite recursion, and the error could suggest the\nuser to run `git fsck` and discover what the problem is. Depending on\nthe remediation we suggest, though, I think a direct fetch to fix this\nparticular loop would not work.\n\nI'm curious to hear thoughts from people who are more expert than me on\npartial clone and fetching in general, though.\n\nThis change is also still in RFC, for two reasons:\n\nFirst, it's intermittently failing tests for me locally, in weirdly\nflaky ways:\n\n- t0410-partial-clone.sh fails when I run it from prove, but passes when\n I run it manually, every time.\n- t5601-clone.sh and t5505-remote.sh fail nonsensically on `rm -rf` that\n should succeed (and does succeed if I stop the test with test_pause),\n which makes me think there's something else borked in my setup, but\n I'm not sure what.\n- t5616-partial-clone.sh actually does fail in a way that I could see\n having to do with this change (since I guess we might download more\n packs than usual), but I was so confused by the other two errors I\n haven't looked closely yet.\n\nAnd secondly, I didn't write tests verifying the breakage and that this\nchange fixes it yet, either.\n\nI'm going to work on both those things in the background, but I wanted\nto get the description and RFC out early so that folks could take a look\nand we could decide which approach is best.\n\nThanks,\n - Emily\n\na: That commit object went missing as a byproduct of this partial clone\n gc issue that Calvin, Jonathan, Han Young, and others have been\n investigating:\n https://lore.kernel.org/git/20241001191811.1934900-1-calvinwan@google.com/\n---\n promisor-remote.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 9345ae3db2..cf00e31d3b 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -43,7 +43,7 @@ static int fetch_objects(struct repository *repo,\n \tstrvec_pushl(&child.args, \"-c\", \"fetch.negotiationAlgorithm=noop\",\n \t\t \"fetch\", remote_name, \"--no-tags\",\n \t\t \"--no-write-fetch-head\", \"--recurse-submodules=no\",\n-\t\t \"--filter=blob:none\", \"--stdin\", NULL);\n+\t\t \"--filter=blob:none\", \"--refetch\", \"--stdin\", NULL);\n \tif (!git_config_get_bool(\"promisor.quiet\", &quiet) && quiet)\n \t\tstrvec_push(&child.args, \"--quiet\");\n \tif (start_command(&child))\n-- \n2.47.0.rc0.187.ge670bccf7e-goog\n\n"},{"id":"504214","messageId":"xmqqset8c0o7.fsf@gitster.g","threadId":"62250","inReplyTo":"20241003223546.1935471-1-emilyshaffer@google.com","subject":"Re: [RFC PATCH] promisor-remote: always JIT fetch with --refetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-06T22:43:36Z","receivedAt":"2024-10-06T22:43:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n> By the time we decide we need to do a partial clone fetch, we already\n> know the object is missing, even if the_repository->parsed_objects\n> thinks it exists. But --refetch bypasses the local object check, so we\n> can guarantee that a JIT fetch will fix incorrect local caching.\n> ...\n> The culprit is that we're assuming all local refs already must have\n> objects in place. Using --refetch means we ignore that assumption during\n> JIT fetch.\n\nHmph. The whole lazy fetch business looks more and more broken X-<.\nThere is a comment in the refetch code path that tells us to \"perform\na full refetch ignoring existing objects\", but if an object truly\nexists, there should be no need to refetch, and it starts to smell\nmore like \"ignoring somebody who gives us an incorrect information\nthat these objects exist\".\n\nBut a ref that points at a missing commit is \"somebody giving a\nfalse information\" and an option to ignore such misinformation would\nbe a perfect tool fit to sweep such a breakage under the rug.\n\nBut is this sufficient? Looking at how check_exist_and_connected()\ndoes its work, I am not sure how it would cope with a case where an\nobject that is pointed by a ref does happen to exist, but the commit\nthat is referred to by the commit is missing, as it only checks the\nexistence of the tips.\n\n> diff --git a/promisor-remote.c b/promisor-remote.c\n> index 9345ae3db2..cf00e31d3b 100644\n> --- a/promisor-remote.c\n> +++ b/promisor-remote.c\n> @@ -43,7 +43,7 @@ static int fetch_objects(struct repository *repo,\n> \tstrvec_pushl(&child.args, \"-c\", \"fetch.negotiationAlgorithm=noop\",\n> \t\t \"fetch\", remote_name, \"--no-tags\",\n> \t\t \"--no-write-fetch-head\", \"--recurse-submodules=no\",\n> -\t\t \"--filter=blob:none\", \"--stdin\", NULL);\n> +\t\t \"--filter=blob:none\", \"--refetch\", \"--stdin\", NULL);\n> \tif (!git_config_get_bool(\"promisor.quiet\", &quiet) && quiet)\n> \t\tstrvec_push(&child.args, \"--quiet\");\n> \tif (start_command(&child))\n\nThe documentation for \"git fetch --refetch\" says that this grabs\neverything as if we are making a fresh clone, ignoring everything we\nalready have. Which makes the change in this patch prohibitively\nexpensive for asking each single object lazily from the promisor\nremote, but is that really the case? If there is a reasonable\nsafety that prevents us from doing something silly like transferring\none clone worth of data for every single object we lazily fetch,\nperhaps this would be a workable solution (but if that is the case,\nperhaps \"git fetch --refetch\" documentation needs to be rephrased,\nto avoid such an impression).\n\nThanks.\n"},{"id":"504222","messageId":"CAFLLRp+Y1hO6r7mfdghS0q3EyfJhU_e43Hzi9PXgF_EuF9Fuog@mail.gmail.com","threadId":"62250","inReplyTo":"xmqqset8c0o7.fsf@gitster.g","subject":"Re: [RFC PATCH] promisor-remote: always JIT fetch with --refetch","fromName":"Robert Coup","fromEmail":"robert.coup@koordinates.com","sentAt":"2024-10-07T00:21:16Z","receivedAt":"2024-10-07T00:21:33Z","isPatch":true,"sender":{"key":"robert.coup@koordinates.com","avatar":"https://gravatar.com/avatar/d1a87d63ffb562b791992d8a119ebbdd742e703109d23333ca3fca51306ee95c?d=mp&s=160"},"body":"Hi Emily,\n\nI was the one who originally implemented --refetch in [1][2]\n\n[1] https://lore.kernel.org/git/pull.1138.v4.git.1648476131.gitgitgadget@gmail.com/\n[2] https://github.com/gitgitgadget/git/pull/1138\n\nOn Sun, 6 Oct 2024 at 23:43, Junio C Hamano wrote:\n>\n> Hmph. The whole lazy fetch business looks more and more broken X-<.\n> There is a comment in the refetch code path that tells us to \"perform\n> a full refetch ignoring existing objects\", but if an object truly\n> exists, there should be no need to refetch, and it starts to smell\n> more like \"ignoring somebody who gives us an incorrect information\n> that these objects exist\".\n\nBasically --refetch was originally designed to send no 'have's during a fetch,\nthe original motivation being changing a partial clone filter and fetching\nall the newly-applicable trees & blobs in a single transfer.\n\n> The documentation for \"git fetch --refetch\" says that this grabs\n> everything as if we are making a fresh clone, ignoring everything we\n> already have. Which makes the change in this patch prohibitively\n> expensive for asking each single object lazily from the promisor\n> remote, but is that really the case?\n\nFrom a very quick re-review this is correct that it's expensive: refetch sends\nno 'have's, so if you pass a single commit oid then it'll fetch all ancestors\nand all the dependent trees & blobs, duplicating what's in the object store and\nrelying on a repack to clean up. If a commit is missing that's one way to fix\nit, but it's a pretty nuclear option: feels like your option iv (terminate with\nan error) leading to fsck invoking/suggesting --refetch might avoid\nunintentionally recloning the entire repo.\n\nIn my original RFC [3], Jonathan Tan suggested that --refetch could be useful\nto repair missing objects like this, but it was out of scope for me at the time.\nBut maybe there's a way to improve it for this sort of case?\n\n[3] https://lore.kernel.org/git/20220202185957.1928631-1-jonathantanmy@google.com/\n\n> Emily Shaffer writes:\n>\n> This manifested at $DAYJOB in a repo with the following features:\n> * blob-filtered partial clone enabled\n> * commit graph enabled\n> * ref Foo pointing to commit object 6aaaca\n> * object 6aaaca missing[a]\n\nI presume there wasn't an obvious/related cause for commit 6aaaca to go missing\nin the first place?\n\nThanks,\n\nRob :)\n"},{"id":"504223","messageId":"xmqqbjzwbvez.fsf@gitster.g","threadId":"62250","inReplyTo":"CAFLLRp+Y1hO6r7mfdghS0q3EyfJhU_e43Hzi9PXgF_EuF9Fuog@mail.gmail.com","subject":"Re: [RFC PATCH] promisor-remote: always JIT fetch with --refetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-07T00:37:08Z","receivedAt":"2024-10-07T00:37:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robert Coup writes:\n\n> Basically --refetch was originally designed to send no 'have's during a fetch,\n> the original motivation being changing a partial clone filter and fetching\n> all the newly-applicable trees & blobs in a single transfer.\n> ...\n> If a commit is missing that's one way to fix\n> it, but it's a pretty nuclear option: feels like your option iv (terminate with\n> an error) leading to fsck invoking/suggesting --refetch might avoid\n> unintentionally recloning the entire repo.\n> ...\n> In my original RFC [3], Jonathan Tan suggested that --refetch could be useful\n> to repair missing objects like this, but it was out of scope for me at the time.\n> But maybe there's a way to improve it for this sort of case?\n>\n> [3] https://lore.kernel.org/git/20220202185957.1928631-1-jonathantanmy@google.com/\n\nThanks for your comments on the original story behind that option.\n\n> I presume there wasn't an obvious/related cause for commit 6aaaca to go missing\n> in the first place?\n\nEmily had this after the three-dash line\n\n\na: That commit object went missing as a byproduct of this partial clone\n gc issue that Calvin, Jonathan, Han Young, and others have been\n investigating:\n https://lore.kernel.org/git/20241001191811.1934900-1-calvinwan@google.com/\n\nIOW, I think how the lossage was caused is well understood by now.\n\nThanks.\n"},{"id":"504848","messageId":"CAJoAoZ=todZ6Ej9CQcF+f-C6vBZ8x-H6VX0dKhJwfvmJyGOW7w@mail.gmail.com","threadId":"62250","inReplyTo":"xmqqset8c0o7.fsf@gitster.g","subject":"Re: [RFC PATCH] promisor-remote: always JIT fetch with --refetch","fromName":"Emily Shaffer","fromEmail":"nasamuffin@google.com","sentAt":"2024-10-11T16:40:04Z","receivedAt":"2024-10-11T16:40:18Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Sorry for the slow response/page-context-back-in. I'll be working on\nthis today and try to send a different approach, but after that point,\nI'm not sure when the next time I'll get a chance to work on it may\nbe. If I don't come up with something suitable today, it's likely that\nJonathan Tan will take over the effort from me, but I'm not sure\naround when he'll be able to prioritize it.\n\nOn Sun, Oct 6, 2024 at 3:43 PM Junio C Hamano wrote:\n>\n> Emily Shaffer writes:\n>\n> > By the time we decide we need to do a partial clone fetch, we already\n> > know the object is missing, even if the_repository->parsed_objects\n> > thinks it exists. But --refetch bypasses the local object check, so we\n> > can guarantee that a JIT fetch will fix incorrect local caching.\n> > ...\n> > The culprit is that we're assuming all local refs already must have\n> > objects in place. Using --refetch means we ignore that assumption during\n> > JIT fetch.\n>\n> Hmph. The whole lazy fetch business looks more and more broken X-<.\n\nBy \"lazy fetch\", are you referring to the partial clone fetch, or are\nyou referring to the mark_complete stuff? (I know we have been having\nlots of issues with the partial clone fetch at Google in the last\nmonth or so, so excuse me disambiguating :) )\n\n> There is a comment in the refetch code path that tells us to \"perform\n> a full refetch ignoring existing objects\", but if an object truly\n> exists, there should be no need to refetch, and it starts to smell\n> more like \"ignoring somebody who gives us an incorrect information\n> that these objects exist\".\n>\n> But a ref that points at a missing commit is \"somebody giving a\n> false information\" and an option to ignore such misinformation would\n> be a perfect tool fit to sweep such a breakage under the rug.\n>\n> But is this sufficient? Looking at how check_exist_and_connected()\n> does its work, I am not sure how it would cope with a case where an\n> object that is pointed by a ref does happen to exist, but the commit\n> that is referred to by the commit is missing, as it only checks the\n> existence of the tips.\n\nIs that so? mark_complete_and_common claims that it recurses through\nall parents of all local refs and marks them existing, too. Looks like\nit does that in fetch-pack.c:mark_recent_complete_commits(), only up\nto a certain date cutoff, and doesn't do that at all if there's no\ncutoff provided. I don't think I see anywhere else that it's recursing\nover parents, so I'm not sure why the comment says that. In fact, I\nsort of wonder if the comment is wrong; it was introduced in this[1]\nseries much later than this code block has existed. But then, nobody\nquestioned it during the series, so I can also be misreading the code\n:)\n\n>\n> > diff --git a/promisor-remote.c b/promisor-remote.c\n> > index 9345ae3db2..cf00e31d3b 100644\n> > --- a/promisor-remote.c\n> > +++ b/promisor-remote.c\n> > @@ -43,7 +43,7 @@ static int fetch_objects(struct repository *repo,\n> > strvec_pushl(&child.args, \"-c\", \"fetch.negotiationAlgorithm=noop\",\n> > \"fetch\", remote_name, \"--no-tags\",\n> > \"--no-write-fetch-head\", \"--recurse-submodules=no\",\n> > - \"--filter=blob:none\", \"--stdin\", NULL);\n> > + \"--filter=blob:none\", \"--refetch\", \"--stdin\", NULL);\n> > if (!git_config_get_bool(\"promisor.quiet\", &quiet) && quiet)\n> > strvec_push(&child.args, \"--quiet\");\n> > if (start_command(&child))\n>\n> The documentation for \"git fetch --refetch\" says that this grabs\n> everything as if we are making a fresh clone, ignoring everything we\n> already have. Which makes the change in this patch prohibitively\n> expensive for asking each single object lazily from the promisor\n> remote, but is that really the case? If there is a reasonable\n> safety that prevents us from doing something silly like transferring\n> one clone worth of data for every single object we lazily fetch,\n> perhaps this would be a workable solution (but if that is the case,\n> perhaps \"git fetch --refetch\" documentation needs to be rephrased,\n> to avoid such an impression).\n\nYeah, this is on me for not reading the entire documentation, just\nnoticing in code that it disabled this COMPLETE cache thingie. You're\nright that it would be too expensive to use this way. As I said at the\ntop, I'll try to send one of the other alternative approaches today.\n\n - Emily\n\n1: https://lore.kernel.org/git/pull.451.git.1572981981.gitgitgadget@gmail.com/\n"},{"id":"504860","messageId":"xmqqttdied9b.fsf@gitster.g","threadId":"62250","inReplyTo":"CAJoAoZ=todZ6Ej9CQcF+f-C6vBZ8x-H6VX0dKhJwfvmJyGOW7w@mail.gmail.com","subject":"Re: [RFC PATCH] promisor-remote: always JIT fetch with --refetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-11T17:54:40Z","receivedAt":"2024-10-11T17:54:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n> By \"lazy fetch\", are you referring to the partial clone fetch, or are\n\nAnything we do to compensate for the fact that that initial clone or\nfetch can be told to deliberately omit objects, in the hope that we\ncan grab missing objects on demand.\n\n> Yeah, this is on me for not reading the entire documentation, just\n> noticing in code that it disabled this COMPLETE cache thingie. You're\n> right that it would be too expensive to use this way. As I said at the\n> top, I'll try to send one of the other alternative approaches today.\n\nI thought you were already on your vacation ;-). I'll go offline\nend of this week and won't be back until the end of the month.\n\n"},{"id":"505878","messageId":"20241023002806.367082-1-emilyshaffer@google.com","threadId":"62250","inReplyTo":"20241003223546.1935471-1-emilyshaffer@google.com","subject":"[PATCH v2] fetch-pack: don't mark COMPLETE unless we have the full object","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2024-10-23T00:28:05Z","receivedAt":"2024-10-23T00:28:15Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"When fetching, we decide which objects to skip asking for marking\ncertain commit IDs with the COMPLETE flag. This flag is set in\nfetch-pack.c:mark_complete(), which is called from a few different\nfunctions who decide what to mark or not mark. mark_complete() is\ninsulated against null pointer deref and repeatedly writing the same\ncommit to the list of objects to skip; because it's the central function\nwhich decides that an object is COMPLETE and doesn't need to be fetched,\nlet's also insulate it against corruption where the object is not\npresent (even though we think it is).\n\nWithout this check, it's possible to reach a corrupted state where\nfetches can infinitely loop because we decide to skip fetching, but we\ndon't actually have the skipped commit in our object store.\n\nThis manifested at $DAYJOB in a repo with the following features:\n * blob-filtered partial clone enabled\n * commit graph enabled\n * ref Foo pointing to commit object 6aaaca\n * object 6aaaca missing[1]\n\nWith these prerequisites, we noticed that `git fetch` in the repo\nproduced an infinite loop:\n1. `git fetch` tries to fetch, but thinks it has all objects, so it\n noops.\n2. At the end of cmd_fetch(), we try to write_commit_graph_reachable().\n3. write_commit_graph_reachable() does a reachability walk, including\n starting from Foo\n4. The reachability walk tries to peel Foo, and notices it's missing\n 6aaaca.\n5. The partial clone machinery asks for a per-object JIT fetch of\n 6aaaca.\n6. `git fetch` (child process) is asked to fetch 6aaaca.\n7. We put together the_repository->parsed_objects, adding all commit IDs\n reachable from local refs to it\n (fetch-pack.c:mark_complete_and_common_refs(), trace region\n mark_complete_local_refs). We see Foo, so we add 6aaaca to\n the_repository->parsed_objects and mark it as COMPLETE.\n8. cmd_fetch notices that the object ID it was asked for is already\n known, so does not fetch anything new.\n9. GOTO 2.\n\nThe culprit is that we're assuming all local refs already must have\nobjects in place. Let's not assume that, and explicitly check\nhas_object() before marking objects as COMPLETE.\n\nNEEDSWORK: It could be valuable to emit a trace event when we try to\nmark_complete() an object where we don't actually has_object() (to\nunderstand how often we're having to heal from corruption).\n\nSigned-off-by: Emily Shaffer \nHelped-by: Jonathan Tan \n\n1: That commit object went missing as a byproduct of this partial clone\n gc issue:\n https://lore.kernel.org/git/20241001191811.1934900-1-calvinwan@google.com/\n\n---\n\n\nOn the list, Junio and Robert Coup suggested that we should notice cases\nwhen the list of object IDs we are trying to fetch gets filtered to\nnothing during a lazy fetch. However, this turned out to be quite tricky\nto implement - fetch-by-OID stores the OID in a ref object, so it's\nchallenging to guess that the ref we care about is an OID instead of a\nnormal ref. It also required some heuristic approach to catch the exact\nmoment after we removed COMPLETE objects from the list we wanted to\nfetch, and to notice that the wrong objects were missing from that list.\n\n(From the list of alternatives I included with v1, v1 was approach iii;\nJunio and Robert suggested iv; this patch holds something close to ii.)\n\nJonathan Tan suggested that instead, we could be more careful about what\nwe mark COMPLETE or not; it seems like this is pretty straightforward to\ndo.\n\nI do wonder if it's a layering violation to do the has_object() check in\nmark_complete(), which is otherwise a pretty stupid function; I included\nit there because all of the other flavors of mark_complete_* eventually\nboil down to mark_complete(), so we wouldn't need to remember to check\nfor object existence any other time we're trying to do this COMPLETE\nmarking thing.\n\nNote that I added a test to guarantee this behavior works, but without\ncommit graph enabled, the test passes both before and after; I guess\nmaybe it's better to add an explicit regression test for the\ncombination? But, this test fails with reftable - because we don't have\na way (that I know of) to force-create a ref with a bad ID, to force\nthis error condition. In the test as written I'm writing to\n.git/refs/heads/bar directly; that doesn't work for reftable. But `git\nupdate-ref` is too smart to let me set a ref to garbage. Any tips there\nare welcome.\n\nThe CI run is at\nhttps://github.com/nasamuffin/git/actions/runs/11470039702 - it seems\nthe reftable tests are the only things failing.\n\nAlso, I am sending this version, but if there are any additional\ncomments or it requires more changes, please expect Jonathan Tan to take\nover driving this patch the rest of the way for me. As previously\nstated[2], I'll be OOO after this Friday for most of the rest of the\nyear; the rest of this week I'm trying to get the rest of my\nnon-upstream loose ends tied up, so I won't have time to do another\niteration. See folks around Christmastime :)\n\n - Emily\n\n2: https://lore.kernel.org/git/CAJoAoZnovapqMcu72DGR40jRRqRn57uJVTJg82kZ_rohtGDSfQ@mail.gmail.com/\n\nCover letter from v1 follows:\n\nThere are a few alternative approaches for this issue that I talked\nabout with some folks at $DAYJOB:\n\ni. Just disabling the commit graph rewrite allows this to fall\neventually into a path where the fetch actually succeeds. I didn't like\nthis solution - it's just whack-a-mole - so I didn't look too hard into\nwhy it succeeds that way. It *could* make sense to disable commit graph\nrewrite when we do a JIT fetch with blob or tree filter provided - but\nif later we want to implement commit filter (something we've talked\nabout at Google) then I'd worry about this situation coming up again.\n\nii. We could decide not to mark local refs (and commits reachable from\nthem) as COMPLETE in the_repository->parsed_objects. I didn't try this\nsolution out, and I'm not sure what the performance implications are,\nbut Jonathan Tan likes this solution, so I may try it out and see what\nbreaks shortly.\n\niii. We could do all the JIT fetches with --refetch. In my opinion, this\nis the safest/most self-healing solution; the JIT fetch only happens\nwhen we really know we're missing the object, so it doesn't make sense\nfor that fetch to be canceled by any cache. It doesn't have performance\nimplications as far as I can guess (except that I think we still build\nthe parsed_objects hash even though we are going to ignore it, but we\nalready were doing that anyway). Of course, that's what this patch does.\n\niv. We could do nothing; when cmd_fetch gets a fetch-by-object-id but\ndecides there is nothing more to do, it could terminate with an error.\nThat should stop the infinite recursion, and the error could suggest the\nuser to run `git fsck` and discover what the problem is. Depending on\nthe remediation we suggest, though, I think a direct fetch to fix this\nparticular loop would not work.\n\nI'm curious to hear thoughts from people who are more expert than me on\npartial clone and fetching in general, though.\n\nThis change is also still in RFC, for two reasons:\n\nFirst, it's intermittently failing tests for me locally, in weirdly\nflaky ways:\n\n- t0410-partial-clone.sh fails when I run it from prove, but passes when\n I run it manually, every time.\n- t5601-clone.sh and t5505-remote.sh fail nonsensically on `rm -rf` that\n should succeed (and does succeed if I stop the test with test_pause),\n which makes me think there's something else borked in my setup, but\n I'm not sure what.\n- t5616-partial-clone.sh actually does fail in a way that I could see\n having to do with this change (since I guess we might download more\n packs than usual), but I was so confused by the other two errors I\n haven't looked closely yet.\n\nAnd secondly, I didn't write tests verifying the breakage and that this\nchange fixes it yet, either.\n\nI'm going to work on both those things in the background, but I wanted\nto get the description and RFC out early so that folks could take a look\nand we could decide which approach is best.\n\nThanks,\n - Emily\n---\n fetch-pack.c | 4 +++-\n t/t0410-partial-clone.sh | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex f752da93a8..8cb2ce4c54 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -603,7 +603,9 @@ static int mark_complete(const struct object_id *oid)\n {\n \tstruct commit *commit = deref_without_lazy_fetch(oid, 1);\n \n-\tif (commit && !(commit->object.flags & COMPLETE)) {\n+\tif (commit &&\n+\t !(commit->object.flags & COMPLETE) &&\n+\t has_object(the_repository, oid, 0)) {\n \t\tcommit->object.flags |= COMPLETE;\n \t\tcommit_list_insert(commit, &complete);\n \t}\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 818700fbec..95de18ec40 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -241,6 +241,36 @@ test_expect_success 'fetching of missing objects works with ref-in-want enabled'\n \tgrep \"fetch< fetch=.*ref-in-want\" trace\n '\n \n+test_expect_success 'fetching missing objects pointed to by a local ref' '\n+\trm -rf reliable-server unreliable-client &&\n+\ttest_when_finished rm -rf reliable-server unreliable-client &&\n+\ttest_create_repo reliable-server &&\n+\tgit -C reliable-server config uploadpack.allowanysha1inwant 1 &&\n+\tgit -C reliable-server config uploadpack.allowfilter 1 &&\n+\ttest_commit -C reliable-server foo &&\n+\n+\tgit clone --filter=blob:none \"file://$(pwd)/reliable-server\" unreliable-client &&\n+\n+\t# to simulate the unreliable client losing a referenced object by\n+\t# corruption, create the object on the server side, then create only a\n+\t# reference to that object on the client side (without providing the\n+\t# object itself).\n+\ttest_commit -C reliable-server bar &&\n+\tHASH=$(git -C reliable-server rev-parse HEAD) &&\n+\techo \"$HASH\" >unreliable-client/.git/refs/heads/bar &&\n+\n+\t# the object is really missing\n+\t# check if we can rev-parse a partial SHA. partial so we do not fetch it,\n+\t# but barely partial (trim only the last char) so that we do not collide\n+\ttest_must_fail git -C unreliable-client rev-parse ${HASH%%?} &&\n+\n+\t# trigger a remote fetch by checking out `bar`\n+\tgit -C unreliable-client switch bar &&\n+\n+\t# and now we have the missing object\n+\tgit -C unreliable-client rev-parse ${HASH%%?}\n+'\n+\n test_expect_success 'fetching of missing objects from another promisor remote' '\n \tgit clone \"file://$(pwd)/server\" server2 &&\n \ttest_commit -C server2 bar &&\n\nRange-diff against v1:\n1: 092be0a655 ! 1: 4db6bbb4cd promisor-remote: always JIT fetch with --refetch\n - ## promisor-remote.c ##\n -@@ promisor-remote.c: static int fetch_objects(struct repository *repo,\n - \tstrvec_pushl(&child.args, \"-c\", \"fetch.negotiationAlgorithm=noop\",\n - \t\t \"fetch\", remote_name, \"--no-tags\",\n - \t\t \"--no-write-fetch-head\", \"--recurse-submodules=no\",\n --\t\t \"--filter=blob:none\", \"--stdin\", NULL);\n -+\t\t \"--filter=blob:none\", \"--refetch\", \"--stdin\", NULL);\n - \tif (!git_config_get_bool(\"promisor.quiet\", &quiet) && quiet)\n - \t\tstrvec_push(&child.args, \"--quiet\");\n - \tif (start_command(&child))\n + ## fetch-pack.c ##\n +@@ fetch-pack.c: static int mark_complete(const struct object_id *oid)\n + {\n + \tstruct commit *commit = deref_without_lazy_fetch(oid, 1);\n + \n +-\tif (commit && !(commit->object.flags & COMPLETE)) {\n ++\tif (commit &&\n ++\t !(commit->object.flags & COMPLETE) &&\n ++\t has_object(the_repository, oid, 0)) {\n + \t\tcommit->object.flags |= COMPLETE;\n + \t\tcommit_list_insert(commit, &complete);\n + \t}\n +\n + ## t/t0410-partial-clone.sh ##\n +@@ t/t0410-partial-clone.sh: test_expect_success 'fetching of missing objects works with ref-in-want enabled'\n + \tgrep \"fetch< fetch=.*ref-in-want\" trace\n + '\n + \n ++test_expect_success 'fetching missing objects pointed to by a local ref' '\n ++\trm -rf reliable-server unreliable-client &&\n ++\ttest_when_finished rm -rf reliable-server unreliable-client &&\n ++\ttest_create_repo reliable-server &&\n ++\tgit -C reliable-server config uploadpack.allowanysha1inwant 1 &&\n ++\tgit -C reliable-server config uploadpack.allowfilter 1 &&\n ++\ttest_commit -C reliable-server foo &&\n ++\n ++\tgit clone --filter=blob:none \"file://$(pwd)/reliable-server\" unreliable-client &&\n ++\n ++\t# to simulate the unreliable client losing a referenced object by\n ++\t# corruption, create the object on the server side, then create only a\n ++\t# reference to that object on the client side (without providing the\n ++\t# object itself).\n ++\ttest_commit -C reliable-server bar &&\n ++\tHASH=$(git -C reliable-server rev-parse HEAD) &&\n ++\techo \"$HASH\" >unreliable-client/.git/refs/heads/bar &&\n ++\n ++\t# the object is really missing\n ++\t# check if we can rev-parse a partial SHA. partial so we do not fetch it,\n ++\t# but barely partial (trim only the last char) so that we do not collide\n ++\ttest_must_fail git -C unreliable-client rev-parse ${HASH%%?} &&\n ++\n ++\t# trigger a remote fetch by checking out `bar`\n ++\tgit -C unreliable-client switch bar &&\n ++\n ++\t# and now we have the missing object\n ++\tgit -C unreliable-client rev-parse ${HASH%%?}\n ++'\n ++\n + test_expect_success 'fetching of missing objects from another promisor remote' '\n + \tgit clone \"file://$(pwd)/server\" server2 &&\n + \ttest_commit -C server2 bar &&\n-- \n2.47.0.105.g07ac214952-goog\n\n"},{"id":"505965","messageId":"CAJoAoZkcWVo1Hav1s-9Tqa7eddA8PcPxXdRjA4727LFYen83DA@mail.gmail.com","threadId":"62250","inReplyTo":"20241023002806.367082-1-emilyshaffer@google.com","subject":"Re: [PATCH v2] fetch-pack: don't mark COMPLETE unless we have the full object","fromName":"Emily Shaffer","fromEmail":"nasamuffin@google.com","sentAt":"2024-10-23T18:53:08Z","receivedAt":"2024-10-23T18:53:24Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"(I missed ccing reviewers from last round; adding now, although I know\nJunio is vacationing. Sorry about that.)\n\nOn Tue, Oct 22, 2024 at 5:28 PM Emily Shaffer wrote:\n>\n> When fetching, we decide which objects to skip asking for marking\n> certain commit IDs with the COMPLETE flag. This flag is set in\n> fetch-pack.c:mark_complete(), which is called from a few different\n> functions who decide what to mark or not mark. mark_complete() is\n> insulated against null pointer deref and repeatedly writing the same\n> commit to the list of objects to skip; because it's the central function\n> which decides that an object is COMPLETE and doesn't need to be fetched,\n> let's also insulate it against corruption where the object is not\n> present (even though we think it is).\n>\n> Without this check, it's possible to reach a corrupted state where\n> fetches can infinitely loop because we decide to skip fetching, but we\n> don't actually have the skipped commit in our object store.\n>\n> This manifested at $DAYJOB in a repo with the following features:\n> * blob-filtered partial clone enabled\n> * commit graph enabled\n> * ref Foo pointing to commit object 6aaaca\n> * object 6aaaca missing[1]\n>\n> With these prerequisites, we noticed that `git fetch` in the repo\n> produced an infinite loop:\n> 1. `git fetch` tries to fetch, but thinks it has all objects, so it\n> noops.\n> 2. At the end of cmd_fetch(), we try to write_commit_graph_reachable().\n> 3. write_commit_graph_reachable() does a reachability walk, including\n> starting from Foo\n> 4. The reachability walk tries to peel Foo, and notices it's missing\n> 6aaaca.\n> 5. The partial clone machinery asks for a per-object JIT fetch of\n> 6aaaca.\n> 6. `git fetch` (child process) is asked to fetch 6aaaca.\n> 7. We put together the_repository->parsed_objects, adding all commit IDs\n> reachable from local refs to it\n> (fetch-pack.c:mark_complete_and_common_refs(), trace region\n> mark_complete_local_refs). We see Foo, so we add 6aaaca to\n> the_repository->parsed_objects and mark it as COMPLETE.\n> 8. cmd_fetch notices that the object ID it was asked for is already\n> known, so does not fetch anything new.\n> 9. GOTO 2.\n>\n> The culprit is that we're assuming all local refs already must have\n> objects in place. Let's not assume that, and explicitly check\n> has_object() before marking objects as COMPLETE.\n>\n> NEEDSWORK: It could be valuable to emit a trace event when we try to\n> mark_complete() an object where we don't actually has_object() (to\n> understand how often we're having to heal from corruption).\n>\n> Signed-off-by: Emily Shaffer \n> Helped-by: Jonathan Tan \n>\n> 1: That commit object went missing as a byproduct of this partial clone\n> gc issue:\n> https://lore.kernel.org/git/20241001191811.1934900-1-calvinwan@google.com/\n>\n> ---\n>\n>\n> On the list, Junio and Robert Coup suggested that we should notice cases\n> when the list of object IDs we are trying to fetch gets filtered to\n> nothing during a lazy fetch. However, this turned out to be quite tricky\n> to implement - fetch-by-OID stores the OID in a ref object, so it's\n> challenging to guess that the ref we care about is an OID instead of a\n> normal ref. It also required some heuristic approach to catch the exact\n> moment after we removed COMPLETE objects from the list we wanted to\n> fetch, and to notice that the wrong objects were missing from that list.\n>\n> (From the list of alternatives I included with v1, v1 was approach iii;\n> Junio and Robert suggested iv; this patch holds something close to ii.)\n>\n> Jonathan Tan suggested that instead, we could be more careful about what\n> we mark COMPLETE or not; it seems like this is pretty straightforward to\n> do.\n>\n> I do wonder if it's a layering violation to do the has_object() check in\n> mark_complete(), which is otherwise a pretty stupid function; I included\n> it there because all of the other flavors of mark_complete_* eventually\n> boil down to mark_complete(), so we wouldn't need to remember to check\n> for object existence any other time we're trying to do this COMPLETE\n> marking thing.\n>\n> Note that I added a test to guarantee this behavior works, but without\n> commit graph enabled, the test passes both before and after; I guess\n> maybe it's better to add an explicit regression test for the\n> combination? But, this test fails with reftable - because we don't have\n> a way (that I know of) to force-create a ref with a bad ID, to force\n> this error condition. In the test as written I'm writing to\n> .git/refs/heads/bar directly; that doesn't work for reftable. But `git\n> update-ref` is too smart to let me set a ref to garbage. Any tips there\n> are welcome.\n\nI keep thinking about this test, and the more I think, the less\nvaluable I believe it is.\n\nI think we aren't super in the habit of writing regression tests, but\nwould it be that valuable to write a regression test in this case\ninstead? On the other hand, I think the code diff is quite obviously a\ngood idea, and we can see from the test suite that there isn't really\na performance hit from it. Is it necessary to add a test at all?\n\nOr, I guess that we could try to inspect how many fetch attempts were\nneeded to do the JIT fetch in this test. I suspect the number will be\ntoo high without this patch - I know it recurses at least more than\nonce. I dunno. Anybody have stronger opinions than me?\n\n>\n> The CI run is at\n> https://github.com/nasamuffin/git/actions/runs/11470039702 - it seems\n> the reftable tests are the only things failing.\n>\n> Also, I am sending this version, but if there are any additional\n> comments or it requires more changes, please expect Jonathan Tan to take\n> over driving this patch the rest of the way for me. As previously\n> stated[2], I'll be OOO after this Friday for most of the rest of the\n> year; the rest of this week I'm trying to get the rest of my\n> non-upstream loose ends tied up, so I won't have time to do another\n> iteration. See folks around Christmastime :)\n>\n> - Emily\n>\n> 2: https://lore.kernel.org/git/CAJoAoZnovapqMcu72DGR40jRRqRn57uJVTJg82kZ_rohtGDSfQ@mail.gmail.com/\n>\n> Cover letter from v1 follows:\n>\n> There are a few alternative approaches for this issue that I talked\n> about with some folks at $DAYJOB:\n>\n> i. Just disabling the commit graph rewrite allows this to fall\n> eventually into a path where the fetch actually succeeds. I didn't like\n> this solution - it's just whack-a-mole - so I didn't look too hard into\n> why it succeeds that way. It *could* make sense to disable commit graph\n> rewrite when we do a JIT fetch with blob or tree filter provided - but\n> if later we want to implement commit filter (something we've talked\n> about at Google) then I'd worry about this situation coming up again.\n>\n> ii. We could decide not to mark local refs (and commits reachable from\n> them) as COMPLETE in the_repository->parsed_objects. I didn't try this\n> solution out, and I'm not sure what the performance implications are,\n> but Jonathan Tan likes this solution, so I may try it out and see what\n> breaks shortly.\n>\n> iii. We could do all the JIT fetches with --refetch. In my opinion, this\n> is the safest/most self-healing solution; the JIT fetch only happens\n> when we really know we're missing the object, so it doesn't make sense\n> for that fetch to be canceled by any cache. It doesn't have performance\n> implications as far as I can guess (except that I think we still build\n> the parsed_objects hash even though we are going to ignore it, but we\n> already were doing that anyway). Of course, that's what this patch does.\n>\n> iv. We could do nothing; when cmd_fetch gets a fetch-by-object-id but\n> decides there is nothing more to do, it could terminate with an error.\n> That should stop the infinite recursion, and the error could suggest the\n> user to run `git fsck` and discover what the problem is. Depending on\n> the remediation we suggest, though, I think a direct fetch to fix this\n> particular loop would not work.\n>\n> I'm curious to hear thoughts from people who are more expert than me on\n> partial clone and fetching in general, though.\n>\n> This change is also still in RFC, for two reasons:\n>\n> First, it's intermittently failing tests for me locally, in weirdly\n> flaky ways:\n>\n> - t0410-partial-clone.sh fails when I run it from prove, but passes when\n> I run it manually, every time.\n> - t5601-clone.sh and t5505-remote.sh fail nonsensically on `rm -rf` that\n> should succeed (and does succeed if I stop the test with test_pause),\n> which makes me think there's something else borked in my setup, but\n> I'm not sure what.\n> - t5616-partial-clone.sh actually does fail in a way that I could see\n> having to do with this change (since I guess we might download more\n> packs than usual), but I was so confused by the other two errors I\n> haven't looked closely yet.\n>\n> And secondly, I didn't write tests verifying the breakage and that this\n> change fixes it yet, either.\n>\n> I'm going to work on both those things in the background, but I wanted\n> to get the description and RFC out early so that folks could take a look\n> and we could decide which approach is best.\n>\n> Thanks,\n> - Emily\n> ---\n> fetch-pack.c | 4 +++-\n> t/t0410-partial-clone.sh | 30 ++++++++++++++++++++++++++++++\n> 2 files changed, 33 insertions(+), 1 deletion(-)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index f752da93a8..8cb2ce4c54 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -603,7 +603,9 @@ static int mark_complete(const struct object_id *oid)\n> {\n> struct commit *commit = deref_without_lazy_fetch(oid, 1);\n>\n> - if (commit && !(commit->object.flags & COMPLETE)) {\n> + if (commit &&\n> + !(commit->object.flags & COMPLETE) &&\n> + has_object(the_repository, oid, 0)) {\n> commit->object.flags |= COMPLETE;\n> commit_list_insert(commit, &complete);\n> }\n> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\n> index 818700fbec..95de18ec40 100755\n> --- a/t/t0410-partial-clone.sh\n> +++ b/t/t0410-partial-clone.sh\n> @@ -241,6 +241,36 @@ test_expect_success 'fetching of missing objects works with ref-in-want enabled'\n> grep \"fetch< fetch=.*ref-in-want\" trace\n> '\n>\n> +test_expect_success 'fetching missing objects pointed to by a local ref' '\n> + rm -rf reliable-server unreliable-client &&\n> + test_when_finished rm -rf reliable-server unreliable-client &&\n> + test_create_repo reliable-server &&\n> + git -C reliable-server config uploadpack.allowanysha1inwant 1 &&\n> + git -C reliable-server config uploadpack.allowfilter 1 &&\n> + test_commit -C reliable-server foo &&\n> +\n> + git clone --filter=blob:none \"file://$(pwd)/reliable-server\" unreliable-client &&\n> +\n> + # to simulate the unreliable client losing a referenced object by\n> + # corruption, create the object on the server side, then create only a\n> + # reference to that object on the client side (without providing the\n> + # object itself).\n> + test_commit -C reliable-server bar &&\n> + HASH=$(git -C reliable-server rev-parse HEAD) &&\n> + echo \"$HASH\" >unreliable-client/.git/refs/heads/bar &&\n> +\n> + # the object is really missing\n> + # check if we can rev-parse a partial SHA. partial so we do not fetch it,\n> + # but barely partial (trim only the last char) so that we do not collide\n> + test_must_fail git -C unreliable-client rev-parse ${HASH%%?} &&\n> +\n> + # trigger a remote fetch by checking out `bar`\n> + git -C unreliable-client switch bar &&\n> +\n> + # and now we have the missing object\n> + git -C unreliable-client rev-parse ${HASH%%?}\n> +'\n> +\n> test_expect_success 'fetching of missing objects from another promisor remote' '\n> git clone \"file://$(pwd)/server\" server2 &&\n> test_commit -C server2 bar &&\n>\n> Range-diff against v1:\n> 1: 092be0a655 ! 1: 4db6bbb4cd promisor-remote: always JIT fetch with --refetch\n> - ## promisor-remote.c ##\n> -@@ promisor-remote.c: static int fetch_objects(struct repository *repo,\n> - strvec_pushl(&child.args, \"-c\", \"fetch.negotiationAlgorithm=noop\",\n> - \"fetch\", remote_name, \"--no-tags\",\n> - \"--no-write-fetch-head\", \"--recurse-submodules=no\",\n> -- \"--filter=blob:none\", \"--stdin\", NULL);\n> -+ \"--filter=blob:none\", \"--refetch\", \"--stdin\", NULL);\n> - if (!git_config_get_bool(\"promisor.quiet\", &quiet) && quiet)\n> - strvec_push(&child.args, \"--quiet\");\n> - if (start_command(&child))\n> + ## fetch-pack.c ##\n> +@@ fetch-pack.c: static int mark_complete(const struct object_id *oid)\n> + {\n> + struct commit *commit = deref_without_lazy_fetch(oid, 1);\n> +\n> +- if (commit && !(commit->object.flags & COMPLETE)) {\n> ++ if (commit &&\n> ++ !(commit->object.flags & COMPLETE) &&\n> ++ has_object(the_repository, oid, 0)) {\n> + commit->object.flags |= COMPLETE;\n> + commit_list_insert(commit, &complete);\n> + }\n> +\n> + ## t/t0410-partial-clone.sh ##\n> +@@ t/t0410-partial-clone.sh: test_expect_success 'fetching of missing objects works with ref-in-want enabled'\n> + grep \"fetch< fetch=.*ref-in-want\" trace\n> + '\n> +\n> ++test_expect_success 'fetching missing objects pointed to by a local ref' '\n> ++ rm -rf reliable-server unreliable-client &&\n> ++ test_when_finished rm -rf reliable-server unreliable-client &&\n> ++ test_create_repo reliable-server &&\n> ++ git -C reliable-server config uploadpack.allowanysha1inwant 1 &&\n> ++ git -C reliable-server config uploadpack.allowfilter 1 &&\n> ++ test_commit -C reliable-server foo &&\n> ++\n> ++ git clone --filter=blob:none \"file://$(pwd)/reliable-server\" unreliable-client &&\n> ++\n> ++ # to simulate the unreliable client losing a referenced object by\n> ++ # corruption, create the object on the server side, then create only a\n> ++ # reference to that object on the client side (without providing the\n> ++ # object itself).\n> ++ test_commit -C reliable-server bar &&\n> ++ HASH=$(git -C reliable-server rev-parse HEAD) &&\n> ++ echo \"$HASH\" >unreliable-client/.git/refs/heads/bar &&\n> ++\n> ++ # the object is really missing\n> ++ # check if we can rev-parse a partial SHA. partial so we do not fetch it,\n> ++ # but barely partial (trim only the last char) so that we do not collide\n> ++ test_must_fail git -C unreliable-client rev-parse ${HASH%%?} &&\n> ++\n> ++ # trigger a remote fetch by checking out `bar`\n> ++ git -C unreliable-client switch bar &&\n> ++\n> ++ # and now we have the missing object\n> ++ git -C unreliable-client rev-parse ${HASH%%?}\n> ++'\n> ++\n> + test_expect_success 'fetching of missing objects from another promisor remote' '\n> + git clone \"file://$(pwd)/server\" server2 &&\n> + test_commit -C server2 bar &&\n> --\n> 2.47.0.105.g07ac214952-goog\n>\n"},{"id":"505968","messageId":"ZxlYYQ9VULcqj/YT@nand.local","threadId":"62250","inReplyTo":"20241023002806.367082-1-emilyshaffer@google.com","subject":"Re: [PATCH v2] fetch-pack: don't mark COMPLETE unless we have the full object","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-23T20:11:13Z","receivedAt":"2024-10-23T20:11:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 22, 2024 at 05:28:05PM -0700, Emily Shaffer wrote:\n> This change is also still in RFC, for two reasons:\n>\n> First, it's intermittently failing tests for me locally, in weirdly\n> flaky ways:\n>\n> - t0410-partial-clone.sh fails when I run it from prove, but passes when\n> I run it manually, every time.\n> - t5601-clone.sh and t5505-remote.sh fail nonsensically on `rm -rf` that\n> should succeed (and does succeed if I stop the test with test_pause),\n> which makes me think there's something else borked in my setup, but\n> I'm not sure what.\n> - t5616-partial-clone.sh actually does fail in a way that I could see\n> having to do with this change (since I guess we might download more\n> packs than usual), but I was so confused by the other two errors I\n> haven't looked closely yet.\n>\n> And secondly, I didn't write tests verifying the breakage and that this\n> change fixes it yet, either.\n>\n> I'm going to work on both those things in the background, but I wanted\n> to get the description and RFC out early so that folks could take a look\n> and we could decide which approach is best.\n\nI am a little confused. Here you say that this patch is still in RFC,\nbut the subject line dropped the RFC present in the first round. What is\nthe state of this patch's readiness?\n\nThanks,\nTaylor\n"},{"id":"506266","messageId":"20241028225504.4151804-1-jonathantanmy@google.com","threadId":"62250","inReplyTo":"ZxlYYQ9VULcqj/YT@nand.local","subject":"Re: [PATCH v2] fetch-pack: don't mark COMPLETE unless we have the full object","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-28T22:55:04Z","receivedAt":"2024-10-28T22:55:07Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Taylor Blau writes:\n> > I'm going to work on both those things in the background, but I wanted\n> > to get the description and RFC out early so that folks could take a look\n> > and we could decide which approach is best.\n> \n> I am a little confused. Here you say that this patch is still in RFC,\n> but the subject line dropped the RFC present in the first round. What is\n> the state of this patch's readiness?\n> \n> Thanks,\n> Taylor\n\nAs Emily said [1], I'll be taking over driving this patch.\n\nThe tl;dr is this patch is not ready, so I think you (the interim\nmaintainer) can drop it.\n\nThis patch strives to avoid marking missing objects as COMPLETE by doing\na check in mark_complete(), but deref_without_lazy_fetch() already makes\nsuch a check (note how it can return NULL; also its name implies that\nit knows something about missing objects, namely that it says it won't\nlazy fetch them) so the question should be: why doesn't it return NULL\nwhen an object is missing? It turns out that it first checks the commit\ngraph file and if it's there, then it's considered to be present, so\nit does not fetch at all. However there are code paths during commit\ngraph writing (executed after every fetch, in builtin/fetch.c) that\naccess the object store directly without going through the commit graph\nfile (search for \"object_info\" in commit-graph.c), and those functions\nperform lazy fetches when the object is missing. So there's an infinite\nloop of \"commit graph writer reads X\" -> \"fetch X\" (nothing gets\nfetched) -> \"write new commit graph, as we always do after a fetch\" ->\n\"commit graph writer reads X\" -> ...\n\nSo my initial proposal to not mark objects as COMPLETE if they are\nmissing does not work, because they are already not marked as COMPLETE\nif they are missing. One could say that we should check both the commit\ngraph file and the object store (or, perhaps even better, only the\nobject store) before stating that an object is present, but I think\nthat Git already assumes in some places that a commit is present merely\nby its presence in the commit graph file, and it's not worth changing\nthis design.\n\nOne solution to fix this is to make the commit graph writer never\nlazy-fetch. This closes us off to being able to have missing commits\nin a partial clone (at least, if we also want to use the commit graph\nfile). This might be a reasonable thing to do - at least, partial clone\nhas been around for a few years and we've not made many concrete steps\ntowards that - but I feel that we shouldn't close ourselves off if\nthere's an alternative.\n\nThe alternative I'm currently thinking of is to detect if we didn't\nfetch any packfiles, and if we didn't, don't write a commit graph\n(and don't GC), much like we do when we have --negotiate-only. (A\npackfile-less fetch still can cause refs to be rewritten and thus reduce\nthe number of reachable objects, thus enabling a smaller commit graph to\nbe written and some objects to be GC-ed, but I think that this situation\nstill doesn't warrant commit graph writing and/or GC - we can just do\nthose next time.) The main issue is that we don't always know whether\na pack is written or not - in particular, if we use something other\nthan \"connect\" or \"stateless-connect\" on a remote helper, we won't know\nif a packfile was sent. We can solve this by (1) only write the commit\ngraph and GC if we know for sure that a packfile was sent, or (2) write\nthe commit graph and GC unless we know for sure that a packfile was\nnot sent. I'm leaning towards (1) because it seems more conceptually\ncoherent even though it is a change of behavior (from auto commit graph\nand GC to no), because I think that the repos that need scalability\nthe most already use protocol v2 during fetching (which does require\n\"connect\" or \"stateless-connect\" from remote helpers, so we're covered\nhere), but am OK with (2) as well.\n\nFeel free to let me know if you have any ideas. In the meantime I'll\nlook at (1).\n\n[1] https://lore.kernel.org/git/20241023002806.367082-1-emilyshaffer@google.com/\n"},{"id":"506303","messageId":"cover.1730235646.git.jonathantanmy@google.com","threadId":"62250","inReplyTo":"20241003223546.1935471-1-emilyshaffer@google.com","subject":"[PATCH 0/2] When fetching, warn if in commit graph but not obj db","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-29T21:11:03Z","receivedAt":"2024-10-29T21:11:10Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"I mentioned previously [1] the possibility of not running maintenance\nsteps (commit graph writing and \"git maintenance\") if no packs were\nfetched, but looking at things again, I think that we shouldn't do\nthat - in particular, if I ran \"git fetch --refetch\", I would fully\nexpect the objects to be repacked, even if Git wasn't able to detect\nconclusively whether a pack was transmitted.\n\nSo I went back to my original idea of detecting when an object is\nmissing. In trying to balance the concerns of both doing something as\nreasonable as possible in such a repo corruption case, and not slowing\ndown and/or unnecessarily complicating the main code flow, I decided\nto detect when an object is present in the commit graph but not in the\nobject DB, and to limit this detection for objects specified in the\nfetch refspec.\n\nUpon detection, we can't fix it due to reasons mentioned in the commit\nmessage, so I decided to print a warning. An alternate option is to make\nit a fatal error (instead of a warning) if an object is detected to be\nin the commit graph but not the object DB. I haven't thought through the\nramifications of that, though.\n\n[1] https://lore.kernel.org/git/20241028225504.4151804-1-jonathantanmy@google.com/\n\nJonathan Tan (2):\n Revert \"fetch-pack: add a deref_without_lazy_fetch_extended()\"\n fetch-pack: warn if in commit graph but not obj db\n\n fetch-pack.c | 45 +++++++++++++++++++++++++--------------------\n object.h | 2 +-\n 2 files changed, 26 insertions(+), 21 deletions(-)\n\n-- \n2.47.0.163.g1226f6d8fa-goog\n\n"},{"id":"506304","messageId":"4dea8933cf05a5020da7d78c088f4b091100952c.1730235646.git.jonathantanmy@google.com","threadId":"62250","inReplyTo":"cover.1730235646.git.jonathantanmy@google.com","subject":"[PATCH 1/2] Revert \"fetch-pack: add a deref_without_lazy_fetch_extended()\"","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-29T21:11:04Z","receivedAt":"2024-10-29T21:11:12Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This reverts commit a6e65fb39caf18259c660c1c7910d5bf80bc15cb.\n\nThe commit message of that commit mentions that the new function \"will\nbe used for the bundle-uri client in a subsequent commit\", but it seems\nthat eventually it wasn't used.\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 25 +++++++------------------\n 1 file changed, 7 insertions(+), 18 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex f752da93a8..6728a0d2f5 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -122,12 +122,11 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,\n \t\tcb(negotiator, cache.items[i]);\n }\n \n-static struct commit *deref_without_lazy_fetch_extended(const struct object_id *oid,\n-\t\t\t\t\t\t\tint mark_tags_complete,\n-\t\t\t\t\t\t\tenum object_type *type,\n-\t\t\t\t\t\t\tunsigned int oi_flags)\n+static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n+\t\t\t\t\t int mark_tags_complete)\n {\n-\tstruct object_info info = { .typep = type };\n+\tenum object_type type;\n+\tstruct object_info info = { .typep = &type };\n \tstruct commit *commit;\n \n \tcommit = lookup_commit_in_graph(the_repository, oid);\n@@ -136,9 +135,9 @@ static struct commit *deref_without_lazy_fetch_extended(const struct object_id *\n \n \twhile (1) {\n \t\tif (oid_object_info_extended(the_repository, oid, &info,\n-\t\t\t\t\t oi_flags))\n+\t\t\t\t\t OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK))\n \t\t\treturn NULL;\n-\t\tif (*type == OBJ_TAG) {\n+\t\tif (type == OBJ_TAG) {\n \t\t\tstruct tag *tag = (struct tag *)\n \t\t\t\tparse_object(the_repository, oid);\n \n@@ -152,7 +151,7 @@ static struct commit *deref_without_lazy_fetch_extended(const struct object_id *\n \t\t}\n \t}\n \n-\tif (*type == OBJ_COMMIT) {\n+\tif (type == OBJ_COMMIT) {\n \t\tstruct commit *commit = lookup_commit(the_repository, oid);\n \t\tif (!commit || repo_parse_commit(the_repository, commit))\n \t\t\treturn NULL;\n@@ -162,16 +161,6 @@ static struct commit *deref_without_lazy_fetch_extended(const struct object_id *\n \treturn NULL;\n }\n \n-\n-static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n-\t\t\t\t\t int mark_tags_complete)\n-{\n-\tenum object_type type;\n-\tunsigned flags = OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK;\n-\treturn deref_without_lazy_fetch_extended(oid, mark_tags_complete,\n-\t\t\t\t\t\t &type, flags);\n-}\n-\n static int rev_list_insert_ref(struct fetch_negotiator *negotiator,\n \t\t\t const struct object_id *oid)\n {\n-- \n2.47.0.163.g1226f6d8fa-goog\n\n"},{"id":"506305","messageId":"1027ff2cb7d9af5cc9ce6b653d28150457db8703.1730235646.git.jonathantanmy@google.com","threadId":"62250","inReplyTo":"cover.1730235646.git.jonathantanmy@google.com","subject":"[PATCH 2/2] fetch-pack: warn if in commit graph but not obj db","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-29T21:11:05Z","receivedAt":"2024-10-29T21:11:14Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When fetching, there is a step in which sought objects are first checked\nagainst the local repository; only objects that are not in the local\nrepository are then fetched. This check first looks up the commit graph\nfile, and returns \"present\" if the object is in there.\n\nHowever, the action of first looking up the commit graph file is not\ndone everywhere in Git, especially if the type of the object at the time\nof lookup is not known. This means that in a repo corruption situation,\na user may encounter an \"object missing\" error, attempt to fetch it, and\nstill encounter the same error later when they reattempt their original\naction, because the object is present in the commit graph file but not in\nthe object DB.\n\nTherefore, detect when this occurs and print a warning. (Note that\nwe cannot proceed to include this object in the list of objects to\nbe fetched without changing at least the fetch negotiation code:\nwhat would happen is that the client will send \"want X\" and \"have X\"\nand when I tested at $DAYJOB with a work server that uses JGit, the\nserver reasonably returned an empty packfile. And changing the fetch\nnegotiation code to only use the object DB when deciding what to report\nas \"have\" would be an unnecessary slowdown, I think.)\n\nThis was discovered when a lazy fetch of a missing commit completed with\nnothing actually fetched, and the writing of the commit graph file after\nevery fetch then attempted to read said missing commit, triggering a\nlazy fetch of said missing commit, resulting in an infinite loop with no\nuser-visible indication (until they check the list of processes running\non their computer). With this fix, at least a warning message will be\nprinted. Note that although the repo corruption we discovered was caused\nby a bug in GC in a partial clone, the behavior that this patch teaches\nGit to warn about applies to any repo with commit graph enabled and with\na missing commit, whether it is a partial clone or not.\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 22 +++++++++++++++++++---\n object.h | 2 +-\n 2 files changed, 20 insertions(+), 4 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 6728a0d2f5..5a0020366b 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -57,6 +57,7 @@ static struct string_list uri_protocols = STRING_LIST_INIT_DUP;\n #define ALTERNATE\t(1U << 1)\n #define COMMON\t\t(1U << 6)\n #define REACH_SCRATCH\t(1U << 7)\n+#define COMPLETE_FROM_COMMIT_GRAPH\t(1U << 8)\n \n /*\n * After sending this many \"have\"s if we do not get any new ACK , we\n@@ -123,15 +124,18 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,\n }\n \n static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n-\t\t\t\t\t int mark_tags_complete)\n+\t\t\t\t\t int mark_additional_complete_information)\n {\n \tenum object_type type;\n \tstruct object_info info = { .typep = &type };\n \tstruct commit *commit;\n \n \tcommit = lookup_commit_in_graph(the_repository, oid);\n-\tif (commit)\n+\tif (commit) {\n+\t\tif (mark_additional_complete_information)\n+\t\t\tcommit->object.flags |= COMPLETE_FROM_COMMIT_GRAPH;\n \t\treturn commit;\n+\t}\n \n \twhile (1) {\n \t\tif (oid_object_info_extended(the_repository, oid, &info,\n@@ -143,7 +147,7 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n \n \t\t\tif (!tag->tagged)\n \t\t\t\treturn NULL;\n-\t\t\tif (mark_tags_complete)\n+\t\t\tif (mark_additional_complete_information)\n \t\t\t\ttag->object.flags |= COMPLETE;\n \t\t\toid = &tag->tagged->oid;\n \t\t} else {\n@@ -809,6 +813,14 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \tsave_commit_buffer = old_save_commit_buffer;\n }\n \n+static void warn_in_commit_graph_only(const struct object_id *oid)\n+{\n+\twarning(_(\"You are attempting to fetch %s, which is in the commit graph file but not in the object database.\"),\n+\t\toid_to_hex(oid));\n+\twarning(_(\"This is probably due to repo corruption.\"));\n+\twarning(_(\"If you are attempting to repair this repo corruption by refetching the missing object, use 'git fetch --refetch' with the missing object.\"));\n+}\n+\n /*\n * Returns 1 if every object pointed to by the given remote refs is available\n * locally and reachable from a local ref, and 0 otherwise.\n@@ -830,6 +842,10 @@ static int everything_local(struct fetch_pack_args *args,\n \t\t\t\t ref->name);\n \t\t\tcontinue;\n \t\t}\n+\t\tif (o->flags & COMPLETE_FROM_COMMIT_GRAPH) {\n+\t\t\tif (!has_object(the_repository, remote, 0))\n+\t\t\t\twarn_in_commit_graph_only(remote);\n+\t\t}\n \t\tprint_verbose(args, _(\"already have %s (%s)\"), oid_to_hex(remote),\n \t\t\t ref->name);\n \t}\ndiff --git a/object.h b/object.h\nindex 17f32f1103..196e489253 100644\n--- a/object.h\n+++ b/object.h\n@@ -65,7 +65,7 @@ void object_array_init(struct object_array *array);\n /*\n * object flag allocation:\n * revision.h: 0---------10 15 23------27\n- * fetch-pack.c: 01 67\n+ * fetch-pack.c: 01 6-8\n * negotiator/default.c: 2--5\n * walker.c: 0-2\n * upload-pack.c: 4 11-----14 16-----19\n-- \n2.47.0.163.g1226f6d8fa-goog\n\n"},{"id":"506339","messageId":"ikraw375qal5ioopai5wk3i5vtpnyurxxd2ixsdujehpypuova@in5idqkd6wee","threadId":"62250","inReplyTo":"cover.1730235646.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/2] When fetching, warn if in commit graph but not obj db","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-10-30T21:22:29Z","receivedAt":"2024-10-30T21:22:35Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.10.29 14:11, Jonathan Tan wrote:\n> I mentioned previously [1] the possibility of not running maintenance\n> steps (commit graph writing and \"git maintenance\") if no packs were\n> fetched, but looking at things again, I think that we shouldn't do\n> that - in particular, if I ran \"git fetch --refetch\", I would fully\n> expect the objects to be repacked, even if Git wasn't able to detect\n> conclusively whether a pack was transmitted.\n> \n> [1] https://lore.kernel.org/git/20241028225504.4151804-1-jonathantanmy@google.com/\n\nA note for upstream, because I'm not sure it was ever explicitly\nmentioned: at $DAYJOB, we saw this fetch recursion error as a\nside-effect of the erroneous GC of local commits discussed at [2].\n\n[2] https://lore.kernel.org/git/cover.1729792911.git.jonathantanmy@google.com/\n\n> So I went back to my original idea of detecting when an object is\n> missing. In trying to balance the concerns of both doing something as\n> reasonable as possible in such a repo corruption case, and not slowing\n> down and/or unnecessarily complicating the main code flow, I decided\n> to detect when an object is present in the commit graph but not in the\n> object DB, and to limit this detection for objects specified in the\n> fetch refspec.\n> \n> Upon detection, we can't fix it due to reasons mentioned in the commit\n> message, so I decided to print a warning. An alternate option is to make\n> it a fatal error (instead of a warning) if an object is detected to be\n> in the commit graph but not the object DB. I haven't thought through the\n> ramifications of that, though.\n\nAt first glance, I lean towards making this a fatal error, but I'll try\nthinking out loud a bit:\n\nFirst, we believe that [2] above should fix the root cause of the\nparticular case we saw at $DAYJOB (hopefully this type of error doesn't\nhave multiple root causes). So we expect to basically never encounter\nthis error again after [2] is merged and rolled out, and all existing\ncases of repo corruption have been repaired. However, interacting with a\nbroken repo even with a client that includes [2] would still hit this\ncondition and issue a warning.\n\nWith the current implementation, fetching in a corrupt repo would still\ncause git-fetch to infinitely recurse, and therefore would repeatedly\nprint the same error message to the console, until either the user\nnoticed, or we fail to launch a new git-fetch process due to resource\nexhaustion.\n\nI don't see any reason why the above situation is more friendly or\ndesirable than exiting (with the same error message) as soon as we\ndetect this type of corruption. However, I don't feel super strongly\nabout it. If the rest of the list is OK with repeated error messages,\nthen I can live with it.\n\n> Jonathan Tan (2):\n> Revert \"fetch-pack: add a deref_without_lazy_fetch_extended()\"\n> fetch-pack: warn if in commit graph but not obj db\n> \n> fetch-pack.c | 45 +++++++++++++++++++++++++--------------------\n> object.h | 2 +-\n> 2 files changed, 26 insertions(+), 21 deletions(-)\n> \n> -- \n> 2.47.0.163.g1226f6d8fa-goog\n> \n> \n"},{"id":"506340","messageId":"ejrw5wyxianyz5wkwvfw4dhssg23lmxlhgbo7bbzwif742uk5j@rni2fojw6py7","threadId":"62250","inReplyTo":"4dea8933cf05a5020da7d78c088f4b091100952c.1730235646.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/2] Revert \"fetch-pack: add a deref_without_lazy_fetch_extended()\"","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-10-30T21:22:40Z","receivedAt":"2024-10-30T21:22:45Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.10.29 14:11, Jonathan Tan wrote:\n> This reverts commit a6e65fb39caf18259c660c1c7910d5bf80bc15cb.\n> \n> The commit message of that commit mentions that the new function \"will\n> be used for the bundle-uri client in a subsequent commit\", but it seems\n> that eventually it wasn't used.\n> \n> Signed-off-by: Jonathan Tan \n> ---\n> fetch-pack.c | 25 +++++++------------------\n> 1 file changed, 7 insertions(+), 18 deletions(-)\n\nNit: can you mention in the commit description that this cleanup\nsimplifies a later patch to detect a case of repo corruption?\n"},{"id":"506341","messageId":"bzhg2a7mv2xrbahk6o5kpijx4dxmpkm4wrrjhatetowjdowout@hesucnp6cikf","threadId":"62250","inReplyTo":"1027ff2cb7d9af5cc9ce6b653d28150457db8703.1730235646.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] fetch-pack: warn if in commit graph but not obj db","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-10-30T21:22:52Z","receivedAt":"2024-10-30T21:22:57Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.10.29 14:11, Jonathan Tan wrote:\n> When fetching, there is a step in which sought objects are first checked\n> against the local repository; only objects that are not in the local\n> repository are then fetched. This check first looks up the commit graph\n> file, and returns \"present\" if the object is in there.\n> \n> However, the action of first looking up the commit graph file is not\n> done everywhere in Git, especially if the type of the object at the time\n> of lookup is not known. This means that in a repo corruption situation,\n> a user may encounter an \"object missing\" error, attempt to fetch it, and\n> still encounter the same error later when they reattempt their original\n> action, because the object is present in the commit graph file but not in\n> the object DB.\n> \n> Therefore, detect when this occurs and print a warning. (Note that\n> we cannot proceed to include this object in the list of objects to\n> be fetched without changing at least the fetch negotiation code:\n> what would happen is that the client will send \"want X\" and \"have X\"\n> and when I tested at $DAYJOB with a work server that uses JGit, the\n> server reasonably returned an empty packfile. And changing the fetch\n> negotiation code to only use the object DB when deciding what to report\n> as \"have\" would be an unnecessary slowdown, I think.)\n> \n> This was discovered when a lazy fetch of a missing commit completed with\n> nothing actually fetched, and the writing of the commit graph file after\n> every fetch then attempted to read said missing commit, triggering a\n> lazy fetch of said missing commit, resulting in an infinite loop with no\n> user-visible indication (until they check the list of processes running\n> on their computer). With this fix, at least a warning message will be\n> printed. Note that although the repo corruption we discovered was caused\n> by a bug in GC in a partial clone, the behavior that this patch teaches\n> Git to warn about applies to any repo with commit graph enabled and with\n> a missing commit, whether it is a partial clone or not.\n> \n> Signed-off-by: Jonathan Tan \n> ---\n> fetch-pack.c | 22 +++++++++++++++++++---\n> object.h | 2 +-\n> 2 files changed, 20 insertions(+), 4 deletions(-)\n> \n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 6728a0d2f5..5a0020366b 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -57,6 +57,7 @@ static struct string_list uri_protocols = STRING_LIST_INIT_DUP;\n> #define ALTERNATE\t(1U << 1)\n> #define COMMON\t\t(1U << 6)\n> #define REACH_SCRATCH\t(1U << 7)\n> +#define COMPLETE_FROM_COMMIT_GRAPH\t(1U << 8)\n\nWe're defining a new flag, and we note it in object.h as well below, so\nlooks good so far.\n\n\n> /*\n> * After sending this many \"have\"s if we do not get any new ACK , we\n> @@ -123,15 +124,18 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,\n> }\n> \n> static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n> -\t\t\t\t\t int mark_tags_complete)\n> +\t\t\t\t\t int mark_additional_complete_information)\n\nWe're already marking some completion flags here, so we're just making\nthe parameter name more descriptive, OK.\n\n\n> {\n> \tenum object_type type;\n> \tstruct object_info info = { .typep = &type };\n> \tstruct commit *commit;\n> \n> \tcommit = lookup_commit_in_graph(the_repository, oid);\n> -\tif (commit)\n> +\tif (commit) {\n> +\t\tif (mark_additional_complete_information)\n> +\t\t\tcommit->object.flags |= COMPLETE_FROM_COMMIT_GRAPH;\n> \t\treturn commit;\n> +\t}\n\nWe already have a case where we're checking the commit graph, so we can\nalso mark the commit complete here... well, not the original \"COMPLETE\"\nflag since we don't want to change behavior, but our new\nCOMPLETE_FROM_COMMIT_GRAPH flag. Sounds good.\n\n\n> \n> \twhile (1) {\n> \t\tif (oid_object_info_extended(the_repository, oid, &info,\n> @@ -143,7 +147,7 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n> \n> \t\t\tif (!tag->tagged)\n> \t\t\t\treturn NULL;\n> -\t\t\tif (mark_tags_complete)\n> +\t\t\tif (mark_additional_complete_information)\n> \t\t\t\ttag->object.flags |= COMPLETE;\n> \t\t\toid = &tag->tagged->oid;\n> \t\t} else {\n> @@ -809,6 +813,14 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n> \tsave_commit_buffer = old_save_commit_buffer;\n> }\n> \n> +static void warn_in_commit_graph_only(const struct object_id *oid)\n> +{\n> +\twarning(_(\"You are attempting to fetch %s, which is in the commit graph file but not in the object database.\"),\n> +\t\toid_to_hex(oid));\n> +\twarning(_(\"This is probably due to repo corruption.\"));\n> +\twarning(_(\"If you are attempting to repair this repo corruption by refetching the missing object, use 'git fetch --refetch' with the missing object.\"));\n> +}\n> +\n\nHere's the new warning. As mentioned in my reply to the cover letter, I\nfeel like it makes more sense to die(), but I don't feel too strongly\nabout it.\n\n\n> /*\n> * Returns 1 if every object pointed to by the given remote refs is available\n> * locally and reachable from a local ref, and 0 otherwise.\n> @@ -830,6 +842,10 @@ static int everything_local(struct fetch_pack_args *args,\n> \t\t\t\t ref->name);\n> \t\t\tcontinue;\n> \t\t}\n> +\t\tif (o->flags & COMPLETE_FROM_COMMIT_GRAPH) {\n> +\t\t\tif (!has_object(the_repository, remote, 0))\n> +\t\t\t\twarn_in_commit_graph_only(remote);\n> +\t\t}\n\nAnd now that we're checking what's local, we issue our warning if we\nhave an object missing from the DB but mentioned in the commit graph.\nSeems fine, although I wonder if it makes more sense to fail earlier. It\nlooks like the only place we do the\n`mark_additional_complete_information` checks is in `mark_complete()`,\nso should we just check this condition there? No strong feelings either\nway, just curious.\n\n\n> \t\tprint_verbose(args, _(\"already have %s (%s)\"), oid_to_hex(remote),\n> \t\t\t ref->name);\n> \t}\n> diff --git a/object.h b/object.h\n> index 17f32f1103..196e489253 100644\n> --- a/object.h\n> +++ b/object.h\n> @@ -65,7 +65,7 @@ void object_array_init(struct object_array *array);\n> /*\n> * object flag allocation:\n> * revision.h: 0---------10 15 23------27\n> - * fetch-pack.c: 01 67\n> + * fetch-pack.c: 01 6-8\n> * negotiator/default.c: 2--5\n> * walker.c: 0-2\n> * upload-pack.c: 4 11-----14 16-----19\n> -- \n> 2.47.0.163.g1226f6d8fa-goog\n> \n> \n"},{"id":"506407","messageId":"ZyPvqPK1s5lUtH+N@nand.local","threadId":"62250","inReplyTo":"1027ff2cb7d9af5cc9ce6b653d28150457db8703.1730235646.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] fetch-pack: warn if in commit graph but not obj db","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-31T20:59:20Z","receivedAt":"2024-10-31T20:59:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 29, 2024 at 02:11:05PM -0700, Jonathan Tan wrote:\n> When fetching, there is a step in which sought objects are first checked\n> against the local repository; only objects that are not in the local\n> repository are then fetched. This check first looks up the commit graph\n> file, and returns \"present\" if the object is in there.\n\nOK.\n\n> However, the action of first looking up the commit graph file is not\n> done everywhere in Git, especially if the type of the object at the time\n> of lookup is not known. This means that in a repo corruption situation,\n> a user may encounter an \"object missing\" error, attempt to fetch it, and\n> still encounter the same error later when they reattempt their original\n> action, because the object is present in the commit graph file but not in\n> the object DB.\n\nI think the type of repository corruption here may be underspecified.\n\nYou say that we have some object, say X, whose type is not known. So we\ndon't load the commit-graph, realize that X is missing, and then try and\nfetch it. In this scenario, is X actually in the commit-graph, but not\nin the object database? Further, if X is in the commit-graph, I assume\nwe do not look it up there because we first try and find its type, which\nfails, so we assume we don't have it (despite it appearing corruptly in\nthe commit-graph)?\n\nI think that matches the behavior you're describing, but I want to make\nsure that I'm not thinking of something else.\n\n> This was discovered when a lazy fetch of a missing commit completed with\n> nothing actually fetched, and the writing of the commit graph file after\n> every fetch then attempted to read said missing commit, triggering a\n> lazy fetch of said missing commit, resulting in an infinite loop with no\n> user-visible indication (until they check the list of processes running\n> on their computer).\n\nYuck :-).\n\n> Signed-off-by: Jonathan Tan \n> ---\n> fetch-pack.c | 22 +++++++++++++++++++---\n> object.h | 2 +-\n> 2 files changed, 20 insertions(+), 4 deletions(-)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 6728a0d2f5..5a0020366b 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -57,6 +57,7 @@ static struct string_list uri_protocols = STRING_LIST_INIT_DUP;\n> #define ALTERNATE\t(1U << 1)\n> #define COMMON\t\t(1U << 6)\n> #define REACH_SCRATCH\t(1U << 7)\n> +#define COMPLETE_FROM_COMMIT_GRAPH\t(1U << 8)\n>\n> /*\n> * After sending this many \"have\"s if we do not get any new ACK , we\n> @@ -123,15 +124,18 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,\n> }\n>\n> static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n> -\t\t\t\t\t int mark_tags_complete)\n> +\t\t\t\t\t int mark_additional_complete_information)\n> {\n> \tenum object_type type;\n> \tstruct object_info info = { .typep = &type };\n> \tstruct commit *commit;\n>\n> \tcommit = lookup_commit_in_graph(the_repository, oid);\n> -\tif (commit)\n> +\tif (commit) {\n> +\t\tif (mark_additional_complete_information)\n> +\t\t\tcommit->object.flags |= COMPLETE_FROM_COMMIT_GRAPH;\n> \t\treturn commit;\n> +\t}\n>\n> \twhile (1) {\n> \t\tif (oid_object_info_extended(the_repository, oid, &info,\n> @@ -143,7 +147,7 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n>\n> \t\t\tif (!tag->tagged)\n> \t\t\t\treturn NULL;\n> -\t\t\tif (mark_tags_complete)\n> +\t\t\tif (mark_additional_complete_information)\n> \t\t\t\ttag->object.flags |= COMPLETE;\n> \t\t\toid = &tag->tagged->oid;\n> \t\t} else {\n> @@ -809,6 +813,14 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n> \tsave_commit_buffer = old_save_commit_buffer;\n> }\n>\n> +static void warn_in_commit_graph_only(const struct object_id *oid)\n> +{\n> +\twarning(_(\"You are attempting to fetch %s, which is in the commit graph file but not in the object database.\"),\n> +\t\toid_to_hex(oid));\n> +\twarning(_(\"This is probably due to repo corruption.\"));\n> +\twarning(_(\"If you are attempting to repair this repo corruption by refetching the missing object, use 'git fetch --refetch' with the missing object.\"));\n> +}\n> +\n> /*\n> * Returns 1 if every object pointed to by the given remote refs is available\n> * locally and reachable from a local ref, and 0 otherwise.\n> @@ -830,6 +842,10 @@ static int everything_local(struct fetch_pack_args *args,\n> \t\t\t\t ref->name);\n> \t\t\tcontinue;\n> \t\t}\n> +\t\tif (o->flags & COMPLETE_FROM_COMMIT_GRAPH) {\n> +\t\t\tif (!has_object(the_repository, remote, 0))\n> +\t\t\t\twarn_in_commit_graph_only(remote);\n\nYou discuss this a little bit in your commit message, but I wonder if we\nshould just die() here. I feel like we're trying to work around a\nsituation where the commit-graph is obviously broken because it refers\nto commit objects that don't actually exist in the object store.\n\nA few thoughts in this area:\n\n - What situation provokes this to be true? I could imagine there is\n some bug that we don't fully have a grasp of. But I wonder if it is\n even easier to provoke than that, say by pruning some objects out of\n the object store, then not rewriting the commit-graph, leaving some\n of the references dangling.\n\n - Does 'git fsck' catch this case within the commit-graph?\n\n - Are the other areas of the code that rely on the assumption that all\n entries in the commit-graph actually exist on disk? If so, are they\n similarly broken?\n\nAnother thought about this whole thing is that we essentially have a\ncode path that says: \"I found this object from the commit-graph, but\ndon't know if I actually have it on disk, so mark it to be checked later\nvia has_object()\".\n\nI wonder if it would be more straightforward to replace the call to\nlookup_commit_in_graph() with a direct call to has_object() in the\nderef_without_lazy_fetch() function, which I think would both (a)\neliminate the need for a new flag bit to be allocated, and (b) prevent\nlooking up the object twice.\n\nThoughts?\n\nThanks,\nTaylor\n"},{"id":"506410","messageId":"cover.1730409376.git.jonathantanmy@google.com","threadId":"62250","inReplyTo":"cover.1730235646.git.jonathantanmy@google.com","subject":"[PATCH v2 0/2] When fetching, die if in commit graph but not obj db","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-31T21:18:59Z","receivedAt":"2024-10-31T21:19:05Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks everyone for your review comments. I've updated the patch 1\ncommit message as Josh requested. I'll reply individually to comments on\npatch 2.\n\nJonathan Tan (2):\n Revert \"fetch-pack: add a deref_without_lazy_fetch_extended()\"\n fetch-pack: warn if in commit graph but not obj db\n\n fetch-pack.c | 42 +++++++++++-----------\n t/t5330-no-lazy-fetch-with-commit-graph.sh | 2 +-\n 2 files changed, 23 insertions(+), 21 deletions(-)\n\nRange-diff against v1:\n1: 4dea8933cf ! 1: 34e87b8388 Revert \"fetch-pack: add a deref_without_lazy_fetch_extended()\"\n @@ Commit message\n \n This reverts commit a6e65fb39caf18259c660c1c7910d5bf80bc15cb.\n \n + This revert simplifies the next patch in this patch set.\n +\n The commit message of that commit mentions that the new function \"will\n be used for the bundle-uri client in a subsequent commit\", but it seems\n that eventually it wasn't used.\n2: 1027ff2cb7 ! 2: 631b9a8677 fetch-pack: warn if in commit graph but not obj db\n @@ Commit message\n action, because the object is present in the commit graph file but not in\n the object DB.\n \n - Therefore, detect when this occurs and print a warning. (Note that\n - we cannot proceed to include this object in the list of objects to\n - be fetched without changing at least the fetch negotiation code:\n - what would happen is that the client will send \"want X\" and \"have X\"\n - and when I tested at $DAYJOB with a work server that uses JGit, the\n - server reasonably returned an empty packfile. And changing the fetch\n - negotiation code to only use the object DB when deciding what to report\n - as \"have\" would be an unnecessary slowdown, I think.)\n + Therefore, make it a fatal error when this occurs. (Note that we cannot\n + proceed to include this object in the list of objects to be fetched\n + without changing at least the fetch negotiation code: what would happen\n + is that the client will send \"want X\" and \"have X\" and when I tested\n + at $DAYJOB with a work server that uses JGit, the server reasonably\n + returned an empty packfile. And changing the fetch negotiation code to\n + only use the object DB when deciding what to report as \"have\" would be\n + an unnecessary slowdown, I think.)\n \n This was discovered when a lazy fetch of a missing commit completed with\n nothing actually fetched, and the writing of the commit graph file after\n every fetch then attempted to read said missing commit, triggering a\n lazy fetch of said missing commit, resulting in an infinite loop with no\n user-visible indication (until they check the list of processes running\n - on their computer). With this fix, at least a warning message will be\n - printed. Note that although the repo corruption we discovered was caused\n - by a bug in GC in a partial clone, the behavior that this patch teaches\n - Git to warn about applies to any repo with commit graph enabled and with\n - a missing commit, whether it is a partial clone or not.\n + on their computer). With this fix, there is no infinite loop. Note that\n + although the repo corruption we discovered was caused by a bug in GC in\n + a partial clone, the behavior that this patch teaches Git to warn about\n + applies to any repo with commit graph enabled and with a missing commit,\n + whether it is a partial clone or not.\n +\n + t5330, introduced in 3a1ea94a49 (commit-graph.c: no lazy fetch in\n + lookup_commit_in_graph(), 2022-07-01), tests that an interaction between\n + fetch and the commit graph does not cause an infinite loop. This patch\n + changes the exit code in that situation, so that test had to be changed.\n \n Signed-off-by: Jonathan Tan \n \n ## fetch-pack.c ##\n -@@ fetch-pack.c: static struct string_list uri_protocols = STRING_LIST_INIT_DUP;\n - #define ALTERNATE\t(1U << 1)\n - #define COMMON\t\t(1U << 6)\n - #define REACH_SCRATCH\t(1U << 7)\n -+#define COMPLETE_FROM_COMMIT_GRAPH\t(1U << 8)\n - \n - /*\n - * After sending this many \"have\"s if we do not get any new ACK , we\n @@ fetch-pack.c: static void for_each_cached_alternate(struct fetch_negotiator *negotiator,\n + \t\tcb(negotiator, cache.items[i]);\n }\n \n ++static void die_in_commit_graph_only(const struct object_id *oid)\n ++{\n ++\tdie(_(\"You are attempting to fetch %s, which is in the commit graph file but not in the object database.\\n\"\n ++\t \"This is probably due to repo corruption.\\n\"\n ++\t \"If you are attempting to repair this repo corruption by refetching the missing object, use 'git fetch --refetch' with the missing object.\"),\n ++\t oid_to_hex(oid));\n ++}\n ++\n static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n -\t\t\t\t\t int mark_tags_complete)\n -+\t\t\t\t\t int mark_additional_complete_information)\n ++\t\t\t\t\t int mark_tags_complete_and_check_obj_db)\n {\n \tenum object_type type;\n \tstruct object_info info = { .typep = &type };\n @@ fetch-pack.c: static void for_each_cached_alternate(struct fetch_negotiator *neg\n \tcommit = lookup_commit_in_graph(the_repository, oid);\n -\tif (commit)\n +\tif (commit) {\n -+\t\tif (mark_additional_complete_information)\n -+\t\t\tcommit->object.flags |= COMPLETE_FROM_COMMIT_GRAPH;\n ++\t\tif (mark_tags_complete_and_check_obj_db) {\n ++\t\t\tif (!has_object(the_repository, oid, 0))\n ++\t\t\t\tdie_in_commit_graph_only(oid);\n ++\t\t}\n \t\treturn commit;\n +\t}\n \n @@ fetch-pack.c: static struct commit *deref_without_lazy_fetch(const struct object\n \t\t\tif (!tag->tagged)\n \t\t\t\treturn NULL;\n -\t\t\tif (mark_tags_complete)\n -+\t\t\tif (mark_additional_complete_information)\n ++\t\t\tif (mark_tags_complete_and_check_obj_db)\n \t\t\t\ttag->object.flags |= COMPLETE;\n \t\t\toid = &tag->tagged->oid;\n \t\t} else {\n -@@ fetch-pack.c: static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n - \tsave_commit_buffer = old_save_commit_buffer;\n - }\n - \n -+static void warn_in_commit_graph_only(const struct object_id *oid)\n -+{\n -+\twarning(_(\"You are attempting to fetch %s, which is in the commit graph file but not in the object database.\"),\n -+\t\toid_to_hex(oid));\n -+\twarning(_(\"This is probably due to repo corruption.\"));\n -+\twarning(_(\"If you are attempting to repair this repo corruption by refetching the missing object, use 'git fetch --refetch' with the missing object.\"));\n -+}\n -+\n - /*\n - * Returns 1 if every object pointed to by the given remote refs is available\n - * locally and reachable from a local ref, and 0 otherwise.\n -@@ fetch-pack.c: static int everything_local(struct fetch_pack_args *args,\n - \t\t\t\t ref->name);\n - \t\t\tcontinue;\n - \t\t}\n -+\t\tif (o->flags & COMPLETE_FROM_COMMIT_GRAPH) {\n -+\t\t\tif (!has_object(the_repository, remote, 0))\n -+\t\t\t\twarn_in_commit_graph_only(remote);\n -+\t\t}\n - \t\tprint_verbose(args, _(\"already have %s (%s)\"), oid_to_hex(remote),\n - \t\t\t ref->name);\n - \t}\n \n - ## object.h ##\n -@@ object.h: void object_array_init(struct object_array *array);\n - /*\n - * object flag allocation:\n - * revision.h: 0---------10 15 23------27\n -- * fetch-pack.c: 01 67\n -+ * fetch-pack.c: 01 6-8\n - * negotiator/default.c: 2--5\n - * walker.c: 0-2\n - * upload-pack.c: 4 11-----14 16-----19\n + ## t/t5330-no-lazy-fetch-with-commit-graph.sh ##\n +@@ t/t5330-no-lazy-fetch-with-commit-graph.sh: test_expect_success 'fetch any commit from promisor with the usage of the commit\n + \ttest_commit -C with-commit any-commit &&\n + \tanycommit=$(git -C with-commit rev-parse HEAD) &&\n + \tGIT_TRACE=\"$(pwd)/trace.txt\" \\\n +-\t\tgit -C with-commit-graph fetch origin $anycommit 2>err &&\n ++\t\ttest_must_fail git -C with-commit-graph fetch origin $anycommit 2>err &&\n + \t! grep \"fatal: promisor-remote: unable to fork off fetch subprocess\" err &&\n + \tgrep \"git fetch origin\" trace.txt >actual &&\n + \ttest_line_count = 1 actual\n-- \n2.47.0.163.g1226f6d8fa-goog\n\n"},{"id":"506411","messageId":"34e87b83884e27e421a64cb4a3594b1dacc2a391.1730409376.git.jonathantanmy@google.com","threadId":"62250","inReplyTo":"cover.1730409376.git.jonathantanmy@google.com","subject":"[PATCH v2 1/2] Revert \"fetch-pack: add a deref_without_lazy_fetch_extended()\"","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-31T21:19:00Z","receivedAt":"2024-10-31T21:19:08Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This reverts commit a6e65fb39caf18259c660c1c7910d5bf80bc15cb.\n\nThis revert simplifies the next patch in this patch set.\n\nThe commit message of that commit mentions that the new function \"will\nbe used for the bundle-uri client in a subsequent commit\", but it seems\nthat eventually it wasn't used.\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 25 +++++++------------------\n 1 file changed, 7 insertions(+), 18 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex f752da93a8..6728a0d2f5 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -122,12 +122,11 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,\n \t\tcb(negotiator, cache.items[i]);\n }\n \n-static struct commit *deref_without_lazy_fetch_extended(const struct object_id *oid,\n-\t\t\t\t\t\t\tint mark_tags_complete,\n-\t\t\t\t\t\t\tenum object_type *type,\n-\t\t\t\t\t\t\tunsigned int oi_flags)\n+static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n+\t\t\t\t\t int mark_tags_complete)\n {\n-\tstruct object_info info = { .typep = type };\n+\tenum object_type type;\n+\tstruct object_info info = { .typep = &type };\n \tstruct commit *commit;\n \n \tcommit = lookup_commit_in_graph(the_repository, oid);\n@@ -136,9 +135,9 @@ static struct commit *deref_without_lazy_fetch_extended(const struct object_id *\n \n \twhile (1) {\n \t\tif (oid_object_info_extended(the_repository, oid, &info,\n-\t\t\t\t\t oi_flags))\n+\t\t\t\t\t OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK))\n \t\t\treturn NULL;\n-\t\tif (*type == OBJ_TAG) {\n+\t\tif (type == OBJ_TAG) {\n \t\t\tstruct tag *tag = (struct tag *)\n \t\t\t\tparse_object(the_repository, oid);\n \n@@ -152,7 +151,7 @@ static struct commit *deref_without_lazy_fetch_extended(const struct object_id *\n \t\t}\n \t}\n \n-\tif (*type == OBJ_COMMIT) {\n+\tif (type == OBJ_COMMIT) {\n \t\tstruct commit *commit = lookup_commit(the_repository, oid);\n \t\tif (!commit || repo_parse_commit(the_repository, commit))\n \t\t\treturn NULL;\n@@ -162,16 +161,6 @@ static struct commit *deref_without_lazy_fetch_extended(const struct object_id *\n \treturn NULL;\n }\n \n-\n-static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n-\t\t\t\t\t int mark_tags_complete)\n-{\n-\tenum object_type type;\n-\tunsigned flags = OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK;\n-\treturn deref_without_lazy_fetch_extended(oid, mark_tags_complete,\n-\t\t\t\t\t\t &type, flags);\n-}\n-\n static int rev_list_insert_ref(struct fetch_negotiator *negotiator,\n \t\t\t const struct object_id *oid)\n {\n-- \n2.47.0.163.g1226f6d8fa-goog\n\n"},{"id":"506412","messageId":"631b9a86778f15b7086e5f17fe850ffa151dd341.1730409376.git.jonathantanmy@google.com","threadId":"62250","inReplyTo":"cover.1730409376.git.jonathantanmy@google.com","subject":"[PATCH v2 2/2] fetch-pack: warn if in commit graph but not obj db","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-31T21:19:01Z","receivedAt":"2024-10-31T21:19:09Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When fetching, there is a step in which sought objects are first checked\nagainst the local repository; only objects that are not in the local\nrepository are then fetched. This check first looks up the commit graph\nfile, and returns \"present\" if the object is in there.\n\nHowever, the action of first looking up the commit graph file is not\ndone everywhere in Git, especially if the type of the object at the time\nof lookup is not known. This means that in a repo corruption situation,\na user may encounter an \"object missing\" error, attempt to fetch it, and\nstill encounter the same error later when they reattempt their original\naction, because the object is present in the commit graph file but not in\nthe object DB.\n\nTherefore, make it a fatal error when this occurs. (Note that we cannot\nproceed to include this object in the list of objects to be fetched\nwithout changing at least the fetch negotiation code: what would happen\nis that the client will send \"want X\" and \"have X\" and when I tested\nat $DAYJOB with a work server that uses JGit, the server reasonably\nreturned an empty packfile. And changing the fetch negotiation code to\nonly use the object DB when deciding what to report as \"have\" would be\nan unnecessary slowdown, I think.)\n\nThis was discovered when a lazy fetch of a missing commit completed with\nnothing actually fetched, and the writing of the commit graph file after\nevery fetch then attempted to read said missing commit, triggering a\nlazy fetch of said missing commit, resulting in an infinite loop with no\nuser-visible indication (until they check the list of processes running\non their computer). With this fix, there is no infinite loop. Note that\nalthough the repo corruption we discovered was caused by a bug in GC in\na partial clone, the behavior that this patch teaches Git to warn about\napplies to any repo with commit graph enabled and with a missing commit,\nwhether it is a partial clone or not.\n\nt5330, introduced in 3a1ea94a49 (commit-graph.c: no lazy fetch in\nlookup_commit_in_graph(), 2022-07-01), tests that an interaction between\nfetch and the commit graph does not cause an infinite loop. This patch\nchanges the exit code in that situation, so that test had to be changed.\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 19 ++++++++++++++++---\n t/t5330-no-lazy-fetch-with-commit-graph.sh | 2 +-\n 2 files changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 6728a0d2f5..fe1fb3c1b7 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -122,16 +122,29 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,\n \t\tcb(negotiator, cache.items[i]);\n }\n \n+static void die_in_commit_graph_only(const struct object_id *oid)\n+{\n+\tdie(_(\"You are attempting to fetch %s, which is in the commit graph file but not in the object database.\\n\"\n+\t \"This is probably due to repo corruption.\\n\"\n+\t \"If you are attempting to repair this repo corruption by refetching the missing object, use 'git fetch --refetch' with the missing object.\"),\n+\t oid_to_hex(oid));\n+}\n+\n static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n-\t\t\t\t\t int mark_tags_complete)\n+\t\t\t\t\t int mark_tags_complete_and_check_obj_db)\n {\n \tenum object_type type;\n \tstruct object_info info = { .typep = &type };\n \tstruct commit *commit;\n \n \tcommit = lookup_commit_in_graph(the_repository, oid);\n-\tif (commit)\n+\tif (commit) {\n+\t\tif (mark_tags_complete_and_check_obj_db) {\n+\t\t\tif (!has_object(the_repository, oid, 0))\n+\t\t\t\tdie_in_commit_graph_only(oid);\n+\t\t}\n \t\treturn commit;\n+\t}\n \n \twhile (1) {\n \t\tif (oid_object_info_extended(the_repository, oid, &info,\n@@ -143,7 +156,7 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n \n \t\t\tif (!tag->tagged)\n \t\t\t\treturn NULL;\n-\t\t\tif (mark_tags_complete)\n+\t\t\tif (mark_tags_complete_and_check_obj_db)\n \t\t\t\ttag->object.flags |= COMPLETE;\n \t\t\toid = &tag->tagged->oid;\n \t\t} else {\ndiff --git a/t/t5330-no-lazy-fetch-with-commit-graph.sh b/t/t5330-no-lazy-fetch-with-commit-graph.sh\nindex 5eb28f0512..feccd58324 100755\n--- a/t/t5330-no-lazy-fetch-with-commit-graph.sh\n+++ b/t/t5330-no-lazy-fetch-with-commit-graph.sh\n@@ -39,7 +39,7 @@ test_expect_success 'fetch any commit from promisor with the usage of the commit\n \ttest_commit -C with-commit any-commit &&\n \tanycommit=$(git -C with-commit rev-parse HEAD) &&\n \tGIT_TRACE=\"$(pwd)/trace.txt\" \\\n-\t\tgit -C with-commit-graph fetch origin $anycommit 2>err &&\n+\t\ttest_must_fail git -C with-commit-graph fetch origin $anycommit 2>err &&\n \t! grep \"fatal: promisor-remote: unable to fork off fetch subprocess\" err &&\n \tgrep \"git fetch origin\" trace.txt >actual &&\n \ttest_line_count = 1 actual\n-- \n2.47.0.163.g1226f6d8fa-goog\n\n"},{"id":"506413","messageId":"20241031212356.545501-1-jonathantanmy@google.com","threadId":"62250","inReplyTo":"bzhg2a7mv2xrbahk6o5kpijx4dxmpkm4wrrjhatetowjdowout@hesucnp6cikf","subject":"Re: [PATCH 2/2] fetch-pack: warn if in commit graph but not obj db","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-31T21:23:56Z","receivedAt":"2024-10-31T21:23:59Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Josh Steadmon writes:\n> > /*\n> > * Returns 1 if every object pointed to by the given remote refs is available\n> > * locally and reachable from a local ref, and 0 otherwise.\n> > @@ -830,6 +842,10 @@ static int everything_local(struct fetch_pack_args *args,\n> > \t\t\t\t ref->name);\n> > \t\t\tcontinue;\n> > \t\t}\n> > +\t\tif (o->flags & COMPLETE_FROM_COMMIT_GRAPH) {\n> > +\t\t\tif (!has_object(the_repository, remote, 0))\n> > +\t\t\t\twarn_in_commit_graph_only(remote);\n> > +\t\t}\n> \n> And now that we're checking what's local, we issue our warning if we\n> have an object missing from the DB but mentioned in the commit graph.\n> Seems fine, although I wonder if it makes more sense to fail earlier. It\n> looks like the only place we do the\n> `mark_additional_complete_information` checks is in `mark_complete()`,\n> so should we just check this condition there? No strong feelings either\n> way, just curious.\n\nWhen we were merely warning, it was useful to mark everything then\ncheck later, so that a warning message would be printed once per\nobject, instead of potentially multiple times. (In the infinite case\nthat we discovered at $DAYJOB, it doesn't really matter since the\nmessage is going to be printed an infinite number of times anyway,\nbut in the \"plain\" case in which the user is missing a commit and does\nnot have automatic commit graph writing enabled, the fetch will indeed\nterminate.)\n\nBut since we're making this a fatal error, yes, it makes sense to fail\nearlier. I've made the change.\n"},{"id":"506414","messageId":"20241031214319.550776-1-jonathantanmy@google.com","threadId":"62250","inReplyTo":"ZyPvqPK1s5lUtH+N@nand.local","subject":"Re: [PATCH 2/2] fetch-pack: warn if in commit graph but not obj db","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-31T21:43:19Z","receivedAt":"2024-10-31T21:43:22Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Taylor Blau writes:\n> > However, the action of first looking up the commit graph file is not\n> > done everywhere in Git, especially if the type of the object at the time\n> > of lookup is not known. This means that in a repo corruption situation,\n> > a user may encounter an \"object missing\" error, attempt to fetch it, and\n> > still encounter the same error later when they reattempt their original\n> > action, because the object is present in the commit graph file but not in\n> > the object DB.\n> \n> I think the type of repository corruption here may be underspecified.\n\nHmm...if you have any specific points you'd like me to elaborate on (or\nbetter yet, wording suggestions), please let me know.\n\n> You say that we have some object, say X, whose type is not known. So we\n> don't load the commit-graph, realize that X is missing, and then try and\n> fetch it.\n\nYes.\n\n> In this scenario, is X actually in the commit-graph, but not\n> in the object database?\n\nYes.\n\n> Further, if X is in the commit-graph, I assume\n> we do not look it up there because we first try and find its type, which\n> fails, so we assume we don't have it (despite it appearing corruptly in\n> the commit-graph)?\n> \n> I think that matches the behavior you're describing, but I want to make\n> sure that I'm not thinking of something else.\n\nStrictly speaking, we are not trying to find its type. We are trying\nto find the object itself. (One could argue that if we find out that\nan object is a commit, we can then ignore the packfile and go look up\nthe commit graph file. I'm not so sure this is a good idea, but this is\nmoot, I think - as far as I know, we currently don't do this.)\n\nBut yes, if the object is not in the object DB, we assume we don't have\nit.\n\n> You discuss this a little bit in your commit message, but I wonder if we\n> should just die() here. I feel like we're trying to work around a\n> situation where the commit-graph is obviously broken because it refers\n> to commit objects that don't actually exist in the object store.\n\nYeah, that seems to be the consensus. I've switched it to a fatal error.\n\n> A few thoughts in this area:\n> \n> - What situation provokes this to be true? I could imagine there is\n> some bug that we don't fully have a grasp of. But I wonder if it is\n> even easier to provoke than that, say by pruning some objects out of\n> the object store, then not rewriting the commit-graph, leaving some\n> of the references dangling.\n\nThe fetching of promisor objects that are descendants of non-promisor\nobjects. [1]\n\nI think that the rewriting of the commit graph happens on every repack,\nthus avoiding the situation you describe (unless there is a bug there).\n\n[1] https://lore.kernel.org/git/20241001191811.1934900-1-calvinwan@google.com/\n\n> - Does 'git fsck' catch this case within the commit-graph?\n\nHonestly, I haven't checked - I've been concentrating on fixing the\nfetch part for now (and also the bug that caused the missing commits\n[2]).\n\n[2] https://lore.kernel.org/git/cover.1729792911.git.jonathantanmy@google.com/\n\n> - Are the other areas of the code that rely on the assumption that all\n> entries in the commit-graph actually exist on disk? If so, are they\n> similarly broken?\n\nYes, the fetch negotiation code. It is not \"broken\" in that it solely\nuses repo_parse_commit() which always checks the commit graph, so as\nlong as the commit graph has everything we need, there will be no error.\n\nThere might be other systems that rely both on the commit graph and the\nobject DB, and thus have an inconsistent view (so, \"similarly broken\" as\nyou describe it) but at least in the partial clone case, the severity of\nthe issue is not as high as in \"fetch\", because these other systems can\nlazily fetch the missing commit and then proceed.\n\n> Another thought about this whole thing is that we essentially have a\n> code path that says: \"I found this object from the commit-graph, but\n> don't know if I actually have it on disk, so mark it to be checked later\n> via has_object()\".\n> \n> I wonder if it would be more straightforward to replace the call to\n> lookup_commit_in_graph() with a direct call to has_object() in the\n> deref_without_lazy_fetch() function, which I think would both (a)\n> eliminate the need for a new flag bit to be allocated, and (b) prevent\n> looking up the object twice.\n> \n> Thoughts?\n> \n> Thanks,\n> Taylor\n\nThis would undo the optimization in 62b5a35a33 (fetch-pack: optimize\nloading of refs via commit graph, 2021-09-01), and also would not work\nwithout changes to the fetch negotiation code - I tried to describe it\nin the commit message, perhaps not very clearly, but the issue is that\neven if we emit \"want X\", the fetch negotiation code would emit \"have\nX\" (the X is the same in both), and at least for our JGit server at\n$DAYJOB, the combination of \"want X\" and \"have X\" results in the server\nsending an empty packfile (reasonable behavior, I think). (And I don't\nthink the changes to the fetch negotiation code are worth it.)\n"},{"id":"506415","messageId":"sbfnqtf5phdstpfrsahxiz5orzhgblflvghyni462aew4n7pe2@74cnj2vjuby3","threadId":"62250","inReplyTo":"cover.1730409376.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 0/2] When fetching, die if in commit graph but not obj db","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-10-31T22:33:41Z","receivedAt":"2024-10-31T22:33:47Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.10.31 14:18, Jonathan Tan wrote:\n> Thanks everyone for your review comments. I've updated the patch 1\n> commit message as Josh requested. I'll reply individually to comments on\n> patch 2.\n> \n> Jonathan Tan (2):\n> Revert \"fetch-pack: add a deref_without_lazy_fetch_extended()\"\n> fetch-pack: warn if in commit graph but not obj db\n> \n> fetch-pack.c | 42 +++++++++++-----------\n> t/t5330-no-lazy-fetch-with-commit-graph.sh | 2 +-\n> 2 files changed, 23 insertions(+), 21 deletions(-)\n\nThis version looks good to me, thanks!\n\nReviewed-by: Josh Steadmon \n"},{"id":"506423","messageId":"xmqqikt74rs5.fsf@gitster.g","threadId":"62250","inReplyTo":"631b9a86778f15b7086e5f17fe850ffa151dd341.1730409376.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] fetch-pack: warn if in commit graph but not obj db","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-01T02:22:18Z","receivedAt":"2024-11-01T02:22:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n> -\t\t\t\t\t int mark_tags_complete)\n> +\t\t\t\t\t int mark_tags_complete_and_check_obj_db)\n> {\n> \tenum object_type type;\n> \tstruct object_info info = { .typep = &type };\n> \tstruct commit *commit;\n> \n> \tcommit = lookup_commit_in_graph(the_repository, oid);\n> -\tif (commit)\n> +\tif (commit) {\n> +\t\tif (mark_tags_complete_and_check_obj_db) {\n> +\t\t\tif (!has_object(the_repository, oid, 0))\n> +\t\t\t\tdie_in_commit_graph_only(oid);\n> +\t\t}\n> \t\treturn commit;\n> +\t}\n\nHmph, even when we are not doing the mark-tags-complete thing,\nwouldn't it be a fatal error if the commit graph claims a commit\nexists but we are missing it?\n\nIt also makes me wonder if it would be sufficient to prevent us from\nsaying \"have X\" if we just pretend as if lookup_commit_in_graph()\nreturned NULL in this case.\n\nIn any case, infinitely recursing to lazily fetch a single commit is\ndefinitely worth fixing. Thanks for digging to the bottom of the\nproblem and fixing it.\n\n"},{"id":"506424","messageId":"xmqqcyjf4m3h.fsf@gitster.g","threadId":"62250","inReplyTo":"xmqqikt74rs5.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] fetch-pack: warn if in commit graph but not obj db","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-01T04:25:06Z","receivedAt":"2024-11-01T04:25:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> \tcommit = lookup_commit_in_graph(the_repository, oid);\n>> -\tif (commit)\n>> +\tif (commit) {\n>> +\t\tif (mark_tags_complete_and_check_obj_db) {\n>> +\t\t\tif (!has_object(the_repository, oid, 0))\n>> +\t\t\t\tdie_in_commit_graph_only(oid);\n>> +\t\t}\n>> \t\treturn commit;\n>> +\t}\n>\n> Hmph, even when we are not doing the mark-tags-complete thing,\n> wouldn't it be a fatal error if the commit graph claims a commit\n> exists but we are missing it?\n>\n> It also makes me wonder if it would be sufficient to prevent us from\n> saying \"have X\" if we just pretend as if lookup_commit_in_graph()\n> returned NULL in this case.\n\nAgain, sorry for the noise.\n\nI think the posted patch is better without either of these two,\nsimply because the \"commit graph lies\" case is a repository\ncorruption, and \"git fsck\" should catch such a corruption (and if\nnot, we should make sure it does).\n\nThe normal codepaths should assume a healthy working repository.\n\nAs has_object() is not without cost, an extra check is warranted\nonly because not checking will go into infinite recursion. If it\ndoes not make us fail in such an unpleasant way if we return such a\ncommit when we are not doing the mark-tags-complete thing (but makes\nus fail in some other controlled way), not paying cost for an extra\ncheck is the right thing.\n\nThanks.\n"},{"id":"506438","messageId":"CAKgqsWWo2r37nsxeYErXjEgrSepBgFpdde9bXuYDTfu4MC3+Ag@mail.gmail.com","threadId":"62250","inReplyTo":"xmqqcyjf4m3h.fsf@gitster.g","subject":"Re: [External] Re: [PATCH v2 2/2] fetch-pack: warn if in commit graph but not obj db","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2024-11-01T08:59:30Z","receivedAt":"2024-11-01T08:59:43Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"On Fri, Nov 1, 2024 at 12:25 PM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> >> commit = lookup_commit_in_graph(the_repository, oid);\n> >> - if (commit)\n> >> + if (commit) {\n> >> + if (mark_tags_complete_and_check_obj_db) {\n> >> + if (!has_object(the_repository, oid, 0))\n> >> + die_in_commit_graph_only(oid);\n> >> + }\n> >> return commit;\n> >> + }\n> >\n> > Hmph, even when we are not doing the mark-tags-complete thing,\n> > wouldn't it be a fatal error if the commit graph claims a commit\n> > exists but we are missing it?\n> >\n> > It also makes me wonder if it would be sufficient to prevent us from\n> > saying \"have X\" if we just pretend as if lookup_commit_in_graph()\n> > returned NULL in this case.\n>\n> Again, sorry for the noise.\n>\n> I think the posted patch is better without either of these two,\n> simply because the \"commit graph lies\" case is a repository\n> corruption, and \"git fsck\" should catch such a corruption (and if\n> not, we should make sure it does).\n>\n> The normal codepaths should assume a healthy working repository.\n>\n> As has_object() is not without cost, an extra check is warranted\n> only because not checking will go into infinite recursion. If it\n> does not make us fail in such an unpleasant way if we return such a\n> commit when we are not doing the mark-tags-complete thing (but makes\n> us fail in some other controlled way), not paying cost for an extra\n> check is the right thing.\n>\n> Thanks.\n\nAlthough the scenario I faked in t/t5330-no-lazy-fetch-with-commit-graph.sh\nusually does not occur, if we are unfortunate enough to encounter this issue,\nI hope it can automatically fix the problem as much as possible without\nrelying on me to take an extra action.\n\nThanks.\n"},{"id":"506449","messageId":"ZyTmnDHGdblD3/FU@nand.local","threadId":"62250","inReplyTo":"20241031214319.550776-1-jonathantanmy@google.com","subject":"Re: [PATCH 2/2] fetch-pack: warn if in commit graph but not obj db","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-11-01T14:33:00Z","receivedAt":"2024-11-01T14:33:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 31, 2024 at 02:43:19PM -0700, Jonathan Tan wrote:\n> > Another thought about this whole thing is that we essentially have a\n> > code path that says: \"I found this object from the commit-graph, but\n> > don't know if I actually have it on disk, so mark it to be checked later\n> > via has_object()\".\n> >\n> > I wonder if it would be more straightforward to replace the call to\n> > lookup_commit_in_graph() with a direct call to has_object() in the\n> > deref_without_lazy_fetch() function, which I think would both (a)\n> > eliminate the need for a new flag bit to be allocated, and (b) prevent\n> > looking up the object twice.\n> >\n> > Thoughts?\n>\n> This would undo the optimization in 62b5a35a33 (fetch-pack: optimize\n> loading of refs via commit graph, 2021-09-01), and also would not work\n> without changes to the fetch negotiation code - I tried to describe it\n> in the commit message, perhaps not very clearly, but the issue is that\n> even if we emit \"want X\", the fetch negotiation code would emit \"have\n> X\" (the X is the same in both), and at least for our JGit server at\n> $DAYJOB, the combination of \"want X\" and \"have X\" results in the server\n> sending an empty packfile (reasonable behavior, I think). (And I don't\n> think the changes to the fetch negotiation code are worth it.)\n\nThanks for the clarifications above. What I was trying to poke at here\nwas... doesn't the change as presented undo that optimization, just in a\ndifferent way?\n\nIn 62b5a35a33 we taught deref_without_lazy_fetch() to lookup commits\nthrough the commit-graph. But in this patch, we now call has_object()\non top of that existing check. Am I missing something obvious?\n\nThanks,\nTaylor\n"},{"id":"506453","messageId":"ZyTxNwZgeOy/+05b@nand.local","threadId":"62250","inReplyTo":"631b9a86778f15b7086e5f17fe850ffa151dd341.1730409376.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] fetch-pack: warn if in commit graph but not obj db","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-11-01T15:18:15Z","receivedAt":"2024-11-01T15:18:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 31, 2024 at 02:19:01PM -0700, Jonathan Tan wrote:\n> diff --git a/t/t5330-no-lazy-fetch-with-commit-graph.sh b/t/t5330-no-lazy-fetch-with-commit-graph.sh\n> index 5eb28f0512..feccd58324 100755\n> --- a/t/t5330-no-lazy-fetch-with-commit-graph.sh\n> +++ b/t/t5330-no-lazy-fetch-with-commit-graph.sh\n> @@ -39,7 +39,7 @@ test_expect_success 'fetch any commit from promisor with the usage of the commit\n> \ttest_commit -C with-commit any-commit &&\n> \tanycommit=$(git -C with-commit rev-parse HEAD) &&\n> \tGIT_TRACE=\"$(pwd)/trace.txt\" \\\n> -\t\tgit -C with-commit-graph fetch origin $anycommit 2>err &&\n> +\t\ttest_must_fail git -C with-commit-graph fetch origin $anycommit 2>err &&\n\nIt appears that this line breaks CI:\n\n https://github.com/ttaylorr/git/actions/runs/11631453312/job/32392591229\n\nbecause you're using a one-shot environment variable assignment before\ncalling a shell function.\n\nThis should instead be:\n\n test_must_fail env GIT_TRACE=\"$(pwd)/trace.txt\" \\\n git -C with-commit-graph fetch origin $anycommit 2>err &&\n\nThanks,\nTaylor\n"},{"id":"506457","messageId":"20241101173323.681359-1-jonathantanmy@google.com","threadId":"62250","inReplyTo":"ZyTmnDHGdblD3/FU@nand.local","subject":"Re: [PATCH 2/2] fetch-pack: warn if in commit graph but not obj db","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-11-01T17:33:23Z","receivedAt":"2024-11-01T17:33:26Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Taylor Blau writes:\n> On Thu, Oct 31, 2024 at 02:43:19PM -0700, Jonathan Tan wrote:\n> > > Another thought about this whole thing is that we essentially have a\n> > > code path that says: \"I found this object from the commit-graph, but\n> > > don't know if I actually have it on disk, so mark it to be checked later\n> > > via has_object()\".\n> > >\n> > > I wonder if it would be more straightforward to replace the call to\n> > > lookup_commit_in_graph() with a direct call to has_object() in the\n> > > deref_without_lazy_fetch() function, which I think would both (a)\n> > > eliminate the need for a new flag bit to be allocated, and (b) prevent\n> > > looking up the object twice.\n> > >\n> > > Thoughts?\n> >\n> > This would undo the optimization in 62b5a35a33 (fetch-pack: optimize\n> > loading of refs via commit graph, 2021-09-01), and also would not work\n> > without changes to the fetch negotiation code - I tried to describe it\n> > in the commit message, perhaps not very clearly, but the issue is that\n> > even if we emit \"want X\", the fetch negotiation code would emit \"have\n> > X\" (the X is the same in both), and at least for our JGit server at\n> > $DAYJOB, the combination of \"want X\" and \"have X\" results in the server\n> > sending an empty packfile (reasonable behavior, I think). (And I don't\n> > think the changes to the fetch negotiation code are worth it.)\n> \n> Thanks for the clarifications above. What I was trying to poke at here\n> was... doesn't the change as presented undo that optimization, just in a\n> different way?\n> \n> In 62b5a35a33 we taught deref_without_lazy_fetch() to lookup commits\n> through the commit-graph. But in this patch, we now call has_object()\n> on top of that existing check. Am I missing something obvious?\n> \n> Thanks,\n> Taylor\n\nderef_without_lazy_fetch() is used in these situations:\n (1) to mark things COMPLETE (the 2nd argument is set to 1)\n (2) all other situations (the 2nd argument is set to 0)\n\n62b5a35a33 teaches deref_without_lazy_fetch() to use the commit-graph in\nall situations.\n\nThe change I have presented in this patch set teaches\nderef_without_lazy_fetch() to read both the commit-graph and the object\nDB in (1) but not (2). So I'm undoing the optimization, but not for\nall situations.\n\nMy understanding of your suggestion was to undo the optimization in\nall situations.\n"},{"id":"506458","messageId":"20241101173640.683565-1-jonathantanmy@google.com","threadId":"62250","inReplyTo":"xmqqikt74rs5.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] fetch-pack: warn if in commit graph but not obj db","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-11-01T17:36:40Z","receivedAt":"2024-11-01T17:36:43Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio C Hamano writes:\n> Jonathan Tan writes:\n> \n> > static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n> > -\t\t\t\t\t int mark_tags_complete)\n> > +\t\t\t\t\t int mark_tags_complete_and_check_obj_db)\n> > {\n> > \tenum object_type type;\n> > \tstruct object_info info = { .typep = &type };\n> > \tstruct commit *commit;\n> > \n> > \tcommit = lookup_commit_in_graph(the_repository, oid);\n> > -\tif (commit)\n> > +\tif (commit) {\n> > +\t\tif (mark_tags_complete_and_check_obj_db) {\n> > +\t\t\tif (!has_object(the_repository, oid, 0))\n> > +\t\t\t\tdie_in_commit_graph_only(oid);\n> > +\t\t}\n> > \t\treturn commit;\n> > +\t}\n> \n> Hmph, even when we are not doing the mark-tags-complete thing,\n> wouldn't it be a fatal error if the commit graph claims a commit\n> exists but we are missing it?\n\nIf we can detect this cheaply, yes it would be ideal if every time\nwe read a commit from the commit graph, we also check that the commit\nexists in the object DB. I don't think we can do it cheaply, though.\n\n> It also makes me wonder if it would be sufficient to prevent us from\n> saying \"have X\" if we just pretend as if lookup_commit_in_graph()\n> returned NULL in this case.\n\n\"have X\" is controlled by the packfile negotiation part, so we would\nhave to change that. (This part controls whether we send \"want X\" for a\nspecific OID or not.)\n\n> In any case, infinitely recursing to lazily fetch a single commit is\n> definitely worth fixing. Thanks for digging to the bottom of the\n> problem and fixing it.\n\nThanks.\n"},{"id":"506459","messageId":"20241101174054.684519-1-jonathantanmy@google.com","threadId":"62250","inReplyTo":"xmqqcyjf4m3h.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] fetch-pack: warn if in commit graph but not obj db","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-11-01T17:40:54Z","receivedAt":"2024-11-01T17:40:57Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio C Hamano writes:\n> Junio C Hamano writes:\n> \n> >> \tcommit = lookup_commit_in_graph(the_repository, oid);\n> >> -\tif (commit)\n> >> +\tif (commit) {\n> >> +\t\tif (mark_tags_complete_and_check_obj_db) {\n> >> +\t\t\tif (!has_object(the_repository, oid, 0))\n> >> +\t\t\t\tdie_in_commit_graph_only(oid);\n> >> +\t\t}\n> >> \t\treturn commit;\n> >> +\t}\n> >\n> > Hmph, even when we are not doing the mark-tags-complete thing,\n> > wouldn't it be a fatal error if the commit graph claims a commit\n> > exists but we are missing it?\n> >\n> > It also makes me wonder if it would be sufficient to prevent us from\n> > saying \"have X\" if we just pretend as if lookup_commit_in_graph()\n> > returned NULL in this case.\n> \n> Again, sorry for the noise.\n> \n> I think the posted patch is better without either of these two,\n> simply because the \"commit graph lies\" case is a repository\n> corruption, and \"git fsck\" should catch such a corruption (and if\n> not, we should make sure it does).\n> \n> The normal codepaths should assume a healthy working repository.\n> \n> As has_object() is not without cost, an extra check is warranted\n> only because not checking will go into infinite recursion. If it\n> does not make us fail in such an unpleasant way if we return such a\n> commit when we are not doing the mark-tags-complete thing (but makes\n> us fail in some other controlled way), not paying cost for an extra\n> check is the right thing.\n> \n> Thanks.\n\nJust checking...by \"the posted patch is better without either\nof these two\", do you mean that we should not use has_object()\nhere? I included it here in as narrow a scope as possible (when\n\"mark_tags_complete_and_check_obj_db\" is true) precisely because not\nchecking will go into infinite recursion, as you said. (And indeed I did\nnot expand the scope because I agree that the normal codepaths should\nassume a healthy working repository.)\n"},{"id":"506461","messageId":"20241101174646.685790-1-jonathantanmy@google.com","threadId":"62250","inReplyTo":"CAKgqsWWo2r37nsxeYErXjEgrSepBgFpdde9bXuYDTfu4MC3+Ag@mail.gmail.com","subject":"Re: [External] Re: [PATCH v2 2/2] fetch-pack: warn if in commit graph but not obj db","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-11-01T17:46:46Z","receivedAt":"2024-11-01T17:46:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Han Xin writes:\n> Although the scenario I faked in t/t5330-no-lazy-fetch-with-commit-graph.sh\n> usually does not occur, if we are unfortunate enough to encounter this issue,\n> I hope it can automatically fix the problem as much as possible without\n> relying on me to take an extra action.\n> \n> Thanks.\n\nNote that unfortunately the user will still need to take an extra\naction.\n\nMy goal at first was to try to teach Git to fetch the commit (in the\ncommit graph file but not in the object DB) anyway, so that (as you\nsaid) the problem will fix itself, but that requires not only a change\nin the part of the code that emits \"want\", but also the part that emits\n\"have\" (the fetch negotiation code). At that point, I decided that it's\nbetter to stop early and warn the user (it was not a fatal error in the\noriginal version of the code, but I have changed it).\n"},{"id":"506462","messageId":"20241101174904.686752-1-jonathantanmy@google.com","threadId":"62250","inReplyTo":"ZyTxNwZgeOy/+05b@nand.local","subject":"Re: [PATCH v2 2/2] fetch-pack: warn if in commit graph but not obj db","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-11-01T17:49:04Z","receivedAt":"2024-11-01T17:49:06Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Taylor Blau writes:\n> On Thu, Oct 31, 2024 at 02:19:01PM -0700, Jonathan Tan wrote:\n> > diff --git a/t/t5330-no-lazy-fetch-with-commit-graph.sh b/t/t5330-no-lazy-fetch-with-commit-graph.sh\n> > index 5eb28f0512..feccd58324 100755\n> > --- a/t/t5330-no-lazy-fetch-with-commit-graph.sh\n> > +++ b/t/t5330-no-lazy-fetch-with-commit-graph.sh\n> > @@ -39,7 +39,7 @@ test_expect_success 'fetch any commit from promisor with the usage of the commit\n> > \ttest_commit -C with-commit any-commit &&\n> > \tanycommit=$(git -C with-commit rev-parse HEAD) &&\n> > \tGIT_TRACE=\"$(pwd)/trace.txt\" \\\n> > -\t\tgit -C with-commit-graph fetch origin $anycommit 2>err &&\n> > +\t\ttest_must_fail git -C with-commit-graph fetch origin $anycommit 2>err &&\n> \n> It appears that this line breaks CI:\n> \n> https://github.com/ttaylorr/git/actions/runs/11631453312/job/32392591229\n> \n> because you're using a one-shot environment variable assignment before\n> calling a shell function.\n> \n> This should instead be:\n> \n> test_must_fail env GIT_TRACE=\"$(pwd)/trace.txt\" \\\n> git -C with-commit-graph fetch origin $anycommit 2>err &&\n> \n> Thanks,\n> Taylor\n\nAh, thanks. I've made the change locally. There are a few ongoing\nconversations about this patch set (thanks everyone for participating)\nso I'll wait a while before sending out the next set.\n"},{"id":"506476","messageId":"xmqqttcqz8tl.fsf@gitster.g","threadId":"62250","inReplyTo":"20241101174054.684519-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] fetch-pack: warn if in commit graph but not obj db","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-02T02:08:22Z","receivedAt":"2024-11-02T02:08:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Junio C Hamano writes:\n>> Junio C Hamano writes:\n>> \n>> >> \tcommit = lookup_commit_in_graph(the_repository, oid);\n>> >> -\tif (commit)\n>> >> +\tif (commit) {\n>> >> +\t\tif (mark_tags_complete_and_check_obj_db) {\n>> >> +\t\t\tif (!has_object(the_repository, oid, 0))\n>> >> +\t\t\t\tdie_in_commit_graph_only(oid);\n>> >> +\t\t}\n>> >> \t\treturn commit;\n>> >> +\t}\n>> >\n>> > Hmph, even when we are not doing the mark-tags-complete thing,\n>> > wouldn't it be a fatal error if the commit graph claims a commit\n>> > exists but we are missing it?\n>> >\n>> > It also makes me wonder if it would be sufficient to prevent us from\n>> > saying \"have X\" if we just pretend as if lookup_commit_in_graph()\n>> > returned NULL in this case.\n>> \n>> Again, sorry for the noise.\n>> \n>> I think the posted patch is better without either of these two,\n>> simply because the \"commit graph lies\" case is a repository\n>> corruption, and \"git fsck\" should catch such a corruption (and if\n>> not, we should make sure it does).\n>> \n>> The normal codepaths should assume a healthy working repository.\n>> \n>> As has_object() is not without cost, an extra check is warranted\n>> only because not checking will go into infinite recursion. If it\n>> does not make us fail in such an unpleasant way if we return such a\n>> commit when we are not doing the mark-tags-complete thing (but makes\n>> us fail in some other controlled way), not paying cost for an extra\n>> check is the right thing.\n>> \n>> Thanks.\n>\n> Just checking...by \"the posted patch is better without either\n> of these two\", do you mean that we should not use has_object()\n> here?\n\nNo, \"these two\" refers to two changes I hinted at in my message,\ni.e. (1) regardless of mark_tags_complete_and_check_obj_db shouldn't\nwe check with has_object() and die? and (2) if we commit=NULL and\nkeep going, would it be sufficient to fix it?\n"},{"id":"506677","messageId":"cover.1730833754.git.jonathantanmy@google.com","threadId":"62250","inReplyTo":"cover.1730235646.git.jonathantanmy@google.com","subject":"[PATCH v3 0/2] When fetching, die if in commit graph but not obj db","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-11-05T19:24:17Z","receivedAt":"2024-11-05T19:24:23Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Changes: the commit message title of the second patch, and a change from\ngrep to test_grep.\n\nJonathan Tan (2):\n Revert \"fetch-pack: add a deref_without_lazy_fetch_extended()\"\n fetch-pack: die if in commit graph but not obj db\n\n fetch-pack.c | 42 +++++++++++-----------\n t/t5330-no-lazy-fetch-with-commit-graph.sh | 4 +--\n 2 files changed, 24 insertions(+), 22 deletions(-)\n\nRange-diff against v2:\n1: 34e87b8388 = 1: 34e87b8388 Revert \"fetch-pack: add a deref_without_lazy_fetch_extended()\"\n2: a35e386a0e ! 2: c92b2c9e50 fetch-pack: warn if in commit graph but not obj db\n @@ Metadata\n Author: Jonathan Tan \n \n ## Commit message ##\n - fetch-pack: warn if in commit graph but not obj db\n + fetch-pack: die if in commit graph but not obj db\n \n When fetching, there is a step in which sought objects are first checked\n against the local repository; only objects that are not in the local\n @@ t/t5330-no-lazy-fetch-with-commit-graph.sh: test_expect_success 'fetch any commi\n -\tGIT_TRACE=\"$(pwd)/trace.txt\" \\\n +\ttest_must_fail env GIT_TRACE=\"$(pwd)/trace.txt\" \\\n \t\tgit -C with-commit-graph fetch origin $anycommit 2>err &&\n - \t! grep \"fatal: promisor-remote: unable to fork off fetch subprocess\" err &&\n +-\t! grep \"fatal: promisor-remote: unable to fork off fetch subprocess\" err &&\n ++\ttest_grep ! \"fatal: promisor-remote: unable to fork off fetch subprocess\" err &&\n \tgrep \"git fetch origin\" trace.txt >actual &&\n + \ttest_line_count = 1 actual\n + '\n-- \n2.47.0.277.g8800431eea-goog\n\n"},{"id":"506678","messageId":"34e87b83884e27e421a64cb4a3594b1dacc2a391.1730833754.git.jonathantanmy@google.com","threadId":"62250","inReplyTo":"cover.1730833754.git.jonathantanmy@google.com","subject":"[PATCH v3 1/2] Revert \"fetch-pack: add a deref_without_lazy_fetch_extended()\"","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-11-05T19:24:18Z","receivedAt":"2024-11-05T19:24:24Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This reverts commit a6e65fb39caf18259c660c1c7910d5bf80bc15cb.\n\nThis revert simplifies the next patch in this patch set.\n\nThe commit message of that commit mentions that the new function \"will\nbe used for the bundle-uri client in a subsequent commit\", but it seems\nthat eventually it wasn't used.\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 25 +++++++------------------\n 1 file changed, 7 insertions(+), 18 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex f752da93a8..6728a0d2f5 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -122,12 +122,11 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,\n \t\tcb(negotiator, cache.items[i]);\n }\n \n-static struct commit *deref_without_lazy_fetch_extended(const struct object_id *oid,\n-\t\t\t\t\t\t\tint mark_tags_complete,\n-\t\t\t\t\t\t\tenum object_type *type,\n-\t\t\t\t\t\t\tunsigned int oi_flags)\n+static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n+\t\t\t\t\t int mark_tags_complete)\n {\n-\tstruct object_info info = { .typep = type };\n+\tenum object_type type;\n+\tstruct object_info info = { .typep = &type };\n \tstruct commit *commit;\n \n \tcommit = lookup_commit_in_graph(the_repository, oid);\n@@ -136,9 +135,9 @@ static struct commit *deref_without_lazy_fetch_extended(const struct object_id *\n \n \twhile (1) {\n \t\tif (oid_object_info_extended(the_repository, oid, &info,\n-\t\t\t\t\t oi_flags))\n+\t\t\t\t\t OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK))\n \t\t\treturn NULL;\n-\t\tif (*type == OBJ_TAG) {\n+\t\tif (type == OBJ_TAG) {\n \t\t\tstruct tag *tag = (struct tag *)\n \t\t\t\tparse_object(the_repository, oid);\n \n@@ -152,7 +151,7 @@ static struct commit *deref_without_lazy_fetch_extended(const struct object_id *\n \t\t}\n \t}\n \n-\tif (*type == OBJ_COMMIT) {\n+\tif (type == OBJ_COMMIT) {\n \t\tstruct commit *commit = lookup_commit(the_repository, oid);\n \t\tif (!commit || repo_parse_commit(the_repository, commit))\n \t\t\treturn NULL;\n@@ -162,16 +161,6 @@ static struct commit *deref_without_lazy_fetch_extended(const struct object_id *\n \treturn NULL;\n }\n \n-\n-static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n-\t\t\t\t\t int mark_tags_complete)\n-{\n-\tenum object_type type;\n-\tunsigned flags = OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK;\n-\treturn deref_without_lazy_fetch_extended(oid, mark_tags_complete,\n-\t\t\t\t\t\t &type, flags);\n-}\n-\n static int rev_list_insert_ref(struct fetch_negotiator *negotiator,\n \t\t\t const struct object_id *oid)\n {\n-- \n2.47.0.277.g8800431eea-goog\n\n"},{"id":"506679","messageId":"c92b2c9e50975cab217a93b3e3a962107d60d0de.1730833754.git.jonathantanmy@google.com","threadId":"62250","inReplyTo":"cover.1730833754.git.jonathantanmy@google.com","subject":"[PATCH v3 2/2] fetch-pack: die if in commit graph but not obj db","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-11-05T19:24:19Z","receivedAt":"2024-11-05T19:24:26Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When fetching, there is a step in which sought objects are first checked\nagainst the local repository; only objects that are not in the local\nrepository are then fetched. This check first looks up the commit graph\nfile, and returns \"present\" if the object is in there.\n\nHowever, the action of first looking up the commit graph file is not\ndone everywhere in Git, especially if the type of the object at the time\nof lookup is not known. This means that in a repo corruption situation,\na user may encounter an \"object missing\" error, attempt to fetch it, and\nstill encounter the same error later when they reattempt their original\naction, because the object is present in the commit graph file but not in\nthe object DB.\n\nTherefore, make it a fatal error when this occurs. (Note that we cannot\nproceed to include this object in the list of objects to be fetched\nwithout changing at least the fetch negotiation code: what would happen\nis that the client will send \"want X\" and \"have X\" and when I tested\nat $DAYJOB with a work server that uses JGit, the server reasonably\nreturned an empty packfile. And changing the fetch negotiation code to\nonly use the object DB when deciding what to report as \"have\" would be\nan unnecessary slowdown, I think.)\n\nThis was discovered when a lazy fetch of a missing commit completed with\nnothing actually fetched, and the writing of the commit graph file after\nevery fetch then attempted to read said missing commit, triggering a\nlazy fetch of said missing commit, resulting in an infinite loop with no\nuser-visible indication (until they check the list of processes running\non their computer). With this fix, there is no infinite loop. Note that\nalthough the repo corruption we discovered was caused by a bug in GC in\na partial clone, the behavior that this patch teaches Git to warn about\napplies to any repo with commit graph enabled and with a missing commit,\nwhether it is a partial clone or not.\n\nt5330, introduced in 3a1ea94a49 (commit-graph.c: no lazy fetch in\nlookup_commit_in_graph(), 2022-07-01), tests that an interaction between\nfetch and the commit graph does not cause an infinite loop. This patch\nchanges the exit code in that situation, so that test had to be changed.\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 19 ++++++++++++++++---\n t/t5330-no-lazy-fetch-with-commit-graph.sh | 4 ++--\n 2 files changed, 18 insertions(+), 5 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 6728a0d2f5..fe1fb3c1b7 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -122,16 +122,29 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,\n \t\tcb(negotiator, cache.items[i]);\n }\n \n+static void die_in_commit_graph_only(const struct object_id *oid)\n+{\n+\tdie(_(\"You are attempting to fetch %s, which is in the commit graph file but not in the object database.\\n\"\n+\t \"This is probably due to repo corruption.\\n\"\n+\t \"If you are attempting to repair this repo corruption by refetching the missing object, use 'git fetch --refetch' with the missing object.\"),\n+\t oid_to_hex(oid));\n+}\n+\n static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n-\t\t\t\t\t int mark_tags_complete)\n+\t\t\t\t\t int mark_tags_complete_and_check_obj_db)\n {\n \tenum object_type type;\n \tstruct object_info info = { .typep = &type };\n \tstruct commit *commit;\n \n \tcommit = lookup_commit_in_graph(the_repository, oid);\n-\tif (commit)\n+\tif (commit) {\n+\t\tif (mark_tags_complete_and_check_obj_db) {\n+\t\t\tif (!has_object(the_repository, oid, 0))\n+\t\t\t\tdie_in_commit_graph_only(oid);\n+\t\t}\n \t\treturn commit;\n+\t}\n \n \twhile (1) {\n \t\tif (oid_object_info_extended(the_repository, oid, &info,\n@@ -143,7 +156,7 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n \n \t\t\tif (!tag->tagged)\n \t\t\t\treturn NULL;\n-\t\t\tif (mark_tags_complete)\n+\t\t\tif (mark_tags_complete_and_check_obj_db)\n \t\t\t\ttag->object.flags |= COMPLETE;\n \t\t\toid = &tag->tagged->oid;\n \t\t} else {\ndiff --git a/t/t5330-no-lazy-fetch-with-commit-graph.sh b/t/t5330-no-lazy-fetch-with-commit-graph.sh\nindex 5eb28f0512..21f36eb8c3 100755\n--- a/t/t5330-no-lazy-fetch-with-commit-graph.sh\n+++ b/t/t5330-no-lazy-fetch-with-commit-graph.sh\n@@ -38,9 +38,9 @@ test_expect_success 'fetch any commit from promisor with the usage of the commit\n \tgit -C with-commit-graph config remote.origin.partialclonefilter blob:none &&\n \ttest_commit -C with-commit any-commit &&\n \tanycommit=$(git -C with-commit rev-parse HEAD) &&\n-\tGIT_TRACE=\"$(pwd)/trace.txt\" \\\n+\ttest_must_fail env GIT_TRACE=\"$(pwd)/trace.txt\" \\\n \t\tgit -C with-commit-graph fetch origin $anycommit 2>err &&\n-\t! grep \"fatal: promisor-remote: unable to fork off fetch subprocess\" err &&\n+\ttest_grep ! \"fatal: promisor-remote: unable to fork off fetch subprocess\" err &&\n \tgrep \"git fetch origin\" trace.txt >actual &&\n \ttest_line_count = 1 actual\n '\n-- \n2.47.0.277.g8800431eea-goog\n\n"},{"id":"506696","messageId":"xmqq4j4lqclg.fsf@gitster.g","threadId":"62250","inReplyTo":"cover.1730833754.git.jonathantanmy@google.com","subject":"Re: [PATCH v3 0/2] When fetching, die if in commit graph but not obj db","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-06T03:12:59Z","receivedAt":"2024-11-06T03:13:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Changes: the commit message title of the second patch, and a change from\n> grep to test_grep.\n>\n> Jonathan Tan (2):\n> Revert \"fetch-pack: add a deref_without_lazy_fetch_extended()\"\n> fetch-pack: die if in commit graph but not obj db\n\nI presume that with this the topic should be ready for 'next'.\n\nThanks.\n"}]}