Skip to content

Add DiffPlex-based merge engine and a test project - #7

Merged
TheValiantOne merged 1 commit into
mainfrom
feature/diffplex-merge-engine
Aug 7, 2026
Merged

Add DiffPlex-based merge engine and a test project#7
TheValiantOne merged 1 commit into
mainfrom
feature/diffplex-merge-engine

Conversation

@TheValiantOne

Copy link
Copy Markdown
Owner

Summary

Unit 6 of the ongoing modernization: a second, in-process IMergeEngine implementation alongside KDiff3MergeEngine, built on the MIT-licensed DiffPlex package, plus this repo's first automated test project.

  • WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs — builds a custom merge loop around DiffPlex.ThreeWayDiffer.CreateDiffs (not its own CreateMerge) so it can auto-resolve whitespace-only conflicts (mirroring KDiff3's --cs "WhiteSpace3FileMergeDefault=2") and render genuine conflicts as git-style markers labeled with real mod names.
  • WitcherScriptMerger.Core/Tools/FileEncoding.cs — the old KDiff3.cs::EnsureUtf16Encoding UTF-16LE+BOM normalization logic, moved to Core and shared by both engines.
  • WitcherScriptMerger.Tests (xunit, references Core only) — 28 tests covering both new files, Hasher.cs, and gated real-install/real-KDiff3 cross-checks.

The DiffPlex bug — read this before the rest of the diff

While verifying the merge loop, I found DiffPlex 1.9.0's ThreeWayDiffer has a confirmed upstream bug: when two independent edits (one per side) interleave or overlap relative to the base text in certain ways, CreateThreeWayDiffBlocks can emit a block list whose OldCount/NewCount don't correspond to the real piece arrays. This is not a bug in this PR's own code — BuildMerge's loop is a faithful port of DiffPlex's own ThreeWayDiffer.CreateMerge, and I reproduced the identical failure calling DiffPlex's own official CreateMerge directly, with both LineChunker (DiffPlex's own default and the only chunker its own test suite ever exercises for 3-way diffs) and LineEndingsPreservingChunker (the one this engine uses for line-ending fidelity).

It surfaces two ways: an outright ArgumentOutOfRangeException, or — worse — no exception at all but silently wrong output (content lost or duplicated), confirmed via a minimal repro (base "a();/b();/c();", one mod inserts a line, the other independently changes b() to B()).

A randomized stress test (100,000 trials against the real, fixed BuildMerge) measured combined failure rates:

Scenario Failure rate
1 edit/side, 50-200 lines (closest to a typical two-mod .ws conflict) 0.35%
1-2 edits/side, 50-200 lines 0.88%
2-3 edits/side, 50-200 lines 2.65%
1-6 edits/side, 50-200 lines 4.99%
1-6 edits/side, 1-19 lines (dense/adversarial) 38.89%

BuildMerge defends against both failure modes (a try/catch around the block-processing loop, plus a post-loop check that oldIndex/newIndex actually reached the true piece counts) and throws a typed DiffAlgorithmException either way. MergeHeadless catches that and reports NeedsManualResolution without writing anything, not even a conflict-marker sidecar — the marker content itself would be built from the same untrustworthy indices, so this is the one case where the engine can't offer any starting point at all.

This is the primary reason the default engine is not flipped (see below) — it's a measured, non-negligible reliability gap KDiff3 doesn't share, not just "not enough cross-checking yet."

What else is in the diff, and why

A code-review pass (before this PR was opened) found 15 issues; 11 were fixed in code and 3 documented as accepted, deferred limitations (all explained in CLAUDE.md and in code comments):

  • Fixed: the DiffPlex bug above; a whitespace-vs-deletion misclassification (a genuine content-vs-deletion conflict was being silently auto-resolved because both sides collapsed to ""); the conflict-marker sidecar's location (see below); a missing user-facing message on a genuine-conflict skip; Paths.cs's own eager static field initializers undermining this PR's AppState.Settings laziness fix one hop out; a non-atomic AppState.Settings lazy-init race (now LazyInitializer.EnsureInitialized); an overly-broad Unicode-whitespace regex that could misclassify real content differences (NBSP vs space) as whitespace-only; a hardcoded "waiting for KDiff3 to close" progress message shown even when the active engine isn't KDiff3; a conflict-marker line-break edge case (lone trailing \r); and a UTF-16LE-vs-UTF-32LE BOM ambiguity in the shared encoding helper.
  • Documented, not fixed (out of scope for this unit, explained in code comments + CLAUDE.md): DiffPlexMergeEngine refuses any conflict with no vanilla file, while KDiff3MergeEngine always attempts a degraded 2-way merge instead — DiffPlex's ThreeWayDiffer has no coherent 2-way mode to fall back to, so building one is new scope; the "merging an updated mod file" outdated-hash guard surfaces as a silent Failed on DiffPlex's interactive path (no UI exists there at all yet, so this isn't a new gap); DeleteIfExists's best-effort exception swallow on stale-sidecar cleanup.

A second regression, found only by running the real CLI end-to-end and inspecting the filesystem (not by unit tests or code review): my first fix for the sidecar location moved it under Paths.TempBundleContent — but FileMerger.CleanUpTempFiles() deletes that entire tree wholesale at the end of every headless merge run, so the sidecar was gone by the time the CLI process exited. Fixed by giving it its own dedicated top-level directory, Paths.DiffPlexConflictsDirectory, uncollided with any existing cleanup routine. Verified via a fresh scratch game/mods tree: the sidecar now persists after the process exits, contains correct UTF-16LE+BOM git-style markers, and the live mods folder has zero stray files.

Two things about that directory worth flagging explicitly (documented in CLAUDE.md, not fixed - low priority for this unit):

  1. Nothing automatically cleans it up (same "accumulates, clear it yourself" property tempbundlecontent already has).
  2. It's Environment.CurrentDirectory-relative. Program.RunCli resets that to the exe's own directory, so CLI-mode sidecars land predictably - but the GUI path never does that reset, so a GUI-mode sidecar's location isn't guaranteed. Not a functional problem today since the GUI-mode DiffPlex path has no UI to open the sidecar from yet anyway.

Verification

  • dotnet build WitcherScriptMerger.sln - succeeds, 0 warnings in Core/Tests, the same 5 pre-existing CA1823 warnings in the host project (none new).
  • dotnet test - 28/28 passing.
  • dotnet format whitespace WitcherScriptMerger.sln --verify-no-changes - clean.
  • End-to-end CLI run (WitcherScriptMerger.exe merge) against a scratch game/mods tree with MergeEngine=diffplex: a whitespace-only conflict auto-solved correctly (UTF-16LE+BOM, matching source1's indentation per WhiteSpace3FileMergeDefault=2 semantics) and a genuine conflict correctly skipped, producing a well-formed conflict-marker sidecar at the new location that survives process exit, with zero stray files in the live mods folder.
  • Real-KDiff3 A/B cross-check: did not run this session. WSM_TEST_GAME_DIR is unset in this environment, so KDiff3CrossCheckTests and the Hasher live-install cross-check no-op (by design - they never fail when unset, just skip their assertions). The CLI E2E run above used a placeholder (non-functional) KDiff3.exe since only the DiffPlex path needed exercising. The gate exists for anyone running with a real install and WSM_TEST_GAME_DIR set.
  • CI does not run dotnet test. Per .github/workflows/build.yml, CI runs dotnet build --configuration Release and dotnet format whitespace --verify-no-changes only. Wiring the new test project into CI wasn't in scope for this unit.

Default engine: NOT flipped

AppState.MergeEngine still defaults to KDiff3MergeEngine. DiffPlexMergeEngine is available via the MergeEngine=diffplex App.config switch but is not production-ready as the default, primarily because of the measured DiffPlex ThreeWayDiffer failure rates above, and secondarily because it hasn't been cross-checked against KDiff3 on real conflicting files (that check exists but didn't run this session - see above).

AI assistance disclosure

Developed with AI assistance (Claude Code), per CONTRIBUTING.md.

Adds DiffPlexMergeEngine, an in-process alternative to KDiff3MergeEngine
built on the MIT-licensed DiffPlex package, plus WitcherScriptMerger.Tests
(xunit), the repo's first automated test project. Not the default engine:
DiffPlex's ThreeWayDiffer has a confirmed upstream bug that can produce
internally inconsistent diff-block metadata on multi-edit conflicts
(measured 0.35%-38.89% failure rate depending on edit density) -
DiffPlexMergeEngine detects and safely refuses rather than trusting
corrupted output, but this is a real reliability gap KDiff3 doesn't share.

Also fixes two things code review and end-to-end testing surfaced that
undermined this PR's own AppState.Settings laziness fix: Paths.cs's
eager static field initializers, and a non-atomic Settings lazy-init race.

AI-assisted development per CONTRIBUTING.md.
@TheValiantOne
TheValiantOne merged commit b2ac346 into main Aug 7, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant