[PATCH v2] builtin/repack.c: remove redundant pack-based bitmaps

[Taylor Blau <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