fix(model-datastructure): fix merge algorithm

Fix topological sort in `LinearHistory`.
Add fast-forward merge in `VersionMerger`.
The fast-forward merge is needed with the new topological sorting.

Author: Oleksandr Dzhychko

model-datastructure/src/commonMain/kotlin/org/modelix/model/LinearHistory.kt

@@ -1,99 +1,60 @@
 package org.modelix.model
 
 import org.modelix.model.lazy.CLVersion
-import org.modelix.model.lazy.IDeserializingKeyValueStore
-import org.modelix.model.lazy.KVEntryReference
-import org.modelix.model.persistent.CPVersion
 
-/**
- * Was introduced in https://github.com/modelix/modelix/commit/19c74bed5921028af3ac3ee9d997fc1c4203ad44
- * together with the UndoOp. The idea is that an undo should only revert changes if there is no other change that relies
- * on it. In that case the undo should do nothing, to not indirectly undo newer changes.
- * For example, if you added a node and someone else started changing properties on the that node, your undo should not
- * remove the node to not lose the property changes.
- * This requires the versions to be ordered in a way that the undo appears later.
- */
 class LinearHistory(val baseVersionHash: String?) {
 
-    val version2directDescendants: MutableMap<Long, Set<Long>> = HashMap()
-    val versions: MutableMap<Long, CLVersion> = LinkedHashMap()
-
     /**
-     * @param fromVersions it is assumed that the versions are sorted by the oldest version first. When merging a new
-     *        version into an existing one the new version should appear after the existing one. The resulting order
-     *        will prefer existing versions to new ones, meaning during the conflict resolution the existing changes
-     *        have a higher probability of surviving.
-     * @returns oldest version first
+     * Order all versions descending from any versions in [[fromVersions]] topologically.
+     * This means that a version must come after all its descendants.
+     * Returns the ordered versions starting with the earliest version.
      */
-    fun load(vararg fromVersions: CLVersion): List<CLVersion> {
-        for (fromVersion in fromVersions) {
-            collect(fromVersion)
-        }
-
-        var result: List<Long> = emptyList()
-
-        for (version in versions.values.filter { !it.isMerge() }.sortedBy { it.id }) {
-            val descendantIds = collectAllDescendants(version.id).filter { !versions[it]!!.isMerge() }.sorted().toSet()
-            val idsInResult = result.toHashSet()
-            if (idsInResult.contains(version.id)) {
-                result =
-                    result +
-                    descendantIds.filter { !idsInResult.contains(it) }
-            } else {
-                result =
-                    result.filter { !descendantIds.contains(it) } +
-                    version.id +
-                    result.filter { descendantIds.contains(it) } +
-                    descendantIds.filter { !idsInResult.contains(it) }
+    fun loadLazy(vararg fromVersions: CLVersion) = sequence {
+        // The algorithm sorts the versions topologically.
+        // It performs a depth-first search.
+        // It is implemented as an iterative algorithm with a stack.
+
+        val stack = ArrayDeque<CLVersion>()
+        val visited = mutableSetOf<CLVersion>()
+
+        // Ensure deterministic merging,
+        // by putting versions with lower id before versions with higher id.
+        fromVersions.sortedBy { it.id }.forEach { fromVersion ->
+            // Not putting fromVersions directly on the stack and checking visited.contains(fromVersion) ,
+            // ensures the algorithm terminates if one version in `fromVersion`
+            // is a descendant of another version in `fromVersion`
+            if (!visited.contains(fromVersion)) {
+                stack.addLast(fromVersion)
             }
-        }
-        return result.map { versions[it]!! }
-    }
-
-    private fun collectAllDescendants(root: Long): Set<Long> {
-        val result = LinkedHashSet<Long>()
-        var previousSize = 0
-        result += root
-
-        while (previousSize != result.size) {
-            val nextElements = result.asSequence().drop(previousSize).toList()
-            previousSize = result.size
-            for (ancestor in nextElements) {
-                version2directDescendants[ancestor]?.let { result += it }
-            }
-        }
-
-        return result.drop(1).toSet()
-    }
-
-    private fun collect(root: CLVersion) {
-        if (root.getContentHash() == baseVersionHash) return
-
-        var previousSize = versions.size
-        versions[root.id] = root
-
-        while (previousSize != versions.size) {
-            val nextElements = versions.asSequence().drop(previousSize).map { it.value }.toList()
-            previousSize = versions.size
-
-            for (descendant in nextElements) {
-                val ancestors = if (descendant.isMerge()) {
-                    sequenceOf(
-                        getVersion(descendant.data!!.mergedVersion1!!, descendant.store),
-                        getVersion(descendant.data!!.mergedVersion2!!, descendant.store),
-                    )
+            while (stack.isNotEmpty()) {
+                val version = stack.last()
+                val versionWasVisited = !visited.add(version)
+                if (versionWasVisited) {
+                    stack.removeLast()
+                    if (!version.isMerge()) {
+                        yield(version)
+                    }
+                }
+                val descendants = if (version.isMerge()) {
+                    // Put version 1 last, so that is processed first.
+                    // We are using a stack and the last version is viewed first.
+                    listOf(version.getMergedVersion2()!!, version.getMergedVersion1()!!)
                 } else {
-                    sequenceOf(descendant.baseVersion)
-                }.filterNotNull().filter { it.getContentHash() != baseVersionHash }.toList()
-                for (ancestor in ancestors) {
-                    versions[ancestor.id] = ancestor
-                    version2directDescendants[ancestor.id] = (version2directDescendants[ancestor.id] ?: emptySet()) + setOf(descendant.id)
+                    listOfNotNull(version.baseVersion)
                 }
+                // Ignore descendant, if it is a base version.
+                val relevantDescendants = descendants.filter { it.getContentHash() != baseVersionHash }
+                // Ignore already visited descendants.
+                val nonCheckedDescendants = relevantDescendants.filterNot { visited.contains(it) }
+                nonCheckedDescendants.forEach { stack.addLast(it) }
             }
         }
     }
 
-    private fun getVersion(hash: KVEntryReference<CPVersion>, store: IDeserializingKeyValueStore): CLVersion {
-        return CLVersion(hash.getValue(store), store)
+    /**
+     * Same as [[loadLazy]], but returning as a list instead of a lazy sequence.
+     */
+    fun load(vararg fromVersions: CLVersion): List<CLVersion> {
+        return loadLazy(*fromVersions).toList()
     }
 }

model-datastructure/src/commonMain/kotlin/org/modelix/model/VersionMerger.kt

@@ -45,30 +45,27 @@ class VersionMerger(private val storeCache: IDeserializingKeyValueStore, private
         }
     }
 
-    private fun collectLatestNonMerges(version: CLVersion?, visited: MutableSet<String>, result: MutableSet<Long>) {
-        if (version == null) return
-        if (!visited.add(version.getContentHash())) return
-        if (version.isMerge()) {
-            collectLatestNonMerges(version.getMergedVersion1(), visited, result)
-            collectLatestNonMerges(version.getMergedVersion2(), visited, result)
-        } else {
-            result.add(version.id)
-        }
-    }
-
     protected fun mergeHistory(leftVersion: CLVersion, rightVersion: CLVersion): CLVersion {
         if (leftVersion.hash == rightVersion.hash) return leftVersion
-        val commonBase = Companion.commonBaseVersion(leftVersion, rightVersion)
+        val commonBase = commonBaseVersion(leftVersion, rightVersion)
         if (commonBase?.hash == leftVersion.hash) return rightVersion
         if (commonBase?.hash == rightVersion.hash) return leftVersion
 
-        val leftNonMerges = HashSet<Long>().also { collectLatestNonMerges(leftVersion, HashSet(), it) }
-        val rightNonMerges = HashSet<Long>().also { collectLatestNonMerges(rightVersion, HashSet(), it) }
+        val leftNonMerges = LinearHistory(commonBase?.hash).loadLazy(leftVersion).toSet()
+        val rightNonMerges = LinearHistory(commonBase?.hash).loadLazy(rightVersion).toSet()
         if (leftNonMerges == rightNonMerges) {
             // If there is no actual change on both sides, but they just did the same merge, we have to pick one
             // of them, otherwise both sides will continue creating merges forever.
             return if (leftVersion.id < rightVersion.id) leftVersion else rightVersion
         }
+        if (leftNonMerges.containsAll(rightNonMerges)) {
+            // No merge needed, fast-forward to the left version
+            return leftVersion
+        }
+        if (rightNonMerges.containsAll(leftNonMerges)) {
+            // No merge needed, fast-forward to the right version
+            return rightVersion
+        }
 
         val versionsToApply = LinearHistory(commonBase?.hash).load(leftVersion, rightVersion)
 

model-datastructure/src/commonTest/kotlin/LinearHistoryTest.kt

@@ -23,6 +23,7 @@ import org.modelix.model.operations.IOperation
 import org.modelix.model.persistent.MapBaseStore
 import kotlin.test.Test
 import kotlin.test.assertEquals
+import kotlin.test.assertTrue
 
 class LinearHistoryTest {
     val initialTree = CLTree.builder(ObjectStoreCache(MapBaseStore())).repositoryId("LinearHistoryTest").build()
@@ -149,37 +150,90 @@ class LinearHistoryTest {
 
         // val expected = SlowLinearHistory(v1000004d3.getContentHash()).load(v300000075, v1000004ee)
         val expected = listOf(
-            v1000004d5,
             v200000353,
+            v1000004d5,
+            v200000356,
             v1000004d8,
+            v200000359,
             v1000004dc,
             v1000004df,
-            v1000004e2,
-            v1000004e5,
-            v1000004e7,
-            v1000004e9,
-            v200000356,
-            v200000359,
             v20000035d,
             v20000035f,
+            v1000004e2,
             v200000362,
+            v1000004e5,
             v200000365,
+            v1000004e7,
             v200000367,
+            v1000004e9,
             v200000369,
             v20000036c,
         )
         assertHistory(v300000075, v1000004ee, expected)
     }
 
+    @Test
+    fun correctHistoryIfIdsAreNotAscending() {
+        val v1 = version(1, null)
+        val v2 = version(2, v1)
+        val v3 = version(3, v1)
+        val v9 = version(9, v2)
+        val v4 = merge(4, v2, v3)
+        val v8 = version(8, v9)
+
+        val expected = listOf(v2, v3, v9, v8)
+        assertHistory(v4, v8, expected)
+    }
+
     private fun assertHistory(v1: CLVersion, v2: CLVersion, expected: List<CLVersion>) {
-        val actual = history(v1, v2)
-        assertEquals(expected.map { it.id.toString(16) }, actual.map { it.id.toString(16) })
-        assertEquals(expected, actual)
+        val historyMergingFirstIntoSecond = history(v1, v2)
+        val historyMeringSecondIntoFirst = history(v2, v1)
+        assertEquals(
+            historyMergingFirstIntoSecond.map { it.id.toString(16) },
+            historyMeringSecondIntoFirst.map { it.id.toString(16) },
+        )
+        assertEquals(expected.map { it.id.toString(16) }, historyMergingFirstIntoSecond.map { it.id.toString(16) })
     }
 
     private fun history(v1: CLVersion, v2: CLVersion): List<CLVersion> {
         val base = VersionMerger.commonBaseVersion(v1, v2)
-        return LinearHistory(base?.getContentHash()).load(v1, v2)
+        val history = LinearHistory(base?.getContentHash()).load(v1, v2)
+        assertHistoryIsCorrect(history)
+        return history
+    }
+
+    private fun assertHistoryIsCorrect(history: List<CLVersion>) {
+        history.forEach { version ->
+            val versionIndex = history.indexOf(version)
+            getDescendants(version).forEach { descendent ->
+                val descendantIndex = history.indexOf(descendent)
+                // A descendant might not be in history
+                // (1) if it is a merge or
+                // (2) if it is a descendant of a common version.
+                // The descendantIndex is then -1.
+                assertTrue(
+                    versionIndex > descendantIndex,
+                    "${version.id.toString(16)} must come after its descendant ${descendent.id.toString(16)} in ${history.map { it.id.toString() }} .",
+                )
+            }
+        }
+    }
+
+    private fun getChildren(version: CLVersion): List<CLVersion> {
+        return if (version.isMerge()) {
+            listOf(version.getMergedVersion1()!!, version.getMergedVersion2()!!)
+        } else {
+            listOfNotNull(version.baseVersion)
+        }
+    }
+
+    private fun getDescendants(version: CLVersion): MutableSet<CLVersion> {
+        val descendants = mutableSetOf<CLVersion>()
+        getChildren(version).forEach { descendant ->
+            descendants.add(descendant)
+            descendants.addAll(getDescendants(descendant))
+        }
+        return descendants
     }
 
     private fun version(id: Long, base: CLVersion?): CLVersion {

model-datastructure/src/commonTest/kotlin/UndoTest.kt

@@ -36,6 +36,30 @@ class UndoTest {
 
     @Test
     fun undo_random() {
+        /*
+            digraph G {
+              1 [label="id=1"]
+              2 [label="id=2\nv[0]"]
+              3 [label="id=3\nv[1]"]
+              4 [label="id=4\nv[2]"]
+              undo [label="id=5\nversion_1_1u\n>>undo v[1]<<"]
+              version_0_1 [label="id=30064771077\nversion_0_1"]
+              version_2_1 [label="id=30064771078\nversion_2_1"]
+
+              2 -> 1
+              3 -> 1
+              4 -> 1
+              undo -> 3
+              version_0_1 -> 2
+              version_0_1 -> 3
+              version_2_1 -> 3
+              version_2_1 -> 4
+              version_0_1_1u -> version_0_1
+              version_0_1_1u -> undo
+              version_2_1_1u -> undo
+              version_2_1_1u -> version_2_1
+            }
+         */
         val idGenerator = IdGenerator.newInstance(7)
         val versionIdGenerator = IdGenerator.newInstance(0)
         val store = ObjectStoreCache(MapBaseStore())