git: Fix a panic when updating the split diff (#54352)

Cole Miller created

Self-Review Checklist:

- [x] I've reviewed my own diff for quality, security, and reliability
- [x] Unsafe blocks (if any) have justifying comments
- [x] The content is consistent with the [UI/UX
checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist)
- [ ] Tests cover the new/changed behavior
- [x] Performance impact has been considered and is acceptable

Release Notes:

- Fixed a panic when using the split diff view.

Change summary

crates/editor/src/display_map.rs           | 77 ++++--------------
crates/editor/src/display_map/block_map.rs | 25 ++---
crates/editor/src/split.rs                 | 95 +++++++++--------------
crates/git_ui/src/project_diff.rs          |  1 
4 files changed, 67 insertions(+), 131 deletions(-)

Detailed changes

crates/editor/src/display_map.rs 🔗

@@ -194,12 +194,6 @@ pub struct CompanionExcerptPatch {
     pub target_excerpt_range: Range<MultiBufferPoint>,
 }
 
-pub type ConvertMultiBufferRows = fn(
-    &MultiBufferSnapshot,
-    &MultiBufferSnapshot,
-    Range<MultiBufferPoint>,
-) -> Vec<CompanionExcerptPatch>;
-
 /// Decides how text in a [`MultiBuffer`] should be displayed in a buffer, handling inlay hints,
 /// folding, hard tabs, soft wrapping, custom blocks (like diagnostics), and highlighting.
 ///
@@ -237,26 +231,14 @@ pub struct DisplayMap {
 
 pub(crate) struct Companion {
     rhs_display_map_id: EntityId,
-    rhs_buffer_to_lhs_buffer: HashMap<BufferId, BufferId>,
-    lhs_buffer_to_rhs_buffer: HashMap<BufferId, BufferId>,
-    rhs_rows_to_lhs_rows: ConvertMultiBufferRows,
-    lhs_rows_to_rhs_rows: ConvertMultiBufferRows,
     rhs_custom_block_to_balancing_block: RefCell<HashMap<CustomBlockId, CustomBlockId>>,
     lhs_custom_block_to_balancing_block: RefCell<HashMap<CustomBlockId, CustomBlockId>>,
 }
 
 impl Companion {
-    pub(crate) fn new(
-        rhs_display_map_id: EntityId,
-        rhs_rows_to_lhs_rows: ConvertMultiBufferRows,
-        lhs_rows_to_rhs_rows: ConvertMultiBufferRows,
-    ) -> Self {
+    pub(crate) fn new(rhs_display_map_id: EntityId) -> Self {
         Self {
             rhs_display_map_id,
-            rhs_buffer_to_lhs_buffer: Default::default(),
-            lhs_buffer_to_rhs_buffer: Default::default(),
-            rhs_rows_to_lhs_rows,
-            lhs_rows_to_rhs_rows,
             rhs_custom_block_to_balancing_block: Default::default(),
             lhs_custom_block_to_balancing_block: Default::default(),
         }
@@ -284,12 +266,11 @@ impl Companion {
         our_snapshot: &MultiBufferSnapshot,
         bounds: Range<MultiBufferPoint>,
     ) -> Vec<CompanionExcerptPatch> {
-        let convert_fn = if self.is_rhs(display_map_id) {
-            self.rhs_rows_to_lhs_rows
+        if self.is_rhs(display_map_id) {
+            crate::split::patches_for_rhs_range(companion_snapshot, our_snapshot, bounds)
         } else {
-            self.lhs_rows_to_rhs_rows
-        };
-        convert_fn(companion_snapshot, our_snapshot, bounds)
+            crate::split::patches_for_lhs_range(companion_snapshot, our_snapshot, bounds)
+        }
     }
 
     pub(crate) fn convert_point_from_companion(
@@ -299,17 +280,13 @@ impl Companion {
         companion_snapshot: &MultiBufferSnapshot,
         point: MultiBufferPoint,
     ) -> Range<MultiBufferPoint> {
-        let convert_fn = if self.is_rhs(display_map_id) {
-            self.lhs_rows_to_rhs_rows
+        let patches = if self.is_rhs(display_map_id) {
+            crate::split::patches_for_lhs_range(our_snapshot, companion_snapshot, point..point)
         } else {
-            self.rhs_rows_to_lhs_rows
+            crate::split::patches_for_rhs_range(our_snapshot, companion_snapshot, point..point)
         };
 
-        let excerpt = convert_fn(our_snapshot, companion_snapshot, point..point)
-            .into_iter()
-            .next();
-
-        let Some(excerpt) = excerpt else {
+        let Some(excerpt) = patches.into_iter().next() else {
             return Point::zero()..our_snapshot.max_point();
         };
         excerpt.patch.edit_for_old_position(point).new
@@ -322,38 +299,17 @@ impl Companion {
         companion_snapshot: &MultiBufferSnapshot,
         point: MultiBufferPoint,
     ) -> Range<MultiBufferPoint> {
-        let convert_fn = if self.is_rhs(display_map_id) {
-            self.rhs_rows_to_lhs_rows
+        let patches = if self.is_rhs(display_map_id) {
+            crate::split::patches_for_rhs_range(companion_snapshot, our_snapshot, point..point)
         } else {
-            self.lhs_rows_to_rhs_rows
+            crate::split::patches_for_lhs_range(companion_snapshot, our_snapshot, point..point)
         };
 
-        let excerpt = convert_fn(companion_snapshot, our_snapshot, point..point)
-            .into_iter()
-            .next();
-
-        let Some(excerpt) = excerpt else {
+        let Some(excerpt) = patches.into_iter().next() else {
             return Point::zero()..companion_snapshot.max_point();
         };
         excerpt.patch.edit_for_old_position(point).new
     }
-
-    fn buffer_to_companion_buffer(&self, display_map_id: EntityId) -> &HashMap<BufferId, BufferId> {
-        if self.is_rhs(display_map_id) {
-            &self.rhs_buffer_to_lhs_buffer
-        } else {
-            &self.lhs_buffer_to_rhs_buffer
-        }
-    }
-
-    pub(crate) fn lhs_to_rhs_buffer(&self, lhs_buffer_id: BufferId) -> Option<BufferId> {
-        self.lhs_buffer_to_rhs_buffer.get(&lhs_buffer_id).copied()
-    }
-
-    pub(crate) fn add_buffer_mapping(&mut self, lhs_buffer: BufferId, rhs_buffer: BufferId) {
-        self.lhs_buffer_to_rhs_buffer.insert(lhs_buffer, rhs_buffer);
-        self.rhs_buffer_to_lhs_buffer.insert(rhs_buffer, lhs_buffer);
-    }
 }
 
 #[derive(Default, Debug)]
@@ -514,9 +470,12 @@ impl DisplayMap {
             // entries: the block map doesn't remove buffers from
             // `folded_buffers` when they leave the multibuffer, so we
             // unfold any RHS buffers whose companion mapping is missing.
+            let rhs_snapshot = self.buffer.read(cx).snapshot(cx);
             let mut buffers_to_unfold = Vec::new();
             for my_buffer in self.folded_buffers() {
-                let their_buffer = companion.read(cx).rhs_buffer_to_lhs_buffer.get(my_buffer);
+                let their_buffer = rhs_snapshot
+                    .diff_for_buffer_id(*my_buffer)
+                    .map(|diff| diff.base_text().remote_id());
 
                 let Some(their_buffer) = their_buffer else {
                     buffers_to_unfold.push(*my_buffer);
@@ -526,7 +485,7 @@ impl DisplayMap {
                 companion_display_map
                     .block_map
                     .folded_buffers
-                    .insert(*their_buffer);
+                    .insert(their_buffer);
             }
             for buffer_id in buffers_to_unfold {
                 self.block_map.folded_buffers.remove(&buffer_id);

crates/editor/src/display_map/block_map.rs 🔗

@@ -2056,13 +2056,15 @@ impl BlockMapWriter<'_> {
             if let Some(companion) = &self.companion
                 && companion.inverse.is_some()
             {
-                companion_buffer_ids.extend(
-                    companion
-                        .companion
-                        .buffer_to_companion_buffer(companion.display_map_id)
-                        .get(&buffer_id)
-                        .copied(),
-                )
+                if let Some(diff) = multi_buffer_snapshot.diff_for_buffer_id(buffer_id) {
+                    let companion_buffer_id =
+                        if companion.companion.is_rhs(companion.display_map_id) {
+                            diff.base_text().remote_id()
+                        } else {
+                            diff.buffer_id()
+                        };
+                    companion_buffer_ids.insert(companion_buffer_id);
+                }
             }
         }
         ranges.sort_unstable_by_key(|range| range.start);
@@ -2869,7 +2871,6 @@ mod tests {
         display_map::{
             Companion, fold_map::FoldMap, inlay_map::InlayMap, tab_map::TabMap, wrap_map::WrapMap,
         },
-        split::{convert_lhs_rows_to_rhs, convert_rhs_rows_to_lhs},
         test::test_font,
     };
     use buffer_diff::BufferDiff;
@@ -4680,13 +4681,7 @@ mod tests {
 
         let rhs_entity_id = rhs_multibuffer.entity_id();
 
-        let companion = cx.new(|_| {
-            Companion::new(
-                rhs_entity_id,
-                convert_rhs_rows_to_lhs,
-                convert_lhs_rows_to_rhs,
-            )
-        });
+        let companion = cx.new(|_| Companion::new(rhs_entity_id));
 
         let rhs_edits = Patch::new(vec![text::Edit {
             old: WrapRow(0)..rhs_wrap_snapshot.max_point().row(),

crates/editor/src/split.rs 🔗

@@ -43,7 +43,7 @@ use crate::{
 };
 use zed_actions::assistant::InlineAssist;
 
-pub(crate) fn convert_lhs_rows_to_rhs(
+pub(crate) fn patches_for_lhs_range(
     rhs_snapshot: &MultiBufferSnapshot,
     lhs_snapshot: &MultiBufferSnapshot,
     lhs_bounds: Range<MultiBufferPoint>,
@@ -56,7 +56,7 @@ pub(crate) fn convert_lhs_rows_to_rhs(
     )
 }
 
-pub(crate) fn convert_rhs_rows_to_lhs(
+pub(crate) fn patches_for_rhs_range(
     lhs_snapshot: &MultiBufferSnapshot,
     rhs_snapshot: &MultiBufferSnapshot,
     rhs_bounds: Range<MultiBufferPoint>,
@@ -69,7 +69,7 @@ pub(crate) fn convert_rhs_rows_to_lhs(
     )
 }
 
-fn rhs_range_to_base_text_range(
+fn buffer_range_to_base_text_range(
     rhs_range: &Range<Point>,
     diff_snapshot: &BufferDiffSnapshot,
     rhs_buffer_snapshot: &text::BufferSnapshot,
@@ -89,19 +89,19 @@ fn translate_lhs_selections_to_rhs(
     splittable: &SplittableEditor,
     cx: &App,
 ) -> HashMap<Entity<Buffer>, (Vec<Range<BufferOffset>>, Option<u32>)> {
-    let rhs_display_map = splittable.rhs_editor.read(cx).display_map.read(cx);
-    let Some(companion) = rhs_display_map.companion() else {
+    let Some(lhs) = &splittable.lhs else {
         return HashMap::default();
     };
-    let companion = companion.read(cx);
+    let lhs_snapshot = lhs.multibuffer.read(cx).snapshot(cx);
 
     let mut translated: HashMap<Entity<Buffer>, (Vec<Range<BufferOffset>>, Option<u32>)> =
         HashMap::default();
 
     for (lhs_buffer_id, (ranges, scroll_offset)) in selections_by_buffer {
-        let Some(rhs_buffer_id) = companion.lhs_to_rhs_buffer(*lhs_buffer_id) else {
+        let Some(diff) = lhs_snapshot.diff_for_buffer_id(*lhs_buffer_id) else {
             continue;
         };
+        let rhs_buffer_id = diff.buffer_id();
 
         let Some(rhs_buffer) = splittable
             .rhs_editor
@@ -155,19 +155,19 @@ fn translate_lhs_hunks_to_rhs(
     splittable: &SplittableEditor,
     cx: &App,
 ) -> Vec<MultiBufferDiffHunk> {
-    let rhs_display_map = splittable.rhs_editor.read(cx).display_map.read(cx);
-    let Some(companion) = rhs_display_map.companion() else {
+    let Some(lhs) = &splittable.lhs else {
         return vec![];
     };
-    let companion = companion.read(cx);
+    let lhs_snapshot = lhs.multibuffer.read(cx).snapshot(cx);
     let rhs_snapshot = splittable.rhs_multibuffer.read(cx).snapshot(cx);
     let rhs_hunks: Vec<MultiBufferDiffHunk> = rhs_snapshot.diff_hunks().collect();
 
     let mut translated = Vec::new();
     for lhs_hunk in lhs_hunks {
-        let Some(rhs_buffer_id) = companion.lhs_to_rhs_buffer(lhs_hunk.buffer_id) else {
+        let Some(diff) = lhs_snapshot.diff_for_buffer_id(lhs_hunk.buffer_id) else {
             continue;
         };
+        let rhs_buffer_id = diff.buffer_id();
         if let Some(rhs_hunk) = rhs_hunks.iter().find(|rhs_hunk| {
             rhs_hunk.buffer_id == rhs_buffer_id
                 && rhs_hunk.diff_base_byte_range == lhs_hunk.diff_base_byte_range
@@ -207,9 +207,12 @@ where
             return;
         };
 
-        let diff = source_snapshot
-            .diff_for_buffer_id(first.source_buffer_snapshot.remote_id())
-            .expect("buffer with no diff when creating patches");
+        let Some(diff) =
+            source_snapshot.diff_for_buffer_id(first.source_buffer_snapshot.remote_id())
+        else {
+            pending.clear();
+            return;
+        };
         let source_is_lhs =
             first.source_buffer_snapshot.remote_id() == diff.base_text().remote_id();
         let target_buffer_id = if source_is_lhs {
@@ -217,9 +220,10 @@ where
         } else {
             diff.base_text().remote_id()
         };
-        let target_buffer = target_snapshot
-            .buffer_for_id(target_buffer_id)
-            .expect("missing corresponding buffer");
+        let Some(target_buffer) = target_snapshot.buffer_for_id(target_buffer_id) else {
+            pending.clear();
+            return;
+        };
         let rhs_buffer = if source_is_lhs {
             target_buffer
         } else {
@@ -412,7 +416,6 @@ pub struct SplittableEditor {
 struct LhsEditor {
     multibuffer: Entity<MultiBuffer>,
     editor: Entity<Editor>,
-    companion: Entity<Companion>,
     was_last_focused: bool,
     _subscriptions: Vec<Subscription>,
 }
@@ -485,6 +488,7 @@ impl SplittableEditor {
             editor.disable_runnables();
             editor.disable_inline_diagnostics();
             editor.set_minimap_visibility(crate::MinimapVisibility::Disabled, window, cx);
+            editor.start_temporary_diff_override();
             editor
         });
         // TODO(split-diff) we might want to tag editor events with whether they came from rhs/lhs
@@ -603,12 +607,10 @@ impl SplittableEditor {
                             .filter_map(|anchor| {
                                 let (anchor, lhs_buffer) =
                                     lhs_snapshot.anchor_to_buffer_anchor(*anchor)?;
-                                let rhs_buffer_id =
-                                    lhs.companion.read(cx).lhs_to_rhs_buffer(anchor.buffer_id)?;
+                                let diff = lhs_snapshot.diff_for_buffer_id(anchor.buffer_id)?;
+                                let rhs_buffer_id = diff.buffer_id();
                                 let rhs_buffer = rhs_snapshot.buffer_for_id(rhs_buffer_id)?;
-                                let diff = this.rhs_multibuffer.read(cx).diff_for(rhs_buffer_id)?;
-                                let diff_snapshot = diff.read(cx).snapshot(cx);
-                                let rhs_point = diff_snapshot.base_text_point_to_buffer_point(
+                                let rhs_point = diff.base_text_point_to_buffer_point(
                                     anchor.to_point(&lhs_buffer),
                                     &rhs_buffer,
                                 );
@@ -697,18 +699,11 @@ impl SplittableEditor {
         let rhs_display_map = self.rhs_editor.read(cx).display_map.clone();
         let lhs_display_map = lhs_editor.read(cx).display_map.clone();
         let rhs_display_map_id = rhs_display_map.entity_id();
-        let companion = cx.new(|_| {
-            Companion::new(
-                rhs_display_map_id,
-                convert_rhs_rows_to_lhs,
-                convert_lhs_rows_to_rhs,
-            )
-        });
+        let companion = cx.new(|_| Companion::new(rhs_display_map_id));
         let lhs = LhsEditor {
             editor: lhs_editor,
             multibuffer: lhs_multibuffer,
             was_last_focused: false,
-            companion: companion.clone(),
             _subscriptions: subscriptions,
         };
 
@@ -734,7 +729,7 @@ impl SplittableEditor {
 
         self.lhs = Some(lhs);
 
-        self.sync_lhs_for_paths(all_paths, &companion, cx);
+        self.sync_lhs_for_paths(all_paths, cx);
 
         rhs_display_map.update(cx, |dm, cx| {
             dm.set_companion(Some((lhs_display_map, companion.clone())), cx);
@@ -1048,7 +1043,7 @@ impl SplittableEditor {
         cx: &mut Context<Self>,
     ) -> bool {
         let has_ranges = ranges.clone().into_iter().next().is_some();
-        let Some(companion) = self.companion(cx) else {
+        if self.lhs.is_none() {
             return self.rhs_multibuffer.update(cx, |rhs_multibuffer, cx| {
                 let added_a_new_excerpt = rhs_multibuffer.update_excerpts_for_path(
                     path,
@@ -1066,7 +1061,7 @@ impl SplittableEditor {
                 }
                 added_a_new_excerpt
             });
-        };
+        }
 
         let result = self.rhs_multibuffer.update(cx, |rhs_multibuffer, cx| {
             let added_a_new_excerpt = rhs_multibuffer.update_excerpts_for_path(
@@ -1086,7 +1081,7 @@ impl SplittableEditor {
             added_a_new_excerpt
         });
 
-        self.sync_lhs_for_paths(vec![(path, diff)], &companion, cx);
+        self.sync_lhs_for_paths(vec![(path, diff)], cx);
         result
     }
 
@@ -1097,12 +1092,12 @@ impl SplittableEditor {
         direction: ExpandExcerptDirection,
         cx: &mut Context<Self>,
     ) {
-        let Some(companion) = self.companion(cx) else {
+        if self.lhs.is_none() {
             self.rhs_multibuffer.update(cx, |rhs_multibuffer, cx| {
                 rhs_multibuffer.expand_excerpts(excerpt_anchors, lines, direction, cx);
             });
             return;
-        };
+        }
 
         let paths: Vec<_> = self.rhs_multibuffer.update(cx, |rhs_multibuffer, cx| {
             let snapshot = rhs_multibuffer.snapshot(cx);
@@ -1121,7 +1116,7 @@ impl SplittableEditor {
             paths
         });
 
-        self.sync_lhs_for_paths(paths, &companion, cx);
+        self.sync_lhs_for_paths(paths, cx);
     }
 
     pub fn remove_excerpts_for_path(&mut self, path: PathKey, cx: &mut Context<Self>) {
@@ -1147,18 +1142,9 @@ impl SplittableEditor {
         Some(&self.rhs_editor)
     }
 
-    fn companion(&self, cx: &App) -> Option<Entity<Companion>> {
-        if self.lhs.is_none() {
-            return None;
-        }
-        let rhs_display_map = self.rhs_editor.read(cx).display_map.clone();
-        rhs_display_map.read(cx).companion().cloned()
-    }
-
     fn sync_lhs_for_paths(
         &self,
         paths: Vec<(PathKey, Entity<BufferDiff>)>,
-        companion: &Entity<Companion>,
         cx: &mut Context<Self>,
     ) {
         let Some(lhs) = &self.lhs else { return };
@@ -1186,7 +1172,7 @@ impl SplittableEditor {
                 for info in rhs_multibuffer_snapshot.excerpts_for_buffer(main_buffer_id) {
                     have_excerpt = true;
                     let rhs_context = info.context.to_point(&main_buffer_snapshot);
-                    let lhs_context = rhs_range_to_base_text_range(
+                    let lhs_context = buffer_range_to_base_text_range(
                         &rhs_context,
                         &diff_snapshot,
                         &main_buffer_snapshot,
@@ -1242,12 +1228,6 @@ impl SplittableEditor {
                         cx,
                     );
                 }
-
-                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, _| {
-                    c.add_buffer_mapping(lhs_buffer_id, rhs_buffer_id);
-                });
             }
         });
     }
@@ -1689,8 +1669,11 @@ impl SplittableEditor {
                     .unwrap();
                 let lhs_range = lhs_excerpt.context.to_point(&lhs_buffer_snapshot);
                 let rhs_range = rhs_excerpt.context.to_point(&rhs_buffer_snapshot);
-                let expected_lhs_range =
-                    rhs_range_to_base_text_range(&rhs_range, &diff_snapshot, &rhs_buffer_snapshot);
+                let expected_lhs_range = buffer_range_to_base_text_range(
+                    &rhs_range,
+                    &diff_snapshot,
+                    &rhs_buffer_snapshot,
+                );
                 assert_eq!(
                     lhs_range, expected_lhs_range,
                     "corresponding lhs excerpt should have a matching range"

crates/git_ui/src/project_diff.rs 🔗

@@ -378,7 +378,6 @@ impl ProjectDiff {
                         editor.register_addon(BranchDiffAddon {
                             branch_diff: branch_diff.clone(),
                         });
-                        editor.start_temporary_diff_override();
                     }
                 }
             });