Re: [PATCH v3 00/19] midx: incremental multi-pack indexes, part one

From: Jeff King <peff@peff.net>
Date: Aug 12, 2024, 14:27 UTC
Message-ID: <20240812142712.GA3800004@coredump.intra.peff.net>
In-Reply-To: <cover.1722958595.git.me@ttaylorr.com>

On Tue, Aug 06, 2024 at 11:36:36AM -0400, Taylor Blau wrote:

Show 18 quoted lines

> As usual, a range-diff is below, but the main changes since last time
> are as follows:
> 
>   - Documentation improvements to clarify what happens when both an
>     incremental- and non-incremental MIDX are both present in a
>     repository.
> 
>   - Commit message typofix on 3/19 to fix an error in one of the
>     technical examples.
> 
>   - Dropped a custom 'local_pack_int_id' in 4/19 to make the remaining
>     diff easier to read.
> 
>   - Minor bugfix in 7/19 where we incorrectly terminated the object
>     abbreviation disambiguation step for incremental MIDXs.
> 
>   - Various additional bits of information in the commit message to
>     explain anything that was subtle.

This all looks good to me.

I read over your responses to my previous review, but as you answered all of my questions I didn't respond to each individually. :)

I looked over the changes in this iteration and they address all the small points I brought up. In the bigger picture, I do think there are probably still issues lurking around the global/local pack and objection position ids. But where you have the series now seems like a good cut-off point:

  - I suspect pack_pos_to_midx() might need to be adjusted. But it's not
    an issue that can be triggered until until we support incremental
    midx bitmaps. And that should definitely go in its own series (and
    preparing is kind of pointless because we don't know what the
    correct interface will be until mid-way through that topic).

  - I won't be surprised if there is some global/local bug that shakes
    out in the long run. But I don't have a clever way of preventing it
    or avoiding the need to deal with the distinction. So I think the
    best path is forward, and to let the shaking commence.

    The most important thing to that the new on-disk files are sane,
    since those are hard to walk back later. A simple bug in the code
    can always be fixed. Likewise, the changes here are unlikely to
    create a bug for anybody using a single unchained midx. So even if
    there is a bug, it will only be triggerable for people using the
    experimental mode.

-Peff