Re: [PATCH v2 08/15] midx: allow marking a pack as preferred

[Jonathan Tan <jonathantanmy@google.com>]

> @@ -589,12 +619,17 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,
>  				nth_midxed_pack_midx_entry(m,
>  							   &entries_by_fanout[nr_fanout],
>  							   cur_object);
> +				if (nth_midxed_pack_int_id(m, cur_object) == preferred_pack)
> +					entries_by_fanout[nr_fanout].preferred = 1;
> +				else
> +					entries_by_fanout[nr_fanout].preferred = 0;
>  				nr_fanout++;
>  			}
>  		}
>  
>  		for (cur_pack = start_pack; cur_pack < nr_packs; cur_pack++) {
>  			uint32_t start = 0, end;
> +			int preferred = cur_pack == preferred_pack;
>  
>  			if (cur_fanout)
>  				start = get_pack_fanout(info[cur_pack].p, cur_fanout - 1);
> @@ -602,7 +637,11 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,
>  
>  			for (cur_object = start; cur_object < end; cur_object++) {
>  				ALLOC_GROW(entries_by_fanout, nr_fanout + 1, alloc_fanout);
> -				fill_pack_entry(cur_pack, info[cur_pack].p, cur_object, &entries_by_fanout[nr_fanout]);
> +				fill_pack_entry(cur_pack,
> +						info[cur_pack].p,
> +						cur_object,
> +						&entries_by_fanout[nr_fanout],
> +						preferred);
>  				nr_fanout++;
>  			}
>  		}

I was initially confused that "preferred" was set twice, but this makes
sense - the first one is when an existing midx is reused, and the second
one is for objects in packs that the midx (if it exists) does not cover.

> @@ -828,7 +869,19 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *
>  	if (ctx.m && ctx.nr == ctx.m->num_packs && !packs_to_drop)
>  		goto cleanup;
>  
> -	ctx.entries = get_sorted_entries(ctx.m, ctx.info, ctx.nr, &ctx.entries_nr);
> +	if (preferred_pack_name) {
> +		for (i = 0; i < ctx.nr; i++) {
> +			if (!cmp_idx_or_pack_name(preferred_pack_name,
> +						  ctx.info[i].pack_name)) {
> +				ctx.preferred_pack_idx = i;
> +				break;
> +			}
> +		}
> +	} else
> +		ctx.preferred_pack_idx = -1;

Looks safer to put "ctx.preferred_pack_idx = -1" before the "if", just
in case the given pack name does not exist?

> @@ -889,6 +942,31 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *
>  			pack_name_concat_len += strlen(ctx.info[i].pack_name) + 1;
>  	}
>  
> +	/*
> +	 * Recompute the preferred_pack_idx (if applicable) according to the
> +	 * permuted pack order.
> +	 */
> +	ctx.preferred_pack_idx = -1;
> +	if (preferred_pack_name) {
> +		ctx.preferred_pack_idx = lookup_idx_or_pack_name(ctx.info,
> +							     ctx.nr,
> +							     preferred_pack_name);
> +		if (ctx.preferred_pack_idx < 0)
> +			warning(_("unknown preferred pack: '%s'"),
> +				preferred_pack_name);
> +		else {
> +			uint32_t orig = ctx.info[ctx.preferred_pack_idx].orig_pack_int_id;
> +			uint32_t perm = ctx.pack_perm[orig];
> +
> +			if (perm == PACK_EXPIRED) {
> +				warning(_("preferred pack '%s' is expired"),
> +					preferred_pack_name);
> +				ctx.preferred_pack_idx = -1;
> +			} else
> +				ctx.preferred_pack_idx = perm;
> +		}
> +	}

I couldn't figure out why the preferred pack index needs to be
recalculated here, since the pack entries would have already been
sorted. Also, the tests still pass when I comment this part out. A
comment describing what's going on would be helpful.

All previous patches look good to me.