Fix duplicated multi-buffer excerpts (#29193)

Conrad Irwin and João Marcos created

- **add test case**
- **Merge excerpts more aggressively**
- **Randomized test for set_excerpts_for_path**

Closes #ISSUE

Release Notes:

- Fixed duplicted excerpts (and resulting panics)

---------

Co-authored-by: João Marcos <marcospb19@hotmail.com>

Change summary

crates/diagnostics/src/diagnostics_tests.rs   |   4 
crates/editor/src/test.rs                     |   3 
crates/git_ui/src/project_diff.rs             |  85 ++++++++++
crates/multi_buffer/src/multi_buffer.rs       | 173 ++++++++++----------
crates/multi_buffer/src/multi_buffer_tests.rs |  75 +++++++++
5 files changed, 251 insertions(+), 89 deletions(-)

Detailed changes

crates/diagnostics/src/diagnostics_tests.rs 🔗

@@ -760,7 +760,7 @@ async fn test_random_diagnostics_blocks(cx: &mut TestAppContext, mut rng: StdRng
 
     // The mutated view may contain more than the reference view as
     // we don't currently shrink excerpts when diagnostics were removed.
-    let mut ref_iter = reference_excerpts.lines();
+    let mut ref_iter = reference_excerpts.lines().filter(|line| *line != "§ -----");
     let mut next_ref_line = ref_iter.next();
     let mut skipped_block = false;
 
@@ -768,7 +768,7 @@ async fn test_random_diagnostics_blocks(cx: &mut TestAppContext, mut rng: StdRng
         if let Some(ref_line) = next_ref_line {
             if mut_line == ref_line {
                 next_ref_line = ref_iter.next();
-            } else if mut_line.contains('§') {
+            } else if mut_line.contains('§') && mut_line != "§ -----" {
                 skipped_block = true;
             }
         }

crates/editor/src/test.rs 🔗

@@ -199,6 +199,9 @@ pub fn editor_content_with_blocks(editor: &Entity<Editor>, cx: &mut VisualTestCo
                         lines[row.0 as usize].push_str("§ ");
                         lines[row.0 as usize].push_str(block_lines[0].trim_end());
                         for i in 1..height as usize {
+                            if row.0 as usize + i >= lines.len() {
+                                lines.push("".to_string());
+                            };
                             lines[row.0 as usize + i].push_str("§ ");
                             lines[row.0 as usize + i].push_str(block_lines[i].trim_end());
                         }

crates/git_ui/src/project_diff.rs 🔗

@@ -1568,7 +1568,7 @@ mod tests {
         fs.insert_tree(
             "/a",
             json!({
-                ".git":{},
+                ".git": {},
                 "a.txt": "created\n",
                 "b.txt": "really changed\n",
                 "c.txt": "unchanged\n"
@@ -1646,4 +1646,87 @@ mod tests {
         "
         ));
     }
+
+    #[gpui::test]
+    async fn test_excerpts_splitting_after_restoring_the_middle_excerpt(cx: &mut TestAppContext) {
+        init_test(cx);
+
+        let git_contents = indoc! {r#"
+            #[rustfmt::skip]
+            fn main() {
+                let x = 0.0; // this line will be removed
+                // 1
+                // 2
+                // 3
+                let y = 0.0; // this line will be removed
+                // 1
+                // 2
+                // 3
+                let arr = [
+                    0.0, // this line will be removed
+                    0.0, // this line will be removed
+                    0.0, // this line will be removed
+                    0.0, // this line will be removed
+                ];
+            }
+        "#};
+        let buffer_contents = indoc! {"
+            #[rustfmt::skip]
+            fn main() {
+                // 1
+                // 2
+                // 3
+                // 1
+                // 2
+                // 3
+                let arr = [
+                ];
+            }
+        "};
+
+        let fs = FakeFs::new(cx.executor());
+        fs.insert_tree(
+            "/a",
+            json!({
+                ".git": {},
+                "main.rs": buffer_contents,
+            }),
+        )
+        .await;
+
+        fs.set_git_content_for_repo(
+            Path::new("/a/.git"),
+            &[("main.rs".into(), git_contents.to_owned(), None)],
+        );
+
+        let project = Project::test(fs, [Path::new("/a")], cx).await;
+        let (workspace, cx) =
+            cx.add_window_view(|window, cx| Workspace::test_new(project, window, cx));
+
+        cx.run_until_parked();
+
+        cx.focus(&workspace);
+        cx.update(|window, cx| {
+            window.dispatch_action(project_diff::Diff.boxed_clone(), cx);
+        });
+
+        cx.run_until_parked();
+
+        let item = workspace.update(cx, |workspace, cx| {
+            workspace.active_item_as::<ProjectDiff>(cx).unwrap()
+        });
+        cx.focus(&item);
+        let editor = item.update(cx, |item, _| item.editor.clone());
+
+        let mut cx = EditorTestContext::for_editor_in(editor, cx).await;
+
+        cx.assert_excerpts_with_selections(&format!("[EXCERPT]\nˇ{git_contents}"));
+
+        cx.dispatch_action(editor::actions::GoToHunk);
+        cx.dispatch_action(editor::actions::GoToHunk);
+        cx.dispatch_action(git::Restore);
+        cx.dispatch_action(editor::actions::MoveToBeginning);
+
+        cx.assert_excerpts_with_selections(&format!("[EXCERPT]\nˇ{git_contents}"));
+    }
 }

crates/multi_buffer/src/multi_buffer.rs 🔗

@@ -1687,7 +1687,7 @@ impl MultiBuffer {
             if let Some(last_range) = merged_ranges.last_mut() {
                 debug_assert!(last_range.context.start <= range.context.start);
                 if last_range.context.end >= range.context.start {
-                    last_range.context.end = range.context.end;
+                    last_range.context.end = range.context.end.max(last_range.context.end);
                     *counts.last_mut().unwrap() += 1;
                     continue;
                 }
@@ -1741,106 +1741,106 @@ impl MultiBuffer {
         excerpts_cursor.next(&());
 
         loop {
-            let (new, existing) = match (new_iter.peek(), existing_iter.peek()) {
-                (Some(new), Some(existing)) => (new, existing),
-                (None, None) => break,
-                (None, Some(_)) => {
-                    let existing_id = existing_iter.next().unwrap();
-                    if let Some((new_id, last)) = to_insert.last() {
-                        let locator = snapshot.excerpt_locator_for_id(existing_id);
-                        excerpts_cursor.seek_forward(&Some(locator), Bias::Left, &());
-                        if let Some(existing_excerpt) = excerpts_cursor
-                            .item()
-                            .filter(|e| e.buffer_id == buffer_snapshot.remote_id())
-                        {
-                            let existing_end = existing_excerpt
-                                .range
-                                .context
-                                .end
-                                .to_point(&buffer_snapshot);
-                            if existing_end <= last.context.end {
-                                self.snapshot
-                                    .borrow_mut()
-                                    .replaced_excerpts
-                                    .insert(existing_id, *new_id);
-                            }
-                        };
+            let new = new_iter.peek();
+            let existing = if let Some(existing_id) = existing_iter.peek() {
+                let locator = snapshot.excerpt_locator_for_id(*existing_id);
+                excerpts_cursor.seek_forward(&Some(locator), Bias::Left, &());
+                if let Some(excerpt) = excerpts_cursor.item() {
+                    if excerpt.buffer_id != buffer_snapshot.remote_id() {
+                        to_remove.push(*existing_id);
+                        existing_iter.next();
+                        continue;
+                    }
+                    Some((
+                        *existing_id,
+                        excerpt.range.context.to_point(&buffer_snapshot),
+                    ))
+                } else {
+                    None
+                }
+            } else {
+                None
+            };
+
+            if let Some((last_id, last)) = to_insert.last_mut() {
+                if let Some(new) = new {
+                    if last.context.end >= new.context.start {
+                        last.context.end = last.context.end.max(new.context.end);
+                        excerpt_ids.push(*last_id);
+                        new_iter.next();
+                        continue;
                     }
+                }
+                if let Some((existing_id, existing_range)) = &existing {
+                    if last.context.end >= existing_range.start {
+                        last.context.end = last.context.end.max(existing_range.end);
+                        to_remove.push(*existing_id);
+                        self.snapshot
+                            .borrow_mut()
+                            .replaced_excerpts
+                            .insert(*existing_id, *last_id);
+                        existing_iter.next();
+                        continue;
+                    }
+                }
+            }
+
+            match (new, existing) {
+                (None, None) => break,
+                (None, Some((existing_id, _))) => {
+                    existing_iter.next();
                     to_remove.push(existing_id);
                     continue;
                 }
                 (Some(_), None) => {
                     added_a_new_excerpt = true;
-                    to_insert.push((next_excerpt_id(), new_iter.next().unwrap()));
+                    let new_id = next_excerpt_id();
+                    excerpt_ids.push(new_id);
+                    to_insert.push((new_id, new_iter.next().unwrap()));
                     continue;
                 }
-            };
-            let locator = snapshot.excerpt_locator_for_id(*existing);
-            excerpts_cursor.seek_forward(&Some(locator), Bias::Left, &());
-            let Some(existing_excerpt) = excerpts_cursor
-                .item()
-                .filter(|e| e.buffer_id == buffer_snapshot.remote_id())
-            else {
-                to_remove.push(existing_iter.next().unwrap());
-                to_insert.push((next_excerpt_id(), new_iter.next().unwrap()));
-                continue;
-            };
-
-            let existing_start = existing_excerpt
-                .range
-                .context
-                .start
-                .to_point(&buffer_snapshot);
-            let existing_end = existing_excerpt
-                .range
-                .context
-                .end
-                .to_point(&buffer_snapshot);
+                (Some(new), Some((_, existing_range))) => {
+                    if existing_range.end < new.context.start {
+                        let existing_id = existing_iter.next().unwrap();
+                        to_remove.push(existing_id);
+                        continue;
+                    } else if existing_range.start > new.context.end {
+                        let new_id = next_excerpt_id();
+                        excerpt_ids.push(new_id);
+                        to_insert.push((new_id, new_iter.next().unwrap()));
+                        continue;
+                    }
 
-            if existing_end < new.context.start {
-                let existing_id = existing_iter.next().unwrap();
-                if let Some((new_id, last)) = to_insert.last() {
-                    if existing_end <= last.context.end {
+                    if existing_range.start == new.context.start
+                        && existing_range.end == new.context.end
+                    {
+                        self.insert_excerpts_with_ids_after(
+                            insert_after,
+                            buffer.clone(),
+                            mem::take(&mut to_insert),
+                            cx,
+                        );
+                        insert_after = existing_iter.next().unwrap();
+                        excerpt_ids.push(insert_after);
+                        new_iter.next();
+                    } else {
+                        let existing_id = existing_iter.next().unwrap();
+                        let new_id = next_excerpt_id();
                         self.snapshot
                             .borrow_mut()
                             .replaced_excerpts
-                            .insert(existing_id, *new_id);
+                            .insert(existing_id, new_id);
+                        to_remove.push(existing_id);
+                        let mut range = new_iter.next().unwrap();
+                        range.context.start = range.context.start.min(existing_range.start);
+                        range.context.end = range.context.end.max(existing_range.end);
+                        excerpt_ids.push(new_id);
+                        to_insert.push((new_id, range));
                     }
                 }
-                to_remove.push(existing_id);
-                continue;
-            } else if existing_start > new.context.end {
-                to_insert.push((next_excerpt_id(), new_iter.next().unwrap()));
-                continue;
-            }
-
-            if existing_start == new.context.start && existing_end == new.context.end {
-                excerpt_ids.extend(to_insert.iter().map(|(id, _)| id));
-                self.insert_excerpts_with_ids_after(
-                    insert_after,
-                    buffer.clone(),
-                    mem::take(&mut to_insert),
-                    cx,
-                );
-                insert_after = existing_iter.next().unwrap();
-                excerpt_ids.push(insert_after);
-                new_iter.next();
-            } else {
-                let existing_id = existing_iter.next().unwrap();
-                let new_id = next_excerpt_id();
-                self.snapshot
-                    .borrow_mut()
-                    .replaced_excerpts
-                    .insert(existing_id, new_id);
-                to_remove.push(existing_id);
-                let mut range = new_iter.next().unwrap();
-                range.context.start = range.context.start.min(existing_start);
-                range.context.end = range.context.end.max(existing_end);
-                to_insert.push((new_id, range));
-            }
+            };
         }
 
-        excerpt_ids.extend(to_insert.iter().map(|(id, _)| id));
         self.insert_excerpts_with_ids_after(insert_after, buffer, to_insert, cx);
         self.remove_excerpts(to_remove, cx);
         if excerpt_ids.is_empty() {
@@ -1849,7 +1849,8 @@ impl MultiBuffer {
             for excerpt_id in &excerpt_ids {
                 self.paths_by_excerpt.insert(*excerpt_id, path.clone());
             }
-            self.excerpts_by_path.insert(path, excerpt_ids.clone());
+            self.excerpts_by_path
+                .insert(path, excerpt_ids.iter().dedup().cloned().collect());
         }
 
         (excerpt_ids, added_a_new_excerpt)

crates/multi_buffer/src/multi_buffer_tests.rs 🔗

@@ -2476,6 +2476,81 @@ impl ReferenceMultibuffer {
     }
 }
 
+#[gpui::test(iterations = 100)]
+async fn test_random_set_ranges(cx: &mut TestAppContext, mut rng: StdRng) {
+    let base_text = "a\n".repeat(100);
+    let buf = cx.update(|cx| cx.new(|cx| Buffer::local(base_text, cx)));
+    let multibuffer = cx.new(|_| MultiBuffer::new(Capability::ReadWrite));
+
+    let operations = env::var("OPERATIONS")
+        .map(|i| i.parse().expect("invalid `OPERATIONS` variable"))
+        .unwrap_or(10);
+
+    fn row_ranges(ranges: &Vec<Range<Point>>) -> Vec<Range<u32>> {
+        ranges
+            .iter()
+            .map(|range| range.start.row..range.end.row)
+            .collect()
+    }
+
+    for _ in 0..operations {
+        let snapshot = buf.update(cx, |buf, _| buf.snapshot());
+        let num_ranges = rng.gen_range(0..=10);
+        let max_row = snapshot.max_point().row;
+        let mut ranges = (0..num_ranges)
+            .map(|_| {
+                let start = rng.gen_range(0..max_row);
+                let end = rng.gen_range(start + 1..max_row + 1);
+                Point::row_range(start..end)
+            })
+            .collect::<Vec<_>>();
+        ranges.sort_by_key(|range| range.start);
+        log::info!("Setting ranges: {:?}", row_ranges(&ranges));
+        let (created, _) = multibuffer.update(cx, |multibuffer, cx| {
+            multibuffer.set_excerpts_for_path(
+                PathKey::for_buffer(&buf, cx),
+                buf.clone(),
+                ranges.clone(),
+                2,
+                cx,
+            )
+        });
+
+        assert_eq!(created.len(), ranges.len());
+
+        let snapshot = multibuffer.update(cx, |multibuffer, cx| multibuffer.snapshot(cx));
+        let mut last_end = None;
+        let mut seen_ranges = Vec::default();
+
+        for (_, buf, range) in snapshot.excerpts() {
+            let start = range.context.start.to_point(&buf);
+            let end = range.context.end.to_point(&buf);
+            seen_ranges.push(start..end);
+
+            if let Some(last_end) = last_end.take() {
+                assert!(
+                    start > last_end,
+                    "multibuffer has out-of-order ranges: {:?}; {:?} <= {:?}",
+                    row_ranges(&seen_ranges),
+                    start,
+                    last_end
+                )
+            }
+
+            ranges.retain(|range| range.start < start || range.end > end);
+
+            last_end = Some(end)
+        }
+
+        assert!(
+            ranges.is_empty(),
+            "multibuffer {:?} did not include all ranges: {:?}",
+            row_ranges(&seen_ranges),
+            row_ranges(&ranges)
+        );
+    }
+}
+
 #[gpui::test(iterations = 100)]
 async fn test_random_multibuffer(cx: &mut TestAppContext, mut rng: StdRng) {
     let operations = env::var("OPERATIONS")