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

[Taylor Blau <me@ttaylorr.com>]

On Fri, Jun 26, 2026 at 02:28:18PM -0700, Junio C Hamano wrote:
> 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.

I think that this would work nicely. I think that the skip_prefix()
variant is easy enough to read, and is clearly more efficient.

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

Yeah, this is gross (and my fault). Presumably I was swapping out one
variant for another and didn't stage the removal of one of the
strip_suffix() calls.

Thanks,
Taylor