Use GIX_TEST_FIXTURE_HASH for gix-index tests#2598
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends gix-index's test suite to run under GIX_TEST_FIXTURE_HASH=sha256 in addition to sha1, as part of the broader SHA-256 transition tracked in issue #281. Generated fixtures are re-built through gix_testtools (which honors the env var via GIT_DEFAULT_HASH), while Loose fixtures (which embed hard-coded SHA-1 bytes) are kept on SHA-1; tests that depend on hash-specific assertions are gated to SHA-1 only.
Changes:
- Plumb
gix_testtools::object_hash()(instead of unconditionalgix_hash::Kind::Sha1) through thegix-indextest helpers (Fixture::open,odb_at,hex_to_id, directgix_index::File::at/State::from_bytescallers), with a small SHA-1→SHA-256 hex translation map for fixture-specific hashes. - Skip tests that rely on un-regeneratable hard-coded SHA-1 hashes when running under SHA-256 (early
returnorcontinueonLoosefixtures and_needs_archivepaths). - Update the
v2_empty.shfixture to pick the empty-tree hash byGIX_TEST_FIXTURE_HASH, and add the SHA-1+SHA-256gix-indexinvocations injustfile. Also a drive-by switch ingix-traverse/tests/traverse/util.rs::odb_atfromobject_hash_from_env().unwrap_or_default()toobject_hash().
Reviewed changes
Copilot reviewed 8 out of 26 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| justfile | Run gix-index tests under both sha1 and sha256 fixture hashes. |
| gix-traverse/tests/traverse/util.rs | Use the equivalent object_hash() helper. |
| gix-index/tests/index/main.rs | Hash-aware hex_to_id, new SHA-1↔SHA-256 hash map, new odb_at, and per-variant hash selection in Fixture::open. |
| gix-index/tests/index/init.rs | Use the hash-aware odb_at/object_hash() instead of hard-coded SHA-1. |
| gix-index/tests/index/file/write.rs | Pass object_hash() to State::from_bytes; skip Loose fixtures under SHA-256. |
| gix-index/tests/index/file/read.rs | Pass object_hash() to gix_index::File::at; early-return for SHA-1-only _needs_archive tests. |
| gix-index/tests/index/file/init.rs | Use object_hash(); skip Loose fixtures under SHA-256. |
| gix-index/tests/fixtures/make_index/v2_empty.sh | Pick the empty-tree id based on GIX_TEST_FIXTURE_HASH. |
Comments suppressed due to low confidence (3)
gix-index/tests/index/main.rs:50
- Matching on
gix_hash::Kindwith a wildcard arm that callsunimplemented!()defeats Rust's exhaustiveness checking: if a newKindvariant is added in the future, this helper will silently compile and only fail at runtime. Consider matching the variants exhaustively (without a wildcard) so the compiler flags missing cases.
match gix_testtools::object_hash_from_env().unwrap_or_default() {
gix_hash::Kind::Sha1 => ObjectId::from_hex(hex.as_bytes()).expect("40 bytes hex"),
gix_hash::Kind::Sha256 => ObjectId::from_hex(
SHA1_TO_SHA256_HASHES
.get(hex)
.unwrap_or_else(|| panic!("SHA-1 {hex} wasn't mapped to SHA-256 yet"))
.as_bytes(),
)
.expect("64 bytes hex"),
_ => unimplemented!(),
}
gix-index/tests/index/main.rs:63
- There is significant duplication between this
odb_athelper and the one updated ingix-traverse/tests/traverse/util.rsin this same PR (and there is a similar pattern in other test crates). Consider extracting a shared helper intogix-testtoolsso that all test crates can use one canonical implementation that respectsGIX_TEST_FIXTURE_HASH. This is optional, but it would reduce drift as more crates gain SHA-256 test coverage.
pub fn odb_at(objects_dir: impl Into<PathBuf>) -> Result<gix_odb::Handle> {
Ok(gix_odb::at_opts(
objects_dir,
Vec::new(),
gix_odb::store::init::Options {
object_hash: gix_testtools::object_hash(),
..Default::default()
},
)?)
}
gix-index/tests/index/main.rs:141
- In
Fixture::open(), theLoosearm always hard-codesgix_hash::Kind::Sha1regardless ofGIX_TEST_FIXTURE_HASH. Several test sites (e.g. ingix-index/tests/index/access.rslikedirwalk_api_and_icase_support,entry_by_path_with_conflicting_file,remove_entries, etc.) callFixture::Loose(...).open()without the SHA-1-only guard that was added infile/write.rsandfile/init.rs. UnderGIX_TEST_FIXTURE_HASH=sha256, these tests will still run and exercise a SHA-1File— which is what makes them pass, but it also means the test suite's SHA-256 mode silently degrades coverage of these code paths. It's worth at least leaving a comment nearFixture::open()documenting this intentional behavior so future readers don't try to "fix" it.
pub fn open(&self) -> gix_index::File {
let object_hash = match self {
// Generated fixtures are generated by `gix_testtools`, respecting
// `GIX_TEST_FIXTURE_HASH`.
Fixture::Generated(_) => gix_testtools::object_hash(),
// Loose fixtures are not generated by `gix_testtools`. They contain hard-coded SHA-1
// hashes.
Fixture::Loose(_) => gix_hash::Kind::Sha1,
};
gix_index::File::at(self.to_path(), object_hash, false, Default::default())
.expect("fixtures are always readable")
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d56ceb732
ℹ️ 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".
2d56ceb to
6ab2993
Compare
3c35a3c to
e536f7f
Compare
e536f7f to
bc4064c
Compare
Sebastian Thiel (Byron)
left a comment
There was a problem hiding this comment.
Thanks a lot for your continued efforts, and sorry for the long wait!
I left a couple of notes that you might find helpful for future work.
b5b2d54
into
GitoxideLabs:main
This PR makes
gix-indextests run withGIX_TEST_FIXTURE_HASH=sha256.From what I can tell, the error on CI is not related to changes in this PR, so I’m going to mark it as ready for review.
From what I understand, there’s a couple of fixtures in
gix-indexthat contain hard-coded SHA-1 hashes and can’t be generated. In particular, there’senum Fixturethat has two variants,GeneratedandLoose. My changes assume that onlyGeneratedfixtures can be created for SHA-256, so SHA-256 tests are skipped forLoosefixtures.This is part of #281.