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

On Fri, Jul 23, 2021 at 04:50:31AM -0400, Jeff King wrote:

On Wed, Jul 21, 2021 at 04:16:07PM -0400, Taylor Blau wrote:

quoted

quoted

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.)

I do like fixing it up front. Here's my attempt at rewriting the commit
message. I tried to omit details about pack order, and instead refer to
the revindex code, and instead add more explanation of how this relates
to the pack-reuse code.

Something like:

[...]

Thoughts?

I like it, although reading it fresh I found the sentence beginning with
"So if the user did not specify a preferred pack" to be a little
confusing. To connect it back to the previous paragraph, I added:

  ... in order to avoid a situation where no pack is marked as preferred
  (breaking our assumption about the pack representing the object at the
  0th bit).

and that read out much clearer (to me at least).

Thanks,
Taylor