Re: [PATCH v2 10/18] midx: do not require packs to be sorted in lexicographic order
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Jan 27, 2026, 07:34 UTC
- 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]
Show 6 quoted lines
> 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/?
Show 13 quoted lines
> 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.
Show 5 quoted lines
> @@ -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.
Show 8 quoted lines
> @@ -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, ...
Show 11 quoted lines
> @@ -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.
Show 15 quoted lines
> 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