Fix bugs with applying hunks from branch buffers (#18721)

Release Notes:

- N/A

---------

Co-authored-by: Marshall <marshall@zed.dev>

crates/clock/src/clock.rs

@@ -216,10 +216,11 @@ impl fmt::Debug for Global {
             if timestamp.replica_id > 0 {
                 write!(f, ", ")?;
             }
-            write!(f, "{}: {}", timestamp.replica_id, timestamp.value)?;
-        }
-        if self.local_branch_value > 0 {
-            write!(f, "<branch>: {}", self.local_branch_value)?;
+            if timestamp.replica_id == LOCAL_BRANCH_REPLICA_ID {
+                write!(f, "<branch>: {}", timestamp.value)?;
+            } else {
+                write!(f, "{}: {}", timestamp.replica_id, timestamp.value)?;
+            }
         }
         write!(f, "}}")
     }

crates/language/src/buffer.rs

@@ -18,6 +18,7 @@ use crate::{
 };
 use anyhow::{anyhow, Context, Result};
 use async_watch as watch;
+use clock::Lamport;
 pub use clock::ReplicaId;
 use futures::channel::oneshot;
 use gpui::{
@@ -90,7 +91,7 @@ enum BufferDiffBase {
     PastBufferVersion {
         buffer: Model<Buffer>,
         rope: Rope,
-        operations_to_ignore: Vec<clock::Lamport>,
+        merged_operations: Vec<Lamport>,
     },
 }
 
@@ -802,7 +803,7 @@ impl Buffer {
                 diff_base: Some(BufferDiffBase::PastBufferVersion {
                     buffer: this.clone(),
                     rope: self.as_rope().clone(),
-                    operations_to_ignore: Vec::new(),
+                    merged_operations: Default::default(),
                 }),
                 language: self.language.clone(),
                 has_conflict: self.has_conflict,
@@ -834,34 +835,32 @@ impl Buffer {
             return;
         };
 
-        base_buffer.update(cx, |base_buffer, cx| {
-            let edits = self
-                .edits_since::<usize>(&base_buffer.version)
-                .filter_map(|edit| {
-                    if range
-                        .as_ref()
-                        .map_or(true, |range| range.overlaps(&edit.new))
-                    {
-                        Some((edit.old, self.text_for_range(edit.new).collect::<String>()))
-                    } else {
-                        None
-                    }
-                })
-                .collect::<Vec<_>>();
+        let mut edits = Vec::new();
+        for edit in self.edits_since::<usize>(&base_buffer.read(cx).version()) {
+            if let Some(range) = &range {
+                if range.start > edit.new.end || edit.new.start > range.end {
+                    continue;
+                }
+            }
+            edits.push((
+                edit.old.clone(),
+                self.text_for_range(edit.new.clone()).collect::<String>(),
+            ));
+        }
 
-            let operation = base_buffer.edit(edits, None, cx);
+        let operation = base_buffer.update(cx, |base_buffer, cx| {
+            cx.emit(BufferEvent::DiffBaseChanged);
+            base_buffer.edit(edits, None, cx)
+        });
 
-            // Prevent this operation from being reapplied to the branch.
+        if let Some(operation) = operation {
             if let Some(BufferDiffBase::PastBufferVersion {
-                operations_to_ignore,
-                ..
+                merged_operations, ..
             }) = &mut self.diff_base
             {
-                operations_to_ignore.extend(operation);
+                merged_operations.push(operation);
             }
-
-            cx.emit(BufferEvent::DiffBaseChanged);
-        });
+        }
     }
 
     fn on_base_buffer_event(
@@ -870,31 +869,34 @@ impl Buffer {
         event: &BufferEvent,
         cx: &mut ModelContext<Self>,
     ) {
-        if let BufferEvent::Operation { operation, .. } = event {
-            if let Some(BufferDiffBase::PastBufferVersion {
-                operations_to_ignore,
-                ..
-            }) = &mut self.diff_base
-            {
-                let mut is_ignored = false;
-                if let Operation::Buffer(text::Operation::Edit(buffer_operation)) = &operation {
-                    operations_to_ignore.retain(|operation_to_ignore| {
-                        match buffer_operation.timestamp.cmp(&operation_to_ignore) {
-                            Ordering::Less => true,
-                            Ordering::Equal => {
-                                is_ignored = true;
-                                false
-                            }
-                            Ordering::Greater => false,
-                        }
-                    });
-                }
-                if !is_ignored {
-                    self.apply_ops([operation.clone()], cx);
-                    self.diff_base_version += 1;
-                }
+        let BufferEvent::Operation { operation, .. } = event else {
+            return;
+        };
+        let Some(BufferDiffBase::PastBufferVersion {
+            merged_operations, ..
+        }) = &mut self.diff_base
+        else {
+            return;
+        };
+
+        let mut operation_to_undo = None;
+        if let Operation::Buffer(text::Operation::Edit(operation)) = &operation {
+            if let Ok(ix) = merged_operations.binary_search(&operation.timestamp) {
+                merged_operations.remove(ix);
+                operation_to_undo = Some(operation.timestamp);
             }
         }
+
+        self.apply_ops([operation.clone()], cx);
+
+        if let Some(timestamp) = operation_to_undo {
+            let operation = self
+                .text
+                .undo_operations([(timestamp, u32::MAX)].into_iter().collect());
+            self.send_operation(Operation::Buffer(operation), true, cx);
+        }
+
+        self.diff_base_version += 1;
     }
 
     #[cfg(test)]

crates/language/src/buffer_tests.rs

@@ -2485,15 +2485,73 @@ fn test_branch_and_merge(cx: &mut TestAppContext) {
 }
 
 #[gpui::test]
-fn test_merge_into_base(cx: &mut AppContext) {
-    init_settings(cx, |_| {});
+fn test_merge_into_base(cx: &mut TestAppContext) {
+    cx.update(|cx| init_settings(cx, |_| {}));
+
     let base = cx.new_model(|cx| Buffer::local("abcdefghijk", cx));
     let branch = base.update(cx, |buffer, cx| buffer.branch(cx));
+
+    // Make 3 edits, merge one into the base.
     branch.update(cx, |branch, cx| {
-        branch.edit([(0..3, "ABC"), (7..9, "HI")], None, cx);
+        branch.edit([(0..3, "ABC"), (7..9, "HI"), (11..11, "LMN")], None, cx);
         branch.merge_into_base(Some(5..8), cx);
     });
-    assert_eq!(base.read(cx).text(), "abcdefgHIjk");
+
+    branch.read_with(cx, |branch, _| assert_eq!(branch.text(), "ABCdefgHIjkLMN"));
+    base.read_with(cx, |base, _| assert_eq!(base.text(), "abcdefgHIjk"));
+
+    // Undo the one already-merged edit. Merge that into the base.
+    branch.update(cx, |branch, cx| {
+        branch.edit([(7..9, "hi")], None, cx);
+        branch.merge_into_base(Some(5..8), cx);
+    });
+    base.read_with(cx, |base, _| assert_eq!(base.text(), "abcdefghijk"));
+
+    // Merge an insertion into the base.
+    branch.update(cx, |branch, cx| {
+        branch.merge_into_base(Some(11..11), cx);
+    });
+
+    branch.read_with(cx, |branch, _| assert_eq!(branch.text(), "ABCdefghijkLMN"));
+    base.read_with(cx, |base, _| assert_eq!(base.text(), "abcdefghijkLMN"));
+
+    // Deleted the inserted text and merge that into the base.
+    branch.update(cx, |branch, cx| {
+        branch.edit([(11..14, "")], None, cx);
+        branch.merge_into_base(Some(10..11), cx);
+    });
+
+    base.read_with(cx, |base, _| assert_eq!(base.text(), "abcdefghijk"));
+}
+
+#[gpui::test]
+fn test_undo_after_merge_into_base(cx: &mut TestAppContext) {
+    cx.update(|cx| init_settings(cx, |_| {}));
+
+    let base = cx.new_model(|cx| Buffer::local("abcdefghijk", cx));
+    let branch = base.update(cx, |buffer, cx| buffer.branch(cx));
+
+    // Make 2 edits, merge one into the base.
+    branch.update(cx, |branch, cx| {
+        branch.edit([(0..3, "ABC"), (7..9, "HI")], None, cx);
+        branch.merge_into_base(Some(7..7), cx);
+    });
+    base.read_with(cx, |base, _| assert_eq!(base.text(), "abcdefgHIjk"));
+    branch.read_with(cx, |branch, _| assert_eq!(branch.text(), "ABCdefgHIjk"));
+
+    // Undo the merge in the base buffer.
+    base.update(cx, |base, cx| {
+        base.undo(cx);
+    });
+    base.read_with(cx, |base, _| assert_eq!(base.text(), "abcdefghijk"));
+    branch.read_with(cx, |branch, _| assert_eq!(branch.text(), "ABCdefgHIjk"));
+
+    // Merge that operation into the base again.
+    branch.update(cx, |branch, cx| {
+        branch.merge_into_base(Some(7..7), cx);
+    });
+    base.read_with(cx, |base, _| assert_eq!(base.text(), "abcdefgHIjk"));
+    branch.read_with(cx, |branch, _| assert_eq!(branch.text(), "ABCdefgHIjk"));
 }
 
 fn start_recalculating_diff(buffer: &Model<Buffer>, cx: &mut TestAppContext) {

crates/text/src/text.rs

@@ -1430,16 +1430,22 @@ impl Buffer {
             counts.insert(edit_id, self.undo_map.undo_count(edit_id) + 1);
         }
 
+        let operation = self.undo_operations(counts);
+        self.history.push(operation.clone());
+        operation
+    }
+
+    pub fn undo_operations(&mut self, counts: HashMap<clock::Lamport, u32>) -> Operation {
+        let timestamp = self.lamport_clock.tick();
+        let version = self.version();
+        self.snapshot.version.observe(timestamp);
         let undo = UndoOperation {
-            timestamp: self.lamport_clock.tick(),
-            version: self.version(),
+            timestamp,
+            version,
             counts,
         };
         self.apply_undo(&undo);
-        self.snapshot.version.observe(undo.timestamp);
-        let operation = Operation::Undo(undo);
-        self.history.push(operation.clone());
-        operation
+        Operation::Undo(undo)
     }
 
     pub fn push_transaction(&mut self, transaction: Transaction, now: Instant) {