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.
On Fri, Aug 3, 2018 at 9:40 AM Junio C Hamano [off-list ref] wrote:
quoted
[...]
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.
Yeah, in fact, since this was one area where people actually had
performance issues with the hash, it might be worth considering
_weakening_ the hash.
Things like the pack index files (and just the regular file index) are
entirely local, and the SHA1 in those is really just a fancy CRC. It's
really just "good protection against disk corruption" (it happens),
and in fact you cannot use it as protection against active tampering,
since there's no secret there and any active attacker that has access
to your local filesystem could just recompute the hash anyway.
And because they are local anyway and aren't really transported
(modulo "shared repositories" where you use them across users or
legacy rsync-like synchronization), they can be handled separately
from any hashing changes. The pack and index file formats have in fact
been changed before.
It might make sense to either keep it as SHA1 (just to minimize any
changes) or if there are still issues with index file performance it
could even be made to use something fast-but-not-cryptographic like
just making it use crc32().
Remember: one of the original core git design requirements was
"corruption detection".
For normal hashed objects, that came naturally, and the normal object
store additionally has active tamper protection thanks to the
interconnected nature of the hashes and the distribution of the
objects.
But for things like packfiles and the file index, it is just a
separate checksum. There is no tamper protection anyway, since anybody
who can modify them directly can just recompute the hash checksum.
The fact that both of these things used SHA1 was more of a convenience
issue than anything else, and the pack/index file checksum is
fundamentally not cryptographic (but a cryptographic hash is obviously
by definition also a very good corruption detector).
Linus