Jonathan Nieder <jrnie...@gmail.com> writes: > Sorry for the slow review. I finally got a chance to look this over > again. > > My main nits are about the commit message: I think it still focuses > too much on the process instead of the usual "knowing what I know now, > here's the clearest explanation for why we need this patch" approach. > I can send a patch illustrating what I mean tomorrow morning.
I'll turn 'Will merge to next' to 'Hold' for now. >> Object format >> ~~~~~~~~~~~~~ >> The content as a byte sequence of a tag, commit, or tree object named >> -by sha1 and newhash differ because an object named by newhash-name refers to >> +by sha1 and sha256 differ because an object named by sha256-name refers to > > Not about this patch: this should say SHA-1 and SHA-256, I think. > Leaving it as is in this patch as you did is the right thing. Perhaps deserves a "NEEDSWORK" comment, though. > [...] >> @@ -255,10 +252,10 @@ network byte order): >> up to and not including the table of CRC32 values. >> - Zero or more NUL bytes. >> - The trailer consists of the following: >> - - A copy of the 20-byte NewHash checksum at the end of the >> + - A copy of the 20-byte SHA-256 checksum at the end of the > > Not about this patch: a SHA-256 is 32 bytes. Leaving that for a > separate patch as you did is the right thing, though. > > [...] >> - - 20-byte NewHash checksum of all of the above. >> + - 20-byte SHA-256 checksum of all of the above. > > Likewise. Hmph, I've always assumed since NewHash plan was written that this part was not about tamper resistance but was about bit-flipping detection and it was deliberate to stick to 20-byte sum, truncating as necessary. It definitely is a good idea to leave it for a separate patch to update this part. Thanks.
