On 8/19/2022 5:30 PM, Taylor Blau wrote: > 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.) ... > This resolves the bug described in the first patch of this series. Thinking ahead to when this is a commit, perhaps this could instead refer to the 'preferred pack change with existing MIDX bitmap' test case? > @@ -610,10 +609,7 @@ static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout, > nth_midxed_pack_midx_entry(m, > &fanout->entries[fanout->nr], > cur_object); > - if (nth_midxed_pack_int_id(m, cur_object) == preferred_pack) > - fanout->entries[fanout->nr].preferred = 1; > - else > - fanout->entries[fanout->nr].preferred = 0; > + fanout->entries[fanout->nr].preferred = 0; > fanout->nr++; Here, we have lost the ability to set the 'preferred' bit from the previous MIDX. Good. > @@ -694,6 +689,11 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m, > preferred, cur_fanout); > } > > + if (-1 < preferred_pack && preferred_pack < start_pack) > + midx_fanout_add_pack_fanout(&fanout, info, > + preferred_pack, 1, > + cur_fanout); > + And here, when there is a preferred pack _in the previous MIDX_, we add its objects a second time, but now with the preferred bit on. If the preferred pack is _not_ in the previous MIDX, then the 'preferred_pack < start_pack' condition will fail and the bits would have been set within the for loop. > @@ -346,7 +346,7 @@ test_expect_success 'preferred pack change with existing MIDX bitmap' ' > test_path_is_file $midx && > test_path_is_file $midx-$(midx_checksum $objdir).bitmap && > > - test_must_fail git clone --no-local . clone2 > + git clone --no-local . clone2 I mentioned in patch 1 that this test could use some comments about what is unexpected and what _is_ expected. I think this comment needs an update in this patch: # Generate a new MIDX which changes the preferred pack to a pack # contained in the existing MIDX, such that not all objects from # p2 that appear in the MIDX had their copy selected from p2. Thanks, -Stolee
