From b0a16a760146e9e07d134572364fac850daa9624 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Mon, 7 Oct 2024 16:28:33 -0700 Subject: [PATCH] Fix bugs with applying hunks from branch buffers (#18721) Release Notes: - N/A --------- Co-authored-by: Marshall --- crates/clock/src/clock.rs | 9 +-- crates/language/src/buffer.rs | 96 +++++++++++++++-------------- crates/language/src/buffer_tests.rs | 66 ++++++++++++++++++-- crates/text/src/text.rs | 18 ++++-- 4 files changed, 128 insertions(+), 61 deletions(-) diff --git a/crates/clock/src/clock.rs b/crates/clock/src/clock.rs index 2b45e4a8fa..acbde90dc1 100644 --- a/crates/clock/src/clock.rs +++ b/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, ": {}", self.local_branch_value)?; + if timestamp.replica_id == LOCAL_BRANCH_REPLICA_ID { + write!(f, ": {}", timestamp.value)?; + } else { + write!(f, "{}: {}", timestamp.replica_id, timestamp.value)?; + } } write!(f, "}}") } diff --git a/crates/language/src/buffer.rs b/crates/language/src/buffer.rs index 4990b9074f..160e8b3ba9 100644 --- a/crates/language/src/buffer.rs +++ b/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, rope: Rope, - operations_to_ignore: Vec, + merged_operations: Vec, }, } @@ -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::(&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::())) - } else { - None - } - }) - .collect::>(); + let mut edits = Vec::new(); + for edit in self.edits_since::(&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::(), + )); + } - 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, ) { - 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)] diff --git a/crates/language/src/buffer_tests.rs b/crates/language/src/buffer_tests.rs index da53d5a763..fe390d5510 100644 --- a/crates/language/src/buffer_tests.rs +++ b/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, cx: &mut TestAppContext) { diff --git a/crates/text/src/text.rs b/crates/text/src/text.rs index 80eafcf4eb..6c941401be 100644 --- a/crates/text/src/text.rs +++ b/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) -> 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) {