Skip to content

Run gix-odb tests under SHA-256#2653

Merged
Sebastian Thiel (Byron) merged 6 commits into
GitoxideLabs:mainfrom
10ne1:dev/aratiu/sha256-odb
Jun 21, 2026
Merged

Run gix-odb tests under SHA-256#2653
Sebastian Thiel (Byron) merged 6 commits into
GitoxideLabs:mainfrom
10ne1:dev/aratiu/sha256-odb

Conversation

@10ne1

Copy link
Copy Markdown
Contributor

What

gix-odb handles SHA-256 fine, but its tests opened fixtures with the SHA-1-defaulting gix_odb::at(), so the SHA-256 paths went untested, so we make the dynamic-store, memory and loose-store tests hash-aware and we add the SHA-256 test runs to the CI.

gix-odb was the last crate in the object-storage stack without these runs.

Part of #281.

Testing

Full gix-odb suite passes under both hashes, including the parallel-gated test:

GIX_TEST_FIXTURE_HASH=sha1   cargo nextest run -p gix-odb --features parallel
GIX_TEST_FIXTURE_HASH=sha256 cargo nextest run -p gix-odb --features parallel

AI disclosure

This PR was written with the help of Claude Code, but was not vibe-coded. I'm not a fan of vibe-coding. I used AI to better understand the codebase myself (asked it a lot of questions), review both my code and the code generated by AI, used it especially to detect bugs and corner-cases and suggest fixes, however I very carefully reviewed & edited each code/comment lines, the commit messages and so on, until I was satisfied with the result.

Copilot AI review requested due to automatic review settings June 18, 2026 21:08

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR extends gix-odb’s test coverage to run against SHA-256 fixtures (in addition to SHA-1), ensuring hash-kind-dependent object database paths are exercised and adding corresponding CI invocations via the justfile.

Changes:

  • Make gix-odb tests hash-aware by selecting object IDs and store initialization options based on GIX_TEST_FIXTURE_HASH.
  • Add SHA-1/SHA-256 gix-odb test runs (including the parallel-gated regression test under SHA-256) to the unit-tests just recipe.
  • Track new SHA-256 scripted-fixture archives in the gix-odb fixtures .gitignore.

Reviewed changes

Copilot reviewed 7 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
justfile Runs gix-odb test suite under both GIX_TEST_FIXTURE_HASH=sha1 and sha256, including parallel under SHA-256.
gix-odb/tests/odb/main.rs Adds helpers to open stores with the selected fixture hash and choose hash-appropriate object IDs.
gix-odb/tests/odb/store/dynamic.rs Uses hash-aware store opening and hash-length-correct “missing” IDs; updates content-derived IDs via helper.
gix-odb/tests/odb/memory.rs Adjusts expected object IDs and tree size expectations to account for SHA-256 hash length.
gix-odb/tests/odb/store/loose.rs Uses the selected fixture hash when comparing permissions against Git-produced objects.
gix-odb/tests/odb/regression.rs Opens the scripted multi-index fixture with hash-aware store options; minor assertion formatting update.
gix-odb/tests/fixtures/generated-archives/.gitignore Ignores newly generated _sha256 scripted-fixture archives.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread gix-odb/tests/odb/main.rs

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Comment thread gix-odb/tests/fixtures/generated-archives/.gitignore
Adrian Ratiu (10ne1) and others added 5 commits June 21, 2026 10:35
The dynamic store tests opened scripted fixtures with `gix_odb::at()`
which defaults the store object hash to SHA-1.

Against a SHA-256 fixture, the store rejected the multi-pack-index and
misparsed the pack indices (iterating zero objects) and hard-coded SHA-1
object ids were fed to a SHA-256 store.

Add some shared test helpers and use them in the dynamic-store tests:
- fixture_options() and odb_at() open a store configured for the
  selected fixture hash
- id_for_hash() selects content-derived ids that differ between SHA-1
  and SHA-256 and `missing_id()` sizes guaranteed-missing lookup ids
  from the handle's own hash

Commit the SHA-256 fixture archives whose SHA-1 siblings are tracked
(`make_replaced_history`, `make_repo_multi_index`) and gitignore the
host-specific ones whose siblings are gitignored.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
Signed-off-by: Adrian Ratiu <[email protected]>
Reuse the helpers added in the previous commit for the dynamic-store tests
to open the `repo_with_loose_objects` fixture with its own object hash and
derive the hash-dependent ids and the written tree's size.

The loose permission test also hashes with the fixture's object hash.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
Signed-off-by: Adrian Ratiu <[email protected]>
gix-odb was the only crate in the object storage stack without
GIX_TEST_FIXTURE_HASH=* runs in the dual-hash test target, which
is why the SHA-256 gaps in its tests went unnoticed.

Enable both the SHA-1 and SHA-256 runs alongside the existing
gix-pack and others.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
Signed-off-by: Adrian Ratiu <[email protected]>
Clippy's `uninlined_format_args` flagged the assert message in the
multi-threaded find regression test.

Capture the variables inline.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
Signed-off-by: Adrian Ratiu <[email protected]>
The multi-threaded regression test opened the scripted multi-index fixture
with the SHA-1-defaulting gix_odb::at(), so against a SHA-256 fixture the
store then rejected the multi-pack-index and iterated zero objects, the
threads asserted over nothing and the test passed without exercising the
concurrency path.

It is also `parallel`-gated and the dual-hash gix-odb runs use the default
features, so the test never ran under SHA-256 at all.

Thus we open the fixture with odb_at(), so the store adopts the fixture's
object hash and add a `--features parallel` SHA-256 run so the concurrency
path is actually covered.

The matching SHA-1 + parallel case is already covered by the existing
`--features parallel` run.

Commit the SHA-256 multi-index fixtures the new run generates, matching
their tracked SHA-1 siblings.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
Signed-off-by: Adrian Ratiu <[email protected]>
Copilot AI review requested due to automatic review settings June 21, 2026 02:35

@Byron Sebastian Thiel (Byron) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks a lot, much appreciated.

Please note that I rewrote your commits partially and decided to remove the big fixtures that already existed in similar sizes for SHA-1. Unfortunately there was no way around that except for taking you into another review round and this is something I will always try to avoid.

Nothing else changed though, as I put my review changes into a new commit accordingly.

Also, for some reason, I still couldn't push using the normal GitHub mechanism. So I accepted your invite to collaborate on your fork, which fortunately works :).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.

Comment thread gix-odb/tests/fixtures/generated-archives/.gitignore
Comment thread gix-odb/tests/fixtures/generated-archives/.gitignore
Comment thread gix-odb/tests/fixtures/generated-archives/.gitignore
@Byron
Sebastian Thiel (Byron) merged commit e68a251 into GitoxideLabs:main Jun 21, 2026
56 of 58 checks passed
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.

3 participants