@matheusbsbs / matheus-bagliano / commits / 795006fff4

pack-bitmap: gracefully handle missing BTMP chunks

In 0fea6b73f1 (Merge branch 'tb/multi-pack-verbatim-reuse', 2024-01-12) we have introduced multi-pack verbatim reuse of objects. This series has introduced a new BTMP chunk, which encodes information about bitmapped objects in the multi-pack index. Starting with dab60934e3 (pack-bitmap: pass `bitmapped_pack` struct to pack-reuse functions, 2023-12-14) we use this information to figure out objects which we can reuse from each of the packfiles. One thing that we glossed over though is backwards compatibility with repositories that do not yet have BTMP chunks in their multi-pack index. In that case, `nth_bitmapped_pack()` would return an error, which causes us to emit a warning followed by another error message. These warnings are visible to users that fetch from a repository: ``` $ git fetch ... remote: error: MIDX does not contain the BTMP chunk remote: warning: unable to load pack: 'pack-f6bb7bd71d345ea9fe604b60cab9ba9ece54ffbe.idx', disabling pack-reuse remote: Enumerating objects: 40, done. remote: Counting objects: 100% (40/40), done. remote: Compressing objects: 100% (39/39), done. remote: Total 40 (delta 5), reused 0 (delta 0), pack-reused 0 (from 0) ... ``` While the fetch succeeds the user is left wondering what they did wrong. Furthermore, as visible both from the warning and from the reuse stats, pack-reuse is completely disabled in such repositories. What is quite interesting is that this issue can even be triggered in case `pack.allowPackReuse=single` is set, which is the default value. One could have expected that in this case we fall back to the old logic, which is to use the preferred packfile without consulting BTMP chunks at all. But either we fail with the above error in case they are missing, or we use the first pack in the multi-pack-index. The former case disables pack-reuse altogether, whereas the latter case may result in reusing objects from a suboptimal packfile. Fix this issue by partially reverting the logic back to what we had before this patch series landed. Namely, in the case where we have no BTMP chunks or when `pack.allowPackReuse=single` are set, we use the preferred pack instead of consulting the BTMP chunks. Helped-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed Apr 15, 2024 at 08:41 UTC 795006fff45fb2b5e92be0121f92a876c409a1a3
3 files changed +42 -23
midx.c
+4 -3
@@ -198,9 +198,10 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
198
199 pair_chunk(cf, MIDX_CHUNKID_LARGEOFFSETS, &m->chunk_large_offsets,
200 &m->chunk_large_offsets_len);
201 - pair_chunk(cf, MIDX_CHUNKID_BITMAPPEDPACKS,
202 - (const unsigned char **)&m->chunk_bitmapped_packs,
203 - &m->chunk_bitmapped_packs_len);
201 + if (git_env_bool("GIT_TEST_MIDX_READ_BTMP", 1))
202 + pair_chunk(cf, MIDX_CHUNKID_BITMAPPEDPACKS,
203 + (const unsigned char **)&m->chunk_bitmapped_packs,
204 + &m->chunk_bitmapped_packs_len);
205
206 if (git_env_bool("GIT_TEST_MIDX_READ_RIDX", 1))
207 pair_chunk(cf, MIDX_CHUNKID_REVINDEX, &m->chunk_revindex,
pack-bitmap.c
+21 -20
@@ -2056,7 +2056,10 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
2056
2057 load_reverse_index(r, bitmap_git);
2058
2059 - if (bitmap_is_midx(bitmap_git)) {
2059 + if (!bitmap_is_midx(bitmap_git) || !bitmap_git->midx->chunk_bitmapped_packs)
2060 + multi_pack_reuse = 0;
2061 +
2062 + if (multi_pack_reuse) {
2063 for (i = 0; i < bitmap_git->midx->num_packs; i++) {
2064 struct bitmapped_pack pack;
2065 if (nth_bitmapped_pack(r, bitmap_git->midx, &pack, i) < 0) {
@@ -2069,34 +2072,32 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
2072 if (!pack.bitmap_nr)
2073 continue;
2074
2072 - if (!multi_pack_reuse && pack.bitmap_pos) {
2073 - /*
2074 - * If we're only reusing a single pack, skip
2075 - * over any packs which are not positioned at
2076 - * the beginning of the MIDX bitmap.
2077 - *
2078 - * This is consistent with the existing
2079 - * single-pack reuse behavior, which only reuses
2080 - * parts of the MIDX's preferred pack.
2081 - */
2082 - continue;
2083 - }
2084 -
2075 ALLOC_GROW(packs, packs_nr + 1, packs_alloc);
2076 memcpy(&packs[packs_nr++], &pack, sizeof(pack));
2077
2078 objects_nr += pack.p->num_objects;
2089 -
2090 - if (!multi_pack_reuse)
2091 - break;
2079 }
2080
2081 QSORT(packs, packs_nr, bitmapped_pack_cmp);
2082 } else {
2096 - ALLOC_GROW(packs, packs_nr + 1, packs_alloc);
2083 + struct packed_git *pack;
2084 +
2085 + if (bitmap_is_midx(bitmap_git)) {
2086 + uint32_t preferred_pack_pos;
2087 +
2088 + if (midx_preferred_pack(bitmap_git->midx, &preferred_pack_pos) < 0) {
2089 + warning(_("unable to compute preferred pack, disabling pack-reuse"));
2090 + return;
2091 + }
2092
2098 - packs[packs_nr].p = bitmap_git->pack;
2099 - packs[packs_nr].bitmap_nr = bitmap_git->pack->num_objects;
2093 + pack = bitmap_git->midx->packs[preferred_pack_pos];
2094 + } else {
2095 + pack = bitmap_git->pack;
2096 + }
2097 +
2098 + ALLOC_GROW(packs, packs_nr + 1, packs_alloc);
2099 + packs[packs_nr].p = pack;
2100 + packs[packs_nr].bitmap_nr = pack->num_objects;
2101 packs[packs_nr].bitmap_pos = 0;
2102
2103 objects_nr = packs[packs_nr++].bitmap_nr;
t/t5326-multi-pack-bitmaps.sh
+17
@@ -513,4 +513,21 @@ test_expect_success 'corrupt MIDX with bitmap causes fallback' '
513 )
514 '
515
516 +for allow_pack_reuse in single multi
517 +do
518 + test_expect_success "reading MIDX without BTMP chunk does not complain with $allow_pack_reuse pack reuse" '
519 + test_when_finished "rm -rf midx-without-btmp" &&
520 + git init midx-without-btmp &&
521 + (
522 + cd midx-without-btmp &&
523 + test_commit initial &&
524 +
525 + git repack -Adbl --write-bitmap-index --write-midx &&
526 + GIT_TEST_MIDX_READ_BTMP=false git -c pack.allowPackReuse=$allow_pack_reuse \
527 + pack-objects --all --use-bitmap-index --stdout </dev/null >/dev/null 2>err &&
528 + test_must_be_empty err
529 + )
530 + '
531 +done
532 +
533 test_done