Re: [PATCH v2 01/19] Documentation: describe incremental MIDX format

From: Taylor Blau <me@ttaylorr.com>
Date: Aug 1, 2024, 18:52 UTC
Message-ID: <ZqvZeWDGDAeZNZjW@nand.local>
In-Reply-To: <20240801091952.GA1159276@coredump.intra.peff.net>

On Thu, Aug 01, 2024 at 05:19:52AM -0400, Jeff King wrote:

Show 15 quoted lines

> > +As above, there are no fundamental limitations that stand in the way of
> > +extending the incremental MIDX format to support reachability bitmaps.
> > +The design below specifically takes this into account, and support for
> > +reachability bitmaps will be added in a future patch series. It is
> > +omitted from this series for the same reason as above.
>
> It is nice that you added a bit of a roadmap here about what is
> implemented and what is not, and that the design takes into account
> future directions (especially incremental bitmap generation).
>
> It does feel a little funny to say "this series" in text that will go
> into the repository (i.e., somebody reading the checked out file will
> say "huh? which series?"). I'm not sure how to word it better, except to
> maybe just say "in the future" and "it is omitted for now" (and
> obviously it's a pretty minor point).

s/this series/the current implementation/ ?

Definitely an oversight on my part, thanks for catching. I'll squash it in.

Show 13 quoted lines

> > +The `multi-pack-index-$H1.midx` file contains the first layer of the
> > +multi-pack-index chain. The `multi-pack-index-$H2.midx` file contains
> > +the second layer of the chain, and so on.
>
> Makes sense. How does the chained multi-pack-index.d interact with a
> singular multi-pack-index? Generally we should not have both at the same
> time, but I'd imagine they both exist for a brief period when moving
> from one to another.
>
> I assume the rules are the same as for commit-graphs, which use the same
> on-disk structure. I can't think of a reason to prefer one over the
> other but this might be a good place to document what does/should
> happen.

The commit-graph code reads the non-chained commit-graph first (see commit-graph.c::read_commit_graph_one() for exact details, paraphrased here):

    struct commit_graph *g = load_commit_graph_v1(...);
    if (!g)
            g = load_commit_graph_chain(...);
    return g;

and I matched the same for the MIDX code. I think there are reasonable arguments for preferring either one over the other, so I think the easiest thing to do is just throw our hands up and stick with the convention ;-).

Show 17 quoted lines

> > +=== Object positions for incremental MIDXs
> > +
> > +In the original multi-pack-index design, we refer to objects via their
> > +lexicographic position (by object IDs) within the repository's singular
> > +multi-pack-index. In the incremental multi-pack-index design, we refer
> > +to objects via their index into a concatenated lexicographic ordering
> > +among each component in the MIDX chain.
>
> How do duplicate objects work here? I guess there aren't any duplicates
> in the midx itself, only in the constituent packfiles. So from the
> perspective of this section, I guess it doesn't matter? And from the
> perspective of bitmaps (where the duplicate issue came up before), it is
> business as usual: the midx revindex gives the bit order, and we'd
> presumably concatenate the individual revindexes in chain order.
>
> (Mostly just thinking out loud; I'm not sure there's much for you to
> answer there).

Right. In a pre-incremental MIDX world, MIDXs contain no duplicate object entries with respect to themselves. In this new world, the same is true, with the additional property that MIDXs also contain no duplicates with respect to their ancestors (when part of a MIDX chain).

> Looking good so far...
>
> -Peff

Thanks, Taylor