From 3357736aea3ccab3ffd7da9dc6e6621eaf31ffb0 Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Mon, 21 Apr 2025 23:25:09 -0600 Subject: [PATCH] Fix duplicated multi-buffer excerpts (#29193) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - **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 --- 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(-) diff --git a/crates/diagnostics/src/diagnostics_tests.rs b/crates/diagnostics/src/diagnostics_tests.rs index 896839ae24..8e455d8523 100644 --- a/crates/diagnostics/src/diagnostics_tests.rs +++ b/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; } } diff --git a/crates/editor/src/test.rs b/crates/editor/src/test.rs index 5ab6866495..9af7d1e25c 100644 --- a/crates/editor/src/test.rs +++ b/crates/editor/src/test.rs @@ -199,6 +199,9 @@ pub fn editor_content_with_blocks(editor: &Entity, 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()); } diff --git a/crates/git_ui/src/project_diff.rs b/crates/git_ui/src/project_diff.rs index 7975d11960..bd53543aad 100644 --- a/crates/git_ui/src/project_diff.rs +++ b/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::(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}")); + } } diff --git a/crates/multi_buffer/src/multi_buffer.rs b/crates/multi_buffer/src/multi_buffer.rs index 7c2b7c8f73..e2be9f436e 100644 --- a/crates/multi_buffer/src/multi_buffer.rs +++ b/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; - }; + (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; + } - let existing_start = existing_excerpt - .range - .context - .start - .to_point(&buffer_snapshot); - let existing_end = existing_excerpt - .range - .context - .end - .to_point(&buffer_snapshot); - - 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) diff --git a/crates/multi_buffer/src/multi_buffer_tests.rs b/crates/multi_buffer/src/multi_buffer_tests.rs index 012d8e1e0a..1aa5cfc2ac 100644 --- a/crates/multi_buffer/src/multi_buffer_tests.rs +++ b/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>) -> Vec> { + 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::>(); + 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")