From bbc7fcc54f40dd9c6be5eb057b58e01b49d1ae58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Marcos?= Date: Mon, 24 Mar 2025 16:25:56 -0300 Subject: [PATCH] Fix crash when toggling deleted hunk (#27138) Release Notes: - Fix rare crash when toggling deleted hunks. --------- Co-authored-by: Max Brunsfeld --- crates/buffer_diff/src/buffer_diff.rs | 110 ++++++++++++++++++++------ crates/fs/src/fs.rs | 5 ++ crates/git_ui/src/commit_modal.rs | 2 - crates/project/src/project_tests.rs | 6 +- 4 files changed, 94 insertions(+), 29 deletions(-) diff --git a/crates/buffer_diff/src/buffer_diff.rs b/crates/buffer_diff/src/buffer_diff.rs index 0ae9095021..4a08961b0f 100644 --- a/crates/buffer_diff/src/buffer_diff.rs +++ b/crates/buffer_diff/src/buffer_diff.rs @@ -7,8 +7,7 @@ use std::cmp::Ordering; use std::mem; use std::{future::Future, iter, ops::Range, sync::Arc}; use sum_tree::SumTree; -use text::{Anchor, Bias, BufferId, OffsetRangeExt, Point}; -use text::{AnchorRangeExt, ToOffset as _}; +use text::{Anchor, Bias, BufferId, OffsetRangeExt, Point, ToOffset as _}; use util::ResultExt; pub struct BufferDiff { @@ -189,7 +188,7 @@ impl BufferDiffSnapshot { impl BufferDiffInner { /// Returns the new index text and new pending hunks. - fn stage_or_unstage_hunks( + fn stage_or_unstage_hunks_impl( &mut self, unstaged_diff: &Self, stage: bool, @@ -234,9 +233,6 @@ impl BufferDiffInner { } }; - let mut unstaged_hunk_cursor = unstaged_diff.hunks.cursor::(buffer); - unstaged_hunk_cursor.next(buffer); - let mut pending_hunks = SumTree::new(buffer); let mut old_pending_hunks = unstaged_diff .pending_hunks @@ -252,18 +248,16 @@ impl BufferDiffInner { { let preceding_pending_hunks = old_pending_hunks.slice(&buffer_range.start, Bias::Left, buffer); - pending_hunks.append(preceding_pending_hunks, buffer); - // skip all overlapping old pending hunks - while old_pending_hunks - .item() - .is_some_and(|preceding_pending_hunk_item| { - preceding_pending_hunk_item - .buffer_range - .overlaps(&buffer_range, buffer) - }) - { + // Skip all overlapping or adjacent old pending hunks + while old_pending_hunks.item().is_some_and(|old_hunk| { + old_hunk + .buffer_range + .start + .cmp(&buffer_range.end, buffer) + .is_le() + }) { old_pending_hunks.next(buffer); } @@ -291,6 +285,9 @@ impl BufferDiffInner { // append the remainder pending_hunks.append(old_pending_hunks.suffix(buffer), buffer); + let mut unstaged_hunk_cursor = unstaged_diff.hunks.cursor::(buffer); + unstaged_hunk_cursor.next(buffer); + let mut prev_unstaged_hunk_buffer_offset = 0; let mut prev_unstaged_hunk_base_text_offset = 0; let mut edits = Vec::<(Range, String)>::new(); @@ -357,7 +354,13 @@ impl BufferDiffInner { edits.push((index_range, replacement_text)); } - debug_assert!(edits.iter().is_sorted_by_key(|(range, _)| range.start)); + #[cfg(debug_assertions)] // invariants: non-overlapping and sorted + { + for window in edits.windows(2) { + let (range_a, range_b) = (&window[0].0, &window[1].0); + debug_assert!(range_a.end < range_b.start); + } + } let mut new_index_text = Rope::new(); let mut index_cursor = index_text.cursor(0); @@ -854,7 +857,7 @@ impl BufferDiff { file_exists: bool, cx: &mut Context, ) -> Option { - let (new_index_text, new_pending_hunks) = self.inner.stage_or_unstage_hunks( + let (new_index_text, new_pending_hunks) = self.inner.stage_or_unstage_hunks_impl( &self.secondary_diff.as_ref()?.read(cx).inner, stage, &hunks, @@ -1240,13 +1243,13 @@ impl DiffHunkStatus { } } -/// Range (crossing new lines), old, new #[cfg(any(test, feature = "test-support"))] #[track_caller] pub fn assert_hunks( diff_hunks: HunkIter, buffer: &text::BufferSnapshot, diff_base: &str, + // Line range, deleted, added, status expected_hunks: &[(Range, ExpectedText, ExpectedText, DiffHunkStatus)], ) where HunkIter: Iterator, @@ -1267,11 +1270,11 @@ pub fn assert_hunks( let expected_hunks: Vec<_> = expected_hunks .iter() - .map(|(r, old_text, new_text, status)| { + .map(|(line_range, deleted_text, added_text, status)| { ( - Point::new(r.start, 0)..Point::new(r.end, 0), - old_text.as_ref(), - new_text.as_ref().to_string(), + Point::new(line_range.start, 0)..Point::new(line_range.end, 0), + deleted_text.as_ref(), + added_text.as_ref().to_string(), *status, ) }) @@ -1286,6 +1289,7 @@ mod tests { use super::*; use gpui::TestAppContext; + use pretty_assertions::{assert_eq, assert_ne}; use rand::{rngs::StdRng, Rng as _}; use text::{Buffer, BufferId, Rope}; use unindent::Unindent as _; @@ -1705,6 +1709,66 @@ mod tests { } } + #[gpui::test] + async fn test_toggling_stage_and_unstage_same_hunk(cx: &mut TestAppContext) { + let head_text = " + one + two + three + " + .unindent(); + let index_text = head_text.clone(); + let buffer_text = " + one + three + " + .unindent(); + + let buffer = Buffer::new(0, BufferId::new(1).unwrap(), buffer_text.clone()); + let unstaged = BufferDiff::build_sync(buffer.clone(), index_text, cx); + let uncommitted = BufferDiff::build_sync(buffer.clone(), head_text.clone(), cx); + let unstaged_diff = cx.new(|cx| { + let mut diff = BufferDiff::new(&buffer, cx); + diff.set_state(unstaged, &buffer); + diff + }); + let uncommitted_diff = cx.new(|cx| { + let mut diff = BufferDiff::new(&buffer, cx); + diff.set_state(uncommitted, &buffer); + diff.set_secondary_diff(unstaged_diff.clone()); + diff + }); + + uncommitted_diff.update(cx, |diff, cx| { + let hunk = diff.hunks(&buffer, cx).next().unwrap(); + + let new_index_text = diff + .stage_or_unstage_hunks(true, &[hunk.clone()], &buffer, true, cx) + .unwrap() + .to_string(); + assert_eq!(new_index_text, buffer_text); + + let hunk = diff.hunks(&buffer, &cx).next().unwrap(); + assert_eq!( + hunk.secondary_status, + DiffHunkSecondaryStatus::SecondaryHunkRemovalPending + ); + + let index_text = diff + .stage_or_unstage_hunks(false, &[hunk], &buffer, true, cx) + .unwrap() + .to_string(); + assert_eq!(index_text, head_text); + + let hunk = diff.hunks(&buffer, &cx).next().unwrap(); + // optimistically unstaged (fine, could also be HasSecondaryHunk) + assert_eq!( + hunk.secondary_status, + DiffHunkSecondaryStatus::SecondaryHunkAdditionPending + ); + }); + } + #[gpui::test] async fn test_buffer_diff_compare(cx: &mut TestAppContext) { let base_text = " diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index afe4fce4d7..033189c32e 100644 --- a/crates/fs/src/fs.rs +++ b/crates/fs/src/fs.rs @@ -1176,6 +1176,11 @@ impl FakeFs { self.state.lock().events_paused = true; } + pub fn unpause_events_and_flush(&self) { + self.state.lock().events_paused = false; + self.flush_events(usize::MAX); + } + pub fn buffered_event_count(&self) -> usize { self.state.lock().buffered_events.len() } diff --git a/crates/git_ui/src/commit_modal.rs b/crates/git_ui/src/commit_modal.rs index 0bcbd2a71f..8314bf3c1c 100644 --- a/crates/git_ui/src/commit_modal.rs +++ b/crates/git_ui/src/commit_modal.rs @@ -1,5 +1,3 @@ -// #![allow(unused, dead_code)] - use crate::branch_picker::{self, BranchList}; use crate::git_panel::{commit_message_editor, GitPanel}; use git::{Commit, GenerateCommitMessage}; diff --git a/crates/project/src/project_tests.rs b/crates/project/src/project_tests.rs index 205900dd29..52d0203d3f 100644 --- a/crates/project/src/project_tests.rs +++ b/crates/project/src/project_tests.rs @@ -24,9 +24,7 @@ use pretty_assertions::{assert_eq, assert_matches}; use serde_json::json; #[cfg(not(windows))] use std::os; -use std::{str::FromStr, sync::OnceLock}; - -use std::{mem, num::NonZeroU32, ops::Range, task::Poll}; +use std::{mem, num::NonZeroU32, ops::Range, str::FromStr, sync::OnceLock, task::Poll}; use task::{ResolvedTask, TaskContext}; use unindent::Unindent as _; use util::{ @@ -6359,7 +6357,7 @@ async fn test_staging_hunks(cx: &mut gpui::TestAppContext) { }); } -#[gpui::test(iterations = 10, seeds(340, 472))] +#[gpui::test(seeds(340, 472))] async fn test_staging_hunks_with_delayed_fs_event(cx: &mut gpui::TestAppContext) { use DiffHunkSecondaryStatus::*; init_test(cx);