Implement support for Git hashing with SHA-256#13543
Conversation
github-actions
bot
added
with-tests
Ericson2314
force-pushed
the
git-256
branch
2 times, most recently
from
aea27b8 to
7d8596b
Compare
roberth
left a comment
roberth
left a comment
There was a problem hiding this comment.
LGTM, but note xokdvium's nitpick
| mkSinkHook(CanonPath::root, root.hash, BlobMode::Regular); | ||
|
|
||
| ASSERT_EQ(files->root, files2->root); | ||
| EXPECT_EQ(files->root, files2->root); |
There was a problem hiding this comment.
Oh ASSERT will end the test if it fails, while EXPECT will register a failure but keep on going. The two loops are independent (really should be two separate tests, but I didn't want more macros), so EXPECT is better in this case.
SHA-256 is Git's next hash algorithm. The world is still basically stuck on SHA-1 with git, but shouldn't be. We can at least do our part to get ready. On the C++ implementation side, only a little bit of generalization was needed, and that was fairly straight-forward. The tests (unit and system) were actually bigger, and care was taken to make sure they were all cover both algorithms equally.
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
| unreachable(); | ||
| } | ||
|
|
||
| const StringSet hashAlgorithms = {"blake3", "md5", "sha1", "sha256", "sha512"}; |
There was a problem hiding this comment.
Maybe the supported git hash algorithms could be defined as subset of hashAlgorithm so it can be used in for example src/libutil-tests/git.cc line 264 and src/libutil/git.cc line 145
This would also make the parameters in src/libutil/include/nix/util/git.hh safer
There was a problem hiding this comment.
I think I understand what you are saying, and it makes sense to me. (Enum > string, for sure.) Would you like to make a PR for this? :)
There was a problem hiding this comment.
I think I understand what you are saying, and it makes sense to me. (Enum > string, for sure.) Would you like to make a PR for this? :)
looks like a good point to work myself into the codebase. Hope I don't forget about it when I find the time
5 participants
Motivation
SHA-256 is Git's next hash algorithm. The world is still basically stuck on SHA-1 with git, but shouldn't be. We can at least do our part to get ready.
On the C++ implementation side, only a little bit of generalization was needed, and that was fairly straight-forward. The tests (unit and system) were actually bigger, and care was taken to make sure they were all cover both algorithms equally.
Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.