git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Taylor Blau <me@ttaylorr.com>
Date
Aug 22, 2022, 18:14 UTC
Message-ID
<YwPHh/EDIS0S4uoj@nand.local>
In-Reply-To
<3cecd058-aec2-d5f9-ef79-58cc10ce14fb@github.com>
On Mon, Aug 22, 2022 at 01:03:11PM -0400, Derrick Stolee wrote:
Show 16 quoted lines
> 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?
Good idea, thanks.
Show 13 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.

Yep, we don't want to propagate any of these bits forward when reusing an existing MIDX. Thinking on it more, I think this is the only legitimate use of MIDX reuse in the "I'm about to write bitmaps" context.

I mentioned before the idea that we could use `--stdin-packs` now when writing a MIDX bitmap where before it wasn't implemented (likely due to problems caused by this bug). But the whole premise doesn't make a ton of sense:

  - Every pack that's in the include_packs list would need to be handled
    separately.
  - And every pack that *isn't* in that list would be skipped.

Which means that it wouldn't help at all to reuse an existing MIDX. The reason that we'd need to handle all included packs separately is subtle and a little different from what's going on here, though. The problem there is that if you have two packs, say P1 and P2, and P1 is in the include list but P2 is not, then any objects duplicated between the two and selected from P2 will disappear when writing the new MIDX.

Since the set of packs that are going into the new MIDX are precisely equal to the set of packs that we'd need to handle separately, it probably makes sense to continue to avoid using the existing MIDX when writing a bitmap with the `--stdin-packs` option.

Show 15 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.
Exactly!
Show 14 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.

Good eyes, thanks for spotting. I updated the comment below, too (which doesn't exist in this version of the patch, but you suggested adding to the first patch in this series).

Thanks, Taylor

Previous: Derrick StoleeNext: Derrick Stolee
Message 17 of 29 in “midx: permit changing the preferred pack when reusing the MIDX”
  1. 0/6 midx: permit changing the preferred pack when reusing the MIDXTaylor Blau, Aug 19, 2022
  2. 1/6 t5326: demonstrate potential bitmap corruptionTaylor Blau, Aug 19, 2022
  3. Derrick StoleeAug 22, 2022
  4. Taylor BlauAug 22, 2022
  5. Junio C HamanoAug 22, 2022
  6. Taylor BlauAug 22, 2022
  7. 2/6 t/lib-bitmap.sh: avoid silencing stderrTaylor Blau, Aug 19, 2022
  8. Abhradeep ChakrabortyAug 20, 2022
  9. Taylor BlauAug 22, 2022
  10. 3/6 midx.c: extract `struct midx_fanout`Taylor Blau, Aug 19, 2022
  11. 4/6 midx.c: extract `midx_fanout_add_midx_fanout()`Taylor Blau, Aug 19, 2022
  12. 5/6 midx.c: extract `midx_fanout_add_pack_fanout()`Taylor Blau, Aug 19, 2022
  13. 6/6 midx.c: include preferred pack correctly with existing MIDXTaylor Blau, Aug 19, 2022
  14. Abhradeep ChakrabortyAug 20, 2022
  15. Taylor BlauAug 22, 2022
  16. Derrick StoleeAug 22, 2022
  17. Taylor BlauAug 22, 2022
  18. Derrick StoleeAug 22, 2022
  19. Taylor BlauAug 22, 2022
  20. 0/7 midx: permit changing the preferred pack when reusing the MIDXTaylor Blau, Aug 22, 2022
  21. 1/7 t5326: demonstrate potential bitmap corruptionTaylor Blau, Aug 22, 2022
  22. 2/7 t/lib-bitmap.sh: avoid silencing stderrTaylor Blau, Aug 22, 2022
  23. 3/7 midx.c: extract `struct midx_fanout`Taylor Blau, Aug 22, 2022
  24. 4/7 midx.c: extract `midx_fanout_add_midx_fanout()`Taylor Blau, Aug 22, 2022
  25. 5/7 midx.c: extract `midx_fanout_add_pack_fanout()`Taylor Blau, Aug 22, 2022
  26. 7/7 midx.c: avoid adding preferred objects twiceTaylor Blau, Aug 22, 2022
  27. Derrick StoleeAug 23, 2022
  28. 6/7 midx.c: include preferred pack correctly with existing MIDXTaylor Blau, Aug 22, 2022
  29. Derrick StoleeAug 23, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.