Conversation
|
git-add? |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors base encoding functionality by extracting Base16/Base32/Base64 implementations from the hash module into dedicated base-n modules. The goal is to make the hash code less cluttered and create more reusable encoding utilities that can be used independently of hashing.
Key changes include:
- Extracted base64 encoding/decoding from
util.ccinto newbase-n.ccmodule - Moved base16 encoding logic from hash module to
base-n.cc - Enhanced base-nix-32 module with decode functionality
- Updated all call sites to use new namespaced functions (
base64::encode,base16::encode, etc.)
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libutil/util.cc | Removed base64 encoding functions (moved to base-n.cc) |
| src/libutil/base-n.cc | New file containing base16 and base64 encoding/decoding implementations |
| src/libutil/include/nix/util/base-n.hh | New header defining base16 and base64 namespaces and function signatures |
| src/libutil/include/nix/util/array-from-string-literal.hh | New utility for converting string literals to arrays without null terminator |
| src/libutil/base-nix-32.cc | Added decode function and updated to use std::byte |
| src/libutil/include/nix/util/base-nix-32.hh | Updated signatures and added decode function declaration |
| src/libutil/hash.cc | Refactored to use new base-n functions and simplified hash parsing logic |
| src/libutil/include/nix/util/hash.hh | Removed base encoding length calculation methods |
| Multiple files | Updated includes and function calls to use new base-n namespaces |
bcb3f3b to
51bb429
Compare
|
|
||
| TEST(base64Decode, decodeThrowsOnInvalidChar) | ||
| { | ||
| ASSERT_THROW(base64::decode("cXVvZCBlcm_0IGRlbW9uc3RyYW5kdW0="), Error); |
There was a problem hiding this comment.
Can you also add a test where there are extra trailing padding = characters at the end? Both snix/lix ran into the fact that parser isn't entirely correct in that regard and nixpkgs relies on some of the broken behaviour.
There was a problem hiding this comment.
Here's what I'm talking about: https://git.snix.dev/snix/snix/src/commit/2a29b90c7f3f3c52b5bdae50260fb0bd903c6b38/snix/nix-compat/src/nixhash/mod.rs#L431
2e88419 to
0b56d79
Compare
|
OK I think this is ready. |
0b56d79 to
a14bfbe
Compare
a14bfbe to
27d2124
Compare
Make it separate from Hash, since other things can be base-encoded too. This isn't really needed for Nix, but it makes the code easier to read e.g. for someone reimplementing this stuff in a different language. (Of course, Base16/Base64 should be gotten off-the-shelf, but now the hash code, which is more bespoke, is less cluttered with the parts that would be from some library.) Many reimplementations of "Nix32" and our hash type already exist, so this cleanup is coming years too late, but I say better late than never / it is always good to nudge the code in the direction of being a "living spec". Co-authored-by: Sergei Zimmerman <sergei@zimmerman.foo>
27d2124 to
9918312
Compare
|
OK, think this is ready now? |
xokdvium
left a comment
There was a problem hiding this comment.
Reviewed with --color-moved. Most of the diff is just moving code around.
Motivation
Make it separate from Hash, since other things can be base-encoded too.
This isn't really needed for Nix, but it makes the code easier to read e.g. for someone reimplementing this stuff in a different language. (Of course, Base16/Base64 should be gotten off-the-shelf, but now the hash code, which is more bespoke, is less cluttered with the parts that would be from some library.)
Many reimplementations of "Nix32" and our hash type already exist, so this cleanup is coming years too late, but I say better late than never / it is always good to nudge the code in the direction of being a "living spec".
Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.