From: Taylor Blau <me@ttaylorr.com>
Date: Fri, 10 Oct 2025 23:15:54 GMT
Subject: Re: [PATCH 00/49] repack: prepare for incremental MIDX-based repacking
Message-ID: <aOmTqt4m/R/YrOGg@nand.local>
In-Reply-To: <20251010062913.GJ1965904@coredump.intra.peff.net>

On Fri, Oct 10, 2025 at 02:29:13AM -0400, Jeff King wrote:
> On Sun, Sep 28, 2025 at 06:07:10PM -0400, Taylor Blau wrote:
>
> > This series moves the vast majority of the builtin's functionality into
> > several new compilation units, all grouped under a new repack.h API:
> >
> >  * repack.c
> >  * repack-cruft.c
> >  * repack-filtered.c
> >  * repack-geometry.c
> >  * repack-midx.c
> >  * repack-promisor.c
>
> I've read up through patch 37. My eyes were starting to glaze over at
> that point, and it looked like the patches were starting to get more
> interesting, so I've saved the rest for another session.

Thanks for the review thus far!

> From what I read, the changes all look reasonable. I brought up a few
> small naming and interface questions, but I don't think there's anything
> incorrect here. And there may be diminishing returns in polishing an API
> which is realistically only going to be called from one place.
>
> The split you have seems reasonable. It's not quite as clean as one
> might hope (e.g., repack's existing_packs gets passed around to some of
> the other subsystems). But again, diminishing returns on trying to
> untangle it further.

Yeah, I think we could probably go on for quite a bit longer polishing
the naming, interfaces, etc. I tried to strike a balance in this series
to change as little as possible while still ending up with a
halfway-decent looking repack.h ;-).

As you noted, the repack code is extremely fragile/brittle. That is a
big part of why I both (a) took such an incremental approach as in this
series and (b) tried to avoid making too many changes as I moved code
around.

I think that (b) especially leaves us with an opportunity to clean these
interfaces up after this series has landed, and I intend on doing so. My
hope is that this series gives us a solid foundation to make those
changes on, as opposed to further hacking up builtin/repack.c ;-).

Thanks,
Taylor