Re: [PATCH v2 8/8] repack: include required packs in incremental MIDX writes

From: Jeff King <peff@peff.net>
Date: Oct 2, 2026, 23:41 UTC
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:

Show 11 quoted lines

> 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:

Show 5 quoted lines

> @@ -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