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

From: Jeff King <peff@peff.net>
Date: Aug 1, 2024, 11:14 UTC
Message-ID: <20240801111410.GS1159276@coredump.intra.peff.net>
In-Reply-To: <cover.1721250704.git.me@ttaylorr.com>

On Wed, Jul 17, 2024 at 05:11:54PM -0400, Taylor Blau wrote:

Show 20 quoted lines

> This series implements incremental MIDXs, which allow for storing
> a MIDX across multiple layers, each with their own distinct set of
> packs.
> 
> This round is mostly unchanged from the previous since there has not yet
> been substantial review. But it does rebase to current 'master' (which
> is 04f5a52757 (Post 2.46-rc0 batch #2, 2024-07-16), at the time of
> writing).
> 
> Importantly, this rebase moves this topic to be based on an ancestor of
> 0c5a62f14b (midx-write.c: do not read existing MIDX with
> `packs_to_include`, 2024-06-11), which resulted in a non-trivial
> conflict prior to this rebase.
> 
> The rest of the topic is unchanged. I don't expect that we'll see much
> review here for the next couple of weeks while we are in the -rc phase,
> but I figured it would be useful to have it on the list for folks that
> are interested in taking a look.
> 
> Thanks in advance for any review! :-)

I gave it a pretty thorough look. Everything looks good for the most part. I left a few comments, but mostly just thinking my way through things.

The trickiest parts were:

  - the confusion between when we want local per-layer positions versus
    global positions within the whole chainfile, or whether functions
    are operating on a single layer versus the whole chain. I mused a
    bit on how we could do it differently, but ultimately I'm not sure
    there any good solutions.

  - the changes you did make look good, but it's hard to know if there's
    code lurking that still needs to be adjusted for chained midx's. For
    that I think I'd turn more towards testing than code review. I'm not
    sure how much interesting coverage we're getting from the GIT_TEST
    variable, just because the repositories made in most of the tests
    are so trivial.

    I'd love to see the results on a real workload (both a big repo, but
    also how things behave over days or weeks of repository maintenance
    done with incremental midxs). I know that can be hard to do, though.

-Peff