feat: add sha256 feature to gix-object
#2359
Conversation
The flag mostly enables `sha256` in `gix-hash`.
Byron
left a comment
Byron
left a comment
There was a problem hiding this comment.
Thanks for getting this started.
Even though gix-object is a great crate, it's also a bigger one and I wonder if we can start with something smaller.
I was thinking gix-commitgraph is a good place because it has some dependency on the hash for parsing the cache format, while being quite small overall.
Once we figure out how to test this there, we can apply that to other crates as well.
Lastly, I think we will have to go down the route of not having sha1 as default, maybe that's something to tackle first.
What's your thoughts on this?
gix-object/tests/object/encode.rs
Outdated
| #[test] | ||
| #[cfg(feature = "sha256")] | ||
| fn write_to_does_not_validate_sha256() { | ||
| let mut tree = gix_object::Tree::empty(); | ||
| tree.entries.push(tree::Entry { | ||
| mode: EntryKind::Blob.into(), | ||
| filename: "".into(), | ||
| oid: gix_hash::Kind::Sha256.null(), | ||
| }); | ||
| tree.entries.push(tree::Entry { | ||
| mode: EntryKind::Tree.into(), | ||
| filename: "something\nwith\newlines\n".into(), | ||
| oid: gix_hash::ObjectId::empty_tree(gix_hash::Kind::Sha256), | ||
| }); | ||
| tree.write_to(&mut std::io::sink()) | ||
| .expect("write succeeds, no validation is performed"); | ||
| } |
There was a problem hiding this comment.
Here I think it's time to think hard if we can work this out with loops instead. This effectively abstracts over gix_hash::Kind, and there can be a function producing the list of enabled hashes.
There was a problem hiding this comment.
What about adding something like gix_hash::Kind::all()?
There was a problem hiding this comment.
Yes, let's do that, returning a &'static [Kind] slice should be easiest.
There was a problem hiding this comment.
That looks good, and when looking at it, I realise that I am not looking forward to Git adding its next hash 😅.
| path = "./benches/edit_tree.rs" | ||
|
|
||
|
|
||
| [features] |
There was a problem hiding this comment.
I also realise that for docs.rs one can only set features that are defined in the crate itself, which means that we'd always have to repeat the hash features here (and enable one of them for docs.rs) so docs don't break.
On the bright side, this automatically sets everything up to for a non-default SHA1 feature which should follow soon. Maybe it can be implemented so that we also enable at least one hash with a RUSTFLAG, that way CI can easily be tuned to not fail, and to no need adjustments for each and every invocation… heck, this flag could even be set in .cargo/config.toml in this repository.
It feels a bit like I should set that up, but what matters most as that you can keep going exactly like this.
I have also update the tasks here: #281 (comment) - sorry the fuzzyness, the groundworks I should just do in this case.
Previously written
When seeing this, I wonder if we really should go down that route. Albeit having written about it, i.e. having each consumer of gix-hash provide a sha1/sha256 feature, I also remember now that for this very case we have gix-features.
The idea is that at most, consumers pull that in and use it to configure certain basics. gix-hash technically is such a basic now.
Thus we could claim that without hash configuration, which really can only happen in plumbing, there will be a helpful compiler error guiding users to pulling in gix-hash to set up its hash support.
If you don't feel strongly about that, then let's try this? If it's unclear what to do, I could also make the change here, please let me know.
There was a problem hiding this comment.
I’m not sure I fully understand the relationship between gix-features and feature flags just yet, but regardless, if you want to do some or most parts of this yourself, that’s completely fine with me! You know better what the strategic goals are while I’ve been approaching these tasks more from a tactical point of view so far. The only thing I’d ask for is advance notice on relevant PRs, so I can avoid doing unnecessary work. 😄
There was a problem hiding this comment.
Yes, I'd also want to avoid waste. To remove some of the insecurity I stirred up here, I think gix-features isn't involved as each downstream crate must have their own feature toggles for hashes. The only reason for this is docs.rs which now must also set the hash (once there is no default hash), and can only do so for the crate it generates docs for.
|
|
||
| [features] | ||
| ## Support for SHA256 hashes and digests. | ||
| sha256 = ["gix-hash/sha256"] |
There was a problem hiding this comment.
I also realise that the test has to enable all hashes itself, it can do so via via its own crate (we need these on the crate so docs.rs can work).gix-hash or
Also, I feel like I will go hands-on with this soon as this PR may very well set up how we will do it for a ton of other crates, so we should be sure it's the 'right' path 😅.
2 participants
The flag mostly enables
sha256ingix-hash.