Re: [PATCH v2 17/18] midx: implement MIDX compaction

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 27, 2026, 22:13 UTC
Message-ID: <aXk4iIHRu3mPxY6S@nand.local>
In-Reply-To: <aXhqroubXFbnBgJI@pks.im>

On Tue, Jan 27, 2026 at 08:35:10AM +0100, Patrick Steinhardt wrote:

Show 13 quoted lines

> > +	if (!from_midx)
> > +		die(_("could not find MIDX: %s"), argv[0]);
> > +	if (!to_midx)
> > +		die(_("could not find MIDX: %s"), argv[1]);
> > +	if (from_midx == to_midx)
> > +		die(_("MIDX compaction endpoints must be unique"));
> > +
> > +	for (m = from_midx; m; m = m->base_midx) {
> > +		if (m == to_midx)
> > +			die(_("MIDX %s must be an ancestor of %s"), argv[0], argv[1]);
> > +	}
>
> These new checks all feel sensible to me.

Thanks for taking a look and suggesting them in the first place!

Show 17 quoted lines

> > diff --git a/midx-write.c b/midx-write.c
> > index ca2469213e6..afa077a09cc 100644
> > --- a/midx-write.c
> > +++ b/midx-write.c
> > @@ -1120,12 +1216,23 @@ static bool midx_needs_update(struct multi_pack_index *midx, struct write_midx_c
> > @@ -1162,6 +1270,19 @@ static int write_midx_internal(struct write_midx_opts *opts)
> >  		die(_("unknown MIDX version: %d"), ctx.version);
> >
> >  	ctx.incremental = !!(opts->flags & MIDX_WRITE_INCREMENTAL);
> > +	ctx.compact = !!(opts->flags & MIDX_WRITE_COMPACT);
> > +
> > +	if (ctx.compact) {
> > +		if (ctx.version != MIDX_VERSION_V2)
> > +			die(_("cannot perform MIDX compaction with v1 format"));
>
> Right. So if the user has configured "midx.version=1" they cannot
> compact.

Exactly. I think the limitation here is a fundamental one, too, since by its nature compaction *must* retain the pseudo-pack order concatenated across each MIDX layer in the compaction range. With midx.version=1, we don't have a way to express that information in a backwards-compatible way, so midx.version=2 here is a requirement.

Show 10 quoted lines

> > @@ -1354,12 +1491,19 @@ static int write_midx_internal(struct write_midx_opts *opts)
> >  			ctx.large_offsets_needed = 1;
> >  	}
> >
> > -	QSORT(ctx.info, ctx.nr, pack_info_compare);
> > +	if (ctx.compact) {
> > +		if (ctx.version != MIDX_VERSION_V2)
> > +			BUG("performing MIDX compaction with v1 MIDX");
>
> Isn't this `BUG()` redundant with the above call to `die()`?

Technically, though I put it in here as a sanity check to ensure that any potential regressions with the above die() don't cause us to get into a worse situation that would result in bitmap corruption.

Thanks, Taylor