Skip to content

Make sha1 optional in gix-hash#2332

Merged
Sebastian Thiel (Byron) merged 3 commits intoGitoxideLabs:mainfrom
cruessler:make-sha-1-optional
Jan 3, 2026
Merged

Make sha1 optional in gix-hash#2332
Sebastian Thiel (Byron) merged 3 commits intoGitoxideLabs:mainfrom
cruessler:make-sha-1-optional

Conversation

@cruessler
Copy link
Copy Markdown
Contributor

This PR makes the sha1 flag optional in gix-hash. It modifies justfile to run cargo check as well as cargo nextest with 3 configurations: with sha1 only, with sha256 only, with both sha1 and sha256.

It’s mostly done, but still opened as a draft, so I can wait for CI to be green before I’m going to mark it as ready.

I haven’t checked all of gix-hash’s comments yet, some might need minor changes in case they still reference a sha1-only world.

@cruessler
Copy link
Copy Markdown
Contributor Author

I just went through all instances of feature = "sha1" to check whether there was any context that we would need to update in comments or code, but I only found one TODO: that I was able to remove as the tests that it mentioned should be added were already there.

@cruessler Christoph Rüßler (cruessler) marked this pull request as ready for review January 3, 2026 11:23
Copy link
Copy Markdown
Member

@Byron Sebastian Thiel (Byron) left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome, and I say so after a full armchair review.
Ok, after writing this I had to check it out and compile the variations on gix-hash myself, but it turned out as good as CI implies :).

Actually, I tried this:

cargo test -p gix-hash --no-default-features

I thought this should trigger a compile error, but it doesn't because basically everything one depends on (like gix-testtools) will reference the gix-hash crate, which in turn enables the default hash sha1.

While I don't think this needs to be solved in this PR, I think now we'd have to push the hash selection all the way up to the gix crate 😅.

That would probably reveal a couple of (expected) issues with missing attributes in the gix-hash test suite, while actually fulfilling this PR's promise, which effectively is the ability to compile gix without sha1 support.

Does this follow-up sound interesting to you?

@cruessler
Copy link
Copy Markdown
Contributor Author

I’m not entirely sure I get what you mean. :-)

This is the error messages I get when trying to compile gix-hash:

branch-8 on  make-sha-1-optional is 📦 v0.49.0 via 🦀 v1.92.0
❯ cargo check --package gix-hash --no-default-features
    Checking gix-hash v0.21.1 (/home/christoph/worktrees/gitoxide/branch-8/gix-hash)
error: Please set either the `sha1` or the `sha256` feature flag
  --> gix-hash/src/lib.rs:13:1
   |
13 | compile_error!("Please set either the `sha1` or the `sha256` feature flag");
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[…]

Some errors have detailed explanations: E0004, E0308, E0665.
For more information about an error, try `rustc --explain E0004`.
warning: `gix-hash` (lib) generated 39 warnings
error: could not compile `gix-hash` (lib) due to 15 previous errors; 39 warnings emitted

Is there something missing?

Apart from that, I’m very much interested in moving hash selection to gix (and possibly other crates that need to be involved). 😄

@Byron Sebastian Thiel (Byron) merged commit 4150203 into GitoxideLabs:main Jan 3, 2026
28 checks passed
@Byron
Copy link
Copy Markdown
Member

I am talking about cargo test -p gix-hash --no-default-features, which doesn't fail.

And indeed, to fix it one would have to remove the default sha1 feature, and leave the selection to gix.
But… I realise this probably doesn't work as so many crates need access to a selected hash.
So gix-object for instance also needs to expose the hash selection, and those who need it, and… it's a nightmare.
Maybe one can let the tests for each consumer of gix-hash set its hash, which would make sense as eventually we have to add hash-specific tests and that needs a way to be controlled.

Then, one will probably get around exposing the hash toggle in every consumer of gix-hash.

@Byron Sebastian Thiel (Byron) mentioned this pull request Jan 5, 2026
23 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants