Re: [PATCH v2 10/18] midx: do not require packs to be sorted in lexicographic order

[Junio C Hamano <gitster@pobox.com>]

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

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

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

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

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

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

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

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