Re: [PATCH v3 00/30] pack-bitmap: pseudo-merge reachability bitmaps

From: Taylor Blau <me@ttaylorr.com>
Date: May 23, 2024, 20:42 UTC
Message-ID: <Zk+qG4INyrqPL5SN@nand.local>
In-Reply-To: <20240523110532.GA1326297@coredump.intra.peff.net>

On Thu, May 23, 2024 at 07:05:32AM -0400, Jeff King wrote:

Show 7 quoted lines

> On Tue, May 21, 2024 at 03:01:38PM -0400, Taylor Blau wrote:
>
> > Here is another reroll my topic to introduce pseudo-merge bitmaps.
>
> OK, I got through the whole thing. I left a few small comments, but
> mostly just observations. Overall, the shape of it looks pretty good.
> The much bigger question to me is: does it work?

Thanks for working through this series, I appreciate you taking a close look, especially since it's on the longer end.

> The perf results you showed at the end are underwhelming, but I think
> that's mostly because it's not an interesting repository. I think it
> would be nice to see at least some point-in-time benchmarks on a
> single repository.

I'd say it's mostly because I misspelt "pseudo" as "psuedo", but either way ;-).

Show 20 quoted lines

> I know you don't have those answers yet, and I know there's some
> chicken-and-egg with getting this integrated so that you can start to
> explore that. So I mostly reviewed this with an eye towards:
>
>   - does the idea make sense (I think it does, but I'm kind of biased)
>
>   - are the patches going to hurt anybody who isn't using the new
>     feature (I think the answer is no)
>
>   - does the on-disk representation seem right, since that is hard to
>     change later (I didn't see any issues)
>
>   - does the implementation look clean and plausibly correct (yes, but
>     what I'm getting at is that I didn't pore over all of the new code
>     with a microscope. Mostly I think the proof is in the pudding that
>     it provides the same correct answers more quickly).
>
> So to my eyes it looks good to move forward and let people start playing
> with it. The big "experimental" warning in the config is good. Maybe
> we'd want want in gitpacking(7), too?

I think adding a matching warning in gitpacking(7) is a good idea. Your feeling matches my own here that the burden of this series on those not using pseudo-merge bitmaps is negligible, and that this unblocks those who want to move forward with testing this out in the wild on doing so.

I'll send a v4 shortly with the minor changes that I picked up from your last read-through, and then I think we are good to go here.

Thanks, Taylor