Skip to content

2 fixes for 3-way-merge - #3671

Merged
sdottaka merged 2 commits into
WinMerge:masterfrom
Thorium:fix/3way-merge-adjacent-insertions
Oct 10, 2026
Merged

sdottaka merged 2 commits into
WinMerge:masterfrom
Thorium:fix/3way-merge-adjacent-insertions

Conversation

@Thorium

@Thorium Thorium commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

Two fixes for 3-way merging, each with a test that fails on master.
Fix 1 makes the line merge more cautious: both-sided additions that used to be merged silently are now conflicts.

1. Blocks added by both sides in the same place were merged twice

A 3-way compare combines two comparisons (BASE–LOCAL, BASE–REMOTE). An inserted block whose first line matches the line after it can sit one line higher or lower, and the two comparisons don't always pick the same position. The two additions then count as unrelated one-sided changes, and
Auto Merge takes both.

LOCAL               BASE            REMOTE
--------            --------        --------
comment 1           comment 1       comment 1
member 1            member 1        member 1
result              result          result

comment x           comment 2       comment x
member x            ...             member x
result                              result

comment y                           comment 2
member y                            ...
other

comment 2
...

Before: two differences (LOCAL only, REMOTE only); Auto Merge writes comment x / member x / result twice, with no conflict.
After: one difference, reported as a conflict (or as "both sides agree" when the two blocks are identical).

Fix: AlignInsertions() in Src/DiffWrapper.cpp moves an inserted block only when it can meet a block inserted by the other side, and only to that position. Differences the two comparisons already agree on are not touched, and blocks that are part of a detected moved block stay where they are.

Test: DiffWrapper.RunFileDiff_3way_BothSidesAddInTheSamePlace (all four diff algorithms, with and without moved-block detection).

2. /noninteractive auto-merge never exits

WinMergeU /noninteractive /minimize /am /o merged.txt LOCAL.txt BASE.txt REMOTE.txt

Before: the process stays open; closing a modified document shows the "save changes?" prompt even in an unattended run. Happens with and without the merge result pane, and for image compare. After: the merged content is written to the /o path and WinMerge exits. Unresolved conflicts in a merge result are written as conflict sections. Without /o, the changes are discarded.

Fix: CMergeDoc::PromptAndSaveIfNeeded(), CMergeDoc::SaveMergeResult() and CImgMergeFrame::PromptAndSaveIfNeeded() no longer prompt when /noninteractive is set.

Test: NonInteractiveTest in Testing/GoogleTest/GUITests (clean merge and merge with a conflict, each with the result pane on and off; image merge).

1. Blocks added by both sides in the same place were merged twice
2. `/noninteractive` auto-merge never exits
@sdottaka

sdottaka commented Oct 5, 2026

Copy link
Copy Markdown
Member

I tried it, but I can't reproduce the duplicated merge. In the merge result pane I get this:

comment 1
member 1
result

<Merge Conflict>
comment 2
...

Could you attach the LOCAL/BASE/REMOTE files that produce the duplicated comment x / member x / result on master? The example in the description looks simplified, and I assume the exact input and diff algorithm matter.

image

@Thorium

Thorium commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

Sorry, I simplified too much. Attached is a more complete example of where it should occur.

pr3671-repro.zip

Real-world case:

BASE: https://github.com/bryanedds/Nu/blob/0aaf555c4f757b93f664beb0160d9ec92a6352a6/Nu/Nu/World/WorldImNui.fs
LOCAL: https://github.com/bryanedds/Nu/blob/0de5a993bae848587df2ea7bb2b7cd32361205b6/Nu/Nu/World/WorldImNui.fs
REMOTE: https://github.com/bryanedds/Nu/blob/645dd659fd48af545ee4e09592d95de5428a4a1d/Nu/Nu/World/WorldImNui.fs

Both sides add static member doTmxMap; on master the merge result contains it twice, with no conflict. Git reports a conflict there.

@sdottaka

sdottaka commented Oct 7, 2026

Copy link
Copy Markdown
Member

Thanks for attaching the example. I was able to reproduce the issue.

Since this is related to the comparison algorithm, I'd like to think about it a little more.

@Thorium

Thorium commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Yes, I tested about 500 merges on random repositories, and only 2 failed, so this isn't a common case. The main problem is that it can produce incorrect behaviour without the user possibly noticing it. Sorry to bundle 2 fixes into one PR; I thought it was the fastest way to get out of beta. Happy to leave these for you.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Freshly built conflict-only results can exit without writing the requested output, and the regression test currently masks this path.

2 open findings
What changed in this PR

Improves three-way merge correctness and unattended auto-merge shutdown behavior.

Changes:

  • Aligns ambiguous insertions from both sides into one conflict.
  • Saves unattended text/image merge output without prompting.
  • Adds regression and GUI tests.
File Description
Src/​DiffWrapper.cpp Aligns equivalent insertion positions.
Src/​MergeDoc.cpp Handles unattended text saving.
Src/​MergeDocResultPane.cpp Suppresses unattended conflict prompts.
Src/​ImgMergeFrm.cpp Handles unattended image saving.
Testing/​GoogleTest/​DiffWrapper/​DiffWrapper_test.cpp Tests insertion alignment.
Testing/​GoogleTest/​GUITests/​NonInteractiveTest.cpp Tests unattended merges.
Testing/​GoogleTest/​GUITests/​GUITestUtils.cpp Adds process execution helper.
Testing/​GoogleTest/​GUITests/​GUITestUtils.h Declares execution helper.
Testing/​GoogleTest/​GUITests/​GUITests.vcxproj Registers the new test.
Testing/​GoogleTest/​GUITests/​GUITests.vcxproj.filters Adds the test to project filters.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread Src/MergeDoc.cpp Outdated
Comment thread Testing/GoogleTest/GUITests/NonInteractiveTest.cpp
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@sdottaka

Copy link
Copy Markdown
Member

Thanks for the interesting PR! I'll merge it.

For future PRs, it would be helpful if you could keep unrelated changes in separate PRs.

In particular, PR titles are used directly in the changelog and release notes, so a title like "2 fixes for 3-way merge" doesn't clearly describe the changes, especially when the fixes are unrelated.

@sdottaka
sdottaka merged commit 3bf6aab into WinMerge:master Oct 10, 2026
3 checks passed
Thorium added a commit to Thorium/winmerge that referenced this pull request Oct 11, 2026
Brings in the merge-result panel fixes (WinMerge#3515 parts 2-4), 3-way merge
fixes (WinMerge#3671), Moved line filtering (WinMerge#3681) and the recent comparison
list (WinMerge#3663, WinMerge#3673).

Conflicts:
- Src/resource.h: master's IDC_RECENT_COMPARES_SHOW/LIST took 1698/1699;
  IDC_IGNORETRAILINGPUNCT_CHECK and IDC_TREE_SITTER_OCCURRENCE_CHECK move
  to 1720/1721.
- Src/MergeDocResultPane.cpp: both sides skipped the unresolved prompt
  when non-interactive; kept master's comment.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Thorium
Thorium deleted the fix/3way-merge-adjacent-insertions branch October 11, 2026 15:54
@sdottaka sdottaka added this to the v2.16.60 milestone Oct 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants