From: Taylor Blau <ttaylorr@openai.com>
Date: Sat, 03 Oct 2026 01:01:59 GMT
Subject: Re: [PATCH v2 8/8] repack: include required packs in incremental MIDX writes
Message-ID: <asBUBwM2N8lQM602@com-79390>
In-Reply-To: <20261002234157.GF834759@coredump.intra.peff.net>

On Fri, Oct 02, 2026 at 07:41:57PM -0400, Jeff King wrote:
> 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.

Yeah, that's right. I wrote this code in the first place, and it wasn't
even *that* long ago and I had to spend a not-insignificant amount of
time (re)acquainting myself with this area before writing this patch.

Thanks,
Taylor