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() {