Use GIX_TEST_FIXTURE_HASH for gix-index tests#2598
Use GIX_TEST_FIXTURE_HASH for gix-index tests#2598Christoph Rüßler (cruessler) wants to merge 4 commits into
GIX_TEST_FIXTURE_HASH for gix-index tests#2598Conversation
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.
| /// Convert a hexadecimal hash into its corresponding `ObjectId` or _panic_. | ||
| pub fn hex_to_id(hex: &str) -> ObjectId { | ||
| ObjectId::from_hex(hex.as_bytes()).expect("40 bytes hex") | ||
| match gix_testtools::object_hash_from_env().unwrap_or_default() { |
| ( | ||
| "496d6428b9cf92981dc9495211e6e1120fb6f2ba", | ||
| "5f6f307bcc469c02acba4f7da42d8d4defdda8209777fe732956f1e2fa0db3ff", | ||
| ), | ||
| ( | ||
| "4b825dc642cb6eb9a060e54bf8d69288fbee4904", | ||
| "6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321", | ||
| ), | ||
| ( | ||
| "72d53f787d86a932a25a8537cee236d81846a8f1", | ||
| "645f454faab2cb984a331efafb76345242aa1d2029dff13a63935a31752f05ee", | ||
| ), | ||
| ( | ||
| "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391", | ||
| "473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813", | ||
| ), |
| if gix_testtools::object_hash() != gix_hash::Kind::Sha1 { | ||
| return; | ||
| } |
| git read-tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904 ;; | ||
| sha256) | ||
| git read-tree 6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321 ;; | ||
| *) |
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".
| let object_hash = if needs_archive { | ||
| // Fixtures that need an archive contain hard-coded SHA-1 hashes. | ||
| gix_hash::Kind::Sha1 | ||
| } else { |
There was a problem hiding this comment.
Respect fixture hash when loading archived index files
When needs_archive is true, try_file() always decodes the index as SHA-1, even though scripted_fixture_read_only_needs_archive() selects hash-specific archives (for example v2_more_files_sha256.tar under GIX_TEST_FIXTURE_HASH=sha256). This makes SHA-256 archived fixtures unreadable or misinterpreted and is why SHA-256 coverage had to be skipped instead of exercised. The helper should pass gix_testtools::object_hash() (or derive from fixture type), not hardcode SHA-1 for all archived fixtures.
Useful? React with 👍 / 👎.
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.