From: Taylor Blau <me@ttaylorr.com>
Date: Wed, 14 Jan 2026 21:44:54 GMT
Subject: Re: [PATCH v2 10/18] midx: do not require packs to be sorted in lexicographic order
Message-ID: <aWgOVnp+zkGiR8p6@nand.local>
In-Reply-To: <xmqqtswnevfc.fsf@gitster.g>

On Wed, Jan 14, 2026 at 01:28:07PM -0800, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > @@ -374,7 +374,7 @@ HEADER:
> >  	    The signature is: {'M', 'I', 'D', 'X'}
> >
> >  	1-byte version number:
> > -	    Git only writes or recognizes version 1.
> > +	    Git only writes version 2, but recognizes versions 1 and 2.
>
> We only write version 2, then ...

Ah, thanks for spotting.

This comment is outdated, and I didn't catch it when proof-reading
the patches. I wrote this line before adding the "midx.version" config
escape hatch, and doing so made this comment stale.

It should probably look something like:

    Git writes the version specified by the "midx.version"
    configuration option, which defaults to 2. It recognizes
    both versions 1 and 2.

I'll update it and include it in the subsequent round.

> >  	hashwrite_be32(f, MIDX_SIGNATURE);
> > -	hashwrite_u8(f, MIDX_VERSION);
> > +	hashwrite_u8(f, version);
> >  	hashwrite_u8(f, oid_version(hash_algo));
> >  	hashwrite_u8(f, num_chunks);
> >  	hashwrite_u8(f, 0); /* unused */
> > @@ -105,6 +108,8 @@ struct write_midx_context {
> >
> >  	uint32_t preferred_pack_idx;
> >
> > +	int version; /* must be MIDX_VERSION_V1 or _V2 */
> > +
>
> Ditto.  When we are writing it out, shouldn't this always be 2
> anyway?

For this and below, the code here is right and the documentation is
wrong (per above).

> > @@ -410,7 +415,9 @@ static int write_midx_pack_names(struct hashfile *f, void *data)
> >  		if (ctx->info[i].expired)
> >  			continue;
> >
> > -		if (i && strcmp(ctx->info[i].pack_name, ctx->info[i - 1].pack_name) <= 0)
> > +		if (ctx->version == MIDX_VERSION_V1 &&
> > +		    i && strcmp(ctx->info[i].pack_name,
> > +				ctx->info[i - 1].pack_name) <= 0)
> >  			BUG("incorrect pack-file order: %s before %s",
> >  			    ctx->info[i - 1].pack_name,
> >  			    ctx->info[i].pack_name);
>
> Ditto.

Ditto (and so on) ;-).

Thanks,
Taylor