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>

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 3673b14b89b275e8292f01826e72c2d4ce383fe3..c33392296107e1c2b4304aafef9dbcaf44803f95 100644 (file)
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1182,12 +1182,12 @@ static size_t write_reused_pack_verbatim(struct bitmapped_pack *reuse_packfile,
        size_t pos = 0;
        size_t end;
 
-       if (reuse_packfile->bitmap_pos) {
+       if (reuse_packfile->bitmap_pos ||
+           reuse_packfile->bitmap_nr != reuse_packfile->p->num_objects) {
                /*
-                * We can't reuse whole chunks verbatim out of
-                * non-preferred packs since we can't guarantee that
-                * all duplicate objects were resolved in favor of
-                * that pack.
+                * We can't reuse whole chunks verbatim from non-preferred
+                * packs or packs with entries missing from the bitmap because
+                * bitmap and pack positions may differ.
                 *
                 * Even if we have a whole eword_t worth of bits that
                 * 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,
                 * pack, causing us to send duplicate or unwanted
                 * objects.
                 *
-                * Handle non-preferred packs from within
+                * Handle these packs from within
                 * write_reused_pack(), which inspects and reuses
                 * individual bits.
                 */
@@ -1263,20 +1263,18 @@ static void write_reused_pack(struct bitmapped_pack *reuse_packfile,
                        if (pos + offset >= reuse_packfile->bitmap_pos + reuse_packfile->bitmap_nr)
                                goto done;
 
-                       if (reuse_packfile->bitmap_pos) {
+                       if (reuse_packfile->bitmap_pos ||
+                           reuse_packfile->bitmap_nr != reuse_packfile->p->num_objects) {
                                /*
-                                * When doing multi-pack reuse on a
-                                * non-preferred pack, translate bit positions
-                                * from the MIDX pseudo-pack order back to their
-                                * pack-relative positions before attempting
-                                * reuse.
+                                * Translate MIDX bitmap positions which do not
+                                * correspond directly to physical pack positions.
                                 */
                                struct multi_pack_index *m = reuse_packfile->from_midx;
                                uint32_t midx_pos;
                                off_t pack_ofs;
 
                                if (!m)
-                                       BUG("non-zero bitmap position without MIDX");
+                                       BUG("cannot translate bitmap position without MIDX");
 
                                midx_pos = pack_pos_to_midx(m, pos + offset);
                                pack_ofs = nth_midxed_offset(m, midx_pos);

diff --git a/pack-bitmap.c b/pack-bitmap.c
index d8dc4ae8d1633c43dbcfeeafd89ae236106c7352..81412904365f40318fe0773009ed23d479e6a119 100644 (file)
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -2383,7 +2383,8 @@ static void reuse_partial_packfile_from_bitmap_1(struct bitmap_index *bitmap_git
        struct pack_window *w_curs = NULL;
        size_t pos = pack->bitmap_pos / BITS_IN_EWORD;
 
-       if (!pack->bitmap_pos) {
+       if (!pack->bitmap_pos &&
+           pack->bitmap_nr == pack->p->num_objects) {
                /*
                 * If we're processing the first (in the case of a MIDX, the
                 * 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
                 *   all ties are broken in favor of that pack (i.e. the one
                 *   we're currently processing). So any duplicate bases will be
                 *   resolved in favor of the pack we're processing.
+                *
+                * The range must also contain every physical pack entry so that
+                * bitmap and pack positions correspond.
                 */
                while (pos < result->word_alloc &&
                       pos < pack->bitmap_nr / BITS_IN_EWORD &&
@@ -2527,14 +2531,26 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
        } else {
                struct packed_git *pack;
                uint32_t pack_int_id;
+               uint32_t bitmap_nr;
 
                if (bitmap_is_midx(bitmap_git)) {
+                       struct bitmapped_pack bitmapped_pack;
                        struct multi_pack_index *m = bitmap_git->midx;
                        uint32_t preferred_pack_pos;
 
                        while (m->base_midx)
                                m = m->base_midx;
 
+                       if (!m->chunk_bitmapped_packs) {
+                               /*
+                                * Without BTMP, determining the preferred
+                                * pack's range requires scanning the pseudo-pack.
+                                * Skip reuse and leave the bitmap result for
+                                * normal packing.
+                                */
+                               return;
+                       }
+
                        if (midx_preferred_pack(m, &preferred_pack_pos) < 0) {
                                warning(_("unable to compute preferred pack, disabling pack-reuse"));
                                return;
@@ -2542,6 +2558,12 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
 
                        pack = nth_midxed_pack(m, preferred_pack_pos);
                        pack_int_id = preferred_pack_pos;
+
+                       if (nth_bitmapped_pack(m, &bitmapped_pack,
+                                              pack_int_id) < 0)
+                               return;
+                       pack = bitmapped_pack.p;
+                       bitmap_nr = bitmapped_pack.bitmap_nr;
                } else {
                        pack = bitmap_git->pack;
                        /*
@@ -2554,13 +2576,14 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
                         * that we do not expect to read this field.
                         */
                        pack_int_id = -1;
+                       bitmap_nr = pack->num_objects;
                }
 
                if (is_pack_valid(pack)) {
                        ALLOC_GROW(packs, packs_nr + 1, packs_alloc);
                        packs[packs_nr].p = pack;
                        packs[packs_nr].pack_int_id = pack_int_id;
-                       packs[packs_nr].bitmap_nr = pack->num_objects;
+                       packs[packs_nr].bitmap_nr = bitmap_nr;
                        packs[packs_nr].bitmap_pos = 0;
                        packs[packs_nr].from_midx = bitmap_git->midx;
                        packs_nr++;

diff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh
index 881ce668e1d14e17be043d208580fa35f1e1f1e0..126ab9df991ce41428a8a6039a34dfad274b13db 100755 (executable)
--- a/t/t5332-multi-pack-reuse.sh
+++ b/t/t5332-multi-pack-reuse.sh
@@ -4,6 +4,7 @@ test_description='pack-objects multi-pack reuse'
 
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-bitmap.sh
+. "$TEST_DIRECTORY"/lib-pack.sh
 
 GIT_TEST_MULTI_PACK_INDEX=0
 GIT_TEST_MULTI_PACK_INDEX_WRITE_INCREMENTAL=0
@@ -32,6 +33,14 @@ pack_position () {
        grep "$1" objects | cut -d" " -f1
 }
 
+# B as an OFS_DELTA against A at the given one-byte distance.
+pack_obj_b_ofs_a () {
+       pack_obj "$B" "$A" >b-ref.tmp &&
+       printf "\145" &&
+       printf "\\$(printf "%03o" "$1")" &&
+       dd if=b-ref.tmp bs=1 skip=$((1 + $(test_oid rawsz))) 2>/dev/null
+}
+
 # test_pack_objects_reused_all <pack-reused> <packs-reused>
 test_pack_objects_reused_all () {
        : >trace2.txt &&
@@ -288,4 +297,84 @@ test_expect_success 'duplicate objects with verbatim reuse' '
        )
 '
 
+test_expect_success 'reuse with intra-pack duplicate objects' '
+       git init intra-pack-duplicate-objects &&
+       (
+               cd intra-pack-duplicate-objects &&
+
+               # Make enough objects to exercise whole-word reuse.
+               test_commit_bulk 20 &&
+               test_commit --printf A a "\7\0" &&
+               test_commit --printf B b "\7\76" &&
+
+               objects_nr=$(git rev-list --count --objects --all) &&
+               git rev-list --objects --all |
+               cut -d" " -f1 >objects &&
+               A=$(test_oid packlib_7_0) &&
+               B=$(test_oid packlib_7_76) &&
+               grep -v -e "^$A$" -e "^$B$" objects >rest &&
+               pack_obj "$A" >a-full &&
+               pack_obj "$B" >b-full &&
+               while read oid
+               do
+                       pack_obj "$oid" || exit 1
+               done <rest >rest.entries &&
+               {
+                       # Arrange the pack as A, B, A, C..., so that physical
+                       # positions diverge from MIDX pseudo-pack order.
+                       pack_header $((objects_nr + 1)) &&
+                       cat a-full b-full a-full rest.entries
+               } >duplicate.pack &&
+               pack_trailer duplicate.pack &&
+               clear_packs &&
+               git index-pack --stdin <duplicate.pack &&
+
+               git multi-pack-index write --bitmap &&
+               git config pack.allowPackReuse single &&
+               test_pack_objects_reused_all "$objects_nr" 1 &&
+               rm -f got.idx &&
+               test_env GIT_TEST_MIDX_READ_BTMP=false \
+                       test_pack_objects_reused_all 0 0
+       )
+'
+
+test_expect_success 'omit delta whose duplicate base is not selected' '
+       (
+               cd intra-pack-duplicate-objects &&
+
+               A=$(test_oid packlib_7_0) &&
+               B=$(test_oid packlib_7_76) &&
+               objects_nr=$(wc -l <objects) &&
+
+               a_size=$(wc -c <a-full) &&
+               test "$((2 * a_size))" -lt 128 &&
+
+               # The .idx order of duplicate OIDs is unspecified. Try a delta
+               # against each copy and keep the pack whose base was omitted.
+               for distance in "$((2 * a_size))" "$a_size"
+               do
+                       base_offset=$((12 + 2 * a_size - distance)) &&
+                       pack_obj_b_ofs_a "$distance" >b-delta &&
+                       {
+                               pack_header "$((objects_nr + 1))" &&
+                               cat a-full a-full b-delta rest.entries
+                       } >candidate.pack &&
+                       pack_trailer candidate.pack &&
+                       clear_packs &&
+                       git index-pack --stdin <candidate.pack &&
+                       git multi-pack-index write --bitmap || return 1
+
+                       selected_offset=$(
+                               test-tool read-midx --show-objects "$objdir" |
+                               awk -v oid="$A" "\$1 == oid { print \$2 }"
+                       ) || return 1
+                       test -n "$selected_offset" || return 1
+                       test "$selected_offset" = "$base_offset" || break
+               done &&
+               test "$selected_offset" != "$base_offset" &&
+
+               test_pack_objects_reused_all "$((objects_nr - 1))" 1
+       )
+'
+
 test_done