On Mon, Nov 23, 2020 at 09:43:36PM -0500, Jeff King wrote: > On Sat, Nov 21, 2020 at 09:31:50PM -0500, Taylor Blau wrote: > > > > There was SZEDER's comment on that last patch in v2, where future > > > readers of that patch will have to wonder why it does s/256/270/ in a > > > test. I agree with SZEDER that the change should be mentioned in the > > > commit message, even if it's just "unfortunately, we have some magicness > > > here, plus we want to pass both with SHA-1 and SHA-256; turns out 270 > > > hits the problem we want to test for". > > > > Thanks for reviewing it, and noticing a couple of problems in the > > earlier patches, too. If folks are happy with the replacement that I > > sent [1], then I am too :-). > > > > I don't think that the "big" patch generated a ton of review on the > > list, but maybe that's OK. Peff, Stolee, and I all reviewed that patch > > extensively when deploying it at GitHub (where it has been running since > > late Summer). > > Hrm. I thought you were going to integrate the extra checks I suggested > for load_bitmap_entries_v1(). Which is looks like you did in patch 17. > After that, the s/256/270/ hack should not be necessary anymore (if it > is, then we should keep fixing more spots). Oops. I even wrote down a big "S/256/270" in my notebook after you and I talked last about it, and then promptly forgot about it before sending v2. In any case, I have all of that fixed up, as well as other comments and suggestions from review, all of which were very helpful. (Thanks everybody who took a look at this monstrously large series, and apologies in advance for more to come ;-)). I think I would like a little more clarification on the discussion in [1]. From my reading, Jonathan Tan wants comments about the algorithm in the code, but Stolee would rather rely on the commits, especially since the algorithm changes later on in the series relative to the patch that this is downthread from. Once we can reach a good decision there, I'll send a v3 (which currently lives in my fork[2]). > -Peff Thanks, Taylor [1]: https://lore.kernel.org/git/ea0c8c5d-6bc3-0dca-4fa1-fb461ed7ccb9@xxxxxxxxx/ [2]: https://github.com/ttaylorr/git/compare/tb/bitmap-build-fast-for-upstream
