Re: [RFC PATCH 04/10] repack: teach MIDX retention about geometric rollups

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

Taylor Blau <me@ttaylorr.com> writes:

> +static int pack_geometry_contains_pack(struct packed_git **packs,
> +				       uint32_t packs_nr,
> +				       const char *base)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	uint32_t i;
> +
> +	for (i = 0; i < packs_nr; i++) {
> +		strbuf_reset(&buf);
> +		strbuf_addstr(&buf, pack_basename(packs[i]));
> +		strbuf_strip_suffix(&buf, ".pack");
> +
> +		if (!strcmp(buf.buf, base)) {
> +			strbuf_release(&buf);
> +			return 1;
> +		}
> +	}
> +
> +	strbuf_release(&buf);
> +	return 0;
> +}

It feels slightly inefficient to repeatedly strbuf_reset(),
strbuf_addstr(), and strbuf_strip_suffix() in the loop.  I do not
know if my understanding of what existing_packs_retain_midx_packs()
passes down in buf.buf as base is correct or not, but if so,
wouldn't it equivalent to

	for (uint32_t i = 0; i < packs_nr; i++) {
                const char *pack_name = pack_basename(packs[i]);
                const char *suffix;

                if (skip_prefix(pack_name, base, &suffix) &&
                    !strcmp(suffix, ".pack"))
                        return 1;
	}

perhaps?

Starting from "/path/to/objects/pack/pack-deadbeef.pack", you take
the basename of it to have "pack-deadbeef.pack" in buf, strip out
the ".pack" suffix to get "pack-deadbeef" in buf and then compare it
with the base.

Instead, pack_name in the rewitten one becomes the basename of the
packfile path, i.e., "pack-deadbeef.pack", then we see if it begins
with base and take the remainder in suffix, and finally we check if
that remaining suffix is ".pack".

Which should be equivalent.

> + * freshly-written pack supersedes them. When doing a geometric repack,
> + * packs below the split are rewritten into the new MIDX tip and should
> + * remain eligible for deletion.
>   */
> -void existing_packs_retain_midx_packs(struct existing_packs *existing)
> +void existing_packs_retain_midx_packs(struct existing_packs *existing,
> +				      const struct pack_geometry *geometry)
>  {
>  	struct string_list_item *item;
>  	struct strbuf buf = STRBUF_INIT;
> @@ -315,6 +351,9 @@ void existing_packs_retain_midx_packs(struct existing_packs *existing)
>  		strbuf_strip_suffix(&buf, ".pack");
>  		strbuf_strip_suffix(&buf, ".idx");

Not a fault of this patch, but it makes the hairs on the back of my
head tingle to see that a bogus input like "pack-foobar.idx.pack"
happily is taken, while "pack-foobar.pack.idx", an equally bogus
input, is not.

> +		if (pack_geometry_contains_rollup(geometry, buf.buf))
> +			continue;