Stop apply_diff() from creating non-monotonic changes

Commit ba41928 (Remove spurious newline when | replaces at buffer end,
2024-11-28) also tried to fix a "DiffOp::Add" code path. Unless I'm
misremembering, I had run into a crash and extracted this unit test.

When apply_diff() decides to erase the last line, and then insert something
at the end, buffer changes might look like:

	{.type=Erase,  .begin={2, 0}, .end={3, 0}}
	{.type=Insert, .begin={1, 5}, .end={5, 0}}

The "{1, 5}" is caused by "pos = buffer.prev(pos);" added by the
aforementioned commit.  It's problematic because it breaks the caller's
"ForwardChangesTracker" invariant: the previous Erase leaves the cursor at
{2, 0}, so the insert cannot start before that.

Additionally, the inserted range returned by apply_diff() would be
"{ {2, 0}, {4, 0} }" whose end position is weirdly different from the Insert's
end position.  It questionable that we are passing on this weird state.

The "pos = buffer.prev(pos);" statement was added so we actually delete the
current final newline.  We still leave "tried_to_erase_final_newline" set,
meaning we'll also delete the the final newline after we're done.

But deleting the "current final newline" the way we do it is confusing,
because we do it at every step DiffOp::Add step.  This would blow up if
there are multiple successive contiguous DiffOp::Add events. I doubt this
ever happens but it's still confusing.

Remove this logic, and restore historical behavior in case we append at the
buffer end. This does not break the system test added by ba41928 because
in that case, we
1. first delete the entire buffer, setting pos={0, 0}, but also restoring
   the EOL invariant, so the end position would be {0, 1}.
2. then we insert at pos={0, 0}, and since "buffer.is_end(pos)" is false we
   don't run into this bad code path.

While at it add an assertion to clarify that we already assume that the
diff algorithm never gives us empty DiffOp::Keep, and always monotonically
increasing positions, except for the very special case where we deleted
until buffer end, which should already be handled.

The unit test is not perfect because it depends on the current behavior of
the diff algorithm.

Author: krobelus

src/buffer.hh

@@ -224,6 +224,10 @@ public:
         Type type;
         BufferCoord begin;
         BufferCoord end;
+
+        #ifdef KAK_DEBUG
+        bool operator==(const Change&) const = default;
+        #endif
     };
     ConstArrayView<Change> changes_since(size_t timestamp) const;
 

src/normal.cc

@@ -1,5 +1,7 @@
 #include "normal.hh"
 
+#include "array_view.hh"
+#include "assert.hh"
 #include "buffer.hh"
 #include "buffer_manager.hh"
 #include "buffer_utils.hh"
@@ -20,6 +22,7 @@
 #include "selectors.hh"
 #include "shell_manager.hh"
 #include "string.hh"
+#include "units.hh"
 #include "user_interface.hh"
 #include "unit_tests.hh"
 #include "window.hh"
@@ -555,19 +558,21 @@ BufferRange apply_diff(Buffer& buffer, BufferCoord pos, ArrayView<StringView> li
         switch (op)
         {
         case DiffOp::Keep:
+            kak_assert(not tried_to_erase_final_newline);
             pos = buffer.advance(pos, byte_count(lines_before, posA, len));
             posA += len;
             posB += len;
             break;
         case DiffOp::Add:
-            if (buffer.is_end(pos) and tried_to_erase_final_newline)
-                pos = buffer.prev(pos);
+            if (buffer.is_end(pos))
+                tried_to_erase_final_newline = false;
             pos = buffer.insert(pos, {lines_after[posB].begin(),
                                       lines_after[posB + len - 1].end()}).end;
             posB += len;
             break;
         case DiffOp::Remove:
         {
+            kak_assert(not tried_to_erase_final_newline);
             BufferCoord end = buffer.advance(pos, byte_count(lines_before, posA, len));
             tried_to_erase_final_newline |= buffer.is_end(end);
             pos = buffer.erase(pos, end);
@@ -584,6 +589,70 @@ BufferRange apply_diff(Buffer& buffer, BufferCoord pos, ArrayView<StringView> li
     return {first, pos};
 }
 
+UnitTest test_apply_diff{[] {
+    using Change = Buffer::Change;
+    auto validate = [&](LineCount line,
+                        StringView new_text,
+                        BufferRange expected_new_range,
+                        ConstArrayView<Change> expected_buffer_changes)
+    {
+        Buffer buffer{"", Buffer::Flags::None, {
+            StringData::create("line1\n"),
+            StringData::create("line2\n"),
+            StringData::create("line3\n"),
+        }};
+        const size_t timestamp = buffer.timestamp();
+        Vector<StringView> old_text;
+        for (auto i = line; i < buffer.line_count(); i++)
+            old_text.push_back(buffer[i]);
+        BufferRange new_text_range = apply_diff(buffer, BufferCoord{line, 0}, old_text, new_text);
+        kak_assert(new_text_range == expected_new_range);
+        kak_assert(buffer.changes_since(timestamp) == expected_buffer_changes);
+    };
+    // When appending at end, we add any missing newline
+    validate(
+        /*line=*/3,
+        "added-line3-missing-eol",
+        BufferRange{{3, 0}, {4, 0}},
+        {
+            Change{Change::Insert, {3, 0}, {4, 0}},
+        }
+    );
+    // Special case: erasing until buffer end also erases the final newline.
+    validate(
+        /*line=*/2,
+        "",
+        BufferRange{{1, 5}, {1, 5}},
+        {
+            Change{Change::Erase, {2, 0}, {3, 0}},
+        }
+    );
+    // Erasing and appending at end still produces forward-only changes.
+    validate(
+        /*line=*/2,
+        "changed-line3\n"
+        "added-line4\n"
+        "added-line5\n",
+        BufferRange{{2, 0}, {5, 0}},
+        {
+            Change{Change::Erase,  {2, 0}, {3, 0}},
+            Change{Change::Insert, {2, 0}, {5, 0}},
+        }
+    );
+    // Same result when append is missing newline.
+    validate(
+        /*line=*/2,
+        "changed-line3\n"
+        "added-line4\n"
+        "added-line5-missing-eol",
+        BufferRange{{2, 0}, {5, 0}},
+        {
+            Change{Change::Erase,  {2, 0}, {3, 0}},
+            Change{Change::Insert, {2, 0}, {5, 0}},
+        }
+    );
+}};
+
 template<bool replace>
 void pipe(Context& context, NormalParams params)
 {