From: Taylor Blau <me@ttaylorr.com>
Date: Tue, 27 Jan 2026 22:13:28 GMT
Subject: Re: [PATCH v2 17/18] midx: implement MIDX compaction
Message-ID: <aXk4iIHRu3mPxY6S@nand.local>
In-Reply-To: <aXhqroubXFbnBgJI@pks.im>

On Tue, Jan 27, 2026 at 08:35:10AM +0100, Patrick Steinhardt wrote:
> > +	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!

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

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