git: Correct logic for updating companion excerpt IDs mapping (#48906)

Cole Miller created 2 months ago

We try to remove old excerpt IDs from the companion when mutating
excerpts for a path, but we were incorrectly doing this using the
excerpt IDs for the right-hand side multibuffer _after_ any excerpt
mutations had occurred, causing stale excerpt IDs to accumulate.

Release Notes:

- N/A

Change summary

crates/editor/src/split.rs | 201 +++++++++++++++++++++------------------
1 file changed, 108 insertions(+), 93 deletions(-)

Detailed changes

crates/editor/src/split.rs

@@ -1002,41 +1002,33 @@ impl SplittableEditor {
         cx: &mut Context<Self>,
     ) -> (Vec<Range<Anchor>>, bool) {
         let rhs_display_map = self.rhs_editor.read(cx).display_map.clone();
-        let lhs = &mut self.lhs;
-        let (anchors, added_a_new_excerpt) =
-            self.rhs_multibuffer.update(cx, |rhs_multibuffer, cx| {
-                let (anchors, added_a_new_excerpt) = rhs_multibuffer.set_excerpts_for_path(
-                    path.clone(),
-                    buffer.clone(),
-                    ranges,
-                    context_line_count,
-                    cx,
-                );
-                if !anchors.is_empty()
-                    && rhs_multibuffer
-                        .diff_for(buffer.read(cx).remote_id())
-                        .is_none_or(|old_diff| old_diff.entity_id() != diff.entity_id())
-                {
-                    rhs_multibuffer.add_diff(diff.clone(), cx);
-                }
-
-                if let Some(lhs) = lhs {
-                    lhs.multibuffer.update(cx, |lhs_multibuffer, lhs_cx| {
-                        LhsEditor::sync_path_excerpts(
-                            path,
-                            rhs_multibuffer,
-                            lhs_multibuffer,
-                            diff,
-                            &rhs_display_map,
-                            lhs_cx,
-                        )
-                    })
-                }
-
-                (anchors, added_a_new_excerpt)
-            });
-
-        (anchors, added_a_new_excerpt)
+        let lhs = self.lhs.as_ref();
+        self.rhs_multibuffer.update(cx, |rhs_multibuffer, cx| {
+            mutate_excerpts_for_paths(
+                rhs_multibuffer,
+                lhs,
+                &rhs_display_map,
+                vec![(path.clone(), diff.clone())],
+                cx,
+                |rhs_multibuffer, cx| {
+                    let (anchors, added_a_new_excerpt) = rhs_multibuffer.set_excerpts_for_path(
+                        path.clone(),
+                        buffer.clone(),
+                        ranges,
+                        context_line_count,
+                        cx,
+                    );
+                    if !anchors.is_empty()
+                        && rhs_multibuffer
+                            .diff_for(buffer.read(cx).remote_id())
+                            .is_none_or(|old_diff| old_diff.entity_id() != diff.entity_id())
+                    {
+                        rhs_multibuffer.add_diff(diff.clone(), cx);
+                    }
+                    (anchors, added_a_new_excerpt)
+                },
+            )
+        })
     }
 
     fn expand_excerpts(
@@ -1047,37 +1039,35 @@ impl SplittableEditor {
         cx: &mut Context<Self>,
     ) {
         let rhs_display_map = self.rhs_editor.read(cx).display_map.clone();
-
-        let lhs = &mut self.lhs;
-        let lhs_multibuffer = lhs.as_ref().map(|l| l.multibuffer.clone());
+        let lhs = self.lhs.as_ref();
         self.rhs_multibuffer.update(cx, |rhs_multibuffer, cx| {
-            if let Some(lhs_multibuffer) = lhs_multibuffer {
+            if lhs.is_some() {
                 let snapshot = rhs_multibuffer.snapshot(cx);
-                let affected_paths: HashMap<_, _> = excerpt_ids
+                let paths_with_diffs: Vec<_> = excerpt_ids
                     .clone()
-                    .map(|excerpt_id| {
-                        let path = rhs_multibuffer.path_for_excerpt(excerpt_id).unwrap();
-                        let buffer = snapshot.buffer_for_excerpt(excerpt_id).unwrap();
-                        let diff = rhs_multibuffer.diff_for(buffer.remote_id()).unwrap();
-                        (path, diff)
+                    .filter_map(|excerpt_id| {
+                        let path = rhs_multibuffer.path_for_excerpt(excerpt_id)?;
+                        let buffer = snapshot.buffer_for_excerpt(excerpt_id)?;
+                        let diff = rhs_multibuffer.diff_for(buffer.remote_id())?;
+                        Some((path, diff))
                     })
+                    .collect::<HashMap<_, _>>()
+                    .into_iter()
                     .collect();
-                rhs_multibuffer.expand_excerpts(excerpt_ids.clone(), lines, direction, cx);
-                for (path, diff) in affected_paths {
-                    lhs_multibuffer.update(cx, |lhs_multibuffer, lhs_cx| {
-                        LhsEditor::sync_path_excerpts(
-                            path,
-                            rhs_multibuffer,
-                            lhs_multibuffer,
-                            diff,
-                            &rhs_display_map,
-                            lhs_cx,
-                        );
-                    });
-                }
+
+                mutate_excerpts_for_paths(
+                    rhs_multibuffer,
+                    lhs,
+                    &rhs_display_map,
+                    paths_with_diffs,
+                    cx,
+                    |rhs_multibuffer, cx| {
+                        rhs_multibuffer.expand_excerpts(excerpt_ids.clone(), lines, direction, cx);
+                    },
+                );
             } else {
-                rhs_multibuffer.expand_excerpts(excerpt_ids.clone(), lines, direction, cx);
-            };
+                rhs_multibuffer.expand_excerpts(excerpt_ids, lines, direction, cx);
+            }
         });
     }
 
@@ -1086,15 +1076,17 @@ impl SplittableEditor {
 
         if let Some(lhs) = &self.lhs {
             self.rhs_multibuffer.update(cx, |rhs_multibuffer, cx| {
-                lhs.multibuffer.update(cx, |lhs_multibuffer, cx| {
-                    LhsEditor::remove_mappings_for_path(
-                        &path,
-                        rhs_multibuffer,
-                        lhs_multibuffer,
-                        &rhs_display_map,
-                        cx,
-                    );
-                });
+                let rhs_excerpt_ids: Vec<ExcerptId> =
+                    rhs_multibuffer.excerpts_for_path(&path).collect();
+                let lhs_excerpt_ids: Vec<ExcerptId> =
+                    lhs.multibuffer.read(cx).excerpts_for_path(&path).collect();
+
+                if let Some(companion) = rhs_display_map.read(cx).companion().cloned() {
+                    companion.update(cx, |c, _| {
+                        c.remove_excerpt_mappings(lhs_excerpt_ids, rhs_excerpt_ids);
+                    });
+                }
+
                 rhs_multibuffer.remove_excerpts_for_path(path.clone(), cx);
             });
             lhs.multibuffer.update(cx, |lhs_multibuffer, cx| {
@@ -1938,6 +1930,44 @@ impl Render for SplittableEditor {
     }
 }
 
+fn mutate_excerpts_for_paths<R>(
+    rhs_multibuffer: &mut MultiBuffer,
+    lhs: Option<&LhsEditor>,
+    rhs_display_map: &Entity<DisplayMap>,
+    paths_with_diffs: Vec<(PathKey, Entity<BufferDiff>)>,
+    cx: &mut Context<MultiBuffer>,
+    mutate: impl FnOnce(&mut MultiBuffer, &mut Context<MultiBuffer>) -> R,
+) -> R {
+    let old_rhs_ids: Vec<_> = paths_with_diffs
+        .iter()
+        .map(|(path, _)| {
+            rhs_multibuffer
+                .excerpts_for_path(path)
+                .collect::<Vec<ExcerptId>>()
+        })
+        .collect();
+
+    let result = mutate(rhs_multibuffer, cx);
+
+    if let Some(lhs) = lhs {
+        for ((path, diff), old_rhs_ids) in paths_with_diffs.into_iter().zip(old_rhs_ids) {
+            lhs.multibuffer.update(cx, |lhs_multibuffer, lhs_cx| {
+                LhsEditor::sync_path_excerpts(
+                    path,
+                    old_rhs_ids,
+                    rhs_multibuffer,
+                    lhs_multibuffer,
+                    diff,
+                    rhs_display_map,
+                    lhs_cx,
+                );
+            });
+        }
+    }
+
+    result
+}
+
 impl LhsEditor {
     fn update_path_excerpts_from_rhs(
         path_key: PathKey,
@@ -2012,19 +2042,21 @@ impl LhsEditor {
 
     fn sync_path_excerpts(
         path_key: PathKey,
+        old_rhs_excerpt_ids: Vec<ExcerptId>,
         rhs_multibuffer: &MultiBuffer,
         lhs_multibuffer: &mut MultiBuffer,
         diff: Entity<BufferDiff>,
         rhs_display_map: &Entity<DisplayMap>,
         lhs_cx: &mut Context<MultiBuffer>,
     ) {
-        Self::remove_mappings_for_path(
-            &path_key,
-            rhs_multibuffer,
-            lhs_multibuffer,
-            rhs_display_map,
-            lhs_cx,
-        );
+        let old_lhs_excerpt_ids: Vec<ExcerptId> =
+            lhs_multibuffer.excerpts_for_path(&path_key).collect();
+
+        if let Some(companion) = rhs_display_map.read(lhs_cx).companion().cloned() {
+            companion.update(lhs_cx, |c, _| {
+                c.remove_excerpt_mappings(old_lhs_excerpt_ids, old_rhs_excerpt_ids);
+            });
+        }
 
         let mappings = Self::update_path_excerpts_from_rhs(
             path_key,
@@ -2046,23 +2078,6 @@ impl LhsEditor {
             });
         }
     }
-
-    fn remove_mappings_for_path(
-        path_key: &PathKey,
-        rhs_multibuffer: &MultiBuffer,
-        lhs_multibuffer: &MultiBuffer,
-        rhs_display_map: &Entity<DisplayMap>,
-        cx: &mut App,
-    ) {
-        let rhs_excerpt_ids: Vec<ExcerptId> = rhs_multibuffer.excerpts_for_path(path_key).collect();
-        let lhs_excerpt_ids: Vec<ExcerptId> = lhs_multibuffer.excerpts_for_path(path_key).collect();
-
-        if let Some(companion) = rhs_display_map.read(cx).companion().cloned() {
-            companion.update(cx, |c, _| {
-                c.remove_excerpt_mappings(lhs_excerpt_ids, rhs_excerpt_ids);
-            });
-        }
-    }
 }
 
 #[cfg(test)]