Re: [PATCH v2 09/24] midx: infer preferred pack when not given one

On Wed, Jul 21, 2021 at 06:34:25AM -0400, Jeff King wrote:

On Mon, Jun 21, 2021 at 06:25:21PM -0400, Taylor Blau wrote:

quoted

In 9218c6a40c (midx: allow marking a pack as preferred, 2021-03-30), the
multi-pack index code learned how to select a pack which all duplicate
objects are selected from. That is, if an object appears in multiple
packs, select the copy in the preferred pack before breaking ties
according to the other rules like pack mtime and readdir() order.

Not specifying a preferred pack can cause serious problems with
multi-pack reachability bitmaps, because these bitmaps rely on having at
least one pack from which all duplicates are selected. Not having such a
pack causes problems with the pack reuse code (e.g., like assuming that
a base object was sent from that pack via reuse when in fact the base
was selected from a different pack).

It might be helpful to use a more descriptive name for "pack reuse code"
here, since it's kind of vague for people who have not been actively
working on bitmaps.

I don't have a short name for that chunk of code, but maybe:

  ...causes problems with the code in pack-objects to reuse packs
  verbatim (e.g., that code assumes that a delta object in a chunk of
  pack sent verbatim will have its base object sent from the same pack).

Thanks; I like what you wrote here.

quoted

  - The psuedo pack-order (described in
    Documentation/technical/bitmap-format.txt) is computed by
    midx_pack_order(), and sorts by pack ID and pack offset, with
    preferred packs sorting first.

I think the .rev description in pack-format.txt may be a better
reference here.

Ditto, I changed that, too.

quoted

  - But! Pack IDs come from incrementing the pack count in
    add_pack_to_midx(), which is a callback to
    for_each_file_in_pack_dir(), meaning that pack IDs are assigned in
    readdir() order.

[ ... ]

This explanation is rather confusing, but I'm not sure if we can do much
better. I followed all of it, because I was there when we found the bug
that this is fixing. And of course that happened _after_ we implemented
midx bitmaps and in particular adapted the verbatim reuse stuff in
pack-objects to make use of it.

I see why you'd want to float the fix up before then, so we don't ever
have the broken state. But it's hard to understand what bug this is
fixing, because the bug does not even exist yet at this point in
the series!

I dunno. Like I said, I was able to follow it, so maybe it is
sufficient. I'm just not sure others would be able to.

I think that others will follow it, too. But I agree that it is
confusing, since we're fixing a bug that doesn't yet exist. In reality,
I wrote this patch after sending v1, and then reordered its position to
come before the implementation of MIDX bitmaps for that reason.

So in one sense, I prefer it this way because we don't ever introduce
the bug.  But in another sense, it is very jarring to read about an
interaction that has no basis in the code (yet).

I think that the best thing we could do without adding any significant
reordering would be to just call out the situation we're in. I added
this onto the end of the commit message which I think makes things a
little clearer:

    (Note that multi-pack reachability bitmaps have yet to be
    implemented; so in that sense this patch is fixing a bug which does
    not yet exist.  But by having this patch beforehand, we can prevent
    the bug from ever materializing.)

Thanks,
Taylor