-
-
Notifications
You must be signed in to change notification settings - Fork 447
fix: coalesce split Myers hunks to prevent false merge conflicts #2476
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
Mattias Granlund (mtsgrd)
wants to merge
1
commit into
GitoxideLabs:main
Choose a base branch
from
mtsgrd:fix/false-conflict-empty-insertion-overlap
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| use gix_merge::blob::{builtin_driver, builtin_driver::text::Conflict, Resolution}; | ||
| use imara_diff::intern::InternedInput; | ||
|
|
||
| /// Minimal reproduction: Myers produces a false conflict where git merge-file resolves cleanly. | ||
| /// | ||
| /// base: alpha_x / (blank) / bravo_x / charlie_x / (blank) | ||
| /// ours: (blank) / (blank) / bravo_x / charlie_x | ||
| /// theirs: alpha_x / (blank) / charlie_x / (blank) | ||
| /// | ||
| /// base→ours: alpha_x deleted (replaced by blank), trailing blank removed | ||
| /// base→theirs: bravo_x deleted | ||
| /// | ||
| /// These are non-overlapping changes that git merges cleanly. | ||
| /// See https://github.com/GitoxideLabs/gitoxide/issues/2475 | ||
| #[test] | ||
| fn myers_false_conflict_with_blank_line_ambiguity() { | ||
| let base = b"alpha_x\n\nbravo_x\ncharlie_x\n\n"; | ||
| let ours = b"\n\nbravo_x\ncharlie_x\n"; | ||
| let theirs = b"alpha_x\n\ncharlie_x\n\n"; | ||
|
|
||
| let labels = builtin_driver::text::Labels { | ||
| ancestor: Some("base".into()), | ||
| current: Some("ours".into()), | ||
| other: Some("theirs".into()), | ||
| }; | ||
|
|
||
| // Histogram resolves cleanly. | ||
| { | ||
| let options = builtin_driver::text::Options { | ||
| diff_algorithm: imara_diff::Algorithm::Histogram, | ||
| conflict: Conflict::Keep { | ||
| style: builtin_driver::text::ConflictStyle::Merge, | ||
| marker_size: 7.try_into().unwrap(), | ||
| }, | ||
| }; | ||
| let mut out = Vec::new(); | ||
| let mut input = InternedInput::default(); | ||
| let res = builtin_driver::text(&mut out, &mut input, labels, ours, base, theirs, options); | ||
| assert_eq!(res, Resolution::Complete, "Histogram should resolve cleanly"); | ||
| } | ||
|
|
||
| // Myers should also resolve cleanly (it used to produce a false conflict because | ||
| // imara-diff's Myers splits the ours change into two hunks — a deletion at base[0] | ||
| // and an empty insertion at base[2] — and the insertion collided with theirs' | ||
| // deletion at base[2]). | ||
| { | ||
| let options = builtin_driver::text::Options { | ||
| diff_algorithm: imara_diff::Algorithm::Myers, | ||
| conflict: Conflict::Keep { | ||
| style: builtin_driver::text::ConflictStyle::Merge, | ||
| marker_size: 7.try_into().unwrap(), | ||
| }, | ||
| }; | ||
| let mut out = Vec::new(); | ||
| let mut input = InternedInput::default(); | ||
| let res = builtin_driver::text(&mut out, &mut input, labels, ours, base, theirs, options); | ||
| assert_eq!( | ||
| res, | ||
| Resolution::Complete, | ||
| "Myers should resolve cleanly (git merge-file does). Output:\n{}", | ||
| String::from_utf8_lossy(&out) | ||
| ); | ||
| } | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| mod builtin_driver; | ||
| mod false_conflict; | ||
| mod pipeline; | ||
| mod platform; | ||
|
|
||
|
|
||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tl;dr: I think this should be fixed on the diff-algo side rather than merge-algo side.
If we just add another blank line to each of these revisions, you still get a conflict.
This is down to the naive Myer's diff algorithm doing a greedy longest substring match between revisions s.t. the matching intermediate lines get matched. The fix seems to just try to look "one line back" from the hunk, but that just happens to work because there was precisely one line separating these hunks. My edit here separates the hunks by 2 lines instead, making the conflict reappear.
The base-ours diff here is with base Myers this:
This is natural in Myers as the intermediate matching section is greedily matched. I've validated this output both with an old Myers implementation I wrote myself a few years ago, and with gix. It doesn't matter how many blank lines separate the hunks, they'll be matched all the same, as evidenced by my above tweak to the test case causing it to fail again. So checking just one line back is an edge case fix for an edge case.
This will always conflict with the removal of
bravo_x, which looks like this.alpha_x -bravo_x charlie_xNow, Git's Myers implementation is doing something to prioritize hunk cohesion, because its diff output is different:
This does not conflict with the removal av
bravo_xbecause there's a buffering matched line between the two hunks. I don't know exactly what optimization Git has here, but the point I'm making is that I think this should be a diff-algorithm fix rather than merge-algorithm fix.As a final note, I think the test case is a bit misleading as it makes this out to be some issue with blank lines in particular, but it's not. It's an issue with matching lines. Replacing the blank lines with
blahas the exact same effect.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for looking into this Mattias Granlund (@mtsgrd), and particularly to Simon Larsén (@slarse) for the eager review!
Without having even looked at the details, this makes me think that Git might not run into it because it makes diff-slider adjustments, maybe even to the point where it can avoid conflicts, which would also be rather puzzling to me.
Maybe I try to port the diff over to using imara-diff v0.2 which ships with 95% Git-diff-slider compatibility from what we could tell.