diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 3673b14b89..c333922961 100644 --- 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 d8dc4ae8d1..8141290436 100644 --- 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 881ce668e1..126ab9df99 100755 --- 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 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.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 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