From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 27 Jan 2026 07:34:59 GMT
Subject: Re: [PATCH v2 10/18] midx: do not require packs to be sorted in lexicographic order
Message-ID: <aXhqo3f-NeUcO2IM@pks.im>
In-Reply-To: <72bcd4ed6c7f685f58bb3b905fe553173abe1845.1768420450.git.me@ttaylorr.com>

On Wed, Jan 14, 2026 at 02:54:45PM -0500, Taylor Blau wrote:
[snip]
> Because this change produces MIDXs which may not be correctly read with
> external tools or older versions of Git. Though older versions of Git
> know how to gracefully degrade and ignore any MIDX(s) they consider
> corrupt, external tools may not be as robust. To avoid unintentionally
> breaking any such tools, guard this change behind a version bump in the
> MIDX's on-disk format.

s/Because t/T/?

> diff --git a/midx-write.c b/midx-write.c
> index 8a54644e427..5c8700065a1 100644
> --- a/midx-write.c
> +++ b/midx-write.c
> @@ -36,10 +36,13 @@ extern int cmp_idx_or_pack_name(const char *idx_or_pack_name,
>  
>  static size_t write_midx_header(const struct git_hash_algo *hash_algo,
>  				struct hashfile *f, unsigned char num_chunks,
> -				uint32_t num_packs)
> +				uint32_t num_packs, int version)
>  {
> +	if (version != MIDX_VERSION_V1 && version != MIDX_VERSION_V2)
> +		BUG("unexpected MIDX version: %d", version);

Asking the function to write a MIDX header of an unsupported version
would be a bug indeed.

> @@ -105,6 +108,8 @@ struct write_midx_context {
>  
>  	uint32_t preferred_pack_idx;
>  
> +	int version; /* must be MIDX_VERSION_V1 or _V2 */

Tiny nit: this could be converted into an `enum` for implicit
documentation.

> @@ -1100,6 +1113,7 @@ static int write_midx_internal(struct write_midx_opts *opts)
>  	struct tempfile *incr;
>  	struct write_midx_context ctx = {
>  		.preferred_pack_idx = NO_PREFERRED_PACK,
> +		.version = MIDX_VERSION_V2,
>  	 };
>  	struct multi_pack_index *midx_to_free = NULL;
>  	int bitmapped_packs_concat_len = 0;

Okay. Here we set the default version, ...

> @@ -1114,6 +1128,10 @@ static int write_midx_internal(struct write_midx_opts *opts)
>  	ctx.repo = r;
>  	ctx.source = opts->source;
>  
> +	repo_config_get_int(ctx.repo, "midx.version", &ctx.version);
> +	if (ctx.version != MIDX_VERSION_V1 && ctx.version != MIDX_VERSION_V2)
> +		die(_("unknown MIDX version: %d"), ctx.version);
> +
>  	ctx.incremental = !!(opts->flags & MIDX_WRITE_INCREMENTAL);
>  
>  	if (ctx.incremental)

... but the user can change it. Good.

> diff --git a/midx.c b/midx.c
> index 19ef230d3fd..1327d0a3695 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -656,17 +658,40 @@ int cmp_idx_or_pack_name(const char *idx_or_pack_name,
>  	return strcmp(idx_or_pack_name, idx_name);
>  }
>  
> +
> +static int midx_pack_names_cmp(const void *a, const void *b, void *m_)
> +{
> +	struct multi_pack_index *m = m_;
> +	return strcmp(m->pack_names[*(const size_t *)a],
> +		      m->pack_names[*(const size_t *)b]);
> +}

Okay, this took a second to figure out. The `pack_names_sorted` is an
array of `size_t` indexes into `m->pack_names`. So what we get here are
these indices, and we can compare by using those indices via
`m->pack_names`. Makes sense.

I was wondering whether this would be easier to follow if
`pack_names_sorted` was a simple array of unowned pointers. So it would
contain the same pointers as `pack_names`, but properly sorted. It would
have the downside of more confusing ownership semantics though.

I assume we cannot live with a simple `bool sorted` field and then sort
`pack_names` lazily?

Patrick