{"thread":{"id":"58643","subject":"[PATCH] revision: ignore non-existent objects in resolve-undo list","startedAt":"2022-10-18T16:03:29Z","lastAt":"2022-10-18T20:29:29Z","messageCount":4,"participants":["Mathias Rav","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"465207","messageId":"20221018175530.086c8c74@apus","threadId":"58643","inReplyTo":null,"subject":"[PATCH] revision: ignore non-existent objects in resolve-undo list","fromName":"Mathias Rav","fromEmail":"m@git.strova.dk","sentAt":"2022-10-18T15:55:30Z","receivedAt":"2022-10-18T16:03:29Z","isPatch":true,"sender":{"key":"m@git.strova.dk","avatar":"https://avatars.githubusercontent.com/u/373639?v=4"},"body":"Garbage collection could inadvertently prune blobs mentioned only in the\nresolve-undo extension prior to the bugfix in 5a5ea141e7\n(\"revision: mark blobs needed for resolve-undo as reachable\", 2022-06-09).\n\nIf a repository is affected by this bug, an obscure error can occur in\n`git gc` after updating to a version of git that has the bugfix:\n\n\t$ git gc\n\tEnumerating objects: 327687, done.\n\tCounting objects: 100% (327687/327687), done.\n\tDelta compression using up to 8 threads\n\tCompressing objects: 100% (70883/70883), done.\n\tfatal: unable to read 616c8d17f4625f227708aae480e71233f7f58dce\n\tfatal: failed to run repack\n\nA similar error occurs in `git rev-list --objects --indexed-objects`.\n\nFix the error by emitting a warning when the resolve-undo list mentions\nobjects that do not exist and then ignoring the nonexistent object.\n\nThe bugfix 5a5ea141e7 already contained code to emit this warning,\nbut since the code used lookup_blob() (and not parse_object()),\nit would only warn in the unlikely scenario where the resolve-undo list\nmentions an existing object that is not a blob.\n\nI have encountered this error on two different clones of a large\nmono-repository in checkouts with dozens of worktrees that see frequent\nrebasing (causing a lot of git gc churn) and frequent merge conflicts\nduring rebasing, leading to resolve-undo lists in the index.\nSomehow it seems the resolve-undo lists in the index persist after the\nmerge conflicts are resolved, which makes the error more frequent than\nif the resolve-undo lists were only populated during the merge conflict.\n\nSigned-off-by: Mathias Rav \nFixes: 5a5ea141e7\nReported-by: Paul Wagland \n---\n revision.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 36e31942ce..03bc45bef1 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1720,18 +1720,18 @@ static void add_resolve_undo_to_pending(struct index_state *istate, struct rev_i\n \t\tif (!ru)\n \t\t\tcontinue;\n \t\tfor (i = 0; i < 3; i++) {\n-\t\t\tstruct blob *blob;\n+\t\t\tstruct object *obj;\n \n \t\t\tif (!ru->mode[i] || !S_ISREG(ru->mode[i]))\n \t\t\t\tcontinue;\n \n-\t\t\tblob = lookup_blob(revs->repo, &ru->oid[i]);\n-\t\t\tif (!blob) {\n+\t\t\tobj = parse_object(revs->repo, &ru->oid[i]);\n+\t\t\tif (!obj) {\n \t\t\t\twarning(_(\"resolve-undo records `%s` which is missing\"),\n \t\t\t\t\toid_to_hex(&ru->oid[i]));\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tadd_pending_object_with_path(revs, &blob->object, \"\",\n+\t\t\tadd_pending_object_with_path(revs, obj, \"\",\n \t\t\t\t\t\t ru->mode[i], path);\n \t\t}\n \t}\n-- \n2.38.0\n\n"},{"id":"465208","messageId":"xmqqfsflum70.fsf@gitster.g","threadId":"58643","inReplyTo":"20221018175530.086c8c74@apus","subject":"Re: [PATCH] revision: ignore non-existent objects in resolve-undo list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-18T16:32:35Z","receivedAt":"2022-10-18T16:32:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mathias Rav writes:\n\n> Garbage collection could inadvertently prune blobs mentioned only in the\n> resolve-undo extension prior to the bugfix in 5a5ea141e7\n> (\"revision: mark blobs needed for resolve-undo as reachable\", 2022-06-09).\n\nOlder versions of Git did not consider blobs referenced by the\nresolve-undo as reachable, and allowed \"git gc\" to expire them out\nof existence. And \"git fsck\" in these versions did report that\nthese blobs are unreachable.\n\nNewer versions of Git on the other hand do consider these blobs as\nreachable, so \"git gc\" would not expire them. And \"git fsck\" would\ncomplain when they are missing, because by definition we should not\nlose reachable objects.\n\nThe error discussed recently on the list was only because older\nversion was used to \"git gc\" away blobs that are still in use.\n\nI think the right solution for such a transitory error is not to\nhide the problem and pretend that such a blob reference does not\nexist, which is what ...\n\n> Fix the error by emitting a warning when the resolve-undo list mentions\n> objects that do not exist and then ignoring the nonexistent object.\n\n... this approach is about. I think it is backwards to sweep the\nproblem under the rug without fixing the underlying problem.\n\nWe should instead be removing the reference that is no longer even\nusable for the purpose of resolve-undo, e.g. when \"rerere forget\n\" reads from the resolve-undo extension to recreate the\nconflicts.\n\nPerhaps \"git reflog --state-fix\" is a good model to follow. Back\nwhen the option was introduced, we found that there was a buggy\nimplementation of \"git gc\" that did not consider commits referenced\nby reflog entries reachable and removed them, breaking \"git reflog\".\nThe solution was to remove these reflog entries that accidentally\nlost commits that they reference because they no longer are usable.\n\nThe manual procedure Peff gave in the thread does work OK, but if it\nmakes it more friendly, a new option to \"update-index\" to fix the\nindex file by removing things that refer to missing objects would\nnot be a bad idea.\n\nThanks.\n"},{"id":"465209","messageId":"xmqqbkq9ulum.fsf@gitster.g","threadId":"58643","inReplyTo":"xmqqfsflum70.fsf@gitster.g","subject":"Re: [PATCH] revision: ignore non-existent objects in resolve-undo list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-18T16:40:01Z","receivedAt":"2022-10-18T16:40:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> Fix the error by emitting a warning when the resolve-undo list mentions\n>> objects that do not exist and then ignoring the nonexistent object.\n>\n> ... this approach is about. I think it is backwards to sweep the\n> problem under the rug without fixing the underlying problem.\n>\n> We should instead be removing the reference that is no longer even\n> usable for the purpose of resolve-undo, e.g. when \"rerere forget\n> \" reads from the resolve-undo extension to recreate the\n> conflicts.\n\nAh, I take half of it back. \"instead\" -> \"in addition\".\n\nThe patch corresponds to revision.c::handle_one_reflog_commit() that\nskips the object referenced by a reflog entry that no longer exists\nas part of the solution to the same problem as \"reflog --stale-fix\"\nsolved, and needs to be an integral half of the solution to the\n\"older gc lose blobs referenced by resolve-undo extension\" problem.\n\nAnd the patch goes in the right direction. It is a bit sad that it\nnow has to do parse_object() but in the normal case, the object\nreferenced should be a blob that exists, for which the cost of\nparsing it would be none (just setting .parsed member to true), so\nit should be OK.\n\nThanks. Will queue.\n"},{"id":"465221","messageId":"Y08Mo8AL4DmFhZao@coredump.intra.peff.net","threadId":"58643","inReplyTo":"xmqqbkq9ulum.fsf@gitster.g","subject":"Re: [PATCH] revision: ignore non-existent objects in resolve-undo list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-18T20:29:23Z","receivedAt":"2022-10-18T20:29:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 18, 2022 at 09:40:01AM -0700, Junio C Hamano wrote:\n\n> And the patch goes in the right direction. It is a bit sad that it\n> now has to do parse_object() but in the normal case, the object\n> referenced should be a blob that exists, for which the cost of\n> parsing it would be none (just setting .parsed member to true), so\n> it should be OK.\n\nThis isn't quite true. parse_object() will still inflate the object\ncontents to check the sha1. I think has_object_file() is probably the\nright thing here. We want to know if the object is missing entirely.\n\nWe'd not notice corrupted bytes, of course, but that is OK. Traversal\ndoes not open blobs we reach via trees, either. For pack-objects, we\nrely on either:\n\n - for repacking to disk, we check the pack crc for already-packed\n objects (which avoids inflating them). For loose objects, we'll\n inflate them later when we convert them to packed form.\n\n - for packing to stdout for fetch/push, the receiver is expected to\n check the sha1 via index-pack, etc.\n\nSo I think just checking \"do we have it? If not, gently skip it\" is the\nright thing here. And in the long run we'd hopefully remove that code,\nas \"we don't have it\" becomes less \"this was probably gc'd with an older\nversion of git\" to \"oops, there is a bug in Git that lost this object\".\n\nI notice that 5a5ea141e7 (revision: mark blobs needed for resolve-undo\nas reachable, 2022-06-09) uses parse_object() in the fsck code path.\nThat _might_ be better as lookup_object(), as earlier stages of fsck\nwould have checked the bytes of each object and created an in-memory\nobject struct. Though I guess in that sense, it doesn't matter;\nparse_object() will hit lookup_object() first and see that in-memory\nstruct.\n\n-Peff\n"}]}