On Thu, Sep 05, 2024 at 01:04:34PM -0400, Taylor Blau wrote: > On Thu, Sep 05, 2024 at 09:51:00AM -0700, Junio C Hamano wrote: > > Taylor Blau <me@xxxxxxxxxxxx> writes: > > > > > If so, I agree, but would note that this series does not yet switch > > > index-pack to use the non-collision detecting SHA-1 implementation when > > > available, so that would not be a prerequisite for merging this series. > > > > Hmph, I am confused. It needs to be corrected in order to address > > collisions of the tail sum Peff raised, as no longer checked the > > tail sum with SHA1DC but with "fast" SHA-1. > > Peff's mail supposes that we have already modified index-pack to use the > non-collision detecting SHA-1 implementation. But this series does not > do that, so I don't think we have an issue to address here. Yes, this is correct. You are modifying only the writing side (which of course would be under attacker control in this scenario anyway). So we are only getting the benefit there, but without any additional threat on the reading side. But I'm not sure how comfortable I am leaving us in that state, even if it is by itself OK. It feels fragile, and we're a small step away from somebody accidentally using the "fast" variant to do reading. If it's possible to fix the overwrite issue without too much code (and I think it probably is), then that leaves us in a much safer spot, even with what's in your series. And the fact that it lets us speed up the reading side _and_ opens the door to possible alternate-hash improvements is the cherry on top. Side note: I do really like the xxHash direction in general, but I think we may be underestimating the difficulty. Obviously it needs a protocol extension for sending packfiles, but what about other cross-repo access? E.g., dumb http blindly downloads the packfiles. How does it learn which hash is in use? -Peff
