[PATCH v2] builtin/repack.c: remove redundant pack-based bitmaps
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Oct 18, 2022, 02:45 UTC
- Message-ID
- <1e0ef7ee7ff5feb323c77e594cd65433fb1d99f7.1666061096.git.me@ttaylorr.com>
- In-Reply-To
- <393fd4c6db78cd694e6d4dfcf24f17e2850ccd99.1665601403.git.me@ttaylorr.com>
When we write a MIDX bitmap after repacking, it is possible that the repository would be left in a state with both pack- and multi-pack reachability bitmaps.
This can occur, for instance, if a pack that was kept (either by having a .keep file, or during a geometric repack in which it is not rolled up) has a bitmap file, and the repack wrote a multi-pack index and bitmap.
When loading a reachability bitmap for the repository, the multi-pack one is always preferred, so the pack-based one is redundant. Let's remove it unconditionally, even if '-d' isn't passed, since there is no practical reason to keep both around. The patch below does just that.
Signed-off-by: Taylor Blau <me@ttaylorr.com> --- A small reroll to address a pair of comments from Peff.
Range-diff against v1:
1: 393fd4c6db ! 1: 1e0ef7ee7f builtin/repack.c: remove redundant pack-based bitmaps
@@ builtin/repack.c: static int write_midx_included_packs(struct string_list *inclu
+ strbuf_addstr(&path, ".bitmap");
+
+ if (unlink(path.buf) && errno != ENOENT)
-+ die_errno(_("could not remove stale bitmap: %s"),
-+ path.buf);
++ warning_errno(_("could not remove stale bitmap: %s"),
++ path.buf);
+
+ strbuf_setlen(&path, packdir_len);
+ }
@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
refs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,
show_progress, write_bitmaps > 0); -+ if (ret) {
-+ string_list_clear(&include, 0);
-+ return ret;
-+ }
-+
-+ if (write_bitmaps)
++ if (!ret && write_bitmaps)
+ remove_redundant_bitmaps(&include, packdir);
+
string_list_clear(&include, 0);
--
-- if (ret)
-- return ret;
- } - reprepare_packed_git(the_repository);
+ if (ret) ## t/t7700-repack.sh ##
@@ t/t7700-repack.sh: test_expect_success '--write-midx -b packs non-kept objects' 'builtin/repack.c | 32 ++++++++++++++++++++++++++++++++ t/t7700-repack.sh | 21 +++++++++++++++++++++ 2 files changed, 53 insertions(+)
diff --git a/builtin/repack.c b/builtin/repack.c index a5bacc7797..c2d2e52bd4 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -661,6 +661,35 @@ static int write_midx_included_packs(struct string_list *include, return finish_command(&cmd); } +static void remove_redundant_bitmaps(struct string_list *include, + const char *packdir) +{ + struct strbuf path = STRBUF_INIT; + struct string_list_item *item; + size_t packdir_len; + + strbuf_addstr(&path, packdir); + strbuf_addch(&path, '/'); + packdir_len = path.len; + + /* + * Remove any pack bitmaps corresponding to packs which are now + * included in the MIDX. + */ + for_each_string_list_item(item, include) { + strbuf_addstr(&path, item->string); + strbuf_strip_suffix(&path, ".idx"); + strbuf_addstr(&path, ".bitmap"); + + if (unlink(path.buf) && errno != ENOENT) + warning_errno(_("could not remove stale bitmap: %s"), + path.buf); + + strbuf_setlen(&path, packdir_len); + } + strbuf_release(&path); +} + static int write_cruft_pack(const struct pack_objects_args *args, const char *pack_prefix, struct string_list *names, @@ -1059,6 +1088,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix) refs_snapshot ? get_tempfile_path(refs_snapshot) : NULL, show_progress, write_bitmaps > 0); + if (!ret && write_bitmaps) + remove_redundant_bitmaps(&include, packdir); + string_list_clear(&include, 0); if (ret) diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh index ca45c4cd2c..2d0e9448dd 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh @@ -426,6 +426,27 @@ test_expect_success '--write-midx -b packs non-kept objects' ' ) ' +test_expect_success '--write-midx removes stale pack-based bitmaps' ' + rm -fr repo && + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + test_commit base && + GIT_TEST_MULTI_PACK_INDEX=0 git repack -Ab && + + pack_bitmap=$(ls $objdir/pack/pack-*.bitmap) && + test_path_is_file "$pack_bitmap" && + + test_commit tip && + GIT_TEST_MULTI_PACK_INDEX=0 git repack -bm && + + test_path_is_file $midx && + test_path_is_file $midx-$(midx_checksum $objdir).bitmap && + test_path_is_missing $pack_bitmap + ) +' + test_expect_success TTY '--quiet disables progress' ' test_terminal env GIT_PROGRESS_DELAY=0 \ git -C midx repack -ad --quiet --write-midx 2>stderr && -- 2.38.0.16.g393fd4c6db