Re: [PATCH v2 10/18] midx: do not require packs to be sorted in lexicographic order
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 14, 2026, 21:28 UTC
- Message-ID
- <xmqqtswnevfc.fsf@gitster.g>
- In-Reply-To
- <72bcd4ed6c7f685f58bb3b905fe553173abe1845.1768420450.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
Show 6 quoted lines
> @@ -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 ...
Show 14 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);
> +... do we need to add version parameter to this function? Unless the writer that calls this helper function demotes version to 1 when it realizes that the pack files we write midx for happens to be sorted, in which case it may need to call this function with version set to 1, I do not quite see why we need it.
Show 12 quoted lines
> 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?
Show 11 quoted lines
> @@ -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.
Show 10 quoted lines
> @@ -1025,6 +1032,12 @@ static bool midx_needs_update(struct multi_pack_index *midx, struct write_midx_c > if (!midx_checksum_valid(midx)) > goto out; > > + /* > + * If the version differs, we need to update. > + */ > + if (midx->version != ctx->version) > + goto out; > +
OK.
Show 16 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;
> @@ -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);
> +Ditto.
Show 9 quoted lines
> @@ -1445,7 +1463,7 @@ static int write_midx_internal(struct write_midx_opts *opts) > } > > write_midx_header(r->hash_algo, f, get_num_chunks(cf), > - ctx.nr - dropped_packs); > + ctx.nr - dropped_packs, ctx.version); > write_chunkfile(cf, &ctx); > > finalize_hashfile(f, midx_hash, FSYNC_COMPONENT_PACK_METADATA,
Ditto.
Show 21 quoted lines
> diff --git a/midx.h b/midx.h
> index a39bcc9d03f..aa99a6cb215 100644
> --- a/midx.h
> +++ b/midx.h
> @@ -11,7 +11,8 @@ struct git_hash_algo;
> struct odb_source;
>
> #define MIDX_SIGNATURE 0x4d494458 /* "MIDX" */
> -#define MIDX_VERSION 1
> +#define MIDX_VERSION_V1 1
> +#define MIDX_VERSION_V2 2
> #define MIDX_BYTE_FILE_VERSION 4
> #define MIDX_BYTE_HASH_VERSION 5
> #define MIDX_BYTE_NUM_CHUNKS 6
> @@ -71,6 +72,7 @@ struct multi_pack_index {
> uint32_t num_packs_in_base;
>
> const char **pack_names;
> + size_t *pack_names_sorted;
> struct packed_git **packs;
> };This does make sense. The code paths that reads existing on-disk files must notice when they are dealing with v1 format and need to act differently.
Thanks.