Improve and expand the changeset calculation tests

src/collection_notifications.cpp

@@ -332,7 +332,7 @@ struct RowInfo {
     size_t shifted_tv_index;
 };
 
-void calculate_moves_unsorted(std::vector<RowInfo>& new_rows, IndexSet const& removed, CollectionChangeIndices& changeset)
+void calculate_moves_unsorted(std::vector<RowInfo>& new_rows, IndexSet& removed, CollectionChangeIndices& changeset)
 {
     size_t expected = 0;
     for (auto& row : new_rows) {
@@ -356,7 +356,7 @@ void calculate_moves_unsorted(std::vector<RowInfo>& new_rows, IndexSet const& re
         // The row still isn't the expected one, so it's a move
         changeset.moves.push_back({row.prev_tv_index, row.tv_index});
         changeset.insertions.add(row.tv_index);
-        changeset.deletions.add(row.prev_tv_index);
+        removed.add(row.prev_tv_index);
     }
 }
 
@@ -446,14 +446,18 @@ private:
             cur.clear();
 
             size_t ai = a[i].row_index;
+            // Find the TV indicies at which this row appears in the new results
+            // There should always be at least one (or it would have been filtered out earlier),
+            // but can be multiple if there are dupes
             auto it = lower_bound(begin(b), end(b), Row{ai, 0},
                                   [](auto a, auto b) { return a.row_index < b.row_index; });
+            REALM_ASSERT(it != end(b) && it->row_index == ai);
             for (; it != end(b) && it->row_index == ai; ++it) {
                 size_t j = it->tv_index;
                 if (j < begin2)
                     continue;
                 if (j >= end2)
-                    break;
+                    break; // b is sorted by tv_index so this can't transition from false to true
 
                 size_t size = length(j);
                 cur.push_back({j, size});
@@ -478,6 +482,10 @@ private:
     void find_longest_matches(size_t begin1, size_t end1, size_t begin2, size_t end2)
     {
         // FIXME: recursion could get too deep here
+        // recursion depth worst case is currently O(N) and each recursion uses 320 bytes of stack
+        // could reduce worst case to O(sqrt(N)) (and typical case to O(log N))
+        // biasing equal selections towards the middle, but that's still
+        // insufficient for Android's 8 KB stacks
         auto m = find_longest_match(begin1, end1, begin2, end2);
         if (!m.size)
             return;
@@ -496,6 +504,8 @@ CollectionChangeBuilder CollectionChangeBuilder::calculate(std::vector<size_t> c
                                                            std::function<bool (size_t)> row_did_change,
                                                            bool sort)
 {
+    REALM_ASSERT_DEBUG(sort || std::is_sorted(begin(next_rows), end(next_rows)));
+
     CollectionChangeBuilder ret;
 
     size_t deleted = 0;

tests/collection_change_indices.cpp

@@ -4,18 +4,18 @@
 
 #include "util/index_helpers.hpp"
 
-TEST_CASE("collection change indices") {
+using namespace realm;
-    using namespace realm;
+
+TEST_CASE("[collection_change] insert()") {
     _impl::CollectionChangeBuilder c;
 
-    SECTION("basic mutation functions") {
+    SECTION("adds the row to the insertions set") {
-        SECTION("insert() adds the row to the insertions set") {
         c.insert(5);
         c.insert(8);
         REQUIRE_INDICES(c.insertions, 5, 8);
     }
 
-        SECTION("insert() shifts previous insertions and modifications") {
+    SECTION("shifts previous insertions and modifications") {
         c.insert(5);
         c.modify(8);
 
@@ -24,7 +24,7 @@ TEST_CASE("collection change indices") {
         REQUIRE_INDICES(c.modifications, 9);
     }
 
-        SECTION("insert() does not shift previous deletions") {
+    SECTION("does not shift previous deletions") {
         c.erase(8);
         c.erase(3);
         c.insert(5);
@@ -33,78 +33,204 @@ TEST_CASE("collection change indices") {
         REQUIRE_INDICES(c.deletions, 3, 8);
     }
 
-        SECTION("modify() adds the row to the modifications set") {
+    SECTION("shifts destination of previous moves after the insertion point") {
-            c.modify(3);
+        c.moves = {{10, 5}, {10, 2}, {3, 10}};
-            c.modify(4);
+        c.insert(4);
-            REQUIRE_INDICES(c.modifications, 3, 4);
+        REQUIRE_MOVES(c, {10, 6}, {10, 2}, {3, 11});
+    }
+}
+
+TEST_CASE("[collection_change] modify()") {
+    _impl::CollectionChangeBuilder c;
+
+    SECTION("marks the row as modified") {
+        c.modify(5);
+        REQUIRE_INDICES(c.modifications, 5);
     }
 
-        SECTION("modify() on an inserted row marks it as both inserted and modified") {
+    SECTION("also marks newly inserted rows as modified") {
-            c.insert(3);
+        c.insert(5);
-            c.modify(3);
+        c.modify(5);
-            REQUIRE_INDICES(c.insertions, 3);
+        REQUIRE_INDICES(c.modifications, 5);
-            REQUIRE_INDICES(c.modifications, 3);
     }
 
-        SECTION("modify() doesn't interact with deleted rows") {
+    SECTION("is idempotent") {
-            c.erase(5);
+        c.modify(5);
-            c.erase(4);
+        c.modify(5);
-            c.erase(3);
+        c.modify(5);
-
+        c.modify(5);
-            c.modify(4);
+        REQUIRE_INDICES(c.modifications, 5);
-            REQUIRE_INDICES(c.modifications, 4);
     }
+}
 
-        SECTION("erase() adds the row to the deletions set") {
+TEST_CASE("[collection_change] erase()") {
+    _impl::CollectionChangeBuilder c;
+
+    SECTION("adds the row to the deletions set") {
         c.erase(5);
         REQUIRE_INDICES(c.deletions, 5);
     }
 
-        SECTION("erase() is shifted for previous deletions") {
+    SECTION("is shifted for previous deletions") {
         c.erase(5);
         c.erase(6);
         REQUIRE_INDICES(c.deletions, 5, 7);
     }
 
-        SECTION("erase() is shifted for previous insertions") {
+    SECTION("is shifted for previous insertions") {
         c.insert(5);
         c.erase(6);
         REQUIRE_INDICES(c.deletions, 5);
     }
 
-        SECTION("erase() removes previous insertions") {
+    SECTION("removes previous insertions") {
         c.insert(5);
         c.erase(5);
         REQUIRE(c.insertions.empty());
         REQUIRE(c.deletions.empty());
     }
 
-        SECTION("erase() removes previous modifications") {
+    SECTION("removes previous modifications") {
         c.modify(5);
         c.erase(5);
         REQUIRE(c.modifications.empty());
         REQUIRE_INDICES(c.deletions, 5);
     }
 
-        SECTION("erase() shifts previous modifications") {
+    SECTION("shifts previous modifications") {
         c.modify(5);
         c.erase(4);
         REQUIRE_INDICES(c.modifications, 4);
         REQUIRE_INDICES(c.deletions, 4);
     }
 
-        SECTION("move() adds the move to the list of moves") {
+    SECTION("removes previous moves to the row being erased") {
+        c.moves = {{10, 5}};
+        c.erase(5);
+        REQUIRE(c.moves.empty());
+    }
+
+    SECTION("shifts the destination of previous moves") {
+        c.moves = {{10, 5}, {10, 2}, {3, 10}};
+        c.erase(4);
+        REQUIRE_MOVES(c, {10, 4}, {10, 2}, {3, 9});
+    }
+}
+
+TEST_CASE("[collection_change] move_over()") {
+    _impl::CollectionChangeBuilder c;
+
+    SECTION("is just erase when row == last_row") {
+        c.move_over(10, 10);
+        REQUIRE_INDICES(c.deletions, 10);
+        REQUIRE(c.moves.empty());
+    }
+
+    SECTION("marks the old last row as moved") {
+        c.move_over(5, 8);
+        REQUIRE_MOVES(c, {8, 5});
+    }
+
+    SECTION("does not mark the old last row as moved if it was newly inserted") {
+        c.insert(8);
+        c.move_over(5, 8);
+        REQUIRE(c.moves.empty());
+    }
+
+    SECTION("removes previous modifications for the removed row") {
+        c.modify(5);
+        c.move_over(5, 8);
+        REQUIRE(c.modifications.empty());
+    }
+
+    SECTION("updates previous insertions for the old last row") {
+        c.insert(5);
+        c.move_over(3, 5);
+        REQUIRE_INDICES(c.insertions, 3);
+    }
+
+    SECTION("updates previous modifications for the old last row") {
+        c.modify(5);
+        c.move_over(3, 5);
+        REQUIRE_INDICES(c.modifications, 3);
+    }
+
+    SECTION("removes moves to the target") {
+        c.move(3, 5);
+        c.move_over(5, 8);
+        REQUIRE_MOVES(c, {8, 5});
+    }
+
+    SECTION("updates moves to the source") {
+        c.move(3, 8);
+        c.move_over(5, 8);
+        REQUIRE_MOVES(c, {3, 5});
+    }
+
+    SECTION("is not shifted by previous calls to move_over()") {
+        c.move_over(5, 10);
+        c.move_over(6, 9);
+        REQUIRE_INDICES(c.deletions, 5, 6, 9, 10);
+        REQUIRE_INDICES(c.insertions, 5, 6);
+        REQUIRE_MOVES(c, {10, 5}, {9, 6});
+    }
+}
+
+TEST_CASE("[collection_change] clear()") {
+    _impl::CollectionChangeBuilder c;
+
+    SECTION("removes all insertions") {
+        c.insertions = {1, 2, 3};
+        c.clear(0);
+        REQUIRE(c.insertions.empty());
+    }
+
+    SECTION("removes all modifications") {
+        c.modifications = {1, 2, 3};
+        c.clear(0);
+        REQUIRE(c.modifications.empty());
+    }
+
+    SECTION("removes all moves") {
+        c.moves = {{1, 3}};
+        c.clear(0);
+        REQUIRE(c.moves.empty());
+    }
+
+    SECTION("sets deletions to the number of rows before any changes") {
+        c.insertions = {1, 2, 3};
+        c.clear(5);
+        REQUIRE_INDICES(c.deletions, 0, 1);
+
+        c.deletions = {1, 2, 3};
+        c.clear(5);
+        REQUIRE_INDICES(c.deletions, 0, 1, 2, 3, 4, 5, 6, 7);
+    }
+
+    SECTION("sets deletions SIZE_T_MAX if that if the given previous size") {
+        c.insertions = {1, 2, 3};
+        c.clear(SIZE_T_MAX);
+        REQUIRE(c.deletions.size() == 1);
+        REQUIRE(c.deletions.begin()->first == 0);
+        REQUIRE(c.deletions.begin()->second == SIZE_T_MAX);
+    }
+}
+
+TEST_CASE("[collection_change] move()") {
+    _impl::CollectionChangeBuilder c;
+
+    SECTION("adds the move to the list of moves") {
         c.move(5, 6);
         REQUIRE_MOVES(c, {5, 6});
     }
 
-        SECTION("move() updates previous moves to the source of this move") {
+    SECTION("updates previous moves to the source of this move") {
         c.move(5, 6);
         c.move(6, 7);
         REQUIRE_MOVES(c, {5, 7});
     }
 
-        SECTION("move() shifts previous moves and is shifted by them") {
+    SECTION("shifts previous moves and is shifted by them") {
         c.move(5, 10);
         c.move(6, 12);
         REQUIRE_MOVES(c, {5, 9}, {7, 12});
@@ -113,14 +239,14 @@ TEST_CASE("collection change indices") {
         REQUIRE_MOVES(c, {5, 10}, {7, 12}, {11, 0});
     }
 
-        SECTION("moving a newly inserted row is not reported as a move") {
+    SECTION("does not report a move if the source is newly inserted") {
         c.insert(5);
         c.move(5, 10);
         REQUIRE_INDICES(c.insertions, 10);
         REQUIRE(c.moves.empty());
     }
 
-        SECTION("move() shifts previous insertions and modifications") {
+    SECTION("shifts previous insertions and modifications") {
         c.insert(5);
         c.modify(6);
         c.move(10, 0);
@@ -129,134 +255,394 @@ TEST_CASE("collection change indices") {
         REQUIRE_MOVES(c, {9, 0});
     }
 
-        SECTION("move_over() marks the old last row as moved") {
+    SECTION("marks the target row as modified if the source row was") {
-            c.move_over(5, 8);
-            REQUIRE_MOVES(c, {8, 5});
-        }
-
-        SECTION("move_over() does not mark the old last row as moved if it was newly inserted") {
-            c.insert(8);
-            c.move_over(5, 8);
-            REQUIRE(c.moves.empty());
-        }
-
-        SECTION("move_over() removes previous modifications for the removed row") {
         c.modify(5);
-            c.move_over(5, 8);
-            REQUIRE(c.modifications.empty());
-        }
 
-        SECTION("move_over() updates previous insertions for the old last row") {
+        c.move(5, 10);
-            c.insert(5);
+        REQUIRE_INDICES(c.modifications, 10);
-            c.move_over(3, 5);
-            REQUIRE_INDICES(c.insertions, 3);
-        }
 
-        SECTION("move_over() updates previous modifications for the old last row") {
+        c.move(6, 12);
-            c.modify(5);
+        REQUIRE_INDICES(c.modifications, 9);
-            c.move_over(3, 5);
-            REQUIRE_INDICES(c.modifications, 3);
     }
+}
 
-        SECTION("move_over() removes moves to the target") {
+TEST_CASE("[collection_change] calculate() unsorted") {
-            c.move(3, 5);
+    _impl::CollectionChangeBuilder c;
-            c.move_over(5, 8);
-            REQUIRE_MOVES(c, {8, 5});
-        }
 
-        SECTION("move_over() updates moves to the source") {
-            c.move(3, 8);
-            c.move_over(5, 8);
-            REQUIRE_MOVES(c, {3, 5});
-        }
-
-        SECTION("move_over() is not shifted by previous calls to move_over()") {
-            c.move_over(5, 10);
-            c.move_over(6, 9);
-            REQUIRE_INDICES(c.deletions, 5, 6, 9, 10);
-            REQUIRE_INDICES(c.insertions, 5, 6);
-            REQUIRE_MOVES(c, {10, 5}, {9, 6});
-        }
-    }
-
-    SECTION("calculate") {
     auto all_modified = [](size_t) { return true; };
     auto none_modified = [](size_t) { return false; };
+    const auto npos = size_t(-1);
 
-        SECTION("no changes") {
+    SECTION("returns an empty set when input and output are identical") {
         c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 2, 3}, none_modified, false);
         REQUIRE(c.empty());
     }
 
-        SECTION("inserting from empty") {
+    SECTION("marks all as inserted when prev is empty") {
         c = _impl::CollectionChangeBuilder::calculate({}, {1, 2, 3}, all_modified, false);
         REQUIRE_INDICES(c.insertions, 0, 1, 2);
     }
 
-        SECTION("deleting all existing") {
+    SECTION("marks all as deleted when new is empty") {
         c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {}, all_modified, false);
         REQUIRE_INDICES(c.deletions, 0, 1, 2);
     }
 
-        SECTION("all rows modified without changing order") {
+    SECTION("marks npos rows in prev as deleted") {
+        c = _impl::CollectionChangeBuilder::calculate({npos, 1, 2, 3, npos}, {1, 2, 3}, all_modified, false);
+        REQUIRE_INDICES(c.deletions, 0, 4);
+    }
+
+    SECTION("marks modified rows which do not move as modified") {
         c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 2, 3}, all_modified, false);
         REQUIRE_INDICES(c.modifications, 0, 1, 2);
     }
 
-        SECTION("single insertion in middle") {
+    SECTION("does not mark unmodified rows as modified") {
+        c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 2, 3}, none_modified, false);
+        REQUIRE(c.modifications.empty());
+    }
+
+    SECTION("marks newly added rows as insertions") {
+        c = _impl::CollectionChangeBuilder::calculate({2, 3}, {1, 2, 3}, all_modified, false);
+        REQUIRE_INDICES(c.insertions, 0);
+
         c = _impl::CollectionChangeBuilder::calculate({1, 3}, {1, 2, 3}, all_modified, false);
         REQUIRE_INDICES(c.insertions, 1);
+
+        c = _impl::CollectionChangeBuilder::calculate({1, 2}, {1, 2, 3}, all_modified, false);
+        REQUIRE_INDICES(c.insertions, 2);
     }
 
-        SECTION("single deletion in middle") {
+    SECTION("marks removed rows as deleted") {
+        c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 2}, all_modified, false);
+        REQUIRE_INDICES(c.deletions, 2);
+
         c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 3}, all_modified, false);
         REQUIRE_INDICES(c.deletions, 1);
+
+        c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {2, 3}, all_modified, false);
+        REQUIRE_INDICES(c.deletions, 0);
     }
 
-        SECTION("mixed insert and delete") {
+    SECTION("marks rows as both inserted and deleted") {
-            c = _impl::CollectionChangeBuilder::calculate({3, 5}, {0, 3}, all_modified, false);
+        c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 3, 4}, all_modified, false);
         REQUIRE_INDICES(c.deletions, 1);
-            REQUIRE_INDICES(c.insertions, 0);
+        REQUIRE_INDICES(c.insertions, 2);
         REQUIRE(c.moves.empty());
     }
 
-        SECTION("modifications are reported for moved rows") {
+    SECTION("marks rows as modified even if they moved") {
-            c = _impl::CollectionChangeBuilder::calculate({3, 5}, {5, 3}, all_modified, false);
+        c = _impl::CollectionChangeBuilder::calculate({5, 3}, {3, 5}, all_modified, false);
         REQUIRE_MOVES(c, {1, 0});
         REQUIRE_INDICES(c.modifications, 0, 1);
     }
 
-        SECTION("unsorted reordering") {
+    SECTION("does not mark rows as modified if they are new") {
+        c = _impl::CollectionChangeBuilder::calculate({3}, {3, 5}, all_modified, false);
+        REQUIRE_INDICES(c.modifications, 0);
+    }
+
+    SECTION("reports moves which can be produced by move_last_over()") {
         auto calc = [&](std::vector<size_t> values) {
-                return _impl::CollectionChangeBuilder::calculate({1, 2, 3}, values, none_modified, false);
+            return _impl::CollectionChangeBuilder::calculate(values, {1, 2, 3}, none_modified, false);
         };
 
-            // The commented-out permutations are not possible with
-            // move_last_over() and so are unhandled by unsorted mode
         REQUIRE(calc({1, 2, 3}).empty());
         REQUIRE_MOVES(calc({1, 3, 2}), {2, 1});
-//            REQUIRE_MOVES(calc({2, 1, 3}), {1, 0});
+        REQUIRE_MOVES(calc({2, 1, 3}), {1, 0});
-//            REQUIRE_MOVES(calc({2, 3, 1}), {1, 0}, {2, 1});
+        REQUIRE_MOVES(calc({2, 3, 1}), {2, 0});
-            REQUIRE_MOVES(calc({3, 1, 2}), {2, 0});
+        REQUIRE_MOVES(calc({3, 1, 2}), {1, 0}, {2, 1});
         REQUIRE_MOVES(calc({3, 2, 1}), {2, 0}, {1, 1});
     }
+}
 
-        SECTION("sorted reordering") {
+TEST_CASE("[collection_change] calculate() sorted") {
-            auto calc = [&](std::vector<size_t> values) {
+    _impl::CollectionChangeBuilder c;
-                return _impl::CollectionChangeBuilder::calculate({1, 2, 3}, values, all_modified, true);
+
+    auto all_modified = [](size_t) { return true; };
+    auto none_modified = [](size_t) { return false; };
+    const auto npos = size_t(-1);
+
+    SECTION("returns an empty set when input and output are identical") {
+        c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 2, 3}, none_modified, true);
+        REQUIRE(c.empty());
+    }
+
+    SECTION("marks all as inserted when prev is empty") {
+        c = _impl::CollectionChangeBuilder::calculate({}, {1, 2, 3}, all_modified, true);
+        REQUIRE_INDICES(c.insertions, 0, 1, 2);
+    }
+
+    SECTION("marks all as deleted when new is empty") {
+        c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {}, all_modified, true);
+        REQUIRE_INDICES(c.deletions, 0, 1, 2);
+    }
+
+    SECTION("marks npos rows in prev as deleted") {
+        c = _impl::CollectionChangeBuilder::calculate({npos, 1, 2, 3, npos}, {1, 2, 3}, all_modified, true);
+        REQUIRE_INDICES(c.deletions, 0, 4);
+    }
+
+    SECTION("marks modified rows which do not move as modified") {
+        c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 2, 3}, all_modified, true);
+        REQUIRE_INDICES(c.modifications, 0, 1, 2);
+    }
+
+    SECTION("does not mark unmodified rows as modified") {
+        c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 2, 3}, none_modified, true);
+        REQUIRE(c.modifications.empty());
+    }
+
+    SECTION("marks newly added rows as insertions") {
+        c = _impl::CollectionChangeBuilder::calculate({2, 3}, {1, 2, 3}, all_modified, true);
+        REQUIRE_INDICES(c.insertions, 0);
+
+        c = _impl::CollectionChangeBuilder::calculate({1, 3}, {1, 2, 3}, all_modified, true);
+        REQUIRE_INDICES(c.insertions, 1);
+
+        c = _impl::CollectionChangeBuilder::calculate({1, 2}, {1, 2, 3}, all_modified, true);
+        REQUIRE_INDICES(c.insertions, 2);
+    }
+
+    SECTION("marks removed rows as deleted") {
+        c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 2}, all_modified, true);
+        REQUIRE_INDICES(c.deletions, 2);
+
+        c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 3}, all_modified, true);
+        REQUIRE_INDICES(c.deletions, 1);
+
+        c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {2, 3}, all_modified, true);
+        REQUIRE_INDICES(c.deletions, 0);
+    }
+
+    SECTION("marks rows as both inserted and deleted") {
+        c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 3, 4}, all_modified, true);
+        REQUIRE_INDICES(c.deletions, 1);
+        REQUIRE_INDICES(c.insertions, 2);
+        REQUIRE(c.moves.empty());
+    }
+
+    SECTION("marks rows as modified even if they moved") {
+        c = _impl::CollectionChangeBuilder::calculate({3, 5}, {5, 3}, all_modified, true);
+        REQUIRE_INDICES(c.deletions, 1);
+        REQUIRE_INDICES(c.insertions, 0);
+        REQUIRE_INDICES(c.modifications, 0, 1);
+    }
+
+    SECTION("does not mark rows as modified if they are new") {
+        c = _impl::CollectionChangeBuilder::calculate({3}, {3, 5}, all_modified, true);
+        REQUIRE_INDICES(c.modifications, 0);
+    }
+
+    SECTION("reports inserts/deletes for simple reorderings") {
+        auto calc = [&](std::vector<size_t> old_rows, std::vector<size_t> new_rows) {
+            return _impl::CollectionChangeBuilder::calculate(old_rows, new_rows, none_modified, true);
         };
 
-            REQUIRE(calc({1, 2, 3}).moves.empty());
+        c = calc({1, 2, 3}, {1, 2, 3});
-            return;
+        REQUIRE(c.insertions.empty());
-            // none of these actually work since it just does insert+delete
+        REQUIRE(c.deletions.empty());
-            REQUIRE_MOVES(calc({1, 3, 2}), {2, 1});
+
-            REQUIRE_MOVES(calc({2, 1, 3}), {1, 0});
+        c = calc({1, 2, 3}, {1, 3, 2});
-            REQUIRE_MOVES(calc({2, 3, 1}), {1, 0}, {2, 1});
+        REQUIRE_INDICES(c.insertions, 1);
-            REQUIRE_MOVES(calc({3, 1, 2}), {2, 0});
+        REQUIRE_INDICES(c.deletions, 2);
-            REQUIRE_MOVES(calc({3, 2, 1}), {2, 0}, {1, 1});
+
+        c = calc({1, 2, 3}, {2, 1, 3});
+        REQUIRE_INDICES(c.insertions, 0);
+        REQUIRE_INDICES(c.deletions, 1);
+
+        c = calc({1, 2, 3}, {2, 3, 1});
+        REQUIRE_INDICES(c.insertions, 2);
+        REQUIRE_INDICES(c.deletions, 0);
+
+        c = calc({1, 2, 3}, {3, 1, 2});
+        REQUIRE_INDICES(c.insertions, 0);
+        REQUIRE_INDICES(c.deletions, 2);
+
+        c = calc({1, 2, 3}, {3, 2, 1});
+        REQUIRE_INDICES(c.insertions, 0, 1);
+        REQUIRE_INDICES(c.deletions, 1, 2);
+
+        c = calc({1, 3, 2}, {1, 2, 3});
+        REQUIRE_INDICES(c.insertions, 1);
+        REQUIRE_INDICES(c.deletions, 2);
+
+        c = calc({1, 3, 2}, {1, 3, 2});
+        REQUIRE(c.insertions.empty());
+        REQUIRE(c.deletions.empty());
+
+        c = calc({1, 3, 2}, {2, 1, 3});
+        REQUIRE_INDICES(c.insertions, 0);
+        REQUIRE_INDICES(c.deletions, 2);
+
+        c = calc({1, 3, 2}, {2, 3, 1});
+        REQUIRE_INDICES(c.insertions, 0, 1);
+        REQUIRE_INDICES(c.deletions, 1, 2);
+
+        c = calc({1, 3, 2}, {3, 1, 2});
+        REQUIRE_INDICES(c.insertions, 0);
+        REQUIRE_INDICES(c.deletions, 1);
+
+        c = calc({1, 3, 2}, {3, 2, 1});
+        REQUIRE_INDICES(c.insertions, 2);
+        REQUIRE_INDICES(c.deletions, 0);
+
+        c = calc({2, 1, 3}, {1, 2, 3});
+        REQUIRE_INDICES(c.insertions, 0);
+        REQUIRE_INDICES(c.deletions, 1);
+
+        c = calc({2, 1, 3}, {1, 3, 2});
+        REQUIRE_INDICES(c.insertions, 2);
+        REQUIRE_INDICES(c.deletions, 0);
+
+        c = calc({2, 1, 3}, {2, 1, 3});
+        REQUIRE(c.insertions.empty());
+        REQUIRE(c.deletions.empty());
+
+        c = calc({2, 1, 3}, {2, 3, 1});
+        REQUIRE_INDICES(c.insertions, 1);
+        REQUIRE_INDICES(c.deletions, 2);
+
+        c = calc({2, 1, 3}, {3, 1, 2});
+        REQUIRE_INDICES(c.insertions, 0, 1);
+        REQUIRE_INDICES(c.deletions, 1, 2);
+
+        c = calc({2, 1, 3}, {3, 2, 1});
+        REQUIRE_INDICES(c.insertions, 0);
+        REQUIRE_INDICES(c.deletions, 2);
+
+        c = calc({2, 3, 1}, {1, 2, 3});
+        REQUIRE_INDICES(c.insertions, 0);
+        REQUIRE_INDICES(c.deletions, 2);
+
+        c = calc({2, 3, 1}, {1, 3, 2});
+        REQUIRE_INDICES(c.insertions, 0, 1);
+        REQUIRE_INDICES(c.deletions, 1, 2);
+
+        c = calc({2, 3, 1}, {2, 1, 3});
+        REQUIRE_INDICES(c.insertions, 1);
+        REQUIRE_INDICES(c.deletions, 2);
+
+        c = calc({2, 3, 1}, {2, 3, 1});
+        REQUIRE(c.insertions.empty());
+        REQUIRE(c.deletions.empty());
+
+        c = calc({2, 3, 1}, {3, 1, 2});
+        REQUIRE_INDICES(c.insertions, 2);
+        REQUIRE_INDICES(c.deletions, 0);
+
+        c = calc({2, 3, 1}, {3, 2, 1});
+        REQUIRE_INDICES(c.insertions, 0);
+        REQUIRE_INDICES(c.deletions, 1);
+
+        c = calc({3, 1, 2}, {1, 2, 3});
+        REQUIRE_INDICES(c.insertions, 2);
+        REQUIRE_INDICES(c.deletions, 0);
+
+        c = calc({3, 1, 2}, {1, 3, 2});
+        REQUIRE_INDICES(c.insertions, 0);
+        REQUIRE_INDICES(c.deletions, 1);
+
+        c = calc({3, 1, 2}, {2, 1, 3});
+        REQUIRE_INDICES(c.insertions, 0, 1);
+        REQUIRE_INDICES(c.deletions, 1, 2);
+
+        c = calc({3, 1, 2}, {2, 3, 1});
+        REQUIRE_INDICES(c.insertions, 0);
+        REQUIRE_INDICES(c.deletions, 2);
+
+        c = calc({3, 1, 2}, {3, 1, 2});
+        REQUIRE(c.insertions.empty());
+        REQUIRE(c.deletions.empty());
+
+        c = calc({3, 1, 2}, {3, 2, 1});
+        REQUIRE_INDICES(c.insertions, 1);
+        REQUIRE_INDICES(c.deletions, 2);
+
+        c = calc({3, 2, 1}, {1, 2, 3});
+        REQUIRE_INDICES(c.insertions, 0, 1);
+        REQUIRE_INDICES(c.deletions, 1, 2);
+
+        c = calc({3, 2, 1}, {1, 3, 2});
+        REQUIRE_INDICES(c.insertions, 0);
+        REQUIRE_INDICES(c.deletions, 2);
+
+        c = calc({3, 2, 1}, {2, 1, 3});
+        REQUIRE_INDICES(c.insertions, 2);
+        REQUIRE_INDICES(c.deletions, 0);
+
+        c = calc({3, 2, 1}, {2, 3, 1});
+        REQUIRE_INDICES(c.insertions, 0);
+        REQUIRE_INDICES(c.deletions, 1);
+
+        c = calc({3, 2, 1}, {3, 1, 2});
+        REQUIRE_INDICES(c.insertions, 1);
+        REQUIRE_INDICES(c.deletions, 2);
+
+        c = calc({3, 2, 1}, {3, 2, 1});
+        REQUIRE(c.insertions.empty());
+        REQUIRE(c.deletions.empty());
     }
 
-        SECTION("merge can collapse insert -> move -> delete to no-op") {
+    SECTION("prefers to produce diffs where modified rows are the ones to move when it is ambiguous") {
+        auto two_modified = [](size_t ndx) { return ndx == 2; };
+        c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 3, 2}, two_modified, true);
+        REQUIRE_INDICES(c.deletions, 1);
+        REQUIRE_INDICES(c.insertions, 2);
+
+        auto three_modified = [](size_t ndx) { return ndx == 3; };
+        c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 3, 2}, three_modified, true);
+        REQUIRE_INDICES(c.deletions, 2);
+        REQUIRE_INDICES(c.insertions, 1);
+    }
+
+    SECTION("prefers smaller diffs over larger diffs moving only modified rows") {
+        auto two_modified = [](size_t ndx) { return ndx == 2; };
+        c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {2, 3, 1}, two_modified, true);
+        REQUIRE_INDICES(c.deletions, 0);
+        REQUIRE_INDICES(c.insertions, 2);
+    }
+
+    SECTION("supports duplicate indices") {
+        c = _impl::CollectionChangeBuilder::calculate({1, 1, 2, 2, 3, 3},
+                                                      {1, 2, 3, 1, 2, 3},
+                                                      all_modified, true);
+        REQUIRE_INDICES(c.deletions, 3, 5);
+        REQUIRE_INDICES(c.insertions, 1, 2);
+    }
+
+    SECTION("deletes and inserts the last option when any in a range could be deleted") {
+        c = _impl::CollectionChangeBuilder::calculate({3, 2, 1, 1, 2, 3},
+                                                      {1, 1, 2, 2, 3, 3},
+                                                      all_modified, true);
+        REQUIRE_INDICES(c.deletions, 0, 1);
+        REQUIRE_INDICES(c.insertions, 3, 5);
+    }
+
+    SECTION("reports insertions/deletions when the number of duplicate entries changes") {
+        c = _impl::CollectionChangeBuilder::calculate({1, 1, 1, 1, 2, 3},
+                                                      {1, 2, 3, 1},
+                                                      all_modified, true);
+        REQUIRE_INDICES(c.deletions, 1, 2, 3);
+        REQUIRE_INDICES(c.insertions, 3);
+
+        c = _impl::CollectionChangeBuilder::calculate({1, 2, 3, 1},
+                                                      {1, 1, 1, 1, 2, 3},
+                                                      all_modified, true);
+        REQUIRE_INDICES(c.deletions, 3);
+        REQUIRE_INDICES(c.insertions, 1, 2, 3);
+    }
+
+    SECTION("properly recurses into smaller subblocks") {
+        std::vector<size_t> prev = {10, 1, 2, 11, 3, 4, 5, 12, 6, 7, 13};
+        std::vector<size_t> next = {13, 1, 2, 12, 3, 4, 5, 11, 6, 7, 10};
+        c = _impl::CollectionChangeBuilder::calculate(prev, next, all_modified, true);
+        REQUIRE_INDICES(c.deletions, 0, 3, 7, 10);
+        REQUIRE_INDICES(c.insertions, 0, 3, 7, 10);
+    }
+
+    SECTION("produces diffs which let merge collapse insert -> move -> delete to no-op") {
         auto four_modified = [](size_t ndx) { return ndx == 4; };
         for (int insert_pos = 0; insert_pos < 4; ++insert_pos) {
             for (int move_to_pos = 0; move_to_pos < 4; ++move_to_pos) {
@@ -278,10 +664,29 @@ TEST_CASE("collection change indices") {
             }
         }
     }
+}
+
+TEST_CASE("[collection_change] merge()") {
+    _impl::CollectionChangeBuilder c;
+
+    SECTION("is a no-op if the new set is empty") {
+        c = {{1, 2, 3}, {4, 5}, {6, 7}, {{8, 9}}};
+        c.merge({});
+        REQUIRE_INDICES(c.deletions, 1, 2, 3, 8);
+        REQUIRE_INDICES(c.insertions, 4, 5, 9);
+        REQUIRE_INDICES(c.modifications, 6, 7);
+        REQUIRE_MOVES(c, {8, 9});
     }
 
-    SECTION("merge") {
+    SECTION("replaces the set with the new set if the old set is empty") {
-        SECTION("deletions are shifted by previous deletions") {
+        c.merge({{1, 2, 3}, {4, 5}, {6, 7}, {{8, 9}}});
+        REQUIRE_INDICES(c.deletions, 1, 2, 3, 8);
+        REQUIRE_INDICES(c.insertions, 4, 5, 9);
+        REQUIRE_INDICES(c.modifications, 6, 7);
+        REQUIRE_MOVES(c, {8, 9});
+    }
+
+    SECTION("shifts deletions by previous deletions") {
         c = {{5}, {}, {}, {}};
         c.merge({{3}, {}, {}, {}});
         REQUIRE_INDICES(c.deletions, 3, 5);
@@ -299,7 +704,7 @@ TEST_CASE("collection change indices") {
         REQUIRE_INDICES(c.deletions, 5, 7);
     }
 
-        SECTION("deletions are shifted by previous insertions") {
+    SECTION("shifts deletions by previous insertions") {
         c = {{}, {5}, {}, {}};
         c.merge({{4}, {}, {}, {}});
         REQUIRE_INDICES(c.deletions, 4);
@@ -309,95 +714,95 @@ TEST_CASE("collection change indices") {
         REQUIRE_INDICES(c.deletions, 5);
     }
 
-        SECTION("deletions shift previous insertions") {
+    SECTION("shifts previous insertions by deletions") {
         c = {{}, {2, 3}, {}, {}};
         c.merge({{1}, {}, {}, {}});
         REQUIRE_INDICES(c.insertions, 1, 2);
     }
 
-        SECTION("deletions remove previous insertions") {
+    SECTION("removes previous insertions for newly deleted rows") {
         c = {{}, {1, 2}, {}, {}};
         c.merge({{2}, {}, {}, {}});
         REQUIRE_INDICES(c.insertions, 1);
     }
 
-        SECTION("deletions remove previous modifications") {
+    SECTION("removes previous modifications for newly deleted rows") {
         c = {{}, {}, {2, 3}, {}};
         c.merge({{2}, {}, {}, {}});
         REQUIRE_INDICES(c.modifications, 2);
     }
 
-        SECTION("deletions shift previous modifications") {
+    SECTION("shifts previous modifications for deletions of other rows") {
         c = {{}, {}, {2, 3}, {}};
         c.merge({{1}, {}, {}, {}});
         REQUIRE_INDICES(c.modifications, 1, 2);
     }
 
-        SECTION("deletions remove previous moves to deleted row") {
+    SECTION("removes moves to rows which have been deleted") {
         c = {{}, {}, {}, {{2, 3}}};
         c.merge({{3}, {}, {}, {}});
         REQUIRE(c.moves.empty());
     }
 
-        SECTION("deletions shift destination of previous moves to after the deleted row") {
+    SECTION("shifts destinations of previous moves to reflect new deletions") {
         c = {{}, {}, {}, {{2, 5}}};
         c.merge({{3}, {}, {}, {}});
         REQUIRE_MOVES(c, {2, 4});
     }
 
-        SECTION("insertions do not interact with previous deletions") {
+    SECTION("does not modify old deletions based on new insertions") {
         c = {{1, 3}, {}, {}, {}};
         c.merge({{}, {1, 2, 3}, {}, {}});
         REQUIRE_INDICES(c.deletions, 1, 3);
         REQUIRE_INDICES(c.insertions, 1, 2, 3);
     }
 
-        SECTION("insertions shift previous insertions") {
+    SECTION("shifts previous insertions to reflect new insertions") {
         c = {{}, {1, 5}, {}, {}};
         c.merge({{}, {1, 4}, {}, {}});
         REQUIRE_INDICES(c.insertions, 1, 2, 4, 7);
     }
 
-        SECTION("insertions shift previous modifications") {
+    SECTION("shifts previous modifications to reflect new insertions") {
         c = {{}, {}, {1, 5}, {}};
         c.merge({{}, {1, 4}, {}, {}});
         REQUIRE_INDICES(c.modifications, 2, 7);
         REQUIRE_INDICES(c.insertions, 1, 4);
     }
 
-        SECTION("insertions shift destination of previous moves") {
+    SECTION("shifts previous move destinations to reflect new insertions") {
         c = {{}, {}, {}, {{2, 5}}};
         c.merge({{}, {3}, {}});
         REQUIRE_MOVES(c, {2, 6});
     }
 
-        SECTION("modifications do not interact with previous deletions") {
+    SECTION("does not modify old deletions based on new modifications") {
         c = {{1, 2, 3}, {}, {}, {}};
         c.merge({{}, {}, {2}});
         REQUIRE_INDICES(c.deletions, 1, 2, 3);
         REQUIRE_INDICES(c.modifications, 2);
     }
 
-        SECTION("modifications are not discarded for previous insertions") {
+    SECTION("tracks modifications made to previously inserted rows") {
         c = {{}, {2}, {}, {}};
         c.merge({{}, {}, {1, 2, 3}});
         REQUIRE_INDICES(c.insertions, 2);
         REQUIRE_INDICES(c.modifications, 1, 2, 3);
     }
 
-        SECTION("modifications are merged with previous modifications") {
+    SECTION("unions modifications with old modifications") {
         c = {{}, {}, {2}, {}};
         c.merge({{}, {}, {1, 2, 3}});
         REQUIRE_INDICES(c.modifications, 1, 2, 3);
     }
 
-        SECTION("modifications are tracked for the destination of previous moves") {
+    SECTION("tracks modifications for previous moves") {
         c = {{}, {}, {}, {{1, 2}}};
         c.merge({{}, {}, {2, 3}});
         REQUIRE_INDICES(c.modifications, 2, 3);
     }
 
-        SECTION("move sources are shifted for previous deletes and insertions") {
+    SECTION("updates new move sources to reflect previous inserts and deletes") {
         c = {{1}, {}, {}, {}};
         c.merge({{}, {}, {}, {{2, 3}}});
         REQUIRE_MOVES(c, {3, 3});
@@ -411,27 +816,27 @@ TEST_CASE("collection change indices") {
         REQUIRE_MOVES(c, {5, 10});
     }
 
-        SECTION("moves update previous modifications to source") {
+    SECTION("updates the row modified for rows moved after a modification") {
         c = {{}, {}, {1}, {}};
         c.merge({{}, {}, {}, {{1, 3}}});
         REQUIRE_INDICES(c.modifications, 3);
         REQUIRE_MOVES(c, {1, 3});
     }
 
-        SECTION("moves update insertion position for previous inserts of source") {
+    SECTION("updates the row inserted for moves of previously new rows") {
         c = {{}, {1}, {}, {}};
         c.merge({{}, {}, {}, {{1, 3}}});
         REQUIRE(c.moves.empty());
         REQUIRE_INDICES(c.insertions, 3);
     }
 
-        SECTION("moves update previous moves to the source") {
+    SECTION("updates old moves when the destination is moved again") {
         c = {{}, {}, {}, {{1, 3}}};
         c.merge({{}, {}, {}, {{3, 5}}});
         REQUIRE_MOVES(c, {1, 5});
     }
 
-        SECTION("moves shift destination of previous moves like an insert/delete pair would") {
+    SECTION("shifts destination of previous moves to reflect new moves like an insert/delete pair would") {
         c = {{}, {}, {}, {{1, 3}}};
         c.merge({{}, {}, {}, {{2, 5}}});
         REQUIRE_MOVES(c, {1, 2}, {3, 5});
@@ -505,5 +910,4 @@ TEST_CASE("collection change indices") {
         REQUIRE_INDICES(c.modifications, 0);
         REQUIRE(c.moves.empty());
     }
-    }
 }