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

From: Taylor Blau <[email-masked]>
Date: Thu, 23 May 2024 16:42:03 -0400

On Thu, May 23, 2024 at 07:05:32AM -0400, Jeff King wrote:
> 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 ;-).

> 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