git: Fix panic in split diff when edits cause excerpts to merge (#49122) (cherry-pick to preview) (#49413)

zed-zippy[bot] , Cole Miller , and Zed Zippy created 1 month ago

Cherry-pick of #49122 to preview

----
Our strategy for keeping excerpts in sync between the two sides of the
split diff is generally

- mutate the excerpts on the RHS
- pull out the new excerpts for the affected path from the RHS
- translate those ranges to the LHS
- update the LHS excerpts with those ranges

But this can break down when the translated ranges overlap on the LHS
despite the original ranges not overlapping on the RHS. This can happen,
for example, when a large deletion in a file crosses several excerpt
boundaries. It's rare because normally excerpt boundaries in the project
diff and branch diff are adjusted to keep entire diff hunks in view, but
we defer this for dirty files, so it's not hard to trigger it when using
the agent or editing an individual file while the diff is open in
another tab.

Release Notes:

- Fixed a panic that could occur when editing files while the project
diff or branch diff was open.

---------

Co-authored-by: Zed Zippy
<234243425+zed-zippy[bot]@users.noreply.github.com>

Co-authored-by: Cole Miller <[e-mail]>
Co-authored-by: Zed Zippy <234243425+zed-zippy[bot]@users.noreply.github.com>

Change summary

crates/editor/src/split.rs              | 192 +++++++++++++++++++++-----
crates/multi_buffer/src/multi_buffer.rs |  31 ++++
crates/multi_buffer/src/path_key.rs     |  24 ++
3 files changed, 203 insertions(+), 44 deletions(-)

Detailed changes

crates/editor/src/split.rs

@@ -674,21 +674,34 @@ impl SplittableEditor {
         // stream this
         for (path, diff) in path_diffs {
             self.rhs_multibuffer.update(cx, |rhs_multibuffer, cx| {
-                lhs.multibuffer.update(cx, |lhs_multibuffer, lhs_cx| {
-                    for (lhs, rhs) in LhsEditor::update_path_excerpts_from_rhs(
+                let sync_result = lhs.multibuffer.update(cx, |lhs_multibuffer, lhs_cx| {
+                    LhsEditor::update_path_excerpts_from_rhs(
                         path.clone(),
                         rhs_multibuffer,
                         lhs_multibuffer,
                         diff.clone(),
                         lhs_cx,
-                    ) {
-                        companion.add_excerpt_mapping(lhs, rhs);
+                    )
+                });
+
+                if let Some((lhs_excerpt_ids, rhs_merge_groups)) = sync_result {
+                    let mut final_rhs_ids = Vec::with_capacity(lhs_excerpt_ids.len());
+                    for group in rhs_merge_groups {
+                        if group.len() == 1 {
+                            final_rhs_ids.push(group[0]);
+                        } else {
+                            let merged_id = rhs_multibuffer.merge_excerpts(&group, cx);
+                            final_rhs_ids.push(merged_id);
+                        }
                     }
-                    companion.add_buffer_mapping(
-                        diff.read(lhs_cx).base_text(lhs_cx).remote_id(),
-                        diff.read(lhs_cx).buffer_id,
-                    );
-                })
+
+                    for (rhs_id, lhs_id) in final_rhs_ids.iter().zip(lhs_excerpt_ids.iter()) {
+                        companion.add_excerpt_mapping(*lhs_id, *rhs_id);
+                    }
+                    let lhs_buffer_id = diff.read(cx).base_text(cx).remote_id();
+                    let rhs_buffer_id = diff.read(cx).buffer_id;
+                    companion.add_buffer_mapping(lhs_buffer_id, rhs_buffer_id);
+                }
             });
         }
 
@@ -1940,18 +1953,52 @@ fn mutate_excerpts_for_paths<R>(
     let result = mutate(rhs_multibuffer, cx);
 
     if let Some(lhs) = lhs {
+        let mut sync_results = Vec::new();
+        let mut diffs_for_mapping = Vec::new();
+
         for ((path, diff), old_rhs_ids) in paths_with_diffs.into_iter().zip(old_rhs_ids) {
-            lhs.multibuffer.update(cx, |lhs_multibuffer, lhs_cx| {
+            let sync_result = lhs.multibuffer.update(cx, |lhs_multibuffer, lhs_cx| {
                 LhsEditor::sync_path_excerpts(
                     path,
                     old_rhs_ids,
                     rhs_multibuffer,
                     lhs_multibuffer,
-                    diff,
+                    diff.clone(),
                     rhs_display_map,
                     lhs_cx,
-                );
+                )
             });
+            if let Some(sync_result) = sync_result {
+                sync_results.push(sync_result);
+                diffs_for_mapping.push(diff);
+            }
+        }
+
+        for ((lhs_excerpt_ids, rhs_merge_groups), diff) in
+            sync_results.into_iter().zip(diffs_for_mapping.into_iter())
+        {
+            let mut final_rhs_ids = Vec::with_capacity(lhs_excerpt_ids.len());
+            for group in rhs_merge_groups {
+                if group.len() == 1 {
+                    final_rhs_ids.push(group[0]);
+                } else {
+                    let merged_id = rhs_multibuffer.merge_excerpts(&group, cx);
+                    final_rhs_ids.push(merged_id);
+                }
+            }
+
+            debug_assert_eq!(final_rhs_ids.len(), lhs_excerpt_ids.len());
+
+            if let Some(companion) = rhs_display_map.read(cx).companion().cloned() {
+                let lhs_buffer_id = diff.read(cx).base_text(cx).remote_id();
+                let rhs_buffer_id = diff.read(cx).buffer_id;
+                companion.update(cx, |c, _| {
+                    for (rhs_id, lhs_id) in final_rhs_ids.iter().zip(lhs_excerpt_ids.iter()) {
+                        c.add_excerpt_mapping(*lhs_id, *rhs_id);
+                    }
+                    c.add_buffer_mapping(lhs_buffer_id, rhs_buffer_id);
+                });
+            }
         }
     }
 
@@ -1965,13 +2012,13 @@ impl LhsEditor {
         lhs_multibuffer: &mut MultiBuffer,
         diff: Entity<BufferDiff>,
         lhs_cx: &mut Context<MultiBuffer>,
-    ) -> Vec<(ExcerptId, ExcerptId)> {
+    ) -> Option<(Vec<ExcerptId>, Vec<Vec<ExcerptId>>)> {
         let rhs_excerpt_ids: Vec<ExcerptId> =
             rhs_multibuffer.excerpts_for_path(&path_key).collect();
 
         let Some(excerpt_id) = rhs_multibuffer.excerpts_for_path(&path_key).next() else {
             lhs_multibuffer.remove_excerpts_for_path(path_key, lhs_cx);
-            return Vec::new();
+            return None;
         };
 
         let rhs_multibuffer_snapshot = rhs_multibuffer.snapshot(lhs_cx);
@@ -2007,14 +2054,14 @@ impl LhsEditor {
             })
             .collect();
 
-        let (ids, _) = lhs_multibuffer.update_path_excerpts(
-            path_key.clone(),
+        let lhs_result = lhs_multibuffer.update_path_excerpts(
+            path_key,
             base_text_buffer.clone(),
             &base_text_buffer_snapshot,
             new,
             lhs_cx,
         );
-        if !ids.is_empty()
+        if !lhs_result.excerpt_ids.is_empty()
             && lhs_multibuffer
                 .diff_for(base_text_buffer.read(lhs_cx).remote_id())
                 .is_none_or(|old_diff| old_diff.entity_id() != diff.entity_id())
@@ -2022,12 +2069,32 @@ impl LhsEditor {
             lhs_multibuffer.add_inverted_diff(diff, lhs_cx);
         }
 
-        let lhs_excerpt_ids: Vec<ExcerptId> =
-            lhs_multibuffer.excerpts_for_path(&path_key).collect();
+        let rhs_merge_groups: Vec<Vec<ExcerptId>> = {
+            let mut groups = Vec::new();
+            let mut current_group = Vec::new();
+            let mut last_id = None;
+
+            for (i, &lhs_id) in lhs_result.excerpt_ids.iter().enumerate() {
+                if last_id == Some(lhs_id) {
+                    current_group.push(rhs_excerpt_ids[i]);
+                } else {
+                    if !current_group.is_empty() {
+                        groups.push(current_group);
+                    }
+                    current_group = vec![rhs_excerpt_ids[i]];
+                    last_id = Some(lhs_id);
+                }
+            }
+            if !current_group.is_empty() {
+                groups.push(current_group);
+            }
+            groups
+        };
 
-        debug_assert_eq!(rhs_excerpt_ids.len(), lhs_excerpt_ids.len());
+        let deduplicated_lhs_ids: Vec<ExcerptId> =
+            lhs_result.excerpt_ids.iter().dedup().copied().collect();
 
-        lhs_excerpt_ids.into_iter().zip(rhs_excerpt_ids).collect()
+        Some((deduplicated_lhs_ids, rhs_merge_groups))
     }
 
     fn sync_path_excerpts(
@@ -2038,7 +2105,7 @@ impl LhsEditor {
         diff: Entity<BufferDiff>,
         rhs_display_map: &Entity<DisplayMap>,
         lhs_cx: &mut Context<MultiBuffer>,
-    ) {
+    ) -> Option<(Vec<ExcerptId>, Vec<Vec<ExcerptId>>)> {
         let old_lhs_excerpt_ids: Vec<ExcerptId> =
             lhs_multibuffer.excerpts_for_path(&path_key).collect();
 
@@ -2048,25 +2115,13 @@ impl LhsEditor {
             });
         }
 
-        let mappings = Self::update_path_excerpts_from_rhs(
+        Self::update_path_excerpts_from_rhs(
             path_key,
             rhs_multibuffer,
             lhs_multibuffer,
-            diff.clone(),
+            diff,
             lhs_cx,
-        );
-
-        let lhs_buffer_id = diff.read(lhs_cx).base_text(lhs_cx).remote_id();
-        let rhs_buffer_id = diff.read(lhs_cx).buffer_id;
-
-        if let Some(companion) = rhs_display_map.read(lhs_cx).companion().cloned() {
-            companion.update(lhs_cx, |c, _| {
-                for (lhs, rhs) in mappings {
-                    c.add_excerpt_mapping(lhs, rhs);
-                }
-                c.add_buffer_mapping(lhs_buffer_id, rhs_buffer_id);
-            });
-        }
+        )
     }
 }
 
@@ -5455,6 +5510,69 @@ mod tests {
         );
     }
 
+    #[gpui::test]
+    async fn test_edit_spanning_excerpt_boundaries_then_resplit(cx: &mut gpui::TestAppContext) {
+        use rope::Point;
+        use unindent::Unindent as _;
+
+        let (editor, mut cx) = init_test(cx, SoftWrap::None, DiffViewStyle::Split).await;
+
+        let base_text = "
+            aaa
+            bbb
+            ccc
+            ddd
+            eee
+            fff
+            ggg
+            hhh
+            iii
+            jjj
+            kkk
+            lll
+        "
+        .unindent();
+        let current_text = base_text.clone();
+
+        let (buffer, diff) = buffer_with_diff(&base_text, &current_text, &mut cx);
+
+        editor.update(cx, |editor, cx| {
+            let path = PathKey::for_buffer(&buffer, cx);
+            editor.set_excerpts_for_path(
+                path,
+                buffer.clone(),
+                vec![
+                    Point::new(0, 0)..Point::new(3, 3),
+                    Point::new(5, 0)..Point::new(8, 3),
+                    Point::new(10, 0)..Point::new(11, 3),
+                ],
+                0,
+                diff.clone(),
+                cx,
+            );
+        });
+
+        cx.run_until_parked();
+
+        buffer.update(cx, |buffer, cx| {
+            buffer.edit([(Point::new(1, 0)..Point::new(10, 0), "")], None, cx);
+        });
+
+        cx.run_until_parked();
+
+        editor.update_in(cx, |splittable_editor, window, cx| {
+            splittable_editor.unsplit(window, cx);
+        });
+
+        cx.run_until_parked();
+
+        editor.update_in(cx, |splittable_editor, window, cx| {
+            splittable_editor.split(window, cx);
+        });
+
+        cx.run_until_parked();
+    }
+
     #[gpui::test]
     async fn test_range_folds_removed_on_split(cx: &mut gpui::TestAppContext) {
         use rope::Point;

crates/multi_buffer/src/multi_buffer.rs

@@ -57,7 +57,7 @@ use theme::SyntaxTheme;
 use util::post_inc;
 use ztracing::instrument;
 
-pub use self::path_key::PathKey;
+pub use self::path_key::{PathExcerptInsertResult, PathKey};
 
 pub static EXCERPT_CONTEXT_LINES: OnceLock<fn(&App) -> u32> = OnceLock::new();
 
@@ -2164,6 +2164,35 @@ impl MultiBuffer {
         None
     }
 
+    pub fn merge_excerpts(
+        &mut self,
+        excerpt_ids: &[ExcerptId],
+        cx: &mut Context<Self>,
+    ) -> ExcerptId {
+        debug_assert!(!excerpt_ids.is_empty());
+        if excerpt_ids.len() == 1 {
+            return excerpt_ids[0];
+        }
+
+        let snapshot = self.snapshot(cx);
+
+        let first_range = snapshot
+            .context_range_for_excerpt(excerpt_ids[0])
+            .expect("first excerpt must exist");
+        let last_range = snapshot
+            .context_range_for_excerpt(*excerpt_ids.last().unwrap())
+            .expect("last excerpt must exist");
+
+        let union_range = first_range.start..last_range.end;
+
+        drop(snapshot);
+
+        self.resize_excerpt(excerpt_ids[0], union_range, cx);
+        self.remove_excerpts(excerpt_ids[1..].iter().copied(), cx);
+
+        excerpt_ids[0]
+    }
+
     pub fn remove_excerpts(
         &mut self,
         excerpt_ids: impl IntoIterator<Item = ExcerptId>,

crates/multi_buffer/src/path_key.rs

@@ -13,6 +13,12 @@ use crate::{
     Anchor, ExcerptId, ExcerptRange, ExpandExcerptDirection, MultiBuffer, build_excerpt_ranges,
 };
 
+#[derive(Debug, Clone)]
+pub struct PathExcerptInsertResult {
+    pub excerpt_ids: Vec<ExcerptId>,
+    pub added_new_excerpt: bool,
+}
+
 #[derive(PartialEq, Eq, Ord, PartialOrd, Clone, Hash, Debug)]
 pub struct PathKey {
     // Used by the derived PartialOrd & Ord
@@ -268,12 +274,15 @@ impl MultiBuffer {
         counts: Vec<usize>,
         cx: &mut Context<Self>,
     ) -> (Vec<Range<Anchor>>, bool) {
-        let (excerpt_ids, added_a_new_excerpt) =
-            self.update_path_excerpts(path, buffer, buffer_snapshot, new, cx);
+        let insert_result = self.update_path_excerpts(path, buffer, buffer_snapshot, new, cx);
 
         let mut result = Vec::new();
         let mut ranges = ranges.into_iter();
-        for (excerpt_id, range_count) in excerpt_ids.into_iter().zip(counts.into_iter()) {
+        for (excerpt_id, range_count) in insert_result
+            .excerpt_ids
+            .into_iter()
+            .zip(counts.into_iter())
+        {
             for range in ranges.by_ref().take(range_count) {
                 let range = Anchor::range_in_buffer(
                     excerpt_id,
@@ -283,7 +292,7 @@ impl MultiBuffer {
                 result.push(range)
             }
         }
-        (result, added_a_new_excerpt)
+        (result, insert_result.added_new_excerpt)
     }
 
     pub fn update_path_excerpts(
@@ -293,7 +302,7 @@ impl MultiBuffer {
         buffer_snapshot: &BufferSnapshot,
         new: Vec<ExcerptRange<Point>>,
         cx: &mut Context<Self>,
-    ) -> (Vec<ExcerptId>, bool) {
+    ) -> PathExcerptInsertResult {
         let mut insert_after = self
             .excerpts_by_path
             .range(..path.clone())
@@ -443,6 +452,9 @@ impl MultiBuffer {
             self.excerpts_by_path.insert(path, excerpt_ids);
         }
 
-        (excerpt_ids, added_a_new_excerpt)
+        PathExcerptInsertResult {
+            excerpt_ids,
+            added_new_excerpt: added_a_new_excerpt,
+        }
     }
 }