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
