From: Taylor Blau <me@ttaylorr.com>
Date: Fri, 09 Jan 2026 23:40:06 GMT
Subject: Re: [PATCH 4/5] repack-promisor: extract function to remove redundant packs
Message-ID: <aWGR1r5PlLL3rWWd@nand.local>
In-Reply-To: <20260105-pks-geometric-repack-with-promisors-v1-4-c4660573437e@pks.im>

On Mon, Jan 05, 2026 at 02:16:44PM +0100, Patrick Steinhardt wrote:
> @@ -226,6 +227,15 @@ void pack_geometry_remove_redundant(struct pack_geometry *geometry,
>  	strbuf_release(&buf);
>  }
>
> +void pack_geometry_remove_redundant(struct pack_geometry *geometry,
> +				    struct string_list *names,
> +				    struct existing_packs *existing,
> +				    const char *packdir)
> +{
> +	remove_redundant_packs(geometry->pack, geometry->split,
> +			       names, existing, packdir);
> +}
> +

The refactoring up to this point looks all good to me.

As a side-note, I would love to get rid of the
pack_geometry_remove_redundant() function altogether. It is kind of a
hack that we handle determining which packs are made redundant by a
repacking operation in two different ways depending on whether or not we
are doing a geometric repack.

I have some patches to do this in a series that implements the
"reachability-guided" geometric repacking technique that I have talked
above[^1] previously. I think (having skimmed the next patch but not yet
fully reviewed it) that this should still all be doable with your
patches. We just have to take two passes (once through the existing
non-promisor packs and then another pass through the promisor
ones).

So I think that this all looks good to me and shouldn't interfere with
that effort, though I'll make a note to rebase those patches on top of
these to make it easier for the maintainer to queue both of them.

Thanks,
Taylor

[^1]: Well, I was pretty sure that I had mentioned it on the list, but
  can't seem to find anything corresponding to it in my "sent" folder.
  In case I haven't talked above it before, the gist is a special mode
  of --stdin-packs that only packs objects from the included set of
  packs which are reachable. If repack generates a cruft pack after the
  fact, that allows us to "incrementally" build up the cruft pack over
  time.