From 0c3071b9c68fa403e3caba18af62d2e4131e4789 Mon Sep 17 00:00:00 2001
From: RoosterDragon <RoosterDragon@outlook.com>
Date: Wed, 27 Apr 2022 21:40:16 +0100
Subject: [PATCH] Adjust MiniYaml node merging when removals are present.

# Example Scenario
MockString:
	CollectionOfStrings:
		StringA: A

MockString:
	CollectionOfStrings:
		StringB: B

MockString:
	-CollectionOfStrings:

MockString:
	CollectionOfStrings:
		StringC: C

MockString:
	CollectionOfStrings:
		StringD: D

# Previous MergePartial result
# The CollectionOfStrings is merged into a single unit, so the C and D items are dragged upwards and jump ahead of the Removal
# When this is processed, the final result removes CollectionOfStrings entirely

MockString:
	CollectionOfStrings:
		StringA: A
		StringB: B
		StringC: C
		StringD: D
	-CollectionOfStrings:

# New MergePartial result
# When merging nodes, we no longer allow merges to jump an intervening removal node.
# This means we can have multiple of a certain key (CollectionOfStrings in this example) which was not the case previously.
# When this is processed, the final result includes C/D but not A/B.

MockString:
	CollectionOfStrings:
		StringA: A
		StringB: B
	-CollectionOfStrings:
	CollectionOfStrings:
		StringC: C
		StringD: D
---
 OpenRA.Game/MiniYaml.cs | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/OpenRA.Game/MiniYaml.cs b/OpenRA.Game/MiniYaml.cs
index 93aa717d64..3c37d26e0a 100644
--- a/OpenRA.Game/MiniYaml.cs
+++ b/OpenRA.Game/MiniYaml.cs
@@ -434,18 +434,39 @@ namespace OpenRA
 
 			var existingDict = existingNodes.ToDictionaryWithConflictLog(x => x.Key, "MiniYaml.Merge", null, x => $"{x.Key} (at {x.Location})");
 			var overrideDict = overrideNodes.ToDictionaryWithConflictLog(x => x.Key, "MiniYaml.Merge", null, x => $"{x.Key} (at {x.Location})");
-			var allKeys = existingDict.Keys.Union(overrideDict.Keys);
 
-			foreach (var key in allKeys)
+			foreach (var node in existingNodes.Concat(overrideNodes))
 			{
-				existingDict.TryGetValue(key, out var existingNode);
-				overrideDict.TryGetValue(key, out var overrideNode);
+				// Append Removal nodes to the result.
+				// Therefore: we know the remainder of the loop deals with plain nodes.
+				if (node.Key.StartsWith("-", StringComparison.Ordinal))
+				{
+					ret.Add(node);
+					continue;
+				}
 
-				var loc = overrideNode?.Location ?? default;
-				var comment = (overrideNode ?? existingNode).Comment;
-				var merged = (existingNode == null || overrideNode == null) ? overrideNode ?? existingNode :
-					new MiniYamlNode(key, MergePartial(existingNode.Value, overrideNode.Value), comment, loc);
-				ret.Add(merged);
+				// If no previous node with this key is present, it is new and can just be appended.
+				var previousNodeIndex = ret.FindLastIndex(n => n.Key == node.Key);
+				if (previousNodeIndex == -1)
+				{
+					ret.Add(node);
+					continue;
+				}
+
+				// A Removal node is closer than the previous node.
+				// We should not merge the new node, as the data being merged will jump before the Removal.
+				// Instead, append it so the previous node is applied, then removed, then the new node is applied.
+				var removalKey = $"-{node.Key}";
+				var previousRemovalNodeIndex = ret.FindLastIndex(n => n.Key == removalKey);
+				if (previousRemovalNodeIndex != -1 && previousRemovalNodeIndex > previousNodeIndex)
+				{
+					ret.Add(node);
+					continue;
+				}
+
+				// A previous node is present with no intervening Removal.
+				// We should merge the new one into it, in place.
+				ret[previousNodeIndex] = new MiniYamlNode(node.Key, MergePartial(ret[previousNodeIndex].Value, node.Value), node.Comment, node.Location);
 			}
 
 			ret.TrimExcess();