Re: [PATCH v2 2/2] doc hash-function-transition: pick SHA-256 as NewHash

Jonathan Nieder [off-list ref] 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.

quoted

 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.

[...]

quoted

@@ -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.

[...]

quoted

-  - 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.