Re: [PATCH 6/6] midx.c: include preferred pack correctly with existing MIDX

[Derrick Stolee <derrickstolee@github.com>]

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