{"thread":{"id":"59844","subject":"[PATCH] pack-bitmap.c: gracefully degrade on failure to load MIDX'd pack","startedAt":"2023-06-07T23:01:09Z","lastAt":"2023-06-12T21:23:33Z","messageCount":2,"participants":["Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"478159","messageId":"f123b68cb8a277fbf105b1789a84b9405a499b79.1686178854.git.me@ttaylorr.com","threadId":"59844","inReplyTo":null,"subject":"[PATCH] pack-bitmap.c: gracefully degrade on failure to load MIDX'd pack","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-06-07T23:01:00Z","receivedAt":"2023-06-07T23:01:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When opening a MIDX bitmap, we the pack-bitmap machinery eagerly calls\n`prepare_midx_pack()` on each of the packs contained in the MIDX. This\nis done in order to populate the array of `struct packed_git *`s held by\nthe MIDX, which we need later on in `load_reverse_index()`, since it\ncalls `load_pack_revindex()` on each of the MIDX'd packs, and requires\nthat the caller provide a pointer to a `struct packed_git`.\n\nWhen opening one of these packs fails, the pack-bitmap code will `die()`\nindicating that it can't open one of the packs in the MIDX. This\nindicates that the MIDX is somehow broken with respect to the current\nstate of the repository. When this is the case, we indeed cannot make\nuse of the MIDX bitmap to speed up reachability traversals.\n\nHowever, it does not mean that we can't perform reachability traversals\nat all. In other failure modes, that same function calls `warning()` and\nthen returns -1, indicating to its caller (`open_bitmap()`) that we\nshould either look for a pack bitmap if one is available, or perform\nnormal object traversal without using bitmaps at all.\n\nThere is no reason why this case should cause us to die. If we instead\ncontinued (by jumping to `cleanup` as this patch does) and avoid using\nbitmaps altogether, we may again try and query the MIDX, which will also\nfail. But when trying to call `fill_midx_entry()` fails, it also returns\na signal of its failure, and prompts the caller to try and locate the\nobject elsewhere.\n\nIn other words, the normal object traversal machinery works fine in the\npresence of a corrupt MIDX, so there is no reason that the MIDX bitmap\nmachinery should abort in that case when we could easily continue.\n\nNote that we *could* in theory try again to load a MIDX bitmap after\ncalling `reprepare_packed_git()`. Even though the `prepare_packed_git()`\ncode is careful to avoid adding a pack that we already have,\n`prepare_midx_pack()` is not. So if we got part of the way through\ncalling `prepare_midx_pack()` on a stale MIDX, and then tried again on a\nfresh MIDX that contains some of the same packs, we would end up with a\nloop through the `->next` pointer.\n\nFor now, let's do the simplest thing possible and fallback to the\nnon-bitmap code when we detect a stale MIDX so that the complete fix as\nabove can be implemented carefully.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n pack-bitmap.c                 |  8 +++++---\n t/t5326-multi-pack-bitmaps.sh | 35 +++++++++++++++++++++++++++++++++++\n 2 files changed, 40 insertions(+), 3 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 999f962602..1c3fd056a8 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -387,9 +387,11 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n \t}\n \n \tfor (i = 0; i < bitmap_git->midx->num_packs; i++) {\n-\t\tif (prepare_midx_pack(the_repository, bitmap_git->midx, i))\n-\t\t\tdie(_(\"could not open pack %s\"),\n-\t\t\t    bitmap_git->midx->pack_names[i]);\n+\t\tif (prepare_midx_pack(the_repository, bitmap_git->midx, i)) {\n+\t\t\twarning(_(\"could not open pack %s\"),\n+\t\t\t\tbitmap_git->midx->pack_names[i]);\n+\t\t\tgoto cleanup;\n+\t\t}\n \t}\n \n \tpreferred = bitmap_git->midx->packs[midx_preferred_pack(bitmap_git)];\ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex f771c442d4..70d1b58709 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -478,4 +478,39 @@ test_expect_success 'git fsck correctly identifies good and bad bitmaps' '\n \tgrep \"bitmap file '\\''$packbitmap'\\'' has invalid checksum\" err\n '\n \n+test_expect_success 'corrupt MIDX with bitmap causes fallback' '\n+\tgit init corrupt-midx-bitmap &&\n+\t(\n+\t\tcd corrupt-midx-bitmap &&\n+\n+\t\ttest_commit first &&\n+\t\tgit repack -d &&\n+\t\ttest_commit second &&\n+\t\tgit repack -d &&\n+\n+\t\tgit multi-pack-index write --bitmap &&\n+\t\tchecksum=$(midx_checksum $objdir) &&\n+\t\tfor f in $midx $midx-$checksum.bitmap\n+\t\tdo\n+\t\t\tmv $f $f.bak || return 1\n+\t\tdone &&\n+\n+\t\t# pack everything together, invalidating the MIDX\n+\t\tgit repack -ad &&\n+\t\t# then restore the now-stale MIDX\n+\t\tfor f in $midx $midx-$checksum.bitmap\n+\t\tdo\n+\t\t\tmv $f.bak $f || return 1\n+\t\tdone &&\n+\n+\t\tgit rev-list --count --objects --use-bitmap-index HEAD >out 2>err &&\n+\t\t# should attempt opening the broken pack twice (once\n+\t\t# from the attempt to load it via the stale bitmap, and\n+\t\t# again when attempting to load it from the stale MIDX)\n+\t\t# before falling back to the non-MIDX case\n+\t\ttest 2 -eq $(grep -c \"could not open pack\" err) &&\n+\t\ttest 6 -eq $(cat out)\n+\t)\n+'\n+\n test_done\n-- \n2.41.0.1.gf123b68cb8\n"},{"id":"478305","messageId":"xmqqpm608jlm.fsf@gitster.g","threadId":"59844","inReplyTo":"f123b68cb8a277fbf105b1789a84b9405a499b79.1686178854.git.me@ttaylorr.com","subject":"Re: [PATCH] pack-bitmap.c: gracefully degrade on failure to load MIDX'd pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-12T21:19:01Z","receivedAt":"2023-06-12T21:23:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> When opening a MIDX bitmap, we the pack-bitmap machinery eagerly calls\n\nI guess s/we // is needed here?\n\n> When opening one of these packs fails, the pack-bitmap code will `die()`\n> indicating that it can't open one of the packs in the MIDX. This\n> indicates that the MIDX is somehow broken with respect to the current\n> state of the repository. When this is the case, we indeed cannot make\n> use of the MIDX bitmap to speed up reachability traversals.\n>\n> However, it does not mean that we can't perform reachability traversals\n> at all. In other failure modes, that same function calls `warning()` and\n> then returns -1, indicating to its caller (`open_bitmap()`) that we\n> should either look for a pack bitmap if one is available, or perform\n> normal object traversal without using bitmaps at all.\n\nMakes sense.\n"}]}