From c5ab8d04e60cef8fd4e2b55c7f42d5fb5f0a29d9 Mon Sep 17 00:00:00 2001 From: Chris Knight Date: Mon, 10 Aug 2026 16:04:03 -0400 Subject: [PATCH] Fix correctness bugs found by review in the function-level merge engine A multi-angle code review of the just-merged function-level merge engine (PR #18) surfaced several real bugs: - The function-level rescue's success notification used DialogIcon. Information, which HeadlessMergeNotifier routes to stdout - corrupting the MCP JSON-RPC stream on every rescue with decisions to report. Now DialogIcon.Warning (stderr), matching every other message in this class. - A file with zero extracted functions/fields (e.g. only top-level consts/enums) crashed with IndexOutOfRangeException in GetSideGapIndex, silently swallowed by a bare catch - the rescue just never fired for exactly this file shape. Fixed the index bug, and separately made TryMerge decline outright when vanilla has no units at all: the entire document is one "gap" in that case, and gap content always reverts to vanilla on reassembly, so "succeeding" here would have silently discarded both mods' real edits instead of the safe decline this now is. - ReconcileInsertions crashed via ToDictionary when one side inserted two same-named units at the same slot - now declines cleanly instead. - NormalizeGap used plain Trim() instead of the NBSP-safe Trim(WhitespaceChars), reintroducing a bug DiffPlexMergeEngine. NormalizeWhitespace was already fixed for. Now reuses that method directly (promoted to internal) instead of a second, drifted copy. - A gap slot made "ineligible" for precise comparison by a nearby deletion got no signal at all when its content actually differed - silently contradicting this engine's own "never empty content silently" contract. Now emits a conservative, location-described caveat note. - MergeBundleConflictHeadless and MergeTextInteractive (both flat and bundle) never threaded the accumulated-mod-names description MergeFlatConflictHeadless already built, so a bundle-packed or interactively-merged multi-mod chain got a misleading "kept MergedBundleContent's edit" instead of naming the real mods - including in a real GUI modal on the interactive path. All four call sites now share one DescribeAccumulated helper. - Function-level decisions recorded during an earlier, successful chain step still got reported even when a later step failed (or, for a bundle, the repack failed) and the file ended up in summary.Skipped, not summary.Merged - plus a casing mismatch between merge.RelativePath and conflict.RelativePath could break exact-match correlation by a downstream consumer. Decisions are now tracked per relative path and only drained for paths that survived to summary.Merged. - The bare catch around the new engine swallowed every exception with zero trace, making a genuine bug indistinguishable from an expected decline. Narrowed to the one anticipated exception type; anything else is now reported via the notifier instead of vanishing silently. - ScriptUnitExtractor's @addField lookup rescanned from the current position to EOF on every extracted unit - for a large vanilla file with zero @addField annotations (a mod-only construct), this meant millions of wasted line scans per rescue attempt. Now a single forward pass computed once, walked with a monotonic pointer. - Fixed a stale CLAUDE.md claim that ScriptUnitExtractor reuses FileEncoding.cs for file I/O - it takes pre-read strings and does none. Re-validated end-to-end against an isolated copy of the same 5 real previously-failing files used to validate PR #18: all 5 still merge successfully after these fixes, with well-formed output and no conflict markers. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01GXAuGMLB44T5Zv5o5ZzKah --- WitcherScriptMerger.Core/CLAUDE.md | 13 +- .../Inventory/FileMerger.cs | 84 +++++++--- .../Tools/DiffPlexMergeEngine.cs | 57 +++++-- .../Tools/FunctionLevelMergeEngine.cs | 152 +++++++++++++----- .../Tools/ScriptUnitExtractor.cs | 35 ++-- .../Tools/FunctionLevelMergeEngineTests.cs | 73 ++++++++- 6 files changed, 318 insertions(+), 96 deletions(-) diff --git a/WitcherScriptMerger.Core/CLAUDE.md b/WitcherScriptMerger.Core/CLAUDE.md index e53305d..53a5d6c 100644 --- a/WitcherScriptMerger.Core/CLAUDE.md +++ b/WitcherScriptMerger.Core/CLAUDE.md @@ -426,10 +426,15 @@ function body only ever gains brace depth from control flow, never another funct declaration. That structural simplicity is what makes plain brace/paren counting sufficient, as long as it's string/comment-aware (a single masking pass shared by both the brace-safe extraction path and the public `StripComments` helper) so a brace or -paren inside a string literal or comment can never be mistaken for real syntax. Reuses -`Tools/FileEncoding.cs` for all file I/O — mod files are inconsistently encoded even -though vanilla is always UTF-16LE+BOM (see "Text-merge input encoding" below), the exact -same hazard this class's own callers already have to account for. +paren inside a string literal or comment can never be mistaken for real syntax. +`ScriptUnitExtractor` itself does no file I/O at all — `Extract`/`StripComments` take +already-read `string` text — encoding normalization is the caller's job: +`DiffPlexMergeEngine.MergeHeadless` reads via `Tools/FileEncoding.cs` before ever +reaching this class, the same `ReadAnyEncoding` call every other text-merge path already +uses (mod files are inconsistently encoded even though vanilla is always UTF-16LE+BOM — +see "Text-merge input encoding" below). Any future caller that reaches `Extract`/ +`StripComments` directly with raw file bytes, rather than through that existing +encoding-normalized path, would need to normalize first itself. **Per-function resolution (`FunctionLevelMergeEngine.TryMerge`)** tries cheap one-sided shortcuts (unchanged, only-one-side-edited, both-sides-made-the-identical-edit) before diff --git a/WitcherScriptMerger.Core/Inventory/FileMerger.cs b/WitcherScriptMerger.Core/Inventory/FileMerger.cs index d1bceaa..d4a1acd 100644 --- a/WitcherScriptMerger.Core/Inventory/FileMerger.cs +++ b/WitcherScriptMerger.Core/Inventory/FileMerger.cs @@ -128,14 +128,28 @@ public class MergeReportData bool _bundleChanged; List _pendingBundleMerges = new List(); - // Drained into HeadlessMergeSummary.FunctionLevelDecisions at the end of - // MergeConflictsHeadless. Appended to (not overwritten) right after every + // Keyed by relativePath (case-insensitive, matching merge.RelativePath's own + // comparison convention elsewhere in this class), drained into + // HeadlessMergeSummary.FunctionLevelDecisions at the end of + // MergeConflictsHeadless - but ONLY for paths that end up in summary.Merged, + // not summary.Skipped. Appended to (not overwritten) right after every // MergeTextHeadless call, since _mergeEngine.LastFunctionLevelDecisions only // reflects the single most recent pairwise MergeHeadless call - a multi-mod // chain can trigger the function-level rescue at more than one step, and each // one's decisions would otherwise be lost the moment the next chain step's // MergeHeadless call resets that property back to empty. - List _functionLevelDecisions = new List(); + // + // Keyed rather than a flat list (an earlier version of this field was a flat + // List, unconditionally drained in full) because a chain can record + // real decisions for an EARLIER successful step and then fail at a LATER step + // (or, for a bundle, succeed at the text-merge level but fail its later + // blob0.bundle repack) - in either case the file ends up in summary.Skipped, + // and a flat, always-drained list would still report those decisions for a + // file that was never actually merged, contradicting the skipped/merged split + // a caller (e.g. the Vortex extension's merge panel) relies on. Keying by + // relativePath lets the drain step at the end of MergeConflictsHeadless include + // only the entries for paths that actually made it into summary.Merged. + Dictionary> _functionLevelDecisionsByPath = new Dictionary>(StringComparer.OrdinalIgnoreCase); // Anchored at BOTH ends ("^...$") - see IsVanillaDlcBundleFolder's own comment // below for why this matters: it's matched against just the extracted folder-name @@ -296,7 +310,8 @@ void MergeFlatFileInteractive(InteractiveMergeRequest file, Merge merge, bool is var source2 = file.OrderedSources[i]; - var mergedFile = MergeTextInteractive(merge, source1, source2); + var oldDescription = DescribeAccumulated(file.OrderedSources.Take(i).Select(s => s.Name)); + var mergedFile = MergeTextInteractive(merge, source1, source2, oldDescription, source2.Name); if (mergedFile != null) { source1 = MergeSource.FromFlatFile(mergedFile, null); @@ -336,7 +351,8 @@ void MergeBundleFileInteractive(InteractiveMergeRequest file, Merge merge, bool break; } - var mergedFile = MergeTextInteractive(merge, source1, source2); + var oldDescription = DescribeAccumulated(file.OrderedSources.Take(i).Select(s => s.Name)); + var mergedFile = MergeTextInteractive(merge, source1, source2, oldDescription, file.OrderedSources[i].Name); if (mergedFile != null) { source1 = MergeSource.FromFlatFile(mergedFile, null); @@ -352,14 +368,14 @@ void MergeBundleFileInteractive(InteractiveMergeRequest file, Merge merge, bool } } - FileInfo MergeTextInteractive(Merge merge, MergeSource source1, MergeSource source2) + FileInfo MergeTextInteractive(Merge merge, MergeSource source1, MergeSource source2, string oldDescription = null, string newDescription = null) { // Deliberately engine-neutral wording rather than naming KDiff3 explicitly // ("waiting for KDiff3 to close") - no external process or window is involved // at all with DiffPlexMergeEngine. ProgressInfo.CurrentAction = $"Merging {source1.Name} && {source2.Name}"; - var result = _mergeEngine.Merge(source1, source2, _vanillaFile, _outputPath); + var result = _mergeEngine.Merge(source1, source2, _vanillaFile, _outputPath, oldDescription, newDescription); if (result != MergeEngineResult.AutoSolved) return null; @@ -548,7 +564,22 @@ public HeadlessMergeSummary MergeConflictsHeadless( } } - summary.FunctionLevelDecisions.AddRange(_functionLevelDecisions); + // Only for paths that survived to summary.Merged - see + // _functionLevelDecisionsByPath's own comment for why a flat, unconditional + // drain here would misattribute decisions to a file that ultimately failed + // (a later chain step, or a bundle repack, both handled above this point). + // Uses summary.Merged's own casing for the output prefix, not + // merge.RelativePath's - `merge` can be an existing record pulled from + // _inventory.Merges via a case-insensitive match (see the isNew branch + // above), whose stored RelativePath could differ in casing from the + // freshly-scanned conflict.RelativePath that summary.Merged actually holds; + // the dictionary lookup itself is case-insensitive either way. + foreach (var relativePath in summary.Merged) + { + if (_functionLevelDecisionsByPath.TryGetValue(relativePath, out var decisionsForThisPath)) + foreach (var decision in decisionsForThisPath) + summary.FunctionLevelDecisions.Add(relativePath + ": " + decision); + } CleanUpTempFiles(); CleanUpEmptyDirectories(); @@ -587,32 +618,34 @@ bool MergeFlatConflictHeadless(ModFile conflict, Merge merge, string mergedModNa _vanillaFile = new FileInfo(conflict.GetVanillaFile()); - // Tracked locally, independent of merge.Mods (which can carry stale entries - // from a previous run when merge is a re-merge pulled from _inventory.Merges - // rather than freshly created) - this is only ever the real mod names folded - // into source1 so far within THIS chain, for FunctionLevelMergeEngine's - // Decisions[] audit text (see DiffPlexMergeEngine.TryFunctionLevelRescue's - // own comment on why source1.Name alone is misleading past the first step). - var accumulatedModNames = new List { orderedNames[0] }; - for (int i = 1; i < orderedNames.Length; ++i) { var hash = conflict.Mods.First(h => h.Name.EqualsIgnoreCase(orderedNames[i])); var source2 = MergeSource.FromFlatFile(new FileInfo(conflict.GetModFile(orderedNames[i])), hash); - var oldDescription = accumulatedModNames.Count > 1 - ? "accumulated merge (" + string.Join(", ", accumulatedModNames) + ")" - : accumulatedModNames[0]; - - var mergedFile = MergeTextHeadless(merge, source1, source2, dryRun, oldDescription, orderedNames[i]); + var mergedFile = MergeTextHeadless(merge, source1, source2, dryRun, DescribeAccumulated(orderedNames.Take(i)), orderedNames[i]); if (mergedFile == null) return false; source1 = MergeSource.FromFlatFile(mergedFile, null); - accumulatedModNames.Add(orderedNames[i]); } return true; } + // The real mod names folded into "source1" so far within a merge chain, for + // FunctionLevelMergeEngine's Decisions[] audit text (see DiffPlexMergeEngine. + // TryFunctionLevelRescue's own comment on why source1.Name alone is misleading + // past a chain's first step - source1 becomes the prior step's accumulated + // output, whose own MergeSource.Name resolves to the merged-mod folder, not a + // real contributing mod). Deliberately takes namesSoFar fresh from the caller's + // own already-authoritative ordered list (orderedNames.Take(i) / OrderedSources. + // Take(i).Select(s => s.Name)) rather than a separately maintained list that + // would just be redundantly re-deriving the same prefix. + static string DescribeAccumulated(IEnumerable namesSoFar) + { + var names = namesSoFar.ToList(); + return names.Count > 1 ? "accumulated merge (" + string.Join(", ", names) + ")" : names[0]; + } + bool MergeBundleConflictHeadless(ModFile conflict, Merge merge, string[] orderedNames, bool dryRun) { merge.BundleName = Path.GetFileName(Paths.RetrieveMergedBundlePath()); @@ -645,7 +678,7 @@ bool MergeBundleConflictHeadless(ModFile conflict, Merge merge, string[] ordered if (!GetUnpackedFiles(conflict.RelativePath, ref source1, ref source2)) return false; - var mergedFile = MergeTextHeadless(merge, source1, source2, dryRun); + var mergedFile = MergeTextHeadless(merge, source1, source2, dryRun, DescribeAccumulated(orderedNames.Take(i)), orderedNames[i]); if (mergedFile == null) return false; source1 = MergeSource.FromFlatFile(mergedFile, null); @@ -746,8 +779,9 @@ FileInfo MergeTextHeadless(Merge merge, MergeSource source1, MergeSource source2 if (_mergeEngine.LastFunctionLevelDecisions.Count > 0) { - foreach (var decision in _mergeEngine.LastFunctionLevelDecisions) - _functionLevelDecisions.Add(merge.RelativePath + ": " + decision); + if (!_functionLevelDecisionsByPath.TryGetValue(merge.RelativePath, out var decisionsForThisPath)) + _functionLevelDecisionsByPath[merge.RelativePath] = decisionsForThisPath = new List(); + decisionsForThisPath.AddRange(_mergeEngine.LastFunctionLevelDecisions); } if (result != MergeEngineResult.AutoSolved) diff --git a/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs b/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs index 7d7bc70..38900c9 100644 --- a/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs +++ b/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs @@ -8,6 +8,7 @@ using DiffPlex; using DiffPlex.Chunkers; using DiffPlex.Model; +using WitcherScriptMerger.FileIndex; using WitcherScriptMerger.Inventory; namespace WitcherScriptMerger.Tools @@ -232,7 +233,7 @@ public MergeEngineResult MergeHeadless( // merge even when the whole-file 3-way diff hits this bug. Only // attempted for .ws files - the extractor is WitcherScript-specific and // has no notion of XML structure. - if (TryFunctionLevelRescue(baseText, oldText, newText, source1, source2, outputPath, oldDescription, newDescription)) + if (TryFunctionLevelRescue(baseText, oldText, newText, source1, source2, outputPath, oldDescription, newDescription, openConflictMarkers)) return MergeEngineResult.AutoSolved; // DiffPlex's own diff algorithm produced output it isn't safe to trust @@ -270,7 +271,7 @@ public MergeEngineResult MergeHeadless( // FunctionLevelMergeEngine's own comment for why this is a fallback that // only ever activates where the whole-file merge has already failed, never // a parallel code path for merges that would have succeeded anyway. - if (TryFunctionLevelRescue(baseText, oldText, newText, source1, source2, outputPath, oldDescription, newDescription)) + if (TryFunctionLevelRescue(baseText, oldText, newText, source1, source2, outputPath, oldDescription, newDescription, openConflictMarkers)) return MergeEngineResult.AutoSolved; // Never write conflict markers to outputPath itself: FileMerger's headless @@ -417,9 +418,13 @@ static void DeleteIfExists(string path) bool TryFunctionLevelRescue( string baseText, string oldText, string newText, FileMerger.MergeSource source1, FileMerger.MergeSource source2, string outputPath, - string oldDescription, string newDescription) + string oldDescription, string newDescription, bool openConflictMarkers) { - if (!Path.GetExtension(outputPath).EqualsIgnoreCase(".ws")) + // ModFile.IsScript, not a locally reinvented extension check - the same + // vocabulary every other file-category dispatch in Core uses for this exact + // question. The extractor is WitcherScript-specific and has no notion of + // XML structure, so .xml conflicts never reach it. + if (!ModFile.IsScript(outputPath)) return false; FunctionLevelMergeResult result; @@ -430,13 +435,28 @@ bool TryFunctionLevelRescue( source1.Name, source2.Name, oldDescription ?? source1.Name, newDescription ?? source2.Name); } - catch + catch (ScriptUnitExtractor.ExtractionException) { - // A latent bug in the new engine must never regress this method below - // its pre-existing behavior - the caller falls through to whatever it - // was already about to do (write a sidecar, or report the - // DiffAlgorithmException as-is) exactly as if this rescue attempt had - // declined outright. + // The one expected, anticipated decline case (FunctionLevelMergeEngine. + // TryMerge itself already narrows to this same exception type) - + // genuinely just means this input doesn't parse cleanly, not a bug. + return false; + } + catch (Exception ex) + { + // Anything else is a genuine defect in the new engine, not an + // anticipated decline - still can't be allowed to regress this method + // below its pre-existing behavior (the caller falls through to + // whatever it was already about to do), but silently swallowing it + // with zero trace would make such a bug permanently unmeasurable from + // field reports alone. DialogIcon.Warning (not Information) so this + // routes to stderr under HeadlessMergeNotifier, never stdout - stdout + // carries MCP JSON-RPC frames only when running under the mcp verb, + // and writing arbitrary text there would corrupt the protocol stream. + AppState.Notifier.ShowMessage( + $"Function-level merge rescue hit an unexpected error for {source1.Name} + {source2.Name} " + + $"({ex.GetType().Name}: {ex.Message}) - falling back to the whole-file result.", + "Function-level rescue error", NotifyButtons.OK, DialogIcon.Warning); return false; } @@ -449,10 +469,17 @@ bool TryFunctionLevelRescue( if (result.Decisions.Count > 0) { + // DialogIcon.Warning, not Information - see the unexpected-exception + // branch above for why Information (which HeadlessMergeNotifier routes + // to stdout) isn't safe here either; this message fires on every + // successful rescue with decisions to report, including during a dry + // run (openConflictMarkers is false only for the dry-run caller), so + // it's reachable far more often than the exception-logging branch. + var previewSuffix = openConflictMarkers ? "" : " (dry run preview - nothing was actually written)"; AppState.Notifier.ShowMessage( $"Merged {source1.Name} + {source2.Name} at the function level after the whole-file merge " + - $"couldn't auto-solve it:\n\n" + string.Join("\n", result.Decisions), - "Merged (function-level)", NotifyButtons.OK, DialogIcon.Information); + $"couldn't auto-solve it{previewSuffix}:\n\n" + string.Join("\n", result.Decisions), + "Merged (function-level)", NotifyButtons.OK, DialogIcon.Warning); } return true; @@ -696,7 +723,11 @@ static bool IsWhitespaceOnlyDifference(IReadOnlyList oldPieces, IReadOnl return NormalizeWhitespace(oldPieces) == NormalizeWhitespace(newPieces); } - static string NormalizeWhitespace(IEnumerable pieces) + // Internal, not private: FunctionLevelMergeEngine.NormalizeGap reuses this + // directly (a single-element pieces array) rather than keeping its own second + // copy of the same regex+trim logic - see that method's own comment for the + // real NBSP-related regression duplicating it once already caused. + internal static string NormalizeWhitespace(IEnumerable pieces) { // Trim(WhitespaceChars), not the parameterless Trim() - see WhitespaceChars' // own comment for the real NBSP-related bug this guards against. diff --git a/WitcherScriptMerger.Core/Tools/FunctionLevelMergeEngine.cs b/WitcherScriptMerger.Core/Tools/FunctionLevelMergeEngine.cs index 76f161e..4c2c059 100644 --- a/WitcherScriptMerger.Core/Tools/FunctionLevelMergeEngine.cs +++ b/WitcherScriptMerger.Core/Tools/FunctionLevelMergeEngine.cs @@ -1,7 +1,6 @@ using System.Collections.Generic; using System.Linq; using System.Text; -using System.Text.RegularExpressions; using DiffPlex; namespace WitcherScriptMerger.Tools @@ -52,8 +51,6 @@ public FunctionLevelMergeResult(bool applied, string mergedText, IReadOnlyList ReconcileInsertions( if (oldInsertions.Count == 0 && newInsertions.Count == 0) return new List(); + // Two insertions with the same name on ONE side (e.g. a mod's own + // copy-paste mistake) is an ambiguity this method can't safely resolve - + // which occurrence did the mod author actually intend? Decline rather than + // guess, same policy as the same-name-different-body-across-sides case + // below. Also avoids ToDictionary throwing on the duplicate key. + if (HasDuplicateNames(oldInsertions) || HasDuplicateNames(newInsertions)) + return null; + var newByName = newInsertions.ToDictionary(u => u.Name); var consumedNewNames = new HashSet(); var result = new List(); @@ -270,44 +308,81 @@ static List ReconcileInsertions( return result; } + static bool HasDuplicateNames(List units) => units.Select(u => u.Name).Distinct().Count() != units.Count; + #endregion #region Gap comparison - // A slot is only compared when both its neighboring vanilla units (if any) are - // present, unmatched-to-nothing, on both sides, and neither side inserted - // anything at this slot - i.e. the simple, overwhelmingly common case (per this - // feature's own real-data measurement: the large majority of a file's gaps sit - // between two functions neither mod touched structurally). Once an insertion or - // deletion touches a slot's boundary, "the equivalent gap on each side" stops - // being a single well-defined span to compare - deferred rather than guessed at. - static bool IsGapComparisonEligible(UnitAlignment oldAlignment, UnitAlignment newAlignment, int slot, int vanillaCount) + enum GapEligibility + { + Eligible, + IneligibleInsertion, + IneligibleDeletion, + } + + // A slot is only precisely compared when both its neighboring vanilla units (if + // any) are present, unmatched-to-nothing, on both sides, and neither side + // inserted anything at this slot - i.e. the simple, overwhelmingly common case + // (per this feature's own real-data measurement: the large majority of a file's + // gaps sit between two functions neither mod touched structurally). Once an + // insertion or deletion touches a slot's boundary, "the equivalent gap on each + // side" stops being a single well-defined span to compare - deferred rather than + // guessed at, but NOT silently: an insertion is already visible in the + // reassembled output (no note needed), while a deletion gets a conservative + // caveat note from TryMerge's caller (see GapEligibility.IneligibleDeletion's + // call site) since non-function content near it has no signal at all otherwise. + static GapEligibility GetGapEligibility(UnitAlignment oldAlignment, UnitAlignment newAlignment, int slot, int vanillaCount) { if (oldAlignment.InsertionsAtSlot[slot].Count > 0 || newAlignment.InsertionsAtSlot[slot].Count > 0) - return false; + return GapEligibility.IneligibleInsertion; if (slot > 0 && (!oldAlignment.MatchedSideIndex[slot - 1].HasValue || !newAlignment.MatchedSideIndex[slot - 1].HasValue)) - return false; + return GapEligibility.IneligibleDeletion; if (slot < vanillaCount && (!oldAlignment.MatchedSideIndex[slot].HasValue || !newAlignment.MatchedSideIndex[slot].HasValue)) - return false; - return true; + return GapEligibility.IneligibleDeletion; + return GapEligibility.Eligible; } - // Only valid when IsGapComparisonEligible(slot) is true, which guarantees - // MatchedSideIndex[slot] (or [slot - 1], for the final slot) has a value. + // Only valid when GetGapEligibility(slot) is Eligible, which guarantees a + // meaningful gap index exists on this side for the requested slot. slot == 0 is + // always gap index 0 outright - the leading gap exists at a fixed position + // regardless of alignment, unlike every other slot, which is anchored to a + // matched vanilla unit's own index. (A prior version of this method derived + // slot 0 via the same "matched unit's own index" branch used for slot 1.. + // vanillaCount-1, which happened to also produce 0 whenever vanillaCount > 0 - + // but that branch requires slot < vanillaCount, which is false whenever + // vanillaCount == 0, falling through to the "final slot" branch below and + // indexing MatchedSideIndex[-1] on an empty array. A file with zero extracted + // functions/fields - e.g. one containing only top-level consts/enums - is a + // real, reachable case, not hypothetical.) static int GetSideGapIndex(UnitAlignment alignment, int slot, int vanillaCount) { - if (slot < vanillaCount && alignment.MatchedSideIndex[slot].HasValue) + if (slot == 0) + return 0; + if (slot < vanillaCount) return alignment.MatchedSideIndex[slot].Value; return alignment.MatchedSideIndex[slot - 1].Value + 1; } + static string DescribeSlot(IReadOnlyList vanillaUnits, int slot, int vanillaCount) + { + if (vanillaCount == 0) + return "in this file"; + if (slot == 0) + return $"before {vanillaUnits[0].Name}"; + if (slot == vanillaCount) + return $"after {vanillaUnits[vanillaCount - 1].Name}"; + return $"between {vanillaUnits[slot - 1].Name} and {vanillaUnits[slot].Name}"; + } + // Reassembly always keeps vanilla's own gap text verbatim (deterministic, // matches DiffPlexMergeEngine's own "take one side" precedent elsewhere) - this // only ever adds an audit note when a side's gap content differs from vanilla's - // by more than whitespace/comments, since that's real, non-mechanical content - // (typically a mod author's own comment) silently not making it into the merged - // output. A purely whitespace/comment difference is never noted - that's exactly - // the class of noise this whole engine exists to stop treating as meaningful. + // by more than whitespace, since that's real, non-mechanical content (a + // comment, but just as easily a default value or an undecorated var - gap + // content isn't only comments) silently not making it into the merged output. A + // purely whitespace difference is never noted - that's exactly the class of + // noise this whole engine exists to stop treating as meaningful. static void NoteGapMismatchIfAny(string baseGap, string oldGap, string newGap, string oldDescription, string newDescription, List decisions) { var baseNorm = NormalizeGap(baseGap); @@ -315,18 +390,23 @@ static void NoteGapMismatchIfAny(string baseGap, string oldGap, string newGap, s var newDiffers = NormalizeGap(newGap) != baseNorm; if (oldDiffers) - decisions.Add($"a comment from {oldDescription} near this position was not preserved (vanilla formatting/comments kept)."); + decisions.Add($"content from {oldDescription} near this position was not preserved (vanilla formatting/content kept)."); if (newDiffers) - decisions.Add($"a comment from {newDescription} near this position was not preserved (vanilla formatting/comments kept)."); + decisions.Add($"content from {newDescription} near this position was not preserved (vanilla formatting/content kept)."); } // Deliberately whitespace-collapse only, NOT comment-stripped: this feeds the - // note above, whose whole point is to detect when comment CONTENT differs, not - // just formatting - stripping comments first would blank away the very thing - // being compared, silently defeating the check (caught by - // TryMerge_GapCommentDifference_NotedButVanillaGapTextKept). Matches - // DiffPlexMergeEngine.NormalizeWhitespace's own whitespace-only spirit. - static string NormalizeGap(string text) => WhitespaceRun.Replace(text, " ").Trim(); + // note above, whose whole point is to detect when gap CONTENT differs, not just + // formatting - stripping comments first would blank away the very thing being + // compared, silently defeating the check (caught by + // TryMerge_GapCommentDifference_NotedButVanillaGapTextKept). Reuses + // DiffPlexMergeEngine.NormalizeWhitespace directly rather than a second, private + // copy of the same regex+trim logic - an earlier version of this method did + // duplicate it, using the parameterless Trim() instead of NormalizeWhitespace's + // deliberate Trim(WhitespaceChars), which silently reintroduced the exact + // NBSP-vs-space false-equivalence bug that method's own comment documents + // fixing (flagged in code review). + static string NormalizeGap(string text) => DiffPlexMergeEngine.NormalizeWhitespace(new[] { text }); #endregion } diff --git a/WitcherScriptMerger.Core/Tools/ScriptUnitExtractor.cs b/WitcherScriptMerger.Core/Tools/ScriptUnitExtractor.cs index e9340bf..9a7bc9d 100644 --- a/WitcherScriptMerger.Core/Tools/ScriptUnitExtractor.cs +++ b/WitcherScriptMerger.Core/Tools/ScriptUnitExtractor.cs @@ -116,16 +116,28 @@ public static ScriptDocument Extract(string text) var kinds = ClassifySpans(text); var mask = BuildMask(text, kinds, blankStringsToo: true); var lineStarts = ComputeLineStarts(text); + var addFieldLineStarts = FindAllAddFieldAnnotationLineStarts(mask, lineStarts); var gaps = new List(); var units = new List(); var cursor = 0; var pos = 0; + // Monotonic pointer into addFieldLineStarts, not a fresh scan per unit - + // see that list's own comment for the real O(units * remaining lines) cost + // a per-call rescan used to have (a vanilla file has zero @addField + // annotations at all, since it's a mod-only construct, so every one of a + // large vanilla file's function extractions used to scan all the way to + // EOF just to confirm that). pos only ever increases across iterations, so + // this index never needs to rewind. + var addFieldIndex = 0; while (pos <= text.Length) { var funcMatch = DeclarationRegex.Match(mask, pos); - var fieldLineStart = FindNextAddFieldAnnotationLineStart(mask, lineStarts, pos); + + while (addFieldIndex < addFieldLineStarts.Count && addFieldLineStarts[addFieldIndex] < pos) + ++addFieldIndex; + var fieldLineStart = addFieldIndex < addFieldLineStarts.Count ? (int?)addFieldLineStarts[addFieldIndex] : null; var funcPos = funcMatch.Success ? funcMatch.Index : int.MaxValue; var fieldPos = fieldLineStart ?? int.MaxValue; @@ -277,24 +289,25 @@ static int ExtendStartBackwardOverAnnotations(string mask, List lineStarts, return resultStart; } - static int? FindNextAddFieldAnnotationLineStart(string mask, List lineStarts, int pos) + // A single O(lines) forward pass over the whole document, computed once per + // Extract call - not a fresh scan-to-EOF per extracted unit (see Extract's own + // comment on why that mattered). lineEnd is computed directly from the loop's + // own lineIndex rather than via GetLineEnd (which would redundantly re-derive + // that same index through a binary search). + static List FindAllAddFieldAnnotationLineStarts(string mask, List lineStarts) { - var lineIndex = GetLineIndex(lineStarts, pos); - if (lineStarts[lineIndex] < pos) - ++lineIndex; - - for (; lineIndex < lineStarts.Count; ++lineIndex) + var result = new List(); + for (var lineIndex = 0; lineIndex < lineStarts.Count; ++lineIndex) { var lineStart = lineStarts[lineIndex]; - var lineEnd = GetLineEnd(mask, lineStarts, lineStart); + var lineEnd = lineIndex + 1 < lineStarts.Count ? lineStarts[lineIndex + 1] : mask.Length; var content = mask.Substring(lineStart, lineEnd - lineStart).Trim(); if (content.Length == 0) continue; if (AddFieldAnnotationRegex.IsMatch(content)) - return lineStart; + result.Add(lineStart); } - - return null; + return result; } #endregion diff --git a/WitcherScriptMerger.Tests/Tools/FunctionLevelMergeEngineTests.cs b/WitcherScriptMerger.Tests/Tools/FunctionLevelMergeEngineTests.cs index 49f796a..5aad082 100644 --- a/WitcherScriptMerger.Tests/Tools/FunctionLevelMergeEngineTests.cs +++ b/WitcherScriptMerger.Tests/Tools/FunctionLevelMergeEngineTests.cs @@ -121,7 +121,7 @@ public void TryMerge_GenuineCollisionExactTie_FallsBackDeterministicallyToOldSid } [Fact] - public void TryMerge_DeletedOnOldSideOnly_NewSideAlsoUnchanged_FunctionDroppedNoDecisionNote() + public void TryMerge_DeletedOnOldSideOnly_NewSideAlsoUnchanged_FunctionDroppedGapsGetCaveatNotes() { var baseText = Fn("A", "\tx = 1;\r\n") + Fn("B", "\ty = 2;\r\n"); var oldText = Fn("B", "\ty = 2;\r\n"); // deleted A entirely, didn't touch B @@ -131,7 +131,12 @@ public void TryMerge_DeletedOnOldSideOnly_NewSideAlsoUnchanged_FunctionDroppedNo Assert.True(result.Applied); Assert.DoesNotContain("function A", result.MergedText); Assert.Contains("function B", result.MergedText); - Assert.Empty(result.Decisions); + // A's deletion makes both neighboring gap slots (before A, between A and B) + // ineligible for precise comparison - each gets a conservative caveat note + // rather than silence, since non-function content near a deletion has no + // other signal at all (see GetGapEligibility.IneligibleDeletion). + Assert.Equal(2, result.Decisions.Count); + Assert.All(result.Decisions, d => Assert.Contains("wasn't automatically verified", d)); } [Fact] @@ -145,13 +150,16 @@ public void TryMerge_EditSurvivesCompetingDeletion_KeepsEditAndRecordsDecision() Assert.True(result.Applied); Assert.Contains("x = 99;", result.MergedText); - var note = Assert.Single(result.Decisions); - Assert.Contains("modB", note); - Assert.Contains("deleted", note); + var deletionNote = Assert.Single(result.Decisions, d => d.Contains("deleted")); + Assert.Contains("modB", deletionNote); + // Plus the same two ineligible-deletion caveat notes as the case above - + // A's deletion on the old side still makes its neighboring gaps + // unverifiable regardless of how the function itself was resolved. + Assert.Equal(3, result.Decisions.Count); } [Fact] - public void TryMerge_DeletedOnBothSides_FunctionDropped() + public void TryMerge_DeletedOnBothSides_FunctionDroppedGapsGetCaveatNotes() { var baseText = Fn("A", "\tx = 1;\r\n") + Fn("B", "\ty = 2;\r\n"); var bothDeleteA = Fn("B", "\ty = 2;\r\n"); @@ -160,7 +168,23 @@ public void TryMerge_DeletedOnBothSides_FunctionDropped() Assert.True(result.Applied); Assert.DoesNotContain("function A", result.MergedText); - Assert.Empty(result.Decisions); + Assert.Equal(2, result.Decisions.Count); + Assert.All(result.Decisions, d => Assert.Contains("wasn't automatically verified", d)); + } + + [Fact] + public void TryMerge_NoDeletionsOrInsertionsAnywhere_NoIneligibleGapCaveatNotes() + { + // Sanity check for the caveat-note feature itself: a file where nothing is + // ever deleted or inserted should never emit an "wasn't automatically + // verified" caveat - only real, deletion-adjacent uncertainty should. + var baseText = Fn("A", "\tx = 1;\r\n") + Fn("B", "\ty = 2;\r\n"); + var oldText = Fn("A", "\tx = 9;\r\n") + Fn("B", "\ty = 2;\r\n"); + + var result = Merge(baseText, oldText, baseText); + + Assert.True(result.Applied); + Assert.DoesNotContain(result.Decisions, d => d.Contains("wasn't automatically verified")); } [Fact] @@ -200,6 +224,41 @@ public void TryMerge_SameNameDifferentBodyInsertionOnBothSides_DeclinesWholeFile Assert.False(result.Applied); } + [Fact] + public void TryMerge_DuplicateNamedInsertionsOnOneSide_DeclinesRatherThanThrowing() + { + // Two insertions with the SAME name on one side (e.g. a mod's own + // copy-paste mistake) used to throw from ReconcileInsertions's + // ToDictionary call - regression test for that crash (caught upstream by + // DiffPlexMergeEngine.TryFunctionLevelRescue's bare catch, but this engine + // should decline cleanly on its own, not rely on a caller's safety net). + var baseText = Fn("A", "\tx = 1;\r\n"); + var oldText = Fn("A", "\tx = 1;\r\n") + Fn("Dup", "\ty = 1;\r\n") + Fn("Dup", "\ty = 2;\r\n"); + + var result = Merge(baseText, oldText, baseText); + + Assert.False(result.Applied); + } + + [Fact] + public void TryMerge_VanillaHasNoExtractedUnitsAtAll_DeclinesRatherThanDiscardingBothEdits() + { + // A file with zero functions/@addField fields (e.g. only top-level + // consts/enums) has nothing for a FUNCTION-level engine to offer - the + // whole document is one gap, and reverting a whole file's real, + // substantive edits to vanilla while reporting it as a successful + // AutoSolved merge would be a materially worse outcome than declining. + // Also a regression test for a real crash this case used to trigger + // (GetSideGapIndex indexing MatchedSideIndex[-1] on an empty array). + var baseText = "const X = 1;\r\n"; + var oldText = "const X = 2;\r\n"; + var newText = "const X = 3;\r\n"; + + var result = Merge(baseText, oldText, newText); + + Assert.False(result.Applied); + } + [Fact] public void TryMerge_ExtractionFailsOnAnySide_Declines() {