Add failing gix blob-merge tests for Myers algorithm issue#12948
Add failing gix blob-merge tests for Myers algorithm issue#12948
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Pull request overview
Adds regression coverage in but-core for an upstream gix blob-merge bug where the Myers diff algorithm produces a false conflict for a specific 3-way merge shape, while Histogram succeeds—documenting the issue and providing a verified workaround path relevant to GitButler’s cherry-pick/tree-merge flows.
Changes:
- Add a
#[should_panic]test documenting the current Myers false-conflict behavior (expected to stop panicking once upstream is fixed). - Add a passing test asserting Histogram merges the same inputs cleanly.
- Register the new
mergetest module in thebut-corecore test harness.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
crates/but-core/tests/core/merge.rs |
Introduces two blob-merge tests (Myers expected-to-fail + Histogram expected-to-pass) driven by fixture inputs. |
crates/but-core/tests/core/main.rs |
Wires the new merge test module into the existing integration-test entrypoint. |
crates/but-core/tests/core/merge.rs
Outdated
|
|
||
| fn fixtures() -> std::path::PathBuf { | ||
| let dir = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")) | ||
| .join("tests/fixtures/merge"); |
There was a problem hiding this comment.
The test reads fixtures from tests/fixtures/merge, but that directory doesn't exist in this crate right now (only tests/fixtures/{scenario,...} are present). As written, both tests will panic at runtime with "fixtures directory missing...". Add the tests/fixtures/merge directory (and the base.svelte/ours.svelte/theirs.svelte files) to the PR, or update the fixture path to an existing location/pattern used by this crate's tests.
| .join("tests/fixtures/merge"); | |
| .join("tests/fixtures/scenario/merge"); |
|
That's quite a find! I wouldn't even be surprised if it's a problem in the actual diff/merge algorithm in |
gix's blob merge (gix_merge::blob::builtin_driver::text) with the Myers diff algorithm produces a false conflict on inputs where git merge-file resolves cleanly. The trigger is a 3-way merge where base→ours is a large expansion (~70 new lines inserted above a section comment) and base→theirs is a single-line deletion of that same comment. imara-diff's Myers implementation produces different hunk boundaries than git's xdiff for this input, causing the merge to see the deletion as overlapping with the insertion — a false conflict. The Histogram algorithm in imara-diff handles the same input correctly. This directly impacts GitButler's commit amend operation: create_tree() uses gix's tree merge for the cherry-pick step, and the false conflict causes valid changes to be rejected with CherryPickMergeConflict. Add a #[should_panic] test that documents the upstream bug and will start failing (reminding us to remove the annotation) once gix is fixed, plus a Histogram-based test that verifies the working alternative. Upstream: GitoxideLabs/gitoxide#2475
gix's blob merge (gix_merge::blob::builtin_driver::text) with the Myers
diff algorithm produces a false conflict on inputs where git merge-file
resolves cleanly. The trigger is a 3-way merge where base→ours is a
large expansion (~70 new lines inserted above a section comment) and
base→theirs is a single-line deletion of that same comment. imara-diff's
Myers implementation produces different hunk boundaries than git's xdiff
for this input, causing the merge to see the deletion as overlapping
with the insertion — a false conflict. The Histogram algorithm in
imara-diff handles the same input correctly.
This directly impacts GitButler's commit amend operation: create_tree()
uses gix's tree merge for the cherry-pick step, and the false conflict
causes valid changes to be rejected with CherryPickMergeConflict.
Add a #[should_panic] test that documents the upstream bug and will
start failing (reminding us to remove the annotation) once gix is fixed,
plus a Histogram-based test that verifies the working alternative.
Upstream: