Re: [PATCH 6/6] midx.c: include preferred pack correctly with existing MIDX
- From
Derrick Stolee <derrickstolee@github.com>
- Date
- Aug 22, 2022, 17:03 UTC
- Message-ID
- <3cecd058-aec2-d5f9-ef79-58cc10ce14fb@github.com>
- In-Reply-To
- <4ddddc959b042faf7ae98a8e8eaa05e77f9ea23e.1660944574.git.me@ttaylorr.com>
On 8/19/2022 5:30 PM, Taylor Blau wrote:
Show 6 quoted lines
> 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?
Show 10 quoted lines
> @@ -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.
Show 9 quoted lines
> @@ -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.
Show 6 quoted lines
> @@ -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