Add ObjectId::Sha256 and Kind::Sha256

[cruessler]

This is a draft that adds ObjectId::Sha256 and Kind::Sha256 to gix-hash. I’m opening this to get early feedback, in particular from a CI run.

Some remarks:

• It feels wrong to put empty_blob_sha256 and empty_tree_sha256 into oid as the comment says oid is related to Sha1, but I’m unsure how to best resolve this tension.
• There’s inconsistencies with respect to spelling: SHA256, SHA-256, Sha256.
• ObjectId::null_sha1 hardcodes 20. Should this be SIZE_OF_SHA1_DIGEST? oid::null_sha1 uses SIZE_OF_SHA1_DIGEST.
• Kind::len_in_hex and Kind::len_in_bytes also hardcode values.
• Hardcoded empty trees and empty blobs appear in more than place (for both SHA-1 and SHA-256). I feel like we should have a single source of truth here.
• Some comments in ObjectId would probably benefit from being rephrased. See e. g. ObjectId::new_sha1 or ObjectId::from_20_bytes.

Sources for the hardcoded values related to SHA-256:

https://github.com/git/git/blob/e85ae279b0d58edc2f4c3fd5ac391b51e1223985/hash.h
https://github.com/git/git/blob/e85ae279b0d58edc2f4c3fd5ac391b51e1223985/hash.c

[Byron]

You are on fire :)! Let me share some early thoughts, without having looked at the code at all.

• It feels wrong to put empty_blob_sha256 and empty_tree_sha256 into oid as the comment says oid is related to Sha1, but I’m unsure how to best resolve this tension.

And indeed, &oid is supposed to represent any hash. if empty_blob somehow is hardcoded to always be SHA1, it's a bug and it needs to take a gix-hash::Kind. The empty_something could also be implemented on gix-hash::Kind while at it.

• There’s inconsistencies with respect to spelling: SHA256, SHA-256, Sha256.

Should we fix them in favor of what Git seems to do: SHA256?

I am definitely open to your suggestions.

• ObjectId::null_sha1 hardcodes 20. Should this be SIZE_OF_SHA1_DIGEST? oid::null_sha1 uses SIZE_OF_SHA1_DIGEST.

Yes, that sounds like an oversight.

• Kind::len_in_hex and Kind::len_in_bytes also hardcode values.

Yes, let's use constants where we have them. And since there is already a precedent with SIZE_OF_SHA1_DIGEST we might also have the respective SHA256 versions and use them.

• Hardcoded empty trees and empty blobs appear in more than place (for both SHA-1 and SHA-256). I feel like we should have a single source of truth here.

I am open to suggestions as to where the empty-* methods should be, but would also not lightheartedly introduce breaking changes. If the new way feels better, we should have it. By now, I always think in terms of gix-hash::Kind::null()/empty_*() by the way, which aligns well with repo.object_hash().…()

• Some comments in ObjectId would probably benefit from being rephrased. See e. g. ObjectId::new_sha1 or ObjectId::from_20_bytes.

Yes, it's a good time to unify this and make it less 'hardcoded'.

Thanks again 🙏

[Byron]

It seems it's totally possible to start a new review per commit, sorry for that.
Generally, I think this kind of 'manual' testing is perfectly fine. Ideally we reach the same test-coverage for SHA256 as for SHA1, while maybe even covering more ground or doing some cleanup. This crate is one of the oldest, and it probably shows.

[Byron]

It would be interesting to see inequality between hashes of different kinds. Of course we know this will always be true, but… it's to be sure? Maybe make both have the same prefix, so an incorrect implementation could get it wrong.

[cruessler]

Makes total sense! Are you suggesting a test such as the following one?

#[test]
#[cfg(all(feature = "sha1", feature = "sha256"))]
fn it_detects_inequality_sha1_and_sha256() {
    let prefix_sha1 = gix_hash::Prefix::new(&hex_to_id("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"), 7).unwrap();
    let prefix_sha256 = gix_hash::Prefix::new(
        &hex_to_id("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"),
        7,
    )
    .unwrap();
    assert_eq!(prefix_sha256.cmp(&prefix_sha1), Ordering::Greater);
    assert_eq!(prefix_sha1.to_string(), "bbbbbbb");
    assert_eq!(prefix_sha256.to_string(), "aaaaaaa");
}

Prefix has #[derive(Ord, PartialOrd)], so this is what the default implementation gives us.

[Byron]

Yes, that would be it.
We'd probably also want to ensure we have a test that turns both on at the same time then.

[cruessler]

I’m not entirely sure I understand what you mean by “a test that turns both on at the same time”. Can you elaborate? :-)

[Byron]

Sorry, I meant a test that runs if both feature toggles are enabled so both hashes are available.

[Byron]

And my second comment, this should have been part of the 'review' above.