Cover gix-pack multi-index write under SHA-256#2655
Closed
Adrian Ratiu (10ne1) wants to merge 2 commits into
Closed
Cover gix-pack multi-index write under SHA-256#2655Adrian Ratiu (10ne1) wants to merge 2 commits into
Adrian Ratiu (10ne1) wants to merge 2 commits into
Conversation
The checksum-verification child progress was always labelled "Sha1 of index", even when verifying a SHA-256 index. The checksum itself already uses the index's own object hash; only the label was misleading. Co-Authored-By: Claude Opus 4.8 <[email protected]> Signed-off-by: Adrian Ratiu <[email protected]>
The existing multi-index write test feeds the static SHA-1 packs and checks pinned SHA-1 values, so writing a multi-index was never exercised with SHA-256 object ids. Add a companion test that builds a multi-index from the hash-parameterized fixture (via GIX_TEST_FIXTURE_HASH), so the writer now runs under both hashes. Co-Authored-By: Claude Opus 4.8 <[email protected]> Signed-off-by: Adrian Ratiu <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR closes a SHA-256 test-coverage gap in gix-pack by ensuring the multi-pack-index writer is exercised using hash-parameterized pack-index fixtures (so it runs under both SHA-1 and SHA-256 test modes), and it makes a small progress-label tweak to avoid SHA-1-specific wording.
Changes:
- Add a new multi-index writer test that sources its input index from the hash-parameterized scripted fixture, enabling SHA-256 coverage.
- Clarify the existing SHA-1-only writer test with a doc comment indicating it has pinned SHA-1 expectations.
- Update the index verification progress label from SHA-1-specific wording to a hash-agnostic label.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| gix-pack/tests/pack/multi_index/write.rs | Adds a hash-parameterized writer test to cover SHA-256 execution paths and documents the SHA-1-only test. |
| gix-pack/src/index/verify.rs | Renames a progress label to be hash-agnostic during checksum verification. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merged
Member
|
Thanks a lot, appreciated! Once again I had to supersede this PR with #2657 to carry a review commit. Closing this one because of it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
After #2653 lands (this PR is not dependent on it btw), all object-storage crates run tests under both hashes, but the gix-pack multi-index writer was only fed the static sha1 pack indices, so it had no sha256 coverage. This closes that gap (test-only, plus the one-line label fix).
Also part of #281
Testing
The new test is added as a plain
#[test], not feature gated, so the dual-hash gix-pack test runs from the justfile already cover it: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.