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

[Jonathan Tan <jonathantanmy@google.com>]

> Funny you mention that; I was wondering the same thing myself the other
> day when reading these patches again before deploying them to a couple
> of testing repositories at GitHub.
> 
> It is totally unnecessary: since we have already marked objects from the
> preferred pack in get_sorted_entries(), the rest of the code doesn't
> care if the preferred pack was permuted or not.
> 
> But we *do* care if the pack which was preferred expired. The 'git
> repack --geometric --write-midx' caller (which will appear in a later
> series) should never do that, so emitting a warning() is worthwhile.

Ah, this makes sense.

> I
> think ultimately you want something like this squashed in:
> 
> --- >8 ---
> 
> diff --git a/midx.c b/midx.c
> index d2c56c4bc6..46f55ff6cf 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -582,7 +582,7 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,
>  						  struct pack_info *info,
>  						  uint32_t nr_packs,
>  						  uint32_t *nr_objects,
> -						  uint32_t preferred_pack)
> +						  int preferred_pack)

Why this change?

>  {
>  	uint32_t cur_fanout, cur_pack, cur_object;
>  	uint32_t alloc_fanout, alloc_objects, total_objects = 0;
> @@ -869,6 +869,7 @@ 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.preferred_pack_idx = -1;
>  	if (preferred_pack_name) {
>  		for (i = 0; i < ctx.nr; i++) {
>  			if (!cmp_idx_or_pack_name(preferred_pack_name,
> @@ -877,8 +878,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *
>  				break;
>  			}
>  		}
> -	} else
> -		ctx.preferred_pack_idx = -1;
> +	}
> 
>  	ctx.entries = get_sorted_entries(ctx.m, ctx.info, ctx.nr, &ctx.entries_nr,
>  					 ctx.preferred_pack_idx);
> @@ -942,28 +942,21 @@ 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;
> +	/* Check that the preferred pack wasn't expired (if given). */
>  	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)
> +		int preferred_idx = lookup_idx_or_pack_name(ctx.info,
> +							    ctx.nr,
> +							    preferred_pack_name);
> +		if (preferred_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 orig = ctx.info[preferred_idx].orig_pack_int_id;
>  			uint32_t perm = ctx.pack_perm[orig];
> 
> -			if (perm == PACK_EXPIRED) {
> +			if (perm == PACK_EXPIRED)
>  				warning(_("preferred pack '%s' is expired"),
>  					preferred_pack_name);
> -				ctx.preferred_pack_idx = -1;
> -			} else
> -				ctx.preferred_pack_idx = perm;
>  		}
>  	}

The rest makes sense.
>