gix-merge: support SHA-256 and fix a file/directory merge bug#2677
Open
Adrian Ratiu (10ne1) wants to merge 4 commits into
Open
gix-merge: support SHA-256 and fix a file/directory merge bug#2677Adrian Ratiu (10ne1) wants to merge 4 commits into
Adrian Ratiu (10ne1) wants to merge 4 commits into
Conversation
When one side edits a file and the other replaces it with a directory, the merge finds the replacement by checking only the next change after the deletion, assuming the dir's addition is adjacent to it. That adjacency is an accident of SHA-1: rename-detection orders changes by object-id and the addition lands next to the deletion. Switching to SHA-256 changes every id and reorders the diff, moving the addition elsewhere, so a merge that resolved cleanly under SHA-1 missed the replacement under SHA-256, left the file in place and panicked. In other words, what happens is that the same input gives a different merge result due to the hash algo. Instead of looking just at the next change, scan all of that side's changes so the outcome no longer depends on the object-id order. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
They hard-coded SHA-1 when opening the object database and its in-memory proxy, so SHA-256 fixtures couldn't be read. Derive the hash from the test environment instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
The script hard-coded SHA-1 object ids in its `update-index` blocks. Add an oid() helper that maps each to its SHA-256 equivalent and route every id through it so the fixture regenerates correctly under both hashes. Most of these ids are content blobs that could be derived with `git hash-object`, but ~10 can't be (conflict-marker merge outputs and submodule commits), so a plain id-to-id table is the simplest approach that handles all of them uniformly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
gix-merge had no entry in the dual-hash test target. Add SHA-1 and SHA-256 runs, with the SHA-256 archives the remaining fixtures need. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
There was a problem hiding this comment.
Pull request overview
This PR extends gix-merge’s tree-merge baseline test infrastructure to run under both SHA-1 and SHA-256 object formats, and adjusts the tree-merge implementation to correctly handle a delete-vs-dir-add ordering scenario that becomes visible under SHA-256 rename-tracking order.
Changes:
- Run
gix-mergenextest under bothGIX_TEST_FIXTURE_HASH=sha1andsha256injustfile. - Make baseline tests and fixture generation SHA-256 aware (ODB init options in Rust, and SHA1→SHA256 OID translation in the fixture script).
- Fix a file/dir merge edge case by scanning side changes for a directory addition instead of assuming adjacency.
Reviewed changes
Copilot reviewed 4 out of 8 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| justfile | Adds dual-hash gix-merge test runs to CI/dev test recipe. |
| gix-merge/tests/merge/tree/baseline.rs | Opens the ODB with the active test hash kind so baselines work under SHA-256. |
| gix-merge/tests/fixtures/tree-baseline.sh | Makes fixture generation portable across SHA-1/SHA-256 by translating hard-coded historical SHA-1 ids. |
| gix-merge/src/tree/function.rs | Fixes delete/dir-add detection under SHA-256 rename-tracking ordering by searching all side changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
182
to
184
| let our_commit_id = gix_hash::ObjectId::from_hex(our_commit_id.as_bytes()).unwrap(); | ||
| let their_commit_id = gix_hash::ObjectId::from_hex(their_commit_id.as_bytes()).unwrap(); | ||
| let merge_info = parse_merge_info(std::fs::read_to_string(subdir_path.join(merge_info_filename)).unwrap()); |
Contributor
Author
There was a problem hiding this comment.
These were not introduced by my commit... I try to not modify unrelated code and keep diffs minimal.
Perhaps it's better to do a separate "unwrap cleanup" PR.
| let object_hash = gix_testtools::object_hash(); | ||
| let odb = gix_odb::at_opts( | ||
| subdir_path.join(".git/objects"), | ||
| Vec::new(), |
Contributor
Author
There was a problem hiding this comment.
Fair point
Comment on lines
+711
to
+715
| changes.iter().any(|change| { | ||
| change.inner.entry_mode().is_tree() | ||
| && change.inner.location() == location | ||
| && matches!(change.inner, Change::Addition { .. }) | ||
| }) |
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
This makes gix-merge work under SHA-256, running its tree-merge baselines under sha256.
This surfaced a pre-existing bug in the merge itself, which this also fixes (see details in the first commit message).
I can't say I'm entirely happy with the
tests/fixtures/tree-baseline.shin the 3rd commit, which I consider overly verbose for the harcoded hashes, however it's the best myself and Claude could come up with.This is part of #281 .
Testing
gix-merge now runs under both hashes; added to the justfile:
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.