On Thu, May 30, 2024 at 03:14:32AM -0400, Jeff King wrote: > On Wed, May 29, 2024 at 06:55:15PM -0400, Taylor Blau wrote: > > > This is a small reroll of my series which has a grab-bag of midx-write > > related cleanups that I pulled out of a larger series to implement > > incremental MIDX chains. > > These all look pretty reasonable to me. Thanks for breaking them off of > the larger series. I think it's generally nice to get things in smaller > chunks. Thanks for reviewing! It is much appreciated, as always :-). > Sometimes it is a little tough to evaluate refactorings without seeing > the larger context in which they'd be used. But all of these were either > immediate improvements, or didn't take much imagination to see where > they'd make later things easier. Yeah, I feel like this is something that I'm still trying to find the right balance with. I think with the pseudo-merge bitmaps series, I erred on the side of making the series too large, which made it difficult to easily review. Seeing the review process on that series made me a little uneasy about sending the incremental MIDX bitmaps series, which was growing similarly large. I think that seeing these 8 patches or so get sent in preparation makes the later series easier to review, but I agree that they are somewhat unsatisfying on their own. I think in this case it was a reasonable trade-off to make, but you could probably make a compelling case either way there ;-). On the bright side, what was once a ~30 patch series is now three series (of which this is the first one). The main series is now 13 or so patches, the end state of which is to have incremental MIDXs working without support for reachability bitmaps. The final series I'm planning in this area adds support for reachability bitmaps with incremental MIDXs, but that series is only half a dozen or so patches. I'm hoping that by sending it in chunks, the end-to-end review process will be more straightforward and approachable. If you have any other ideas to make it easier, definitely let me know! Thanks, Taylor
