address remaining fixmes

Cole Miller created

Change summary

crates/buffer_diff/src/buffer_diff.rs         | 35 +++++++++-----------
crates/multi_buffer/src/multi_buffer.rs       | 11 ++++++
crates/multi_buffer/src/multi_buffer_tests.rs | 34 ++++++-------------
3 files changed, 38 insertions(+), 42 deletions(-)

Detailed changes

crates/buffer_diff/src/buffer_diff.rs 🔗

@@ -329,36 +329,30 @@ impl BufferDiffSnapshot {
         buffer: &text::BufferSnapshot,
     ) -> u32 {
         // TODO(split-diff) expose a parameter to reuse a cursor to avoid repeatedly seeking from the start
-
-        // Find the last hunk that starts before this position.
+        let target = buffer.anchor_before(Point::new(row, 0));
+        // Find the last hunk that starts before the target.
         let mut cursor = self.inner.hunks.cursor::<DiffHunkSummary>(buffer);
-        let position = buffer.anchor_before(Point::new(row, 0));
-        cursor.seek(&position, Bias::Left);
+        cursor.seek(&target, Bias::Left);
         if cursor
             .item()
-            .is_none_or(|hunk| hunk.buffer_range.start.cmp(&position, buffer).is_gt())
+            .is_none_or(|hunk| hunk.buffer_range.start.cmp(&target, buffer).is_gt())
         {
             cursor.prev();
         }
 
         let unclipped_point = if let Some(hunk) = cursor.item()
-            && hunk.buffer_range.start.cmp(&position, buffer).is_le()
+            && hunk.buffer_range.start.cmp(&target, buffer).is_le()
         {
-            let unclipped_point = if position.cmp(&cursor.end().buffer_range.end, buffer).is_ge() {
-                let mut unclipped_point = cursor
-                    .end()
-                    .diff_base_byte_range
-                    .end
-                    .to_point(self.base_text());
+            // Found a hunk that starts before the target.
+            let hunk_base_text_end = cursor.end().diff_base_byte_range.end;
+            let unclipped_point = if target.cmp(&cursor.end().buffer_range.end, buffer).is_ge() {
+                // Target falls strictly between two hunks.
+                let mut unclipped_point = hunk_base_text_end.to_point(self.base_text());
                 unclipped_point +=
                     Point::new(row, 0) - cursor.end().buffer_range.end.to_point(buffer);
                 unclipped_point
             } else if bias == Bias::Right {
-                cursor
-                    .end()
-                    .diff_base_byte_range
-                    .end
-                    .to_point(self.base_text())
+                hunk_base_text_end.to_point(self.base_text())
             } else {
                 hunk.diff_base_byte_range.start.to_point(self.base_text())
             };
@@ -366,13 +360,16 @@ impl BufferDiffSnapshot {
             cursor.next();
             unclipped_point
         } else {
-            // Position is before the added region for the first hunk.
+            // Target is before the added region for the first hunk.
             debug_assert!(self.inner.hunks.first().is_none_or(|first_hunk| {
-                position.cmp(&first_hunk.buffer_range.start, buffer).is_le()
+                target.cmp(&first_hunk.buffer_range.start, buffer).is_le()
             }));
             Point::new(row, 0)
         };
 
+        // If the target falls in the region between two hunks, we added an overshoot above.
+        // There may be changes in the main buffer that are not reflected in the hunks,
+        // so we need to ensure this overshoot keeps us in the corresponding base text region.
         let max_point = if let Some(next_hunk) = cursor.item() {
             next_hunk
                 .diff_base_byte_range

crates/multi_buffer/src/multi_buffer.rs 🔗

@@ -506,6 +506,7 @@ struct BufferState {
     _subscriptions: [gpui::Subscription; 2],
 }
 
+#[derive(Clone)]
 struct DiffState {
     diff: Entity<BufferDiff>,
     /// Whether the [`MultiBuffer`] this is associated with is inverted (i.e.
@@ -2582,6 +2583,16 @@ impl MultiBuffer {
         self.diffs.get(&buffer_id).map(|state| state.diff.clone())
     }
 
+    #[cfg(test)]
+    fn inverted_diff_for_main_buffer(&self, buffer: &Entity<Buffer>) -> Option<Entity<BufferDiff>> {
+        let diff = self.diffs.values().find(|diff| {
+            diff.main_buffer
+                .as_ref()
+                .is_some_and(|main_buffer| main_buffer.entity_id() == buffer.entity_id())
+        })?;
+        Some(diff.diff.clone())
+    }
+
     pub fn expand_diff_hunks(&mut self, ranges: Vec<Range<Anchor>>, cx: &mut Context<Self>) {
         self.expand_or_collapse_diff_hunks(ranges, true, cx);
     }

crates/multi_buffer/src/multi_buffer_tests.rs 🔗

@@ -2295,6 +2295,7 @@ async fn test_diff_hunks_with_multiple_excerpts(cx: &mut TestAppContext) {
 struct ReferenceMultibuffer {
     excerpts: Vec<ReferenceExcerpt>,
     diffs: HashMap<BufferId, Entity<BufferDiff>>,
+    invert_diffs: bool,
 }
 
 #[derive(Debug)]
@@ -2424,11 +2425,7 @@ impl ReferenceMultibuffer {
         }
     }
 
-    fn expected_content(
-        &self,
-        all_diff_hunks_expanded: bool,
-        cx: &App,
-    ) -> (String, Vec<RowInfo>, HashSet<MultiBufferRow>) {
+    fn expected_content(&self, cx: &App) -> (String, Vec<RowInfo>, HashSet<MultiBufferRow>) {
         let mut text = String::new();
         let mut regions = Vec::<ReferenceRegion>::new();
         let mut excerpt_boundary_rows = HashSet::default();
@@ -2459,12 +2456,10 @@ impl ReferenceMultibuffer {
                     continue;
                 }
 
-                if !all_diff_hunks_expanded
-                    && !excerpt.expanded_diff_hunks.iter().any(|expanded_anchor| {
-                        expanded_anchor.to_offset(buffer).max(buffer_range.start)
-                            == hunk_range.start.max(buffer_range.start)
-                    })
-                {
+                if !excerpt.expanded_diff_hunks.iter().any(|expanded_anchor| {
+                    expanded_anchor.to_offset(buffer).max(buffer_range.start)
+                        == hunk_range.start.max(buffer_range.start)
+                }) {
                     log::trace!("skipping a hunk that's not marked as expanded");
                     continue;
                 }
@@ -2750,22 +2745,15 @@ async fn test_random_set_ranges(cx: &mut TestAppContext, mut rng: StdRng) {
 
 #[gpui::test(iterations = 100)]
 async fn test_random_multibuffer(cx: &mut TestAppContext, rng: StdRng) {
-    let multibuffer = cx.new(|_| MultiBuffer::new(Capability::ReadWrite));
-    test_random_multibuffer_impl(multibuffer, cx, rng).await;
+    test_random_multibuffer_impl(cx, rng).await;
 }
 
-async fn test_random_multibuffer_impl(
-    multibuffer: Entity<MultiBuffer>,
-    cx: &mut TestAppContext,
-    mut rng: StdRng,
-) {
+async fn test_random_multibuffer_impl(cx: &mut TestAppContext, mut rng: StdRng) {
     let operations = env::var("OPERATIONS")
         .map(|i| i.parse().expect("invalid `OPERATIONS` variable"))
         .unwrap_or(10);
 
-    multibuffer.read_with(cx, |multibuffer, _| assert!(multibuffer.is_empty()));
-    let all_diff_hunks_expanded =
-        multibuffer.read_with(cx, |multibuffer, _| multibuffer.all_diff_hunks_expanded());
+    let multibuffer = cx.new(|_| MultiBuffer::new(Capability::ReadWrite));
     let mut buffers: Vec<Entity<Buffer>> = Vec::new();
     let mut base_texts: HashMap<BufferId, String> = HashMap::default();
     let mut reference = ReferenceMultibuffer::default();
@@ -2869,7 +2857,7 @@ async fn test_random_multibuffer_impl(
                     assert!(excerpt.contains(anchor));
                 }
             }
-            45..=55 if !reference.excerpts.is_empty() && !all_diff_hunks_expanded => {
+            45..=55 if !reference.excerpts.is_empty() => {
                 multibuffer.update(cx, |multibuffer, cx| {
                     let snapshot = multibuffer.snapshot(cx);
                     let excerpt_ix = rng.random_range(0..reference.excerpts.len());
@@ -3022,7 +3010,7 @@ fn check_multibuffer(
     let actual_row_infos = snapshot.row_infos(MultiBufferRow(0)).collect::<Vec<_>>();
 
     let (expected_text, expected_row_infos, expected_boundary_rows) =
-        reference.expected_content(snapshot.all_diff_hunks_expanded, cx);
+        reference.expected_content(cx);
 
     let has_diff = actual_row_infos
         .iter()