From b523ee6980c97bf6c1002610514bf8fd4e41b0dd Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Fri, 3 May 2024 11:18:43 +0300 Subject: [PATCH] Use Rope instead of String for buffer diff base (#11300) As an attempt to do things better when showing diff hunks, store diff base as Rope, not String, to have cheaper clones when the diff base text is reused, e.g. creating another buffer with the diff base text for hunk diff expanding. Release Notes: - N/A --- crates/collab/src/tests/editor_tests.rs | 4 +- crates/collab/src/tests/integration_tests.rs | 40 +++++++++++++++---- crates/editor/src/editor.rs | 19 +++++---- crates/editor/src/editor_tests.rs | 10 ++--- crates/editor/src/git.rs | 6 ++- crates/editor/src/hunk_diff.rs | 23 ++++++----- crates/editor/src/test.rs | 18 ++++----- crates/editor/src/test/editor_test_context.rs | 3 +- crates/git/src/diff.rs | 14 ++++--- crates/language/src/buffer.rs | 31 +++++++++++--- crates/project/src/project.rs | 14 ++++--- crates/text/src/text.rs | 10 ++++- crates/worktree/src/worktree.rs | 4 +- 13 files changed, 131 insertions(+), 65 deletions(-) diff --git a/crates/collab/src/tests/editor_tests.rs b/crates/collab/src/tests/editor_tests.rs index fd46ec68b6..097a2842c8 100644 --- a/crates/collab/src/tests/editor_tests.rs +++ b/crates/collab/src/tests/editor_tests.rs @@ -2073,7 +2073,7 @@ struct Row10;"#}; .as_singleton() .unwrap() .update(cx, |buffer, cx| { - buffer.set_diff_base(Some(base_text.to_string()), cx); + buffer.set_diff_base(Some(base_text.into()), cx); }); }); editor_cx_b.update_editor(|editor, cx| { @@ -2083,7 +2083,7 @@ struct Row10;"#}; .as_singleton() .unwrap() .update(cx, |buffer, cx| { - buffer.set_diff_base(Some(base_text.to_string()), cx); + buffer.set_diff_base(Some(base_text.into()), cx); }); }); cx_a.executor().run_until_parked(); diff --git a/crates/collab/src/tests/integration_tests.rs b/crates/collab/src/tests/integration_tests.rs index 8c1cbfe259..44889485b0 100644 --- a/crates/collab/src/tests/integration_tests.rs +++ b/crates/collab/src/tests/integration_tests.rs @@ -2570,7 +2570,10 @@ async fn test_git_diff_base_change( // Smoke test diffing buffer_local_a.read_with(cx_a, |buffer, _| { - assert_eq!(buffer.diff_base(), Some(diff_base.as_ref())); + assert_eq!( + buffer.diff_base().map(|rope| rope.to_string()).as_deref(), + Some(diff_base.as_str()) + ); git::diff::assert_hunks( buffer.snapshot().git_diff_hunks_in_row_range(0..4), &buffer, @@ -2591,7 +2594,10 @@ async fn test_git_diff_base_change( // Smoke test diffing buffer_remote_a.read_with(cx_b, |buffer, _| { - assert_eq!(buffer.diff_base(), Some(diff_base.as_ref())); + assert_eq!( + buffer.diff_base().map(|rope| rope.to_string()).as_deref(), + Some(diff_base.as_str()) + ); git::diff::assert_hunks( buffer.snapshot().git_diff_hunks_in_row_range(0..4), &buffer, @@ -2611,7 +2617,10 @@ async fn test_git_diff_base_change( // Smoke test new diffing buffer_local_a.read_with(cx_a, |buffer, _| { - assert_eq!(buffer.diff_base(), Some(new_diff_base.as_ref())); + assert_eq!( + buffer.diff_base().map(|rope| rope.to_string()).as_deref(), + Some(new_diff_base.as_str()) + ); git::diff::assert_hunks( buffer.snapshot().git_diff_hunks_in_row_range(0..4), @@ -2624,7 +2633,10 @@ async fn test_git_diff_base_change( // Smoke test B buffer_remote_a.read_with(cx_b, |buffer, _| { - assert_eq!(buffer.diff_base(), Some(new_diff_base.as_ref())); + assert_eq!( + buffer.diff_base().map(|rope| rope.to_string()).as_deref(), + Some(new_diff_base.as_str()) + ); git::diff::assert_hunks( buffer.snapshot().git_diff_hunks_in_row_range(0..4), &buffer, @@ -2664,7 +2676,10 @@ async fn test_git_diff_base_change( // Smoke test diffing buffer_local_b.read_with(cx_a, |buffer, _| { - assert_eq!(buffer.diff_base(), Some(diff_base.as_ref())); + assert_eq!( + buffer.diff_base().map(|rope| rope.to_string()).as_deref(), + Some(diff_base.as_str()) + ); git::diff::assert_hunks( buffer.snapshot().git_diff_hunks_in_row_range(0..4), &buffer, @@ -2685,7 +2700,10 @@ async fn test_git_diff_base_change( // Smoke test diffing buffer_remote_b.read_with(cx_b, |buffer, _| { - assert_eq!(buffer.diff_base(), Some(diff_base.as_ref())); + assert_eq!( + buffer.diff_base().map(|rope| rope.to_string()).as_deref(), + Some(diff_base.as_str()) + ); git::diff::assert_hunks( buffer.snapshot().git_diff_hunks_in_row_range(0..4), &buffer, @@ -2705,7 +2723,10 @@ async fn test_git_diff_base_change( // Smoke test new diffing buffer_local_b.read_with(cx_a, |buffer, _| { - assert_eq!(buffer.diff_base(), Some(new_diff_base.as_ref())); + assert_eq!( + buffer.diff_base().map(|rope| rope.to_string()).as_deref(), + Some(new_diff_base.as_str()) + ); println!("{:?}", buffer.as_rope().to_string()); println!("{:?}", buffer.diff_base()); println!( @@ -2727,7 +2748,10 @@ async fn test_git_diff_base_change( // Smoke test B buffer_remote_b.read_with(cx_b, |buffer, _| { - assert_eq!(buffer.diff_base(), Some(new_diff_base.as_ref())); + assert_eq!( + buffer.diff_base().map(|rope| rope.to_string()).as_deref(), + Some(new_diff_base.as_str()) + ); git::diff::assert_hunks( buffer.snapshot().git_diff_hunks_in_row_range(0..4), &buffer, diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index c9d20c2765..ef5c97a592 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -4977,10 +4977,16 @@ impl Editor { if !revert_changes.is_empty() { self.transact(cx, |editor, cx| { editor.buffer().update(cx, |multi_buffer, cx| { - for (buffer_id, buffer_revert_ranges) in revert_changes { + for (buffer_id, changes) in revert_changes { if let Some(buffer) = multi_buffer.buffer(buffer_id) { buffer.update(cx, |buffer, cx| { - buffer.edit(buffer_revert_ranges, None, cx); + buffer.edit( + changes.into_iter().map(|(range, text)| { + (range, text.to_string().map(Arc::::from)) + }), + None, + cx, + ); }); } } @@ -5013,7 +5019,7 @@ impl Editor { &mut self, selections: &[Selection], cx: &mut ViewContext<'_, Editor>, - ) -> HashMap, Arc)>> { + ) -> HashMap, Rope)>> { let mut revert_changes = HashMap::default(); self.buffer.update(cx, |multi_buffer, cx| { let multi_buffer_snapshot = multi_buffer.snapshot(cx); @@ -5025,14 +5031,14 @@ impl Editor { } fn prepare_revert_change( - revert_changes: &mut HashMap, Arc)>>, + revert_changes: &mut HashMap, Rope)>>, multi_buffer: &MultiBuffer, hunk: &DiffHunk, cx: &mut AppContext, ) -> Option<()> { let buffer = multi_buffer.buffer(hunk.buffer_id)?; let buffer = buffer.read(cx); - let original_text = buffer.diff_base()?.get(hunk.diff_base_byte_range.clone())?; + let original_text = buffer.diff_base()?.slice(hunk.diff_base_byte_range.clone()); let buffer_snapshot = buffer.snapshot(); let buffer_revert_changes = revert_changes.entry(buffer.remote_id()).or_default(); if let Err(i) = buffer_revert_changes.binary_search_by(|probe| { @@ -5041,9 +5047,8 @@ impl Editor { .start .cmp(&hunk.buffer_range.start, &buffer_snapshot) .then(probe.0.end.cmp(&hunk.buffer_range.end, &buffer_snapshot)) - .then(probe.1.as_ref().cmp(original_text)) }) { - buffer_revert_changes.insert(i, (hunk.buffer_range.clone(), Arc::from(original_text))); + buffer_revert_changes.insert(i, (hunk.buffer_range.clone(), original_text)); Some(()) } else { None diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 76db71bd0b..690713f89e 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -9026,7 +9026,7 @@ async fn test_multibuffer_reverts(cx: &mut gpui::TestAppContext) { .collect::(), cx, ); - buffer.set_diff_base(Some(sample_text), cx); + buffer.set_diff_base(Some(sample_text.into()), cx); }); cx.executor().run_until_parked(); } @@ -10041,17 +10041,17 @@ async fn test_toggle_diff_expand_in_multi_buffer(cx: &mut gpui::TestAppContext) "vvvv\nwwww\nxxxx\nyyyy\nzzzz\n@@@@\n{{{{\n||||\n}}}}\n~~~~\n\u{7f}\u{7f}\u{7f}\u{7f}"; let buffer_1 = cx.new_model(|cx| { let mut buffer = Buffer::local(modified_sample_text_1.to_string(), cx); - buffer.set_diff_base(Some(sample_text_1.clone()), cx); + buffer.set_diff_base(Some(sample_text_1.clone().into()), cx); buffer }); let buffer_2 = cx.new_model(|cx| { let mut buffer = Buffer::local(modified_sample_text_2.to_string(), cx); - buffer.set_diff_base(Some(sample_text_2.clone()), cx); + buffer.set_diff_base(Some(sample_text_2.clone().into()), cx); buffer }); let buffer_3 = cx.new_model(|cx| { let mut buffer = Buffer::local(modified_sample_text_3.to_string(), cx); - buffer.set_diff_base(Some(sample_text_3.clone()), cx); + buffer.set_diff_base(Some(sample_text_3.clone().into()), cx); buffer }); @@ -11351,7 +11351,7 @@ fn assert_hunk_revert( .as_singleton() .unwrap() .update(cx, |buffer, cx| { - buffer.set_diff_base(Some(base_text.to_string()), cx); + buffer.set_diff_base(Some(base_text.into()), cx); }); }); cx.executor().run_until_parked(); diff --git a/crates/editor/src/git.rs b/crates/editor/src/git.rs index 7b43b4c230..3712b4cf4c 100644 --- a/crates/editor/src/git.rs +++ b/crates/editor/src/git.rs @@ -145,7 +145,8 @@ mod tests { 1.five 1.six " - .unindent(), + .unindent() + .into(), ), cx, ); @@ -181,7 +182,8 @@ mod tests { 2.four 2.six " - .unindent(), + .unindent() + .into(), ), cx, ); diff --git a/crates/editor/src/hunk_diff.rs b/crates/editor/src/hunk_diff.rs index e2ca19a567..ef84833a4d 100644 --- a/crates/editor/src/hunk_diff.rs +++ b/crates/editor/src/hunk_diff.rs @@ -226,13 +226,14 @@ impl Editor { .or_else(|| self.current_diff_base_buffer(&buffer, cx)) .or_else(|| create_diff_base_buffer(&buffer, cx)); let buffer = buffer.read(cx); - let deleted_text_lines = buffer.diff_base().and_then(|diff_base| { - Some( - diff_base - .get(hunk.diff_base_byte_range.clone())? - .lines() - .count(), - ) + let deleted_text_lines = buffer.diff_base().map(|diff_base| { + let diff_start_row = diff_base + .offset_to_point(hunk.diff_base_byte_range.start) + .row; + let diff_end_row = + diff_base.offset_to_point(hunk.diff_base_byte_range.end).row; + let line_count = diff_end_row - diff_start_row; + line_count as usize }); Some(( diff_base_buffer?, @@ -542,12 +543,12 @@ fn create_diff_base_buffer(buffer: &Model, cx: &mut AppContext) -> Optio buffer .update(cx, |buffer, _| { let language = buffer.language().cloned(); - let diff_base = buffer.diff_base().map(|s| s.to_owned()); - Some((diff_base?, language)) + let diff_base = buffer.diff_base()?.clone(); + Some((buffer.line_ending(), diff_base, language)) }) - .map(|(diff_base, language)| { + .map(|(line_ending, diff_base, language)| { cx.new_model(|cx| { - let buffer = Buffer::local(diff_base, cx); + let buffer = Buffer::local_normalized(diff_base, line_ending, cx); match language { Some(language) => buffer.with_language(language, cx), None => buffer, diff --git a/crates/editor/src/test.rs b/crates/editor/src/test.rs index 1bb8408889..af1bc75201 100644 --- a/crates/editor/src/test.rs +++ b/crates/editor/src/test.rs @@ -99,12 +99,13 @@ pub fn editor_hunks( .read(cx) .excerpt_containing(Point::new(hunk.associated_range.start, 0), cx) .expect("no excerpt for expanded buffer's hunk start"); - let diff_base = &buffer + let diff_base = buffer .read(cx) .diff_base() .expect("should have a diff base for expanded hunk") - [hunk.diff_base_byte_range.clone()]; - (diff_base.to_owned(), hunk.status(), display_range) + .slice(hunk.diff_base_byte_range.clone()) + .to_string(); + (diff_base, hunk.status(), display_range) }) .collect() } @@ -134,16 +135,13 @@ pub fn expanded_hunks( .read(cx) .excerpt_containing(expanded_hunk.hunk_range.start, cx) .expect("no excerpt for expanded buffer's hunk start"); - let diff_base = &buffer + let diff_base = buffer .read(cx) .diff_base() .expect("should have a diff base for expanded hunk") - [expanded_hunk.diff_base_byte_range.clone()]; - ( - diff_base.to_owned(), - expanded_hunk.status, - hunk_display_range, - ) + .slice(expanded_hunk.diff_base_byte_range.clone()) + .to_string(); + (diff_base, expanded_hunk.status, hunk_display_range) }) .collect() } diff --git a/crates/editor/src/test/editor_test_context.rs b/crates/editor/src/test/editor_test_context.rs index a2798da18f..8ef5480563 100644 --- a/crates/editor/src/test/editor_test_context.rs +++ b/crates/editor/src/test/editor_test_context.rs @@ -21,6 +21,7 @@ use std::{ Arc, }, }; +use text::Rope; use ui::Context; use util::{ assert_set_eq, @@ -271,7 +272,7 @@ impl EditorTestContext { } pub fn set_diff_base(&mut self, diff_base: Option<&str>) { - let diff_base = diff_base.map(String::from); + let diff_base = diff_base.map(Rope::from); self.update_buffer(|buffer, cx| buffer.set_diff_base(diff_base, cx)); } diff --git a/crates/git/src/diff.rs b/crates/git/src/diff.rs index 3cab8cb664..29806bcd56 100644 --- a/crates/git/src/diff.rs +++ b/crates/git/src/diff.rs @@ -1,3 +1,4 @@ +use rope::Rope; use std::{iter, ops::Range}; use sum_tree::SumTree; use text::{Anchor, BufferId, BufferSnapshot, OffsetRangeExt, Point}; @@ -179,11 +180,12 @@ impl BufferDiff { self.tree = SumTree::new(); } - pub async fn update(&mut self, diff_base: &str, buffer: &text::BufferSnapshot) { + pub async fn update(&mut self, diff_base: &Rope, buffer: &text::BufferSnapshot) { let mut tree = SumTree::new(); + let diff_base_text = diff_base.to_string(); let buffer_text = buffer.as_rope().to_string(); - let patch = Self::diff(diff_base, &buffer_text); + let patch = Self::diff(&diff_base_text, &buffer_text); if let Some(patch) = patch { let mut divergence = 0; @@ -345,6 +347,7 @@ mod tests { three " .unindent(); + let diff_base_rope = Rope::from(diff_base.clone()); let buffer_text = " one @@ -355,7 +358,7 @@ mod tests { let mut buffer = Buffer::new(0, BufferId::new(1).unwrap(), buffer_text); let mut diff = BufferDiff::new(); - smol::block_on(diff.update(&diff_base, &buffer)); + smol::block_on(diff.update(&diff_base_rope, &buffer)); assert_hunks( diff.hunks(&buffer), &buffer, @@ -364,7 +367,7 @@ mod tests { ); buffer.edit([(0..0, "point five\n")]); - smol::block_on(diff.update(&diff_base, &buffer)); + smol::block_on(diff.update(&diff_base_rope, &buffer)); assert_hunks( diff.hunks(&buffer), &buffer, @@ -391,6 +394,7 @@ mod tests { ten " .unindent(); + let diff_base_rope = Rope::from(diff_base.clone()); let buffer_text = " A @@ -415,7 +419,7 @@ mod tests { let buffer = Buffer::new(0, BufferId::new(1).unwrap(), buffer_text); let mut diff = BufferDiff::new(); - smol::block_on(diff.update(&diff_base, &buffer)); + smol::block_on(diff.update(&diff_base_rope, &buffer)); assert_eq!(diff.hunks(&buffer).count(), 8); assert_hunks( diff --git a/crates/language/src/buffer.rs b/crates/language/src/buffer.rs index 836e99bad3..8428338fc1 100644 --- a/crates/language/src/buffer.rs +++ b/crates/language/src/buffer.rs @@ -78,7 +78,7 @@ pub enum Capability { /// syntax trees, git status, and diagnostics. pub struct Buffer { text: TextBuffer, - diff_base: Option, + diff_base: Option, git_diff: git::diff::BufferDiff, file: Option>, /// The mtime of the file when this buffer was last loaded from @@ -512,6 +512,25 @@ impl Buffer { ) } + /// Create a new buffer with the given base text that has proper line endings and other normalization applied. + pub fn local_normalized( + base_text_normalized: Rope, + line_ending: LineEnding, + cx: &mut ModelContext, + ) -> Self { + Self::build( + TextBuffer::new_normalized( + 0, + cx.entity_id().as_non_zero_u64().into(), + line_ending, + base_text_normalized, + ), + None, + None, + Capability::ReadWrite, + ) + } + /// Create a new buffer that is a replica of a remote buffer. pub fn remote( remote_id: BufferId, @@ -540,7 +559,7 @@ impl Buffer { let buffer = TextBuffer::new(replica_id, buffer_id, message.base_text); let mut this = Self::build( buffer, - message.diff_base.map(|text| text.into_boxed_str().into()), + message.diff_base.map(|text| text.into()), file, capability, ); @@ -632,7 +651,7 @@ impl Buffer { /// Builds a [Buffer] with the given underlying [TextBuffer], diff base, [File] and [Capability]. pub fn build( buffer: TextBuffer, - diff_base: Option, + diff_base: Option, file: Option>, capability: Capability, ) -> Self { @@ -868,13 +887,13 @@ impl Buffer { } /// Returns the current diff base, see [Buffer::set_diff_base]. - pub fn diff_base(&self) -> Option<&str> { - self.diff_base.as_deref() + pub fn diff_base(&self) -> Option<&Rope> { + self.diff_base.as_ref() } /// Sets the text that will be used to compute a Git diff /// against the buffer text. - pub fn set_diff_base(&mut self, diff_base: Option, cx: &mut ModelContext) { + pub fn set_diff_base(&mut self, diff_base: Option, cx: &mut ModelContext) { self.diff_base = diff_base; self.diff_base_version += 1; if let Some(recalc_task) = self.git_diff_recalc(cx) { diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index c8669bca43..733a06172b 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -98,7 +98,7 @@ use std::{ }; use task::static_source::{StaticSource, TrackedFile}; use terminals::Terminals; -use text::{Anchor, BufferId, LineEnding}; +use text::{Anchor, BufferId, LineEnding, Rope}; use util::{ debug_panic, defer, http::{HttpClient, Url}, @@ -7586,7 +7586,11 @@ impl Project { None } else { let relative_path = path.strip_prefix(&work_directory).ok()?; - repo_entry.repo().lock().load_index_text(relative_path) + repo_entry + .repo() + .lock() + .load_index_text(relative_path) + .map(Rope::from) }; Some((buffer, base_text)) } @@ -7614,7 +7618,7 @@ impl Project { .send(proto::UpdateDiffBase { project_id, buffer_id, - diff_base, + diff_base: diff_base.map(|rope| rope.to_string()), }) .log_err(); } @@ -8694,7 +8698,7 @@ impl Project { this.update(&mut cx, |this, cx| { let buffer_id = envelope.payload.buffer_id; let buffer_id = BufferId::new(buffer_id)?; - let diff_base = envelope.payload.diff_base; + let diff_base = envelope.payload.diff_base.map(Rope::from); if let Some(buffer) = this .opened_buffers .get_mut(&buffer_id) @@ -8859,7 +8863,7 @@ impl Project { .send(proto::UpdateDiffBase { project_id, buffer_id: buffer_id.into(), - diff_base: buffer.diff_base().map(Into::into), + diff_base: buffer.diff_base().map(ToString::to_string), }) .log_err(); diff --git a/crates/text/src/text.rs b/crates/text/src/text.rs index 4c780b3a2b..c02562ea57 100644 --- a/crates/text/src/text.rs +++ b/crates/text/src/text.rs @@ -520,8 +520,16 @@ impl Buffer { pub fn new(replica_id: u16, remote_id: BufferId, mut base_text: String) -> Buffer { let line_ending = LineEnding::detect(&base_text); LineEnding::normalize(&mut base_text); + Self::new_normalized(replica_id, remote_id, line_ending, Rope::from(base_text)) + } - let history = History::new(Rope::from(base_text.as_ref())); + pub fn new_normalized( + replica_id: u16, + remote_id: BufferId, + line_ending: LineEnding, + normalized: Rope, + ) -> Buffer { + let history = History::new(normalized); let mut fragments = SumTree::new(); let mut insertions = SumTree::new(); diff --git a/crates/worktree/src/worktree.rs b/crates/worktree/src/worktree.rs index 1f1ddd508c..b6ddb381a3 100644 --- a/crates/worktree/src/worktree.rs +++ b/crates/worktree/src/worktree.rs @@ -1065,7 +1065,7 @@ impl LocalWorktree { &self, path: &Path, cx: &mut ModelContext, - ) -> Task)>> { + ) -> Task)>> { let path = Arc::from(path); let abs_path = self.absolutize(&path); let fs = self.fs.clone(); @@ -1096,7 +1096,7 @@ impl LocalWorktree { if abs_path_metadata.is_dir || abs_path_metadata.is_symlink { None } else { - git_repo.lock().load_index_text(&repo_path) + git_repo.lock().load_index_text(&repo_path).map(Rope::from) } } }));