midx.c: include preferred pack correctly with existing MIDX

This patch resolves an issue where the object order used to generate a MIDX bitmap would violate an invariant that all of the preferred pack's objects are represented by that pack in the MIDX. The problem arises when reusing an existing MIDX while generating a new one, and occurs specifically when the identity of the preferred pack changes from one MIDX to another, along with a few other conditions: - the new preferred pack must also be present in the existing MIDX - the new preferred pack must *not* have been the preferred pack in the existing MIDX - most importantly, there must be at least one object present in the physical preferred pack (ie., it shows up in that pack's index) but was selected from a *different* pack when the previous MIDX was generated When the above conditions are all met, we end up (incorrectly) discarding copies of some objects in the pack selected as the preferred pack. This is because `get_sorted_entries()` adds objects to its list by doing the following at each fanout level: - first, adding all objects from that fanout level from an existing MIDX - then, adding all objects from that fanout level in each pack *not* included in the existing MIDX So if some object was not selected from the to-be-preferred pack when writing the previous MIDX, then we will never consider it as a candidate when generating the new MIDX. This means that it's possible for the preferred pack to not include all of its objects in the MIDX's pseudo-pack object order, which is an invariant violation of that order. Resolve this by adding all objects from the preferred pack separately when it appears in the existing MIDX (if one was present). This will duplicate objects from that pack that *did* appear in the MIDX, but this is fine, since get_sorted_entries() already handles duplicates. (A future optimization in this area could avoid adding copies of objects that we know already existing in the MIDX.) Note that we no longer need to compute the preferred-ness of objects added from the MIDX, since we only want to select the preferred objects from a single source. (We could still mark these preferred bits, but doing so is redundant and unnecessary). This resolves the bug demonstrated by t5326.174 ("preferred pack change with existing MIDX bitmap"). Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Taylor Blau committed Aug 22, 2022 at 15:50 UTC cdf517be06b83c01d0411709c1fac0c2fc5c1a9b
2 files changed +12 -15
midx.c
+7 -7
@@ -595,7 +595,6 @@ static void midx_fanout_sort(struct midx_fanout *fanout)
595
596 static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,
597 struct multi_pack_index *m,
598 - int preferred_pack,
598 uint32_t cur_fanout)
599 {
600 uint32_t start = 0, end;
@@ -610,10 +609,7 @@ static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,
609 nth_midxed_pack_midx_entry(m,
610 &fanout->entries[fanout->nr],
611 cur_object);
613 - if (nth_midxed_pack_int_id(m, cur_object) == preferred_pack)
614 - fanout->entries[fanout->nr].preferred = 1;
615 - else
616 - fanout->entries[fanout->nr].preferred = 0;
612 + fanout->entries[fanout->nr].preferred = 0;
613 fanout->nr++;
614 }
615 }
@@ -684,8 +680,7 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,
680 fanout.nr = 0;
681
682 if (m)
687 - midx_fanout_add_midx_fanout(&fanout, m, preferred_pack,
688 - cur_fanout);
683 + midx_fanout_add_midx_fanout(&fanout, m, cur_fanout);
684
685 for (cur_pack = start_pack; cur_pack < nr_packs; cur_pack++) {
686 int preferred = cur_pack == preferred_pack;
@@ -694,6 +689,11 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,
689 preferred, cur_fanout);
690 }
691
692 + if (-1 < preferred_pack && preferred_pack < start_pack)
693 + midx_fanout_add_pack_fanout(&fanout, info,
694 + preferred_pack, 1,
695 + cur_fanout);
696 +
697 midx_fanout_sort(&fanout);
698
699 /*
t/t5326-multi-pack-bitmaps.sh
+5 -8
@@ -338,19 +338,16 @@ test_expect_success 'preferred pack change with existing MIDX bitmap' '
338 git pack-objects --all --unpacked $objdir/pack/pack0 &&
339
340 # Generate a new MIDX which changes the preferred pack
341 - # to a pack contained in the existing MIDX, such that
342 - # not all objects from p2 that appear in the MIDX had
343 - # their copy selected from p2.
341 + # to a pack contained in the existing MIDX.
342 git multi-pack-index write --bitmap \
343 --preferred-pack="pack-$p2.pack" &&
344 test_path_is_file $midx &&
345 test_path_is_file $midx-$(midx_checksum $objdir).bitmap &&
346
349 - # When the above circumstances are met, an existing bug
350 - # in the MIDX machinery will cause the reverse index to
351 - # be read incorrectly, resulting in failed clones (among
352 - # other things).
353 - test_must_fail git clone --no-local . clone2
347 + # When the above circumstances are met, the preferred
348 + # pack should change appropriately and clones should
349 + # (still) succeed.
350 + git clone --no-local . clone2
351 )
352 '
353