pack-bitmap.c: gracefully degrade on failure to load MIDX'd pack

When opening a MIDX bitmap, we the pack-bitmap machinery eagerly calls
`prepare_midx_pack()` on each of the packs contained in the MIDX. This
is done in order to populate the array of `struct packed_git *`s held by
the MIDX, which we need later on in `load_reverse_index()`, since it
calls `load_pack_revindex()` on each of the MIDX'd packs, and requires
that the caller provide a pointer to a `struct packed_git`.

When opening one of these packs fails, the pack-bitmap code will `die()`
indicating that it can't open one of the packs in the MIDX. This
indicates that the MIDX is somehow broken with respect to the current
state of the repository. When this is the case, we indeed cannot make
use of the MIDX bitmap to speed up reachability traversals.

However, it does not mean that we can't perform reachability traversals
at all. In other failure modes, that same function calls `warning()` and
then returns -1, indicating to its caller (`open_bitmap()`) that we
should either look for a pack bitmap if one is available, or perform
normal object traversal without using bitmaps at all.

There is no reason why this case should cause us to die. If we instead
continued (by jumping to `cleanup` as this patch does) and avoid using
bitmaps altogether, we may again try and query the MIDX, which will also
fail. But when trying to call `fill_midx_entry()` fails, it also returns
a signal of its failure, and prompts the caller to try and locate the
object elsewhere.

In other words, the normal object traversal machinery works fine in the
presence of a corrupt MIDX, so there is no reason that the MIDX bitmap
machinery should abort in that case when we could easily continue.

Note that we *could* in theory try again to load a MIDX bitmap after
calling `reprepare_packed_git()`. Even though the `prepare_packed_git()`
code is careful to avoid adding a pack that we already have,
`prepare_midx_pack()` is not. So if we got part of the way through
calling `prepare_midx_pack()` on a stale MIDX, and then tried again on a
fresh MIDX that contains some of the same packs, we would end up with a
loop through the `->next` pointer.

For now, let's do the simplest thing possible and fallback to the
non-bitmap code when we detect a stale MIDX so that the complete fix as
above can be implemented carefully.

Signed-off-by: Taylor Blau <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>

Author: ttaylorr

pack-bitmap.c

@@ -387,9 +387,11 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 	}
 
 	for (i = 0; i < bitmap_git->midx->num_packs; i++) {
-		if (prepare_midx_pack(the_repository, bitmap_git->midx, i))
-			die(_("could not open pack %s"),
-			    bitmap_git->midx->pack_names[i]);
+		if (prepare_midx_pack(the_repository, bitmap_git->midx, i)) {
+			warning(_("could not open pack %s"),
+				bitmap_git->midx->pack_names[i]);
+			goto cleanup;
+		}
 	}
 
 	preferred = bitmap_git->midx->packs[midx_preferred_pack(bitmap_git)];

t/t5326-multi-pack-bitmaps.sh

@@ -478,4 +478,39 @@ test_expect_success 'git fsck correctly identifies good and bad bitmaps' '
 	grep "bitmap file '\''$packbitmap'\'' has invalid checksum" err
 '
 
+test_expect_success 'corrupt MIDX with bitmap causes fallback' '
+	git init corrupt-midx-bitmap &&
+	(
+		cd corrupt-midx-bitmap &&
+
+		test_commit first &&
+		git repack -d &&
+		test_commit second &&
+		git repack -d &&
+
+		git multi-pack-index write --bitmap &&
+		checksum=$(midx_checksum $objdir) &&
+		for f in $midx $midx-$checksum.bitmap
+		do
+			mv $f $f.bak || return 1
+		done &&
+
+		# pack everything together, invalidating the MIDX
+		git repack -ad &&
+		# then restore the now-stale MIDX
+		for f in $midx $midx-$checksum.bitmap
+		do
+			mv $f.bak $f || return 1
+		done &&
+
+		git rev-list --count --objects --use-bitmap-index HEAD >out 2>err &&
+		# should attempt opening the broken pack twice (once
+		# from the attempt to load it via the stale bitmap, and
+		# again when attempting to load it from the stale MIDX)
+		# before falling back to the non-MIDX case
+		test 2 -eq $(grep -c "could not open pack" err) &&
+		test 6 -eq $(cat out)
+	)
+'
+
 test_done