From b24f43ee12bfe3e86688c70d35c18a729d27d357 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Thu, 13 Aug 2026 15:29:16 -0400 Subject: [PATCH 01/11] Match lift-ranges range-element on guid rather than id A range-element's id is the possibility's own name, so it moves whenever that name is renamed or respelled. Matching only on the id turns such a move into a deletion plus an addition: the other user's edits to the same element raise a spurious removed-vs-edited conflict, and the merge output can carry two elements for one possibility. Prefer the guid FLEx writes, falling back to the id when either element lacks a guid, since LIFT makes the guid optional. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 1 + .../LiftRangesElementStrategiesMethod.cs | 9 +- .../merge/xml/generic/FindNodeToMerge.cs | 95 +++++++++++++++ .../LiftRanges/LiftRangesFileHandlerTests.cs | 109 ++++++++++++++++++ 4 files changed, 213 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 34c276c4a..010c05099 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed +- [SIL.Chorus.LibChorus] Match `range-element` in `.lift-ranges` on its `guid` rather than its `id`, so that renaming or respelling a range element merges as an edit instead of a deletion plus an addition - Prevent S&R to Internet without full URL - [SIL.Chorus.LibChorus] Correctly handle & and other special characters in passwords - [SIL.Chorus] Fix collection-modified exception when UsbDrives property is read while background scan thread updates the list diff --git a/src/LibChorus/FileTypeHandlers/Lift-Ranges/LiftRangesElementStrategiesMethod.cs b/src/LibChorus/FileTypeHandlers/Lift-Ranges/LiftRangesElementStrategiesMethod.cs index 56cfdaf58..474a8c0d0 100644 --- a/src/LibChorus/FileTypeHandlers/Lift-Ranges/LiftRangesElementStrategiesMethod.cs +++ b/src/LibChorus/FileTypeHandlers/Lift-Ranges/LiftRangesElementStrategiesMethod.cs @@ -42,7 +42,14 @@ internal static void AddLiftRangeElementStrategies(MergeStrategies mergeStrategi // This element appears to not be in the main lift file, so it will be 'extra', but ought not cause harm. // [Optional, multitext] holds zero or more
elements diff --git a/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs b/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs index 5656ebcd2..df176d44b 100644 --- a/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs +++ b/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs @@ -210,6 +210,101 @@ public string GetWarningMessageForAmbiguousNodes(XmlNode nodeForMessage) } } + /// + /// Search for a matching element using an optional attribute that identifies it permanently (a guid), + /// falling back to an ordinary key attribute when either element lacks that attribute. + /// + /// + /// Use this where the ordinary key is derived from user data, and so can be respelled without the + /// underlying object having changed. Matching on such a key alone turns a respelling into a deletion + /// plus an addition, which discards the other user's edits to the same element and can leave two + /// elements standing for one object. + /// + public class FindByPreferredKeyAttribute : IFindMatchingNodesToMerge + { + private readonly string _preferredKeyAttribute; + private readonly string _fallbackKeyAttribute; + + public FindByPreferredKeyAttribute(string preferredKeyAttribute, string fallbackKeyAttribute) + { + _preferredKeyAttribute = preferredKeyAttribute; + _fallbackKeyAttribute = fallbackKeyAttribute; + } + + public XmlNode GetNodeToMerge(XmlNode nodeToMatch, XmlNode parentToSearchIn, HashSet acceptableTargets) + { + if (parentToSearchIn == null) + return null; + + foreach (var match in GetMatchingNodes(nodeToMatch, parentToSearchIn)) + { + if (acceptableTargets.Contains(match)) + return match; + } + return null; + } + + /// + /// Get all matching nodes, or an empty collection, if there are no matches. + /// + /// A collection of zero, or more, matching nodes. + /// may, or may not, be a child of . + public IEnumerable GetMatchingNodes(XmlNode nodeToMatch, XmlNode parentToSearchIn) + { + var matches = new List(); + if (nodeToMatch == null || parentToSearchIn == null) + return matches; + + var preferredKey = XmlUtilities.GetOptionalAttributeString(nodeToMatch, _preferredKeyAttribute); + var fallbackKey = XmlUtilities.GetOptionalAttributeString(nodeToMatch, _fallbackKeyAttribute); + if (string.IsNullOrEmpty(preferredKey) && string.IsNullOrEmpty(fallbackKey)) + return matches; + + foreach (XmlNode childNode in parentToSearchIn.ChildNodes) + { + if (childNode.NodeType != XmlNodeType.Element) + continue; + if (nodeToMatch == childNode) + { + matches.Add(childNode); + continue; + } + if (childNode.Name != nodeToMatch.Name) + continue; + if (IsMatch(childNode, preferredKey, fallbackKey)) + matches.Add(childNode); + } + return matches; + } + + private bool IsMatch(XmlNode candidate, string preferredKey, string fallbackKey) + { + var candidatePreferredKey = XmlUtilities.GetOptionalAttributeString(candidate, _preferredKeyAttribute); + // When both carry the permanent key it decides on its own, since the fallback key may have moved. + if (!string.IsNullOrEmpty(preferredKey) && !string.IsNullOrEmpty(candidatePreferredKey)) + return preferredKey == candidatePreferredKey; + if (string.IsNullOrEmpty(fallbackKey)) + return false; + return fallbackKey == XmlUtilities.GetOptionalAttributeString(candidate, _fallbackKeyAttribute); + } + + /// + /// Get a basic message that is suitable for use in a warning report where ambiguous nodes are found in the same parent node. + /// + /// A message string or null/empty string, if no message is needed for ambiguous nodes. + public string GetWarningMessageForAmbiguousNodes(XmlNode nodeForMessage) + { + Guard.AgainstNull(nodeForMessage, "nodeForMessage"); + + var preferredKey = XmlUtilities.GetOptionalAttributeString(nodeForMessage, _preferredKeyAttribute); + return string.IsNullOrEmpty(preferredKey) + ? string.Format("The key attribute '{0}' has values that are the same '{1}'", + _fallbackKeyAttribute, XmlUtilities.GetOptionalAttributeString(nodeForMessage, _fallbackKeyAttribute)) + : string.Format("The key attribute '{0}' has values that are the same '{1}'", + _preferredKeyAttribute, preferredKey); + } + } + /// /// Assuming the children of the parent to search in form a list (order matters, duplicates allowed), and so do the /// children of the nodeToMatch, find the corresponding object in the list. A corresponding node will have the same key, diff --git a/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs b/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs index fdf060820..83c7187b6 100644 --- a/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs +++ b/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs @@ -7,6 +7,7 @@ using NUnit.Framework; using SIL.IO; using SIL.Progress; +using SIL.TestUtilities; namespace LibChorus.Tests.FileHandlers.LiftRanges { @@ -251,6 +252,114 @@ public void BothEditWithConflictAndWeWin() Assert.AreEqual(ours.Replace("\r\n", "\n"), result); } + /// + /// A range-element id is the possibility's own name, so FLEx respelling names on export + /// (LT-22697 normalizes them) moves the id without the possibility having changed. + /// + [Test] + public void RespelledIdMergesAsAnEditWhenTheGuidIsUnchanged() + { + var common = RangesWithPartOfSpeech(kDecomposedName, kPosGuid, "old"); + // We upgraded, so our export normalizes the id. + var ours = RangesWithPartOfSpeech(kComposedName, kPosGuid, "old"); + // They did not upgrade, and they edited the abbreviation. + var theirs = RangesWithPartOfSpeech(kDecomposedName, kPosGuid, "new"); + + var result = DoMerge(common, ours, theirs, 0, 2); + + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath("//range/range-element", 1); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( + string.Format("//range-element[@guid='{0}' and @id='{1}']", kPosGuid, kComposedName), 1); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( + "//range-element/abbrev/form/text[text()='new']", 1); + } + + /// + /// The guid is optional in LIFT, so a file written without one must still merge on the id. + /// + [Test] + public void RangeElementWithoutAGuidStillMergesOnItsId() + { + var common = RangesWithPartOfSpeech("Noun", null, "old"); + var ours = RangesWithPartOfSpeech("Noun", null, "old"); + var theirs = RangesWithPartOfSpeech("Noun", null, "new"); + + var result = DoMerge(common, ours, theirs, 0, 0); + + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath("//range/range-element", 1); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( + "//range-element/abbrev/form/text[text()='new']", 1); + } + + /// + /// Two possibilities that happen to share a name are still two possibilities. Matching them + /// on the id would merge them into one and lose a guid. + /// + [Test] + public void RangeElementsWithDifferentGuidsAreNotMatched() + { + const string theirGuid = "9c4d3e2b-77a6-4b1e-8d55-6a0f2c3e14bb"; + const string common = +@" + + +"; + var ours = RangesWithPartOfSpeech("Noun", kPosGuid, "n"); + var theirs = RangesWithPartOfSpeech("Noun", theirGuid, "n"); + + var result = DoMerge(common, ours, theirs, 0, 2); + + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath("//range/range-element", 2); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( + string.Format("//range-element[@guid='{0}']", kPosGuid), 1); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( + string.Format("//range-element[@guid='{0}']", theirGuid), 1); + } + + /// + /// A merge done before the guid was preferred could leave one possibility standing as two + /// range-elements, spelled differently. Matching on the guid makes them ambiguous siblings, + /// which the merger collapses back to one. + /// + [Test] + public void RangeElementsDuplicatedByAnEarlierMergeCollapseToOne() + { + var duplicated = string.Format( +@" + + + +old + + +
old
 + + +", kDecomposedName, kComposedName, kPosGuid); + + var result = DoMerge(duplicated, duplicated, duplicated, 0, 0); + + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath("//range/range-element", 1); + Assert.That(_eventListener.Warnings, Is.Not.Empty, "the dropped duplicate should be reported"); + } + + private const string kDecomposedName = "Compléments"; // e + combining acute + private const string kComposedName = "Compléments"; // precomposed e-acute + private const string kPosGuid = "e8c4b4b0-1a2f-4f9e-9f39-3f2b0d8f7a11"; + + private static string RangesWithPartOfSpeech(string id, string guid, string abbrev) + { + return string.Format( +@" + + + +
{2}
 + + +", id, guid == null ? string.Empty : string.Format(" guid='{0}'", guid), abbrev); + } + private string DoMerge(string commonAncestor, string ourContent, string theirContent, int expectedConflictCount, int expectedChangesCount) { From e6d58f3cc5fb43a78aa10437e79ac333798ba6e9 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Thu, 13 Aug 2026 16:06:08 -0400 Subject: [PATCH 02/11] Give the guid strict precedence and index it per parent The finder returned the first candidate in document order, so a sibling that merely still carried the old id could beat the element that actually shared the guid. Consult the guid index first, and fall back to the id only against elements that name no guid of their own. Index each parent's children once instead of rescanning them per child, as the single-key finder already does; a range can hold a couple of thousand elements. Duplicate ids are kept rather than rejected, since two elements may share an id and still differ by guid. Also report both keys in the ambiguity warning, since either can be what formed the group, and build the test's two spellings by normalizing so they cannot be flattened by an editor. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 2 +- .../merge/xml/generic/FindNodeToMerge.cs | 89 ++++++++++++++++--- .../LiftRanges/LiftRangesFileHandlerTests.cs | 42 ++++++++- 3 files changed, 120 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 010c05099..e32c11c37 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,7 +42,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed -- [SIL.Chorus.LibChorus] Match `range-element` in `.lift-ranges` on its `guid` rather than its `id`, so that renaming or respelling a range element merges as an edit instead of a deletion plus an addition +- [SIL.Chorus.LibChorus] Match `range-element` on its `guid` rather than its `id`, so that renaming or respelling a range element merges as an edit instead of a deletion plus an addition - Prevent S&R to Internet without full URL - [SIL.Chorus.LibChorus] Correctly handle & and other special characters in passwords - [SIL.Chorus] Fix collection-modified exception when UsbDrives property is read while background scan thread updates the list diff --git a/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs b/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs index df176d44b..e39b685dd 100644 --- a/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs +++ b/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs @@ -215,15 +215,19 @@ public string GetWarningMessageForAmbiguousNodes(XmlNode nodeForMessage) /// falling back to an ordinary key attribute when either element lacks that attribute. /// /// - /// Use this where the ordinary key is derived from user data, and so can be respelled without the + /// Use this where the ordinary key is derived from user data, and so can be respelled without the /// underlying object having changed. Matching on such a key alone turns a respelling into a deletion /// plus an addition, which discards the other user's edits to the same element and can leave two - /// elements standing for one object. + /// elements standing for one object. + /// Matching is not transitive across a set that mixes elements carrying the permanent key with + /// elements lacking it: A(guid G, id X) matches B(guid G, id Y), and B matches C(no guid, id Y), but A + /// does not match C. Ambiguity resolution over such a set therefore depends on document order. /// public class FindByPreferredKeyAttribute : IFindMatchingNodesToMerge { private readonly string _preferredKeyAttribute; private readonly string _fallbackKeyAttribute; + private readonly Dictionary _indexedParents = new Dictionary(); public FindByPreferredKeyAttribute(string preferredKeyAttribute, string fallbackKeyAttribute) { @@ -233,15 +237,77 @@ public FindByPreferredKeyAttribute(string preferredKeyAttribute, string fallback public XmlNode GetNodeToMerge(XmlNode nodeToMatch, XmlNode parentToSearchIn, HashSet acceptableTargets) { - if (parentToSearchIn == null) + if (nodeToMatch == null || parentToSearchIn == null) + return null; + + var preferredKey = XmlUtilities.GetOptionalAttributeString(nodeToMatch, _preferredKeyAttribute); + var fallbackKey = XmlUtilities.GetOptionalAttributeString(nodeToMatch, _fallbackKeyAttribute); + if (string.IsNullOrEmpty(preferredKey) && string.IsNullOrEmpty(fallbackKey)) return null; - foreach (var match in GetMatchingNodes(nodeToMatch, parentToSearchIn)) + var index = GetIndexFor(parentToSearchIn); + XmlNode match; + if (string.IsNullOrEmpty(preferredKey)) { - if (acceptableTargets.Contains(match)) - return match; + index.ByFallbackKey.TryGetValue(new Tuple(nodeToMatch.Name, fallbackKey), out match); + return match; // May be null, which is fine. } - return null; + // The permanent key wins wherever it is present, whatever the document order. + if (index.ByPreferredKey.TryGetValue(new Tuple(nodeToMatch.Name, preferredKey), out match)) + return match; + // Only an element naming no permanent key of its own can still be the same object. + if (!string.IsNullOrEmpty(fallbackKey)) + index.ByFallbackKeyAlone.TryGetValue(new Tuple(nodeToMatch.Name, fallbackKey), out match); + return match; + } + + /// + /// The children of one parent, indexed by each key this finder can match on, so that merging a + /// large element does not rescan its siblings once per child. + /// + private class ParentIndex + { + internal readonly Dictionary, XmlNode> ByPreferredKey = new Dictionary, XmlNode>(); + internal readonly Dictionary, XmlNode> ByFallbackKey = new Dictionary, XmlNode>(); + /// Only those children that name no preferred key of their own. + internal readonly Dictionary, XmlNode> ByFallbackKeyAlone = new Dictionary, XmlNode>(); + } + + private ParentIndex GetIndexFor(XmlNode parentToSearchIn) + { + ParentIndex index; + if (_indexedParents.TryGetValue(parentToSearchIn, out index)) + return index; + + index = new ParentIndex(); + _indexedParents.Add(parentToSearchIn, index); + foreach (XmlNode childNode in parentToSearchIn.ChildNodes) + { + if (childNode.NodeType != XmlNodeType.Element) + continue; + var preferredKey = XmlUtilities.GetOptionalAttributeString(childNode, _preferredKeyAttribute); + var fallbackKey = XmlUtilities.GetOptionalAttributeString(childNode, _fallbackKeyAttribute); + if (!string.IsNullOrEmpty(preferredKey)) + IndexFirstOnly(index.ByPreferredKey, childNode, preferredKey); + if (string.IsNullOrEmpty(fallbackKey)) + continue; + IndexFirstOnly(index.ByFallbackKey, childNode, fallbackKey); + if (string.IsNullOrEmpty(preferredKey)) + IndexFirstOnly(index.ByFallbackKeyAlone, childNode, fallbackKey); + } + return index; + } + + /// + /// Unlike a finder with a single key, duplicate fallback keys are legitimate here, since two + /// elements can share a respellable key and still be told apart by the permanent one. Keep the + /// first, matching how the merger resolves siblings it cannot tell apart. + /// + private static void IndexFirstOnly(IDictionary, XmlNode> index, XmlNode childNode, string key) + { + var indexKey = new Tuple(childNode.Name, key); + if (!index.ContainsKey(indexKey)) + index.Add(indexKey, childNode); } /// @@ -296,12 +362,15 @@ public string GetWarningMessageForAmbiguousNodes(XmlNode nodeForMessage) { Guard.AgainstNull(nodeForMessage, "nodeForMessage"); + // Either key can be what made the siblings indistinguishable, and by here it is no longer + // known which, so report both rather than claim the guids matched when they may not have. var preferredKey = XmlUtilities.GetOptionalAttributeString(nodeForMessage, _preferredKeyAttribute); + var fallbackKey = XmlUtilities.GetOptionalAttributeString(nodeForMessage, _fallbackKeyAttribute); return string.IsNullOrEmpty(preferredKey) ? string.Format("The key attribute '{0}' has values that are the same '{1}'", - _fallbackKeyAttribute, XmlUtilities.GetOptionalAttributeString(nodeForMessage, _fallbackKeyAttribute)) - : string.Format("The key attribute '{0}' has values that are the same '{1}'", - _preferredKeyAttribute, preferredKey); + _fallbackKeyAttribute, fallbackKey) + : string.Format("The key attributes '{0}' ('{1}') and '{2}' ('{3}') do not tell these elements apart", + _preferredKeyAttribute, preferredKey, _fallbackKeyAttribute, fallbackKey); } } diff --git a/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs b/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs index 83c7187b6..79bb7f068 100644 --- a/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs +++ b/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs @@ -1,6 +1,7 @@ using System; using System.IO; using System.Linq; +using System.Text; using Chorus.FileTypeHandlers; using Chorus.merge; using LibChorus.TestUtilities; @@ -316,6 +317,41 @@ public void RangeElementsWithDifferentGuidsAreNotMatched() string.Format("//range-element[@guid='{0}']", theirGuid), 1); } + /// + /// A sibling that still carries the old id, written by a tool that omits guids, must not be + /// taken as the partner just because it comes first in the file. + /// + [Test] + public void GuidMatchIsPreferredOverAnEarlierSiblingMatchingOnTheOldId() + { + var common = RangesWithPartOfSpeech(kDecomposedName, kPosGuid, "old"); + // A guid-less element carrying the old id sorts ahead of our respelled one. + var ours = string.Format( +@" + + + +
unrelated
 + + +
old
 + + +", kDecomposedName, kComposedName, kPosGuid); + var theirs = RangesWithPartOfSpeech(kDecomposedName, kPosGuid, "new"); + + // The guid-less sibling genuinely is indistinguishable from the ancestor element under the + // fallback rule, so one conflict here is the merger reporting real ambiguity in the input. + var result = DoMerge(common, ours, theirs, 1, 4); + + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath("//range/range-element", 2); + // Their edit belongs to the element sharing the guid, not to the one sharing the old id. + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath(string.Format( + "//range-element[@guid='{0}']/abbrev/form/text[text()='new']", kPosGuid), 1); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( + "//range-element[not(@guid)]/abbrev/form/text[text()='unrelated']", 1); + } + /// /// A merge done before the guid was preferred could leave one possibility standing as two /// range-elements, spelled differently. Matching on the guid makes them ambiguous siblings, @@ -343,8 +379,10 @@ public void RangeElementsDuplicatedByAnEarlierMergeCollapseToOne() Assert.That(_eventListener.Warnings, Is.Not.Empty, "the dropped duplicate should be reported"); } - private const string kDecomposedName = "Compléments"; // e + combining acute - private const string kComposedName = "Compléments"; // precomposed e-acute + // Built rather than written out, since the two spellings are indistinguishable in source and an + // editor or a text filter that normalizes on save would silently make them the same string. + private static readonly string kDecomposedName = "Compléments".Normalize(NormalizationForm.FormD); + private static readonly string kComposedName = "Compléments".Normalize(NormalizationForm.FormC); private const string kPosGuid = "e8c4b4b0-1a2f-4f9e-9f39-3f2b0d8f7a11"; private static string RangesWithPartOfSpeech(string id, string guid, string abbrev) From 847e0ffbcd0e53403a0c03c3630ec7e20cc0eb52 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Thu, 13 Aug 2026 18:52:46 -0400 Subject: [PATCH 03/11] Match guid-bearing and bare elements separately Letting an element that names no guid match one that does made matching non-transitive, so a set holding both could give one element two partners: the same incoming edit was merged into two siblings, raising a conflict nobody caused and, where our side had not also changed, silently applying someone's edit to a different object. Match guid-bearing elements only against guid-bearing ones, and bare elements only against bare ones. That makes matching an equivalence relation, at the price of reading a file that starts naming guids as a deletion plus an addition, once. Co-Authored-By: Claude Opus 5 (1M context) --- .../merge/xml/generic/FindNodeToMerge.cs | 35 +++++++++---------- .../LiftRanges/LiftRangesFileHandlerTests.cs | 6 ++-- 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs b/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs index e39b685dd..28c9862b7 100644 --- a/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs +++ b/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs @@ -212,16 +212,19 @@ public string GetWarningMessageForAmbiguousNodes(XmlNode nodeForMessage) /// /// Search for a matching element using an optional attribute that identifies it permanently (a guid), - /// falling back to an ordinary key attribute when either element lacks that attribute. + /// falling back to an ordinary key attribute among the elements that name no such attribute. /// /// /// Use this where the ordinary key is derived from user data, and so can be respelled without the /// underlying object having changed. Matching on such a key alone turns a respelling into a deletion /// plus an addition, which discards the other user's edits to the same element and can leave two /// elements standing for one object. - /// Matching is not transitive across a set that mixes elements carrying the permanent key with - /// elements lacking it: A(guid G, id X) matches B(guid G, id Y), and B matches C(no guid, id Y), but A - /// does not match C. Ambiguity resolution over such a set therefore depends on document order. + /// Elements naming a permanent key and elements lacking one are matched separately, and never + /// against each other. That keeps matching an equivalence relation, so no element can be paired with + /// two partners; were a permanent key allowed to match a bare one, a set holding both could give one + /// element two partners and merge someone's edit onto an object it was not made against. The price is + /// that a file which starts naming permanent keys for elements that previously had none reads as a + /// deletion plus an addition, once. /// public class FindByPreferredKeyAttribute : IFindMatchingNodesToMerge { @@ -249,15 +252,12 @@ public XmlNode GetNodeToMerge(XmlNode nodeToMatch, XmlNode parentToSearchIn, Has XmlNode match; if (string.IsNullOrEmpty(preferredKey)) { - index.ByFallbackKey.TryGetValue(new Tuple(nodeToMatch.Name, fallbackKey), out match); + // Only among elements that likewise name none, so that this cannot claim an element + // the permanent key already speaks for. + index.ByFallbackKeyAlone.TryGetValue(new Tuple(nodeToMatch.Name, fallbackKey), out match); return match; // May be null, which is fine. } - // The permanent key wins wherever it is present, whatever the document order. - if (index.ByPreferredKey.TryGetValue(new Tuple(nodeToMatch.Name, preferredKey), out match)) - return match; - // Only an element naming no permanent key of its own can still be the same object. - if (!string.IsNullOrEmpty(fallbackKey)) - index.ByFallbackKeyAlone.TryGetValue(new Tuple(nodeToMatch.Name, fallbackKey), out match); + index.ByPreferredKey.TryGetValue(new Tuple(nodeToMatch.Name, preferredKey), out match); return match; } @@ -268,7 +268,6 @@ public XmlNode GetNodeToMerge(XmlNode nodeToMatch, XmlNode parentToSearchIn, Has private class ParentIndex { internal readonly Dictionary, XmlNode> ByPreferredKey = new Dictionary, XmlNode>(); - internal readonly Dictionary, XmlNode> ByFallbackKey = new Dictionary, XmlNode>(); /// Only those children that name no preferred key of their own. internal readonly Dictionary, XmlNode> ByFallbackKeyAlone = new Dictionary, XmlNode>(); } @@ -289,10 +288,7 @@ private ParentIndex GetIndexFor(XmlNode parentToSearchIn) var fallbackKey = XmlUtilities.GetOptionalAttributeString(childNode, _fallbackKeyAttribute); if (!string.IsNullOrEmpty(preferredKey)) IndexFirstOnly(index.ByPreferredKey, childNode, preferredKey); - if (string.IsNullOrEmpty(fallbackKey)) - continue; - IndexFirstOnly(index.ByFallbackKey, childNode, fallbackKey); - if (string.IsNullOrEmpty(preferredKey)) + else if (!string.IsNullOrEmpty(fallbackKey)) IndexFirstOnly(index.ByFallbackKeyAlone, childNode, fallbackKey); } return index; @@ -346,8 +342,11 @@ public IEnumerable GetMatchingNodes(XmlNode nodeToMatch, XmlNode parent private bool IsMatch(XmlNode candidate, string preferredKey, string fallbackKey) { var candidatePreferredKey = XmlUtilities.GetOptionalAttributeString(candidate, _preferredKeyAttribute); - // When both carry the permanent key it decides on its own, since the fallback key may have moved. - if (!string.IsNullOrEmpty(preferredKey) && !string.IsNullOrEmpty(candidatePreferredKey)) + // One names a permanent key and the other does not, so they are matched separately. + if (string.IsNullOrEmpty(preferredKey) != string.IsNullOrEmpty(candidatePreferredKey)) + return false; + // Both name one, and it decides alone: the fallback key may have moved. + if (!string.IsNullOrEmpty(preferredKey)) return preferredKey == candidatePreferredKey; if (string.IsNullOrEmpty(fallbackKey)) return false; diff --git a/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs b/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs index 79bb7f068..b9947e0ba 100644 --- a/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs +++ b/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs @@ -340,9 +340,9 @@ public void GuidMatchIsPreferredOverAnEarlierSiblingMatchingOnTheOldId() ", kDecomposedName, kComposedName, kPosGuid); var theirs = RangesWithPartOfSpeech(kDecomposedName, kPosGuid, "new"); - // The guid-less sibling genuinely is indistinguishable from the ancestor element under the - // fallback rule, so one conflict here is the merger reporting real ambiguity in the input. - var result = DoMerge(common, ours, theirs, 1, 4); + // No conflict: the guid-less sibling is matched only against other guid-less elements, so it + // cannot also claim the element the guid already speaks for. + var result = DoMerge(common, ours, theirs, 0, 3); AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath("//range/range-element", 2); // Their edit belongs to the element sharing the guid, not to the one sharing the old id. From f94c6d08aca299dc652101bb0b2082f8316a97db Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Fri, 14 Aug 2026 09:37:40 -0400 Subject: [PATCH 04/11] Document the finder contracts and pin the guid-dropping cost State on IFindNodeToMerge that discarding a result outside acceptableTargets is the caller's job, since an indexed finder answers from its index without consulting the set, and note that a ParentIndex is built once, so a parent whose children change between searches is still answered from the children it first held. Spell out what matching guid-bearing and bare elements separately costs: where one revision names the permanent key and another does not, the element reads as a deletion plus an addition, so writers that disagree over whether to name it pay that on every merge between them rather than once. DroppingTheGuidReadsAsADeletionPlusAnAddition pins that shape. Co-Authored-By: Claude Opus 5 (1M context) --- .../merge/xml/generic/FindNodeToMerge.cs | 20 +++++++++----- .../LiftRanges/LiftRangesFileHandlerTests.cs | 26 +++++++++++++++++++ 2 files changed, 39 insertions(+), 7 deletions(-) diff --git a/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs b/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs index 28c9862b7..f566b1b35 100644 --- a/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs +++ b/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs @@ -15,9 +15,10 @@ namespace Chorus.merge.xml.generic public interface IFindNodeToMerge { /// - /// Should return null if parentToSearchIn is null. Non-null result should be a value in acceptableTargets, - /// which will be a subset (or all) of the children of parentToSearchIn; any other result will - /// be treated as no match. + /// Should return null if parentToSearchIn is null, and otherwise either null or a child of + /// parentToSearchIn. acceptableTargets is a subset (or all) of the children of parentToSearchIn; + /// an implementation holding only one possible match may ignore it and return that match, so a + /// caller passing a strict subset is responsible for discarding a result outside it. /// XmlNode GetNodeToMerge(XmlNode nodeToMatch, XmlNode parentToSearchIn, HashSet acceptableTargets); } @@ -222,9 +223,12 @@ public string GetWarningMessageForAmbiguousNodes(XmlNode nodeForMessage) /// Elements naming a permanent key and elements lacking one are matched separately, and never /// against each other. That keeps matching an equivalence relation, so no element can be paired with /// two partners; were a permanent key allowed to match a bare one, a set holding both could give one - /// element two partners and merge someone's edit onto an object it was not made against. The price is - /// that a file which starts naming permanent keys for elements that previously had none reads as a - /// deletion plus an addition, once. + /// element two partners and merge someone's edit onto an object it was not made against. + /// The price is that where one revision names a permanent key for an element and another does not, + /// the element reads as a deletion plus an addition, and as a removed-versus-edited conflict if the other + /// revision also edited it. A writer that starts naming permanent keys pays that once. Writers that + /// disagree over whether to name them pay it on every merge between them, so prefer a permanent key only + /// where every writer of the file can be relied on to keep one it finds. /// public class FindByPreferredKeyAttribute : IFindMatchingNodesToMerge { @@ -263,7 +267,9 @@ public XmlNode GetNodeToMerge(XmlNode nodeToMatch, XmlNode parentToSearchIn, Has /// /// The children of one parent, indexed by each key this finder can match on, so that merging a - /// large element does not rescan its siblings once per child. + /// large element does not rescan its siblings once per child. An index is built when its parent is + /// first searched and is never revisited, so a caller that adds or removes children between + /// searches of the same parent will still be answered from the children it first held. /// private class ParentIndex { diff --git a/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs b/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs index b9947e0ba..6e36b2148 100644 --- a/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs +++ b/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs @@ -4,6 +4,7 @@ using System.Text; using Chorus.FileTypeHandlers; using Chorus.merge; +using Chorus.merge.xml.generic; using LibChorus.TestUtilities; using NUnit.Framework; using SIL.IO; @@ -292,6 +293,31 @@ public void RangeElementWithoutAGuidStillMergesOnItsId() "//range-element/abbrev/form/text[text()='new']", 1); } + /// + /// An element that names a guid is matched only against elements that likewise name one, so a + /// writer that drops the guid a previous writer wrote loses the object's identity: the element + /// reads as a deletion and its guid-less replacement as an addition. That is the cost of keeping + /// the two sets apart, and a project whose writers disagree over the guid pays it on every merge + /// between them. + /// + [Test] + public void DroppingTheGuidReadsAsADeletionPlusAnAddition() + { + var common = RangesWithPartOfSpeech("Noun", kPosGuid, "old"); + // Our writer omits the guid the ancestor names. + var ours = RangesWithPartOfSpeech("Noun", null, "old"); + var theirs = RangesWithPartOfSpeech("Noun", kPosGuid, "new"); + + var result = DoMerge(common, ours, theirs, 1, 1); + + _eventListener.AssertFirstConflictType(); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath("//range/range-element", 2); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( + "//range-element[not(@guid)]/abbrev/form/text[text()='old']", 1); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath(string.Format( + "//range-element[@guid='{0}']/abbrev/form/text[text()='new']", kPosGuid), 1); + } + /// /// Two possibilities that happen to share a name are still two possibilities. Matching them /// on the id would merge them into one and lose a guid. From d53682bf4fb0ac8676859904c51f7e1a015e2f05 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Fri, 14 Aug 2026 10:06:36 -0400 Subject: [PATCH 05/11] Name only the key that failed to tell ambiguous siblings apart Which key an element is matched on is decided by whether it names the permanent one, so that key alone is what its ambiguous siblings share. The warning named both, accusing a key the match never consulted and which for two elements sharing a permanent key may well differ between them -- as it does for a possibility left standing twice under two spellings of its name. Co-Authored-By: Claude Opus 5 (1M context) --- .../merge/xml/generic/FindNodeToMerge.cs | 15 +++++++-------- .../LiftRanges/LiftRangesFileHandlerTests.cs | 6 +++++- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs b/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs index f566b1b35..257537823 100644 --- a/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs +++ b/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs @@ -367,15 +367,14 @@ public string GetWarningMessageForAmbiguousNodes(XmlNode nodeForMessage) { Guard.AgainstNull(nodeForMessage, "nodeForMessage"); - // Either key can be what made the siblings indistinguishable, and by here it is no longer - // known which, so report both rather than claim the guids matched when they may not have. + // Whether the element names a permanent key decides which key it was matched on, so that is the + // key whose values are the same. Naming the other one too would accuse a key the match never + // consulted, and which for two elements sharing a permanent key may well differ between them. var preferredKey = XmlUtilities.GetOptionalAttributeString(nodeForMessage, _preferredKeyAttribute); - var fallbackKey = XmlUtilities.GetOptionalAttributeString(nodeForMessage, _fallbackKeyAttribute); - return string.IsNullOrEmpty(preferredKey) - ? string.Format("The key attribute '{0}' has values that are the same '{1}'", - _fallbackKeyAttribute, fallbackKey) - : string.Format("The key attributes '{0}' ('{1}') and '{2}' ('{3}') do not tell these elements apart", - _preferredKeyAttribute, preferredKey, _fallbackKeyAttribute, fallbackKey); + var matchedOnPreferredKey = !string.IsNullOrEmpty(preferredKey); + return string.Format("The key attribute '{0}' has values that are the same '{1}'", + matchedOnPreferredKey ? _preferredKeyAttribute : _fallbackKeyAttribute, + matchedOnPreferredKey ? preferredKey : XmlUtilities.GetOptionalAttributeString(nodeForMessage, _fallbackKeyAttribute)); } } diff --git a/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs b/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs index 6e36b2148..cf9b5d546 100644 --- a/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs +++ b/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs @@ -381,7 +381,8 @@ public void GuidMatchIsPreferredOverAnEarlierSiblingMatchingOnTheOldId() /// /// A merge done before the guid was preferred could leave one possibility standing as two /// range-elements, spelled differently. Matching on the guid makes them ambiguous siblings, - /// which the merger collapses back to one. + /// which the merger collapses back to one. The warning must name the guid as what the two + /// share, since the differing ids are not what made them indistinguishable. /// [Test] public void RangeElementsDuplicatedByAnEarlierMergeCollapseToOne() @@ -403,6 +404,9 @@ public void RangeElementsDuplicatedByAnEarlierMergeCollapseToOne() AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath("//range/range-element", 1); Assert.That(_eventListener.Warnings, Is.Not.Empty, "the dropped duplicate should be reported"); + var warning = _eventListener.Warnings[0].GetFullHumanReadableDescription(); + Assert.That(warning, Does.Contain("'guid'").And.Contain(kPosGuid), warning); + Assert.That(warning, Does.Not.Contain("'id'"), warning); } // Built rather than written out, since the two spellings are indistinguishable in source and an From 16e5b166e8e5bc5ecb0bd0908b6039d679bc932d Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Fri, 14 Aug 2026 14:03:25 -0400 Subject: [PATCH 06/11] Say preferred key and fallback key throughout the finder The class summary described the two keys as an attribute that identifies an element permanently (a guid) and an ordinary key, and the comments followed with permanent, respellable and bare. Three problems: the guid belongs to the LIFT wiring rather than to a generic finder that takes any two attribute names; permanence is a claim the class neither enforces nor checks; and both keys are read as optional attributes, so calling only one of them optional invents an asymmetry that is not there. Use the vocabulary the constructor, the fields and the index already use, which the ParentIndex comment alone was doing. Also drop "falling back to" from the summary, which outlived the change that made the two groups match separately: an element that names the preferred key and finds no partner is never retried against the fallback key. Co-Authored-By: Claude Opus 5 (1M context) --- .../merge/xml/generic/FindNodeToMerge.cs | 39 ++++++++++--------- 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs b/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs index 257537823..30552315b 100644 --- a/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs +++ b/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs @@ -212,22 +212,25 @@ public string GetWarningMessageForAmbiguousNodes(XmlNode nodeForMessage) } /// - /// Search for a matching element using an optional attribute that identifies it permanently (a guid), - /// falling back to an ordinary key attribute among the elements that name no such attribute. + /// Search for a matching element on either of two key attributes. An element that names the preferred + /// key is matched on that key alone; an element that names no preferred key is matched on the fallback + /// key. The two groups are never matched against each other, so a preferred key that finds no partner + /// is not retried against the fallback key. /// /// - /// Use this where the ordinary key is derived from user data, and so can be respelled without the - /// underlying object having changed. Matching on such a key alone turns a respelling into a deletion - /// plus an addition, which discards the other user's edits to the same element and can leave two - /// elements standing for one object. - /// Elements naming a permanent key and elements lacking one are matched separately, and never + /// Use this where the fallback key is derived from user data, and so can change while the object it + /// identifies does not, and the preferred key is one that stays with that object. Matching on the + /// changeable key alone turns such a change into a deletion plus an addition, which discards the other + /// user's edits to the same element and can leave two elements standing for one object. + /// Elements naming the preferred key and elements naming none are matched separately, and never /// against each other. That keeps matching an equivalence relation, so no element can be paired with - /// two partners; were a permanent key allowed to match a bare one, a set holding both could give one - /// element two partners and merge someone's edit onto an object it was not made against. - /// The price is that where one revision names a permanent key for an element and another does not, + /// two partners; were an element naming the preferred key allowed to match one naming none, a set holding + /// both could give one element two partners and merge someone's edit onto an object it was not made + /// against. + /// The price is that where one revision names the preferred key for an element and another does not, /// the element reads as a deletion plus an addition, and as a removed-versus-edited conflict if the other - /// revision also edited it. A writer that starts naming permanent keys pays that once. Writers that - /// disagree over whether to name them pay it on every merge between them, so prefer a permanent key only + /// revision also edited it. A writer that starts naming the preferred key pays that once. Writers that + /// disagree over whether to name it pay it on every merge between them, so choose a preferred key only /// where every writer of the file can be relied on to keep one it finds. /// public class FindByPreferredKeyAttribute : IFindMatchingNodesToMerge @@ -257,7 +260,7 @@ public XmlNode GetNodeToMerge(XmlNode nodeToMatch, XmlNode parentToSearchIn, Has if (string.IsNullOrEmpty(preferredKey)) { // Only among elements that likewise name none, so that this cannot claim an element - // the permanent key already speaks for. + // the preferred key already speaks for. index.ByFallbackKeyAlone.TryGetValue(new Tuple(nodeToMatch.Name, fallbackKey), out match); return match; // May be null, which is fine. } @@ -302,7 +305,7 @@ private ParentIndex GetIndexFor(XmlNode parentToSearchIn) /// /// Unlike a finder with a single key, duplicate fallback keys are legitimate here, since two - /// elements can share a respellable key and still be told apart by the permanent one. Keep the + /// elements can share the fallback key and still be told apart by the preferred one. Keep the /// first, matching how the merger resolves siblings it cannot tell apart. /// private static void IndexFirstOnly(IDictionary, XmlNode> index, XmlNode childNode, string key) @@ -348,10 +351,10 @@ public IEnumerable GetMatchingNodes(XmlNode nodeToMatch, XmlNode parent private bool IsMatch(XmlNode candidate, string preferredKey, string fallbackKey) { var candidatePreferredKey = XmlUtilities.GetOptionalAttributeString(candidate, _preferredKeyAttribute); - // One names a permanent key and the other does not, so they are matched separately. + // One names the preferred key and the other does not, so they are matched separately. if (string.IsNullOrEmpty(preferredKey) != string.IsNullOrEmpty(candidatePreferredKey)) return false; - // Both name one, and it decides alone: the fallback key may have moved. + // Both name it, and it decides alone: the fallback key may have changed. if (!string.IsNullOrEmpty(preferredKey)) return preferredKey == candidatePreferredKey; if (string.IsNullOrEmpty(fallbackKey)) @@ -367,9 +370,9 @@ public string GetWarningMessageForAmbiguousNodes(XmlNode nodeForMessage) { Guard.AgainstNull(nodeForMessage, "nodeForMessage"); - // Whether the element names a permanent key decides which key it was matched on, so that is the + // Whether the element names the preferred key decides which key it was matched on, so that is the // key whose values are the same. Naming the other one too would accuse a key the match never - // consulted, and which for two elements sharing a permanent key may well differ between them. + // consulted, and which for two elements sharing a preferred key may well differ between them. var preferredKey = XmlUtilities.GetOptionalAttributeString(nodeForMessage, _preferredKeyAttribute); var matchedOnPreferredKey = !string.IsNullOrEmpty(preferredKey); return string.Format("The key attribute '{0}' has values that are the same '{1}'", From 624547d2155877e4d85cc2362fdce64fcede1fd8 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Fri, 14 Aug 2026 14:26:09 -0400 Subject: [PATCH 07/11] Keep key attribute and key value apart in the comments "Key" was doing duty for the attribute name, the attribute value and the dictionary key, and "name" for the element name, the attribute name and the possibility's own name, which is the id value. Some sentences only parsed under one reading: "two elements can share the fallback key" is vacuous read as attribute names, since every range-element has an @id. Follow the convention the shipped warning text already uses -- "key attribute" plus the quoted name for the name, "value" for the value -- say "carries a value" rather than "names" for presence, write @guid and @id at the LIFT sites, and say that ParentIndex is keyed on (element name, attribute value), which the tuple built three lines below it did not say. That also lets the summary state a rule it had left out: both keys are read with GetOptionalAttributeString and every test is string.IsNullOrEmpty, so an attribute present but empty counts as absent. Co-Authored-By: Claude Opus 5 (1M context) --- .../LiftRangesElementStrategiesMethod.cs | 7 +- .../merge/xml/generic/FindNodeToMerge.cs | 69 ++++++++++--------- .../LiftRanges/LiftRangesFileHandlerTests.cs | 9 +-- 3 files changed, 46 insertions(+), 39 deletions(-) diff --git a/src/LibChorus/FileTypeHandlers/Lift-Ranges/LiftRangesElementStrategiesMethod.cs b/src/LibChorus/FileTypeHandlers/Lift-Ranges/LiftRangesElementStrategiesMethod.cs index 474a8c0d0..7ff98542c 100644 --- a/src/LibChorus/FileTypeHandlers/Lift-Ranges/LiftRangesElementStrategiesMethod.cs +++ b/src/LibChorus/FileTypeHandlers/Lift-Ranges/LiftRangesElementStrategiesMethod.cs @@ -42,9 +42,10 @@ internal static void AddLiftRangeElementStrategies(MergeStrategies mergeStrategi // This element appears to not be in the main lift file, so it will be 'extra', but ought not cause harm. // - /// Search for a matching element on either of two key attributes. An element that names the preferred - /// key is matched on that key alone; an element that names no preferred key is matched on the fallback - /// key. The two groups are never matched against each other, so a preferred key that finds no partner - /// is not retried against the fallback key. + /// Search for a matching element on the value of either of two key attributes. An element carrying a + /// non-empty value for the preferred key attribute is matched on that value alone; an element carrying + /// none is matched on its value for the fallback key attribute. The two groups are never matched against + /// each other, so an element whose preferred value finds no partner is not retried against its fallback + /// value. An attribute that is present but empty counts as absent throughout. /// /// - /// Use this where the fallback key is derived from user data, and so can change while the object it - /// identifies does not, and the preferred key is one that stays with that object. Matching on the - /// changeable key alone turns such a change into a deletion plus an addition, which discards the other - /// user's edits to the same element and can leave two elements standing for one object. - /// Elements naming the preferred key and elements naming none are matched separately, and never + /// Use this where the fallback key attribute's value is derived from user data, and so can change + /// while the object it identifies does not, and the preferred key attribute holds a value that stays with + /// that object. Matching on the changeable value alone turns such a change into a deletion plus an + /// addition, which discards the other user's edits to the same element and can leave two elements + /// standing for one object. + /// Elements carrying a preferred value and elements carrying none are matched separately, and never /// against each other. That keeps matching an equivalence relation, so no element can be paired with - /// two partners; were an element naming the preferred key allowed to match one naming none, a set holding - /// both could give one element two partners and merge someone's edit onto an object it was not made - /// against. - /// The price is that where one revision names the preferred key for an element and another does not, - /// the element reads as a deletion plus an addition, and as a removed-versus-edited conflict if the other - /// revision also edited it. A writer that starts naming the preferred key pays that once. Writers that - /// disagree over whether to name it pay it on every merge between them, so choose a preferred key only - /// where every writer of the file can be relied on to keep one it finds. + /// two partners; were an element carrying a preferred value allowed to match one carrying none, a set + /// holding both could give one element two partners and merge someone's edit onto an object it was not + /// made against. + /// The price is that where one revision gives an element a preferred value and another leaves that + /// attribute off, the element reads as a deletion plus an addition, and as a removed-versus-edited + /// conflict if the other revision also edited it. A writer that starts writing the preferred attribute + /// pays that once. Writers that disagree over whether to write it pay it on every merge between them, so + /// choose a preferred key attribute only where every writer of the file can be relied on to preserve the + /// value it finds. /// public class FindByPreferredKeyAttribute : IFindMatchingNodesToMerge { @@ -259,8 +262,8 @@ public XmlNode GetNodeToMerge(XmlNode nodeToMatch, XmlNode parentToSearchIn, Has XmlNode match; if (string.IsNullOrEmpty(preferredKey)) { - // Only among elements that likewise name none, so that this cannot claim an element - // the preferred key already speaks for. + // Only among elements that likewise carry no preferred value, so that this cannot claim + // an element that a preferred value already speaks for. index.ByFallbackKeyAlone.TryGetValue(new Tuple(nodeToMatch.Name, fallbackKey), out match); return match; // May be null, which is fine. } @@ -269,15 +272,16 @@ public XmlNode GetNodeToMerge(XmlNode nodeToMatch, XmlNode parentToSearchIn, Has } /// - /// The children of one parent, indexed by each key this finder can match on, so that merging a - /// large element does not rescan its siblings once per child. An index is built when its parent is - /// first searched and is never revisited, so a caller that adds or removes children between - /// searches of the same parent will still be answered from the children it first held. + /// The children of one parent, indexed on (element name, attribute value) for each key attribute this + /// finder can match on, so that merging a large element does not rescan its siblings once per child. + /// An index is built when its parent is first searched and is never revisited, so a caller that adds + /// or removes children between searches of the same parent will still be answered from the children + /// it first held. /// private class ParentIndex { internal readonly Dictionary, XmlNode> ByPreferredKey = new Dictionary, XmlNode>(); - /// Only those children that name no preferred key of their own. + /// Only those children that carry no preferred value of their own. internal readonly Dictionary, XmlNode> ByFallbackKeyAlone = new Dictionary, XmlNode>(); } @@ -304,9 +308,9 @@ private ParentIndex GetIndexFor(XmlNode parentToSearchIn) } /// - /// Unlike a finder with a single key, duplicate fallback keys are legitimate here, since two - /// elements can share the fallback key and still be told apart by the preferred one. Keep the - /// first, matching how the merger resolves siblings it cannot tell apart. + /// Unlike a finder with a single key attribute, two children legitimately hold the same fallback + /// value here, since a preferred value can still tell them apart. Keep the first, matching how the + /// merger resolves siblings it cannot tell apart. /// private static void IndexFirstOnly(IDictionary, XmlNode> index, XmlNode childNode, string key) { @@ -351,10 +355,10 @@ public IEnumerable GetMatchingNodes(XmlNode nodeToMatch, XmlNode parent private bool IsMatch(XmlNode candidate, string preferredKey, string fallbackKey) { var candidatePreferredKey = XmlUtilities.GetOptionalAttributeString(candidate, _preferredKeyAttribute); - // One names the preferred key and the other does not, so they are matched separately. + // One carries a preferred value and the other does not, so they are matched separately. if (string.IsNullOrEmpty(preferredKey) != string.IsNullOrEmpty(candidatePreferredKey)) return false; - // Both name it, and it decides alone: the fallback key may have changed. + // Both carry one, and it decides alone: the fallback value may have changed. if (!string.IsNullOrEmpty(preferredKey)) return preferredKey == candidatePreferredKey; if (string.IsNullOrEmpty(fallbackKey)) @@ -370,9 +374,10 @@ public string GetWarningMessageForAmbiguousNodes(XmlNode nodeForMessage) { Guard.AgainstNull(nodeForMessage, "nodeForMessage"); - // Whether the element names the preferred key decides which key it was matched on, so that is the - // key whose values are the same. Naming the other one too would accuse a key the match never - // consulted, and which for two elements sharing a preferred key may well differ between them. + // Whether the element carries a preferred value decides which key attribute it was matched on, so + // that is the attribute whose values are the same. Naming the other one too would accuse an + // attribute the match never consulted, and whose values, for two elements sharing a preferred + // value, may well differ between them. var preferredKey = XmlUtilities.GetOptionalAttributeString(nodeForMessage, _preferredKeyAttribute); var matchedOnPreferredKey = !string.IsNullOrEmpty(preferredKey); return string.Format("The key attribute '{0}' has values that are the same '{1}'", diff --git a/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs b/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs index cf9b5d546..66bd22907 100644 --- a/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs +++ b/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs @@ -379,10 +379,11 @@ public void GuidMatchIsPreferredOverAnEarlierSiblingMatchingOnTheOldId() } /// - /// A merge done before the guid was preferred could leave one possibility standing as two - /// range-elements, spelled differently. Matching on the guid makes them ambiguous siblings, - /// which the merger collapses back to one. The warning must name the guid as what the two - /// share, since the differing ids are not what made them indistinguishable. + /// A merge done before @guid was preferred could leave one possibility standing as two + /// range-elements, spelled differently. Matching on the guid value makes them ambiguous siblings, + /// which the merger collapses back to one. The warning must name @guid as the attribute whose + /// values are the same, and quote the shared value, since the differing ids are not what made + /// them indistinguishable. /// [Test] public void RangeElementsDuplicatedByAnEarlierMergeCollapseToOne() From 336763e5eedceb8768866cf59d31a30741385832 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Fri, 14 Aug 2026 14:28:38 -0400 Subject: [PATCH 08/11] Name the attribute values as values in the finder Four identifiers said "key" for three different things. IndexFirstOnly took a "key" that was an attribute value and built an "indexKey" that was the index tuple, three lines apart; extract IndexKey so the tuple is named once and built in one place rather than by hand at each of the three lookups. ByFallbackKeyAlone meant "keyed on the fallback value, among the children that carry that alone" but parses as "keyed on the fallback value alone", which is vacuously true of a dictionary and says nothing about the partition it exists to hold; it needed its doc comment to be read correctly, so name it ByFallbackKeyWherePreferredAbsent. IsMatch's bare preferredKey/fallbackKey are the sought node's values while candidatePreferredKey is the other side's, an asymmetry resting on one prefix; prefix both sides. In the tests, kDecomposedName/kComposedName hold the two @id values a possibility is exported under, so "Name" was the possibility-name sense next to element and attribute names -- call them ids, as the assertions do. In RangesWithPartOfSpeech, a null guid silently means "omit @guid" rather than "empty guid", which the parameter name now says. No behavior change; 541 pass and the same 13 fail on Mercurial not being configured here, as before. Co-Authored-By: Claude Opus 5 (1M context) --- .../merge/xml/generic/FindNodeToMerge.cs | 34 ++++++++++++------- .../LiftRanges/LiftRangesFileHandlerTests.cs | 30 ++++++++-------- 2 files changed, 38 insertions(+), 26 deletions(-) diff --git a/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs b/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs index 476b8361d..0ad971d97 100644 --- a/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs +++ b/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs @@ -264,10 +264,10 @@ public XmlNode GetNodeToMerge(XmlNode nodeToMatch, XmlNode parentToSearchIn, Has { // Only among elements that likewise carry no preferred value, so that this cannot claim // an element that a preferred value already speaks for. - index.ByFallbackKeyAlone.TryGetValue(new Tuple(nodeToMatch.Name, fallbackKey), out match); + index.ByFallbackKeyWherePreferredAbsent.TryGetValue(IndexKey(nodeToMatch, fallbackKey), out match); return match; // May be null, which is fine. } - index.ByPreferredKey.TryGetValue(new Tuple(nodeToMatch.Name, preferredKey), out match); + index.ByPreferredKey.TryGetValue(IndexKey(nodeToMatch, preferredKey), out match); return match; } @@ -282,7 +282,7 @@ private class ParentIndex { internal readonly Dictionary, XmlNode> ByPreferredKey = new Dictionary, XmlNode>(); /// Only those children that carry no preferred value of their own. - internal readonly Dictionary, XmlNode> ByFallbackKeyAlone = new Dictionary, XmlNode>(); + internal readonly Dictionary, XmlNode> ByFallbackKeyWherePreferredAbsent = new Dictionary, XmlNode>(); } private ParentIndex GetIndexFor(XmlNode parentToSearchIn) @@ -302,7 +302,7 @@ private ParentIndex GetIndexFor(XmlNode parentToSearchIn) if (!string.IsNullOrEmpty(preferredKey)) IndexFirstOnly(index.ByPreferredKey, childNode, preferredKey); else if (!string.IsNullOrEmpty(fallbackKey)) - IndexFirstOnly(index.ByFallbackKeyAlone, childNode, fallbackKey); + IndexFirstOnly(index.ByFallbackKeyWherePreferredAbsent, childNode, fallbackKey); } return index; } @@ -312,13 +312,23 @@ private ParentIndex GetIndexFor(XmlNode parentToSearchIn) /// value here, since a preferred value can still tell them apart. Keep the first, matching how the /// merger resolves siblings it cannot tell apart. /// - private static void IndexFirstOnly(IDictionary, XmlNode> index, XmlNode childNode, string key) + private static void IndexFirstOnly(IDictionary, XmlNode> index, XmlNode childNode, string keyValue) { - var indexKey = new Tuple(childNode.Name, key); + var indexKey = IndexKey(childNode, keyValue); if (!index.ContainsKey(indexKey)) index.Add(indexKey, childNode); } + /// + /// A ParentIndex entry is identified by the element name as well as the attribute value, matching the + /// tuple indexes on: elements of different names never match, however + /// their keys compare. + /// + private static Tuple IndexKey(XmlNode element, string keyValue) + { + return new Tuple(element.Name, keyValue); + } + /// /// Get all matching nodes, or an empty collection, if there are no matches. /// @@ -352,18 +362,18 @@ public IEnumerable GetMatchingNodes(XmlNode nodeToMatch, XmlNode parent return matches; } - private bool IsMatch(XmlNode candidate, string preferredKey, string fallbackKey) + private bool IsMatch(XmlNode candidate, string soughtPreferredKey, string soughtFallbackKey) { var candidatePreferredKey = XmlUtilities.GetOptionalAttributeString(candidate, _preferredKeyAttribute); // One carries a preferred value and the other does not, so they are matched separately. - if (string.IsNullOrEmpty(preferredKey) != string.IsNullOrEmpty(candidatePreferredKey)) + if (string.IsNullOrEmpty(soughtPreferredKey) != string.IsNullOrEmpty(candidatePreferredKey)) return false; // Both carry one, and it decides alone: the fallback value may have changed. - if (!string.IsNullOrEmpty(preferredKey)) - return preferredKey == candidatePreferredKey; - if (string.IsNullOrEmpty(fallbackKey)) + if (!string.IsNullOrEmpty(soughtPreferredKey)) + return soughtPreferredKey == candidatePreferredKey; + if (string.IsNullOrEmpty(soughtFallbackKey)) return false; - return fallbackKey == XmlUtilities.GetOptionalAttributeString(candidate, _fallbackKeyAttribute); + return soughtFallbackKey == XmlUtilities.GetOptionalAttributeString(candidate, _fallbackKeyAttribute); } /// diff --git a/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs b/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs index 66bd22907..4c3cc1ca5 100644 --- a/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs +++ b/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs @@ -261,17 +261,17 @@ public void BothEditWithConflictAndWeWin() [Test] public void RespelledIdMergesAsAnEditWhenTheGuidIsUnchanged() { - var common = RangesWithPartOfSpeech(kDecomposedName, kPosGuid, "old"); + var common = RangesWithPartOfSpeech(kDecomposedId, kPosGuid, "old"); // We upgraded, so our export normalizes the id. - var ours = RangesWithPartOfSpeech(kComposedName, kPosGuid, "old"); + var ours = RangesWithPartOfSpeech(kComposedId, kPosGuid, "old"); // They did not upgrade, and they edited the abbreviation. - var theirs = RangesWithPartOfSpeech(kDecomposedName, kPosGuid, "new"); + var theirs = RangesWithPartOfSpeech(kDecomposedId, kPosGuid, "new"); var result = DoMerge(common, ours, theirs, 0, 2); AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath("//range/range-element", 1); AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( - string.Format("//range-element[@guid='{0}' and @id='{1}']", kPosGuid, kComposedName), 1); + string.Format("//range-element[@guid='{0}' and @id='{1}']", kPosGuid, kComposedId), 1); AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( "//range-element/abbrev/form/text[text()='new']", 1); } @@ -350,7 +350,7 @@ public void RangeElementsWithDifferentGuidsAreNotMatched() [Test] public void GuidMatchIsPreferredOverAnEarlierSiblingMatchingOnTheOldId() { - var common = RangesWithPartOfSpeech(kDecomposedName, kPosGuid, "old"); + var common = RangesWithPartOfSpeech(kDecomposedId, kPosGuid, "old"); // A guid-less element carrying the old id sorts ahead of our respelled one. var ours = string.Format( @" @@ -363,8 +363,8 @@ public void GuidMatchIsPreferredOverAnEarlierSiblingMatchingOnTheOldId()
old
 -", kDecomposedName, kComposedName, kPosGuid); - var theirs = RangesWithPartOfSpeech(kDecomposedName, kPosGuid, "new"); +", kDecomposedId, kComposedId, kPosGuid); + var theirs = RangesWithPartOfSpeech(kDecomposedId, kPosGuid, "new"); // No conflict: the guid-less sibling is matched only against other guid-less elements, so it // cannot also claim the element the guid already speaks for. @@ -399,7 +399,7 @@ public void RangeElementsDuplicatedByAnEarlierMergeCollapseToOne()
old
 -", kDecomposedName, kComposedName, kPosGuid); +", kDecomposedId, kComposedId, kPosGuid); var result = DoMerge(duplicated, duplicated, duplicated, 0, 0); @@ -410,13 +410,15 @@ public void RangeElementsDuplicatedByAnEarlierMergeCollapseToOne() Assert.That(warning, Does.Not.Contain("'id'"), warning); } - // Built rather than written out, since the two spellings are indistinguishable in source and an - // editor or a text filter that normalizes on save would silently make them the same string. - private static readonly string kDecomposedName = "Compléments".Normalize(NormalizationForm.FormD); - private static readonly string kComposedName = "Compléments".Normalize(NormalizationForm.FormC); + // The two @id values a part of speech named "Compléments" is exported under. Built rather than written + // out, since the two spellings are indistinguishable in source and an editor or a text filter that + // normalizes on save would silently make them the same string. + private static readonly string kDecomposedId = "Compléments".Normalize(NormalizationForm.FormD); + private static readonly string kComposedId = "Compléments".Normalize(NormalizationForm.FormC); private const string kPosGuid = "e8c4b4b0-1a2f-4f9e-9f39-3f2b0d8f7a11"; - private static string RangesWithPartOfSpeech(string id, string guid, string abbrev) + /// A null leaves @guid off the element altogether. + private static string RangesWithPartOfSpeech(string posId, string posGuidOrNull, string abbrevText) { return string.Format( @" @@ -426,7 +428,7 @@ private static string RangesWithPartOfSpeech(string id, string guid, string abbr
{2}
 -", id, guid == null ? string.Empty : string.Format(" guid='{0}'", guid), abbrev); +", posId, posGuidOrNull == null ? string.Empty : string.Format(" guid='{0}'", posGuidOrNull), abbrevText); } private string DoMerge(string commonAncestor, string ourContent, string theirContent, From d3cf42966ce1656da3c9b7009d063c22af57f218 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Fri, 14 Aug 2026 15:12:16 -0400 Subject: [PATCH 09/11] Cut the finder's documentation back to what the code cannot say The class header had grown to 21 lines of prose for a 150-line class, most of it the commit messages that produced it. Cut what is recorded elsewhere and keep what a maintainer must read before reusing the finder. Gone: the summary's two restatements of the partition rule that opens the second paragraph; the counterfactual showing why a mixed set would pair an element twice (847e0ffb); the once-versus-every-merge cost stated three times over (f94c6d08, and DroppingTheGuidReadsAsADeletionPlusAnAddition); the consequences of matching on a changeable value (the CHANGELOG entry and RespelledIdMergesAsAnEditWhenTheGuidIsUnchanged); and the warning comment's account of why the unmatched attribute's values may differ, which the test's own comment now carries. Kept deliberately, being the parts the last two commits bought: "never both for the same element" in the summary, since "fallback" alone still invites the sequential reading that the class name encourages; "preserve the value it finds" rather than "one it finds"; the (element name, attribute value) tuple on ParentIndex; "the attribute whose values are the same"; and the rule that an empty attribute counts as absent, moved into the paragraph on the partition rather than dropped with the rest of the summary. Also kept whole: IFindNodeToMerge.GetNodeToMerge, whose four lines are each an obligation, and which replaced a contract that was wrong. Co-Authored-By: Claude Opus 5 (1M context) --- .../LiftRangesElementStrategiesMethod.cs | 8 +- .../merge/xml/generic/FindNodeToMerge.cs | 81 +++++++++---------- .../LiftRanges/LiftRangesFileHandlerTests.cs | 68 +++++++++++----- 3 files changed, 88 insertions(+), 69 deletions(-) diff --git a/src/LibChorus/FileTypeHandlers/Lift-Ranges/LiftRangesElementStrategiesMethod.cs b/src/LibChorus/FileTypeHandlers/Lift-Ranges/LiftRangesElementStrategiesMethod.cs index 7ff98542c..b653d7e6d 100644 --- a/src/LibChorus/FileTypeHandlers/Lift-Ranges/LiftRangesElementStrategiesMethod.cs +++ b/src/LibChorus/FileTypeHandlers/Lift-Ranges/LiftRangesElementStrategiesMethod.cs @@ -42,10 +42,10 @@ internal static void AddLiftRangeElementStrategies(MergeStrategies mergeStrategi // This element appears to not be in the main lift file, so it will be 'extra', but ought not cause harm. // - /// Should return null if parentToSearchIn is null, and otherwise either null or a child of - /// parentToSearchIn. acceptableTargets is a subset (or all) of the children of parentToSearchIn; - /// an implementation holding only one possible match may ignore it and return that match, so a - /// caller passing a strict subset is responsible for discarding a result outside it. + /// Should return null if parentToSearchIn is null, and otherwise either null or a + /// child of parentToSearchIn. acceptableTargets is a subset (or all) of the children + /// of parentToSearchIn; an implementation holding only one possible match may ignore + /// it and return that match, so a caller passing a strict subset is responsible for + /// discarding a result outside it. /// XmlNode GetNodeToMerge(XmlNode nodeToMatch, XmlNode parentToSearchIn, HashSet acceptableTargets); } @@ -212,29 +213,25 @@ public string GetWarningMessageForAmbiguousNodes(XmlNode nodeForMessage) } /// - /// Search for a matching element on the value of either of two key attributes. An element carrying a - /// non-empty value for the preferred key attribute is matched on that value alone; an element carrying - /// none is matched on its value for the fallback key attribute. The two groups are never matched against - /// each other, so an element whose preferred value finds no partner is not retried against its fallback - /// value. An attribute that is present but empty counts as absent throughout. + /// Search for a matching element on the value of one of two key attributes: the preferred + /// attribute where the element carries a non-empty value for it, the fallback attribute only + /// where it does not. The fallback value is not consulted where a non-empty preferred value + /// is present. /// /// - /// Use this where the fallback key attribute's value is derived from user data, and so can change - /// while the object it identifies does not, and the preferred key attribute holds a value that stays with - /// that object. Matching on the changeable value alone turns such a change into a deletion plus an - /// addition, which discards the other user's edits to the same element and can leave two elements - /// standing for one object. - /// Elements carrying a preferred value and elements carrying none are matched separately, and never - /// against each other. That keeps matching an equivalence relation, so no element can be paired with - /// two partners; were an element carrying a preferred value allowed to match one carrying none, a set - /// holding both could give one element two partners and merge someone's edit onto an object it was not - /// made against. - /// The price is that where one revision gives an element a preferred value and another leaves that - /// attribute off, the element reads as a deletion plus an addition, and as a removed-versus-edited - /// conflict if the other revision also edited it. A writer that starts writing the preferred attribute - /// pays that once. Writers that disagree over whether to write it pay it on every merge between them, so - /// choose a preferred key attribute only where every writer of the file can be relied on to preserve the - /// value it finds. + /// Use this where the fallback key attribute's value is derived from user data, and so + /// can change while the object it identifies does not, and the preferred key attribute holds + /// a value that stays with that object. Matching on the changeable value alone turns such a + /// change into a deletion plus an addition. + /// Elements carrying a preferred value and elements carrying none are matched + /// separately, and never against each other, which keeps matching an equivalence relation: + /// no element is paired with two partners, and no edit lands on an object it was not made + /// against. An attribute that is present but empty counts as absent throughout. + /// The price is that an element whose preferred value one revision writes and another + /// omits reads as a deletion plus an addition, and as a removed-versus-edited conflict where + /// the other revision edited it. Choose a preferred key attribute only where every writer can + /// be relied on to preserve the value it finds; writers that disagree pay that cost on every + /// merge between them. /// public class FindByPreferredKeyAttribute : IFindMatchingNodesToMerge { @@ -262,8 +259,8 @@ public XmlNode GetNodeToMerge(XmlNode nodeToMatch, XmlNode parentToSearchIn, Has XmlNode match; if (string.IsNullOrEmpty(preferredKey)) { - // Only among elements that likewise carry no preferred value, so that this cannot claim - // an element that a preferred value already speaks for. + // Only among elements that likewise carry no preferred value, so that this + // cannot claim an element that a preferred value already speaks for. index.ByFallbackKeyWherePreferredAbsent.TryGetValue(IndexKey(nodeToMatch, fallbackKey), out match); return match; // May be null, which is fine. } @@ -272,11 +269,10 @@ public XmlNode GetNodeToMerge(XmlNode nodeToMatch, XmlNode parentToSearchIn, Has } /// - /// The children of one parent, indexed on (element name, attribute value) for each key attribute this - /// finder can match on, so that merging a large element does not rescan its siblings once per child. - /// An index is built when its parent is first searched and is never revisited, so a caller that adds - /// or removes children between searches of the same parent will still be answered from the children - /// it first held. + /// The children of one parent, indexed on (element name, attribute value) for each key + /// attribute this finder can match on, so siblings are not rescanned once per child. + /// Built at the parent's first search and never revisited: children added or removed + /// between searches of one parent are not seen. /// private class ParentIndex { @@ -308,9 +304,9 @@ private ParentIndex GetIndexFor(XmlNode parentToSearchIn) } /// - /// Unlike a finder with a single key attribute, two children legitimately hold the same fallback - /// value here, since a preferred value can still tell them apart. Keep the first, matching how the - /// merger resolves siblings it cannot tell apart. + /// Unlike a finder with a single key attribute, two children legitimately hold the same + /// fallback value here, since a preferred value can still tell them apart. Keep the + /// first, matching how the merger resolves siblings it cannot tell apart. /// private static void IndexFirstOnly(IDictionary, XmlNode> index, XmlNode childNode, string keyValue) { @@ -320,9 +316,9 @@ private static void IndexFirstOnly(IDictionary, XmlNode> i } /// - /// A ParentIndex entry is identified by the element name as well as the attribute value, matching the - /// tuple indexes on: elements of different names never match, however - /// their keys compare. + /// A ParentIndex entry is identified by the element name as well as the attribute value, + /// matching the tuple indexes on: elements of different + /// names never match, however their keys compare. /// private static Tuple IndexKey(XmlNode element, string keyValue) { @@ -365,7 +361,7 @@ public IEnumerable GetMatchingNodes(XmlNode nodeToMatch, XmlNode parent private bool IsMatch(XmlNode candidate, string soughtPreferredKey, string soughtFallbackKey) { var candidatePreferredKey = XmlUtilities.GetOptionalAttributeString(candidate, _preferredKeyAttribute); - // One carries a preferred value and the other does not, so they are matched separately. + // One carries a preferred value and the other does not, so they are matched apart. if (string.IsNullOrEmpty(soughtPreferredKey) != string.IsNullOrEmpty(candidatePreferredKey)) return false; // Both carry one, and it decides alone: the fallback value may have changed. @@ -384,10 +380,9 @@ public string GetWarningMessageForAmbiguousNodes(XmlNode nodeForMessage) { Guard.AgainstNull(nodeForMessage, "nodeForMessage"); - // Whether the element carries a preferred value decides which key attribute it was matched on, so - // that is the attribute whose values are the same. Naming the other one too would accuse an - // attribute the match never consulted, and whose values, for two elements sharing a preferred - // value, may well differ between them. + // Whether the element carries a preferred value decides which key attribute it was + // matched on, so that is the attribute whose values are the same; naming the other + // would accuse an attribute the match never consulted. var preferredKey = XmlUtilities.GetOptionalAttributeString(nodeForMessage, _preferredKeyAttribute); var matchedOnPreferredKey = !string.IsNullOrEmpty(preferredKey); return string.Format("The key attribute '{0}' has values that are the same '{1}'", diff --git a/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs b/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs index 4c3cc1ca5..61cfbf49f 100644 --- a/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs +++ b/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs @@ -277,7 +277,7 @@ public void RespelledIdMergesAsAnEditWhenTheGuidIsUnchanged() } /// - /// The guid is optional in LIFT, so a file written without one must still merge on the id. + /// @guid is optional in LIFT, so a file written without one must still merge on its id. /// [Test] public void RangeElementWithoutAGuidStillMergesOnItsId() @@ -294,11 +294,32 @@ public void RangeElementWithoutAGuidStillMergesOnItsId() } /// - /// An element that names a guid is matched only against elements that likewise name one, so a - /// writer that drops the guid a previous writer wrote loses the object's identity: the element - /// reads as a deletion and its guid-less replacement as an addition. That is the cost of keeping - /// the two sets apart, and a project whose writers disagree over the guid pays it on every merge - /// between them. + /// The finder branches on a value, not on the attribute, so @guid='' counts as no guid at + /// all: such an element is matched on its @id, and matches one that omits @guid outright, + /// rather than reading as a deletion plus an addition. + /// + [Test] + public void AnEmptyGuidCountsAsNoGuidAtAll() + { + var common = RangesWithPartOfSpeech("Noun", null, "old"); + // Our writer emits the attribute but leaves it empty. + var ours = RangesWithPartOfSpeech("Noun", "", "old"); + var theirs = RangesWithPartOfSpeech("Noun", null, "new"); + + // Two changes for our added attribute, as in the respelled-id case; none for theirs. + var result = DoMerge(common, ours, theirs, 0, 2); + + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath("//range/range-element", 1); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( + "//range-element/abbrev/form/text[text()='new']", 1); + } + + /// + /// An element that names a guid is matched only against elements that likewise name + /// one, so a writer that drops the guid a previous writer wrote loses the object's + /// identity: the element reads as a deletion and its guid-less replacement as an + /// addition. That is the cost of keeping the two sets apart, and a project whose + /// writers disagree over the guid pays it on every merge between them. /// [Test] public void DroppingTheGuidReadsAsADeletionPlusAnAddition() @@ -319,8 +340,8 @@ public void DroppingTheGuidReadsAsADeletionPlusAnAddition() } /// - /// Two possibilities that happen to share a name are still two possibilities. Matching them - /// on the id would merge them into one and lose a guid. + /// Two possibilities that happen to share a name are still two possibilities. + /// Matching them on the id would merge them into one and lose a guid. /// [Test] public void RangeElementsWithDifferentGuidsAreNotMatched() @@ -344,8 +365,8 @@ public void RangeElementsWithDifferentGuidsAreNotMatched() } /// - /// A sibling that still carries the old id, written by a tool that omits guids, must not be - /// taken as the partner just because it comes first in the file. + /// A sibling that still carries the old id, written by a tool that omits guids, must + /// not be taken as the partner just because it comes first in the file. /// [Test] public void GuidMatchIsPreferredOverAnEarlierSiblingMatchingOnTheOldId() @@ -366,12 +387,12 @@ public void GuidMatchIsPreferredOverAnEarlierSiblingMatchingOnTheOldId() ", kDecomposedId, kComposedId, kPosGuid); var theirs = RangesWithPartOfSpeech(kDecomposedId, kPosGuid, "new"); - // No conflict: the guid-less sibling is matched only against other guid-less elements, so it - // cannot also claim the element the guid already speaks for. + // No conflict: the guid-less sibling is matched only against other guid-less + // elements, so it cannot also claim the element the guid already speaks for. var result = DoMerge(common, ours, theirs, 0, 3); AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath("//range/range-element", 2); - // Their edit belongs to the element sharing the guid, not to the one sharing the old id. + // Their edit belongs to the element sharing the guid, not the one sharing the old id. AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath(string.Format( "//range-element[@guid='{0}']/abbrev/form/text[text()='new']", kPosGuid), 1); AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( @@ -379,11 +400,11 @@ public void GuidMatchIsPreferredOverAnEarlierSiblingMatchingOnTheOldId() } /// - /// A merge done before @guid was preferred could leave one possibility standing as two - /// range-elements, spelled differently. Matching on the guid value makes them ambiguous siblings, - /// which the merger collapses back to one. The warning must name @guid as the attribute whose - /// values are the same, and quote the shared value, since the differing ids are not what made - /// them indistinguishable. + /// A merge done before @guid was preferred could leave one possibility standing as + /// two range-elements, spelled differently. Matching on the guid value makes them + /// ambiguous siblings, which the merger collapses back to one. The warning must name + /// @guid as the attribute whose values are the same, and quote the shared value, + /// since the differing ids are not what made them indistinguishable. /// [Test] public void RangeElementsDuplicatedByAnEarlierMergeCollapseToOne() @@ -410,14 +431,17 @@ public void RangeElementsDuplicatedByAnEarlierMergeCollapseToOne() Assert.That(warning, Does.Not.Contain("'id'"), warning); } - // The two @id values a part of speech named "Compléments" is exported under. Built rather than written - // out, since the two spellings are indistinguishable in source and an editor or a text filter that - // normalizes on save would silently make them the same string. + // The two @id values a part of speech named "Compléments" is exported under. Built + // rather than written out, since the two spellings are indistinguishable in source + // and an editor or a text filter that normalizes on save would silently make them + // the same string. private static readonly string kDecomposedId = "Compléments".Normalize(NormalizationForm.FormD); private static readonly string kComposedId = "Compléments".Normalize(NormalizationForm.FormC); private const string kPosGuid = "e8c4b4b0-1a2f-4f9e-9f39-3f2b0d8f7a11"; - /// A null leaves @guid off the element altogether. + /// + /// A null leaves @guid off the element altogether. + /// private static string RangesWithPartOfSpeech(string posId, string posGuidOrNull, string abbrevText) { return string.Format( From ed84df84c8c2ccc9178e4fc38c786cd5e4b4404c Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Mon, 17 Aug 2026 15:37:59 -0400 Subject: [PATCH 10/11] Describe the index helpers by what they do, not by their callers Both helpers are shared by the preferred-key and fallback-key indexes, so neither can speak for one of them. IndexFirstOnly said two children legitimately share a fallback value because a preferred value tells them apart. That is true of the class's matching rule but false of every collision this method actually swallows: the fallback index holds only children carrying no preferred value, so colliding entries there have nothing to tell them apart, and a collision in the preferred index is the duplicated pair an earlier merge left behind. Say instead that a repeated key value is not an error here, which is what the code cannot say and what departs from FindByKeyAttribute. IndexKey opened on the ParentIndex entry it feeds and on the tuple FindByKeyAttribute indexes; keep only why the element name is in the key. Co-Authored-By: Claude Opus 5 (1M context) --- src/LibChorus/merge/xml/generic/FindNodeToMerge.cs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs b/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs index a4175c180..d3092725f 100644 --- a/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs +++ b/src/LibChorus/merge/xml/generic/FindNodeToMerge.cs @@ -304,9 +304,8 @@ private ParentIndex GetIndexFor(XmlNode parentToSearchIn) } /// - /// Unlike a finder with a single key attribute, two children legitimately hold the same - /// fallback value here, since a preferred value can still tell them apart. Keep the - /// first, matching how the merger resolves siblings it cannot tell apart. + /// Two children can carry the same key value, which is not an error here: keep the first + /// and ignore the rest, matching how the merger resolves siblings it cannot tell apart. /// private static void IndexFirstOnly(IDictionary, XmlNode> index, XmlNode childNode, string keyValue) { @@ -316,9 +315,8 @@ private static void IndexFirstOnly(IDictionary, XmlNode> i } /// - /// A ParentIndex entry is identified by the element name as well as the attribute value, - /// matching the tuple indexes on: elements of different - /// names never match, however their keys compare. + /// The element name is part of the key, so elements of different names never match, + /// however their key values compare. /// private static Tuple IndexKey(XmlNode element, string keyValue) { From a4398e7ca1315a2bb2d6788cc6b82900537540bb Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Mon, 17 Aug 2026 16:21:23 -0400 Subject: [PATCH 11/11] Pin what the duplicate-guid collapse costs RangeElementsDuplicatedByAnEarlierMergeCollapseToOne gave both duplicates the same abbrev, so it showed the pair becoming one but never that the loser is discarded rather than reconciled. Give them different content, and a label on the loser alone, so the assertions say which of the two survives and that the other's content does not come with it. The collapse keeps the first in document order. RemoveAmbiguousChildren runs over each revision as it is read, before the merge has any view of which side wrote which element, so there is no better winner for it to pick; the point of the test is that the cost is stated rather than discovered. Co-Authored-By: Claude Opus 5 (1M context) --- .../LiftRanges/LiftRangesFileHandlerTests.cs | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs b/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs index 61cfbf49f..c6cb9e39a 100644 --- a/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs +++ b/src/LibChorusTests/FileHandlers/LiftRanges/LiftRangesFileHandlerTests.cs @@ -406,6 +406,13 @@ public void GuidMatchIsPreferredOverAnEarlierSiblingMatchingOnTheOldId() /// @guid as the attribute whose values are the same, and quote the shared value, /// since the differing ids are not what made them indistinguishable. /// + /// + /// The collapse keeps the first of the pair in document order and discards the other + /// outright, so whatever the earlier merge left on the loser goes with it. The two are + /// given different content here to pin that cost rather than hide it behind a pair that + /// happens to agree. There is no better winner available: this is a sanitizing pass over + /// each revision as it is read, with no view of which side wrote which. + /// [Test] public void RangeElementsDuplicatedByAnEarlierMergeCollapseToOne() { @@ -414,10 +421,11 @@ public void RangeElementsDuplicatedByAnEarlierMergeCollapseToOne() -
old
 +
kept
 -
old
 +
dropped
 + ", kDecomposedId, kComposedId, kPosGuid); @@ -425,6 +433,13 @@ public void RangeElementsDuplicatedByAnEarlierMergeCollapseToOne() var result = DoMerge(duplicated, duplicated, duplicated, 0, 0); AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath("//range/range-element", 1); + // The loser's content is discarded, not merged into the survivor. + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( + "//range-element/abbrev/form/text[text()='kept']", 1); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath( + "//range-element/abbrev/form/text[text()='dropped']", 0); + AssertThatXmlIn.String(result).HasSpecifiedNumberOfMatchesForXpath("//range-element/label", 0); + Assert.That(_eventListener.Warnings, Is.Not.Empty, "the dropped duplicate should be reported"); var warning = _eventListener.Warnings[0].GetFullHumanReadableDescription(); Assert.That(warning, Does.Contain("'guid'").And.Contain(kPosGuid), warning);