{"thread":{"id":"55569","subject":"Two issues with mark_reachable_objects","startedAt":"2021-04-27T10:45:19Z","lastAt":"2021-04-29T01:37:55Z","messageCount":10,"participants":["David Emett","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"423100","messageId":"CAJ-dYSOVx0egnyxJb6ZjgWvEDR=19QPgc70JQ7cXUjUPZ1XDiQ@mail.gmail.com","threadId":"55569","inReplyTo":null,"subject":"Two issues with mark_reachable_objects","fromName":"David Emett","fromEmail":"dave@sp4m.net","sentAt":"2021-04-27T10:45:01Z","receivedAt":"2021-04-27T10:45:19Z","isPatch":false,"sender":{"key":"dave@sp4m.net","avatar":null},"body":"Hi all,\n\nI ran into an issue yesterday with a script that effectively does this:\n\n    set -e\n    git fetch origin refs/blah/blah\n    git checkout FETCH_HEAD\n\nThe checkout failed a few times with a \"fatal: unable to read tree\" error.\n\nI noticed that this only happened when the fetch triggered an automatic GC at\nthe end. After a bit of digging I discovered two separate issues:\n\n1) It seems that FETCH_HEAD is not considered a root by mark_reachable_objects.\n\n2) If the bitmap_git branch in mark_reachable_objects is taken, the mark_recent\n   argument is ignored. This doesn't _completely_ break \"git prune\"'s --expire\n   option, as it turns out there is another explicit mtime check in\n   prune_object (prune.c). If that check passes this is not propagated to\n   referenced objects though. So even if a dangling commit is recent, a prune\n   can discard old objects it references.\n\n(1) has been discussed before on this mailing list [1], but it doesn't look\nlike there was a conclusion.\n\nI assume (2) is not intentional, given that \"git gc --help\" explicitly says\n\"Any object with modification time newer than the --prune date is kept, along\nwith everything reachable from it.\" Is it safe to just run the mark_recent\nblock after the bitmap_git block? Could add_unseen_recent_objects_to_traversal\njust be called at the start of the bitmap_git block if mark_recent?\n\n[1] https://lore.kernel.org/git/20160708025948.GA3226@x/\n"},{"id":"423107","messageId":"YIgijn93f639Pp7Z@coredump.intra.peff.net","threadId":"55569","inReplyTo":"CAJ-dYSOVx0egnyxJb6ZjgWvEDR=19QPgc70JQ7cXUjUPZ1XDiQ@mail.gmail.com","subject":"Re: Two issues with mark_reachable_objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-27T14:41:18Z","receivedAt":"2021-04-27T14:41:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 27, 2021 at 11:45:01AM +0100, David Emett wrote:\n\n> I noticed that this only happened when the fetch triggered an automatic GC at\n> the end. After a bit of digging I discovered two separate issues:\n> \n> 1) It seems that FETCH_HEAD is not considered a root by mark_reachable_objects.\n\nRight, as you discovered, this is known and intentional. I don't have\nanything to add to the thread you linked already.\n\n> 2) If the bitmap_git branch in mark_reachable_objects is taken, the mark_recent\n>    argument is ignored. This doesn't _completely_ break \"git prune\"'s --expire\n>    option, as it turns out there is another explicit mtime check in\n>    prune_object (prune.c). If that check passes this is not propagated to\n>    referenced objects though. So even if a dangling commit is recent, a prune\n>    can discard old objects it references.\n\nBut this one is a scary bug. As you note, it's not _completely_ breaking\n\"--expire\", but it is totally disabling the \"reachable from recent\"\nsafety added in d3038d22f9 (prune: keep objects reachable from recent\nobjects, 2014-10-15).\n\nThe bug here was introduced by me, and comes from a matter of timing.\nDespite what you'll see in the project history, the \"use bitmaps\" patch\nactually predates the \"reachable from recent\" one. I just didn't clean\nit up and upstream it until 2019, and failed to notice the bad\ninteraction between the two.\n\n> I assume (2) is not intentional, given that \"git gc --help\" explicitly says\n> \"Any object with modification time newer than the --prune date is kept, along\n> with everything reachable from it.\" Is it safe to just run the mark_recent\n> block after the bitmap_git block? Could add_unseen_recent_objects_to_traversal\n> just be called at the start of the bitmap_git block if mark_recent?\n\nSo no, definitely not intentional.\n\nI think we'd just want to run the whole mark_recent block after doing\nthe bitmap traversal.\n\nThere may be some subtlety with reusing the rev_info struct again. I\nthink we'd want to reset the pending objects list after calling into the\nbitmap code. It _usually_ does an actual traversal that consumes the\nlist, but not necessarily. I think traverse_bitmap_commit_list()\nprobably ought to be the one to do it, so it behaves more like\ntraverse_commit_list(). (OTOH, I don't think it's _too_ bad if we don't;\nwe'd include those already-seen objects in our traversal, but they\nshould all by definition have the SEEN bit set, so we'd stop there).\n\nI don't think we want to add_unseen_recent_objects_to_traversal() to\ninclude it as part of the same traversal, for the same reason the\nnon-bitmap traversal does not combine them: the mark_recent traversal is\nbest-effort. We set revs->ignore_missing_links to be tolerant of\nalready-broken segments of history.\n\nIt's possible that we could do the second mark_recent traversal also\nwith bitmaps (but still separately). I can't offhand think of a reason\nthat ignore_missing_links wouldn't behave well there. But since we\nexpect it to be small, I'd be more comfortable just using the regular\ntraversal code.\n\n-Peff\n"},{"id":"423109","messageId":"YIgqIiCeiTISIio1@coredump.intra.peff.net","threadId":"55569","inReplyTo":"YIgijn93f639Pp7Z@coredump.intra.peff.net","subject":"Re: Two issues with mark_reachable_objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-27T15:13:38Z","receivedAt":"2021-04-27T15:13:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 27, 2021 at 10:41:18AM -0400, Jeff King wrote:\n\n> I think we'd just want to run the whole mark_recent block after doing\n> the bitmap traversal.\n> \n> There may be some subtlety with reusing the rev_info struct again. I\n> think we'd want to reset the pending objects list after calling into the\n> bitmap code. It _usually_ does an actual traversal that consumes the\n> list, but not necessarily. I think traverse_bitmap_commit_list()\n> probably ought to be the one to do it, so it behaves more like\n> traverse_commit_list(). (OTOH, I don't think it's _too_ bad if we don't;\n> we'd include those already-seen objects in our traversal, but they\n> should all by definition have the SEEN bit set, so we'd stop there).\n\nNope, I was wrong here. It's actually prepare_bitmap_walk() which would\nwant to clear the pending list, and it does so (it may later re-add\nobjects in find_objects(), but if it does so, it will definitely\ntraverse and consume them).\n\n> It's possible that we could do the second mark_recent traversal also\n> with bitmaps (but still separately). I can't offhand think of a reason\n> that ignore_missing_links wouldn't behave well there. But since we\n> expect it to be small, I'd be more comfortable just using the regular\n> traversal code.\n\nI poked at this a bit, and indeed, the bitmap code is not ready to\nhandle the caller passing ignore_missing_links (it performs two separate\ntraversals for the wanted and uninteresting objects, and manipulates\nignore_missing_links itself between the two). It would probably be easy\nto change, but I think we should focus on the minimal fix for the bug\nyou found first.\n\n-Peff\n"},{"id":"423112","messageId":"YIgxPtDmr9sYj0ft@coredump.intra.peff.net","threadId":"55569","inReplyTo":"CAJ-dYSOVx0egnyxJb6ZjgWvEDR=19QPgc70JQ7cXUjUPZ1XDiQ@mail.gmail.com","subject":"[PATCH] prune: save reachable-from-recent objects with bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-27T15:43:58Z","receivedAt":"2021-04-27T15:44:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 27, 2021 at 11:45:01AM +0100, David Emett wrote:\n\n> I assume (2) is not intentional, given that \"git gc --help\" explicitly says\n> \"Any object with modification time newer than the --prune date is kept, along\n> with everything reachable from it.\" Is it safe to just run the mark_recent\n> block after the bitmap_git block? Could add_unseen_recent_objects_to_traversal\n> just be called at the start of the bitmap_git block if mark_recent?\n\nHere's a fix. Thanks very much for reporting.\n\nI was a little surprised you saw this with \"git gc\", as when I tried\ntesting with that, I found that the \"git repack\" run before \"git prune\"\nworks around the bug (see the discussion of t6501 below). But I think\nperhaps it is just that \"gc --auto\" is more willing to do a \"repack -d\"\nsometimes, rather than a full \"repack -A\". At any rate, I was able to\neasily reproduce it for the tests with just git-prune.\n\n-- >8 --\nSubject: prune: save reachable-from-recent objects with bitmaps\n\nWe pass our prune expiration to mark_reachable_objects(), which will\ntraverse not only the reachable objects, but consider any recent ones as\ntips for reachability; see d3038d22f9 (prune: keep objects reachable\nfrom recent objects, 2014-10-15) for details.\n\nHowever, this interacts badly with the bitmap code path added in\nfde67d6896 (prune: use bitmaps for reachability traversal, 2019-02-13).\nIf we hit the bitmap-optimized path, we return immediately to avoid the\nregular traversal, accidentally skipping the \"also traverse recent\"\ncode.\n\nInstead, we should do an if-else for the bitmap versus regular\ntraversal, and then follow up with the \"recent\" traversal in either\ncase. This reuses the \"rev_info\" for a bitmap and then a regular\ntraversal, but that should work OK (the bitmap code clears the pending\narray in the usual way, just like a regular traversal would).\n\nNote that I dropped the comment above the regular traversal here.  It\nhas little explanatory value, and makes the if-else logic much harder to\nread.\n\nHere are a few variants that I rejected:\n\n  - it seems like both the reachability and recent traversals could be\n    done in a single traversal. This was rejected by d3038d22f9 (prune:\n    keep objects reachable from recent objects, 2014-10-15), though the\n    balance may be different when using bitmaps. However, there's a\n    subtle correctness issue, too: we use revs->ignore_missing_links for\n    the recent traversal, but not the reachability one.\n\n  - we could try using bitmaps for the recent traversal, too, which\n    could possibly improve performance. But it would require some fixes\n    in the bitmap code, which uses ignore_missing_links for its own\n    purposes. Plus it would probably not help all that much in practice.\n    We use the reachable tips to generate bitmaps, so those objects are\n    likely not covered by bitmaps (unless they just became unreachable).\n    And in general, we expect the set of unreachable objects to be much\n    smaller anyway, so there's less to gain.\n\nThe test in t5304 detects the bug and confirms the fix.\n\nI also beefed up the tests in t6501, which covers the mtime-checking\ncode more thoroughly, to handle the bitmap case (in addition to just\n\"loose\" and \"packed\" cases). Interestingly, this test doesn't actually\ndetect the bug, because it is running \"git gc\", and not \"prune\"\ndirectly. And \"gc\" will call \"repack\" first, which does not suffer the\nsame bug. So the old-but-reachable-from-recent objects get scooped up\ninto the new pack along with the actually-recent objects, which gives\nboth a recent mtime. But it seemed prudent to get more coverage of the\nbitmap case for related code.\n\nReported-by: David Emett <dave@sp4m.net>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n reachable.c                | 13 ++++---------\n t/t5304-prune.sh           | 13 +++++++++++++\n t/t6501-freshen-objects.sh | 21 +++++++++++++++------\n 3 files changed, 32 insertions(+), 15 deletions(-)\n\ndiff --git a/reachable.c b/reachable.c\nindex 77a60c70a5..a088717eb5 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -227,17 +227,12 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \tif (bitmap_git) {\n \t\ttraverse_bitmap_commit_list(bitmap_git, revs, mark_object_seen);\n \t\tfree_bitmap_index(bitmap_git);\n-\t\treturn;\n+\t} else {\n+\t\tif (prepare_revision_walk(revs))\n+\t\t\tdie(\"revision walk setup failed\");\n+\t\ttraverse_commit_list(revs, mark_commit, mark_object, &cp);\n \t}\n \n-\t/*\n-\t * Set up the revision walk - this will move all commits\n-\t * from the pending list to the commit walking list.\n-\t */\n-\tif (prepare_revision_walk(revs))\n-\t\tdie(\"revision walk setup failed\");\n-\ttraverse_commit_list(revs, mark_commit, mark_object, &cp);\n-\n \tif (mark_recent) {\n \t\trevs->ignore_missing_links = 1;\n \t\tif (add_unseen_recent_objects_to_traversal(revs, mark_recent))\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex b447ce56a9..20fcc2da1f 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -352,4 +352,17 @@ test_expect_success 'trivial prune with bitmaps enabled' '\n \ttest_must_fail git cat-file -e $blob\n '\n \n+test_expect_success 'old reachable-from-recent retained with bitmaps' '\n+\tgit repack -adb &&\n+\tto_drop=$(echo bitmap-from-recent-1 | git hash-object -w --stdin) &&\n+\ttest-tool chmtime -86400 .git/objects/$(test_oid_to_path $to_drop) &&\n+\tto_save=$(echo bitmap-from-recent-2 | git hash-object -w --stdin) &&\n+\ttest-tool chmtime -86400 .git/objects/$(test_oid_to_path $to_save) &&\n+\ttree=$(printf \"100644 blob $to_save\\tfile\\n\" | git mktree) &&\n+\tgit prune --expire=12.hours.ago &&\n+\tgit cat-file -e $tree &&\n+\tgit cat-file -e $to_save &&\n+\ttest_must_fail git cat-file -e $to_drop\n+'\n+\n test_done\ndiff --git a/t/t6501-freshen-objects.sh b/t/t6501-freshen-objects.sh\nindex 75210f012b..de7742cc51 100755\n--- a/t/t6501-freshen-objects.sh\n+++ b/t/t6501-freshen-objects.sh\n@@ -43,15 +43,24 @@ commit () {\n }\n \n maybe_repack () {\n-\tif test -n \"$repack\"; then\n+\tcase \"$title\" in\n+\tloose)\n+\t\t: skip repack\n+\t\t;;\n+\trepack)\n \t\tgit repack -ad\n-\tfi\n+\t\t;;\n+\tbitmap)\n+\t\tgit repack -adb\n+\t\t;;\n+\t*)\n+\t\techo >&2 \"unknown test type in maybe_repack\"\n+\t\treturn 1\n+\t\t;;\n+\tesac\n }\n \n-for repack in '' true; do\n-\ttitle=${repack:+repack}\n-\ttitle=${title:-loose}\n-\n+for title in loose repack bitmap; do\n \ttest_expect_success \"make repo completely empty ($title)\" '\n \t\trm -rf .git &&\n \t\tgit init\n-- \n2.31.1.789.g4530770a26\n\n"},{"id":"423193","messageId":"CAJ-dYSOHEfRq3p_MQMJoHf5PjwQtbJOzJ9Dzxrfe+BDkov7Q8w@mail.gmail.com","threadId":"55569","inReplyTo":"YIgxPtDmr9sYj0ft@coredump.intra.peff.net","subject":"Re: [PATCH] prune: save reachable-from-recent objects with bitmaps","fromName":"David Emett","fromEmail":"dave@sp4m.net","sentAt":"2021-04-28T12:20:03Z","receivedAt":"2021-04-28T12:20:19Z","isPatch":true,"sender":{"key":"dave@sp4m.net","avatar":null},"body":"> Here's a fix. Thanks very much for reporting.\n\nThanks for the quick response! I tried the fix out on the repo I was having\ntrouble with. It's hitting a segfault in traverse_commit_list in the\nmark_recent block. It looks like the issue is that the bitmap code leaves\nrevs->include_check set, with revs->include_check_data pointing at the stack.\nSetting revs->include_check to NULL after the traverse_commit_list call in\nfind_objects in pack-bitmap.c fixes the segfault for me. And the original issue\nappears to be resolved as well, so thanks!\n\n> I was a little surprised you saw this with \"git gc\", as when I tried\n> testing with that, I found that the \"git repack\" run before \"git prune\"\n> works around the bug (see the discussion of t6501 below). But I think\n> perhaps it is just that \"gc --auto\" is more willing to do a \"repack -d\"\n> sometimes, rather than a full \"repack -A\". At any rate, I was able to\n> easily reproduce it for the tests with just git-prune.\n\nI can't say for sure that this bug is what I was seeing originally, however it\ndoes seem quite likely -- the commits that hit issues were minor updates of\ncommits that had been fetched a month or so earlier, and so would certainly fit\nthe bill. Like you though I've just been reproducing with git prune.\n"},{"id":"423197","messageId":"YIl7tUJA6sQe2K73@coredump.intra.peff.net","threadId":"55569","inReplyTo":"CAJ-dYSOHEfRq3p_MQMJoHf5PjwQtbJOzJ9Dzxrfe+BDkov7Q8w@mail.gmail.com","subject":"Re: [PATCH] prune: save reachable-from-recent objects with bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-28T15:13:57Z","receivedAt":"2021-04-28T15:14:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 28, 2021 at 01:20:03PM +0100, David Emett wrote:\n\n> > Here's a fix. Thanks very much for reporting.\n> \n> Thanks for the quick response! I tried the fix out on the repo I was having\n> trouble with. It's hitting a segfault in traverse_commit_list in the\n> mark_recent block. It looks like the issue is that the bitmap code leaves\n> revs->include_check set, with revs->include_check_data pointing at the stack.\n> Setting revs->include_check to NULL after the traverse_commit_list call in\n> find_objects in pack-bitmap.c fixes the segfault for me. And the original issue\n> appears to be resolved as well, so thanks!\n\nOof, good catch. I wondered why my test didn't see this. The answer is\nthat the include_check only applies to commits, not trees. And my test\nonly has a recent-but-unreachable tree. Beefing it up to include an\nactual commit shows the problem (and ASan nicely pinpoints the issue).\n\nThe solution, as you noted, is to have the bitmap code clean up after\nitself.\n\nI'll prepare a re-roll of the series.\n\n-Peff\n"},{"id":"423198","messageId":"YImCNXC5DUvy5gT8@coredump.intra.peff.net","threadId":"55569","inReplyTo":"YIl7tUJA6sQe2K73@coredump.intra.peff.net","subject":"[PATCH v2 0/2] prune: save reachable-from-recent objects with bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-28T15:41:41Z","receivedAt":"2021-04-28T15:44:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 28, 2021 at 11:13:57AM -0400, Jeff King wrote:\n\n> On Wed, Apr 28, 2021 at 01:20:03PM +0100, David Emett wrote:\n> \n> > > Here's a fix. Thanks very much for reporting.\n> > \n> > Thanks for the quick response! I tried the fix out on the repo I was having\n> > trouble with. It's hitting a segfault in traverse_commit_list in the\n> > mark_recent block. It looks like the issue is that the bitmap code leaves\n> > revs->include_check set, with revs->include_check_data pointing at the stack.\n> > Setting revs->include_check to NULL after the traverse_commit_list call in\n> > find_objects in pack-bitmap.c fixes the segfault for me. And the original issue\n> > appears to be resolved as well, so thanks!\n> \n> Oof, good catch. I wondered why my test didn't see this. The answer is\n> that the include_check only applies to commits, not trees. And my test\n> only has a recent-but-unreachable tree. Beefing it up to include an\n> actual commit shows the problem (and ASan nicely pinpoints the issue).\n> \n> The solution, as you noted, is to have the bitmap code clean up after\n> itself.\n> \n> I'll prepare a re-roll of the series.\n\nHere it is.\n\nI did notice one other oddity. The bitmap code doesn't do any sort of\nprogress reporting. So progress is enabled, we'll _just_ count the\nrecent-but-unreachable entries in the progress meter.\n\nThat's not entirely wrong (it represents the actual traversal work we're\ndoing), but is a little funny. However, I think the best thing to do for\nnow is to leave it there. The right path forward is for the bitmap code\nto learn about progress reporting, too (for this case, but also for\nothers). But that's a more involved topic, and I don't want to hold up\nthis fix for it.\n\n  [1/2]: pack-bitmap: clean up include_check after use\n  [2/2]: prune: save reachable-from-recent objects with bitmaps\n\n pack-bitmap.c              |  3 +++\n reachable.c                | 13 ++++---------\n t/t5304-prune.sh           | 16 ++++++++++++++++\n t/t6501-freshen-objects.sh | 21 +++++++++++++++------\n 4 files changed, 38 insertions(+), 15 deletions(-)\n\n-:  ---------- > 1:  43f25e88c5 pack-bitmap: clean up include_check after use\n1:  a7fb2a7f5b ! 2:  0a60ea597d prune: save reachable-from-recent objects with bitmaps\n    @@ t/t5304-prune.sh: test_expect_success 'trivial prune with bitmaps enabled' '\n     +\tto_save=$(echo bitmap-from-recent-2 | git hash-object -w --stdin) &&\n     +\ttest-tool chmtime -86400 .git/objects/$(test_oid_to_path $to_save) &&\n     +\ttree=$(printf \"100644 blob $to_save\\tfile\\n\" | git mktree) &&\n    ++\ttest-tool chmtime -86400 .git/objects/$(test_oid_to_path $tree) &&\n    ++\tcommit=$(echo foo | git commit-tree $tree) &&\n     +\tgit prune --expire=12.hours.ago &&\n    ++\tgit cat-file -e $commit &&\n     +\tgit cat-file -e $tree &&\n     +\tgit cat-file -e $to_save &&\n     +\ttest_must_fail git cat-file -e $to_drop\n\n-Peff\n"},{"id":"423199","messageId":"YImCZpLA0TjAkU9/@coredump.intra.peff.net","threadId":"55569","inReplyTo":"YImCNXC5DUvy5gT8@coredump.intra.peff.net","subject":"[PATCH v2 1/2] pack-bitmap: clean up include_check after use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-28T15:42:30Z","receivedAt":"2021-04-28T15:44:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When a bitmap walk has to traverse (to fill in non-bitmapped objects),\nwe use rev_info's include_check mechanism to let us stop the traversal\nearly. But after setting the function and its data parameter, we never\nclean it up. This means that if the rev_info is used for a subsequent\ntraversal without bitmaps, it will unexpectedly call into our\ninclude_check function (worse, it will do so pointing to a now-defunct\nstack variable in include_check_data, likely resulting in a segfault).\n\nThere's no code which does this now, but it's an accident waiting to\nhappen. Let's clean up after ourselves in the bitmap code.\n\nReported-by: David Emett <dave@sp4m.net>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pack-bitmap.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 3ed15431cd..f2b59fbf48 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -631,6 +631,9 @@ static struct bitmap *find_objects(struct bitmap_index *bitmap_git,\n \t\ttraverse_commit_list_filtered(filter, revs,\n \t\t\t\t\t      show_commit, show_object,\n \t\t\t\t\t      &show_data, NULL);\n+\n+\t\trevs->include_check = NULL;\n+\t\trevs->include_check_data = NULL;\n \t}\n \n \treturn base;\n-- \n2.31.1.791.g8400859cdc\n\n"},{"id":"423200","messageId":"YImCcxVq6AWjtv8k@coredump.intra.peff.net","threadId":"55569","inReplyTo":"YImCNXC5DUvy5gT8@coredump.intra.peff.net","subject":"[PATCH v2 2/2] prune: save reachable-from-recent objects with bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-28T15:42:43Z","receivedAt":"2021-04-28T15:44:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We pass our prune expiration to mark_reachable_objects(), which will\ntraverse not only the reachable objects, but consider any recent ones as\ntips for reachability; see d3038d22f9 (prune: keep objects reachable\nfrom recent objects, 2014-10-15) for details.\n\nHowever, this interacts badly with the bitmap code path added in\nfde67d6896 (prune: use bitmaps for reachability traversal, 2019-02-13).\nIf we hit the bitmap-optimized path, we return immediately to avoid the\nregular traversal, accidentally skipping the \"also traverse recent\"\ncode.\n\nInstead, we should do an if-else for the bitmap versus regular\ntraversal, and then follow up with the \"recent\" traversal in either\ncase. This reuses the \"rev_info\" for a bitmap and then a regular\ntraversal, but that should work OK (the bitmap code clears the pending\narray in the usual way, just like a regular traversal would).\n\nNote that I dropped the comment above the regular traversal here.  It\nhas little explanatory value, and makes the if-else logic much harder to\nread.\n\nHere are a few variants that I rejected:\n\n  - it seems like both the reachability and recent traversals could be\n    done in a single traversal. This was rejected by d3038d22f9 (prune:\n    keep objects reachable from recent objects, 2014-10-15), though the\n    balance may be different when using bitmaps. However, there's a\n    subtle correctness issue, too: we use revs->ignore_missing_links for\n    the recent traversal, but not the reachability one.\n\n  - we could try using bitmaps for the recent traversal, too, which\n    could possibly improve performance. But it would require some fixes\n    in the bitmap code, which uses ignore_missing_links for its own\n    purposes. Plus it would probably not help all that much in practice.\n    We use the reachable tips to generate bitmaps, so those objects are\n    likely not covered by bitmaps (unless they just became unreachable).\n    And in general, we expect the set of unreachable objects to be much\n    smaller anyway, so there's less to gain.\n\nThe test in t5304 detects the bug and confirms the fix.\n\nI also beefed up the tests in t6501, which covers the mtime-checking\ncode more thoroughly, to handle the bitmap case (in addition to just\n\"loose\" and \"packed\" cases). Interestingly, this test doesn't actually\ndetect the bug, because it is running \"git gc\", and not \"prune\"\ndirectly. And \"gc\" will call \"repack\" first, which does not suffer the\nsame bug. So the old-but-reachable-from-recent objects get scooped up\ninto the new pack along with the actually-recent objects, which gives\nboth a recent mtime. But it seemed prudent to get more coverage of the\nbitmap case for related code.\n\nReported-by: David Emett <dave@sp4m.net>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n reachable.c                | 13 ++++---------\n t/t5304-prune.sh           | 16 ++++++++++++++++\n t/t6501-freshen-objects.sh | 21 +++++++++++++++------\n 3 files changed, 35 insertions(+), 15 deletions(-)\n\ndiff --git a/reachable.c b/reachable.c\nindex 77a60c70a5..a088717eb5 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -227,17 +227,12 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \tif (bitmap_git) {\n \t\ttraverse_bitmap_commit_list(bitmap_git, revs, mark_object_seen);\n \t\tfree_bitmap_index(bitmap_git);\n-\t\treturn;\n+\t} else {\n+\t\tif (prepare_revision_walk(revs))\n+\t\t\tdie(\"revision walk setup failed\");\n+\t\ttraverse_commit_list(revs, mark_commit, mark_object, &cp);\n \t}\n \n-\t/*\n-\t * Set up the revision walk - this will move all commits\n-\t * from the pending list to the commit walking list.\n-\t */\n-\tif (prepare_revision_walk(revs))\n-\t\tdie(\"revision walk setup failed\");\n-\ttraverse_commit_list(revs, mark_commit, mark_object, &cp);\n-\n \tif (mark_recent) {\n \t\trevs->ignore_missing_links = 1;\n \t\tif (add_unseen_recent_objects_to_traversal(revs, mark_recent))\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex b447ce56a9..3475b06aeb 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -352,4 +352,20 @@ test_expect_success 'trivial prune with bitmaps enabled' '\n \ttest_must_fail git cat-file -e $blob\n '\n \n+test_expect_success 'old reachable-from-recent retained with bitmaps' '\n+\tgit repack -adb &&\n+\tto_drop=$(echo bitmap-from-recent-1 | git hash-object -w --stdin) &&\n+\ttest-tool chmtime -86400 .git/objects/$(test_oid_to_path $to_drop) &&\n+\tto_save=$(echo bitmap-from-recent-2 | git hash-object -w --stdin) &&\n+\ttest-tool chmtime -86400 .git/objects/$(test_oid_to_path $to_save) &&\n+\ttree=$(printf \"100644 blob $to_save\\tfile\\n\" | git mktree) &&\n+\ttest-tool chmtime -86400 .git/objects/$(test_oid_to_path $tree) &&\n+\tcommit=$(echo foo | git commit-tree $tree) &&\n+\tgit prune --expire=12.hours.ago &&\n+\tgit cat-file -e $commit &&\n+\tgit cat-file -e $tree &&\n+\tgit cat-file -e $to_save &&\n+\ttest_must_fail git cat-file -e $to_drop\n+'\n+\n test_done\ndiff --git a/t/t6501-freshen-objects.sh b/t/t6501-freshen-objects.sh\nindex 75210f012b..de7742cc51 100755\n--- a/t/t6501-freshen-objects.sh\n+++ b/t/t6501-freshen-objects.sh\n@@ -43,15 +43,24 @@ commit () {\n }\n \n maybe_repack () {\n-\tif test -n \"$repack\"; then\n+\tcase \"$title\" in\n+\tloose)\n+\t\t: skip repack\n+\t\t;;\n+\trepack)\n \t\tgit repack -ad\n-\tfi\n+\t\t;;\n+\tbitmap)\n+\t\tgit repack -adb\n+\t\t;;\n+\t*)\n+\t\techo >&2 \"unknown test type in maybe_repack\"\n+\t\treturn 1\n+\t\t;;\n+\tesac\n }\n \n-for repack in '' true; do\n-\ttitle=${repack:+repack}\n-\ttitle=${title:-loose}\n-\n+for title in loose repack bitmap; do\n \ttest_expect_success \"make repo completely empty ($title)\" '\n \t\trm -rf .git &&\n \t\tgit init\n-- \n2.31.1.791.g8400859cdc\n"},{"id":"423225","messageId":"xmqqh7jpan0y.fsf@gitster.g","threadId":"55569","inReplyTo":"YImCcxVq6AWjtv8k@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] prune: save reachable-from-recent objects with bitmaps","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-29T01:37:49Z","receivedAt":"2021-04-29T01:37:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> -for repack in '' true; do\n> -\ttitle=${repack:+repack}\n> -\ttitle=${title:-loose}\n> -\n> +for title in loose repack bitmap; do\n>  \ttest_expect_success \"make repo completely empty ($title)\" '\n>  \t\trm -rf .git &&\n>  \t\tgit init\n\nJust this part alone is worth the update ;-) It was not even clear\nwhat \"repack\" meant in the old loop, or what the significance of\nsetting it to \"true\" (as opposed to say \"false\").\n\nOf course, the early return in the code removed by this patch\nexplains how the bug happened.  Will queue; thanks.\n"}]}