From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 27 Jan 2026 07:35:10 GMT
Subject: Re: [PATCH v2 17/18] midx: implement MIDX compaction
Message-ID: <aXhqroubXFbnBgJI@pks.im>
In-Reply-To: <13336e864f4ed3a6954b782f0bcc090d92ac722c.1768420450.git.me@ttaylorr.com>

On Wed, Jan 14, 2026 at 02:55:10PM -0500, Taylor Blau wrote:
> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
> index c0c6c1760c0..043ee8c478a 100644
> --- a/builtin/multi-pack-index.c
> +++ b/builtin/multi-pack-index.c
> @@ -195,6 +204,70 @@ static int cmd_multi_pack_index_write(int argc, const char **argv,
[snip]
> +	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.

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

> @@ -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()`?

Patrick