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

@@ -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();

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,

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::<str>::from))
+                                    }),
+                                    None,
+                                    cx,
+                                );
                             });
                         }
                     }
@@ -5013,7 +5019,7 @@ impl Editor {
         &mut self,
         selections: &[Selection<Anchor>],
         cx: &mut ViewContext<'_, Editor>,
-    ) -> HashMap<BufferId, Vec<(Range<text::Anchor>, Arc<str>)>> {
+    ) -> HashMap<BufferId, Vec<(Range<text::Anchor>, 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<BufferId, Vec<(Range<text::Anchor>, Arc<str>)>>,
+        revert_changes: &mut HashMap<BufferId, Vec<(Range<text::Anchor>, Rope)>>,
         multi_buffer: &MultiBuffer,
         hunk: &DiffHunk<u32>,
         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

crates/editor/src/editor_tests.rs

@@ -9026,7 +9026,7 @@ async fn test_multibuffer_reverts(cx: &mut gpui::TestAppContext) {
                     .collect::<String>(),
                 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();

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,
             );

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| {
+                    let deleted_text_lines = buffer.diff_base().map(|diff_base| {
-                        Some(
+                        let diff_start_row = diff_base
-                            diff_base
+                            .offset_to_point(hunk.diff_base_byte_range.start)
-                                .get(hunk.diff_base_byte_range.clone())?
+                            .row;
-                                .lines()
+                        let diff_end_row =
-                                .count(),
+                            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<Buffer>, cx: &mut AppContext) -> Optio
     buffer
         .update(cx, |buffer, _| {
             let language = buffer.language().cloned();
-            let diff_base = buffer.diff_base().map(|s| s.to_owned());
+            let diff_base = buffer.diff_base()?.clone();
-            Some((diff_base?, language))
+            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,

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()];
+                .slice(hunk.diff_base_byte_range.clone())
-            (diff_base.to_owned(), hunk.status(), display_range)
+                .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()];
+                .slice(expanded_hunk.diff_base_byte_range.clone())
-            (
+                .to_string();
-                diff_base.to_owned(),
+            (diff_base, expanded_hunk.status, hunk_display_range)
-                expanded_hunk.status,
-                hunk_display_range,
-            )
         })
         .collect()
 }

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));
     }
 

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(

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<String>,
+    diff_base: Option<Rope>,
     git_diff: git::diff::BufferDiff,
     file: Option<Arc<dyn File>>,
     /// 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 {
+        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<String>,
+        diff_base: Option<Rope>,
         file: Option<Arc<dyn File>>,
         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> {
+    pub fn diff_base(&self) -> Option<&Rope> {
-        self.diff_base.as_deref()
+        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<String>, cx: &mut ModelContext<Self>) {
+    pub fn set_diff_base(&mut self, diff_base: Option<Rope>, cx: &mut ModelContext<Self>) {
         self.diff_base = diff_base;
         self.diff_base_version += 1;
         if let Some(recalc_task) = self.git_diff_recalc(cx) {

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();
 

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();
 

crates/worktree/src/worktree.rs

@@ -1065,7 +1065,7 @@ impl LocalWorktree {
         &self,
         path: &Path,
         cx: &mut ModelContext<Worktree>,
-    ) -> Task<Result<(File, String, Option<String>)>> {
+    ) -> Task<Result<(File, String, Option<Rope>)>> {
         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)
                                 }
                             }
                         }));