Re: [PATCH v2 08/15] midx: allow marking a pack as preferred
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Mar 2, 2021, 04:17 UTC
- Message-ID
- <20210302041753.4037658-1-jonathantanmy@google.com>
- In-Reply-To
- <223b89909416ec7c5505f9cedaa80bf86ecc7b2e.1614193703.git.me@ttaylorr.com>
Show 31 quoted lines
> @@ -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.
Show 15 quoted lines
> @@ -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?
Show 28 quoted lines
> @@ -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.