From bcdb475b71b0fbabce57344ac8d2575c23b1bbe0 Mon Sep 17 00:00:00 2001
From: Michael Davis <mcarsondavis@gmail.com>
Date: Fri, 2 Dec 2022 21:09:08 -0600
Subject: [PATCH] Fix transaction composition order in History::changes_since
 (#4981)

* Add a undo/redo split test case for crossing branches

* history: Switch up/down transaction chaining order

The old code tends to work in practice because, usually, either up_txns
or down_txns are empty. When both have contents though, we can run into
a panic trying to compose them all since they will disagree on the
length of the text. This fixes the panic test case in the parent
commit.
---
 helix-core/src/history.rs       |  2 +-
 helix-term/tests/test/splits.rs | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/helix-core/src/history.rs b/helix-core/src/history.rs
index b99e969f..1aac38d9 100644
--- a/helix-core/src/history.rs
+++ b/helix-core/src/history.rs
@@ -131,7 +131,7 @@ pub fn changes_since(&self, revision: usize) -> Option<Transaction> {
             .map(|&n| self.revisions[n].inversion.clone());
         let down_txns = down.iter().map(|&n| self.revisions[n].transaction.clone());
 
-        up_txns.chain(down_txns).reduce(|acc, tx| tx.compose(acc))
+        down_txns.chain(up_txns).reduce(|acc, tx| tx.compose(acc))
     }
 
     /// Undo the last edit.
diff --git a/helix-term/tests/test/splits.rs b/helix-term/tests/test/splits.rs
index a34a24b7..96ced21a 100644
--- a/helix-term/tests/test/splits.rs
+++ b/helix-term/tests/test/splits.rs
@@ -161,5 +161,30 @@ async fn test_changes_in_splits_apply_to_all_views() -> anyhow::Result<()> {
     ))
     .await?;
 
+    // See <https://github.com/helix-editor/helix/issues/4957>.
+    // This sequence undoes part of the history and then adds new changes, creating a
+    // new branch in the history tree. `View::sync_changes` applies transactions down
+    // and up to the lowest common ancestor in the path between old and new revision
+    // numbers. If we apply these up/down transactions in the wrong order, this case
+    // panics.
+    // The key sequence:
+    // * 3[<space>    Create three empty lines so we are at the end of the document.
+    // * <C-w>v<C-s>  Create a split and save that point at the end of the document
+    //                in the jumplist.
+    // * <C-w>w       Switch back to the first window.
+    // * uu           Undo twice (not three times which would bring us back to the
+    //                root of the tree).
+    // * 3[<space>    Create three empty lines. Now the end of the document is past
+    //                where it was on step 1.
+    // * <C-w>q       Close window 1, focusing window 2 and causing a sync. This step
+    //                panics if we don't apply in the right order.
+    // * %d           Clean up the buffer.
+    test((
+        "#[|]#",
+        "3[<space><C-w>v<C-s><C-w>wuu3[<space><C-w>q%d",
+        "#[|]#",
+    ))
+    .await?;
+
     Ok(())
 }