{"thread":{"id":"58614","subject":"[PATCH] builtin/repack.c: remove redundant pack-based bitmaps","startedAt":"2022-10-12T19:05:40Z","lastAt":"2022-10-18T06:29:12Z","messageCount":10,"participants":["Taylor Blau","Jeff King","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"464744","messageId":"393fd4c6db78cd694e6d4dfcf24f17e2850ccd99.1665601403.git.me@ttaylorr.com","threadId":"58614","inReplyTo":null,"subject":"[PATCH] builtin/repack.c: remove redundant pack-based bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-12T19:05:33Z","receivedAt":"2022-10-12T19:05:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When we write a MIDX bitmap after repacking, it is possible that the\nrepository would be left in a state with both pack- and multi-pack\nreachability bitmaps.\n\nThis can occur, for instance, if a pack that was kept (either by having\na .keep file, or during a geometric repack in which it is not rolled up)\nhas a bitmap file, and the repack wrote a multi-pack index and bitmap.\n\nWhen loading a reachability bitmap for the repository, the multi-pack\none is always preferred, so the pack-based one is redundant. Let's\nremove it unconditionally, even if '-d' isn't passed, since there is no\npractical reason to keep both around. The patch below does just that.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c  | 40 +++++++++++++++++++++++++++++++++++++---\n t/t7700-repack.sh | 21 +++++++++++++++++++++\n 2 files changed, 58 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex a5bacc7797..59001e8020 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -661,6 +661,35 @@ static int write_midx_included_packs(struct string_list *include,\n \treturn finish_command(&cmd);\n }\n \n+static void remove_redundant_bitmaps(struct string_list *include,\n+\t\t\t\t     const char *packdir)\n+{\n+\tstruct strbuf path = STRBUF_INIT;\n+\tstruct string_list_item *item;\n+\tsize_t packdir_len;\n+\n+\tstrbuf_addstr(&path, packdir);\n+\tstrbuf_addch(&path, '/');\n+\tpackdir_len = path.len;\n+\n+\t/*\n+\t * Remove any pack bitmaps corresponding to packs which are now\n+\t * included in the MIDX.\n+\t */\n+\tfor_each_string_list_item(item, include) {\n+\t\tstrbuf_addstr(&path, item->string);\n+\t\tstrbuf_strip_suffix(&path, \".idx\");\n+\t\tstrbuf_addstr(&path, \".bitmap\");\n+\n+\t\tif (unlink(path.buf) && errno != ENOENT)\n+\t\t\tdie_errno(_(\"could not remove stale bitmap: %s\"),\n+\t\t\t\t  path.buf);\n+\n+\t\tstrbuf_setlen(&path, packdir_len);\n+\t}\n+\tstrbuf_release(&path);\n+}\n+\n static int write_cruft_pack(const struct pack_objects_args *args,\n \t\t\t    const char *pack_prefix,\n \t\t\t    struct string_list *names,\n@@ -1059,10 +1088,15 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\trefs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,\n \t\t\t\t\t\tshow_progress, write_bitmaps > 0);\n \n+\t\tif (ret) {\n+\t\t\tstring_list_clear(&include, 0);\n+\t\t\treturn ret;\n+\t\t}\n+\n+\t\tif (write_bitmaps)\n+\t\t\tremove_redundant_bitmaps(&include, packdir);\n+\n \t\tstring_list_clear(&include, 0);\n-\n-\t\tif (ret)\n-\t\t\treturn ret;\n \t}\n \n \treprepare_packed_git(the_repository);\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex ca45c4cd2c..2d0e9448dd 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -426,6 +426,27 @@ test_expect_success '--write-midx -b packs non-kept objects' '\n \t)\n '\n \n+test_expect_success '--write-midx removes stale pack-based bitmaps' '\n+       rm -fr repo &&\n+       git init repo &&\n+       test_when_finished \"rm -fr repo\" &&\n+       (\n+\t\tcd repo &&\n+\t\ttest_commit base &&\n+\t\tGIT_TEST_MULTI_PACK_INDEX=0 git repack -Ab &&\n+\n+\t\tpack_bitmap=$(ls $objdir/pack/pack-*.bitmap) &&\n+\t\ttest_path_is_file \"$pack_bitmap\" &&\n+\n+\t\ttest_commit tip &&\n+\t\tGIT_TEST_MULTI_PACK_INDEX=0 git repack -bm &&\n+\n+\t\ttest_path_is_file $midx &&\n+\t\ttest_path_is_file $midx-$(midx_checksum $objdir).bitmap &&\n+\t\ttest_path_is_missing $pack_bitmap\n+       )\n+'\n+\n test_expect_success TTY '--quiet disables progress' '\n \ttest_terminal env GIT_PROGRESS_DELAY=0 \\\n \t\tgit -C midx repack -ad --quiet --write-midx 2>stderr &&\n-- \n2.38.0.16.g393fd4c6db\n"},{"id":"465132","messageId":"Y02YzYS172skpbAb@coredump.intra.peff.net","threadId":"58614","inReplyTo":"393fd4c6db78cd694e6d4dfcf24f17e2850ccd99.1665601403.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/repack.c: remove redundant pack-based bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-17T18:02:53Z","receivedAt":"2022-10-17T18:02:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 12, 2022 at 03:05:33PM -0400, Taylor Blau wrote:\n\n> When we write a MIDX bitmap after repacking, it is possible that the\n> repository would be left in a state with both pack- and multi-pack\n> reachability bitmaps.\n> \n> This can occur, for instance, if a pack that was kept (either by having\n> a .keep file, or during a geometric repack in which it is not rolled up)\n> has a bitmap file, and the repack wrote a multi-pack index and bitmap.\n> \n> When loading a reachability bitmap for the repository, the multi-pack\n> one is always preferred, so the pack-based one is redundant. Let's\n> remove it unconditionally, even if '-d' isn't passed, since there is no\n> practical reason to keep both around. The patch below does just that.\n\nYeah, this is certainly a reasonable thing to be doing. I wonder if you\nwant to share the story of why the original midx-bitmap series did not\ninclude this patch. It is (IMHO, at least) an interesting debugging\ntale.\n\n>  builtin/repack.c  | 40 +++++++++++++++++++++++++++++++++++++---\n>  t/t7700-repack.sh | 21 +++++++++++++++++++++\n>  2 files changed, 58 insertions(+), 3 deletions(-)\n\nThe patch looks good. Two very minor comments, though I'd be happy\nenough to see it merged as-is:\n\n> +static void remove_redundant_bitmaps(struct string_list *include,\n> +\t\t\t\t     const char *packdir)\n> +{\n> +\tstruct strbuf path = STRBUF_INIT;\n> +\tstruct string_list_item *item;\n> +\tsize_t packdir_len;\n> +\n> +\tstrbuf_addstr(&path, packdir);\n> +\tstrbuf_addch(&path, '/');\n> +\tpackdir_len = path.len;\n> +\n> +\t/*\n> +\t * Remove any pack bitmaps corresponding to packs which are now\n> +\t * included in the MIDX.\n> +\t */\n> +\tfor_each_string_list_item(item, include) {\n> +\t\tstrbuf_addstr(&path, item->string);\n> +\t\tstrbuf_strip_suffix(&path, \".idx\");\n> +\t\tstrbuf_addstr(&path, \".bitmap\");\n> +\n> +\t\tif (unlink(path.buf) && errno != ENOENT)\n> +\t\t\tdie_errno(_(\"could not remove stale bitmap: %s\"),\n> +\t\t\t\t  path.buf);\n\nWe could downgrade this to a warning, since there is no downside to\nretaining those files (aside from wasted space). In\nremove_redundant_pack(), we call into unlink_pack_path(), which just\nignores unlink errors (though arguably it should at least warn).\n\n> @@ -1059,10 +1088,15 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \t\t\t\t\t\trefs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,\n>  \t\t\t\t\t\tshow_progress, write_bitmaps > 0);\n>  \n> +\t\tif (ret) {\n> +\t\t\tstring_list_clear(&include, 0);\n> +\t\t\treturn ret;\n> +\t\t}\n> +\n> +\t\tif (write_bitmaps)\n> +\t\t\tremove_redundant_bitmaps(&include, packdir);\n> +\n>  \t\tstring_list_clear(&include, 0);\n> -\n> -\t\tif (ret)\n> -\t\t\treturn ret;\n>  \t}\n\nYou could avoid having to repeat the string-list cleanup here by\nstructuring it like:\n\n  if (!ret && write_bitmaps)\n\tremove_redundant_bitmaps(&include, packdir);\n\n  /* as before, clear string list and possibly return ret */\n\nSince it's only one line, it's not that big a deal, but it simplifies\nthe flow.\n\nIt's correct either way, of course. One thing I did have to do while\nreviewing this was look at this hunk in place. The context omits that\nthis is in the \"if (write_midx)\" conditional, which is of course very\nimportant. ;)\n\n-Peff\n"},{"id":"465137","messageId":"221017.868rlejn4z.gmgdl@evledraar.gmail.com","threadId":"58614","inReplyTo":"Y02YzYS172skpbAb@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/repack.c: remove redundant pack-based bitmaps","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T18:50:07Z","receivedAt":"2022-10-17T18:55:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 17 2022, Jeff King wrote:\n\n> On Wed, Oct 12, 2022 at 03:05:33PM -0400, Taylor Blau wrote:\n> [...]\n>> [...]\n>> +\tfor_each_string_list_item(item, include) {\n>> +\t\tstrbuf_addstr(&path, item->string);\n>> +\t\tstrbuf_strip_suffix(&path, \".idx\");\n>> +\t\tstrbuf_addstr(&path, \".bitmap\");\n>> +\n>> +\t\tif (unlink(path.buf) && errno != ENOENT)\n>> +\t\t\tdie_errno(_(\"could not remove stale bitmap: %s\"),\n>> +\t\t\t\t  path.buf);\n>\n> We could downgrade this to a warning, since there is no downside to\n> retaining those files (aside from wasted space). In\n> remove_redundant_pack(), we call into unlink_pack_path(), which just\n> ignores unlink errors (though arguably it should at least warn).\n\nAn ENOENT would indicate a race with a concurrent process.\n\nThat in itself seems like a wart of our lock management. I.e. we invoked\n\"multi-pack-index\" earlier and took a lock to create the midx.\n\nI don't know how exactly that interact with the pack bitmaps, but should\nwe ideally have taken locks on the relevant associated files instead\nduring the larger operation?\n\nBut anyway, that's a much larger topic, so we could sweep it under the\nrug here.\n\nBut at that point should we be ignoring other unlink() errors?  Maybe we\nshould additionally be ignoring EBUSY.\n\nBut the rest all seem from a quick scan of unlink(2) to be things we'd\nlike to just die on, e.g. EIO.\n"},{"id":"465141","messageId":"Y02shQFeXkwYd2g6@coredump.intra.peff.net","threadId":"58614","inReplyTo":"221017.868rlejn4z.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] builtin/repack.c: remove redundant pack-based bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-17T19:27:01Z","receivedAt":"2022-10-17T19:29:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 17, 2022 at 08:50:07PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> On Mon, Oct 17 2022, Jeff King wrote:\n> \n> > On Wed, Oct 12, 2022 at 03:05:33PM -0400, Taylor Blau wrote:\n> > [...]\n> >> [...]\n> >> +\tfor_each_string_list_item(item, include) {\n> >> +\t\tstrbuf_addstr(&path, item->string);\n> >> +\t\tstrbuf_strip_suffix(&path, \".idx\");\n> >> +\t\tstrbuf_addstr(&path, \".bitmap\");\n> >> +\n> >> +\t\tif (unlink(path.buf) && errno != ENOENT)\n> >> +\t\t\tdie_errno(_(\"could not remove stale bitmap: %s\"),\n> >> +\t\t\t\t  path.buf);\n> >\n> > We could downgrade this to a warning, since there is no downside to\n> > retaining those files (aside from wasted space). In\n> > remove_redundant_pack(), we call into unlink_pack_path(), which just\n> > ignores unlink errors (though arguably it should at least warn).\n> \n> An ENOENT would indicate a race with a concurrent process.\n\nIn the code above, I don't think it is. We included some packs in the\nmidx, and we know that their bitmaps (if any) are now redundant. So we\nremove them by calling unlink() speculatively, rather than asking \"do we\nhave this file? If so, unlink\".\n\nLikewise in remove_redundant_pack(). We are not just removing the\npackfiles, but speculatively removing their .rev, .bitmap, .promisor,\netc. If they don't exist, that's perfectly normal. Arguably we could\nnotice and complain if the .pack or .idx went away in the interim, since\nthose would indicate a race.\n\nSo I think the code above is right to ignore ENOENT. The one in\nremove_redundant_pack() does so, too, but it also ignores everything\nelse.\n\n> But at that point should we be ignoring other unlink() errors?  Maybe we\n> should additionally be ignoring EBUSY.\n\nFor most errors besides ENOENT, I think a warning is the right thing. It\nis OK that we failed to remove the file (it does not make the overall\noperation a failure). But it may be helpful for the user to let them\nknow that the cruft was left (e.g., for EPERM or EIO). And that applies\nhere and in remove_redundant_pack().\n\nReally, both here and remove_redundant_pack() should probably be using\nunlink_or_warn(). Though if you did want to notice the race on deleting\nthe actual .pack file, it would need to be hand-coded.\n\n-Peff\n"},{"id":"465142","messageId":"xmqqlepeyxf6.fsf@gitster.g","threadId":"58614","inReplyTo":"Y02shQFeXkwYd2g6@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/repack.c: remove redundant pack-based bitmaps","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-17T21:04:13Z","receivedAt":"2022-10-17T21:04:24Z","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> Really, both here and remove_redundant_pack() should probably be using\n> unlink_or_warn(). Though if you did want to notice the race on deleting\n> the actual .pack file, it would need to be hand-coded.\n\nYeah, unlink_or_warn() would probably be the right way to go.\n\nOther than that, the patch looks good to you folks?\n\nThanks.\n"},{"id":"465145","messageId":"Y03L0W6TadyhtDij@coredump.intra.peff.net","threadId":"58614","inReplyTo":"xmqqlepeyxf6.fsf@gitster.g","subject":"Re: [PATCH] builtin/repack.c: remove redundant pack-based bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-17T21:40:33Z","receivedAt":"2022-10-17T21:40:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 17, 2022 at 02:04:13PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Really, both here and remove_redundant_pack() should probably be using\n> > unlink_or_warn(). Though if you did want to notice the race on deleting\n> > the actual .pack file, it would need to be hand-coded.\n> \n> Yeah, unlink_or_warn() would probably be the right way to go.\n> \n> Other than that, the patch looks good to you folks?\n\nYeah. I suggested a minor reordering of the one change, but the\nsubstance of the patch looks good to me (and I am OK either way on the\nlittle suggestions).\n\n-Peff\n"},{"id":"465165","messageId":"Y04Rs7V4IpkFtKWC@nand.local","threadId":"58614","inReplyTo":"Y02YzYS172skpbAb@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/repack.c: remove redundant pack-based bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-18T02:38:43Z","receivedAt":"2022-10-18T02:38:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 17, 2022 at 02:02:53PM -0400, Jeff King wrote:\n> On Wed, Oct 12, 2022 at 03:05:33PM -0400, Taylor Blau wrote:\n>\n> > When we write a MIDX bitmap after repacking, it is possible that the\n> > repository would be left in a state with both pack- and multi-pack\n> > reachability bitmaps.\n> >\n> > This can occur, for instance, if a pack that was kept (either by having\n> > a .keep file, or during a geometric repack in which it is not rolled up)\n> > has a bitmap file, and the repack wrote a multi-pack index and bitmap.\n> >\n> > When loading a reachability bitmap for the repository, the multi-pack\n> > one is always preferred, so the pack-based one is redundant. Let's\n> > remove it unconditionally, even if '-d' isn't passed, since there is no\n> > practical reason to keep both around. The patch below does just that.\n>\n> Yeah, this is certainly a reasonable thing to be doing. I wonder if you\n> want to share the story of why the original midx-bitmap series did not\n> include this patch. It is (IMHO, at least) an interesting debugging\n> tale.\n\nSure, though note that this is a mystery that plague me on-and-off for a\nyear or so before finally figuring it out ;-).\n\nWhen we initially deployed MIDX bitmaps at GitHub, I held back this\npatch as a recovery mechanism (ie., if something was horribly broken\nwith MIDX bitmaps, we could quickly deploy a patch to stop reading them,\nknowing that the existing pack bitmaps were still around).\n\nAfter a while building up confidence in the MIDX bitmaps implementation,\nI wrote something that resembles the patch here and deployed it. No big\ndeal, right?\n\nWrong. The \"total time spent running `git repack`\" metrics started\nticking up. But the metrics ticked up even in hosts and sites that had\nnot yet received the patch. Huh?\n\nThis led to many of the trace2 regions in midx.c that I recently[1, 2]\nsent. But after deploying that patch and reverting it over and over\nagain, nothing in the trace2 metrics ended up yielding a satisfying\nanswer.\n\nI spent a few days trying to debug the issue but didn't make any\nprogress. I confirmed that, yes, we were indeed reading MIDX bitmaps and\nnot relying on the pack-based bitmaps for some weird reason. And after I\nconfirmed that, I set the bug aside to work on other things, and mostly\nforgot about it.\n\nBut in an unrelated incident, a colleague of mine noted a bug in a\nGitHub-internal program called `git-get-unpacked-size`. The output of\nthis program is the number of bytes in non-bitmapped packs, and it's\nused as a heuristic to help us pick repository networks to repack\nroughly in order of \"most non-repacked bytes\".\n\nThe bug in `get-unpacked-size` was that we were including the size of\nthe cruft pack, meaning that certain networks would get scheduled much\nmore often. But in fixing that bug, I noticed that packs in the MIDX\nwould also get counted against our unpacked size.\n\nSo I wrote the minimal fix to fix the cruft pack bug, and noted the MIDX\nissue for another day. When we did come back to it (I say \"we\", since\nVictoria Dye implemented the actual fix), I suggested that we ignore any\npacks that appear in a MIDX from the unpacked size, since we only write\na MIDX after repacking a repository.\n\nWhen we did so, I suspected that it might inadvertently solve the original mystery. It did, and here's why.\n\nIndividual maintenance runs did not get slower when removing redundant\npack bitmaps. But because removing the .bitmap file causes that pack to\nbe counted towards a repository's total unpacked size, it biases\nscheduling towards larger repositories (which have bigger packs, and\ntherefore a more inflated overall size). So we weren't slowing anything\ndown, just doing a slightly more expensive maintenance (on bigger\nrepositories) slightly more often. This explained not only the uptick,\nbut why the effect was so resistant to measuring.\n\nIt also explained why we saw the same effect when removing pack bitmaps\nwith MIDX bitmaps deployed to a single site only: whenever `git\nget-unpacked-size` ran on a replica that *did* have the new code, the\nunpacked-size count would get inflated. That explains why we saw\nmaintenance times tick up outside of our testing site.\n\nSo, that's the story :-).\n\n[1]: https://lore.kernel.org/git/dc50527d99680fb0ff1f3240531105badaa221dc.1665612094.git.me@ttaylorr.com/\n[2]: https://lore.kernel.org/git/dd5ceb1ec3c01e8a2af55130718fff0b5eaf2de0.1665612094.git.me@ttaylorr.com/\n\nThanks,\nTaylor\n"},{"id":"465166","messageId":"Y04Sxb/ysk5tgyvZ@nand.local","threadId":"58614","inReplyTo":"Y02YzYS172skpbAb@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/repack.c: remove redundant pack-based bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-18T02:43:17Z","receivedAt":"2022-10-18T02:43:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 17, 2022 at 02:02:53PM -0400, Jeff King wrote:\n> > +\t\tif (unlink(path.buf) && errno != ENOENT)\n> > +\t\t\tdie_errno(_(\"could not remove stale bitmap: %s\"),\n> > +\t\t\t\t  path.buf);\n>\n> We could downgrade this to a warning, since there is no downside to\n> retaining those files (aside from wasted space). In\n> remove_redundant_pack(), we call into unlink_pack_path(), which just\n> ignores unlink errors (though arguably it should at least warn).\n\nI think that downgrading this to `warning_errno()` is appropriate. I'll\nmake that change locally and send a new version.\n\nI think a good separate topic is teaching `remove_redundant_pack()` to\nemit warnings for non-ENOENT errors, too. But I'll leave that for\nanother day :-).\n\n> > @@ -1059,10 +1088,15 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> >  \t\t\t\t\t\trefs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,\n> >  \t\t\t\t\t\tshow_progress, write_bitmaps > 0);\n> >\n> > +\t\tif (ret) {\n> > +\t\t\tstring_list_clear(&include, 0);\n> > +\t\t\treturn ret;\n> > +\t\t}\n> > +\n> > +\t\tif (write_bitmaps)\n> > +\t\t\tremove_redundant_bitmaps(&include, packdir);\n> > +\n> >  \t\tstring_list_clear(&include, 0);\n> > -\n> > -\t\tif (ret)\n> > -\t\t\treturn ret;\n> >  \t}\n>\n> You could avoid having to repeat the string-list cleanup here by\n> structuring it like:\n>\n>   if (!ret && write_bitmaps)\n> \tremove_redundant_bitmaps(&include, packdir);\n>\n>   /* as before, clear string list and possibly return ret */\n>\n> Since it's only one line, it's not that big a deal, but it simplifies\n> the flow.\n>\n> It's correct either way, of course. One thing I did have to do while\n> reviewing this was look at this hunk in place. The context omits that\n> this is in the \"if (write_midx)\" conditional, which is of course very\n> important. ;)\n\nGreat suggestion, thanks. I'll apply it locally and send a reroll now.\n\nThanks,\nTaylor\n"},{"id":"465167","messageId":"1e0ef7ee7ff5feb323c77e594cd65433fb1d99f7.1666061096.git.me@ttaylorr.com","threadId":"58614","inReplyTo":"393fd4c6db78cd694e6d4dfcf24f17e2850ccd99.1665601403.git.me@ttaylorr.com","subject":"[PATCH v2] builtin/repack.c: remove redundant pack-based bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-18T02:45:12Z","receivedAt":"2022-10-18T02:45:33Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When we write a MIDX bitmap after repacking, it is possible that the\nrepository would be left in a state with both pack- and multi-pack\nreachability bitmaps.\n\nThis can occur, for instance, if a pack that was kept (either by having\na .keep file, or during a geometric repack in which it is not rolled up)\nhas a bitmap file, and the repack wrote a multi-pack index and bitmap.\n\nWhen loading a reachability bitmap for the repository, the multi-pack\none is always preferred, so the pack-based one is redundant. Let's\nremove it unconditionally, even if '-d' isn't passed, since there is no\npractical reason to keep both around. The patch below does just that.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\nA small reroll to address a pair of comments from Peff.\n\nRange-diff against v1:\n1:  393fd4c6db ! 1:  1e0ef7ee7f builtin/repack.c: remove redundant pack-based bitmaps\n    @@ builtin/repack.c: static int write_midx_included_packs(struct string_list *inclu\n     +\t\tstrbuf_addstr(&path, \".bitmap\");\n     +\n     +\t\tif (unlink(path.buf) && errno != ENOENT)\n    -+\t\t\tdie_errno(_(\"could not remove stale bitmap: %s\"),\n    -+\t\t\t\t  path.buf);\n    ++\t\t\twarning_errno(_(\"could not remove stale bitmap: %s\"),\n    ++\t\t\t\t      path.buf);\n     +\n     +\t\tstrbuf_setlen(&path, packdir_len);\n     +\t}\n    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix\n      \t\t\t\t\t\trefs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,\n      \t\t\t\t\t\tshow_progress, write_bitmaps > 0);\n\n    -+\t\tif (ret) {\n    -+\t\t\tstring_list_clear(&include, 0);\n    -+\t\t\treturn ret;\n    -+\t\t}\n    -+\n    -+\t\tif (write_bitmaps)\n    ++\t\tif (!ret && write_bitmaps)\n     +\t\t\tremove_redundant_bitmaps(&include, packdir);\n     +\n      \t\tstring_list_clear(&include, 0);\n    --\n    --\t\tif (ret)\n    --\t\t\treturn ret;\n    - \t}\n\n    - \treprepare_packed_git(the_repository);\n    + \t\tif (ret)\n\n      ## t/t7700-repack.sh ##\n     @@ t/t7700-repack.sh: test_expect_success '--write-midx -b packs non-kept objects' '\n\n builtin/repack.c  | 32 ++++++++++++++++++++++++++++++++\n t/t7700-repack.sh | 21 +++++++++++++++++++++\n 2 files changed, 53 insertions(+)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex a5bacc7797..c2d2e52bd4 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -661,6 +661,35 @@ static int write_midx_included_packs(struct string_list *include,\n \treturn finish_command(&cmd);\n }\n\n+static void remove_redundant_bitmaps(struct string_list *include,\n+\t\t\t\t     const char *packdir)\n+{\n+\tstruct strbuf path = STRBUF_INIT;\n+\tstruct string_list_item *item;\n+\tsize_t packdir_len;\n+\n+\tstrbuf_addstr(&path, packdir);\n+\tstrbuf_addch(&path, '/');\n+\tpackdir_len = path.len;\n+\n+\t/*\n+\t * Remove any pack bitmaps corresponding to packs which are now\n+\t * included in the MIDX.\n+\t */\n+\tfor_each_string_list_item(item, include) {\n+\t\tstrbuf_addstr(&path, item->string);\n+\t\tstrbuf_strip_suffix(&path, \".idx\");\n+\t\tstrbuf_addstr(&path, \".bitmap\");\n+\n+\t\tif (unlink(path.buf) && errno != ENOENT)\n+\t\t\twarning_errno(_(\"could not remove stale bitmap: %s\"),\n+\t\t\t\t      path.buf);\n+\n+\t\tstrbuf_setlen(&path, packdir_len);\n+\t}\n+\tstrbuf_release(&path);\n+}\n+\n static int write_cruft_pack(const struct pack_objects_args *args,\n \t\t\t    const char *pack_prefix,\n \t\t\t    struct string_list *names,\n@@ -1059,6 +1088,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\trefs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,\n \t\t\t\t\t\tshow_progress, write_bitmaps > 0);\n\n+\t\tif (!ret && write_bitmaps)\n+\t\t\tremove_redundant_bitmaps(&include, packdir);\n+\n \t\tstring_list_clear(&include, 0);\n\n \t\tif (ret)\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex ca45c4cd2c..2d0e9448dd 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -426,6 +426,27 @@ test_expect_success '--write-midx -b packs non-kept objects' '\n \t)\n '\n\n+test_expect_success '--write-midx removes stale pack-based bitmaps' '\n+       rm -fr repo &&\n+       git init repo &&\n+       test_when_finished \"rm -fr repo\" &&\n+       (\n+\t\tcd repo &&\n+\t\ttest_commit base &&\n+\t\tGIT_TEST_MULTI_PACK_INDEX=0 git repack -Ab &&\n+\n+\t\tpack_bitmap=$(ls $objdir/pack/pack-*.bitmap) &&\n+\t\ttest_path_is_file \"$pack_bitmap\" &&\n+\n+\t\ttest_commit tip &&\n+\t\tGIT_TEST_MULTI_PACK_INDEX=0 git repack -bm &&\n+\n+\t\ttest_path_is_file $midx &&\n+\t\ttest_path_is_file $midx-$(midx_checksum $objdir).bitmap &&\n+\t\ttest_path_is_missing $pack_bitmap\n+       )\n+'\n+\n test_expect_success TTY '--quiet disables progress' '\n \ttest_terminal env GIT_PROGRESS_DELAY=0 \\\n \t\tgit -C midx repack -ad --quiet --write-midx 2>stderr &&\n--\n2.38.0.16.g393fd4c6db\n"},{"id":"465173","messageId":"Y05HrVT9HxxDiXc2@coredump.intra.peff.net","threadId":"58614","inReplyTo":"1e0ef7ee7ff5feb323c77e594cd65433fb1d99f7.1666061096.git.me@ttaylorr.com","subject":"Re: [PATCH v2] builtin/repack.c: remove redundant pack-based bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-18T06:29:01Z","receivedAt":"2022-10-18T06:29:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 17, 2022 at 10:45:12PM -0400, Taylor Blau wrote:\n\n> When we write a MIDX bitmap after repacking, it is possible that the\n> repository would be left in a state with both pack- and multi-pack\n> reachability bitmaps.\n> \n> This can occur, for instance, if a pack that was kept (either by having\n> a .keep file, or during a geometric repack in which it is not rolled up)\n> has a bitmap file, and the repack wrote a multi-pack index and bitmap.\n> \n> When loading a reachability bitmap for the repository, the multi-pack\n> one is always preferred, so the pack-based one is redundant. Let's\n> remove it unconditionally, even if '-d' isn't passed, since there is no\n> practical reason to keep both around. The patch below does just that.\n\nThanks, this version looks great to me.\n\n> 1:  393fd4c6db ! 1:  1e0ef7ee7f builtin/repack.c: remove redundant pack-based bitmaps\n>     @@ builtin/repack.c: static int write_midx_included_packs(struct string_list *inclu\n>      +\t\tstrbuf_addstr(&path, \".bitmap\");\n>      +\n>      +\t\tif (unlink(path.buf) && errno != ENOENT)\n>     -+\t\t\tdie_errno(_(\"could not remove stale bitmap: %s\"),\n>     -+\t\t\t\t  path.buf);\n>     ++\t\t\twarning_errno(_(\"could not remove stale bitmap: %s\"),\n>     ++\t\t\t\t      path.buf);\n\nAs noted elsewhere in the thread, this could be unlink_or_warn(), but\ndoing it manually lets us produce a more specific message, which is\nnice.\n\n-Peff\n"}]}