From: Jeff King <peff@peff.net>
Date: Fri, 02 Oct 2026 23:41:57 GMT
Subject: Re: [PATCH v2 8/8] repack: include required packs in incremental MIDX writes
Message-ID: <20261002234157.GF834759@coredump.intra.peff.net>
In-Reply-To: <a42f775cbe27b385bfc8ff38f33604b3913dc340.1790827875.git.me@ttaylorr.com>

On Wed, Sep 30, 2026 at 11:12:05PM -0500, Taylor Blau wrote:

> The geometric plan from 1da62fb5c86 (repack: implement incremental MIDX
> repacking, 2026-05-19) can omit kept and cruft packs, since neither
> necessarily participates in the geometric repack. Such packs can also be
> lost when replacing a tip layer that contains them. Neither plan
> consults `midx_included_packs()`, so the rules for retaining cruft in
> ordinary MIDX writes do not protect incremental writes.
> 
> Use that selection logic to add missing packs to each plan's write step.
> Skip packs in retained base layers, but include required packs from a
> replaced tip. Count added objects when choosing which layers to compact,
> without changing the preferred pack.

I admit I had a hard time following this patch. I think the point is
that we're going to include some packs in the midx that were not covered
previously. But it was hard to see where that happens. I think the magic
bit is this:

> @@ -557,17 +604,20 @@ static void repack_make_midx_append_plan(struct repack_write_midx_opts *opts,
>  					 size_t *steps_nr_p)
> [..]
> -	for (i = 0; i < opts->names->nr; i++) {
> +	midx_included_packs(&include, opts, m);

where we rely on midx_included_packs() to do that selection.

So I _think_ this is doing the right thing, but my confidence in my
review is kind of low. To some degree I'd just rely on the functional
tests here.

-Peff