@matheusbsbs / matheus-bagliano / commits / fd2739b159

pack-bitmap: handle duplicate pack entries during MIDX reuse

A MIDX pseudo-pack assigns one bit position to each selected OID. Its preferred-pack fast paths handle duplicate OIDs across packs in the MIDX, where the MIDX chooses one copy, but assume that each individual pack contains one entry per OID. Consider a preferred pack ordered [A, B, A, C]. The MIDX retains one copy of A, leaving three pseudo-pack positions for four physical entries. This creates two problems: - In MIDX-backed single-pack reuse, bitmap_nr came from pack->num_objects and therefore counted both copies of A. Read the selected count from the root MIDX layer BTMP chunk instead. If that layer has no BTMP chunk, skip pack reuse and let the ordinary packing path handle the bitmap-selected objects. MIDXs without BTMP lose preferred-pack reuse until rewritten, rather than requiring a scan of the entire pseudo-pack to retain an optional optimization. - Correcting the range length still does not align the two orderings. Once one copy of A is omitted, MIDX position N need not name physical pack entry N. That mismatch matters during both selection and output. Whole-word selection bypasses the per-object check that the exact physical base of a delta is present. Verbatim reuse copies the first N pack entries for the first N bits. The per-object writer likewise used each bit as a physical pack position. Use the direct mapping only when the range begins at zero and its selected count equals the physical entry count. Since selected entries remain in pack-offset order, equal counts mean that none was omitted and the two positions coincide. For every other MIDX range, let whole-word selection fall through to the existing per-bit path, which resolves the selected MIDX offset to a physical pack position before checking the delta base. Disable verbatim prefix reuse, and perform the same translation in the per-object writer. Leave classic single-pack bitmap handling unchanged. The production writer creates those bitmaps only for the pack it just wrote, which has one entry per OID. The test helper can target an existing pack, but rejects duplicate OIDs. Cover the A, B, A, C case in a MIDX-backed preferred pack. Check reuse with BTMP, then hide BTMP and check that the optional reuse path is skipped. Also make B a delta against the omitted copy of A and require normal packing to handle it. Signed-off-by: Taylor Blau <ttaylorr@openai.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Taylor Blau committed Jul 24, 2026 at 16:06 UTC fd2739b159d075cd4c6fa69b2cd876ba1caa5c88
3 files changed +125 -15
builtin/pack-objects.c
+11 -13
@@ -1182,12 +1182,12 @@ static size_t write_reused_pack_verbatim(struct bitmapped_pack *reuse_packfile,
1182 size_t pos = 0;
1183 size_t end;
1184
1185 - if (reuse_packfile->bitmap_pos) {
1185 + if (reuse_packfile->bitmap_pos ||
1186 + reuse_packfile->bitmap_nr != reuse_packfile->p->num_objects) {
1187 /*
1187 - * We can't reuse whole chunks verbatim out of
1188 - * non-preferred packs since we can't guarantee that
1189 - * all duplicate objects were resolved in favor of
1190 - * that pack.
1188 + * We can't reuse whole chunks verbatim from non-preferred
1189 + * packs or packs with entries missing from the bitmap because
1190 + * bitmap and pack positions may differ.
1191 *
1192 * Even if we have a whole eword_t worth of bits that
1193 * could be reused, there may be objects between the
@@ -1196,7 +1196,7 @@ static size_t write_reused_pack_verbatim(struct bitmapped_pack *reuse_packfile,
1196 * pack, causing us to send duplicate or unwanted
1197 * objects.
1198 *
1199 - * Handle non-preferred packs from within
1199 + * Handle these packs from within
1200 * write_reused_pack(), which inspects and reuses
1201 * individual bits.
1202 */
@@ -1263,20 +1263,18 @@ static void write_reused_pack(struct bitmapped_pack *reuse_packfile,
1263 if (pos + offset >= reuse_packfile->bitmap_pos + reuse_packfile->bitmap_nr)
1264 goto done;
1265
1266 - if (reuse_packfile->bitmap_pos) {
1266 + if (reuse_packfile->bitmap_pos ||
1267 + reuse_packfile->bitmap_nr != reuse_packfile->p->num_objects) {
1268 /*
1268 - * When doing multi-pack reuse on a
1269 - * non-preferred pack, translate bit positions
1270 - * from the MIDX pseudo-pack order back to their
1271 - * pack-relative positions before attempting
1272 - * reuse.
1269 + * Translate MIDX bitmap positions which do not
1270 + * correspond directly to physical pack positions.
1271 */
1272 struct multi_pack_index *m = reuse_packfile->from_midx;
1273 uint32_t midx_pos;
1274 off_t pack_ofs;
1275
1276 if (!m)
1279 - BUG("non-zero bitmap position without MIDX");
1277 + BUG("cannot translate bitmap position without MIDX");
1278
1279 midx_pos = pack_pos_to_midx(m, pos + offset);
1280 pack_ofs = nth_midxed_offset(m, midx_pos);
pack-bitmap.c
+25 -2
@@ -2383,7 +2383,8 @@ static void reuse_partial_packfile_from_bitmap_1(struct bitmap_index *bitmap_git
2383 struct pack_window *w_curs = NULL;
2384 size_t pos = pack->bitmap_pos / BITS_IN_EWORD;
2385
2386 - if (!pack->bitmap_pos) {
2386 + if (!pack->bitmap_pos &&
2387 + pack->bitmap_nr == pack->p->num_objects) {
2388 /*
2389 * If we're processing the first (in the case of a MIDX, the
2390 * preferred pack) or the only (in the case of single-pack
@@ -2399,6 +2400,9 @@ static void reuse_partial_packfile_from_bitmap_1(struct bitmap_index *bitmap_git
2400 * all ties are broken in favor of that pack (i.e. the one
2401 * we're currently processing). So any duplicate bases will be
2402 * resolved in favor of the pack we're processing.
2403 + *
2404 + * The range must also contain every physical pack entry so that
2405 + * bitmap and pack positions correspond.
2406 */
2407 while (pos < result->word_alloc &&
2408 pos < pack->bitmap_nr / BITS_IN_EWORD &&
@@ -2527,14 +2531,26 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
2531 } else {
2532 struct packed_git *pack;
2533 uint32_t pack_int_id;
2534 + uint32_t bitmap_nr;
2535
2536 if (bitmap_is_midx(bitmap_git)) {
2537 + struct bitmapped_pack bitmapped_pack;
2538 struct multi_pack_index *m = bitmap_git->midx;
2539 uint32_t preferred_pack_pos;
2540
2541 while (m->base_midx)
2542 m = m->base_midx;
2543
2544 + if (!m->chunk_bitmapped_packs) {
2545 + /*
2546 + * Without BTMP, determining the preferred
2547 + * pack's range requires scanning the pseudo-pack.
2548 + * Skip reuse and leave the bitmap result for
2549 + * normal packing.
2550 + */
2551 + return;
2552 + }
2553 +
2554 if (midx_preferred_pack(m, &preferred_pack_pos) < 0) {
2555 warning(_("unable to compute preferred pack, disabling pack-reuse"));
2556 return;
@@ -2542,6 +2558,12 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
2558
2559 pack = nth_midxed_pack(m, preferred_pack_pos);
2560 pack_int_id = preferred_pack_pos;
2561 +
2562 + if (nth_bitmapped_pack(m, &bitmapped_pack,
2563 + pack_int_id) < 0)
2564 + return;
2565 + pack = bitmapped_pack.p;
2566 + bitmap_nr = bitmapped_pack.bitmap_nr;
2567 } else {
2568 pack = bitmap_git->pack;
2569 /*
@@ -2554,13 +2576,14 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
2576 * that we do not expect to read this field.
2577 */
2578 pack_int_id = -1;
2579 + bitmap_nr = pack->num_objects;
2580 }
2581
2582 if (is_pack_valid(pack)) {
2583 ALLOC_GROW(packs, packs_nr + 1, packs_alloc);
2584 packs[packs_nr].p = pack;
2585 packs[packs_nr].pack_int_id = pack_int_id;
2563 - packs[packs_nr].bitmap_nr = pack->num_objects;
2586 + packs[packs_nr].bitmap_nr = bitmap_nr;
2587 packs[packs_nr].bitmap_pos = 0;
2588 packs[packs_nr].from_midx = bitmap_git->midx;
2589 packs_nr++;
t/t5332-multi-pack-reuse.sh
+89
@@ -4,6 +4,7 @@ test_description='pack-objects multi-pack reuse'
4
5 . ./test-lib.sh
6 . "$TEST_DIRECTORY"/lib-bitmap.sh
7 +. "$TEST_DIRECTORY"/lib-pack.sh
8
9 GIT_TEST_MULTI_PACK_INDEX=0
10 GIT_TEST_MULTI_PACK_INDEX_WRITE_INCREMENTAL=0
@@ -32,6 +33,14 @@ pack_position () {
33 grep "$1" objects | cut -d" " -f1
34 }
35
36 +# B as an OFS_DELTA against A at the given one-byte distance.
37 +pack_obj_b_ofs_a () {
38 + pack_obj "$B" "$A" >b-ref.tmp &&
39 + printf "\145" &&
40 + printf "\\$(printf "%03o" "$1")" &&
41 + dd if=b-ref.tmp bs=1 skip=$((1 + $(test_oid rawsz))) 2>/dev/null
42 +}
43 +
44 # test_pack_objects_reused_all <pack-reused> <packs-reused>
45 test_pack_objects_reused_all () {
46 : >trace2.txt &&
@@ -288,4 +297,84 @@ test_expect_success 'duplicate objects with verbatim reuse' '
297 )
298 '
299
300 +test_expect_success 'reuse with intra-pack duplicate objects' '
301 + git init intra-pack-duplicate-objects &&
302 + (
303 + cd intra-pack-duplicate-objects &&
304 +
305 + # Make enough objects to exercise whole-word reuse.
306 + test_commit_bulk 20 &&
307 + test_commit --printf A a "\7\0" &&
308 + test_commit --printf B b "\7\76" &&
309 +
310 + objects_nr=$(git rev-list --count --objects --all) &&
311 + git rev-list --objects --all |
312 + cut -d" " -f1 >objects &&
313 + A=$(test_oid packlib_7_0) &&
314 + B=$(test_oid packlib_7_76) &&
315 + grep -v -e "^$A$" -e "^$B$" objects >rest &&
316 + pack_obj "$A" >a-full &&
317 + pack_obj "$B" >b-full &&
318 + while read oid
319 + do
320 + pack_obj "$oid" || exit 1
321 + done <rest >rest.entries &&
322 + {
323 + # Arrange the pack as A, B, A, C..., so that physical
324 + # positions diverge from MIDX pseudo-pack order.
325 + pack_header $((objects_nr + 1)) &&
326 + cat a-full b-full a-full rest.entries
327 + } >duplicate.pack &&
328 + pack_trailer duplicate.pack &&
329 + clear_packs &&
330 + git index-pack --stdin <duplicate.pack &&
331 +
332 + git multi-pack-index write --bitmap &&
333 + git config pack.allowPackReuse single &&
334 + test_pack_objects_reused_all "$objects_nr" 1 &&
335 + rm -f got.idx &&
336 + test_env GIT_TEST_MIDX_READ_BTMP=false \
337 + test_pack_objects_reused_all 0 0
338 + )
339 +'
340 +
341 +test_expect_success 'omit delta whose duplicate base is not selected' '
342 + (
343 + cd intra-pack-duplicate-objects &&
344 +
345 + A=$(test_oid packlib_7_0) &&
346 + B=$(test_oid packlib_7_76) &&
347 + objects_nr=$(wc -l <objects) &&
348 +
349 + a_size=$(wc -c <a-full) &&
350 + test "$((2 * a_size))" -lt 128 &&
351 +
352 + # The .idx order of duplicate OIDs is unspecified. Try a delta
353 + # against each copy and keep the pack whose base was omitted.
354 + for distance in "$((2 * a_size))" "$a_size"
355 + do
356 + base_offset=$((12 + 2 * a_size - distance)) &&
357 + pack_obj_b_ofs_a "$distance" >b-delta &&
358 + {
359 + pack_header "$((objects_nr + 1))" &&
360 + cat a-full a-full b-delta rest.entries
361 + } >candidate.pack &&
362 + pack_trailer candidate.pack &&
363 + clear_packs &&
364 + git index-pack --stdin <candidate.pack &&
365 + git multi-pack-index write --bitmap || return 1
366 +
367 + selected_offset=$(
368 + test-tool read-midx --show-objects "$objdir" |
369 + awk -v oid="$A" "\$1 == oid { print \$2 }"
370 + ) || return 1
371 + test -n "$selected_offset" || return 1
372 + test "$selected_offset" = "$base_offset" || break
373 + done &&
374 + test "$selected_offset" != "$base_offset" &&
375 +
376 + test_pack_objects_reused_all "$((objects_nr - 1))" 1
377 + )
378 +'
379 +
380 test_done