Retire KDiff3; make DiffPlexMergeEngine the sole merge engine - #9
Merged
Conversation
KDiff3 (Tools/KDiff3.cs, Tools/KDiff3MergeEngine.cs) and the IMergeEngine interface that used to sit in front of it are deleted. DiffPlexMergeEngine is now the only text-merge engine, called directly by FileMerger - no more engine-selection step at startup, no more MergeEngine/KDiff3Path/ ReviewEachMerge/ShowPathsInKDiff3 App.config settings. User-facing behavior change: a genuine conflict that needs manual resolution no longer opens KDiff3's merge-editor window. Instead it writes a git/diff3-style conflict-marker sidecar file and opens it in the OS's default editor for that file type (Tools/FileOpener.cs, Core-side, Process.Start with UseShellExecute=true) - in both the GUI's interactive path and the CLI/MCP headless path, since they're now the same code path underneath. A dry-run merge_conflicts call still writes the sidecar for a would-be conflict but does not open it, since a preview must not have that kind of side effect. This is a deliberate tradeoff, not a strict improvement: DiffPlexMergeEngine has a measured non-zero failure rate on dense multi-edit conflicts and no vanilla-less 2-way fallback, both of which KDiff3 handled. See docs/decisions/kdiff3-retirement.md for the full rationale and for the empirical KDiff3 process-behavior findings (window-title polling, the 250ms-poll-interval constraint, failed window-suppression attempts, unverified focus restoration) preserved now that the motivating code is gone - previously this only lived in the local, gitignored HANDOFF.md. CLAUDE.md, CONTRIBUTING.md, README.md updated throughout to reflect KDiff3 being gone. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GXAuGMLB44T5Zv5o5ZzKah
- DiffPlexMergeEngine.NormalizeWhitespace used the parameterless string.Trim(), which trims the full Unicode whitespace category (including NBSP) at the edges of the joined comparison text, silently undoing this class's own documented reason for using a narrow ASCII-only regex elsewhere in the same method. A trailing-NBSP-vs-plain-text conflict could be misclassified as whitespace-only and silently discarded. Fixed via Trim(WhitespaceChars), the same explicit ASCII set as the regex; regression test added, and manually verified to fail against the pre-fix code before being restored. - The interactive path's "Merge N of M was canceled" prompt is now shown for every automatic engine refusal (genuine conflict, missing vanilla, outdated hash), not just true user cancellations - DiffPlexMergeEngine's interactive Merge() has no UI to cancel out of at all, unlike the retired KDiff3 engine. Renamed to ConfirmContinueAfterSkippedMerge and reworded to "was skipped", which is accurate for both this method's call sites. - FileOpener.Open's return value was discarded, so the notifier message unconditionally claimed "attempting to open it now for review" even when the sidecar's .conflict extension has no OS file association and the open silently failed. Reordered so the open happens first and the message reflects what actually happened. - OptionsForm.Designer.cs: removing two checkboxes shrank grpMerging.Size but left the button row and ClientSize unchanged, opening a 57px dead-space gap where the original had an 11px one. Cascaded the 46px reduction through the button row and ClientSize to restore the original gap. - Removed a fabricated citation: a comment and CLAUDE.md both claimed CONTRIBUTING.md documents a "no premature abstraction" convention that justified deleting IMergeEngine. It doesn't - grep confirms CONTRIBUTING.md never mentions abstraction, interfaces, or "premature". Reworded to state this as the deletion's own reasoning instead of a false citation. - Fixed a stale comment in BuildMerge still pointing at a Program.cs engine-selection switch this same change deleted. - Mcp/CLAUDE.md's "Minimal required permissions" section didn't mention that merge_conflicts can launch external processes (FileOpener.Open, once per genuine conflict, uncapped) or that DiffPlexConflictsDirectory is a filesystem root the process writes to - both real omissions in a doc whose stated purpose is "exactly what the process touches, and at what privilege level". - Tightened an overclaiming test-suite comment about xunit's parallelization defaults: sequential execution is only guaranteed within one collection (implicitly one per class), not across classes - a future test class also exercising the shared FileOpener.Open static could race this one. Two findings from the same review were evaluated and deliberately not acted on: Program.TryOpenFile has the identical missing-UseShellExecute bug this change's FileOpener.cs documents but doesn't fix (pre-existing, affects 7 untestable GUI call sites, out of scope for this diff); and DiffPlexMergeEngine becoming the sole engine without new verification to close its already- disclosed reliability gap is this change's own stated premise, not an oversight (see docs/decisions/kdiff3-retirement.md). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GXAuGMLB44T5Zv5o5ZzKah
- CLAUDE.md's "Interactive vs. headless split" section still described the sidecar-open behavior in its pre-reorder sequencing (open after the notifier message) and never mentioned dryRun's suppression of it. Updated to match the actual current order and document openConflictMarkers. - The reordering's own code comment claimed FileOpener.Open's return value is "exactly" a signal that an editor opened. It isn't - it only distinguishes Process.Start succeeding from throwing. This change's own end-to-end verification observed a real OpenWith.exe spawn (the OS's file-open picker, since the test machine has no association for ".conflict"), meaning "opened" can be true while what the user sees is a picker, not an editor. Reworded to say what the value actually distinguishes, using that same observed case as a concrete example instead of an unqualified claim. - CLAUDE.md's Project overview claimed origin was still the upstream AnotherSymbiote/WitcherScriptMerger remote and no separate fork existed. False - a separate fork (TheValiantOne/WitcherScriptMerger, default branch main) is the actual origin; upstream (AnotherSymbiote, default branch master) is a second remote kept for reference. This stale claim caused a real mistake: `gh pr create` run without --repo/--base for this same change defaulted to opening a PR against the upstream repo's master branch instead of this fork's main (closed immediately once caught). Corrected, with the mistake called out explicitly so it isn't repeated. - Minor: a code comment's NBSP example used a literal U+00A0 character (visually indistinguishable from a space), rather than an explicit \u00A0 escape - fixed for the same reason the regression test itself avoids a literal character. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GXAuGMLB44T5Zv5o5ZzKah
# Conflicts: # CLAUDE.md # WitcherScriptMerger.Core/Mcp/WsmMcpTools.cs # WitcherScriptMerger.Core/Paths.cs
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
1 participant
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.
Summary
Tools/KDiff3.cs,Tools/KDiff3MergeEngine.cs, and theIMergeEngineinterface that used to sit betweenFileMergerand the merge engine.DiffPlexMergeEngine(in-process, DiffPlex-based, no external binary) is now the sole text-merge engine — there's no more engine-selection step at startup, and theMergeEngine/KDiff3Path/ReviewEachMerge/ShowPathsInKDiff3App.configsettings are gone.DiffPlexMergeEnginehas no UI of its own): a genuine conflict that needs manual resolution writes a git/diff3-style conflict-marker sidecar file and opens it in the OS's default editor for that file type (Tools/FileOpener.cs,Process.StartwithUseShellExecute = true), instead of opening KDiff3's merge-editor window.docs/decisions/kdiff3-retirement.md(new) is a full decision record: the rationale for retiring KDiff3, and the empirical KDiff3 process-behavior findings (window-title polling, the load-bearing 250ms poll interval, five failed window-suppression techniques, unverified foreground-focus restoration) preserved now that the motivating code is gone — this was previously only written up in this fork's local, gitignoredHANDOFF.md.CLAUDE.md,CONTRIBUTING.md,README.mdupdated throughout to reflect KDiff3 being gone.User-facing behavior changes
<<<<<<</|||||||/=======/>>>>>>>, labeled with real mod names) is written under a newDiffPlexConflictsfolder and opened in your default editor for that file type. There is no merge UI anymore — resolving a conflict means editing the sidecar by hand (or comparing the source files yourself) and re-running the merge.dryRunwrites but does not open. The MCPmerge_conflictstool'sdryRunoption (and the equivalent CLI path) still writes a conflict's sidecar for inspection, but does not launch an editor for it — a preview must not have that kind of side effect. (This was caught and fixed during this change's own review — an earlier version of the dry-run path would have opened one editor window per genuine conflict on every preview call.)DiffPlexMergeEnginehas a confirmed upstream DiffPlex bug it has to defend against, with measured failure rates from ~0.35% (realistic single-edit-per-side density) up to double digits on adversarial dense-edit cases — seeCLAUDE.md's Compatibility constraints anddocs/decisions/kdiff3-retirement.mdfor the full numbers. This never produces silently wrong output (the engine detects the failure and refuses rather than risk corrupted merge output), but it does mean more conflicts now need the manual sidecar-editing workflow than would have under KDiff3. This is a deliberate, disclosed tradeoff, not a regression — see the decision doc for why it was accepted.ThreeWayDifferas used here has no equivalent).ReviewEachMergeandShowPathsInKDiff3settings are gone — there's no merge UI left for either to apply to.Known adjacent bug, not fixed here
Program.TryOpenFile(host project, used by several existing "open merged/vanilla file" GUI actions) has the identical missing-UseShellExecute = truebug that this change's newTools/FileOpener.cswas written to avoid — confirmed via code inspection, documented inFileOpener.cs's own header comment. Left unfixed: it's pre-existing, unrelated to KDiff3, affects 7 GUI call sites that can't be exercised end-to-end in this development environment (no display), and fixing it would expand an already large, high-stakes diff into changing behavior that can't be verified here. Worth a follow-upfix/branch.Process note
While preparing this PR,
gh pr create(run without--repo/--base) silently defaulted to opening it against the upstreamAnotherSymbiote/WitcherScriptMergerrepo'smasterbranch instead of this fork'smain— a stale line inCLAUDE.mdclaiming no separate fork existed yet contributed to missing this. That PR (AnotherSymbiote/WitcherScriptMerger#9) was closed immediately with an explanatory comment once caught, and this PR was opened correctly with--repo/--basespecified explicitly.CLAUDE.md's Project overview now documents the correct remote/branch setup and calls out the mistake so it isn't repeated.Verification
dotnet build WitcherScriptMerger.sln --configuration Release: 0 errors, 5 pre-existingCA1823warnings only (unrelated to this change — down frommain's 10, sinceKDiff3.cs's own 5 unused-P/Invoke-field warnings are gone along with the file).dotnet test WitcherScriptMerger.sln: 31/31 passing, including new regression coverage for the dry-run/FileOpenerinteraction and a real NBSP-vs-whitespace classification bug caught during this change's own review (see below).dotnet format whitespace WitcherScriptMerger.sln --verify-no-changes: clean.OpenWith.exeprocess spawn matching the run's timestamp, since visual confirmation isn't possible in this environment) — exercised through both the GUI-triggered interactive path (a purpose-built harness callingFileMerger.MergeFilesInteractivedirectly) and the CLI's headlessmergeverb.grep -ri kdiff3across the repo: only matches the new decision doc and legitimate historical/pointer references in comments and other docs.code-reviewpass (multi-angle, high effort) was run against the full diff. All 11 findings were triaged; the substantive ones were fixed in a second commit on this branch:DiffPlexMergeEngine.NormalizeWhitespaceusedstring.Trim()(Unicode-aware) instead of the class's own documented ASCII-only whitespace set, which could silently discard a genuine NBSP-vs-plain-text content difference at a piece boundary as if it were whitespace-only. Fixed, with a regression test verified to fail against the pre-fix code..conflictextension has no default OS file association and the open can silently fail — reordered so the message reflects the real outcome.OptionsForm.Designer.cs's layout cascade was incomplete after removing two checkboxes, leaving a 57px dead-space gap instead of the original 11px.CLAUDE.mdboth claimedCONTRIBUTING.mddocuments a "no premature abstraction" convention that justified deletingIMergeEngine— it doesn't) was corrected to state the reasoning on its own terms.Mcp/CLAUDE.md's permissions section (missing theDiffPlexConflictsfilesystem root and the process-spawning behavior), and an overclaiming test-suite comment about xunit's parallelization defaults were also fixed.Program.TryOpenFilebug above, and the disclosed DiffPlex reliability gap) were deliberately left as-is — see their sections above.FileOpener.Open/message logic) caught two more claims-vs-code mismatches, fixed in a third commit:CLAUDE.md's prose description of the sidecar-open behavior still described the pre-reorder sequencing and didn't mentiondryRun's suppression of it; and a code comment claimedFileOpener.Open's return value was "exactly" a signal that an editor opened, when the E2E run's ownOpenWith.exeobservation shows it only distinguishes "the launch call succeeded" from "it threw" — the picker case is a real, observed possibility, not a hypothetical.CLAUDE.md's Project overview: it saidoriginwas still the upstreamAnotherSymbiote/WitcherScriptMergerremote and no separate fork existed. False, and its false-ness produced a real mistake while preparing this PR — see below.AI assistance disclosure
This PR was substantially produced by an AI coding agent (Claude Code), including the architecture decisions, the decision-record doc, and the end-to-end verification described above, working from an explicit task specification. All findings from the code-review pass were reviewed and fixed by the same process before this PR was opened.