Re: [PATCH v2 8/8] repack: include required packs in incremental MIDX writes
- From
Taylor Blau <ttaylorr@openai.com>
- Date
- Oct 3, 2026, 01:01 UTC
- 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:
Show 26 quoted lines
> 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