{"thread":{"id":"55354","subject":"[PATCH v1] builtin/pack-objects.c: ignore missing links with --stdin-packs","startedAt":"2021-03-19T15:41:35Z","lastAt":"2021-03-19T20:56:42Z","messageCount":4,"participants":["Taylor Blau","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"419714","messageId":"815623da67d283e8509fc4ac67e939c6140fc39a.1616168441.git.me@ttaylorr.com","threadId":"55354","inReplyTo":null,"subject":"[PATCH v1] builtin/pack-objects.c: ignore missing links with --stdin-packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-03-19T15:40:52Z","receivedAt":"2021-03-19T15:41:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When 'git pack-objects --stdin-packs' encounters a commit in a pack, it\nmarks it as a starting point of a best-effort reachability traversal\nthat is used to populate the name-hash of the objects listed in the\ngiven packs.\n\nThe traversal expects that it should be able to walk the ancestors of\nall commits in a pack without issue. Ordinarily this is the case, but it\nis possible to having missing parents from an unreachable part of the\nrepository. In that case, we'd consider any missing objects in the\nunreachable portion of the graph to be junk.\n\nThis should be handled gracefully: since the traversal is best-effort\n(i.e., we don't strictly need to fill in all of the name-hash fields),\nwe should simply ignore any missing links.\n\nThis patch does that (by setting the 'ignore_missing_links' bit on the\nrev_info struct), and ensures we don't regress in the future by adding a\ntest which demonstrates this case.\n\nIt is a little over-eager, since it will also ignore missing links in\nreachable parts of the packs (which would indicate a corrupted\nrepository), but '--stdin-packs' is explicitly *not* about reachability.\nSo this step isn't making anything worse for a repository which contains\npacks missing reachable objects (since we never drop objects with\n'--stdin-packs').\n\nSigned-off-by: Taylor Blau \n---\nI noticed a small number of repositories using geometric repacks\ncomplained about missing links when doing the best-effort traversal to\nfill in missing namehash fields with 'git pack-objects --stdin-packs'.\n\nThis patch takes care to handle that situation gracefully. It is based\non tb/geometric-repack, which is on 'next'.\n\n builtin/pack-objects.c | 1 +\n t/t5300-pack-object.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 39 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 8cb32763b7..f513138513 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3122,6 +3122,7 @@ static void read_packs_list_from_stdin(void)\n \trevs.blob_objects = 1;\n \trevs.tree_objects = 1;\n \trevs.tag_objects = 1;\n+\trevs.ignore_missing_links = 1;\n\n \twhile (strbuf_getline(&buf, stdin) != EOF) {\n \t\tif (!buf.len)\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 7138a54595..ab509e8c38 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -629,4 +629,42 @@ test_expect_success '--stdin-packs with loose objects' '\n \t)\n '\n\n+test_expect_success '--stdin-packs with broken links' '\n+\t(\n+\t\tcd stdin-packs &&\n+\n+\t\t# make an unreachable object with a bogus parent\n+\t\tgit cat-file -p HEAD >commit &&\n+\t\tsed \"s/$(git rev-parse HEAD^)/$(test_oid zero)/\" in &&\n+\n+\t\tgit pack-objects .git/objects/pack/pack-D expect.raw &&\n+\t\tgit show-index <$(ls test3-*.idx) >actual.raw &&\n+\n+\t\tcut -d\" \" -f2 expect &&\n+\t\tcut -d\" \" -f2 actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n--\n2.30.0.667.g81c0cbc6fd\n"},{"id":"419726","messageId":"xmqqim5nm2g0.fsf@gitster.g","threadId":"55354","inReplyTo":"815623da67d283e8509fc4ac67e939c6140fc39a.1616168441.git.me@ttaylorr.com","subject":"Re: [PATCH v1] builtin/pack-objects.c: ignore missing links with --stdin-packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-19T18:19:11Z","receivedAt":"2021-03-19T18:20:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> When 'git pack-objects --stdin-packs' encounters a commit in a pack, it\n> marks it as a starting point of a best-effort reachability traversal\n> that is used to populate the name-hash of the objects listed in the\n> given packs.\n>\n> The traversal expects that it should be able to walk the ancestors of\n> all commits in a pack without issue. Ordinarily this is the case, but it\n> is possible to having missing parents from an unreachable part of the\n> repository. In that case, we'd consider any missing objects in the\n> unreachable portion of the graph to be junk.\n\nAh, OK. So a pack that is being consolidated, or more likely a\nloose commit that is rolled into the smallest geometric group) may\ncontain an unreachable commit, whose tree or blob has already been\npruned, which is an expected state (i.e. tree or blob may have been\nolder than the commit whose message may have been amended recently\nbefore the entire commit got abandoned). And we do not want to alarm\nusers by warning.\n\n> This should be handled gracefully: since the traversal is best-effort\n> (i.e., we don't strictly need to fill in all of the name-hash fields),\n> we should simply ignore any missing links.\n\nOr the entire set of objects that refer them can be omitted from the\nresulting set of objects (iow, consider a commit that lacks a tree\nor a blob to be checked out stale and prunable without downsides,\nand prune it and its remaining trees and blobs by leaving them out\nof the resulting pack), but I suspect that is a lot more involved\nchange.\n\n> This patch does that (by setting the 'ignore_missing_links' bit on the\n> rev_info struct), and ensures we don't regress in the future by adding a\n> test which demonstrates this case.\n>\n> It is a little over-eager, since it will also ignore missing links in\n> reachable parts of the packs (which would indicate a corrupted\n> repository), but '--stdin-packs' is explicitly *not* about reachability.\n> So this step isn't making anything worse for a repository which contains\n> packs missing reachable objects (since we never drop objects with\n> '--stdin-packs').\n\nYup. I find the reasoning quite sensible.\n\nThanks, will queue.\n\n> Signed-off-by: Taylor Blau \n> ---\n> I noticed a small number of repositories using geometric repacks\n> complained about missing links when doing the best-effort traversal to\n> fill in missing namehash fields with 'git pack-objects --stdin-packs'.\n>\n> This patch takes care to handle that situation gracefully. It is based\n> on tb/geometric-repack, which is on 'next'.\n>\n> builtin/pack-objects.c | 1 +\n> t/t5300-pack-object.sh | 38 ++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 39 insertions(+)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 8cb32763b7..f513138513 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -3122,6 +3122,7 @@ static void read_packs_list_from_stdin(void)\n> \trevs.blob_objects = 1;\n> \trevs.tree_objects = 1;\n> \trevs.tag_objects = 1;\n> +\trevs.ignore_missing_links = 1;\n>\n> \twhile (strbuf_getline(&buf, stdin) != EOF) {\n> \t\tif (!buf.len)\n> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\n> index 7138a54595..ab509e8c38 100755\n> --- a/t/t5300-pack-object.sh\n> +++ b/t/t5300-pack-object.sh\n> @@ -629,4 +629,42 @@ test_expect_success '--stdin-packs with loose objects' '\n> \t)\n> '\n>\n> +test_expect_success '--stdin-packs with broken links' '\n> +\t(\n> +\t\tcd stdin-packs &&\n> +\n> +\t\t# make an unreachable object with a bogus parent\n> +\t\tgit cat-file -p HEAD >commit &&\n> +\t\tsed \"s/$(git rev-parse HEAD^)/$(test_oid zero)/\" +\t\tgit hash-object -w -t commit --stdin >in &&\n> +\n> +\t\tgit pack-objects .git/objects/pack/pack-D +\n> +\t\tPACK_A=\"$(basename .git/objects/pack/pack-A-*.pack)\" &&\n> +\t\tPACK_B=\"$(basename .git/objects/pack/pack-B-*.pack)\" &&\n> +\t\tPACK_C=\"$(basename .git/objects/pack/pack-C-*.pack)\" &&\n> +\t\tPACK_D=\"$(basename .git/objects/pack/pack-D-*.pack)\" &&\n> +\n> +\t\tgit pack-objects test3 --stdin-packs --unpacked <<-EOF &&\n> +\t\t$PACK_A\n> +\t\t^$PACK_B\n> +\t\t$PACK_C\n> +\t\t$PACK_D\n> +\t\tEOF\n> +\n> +\t\t(\n> +\t\t\tgit show-index <$(ls .git/objects/pack/pack-A-*.idx) &&\n> +\t\t\tgit show-index <$(ls .git/objects/pack/pack-C-*.idx) &&\n> +\t\t\tgit show-index <$(ls .git/objects/pack/pack-D-*.idx) &&\n> +\t\t\tgit rev-list --objects --no-object-names \\\n> +\t\t\t\trefs/tags/C..refs/tags/D\n> +\t\t) >expect.raw &&\n> +\t\tgit show-index <$(ls test3-*.idx) >actual.raw &&\n> +\n> +\t\tcut -d\" \" -f2 expect &&\n> +\t\tcut -d\" \" -f2 actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n> test_done\n> --\n> 2.30.0.667.g81c0cbc6fd\n"},{"id":"419731","messageId":"YFT8LqAcCqpG2wyl@coredump.intra.peff.net","threadId":"55354","inReplyTo":"xmqqim5nm2g0.fsf@gitster.g","subject":"Re: [PATCH v1] builtin/pack-objects.c: ignore missing links with --stdin-packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-03-19T19:31:58Z","receivedAt":"2021-03-19T19:32:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 19, 2021 at 11:19:11AM -0700, Junio C Hamano wrote:\n\n> > The traversal expects that it should be able to walk the ancestors of\n> > all commits in a pack without issue. Ordinarily this is the case, but it\n> > is possible to having missing parents from an unreachable part of the\n> > repository. In that case, we'd consider any missing objects in the\n> > unreachable portion of the graph to be junk.\n> \n> Ah, OK. So a pack that is being consolidated, or more likely a\n> loose commit that is rolled into the smallest geometric group) may\n> contain an unreachable commit, whose tree or blob has already been\n> pruned, which is an expected state (i.e. tree or blob may have been\n> older than the commit whose message may have been amended recently\n> before the entire commit got abandoned). And we do not want to alarm\n> users by warning.\n\nYes, though it is not just warning, but rather that before this patch\nwe'd abort the whole repack.\n\n> > This should be handled gracefully: since the traversal is best-effort\n> > (i.e., we don't strictly need to fill in all of the name-hash fields),\n> > we should simply ignore any missing links.\n> \n> Or the entire set of objects that refer them can be omitted from the\n> resulting set of objects (iow, consider a commit that lacks a tree\n> or a blob to be checked out stale and prunable without downsides,\n> and prune it and its remaining trees and blobs by leaving them out\n> of the resulting pack), but I suspect that is a lot more involved\n> change.\n\nIt is safe to omit the whole set from the name-hash traversal, which is\npurely an optimization. But it would generally not be a good idea to\nleave them out of the resulting pack, since that would mean deleting\nthem entirely from the repository (because we'll remove the old packs\nthey were in after pack-objects completes).\n\nIf they are truly unreachable, then it is not strictly wrong to delete\nthem (i.e., we are not corrupting a repository unless it was already\ncorrupted), but:\n\n - if the repository _is_ already corrupted, we are definitely making\n things worse\n\n - we generally try to keep even unreachable parts of the graph\n complete, doing things like keeping unreachable-but-old objects that\n are reachable from unreachable-but-recent. Again, we know here that\n the object graph is incomplete, so anybody pointing a ref at a\n descendant of our broken commit is already corrupting the\n repository. But it probably makes sense to follow the existing rules\n as much as possible, and not make such a situation worse.\n\n> > It is a little over-eager, since it will also ignore missing links in\n> > reachable parts of the packs (which would indicate a corrupted\n> > repository), but '--stdin-packs' is explicitly *not* about reachability.\n> > So this step isn't making anything worse for a repository which contains\n> > packs missing reachable objects (since we never drop objects with\n> > '--stdin-packs').\n> \n> Yup. I find the reasoning quite sensible.\n> \n> Thanks, will queue.\n\nI had seen and discussed the patch before it hit the list, but just to\nmake it explicit: it also looks good to me. ;)\n\n-Peff\n"},{"id":"419735","messageId":"xmqq1rcan9ri.fsf@gitster.g","threadId":"55354","inReplyTo":"YFT8LqAcCqpG2wyl@coredump.intra.peff.net","subject":"Re: [PATCH v1] builtin/pack-objects.c: ignore missing links with --stdin-packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-19T20:55:45Z","receivedAt":"2021-03-19T20:56:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If they are truly unreachable, then it is not strictly wrong to delete\n> them (i.e., we are not corrupting a repository unless it was already\n> corrupted), but:\n>\n> - if the repository _is_ already corrupted, we are definitely making\n> things worse\n\nYes, that is a reason enough not to eject a commit whose tree is\nincomplete.\n"}]}