Add hex_to_id_sha1_only, use in SHA-1 only tests#2595
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refines SHA-1 vs SHA-256 handling in the gix integration tests by separating “hash mapping aware” ID conversion from “SHA-1 literal” ID conversion, preparing gix/tests for stricter SHA-256 fixture runs.
Changes:
- Tighten
hex_to_id()so SHA-256 fixture mode requires a SHA-1→SHA-256 mapping entry (no silent fallback). - Add
hex_to_id_sha1_only()for tests that intentionally hard-code SHA-1 object IDs. - Update SHA-1-only revision-spec and repository tests to use
hex_to_id_sha1_only().
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| gix/tests/gix/util.rs | Makes hex_to_id() strict in SHA-256 mode and adds hex_to_id_sha1_only() for SHA-1-only tests. |
| gix/tests/gix/revision/spec/from_bytes/traverse.rs | Switches expected IDs to hex_to_id_sha1_only() in SHA-1-only traversal assertions. |
| gix/tests/gix/revision/spec/from_bytes/regex.rs | Uses hex_to_id_sha1_only() for SHA-1-only regex revspec expectations. |
| gix/tests/gix/revision/spec/from_bytes/reflog.rs | Uses hex_to_id_sha1_only() for reflog-based SHA-1-only expectations. |
| gix/tests/gix/revision/spec/from_bytes/peel.rs | Uses hex_to_id_sha1_only() for peel tests with hard-coded SHA-1 IDs. |
| gix/tests/gix/revision/spec/from_bytes/mod.rs | Updates module-level imports/usages to prefer hex_to_id_sha1_only() where appropriate. |
| gix/tests/gix/revision/spec/from_bytes/ambiguous.rs | Moves hard-coded SHA-1 expectations to hex_to_id_sha1_only(). |
| gix/tests/gix/repository/mod.rs | Updates SHA-1-only revision parsing expectations to hex_to_id_sha1_only(). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d80c534f85
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
Sebastian Thiel (Byron)
left a comment
There was a problem hiding this comment.
Thanks a lot, much appreciated!
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
gix/tests/gix/util.rs:173
hex_to_id_sha1_only()is documented/named as “SHA-1 only”, but it currently just forwards toObjectId::from_hex(), which will also accept 64-hex inputs and yield a SHA-256 id. If this helper is meant to enforce SHA-1, consider assertinghex.len() == 40(and adjusting the expect message to “40 hex chars” rather than “40 bytes”).
/// Convert a hexadecimal hash into its corresponding `ObjectId` or _panic_.
///
/// This does *not* take `GIX_TEST_FIXTURE_HASH` into account, so it does not map SHA-1 hashes to
/// SHA-256 hashes. Use this for tests that don't need a mapping.
pub fn hex_to_id_sha1_only(hex: &str) -> gix_hash::ObjectId {
gix_hash::ObjectId::from_hex(hex.as_bytes()).expect("40 bytes hex")
}
This is a follow-up to #2578. There’s some tests in
gix/teststhat are SHA-1 only in that they contain hard-coded SHA-1 hashes and don’t have a corresponding version for SHA-256 hashes. They still share the samehex_to_idfunction that returns a SHA-1 or SHA-256 hash, based ongix_testtools::object_hash. The behaviour ofhex_to_iddiverges from other instances ofhex_to_idin other crates: ingix/tests,hex_to_idsilently returns the SHA-1 has that was passed in when there’s no SHA-256 hash in the mapping while in other crates it is a hard error when the SHA-1 hash can’t be found in the mapping. This PR introduces a separatehex_to_id_sha1_onlythat keeps the fallback behaviour, so thathex_to_idcan be changed to mirrorhex_to_idin other crates.