From 930d9321dc66d7fc8a2f7d977f668431c9549b33 Mon Sep 17 00:00:00 2001 From: Cole Miller Date: Fri, 20 Feb 2026 17:11:30 -0500 Subject: [PATCH] git: Fix panic when unstaged diff is recalculated before its primary diff (#49753) For inverted diff APIs like `hunks_intersecting_base_text_range`, we need a snapshot of the main buffer to use as context to compare hunk anchors, convert anchors to points, etc. Previously, we were always using a snapshot of the main buffer at the time when the diff was calculated. This worked well for the hunks of the primary `BufferDiff`, but it's not valid when the diff also has a secondary set of hunks, because those hunks are recalculated on their own schedule--so it's possible for the hunks of the secondary diff to contain anchors that are newer than the original buffer snapshot that was taken at the time the primary diff hunks were last calculated. This caused a panic when using these anchors with the original buffer snapshot. This PR fixes the issue by using the same approach for inverted diffs as for non-inverted diffs at the multibuffer level: we take a snapshot of the main buffer at the time when we snapshot the diff state (including the diff itself), guaranteeing that that snapshot will be new enough to resolve all the anchors included in both the primary diff and the secondary diff. Closes ZED-54J Release Notes: - Fixed a panic that could occur when using the branch diff in split view mode. --------- Co-authored-by: Antonio Scandurra --- crates/buffer_diff/src/buffer_diff.rs | 2 +- crates/editor/src/display_map/block_map.rs | 2 +- crates/editor/src/split.rs | 5 +- crates/git_ui/src/project_diff.rs | 155 ++++++++++++++++++ crates/multi_buffer/src/multi_buffer.rs | 119 ++++++++------ crates/multi_buffer/src/multi_buffer_tests.rs | 86 ++++++++-- 6 files changed, 308 insertions(+), 61 deletions(-) diff --git a/crates/buffer_diff/src/buffer_diff.rs b/crates/buffer_diff/src/buffer_diff.rs index 0b2d34823d80a2e1fc3eba768f501e028034e6ea..2c9a68d5526f2cb0f03bc3da7ab611233091b143 100644 --- a/crates/buffer_diff/src/buffer_diff.rs +++ b/crates/buffer_diff/src/buffer_diff.rs @@ -248,7 +248,7 @@ impl BufferDiffSnapshot { self.inner.buffer_version() } - pub fn original_buffer_snapshot(&self) -> &text::BufferSnapshot { + fn original_buffer_snapshot(&self) -> &text::BufferSnapshot { &self.inner.buffer_snapshot } diff --git a/crates/editor/src/display_map/block_map.rs b/crates/editor/src/display_map/block_map.rs index c986ba2113ef21ac05549913695d13143fb1f800..0073166e3d5ee8989d7c5d16112b86395fc7cebf 100644 --- a/crates/editor/src/display_map/block_map.rs +++ b/crates/editor/src/display_map/block_map.rs @@ -4595,7 +4595,7 @@ mod tests { [ExcerptRange::new(text::Anchor::MIN..text::Anchor::MAX)], cx, ); - mb.add_inverted_diff(diff.clone(), cx); + mb.add_inverted_diff(diff.clone(), rhs_buffer.clone(), cx); mb }); let rhs_multibuffer = cx.new(|cx| { diff --git a/crates/editor/src/split.rs b/crates/editor/src/split.rs index 119b2da80b7ff288f94788029dee15aba546a7f3..570f6c46ddd377b83b3ccda6c19664de48f2a7b7 100644 --- a/crates/editor/src/split.rs +++ b/crates/editor/src/split.rs @@ -2076,7 +2076,10 @@ impl LhsEditor { .diff_for(remote_id) .is_none_or(|old_diff| old_diff.entity_id() != diff.entity_id()) { - lhs_multibuffer.add_inverted_diff(diff, lhs_cx); + let main_buffer_entity = rhs_multibuffer + .buffer(main_buffer.remote_id()) + .expect("main buffer should exist in rhs_multibuffer"); + lhs_multibuffer.add_inverted_diff(diff, main_buffer_entity, lhs_cx); } let rhs_merge_groups: Vec> = { diff --git a/crates/git_ui/src/project_diff.rs b/crates/git_ui/src/project_diff.rs index f56b2c795f7d1de9e26756e9195d4a44c63ba9b7..cf241004338cdad56539e0b181f0b5a0d543744a 100644 --- a/crates/git_ui/src/project_diff.rs +++ b/crates/git_ui/src/project_diff.rs @@ -2529,6 +2529,161 @@ mod tests { assert_eq!(contents, "ours\n"); } + #[gpui::test(iterations = 50)] + async fn test_split_diff_conflict_path_transition_with_dirty_buffer_invalid_anchor_panics( + cx: &mut TestAppContext, + ) { + init_test(cx); + + cx.update(|cx| { + cx.update_global::(|store, cx| { + store.update_user_settings(cx, |settings| { + settings.editor.diff_view_style = Some(DiffViewStyle::Split); + }); + }); + }); + + let build_conflict_text: fn(usize) -> String = |tag: usize| { + let mut lines = (0..80) + .map(|line_index| format!("line {line_index}")) + .collect::>(); + for offset in [5usize, 20, 37, 61] { + lines[offset] = format!("base-{tag}-line-{offset}"); + } + format!("{}\n", lines.join("\n")) + }; + let initial_conflict_text = build_conflict_text(0); + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/project"), + json!({ + ".git": {}, + "helper.txt": "same\n", + "conflict.txt": initial_conflict_text, + }), + ) + .await; + fs.with_git_state(path!("/project/.git").as_ref(), true, |state| { + state + .refs + .insert("MERGE_HEAD".into(), "conflict-head".into()); + }) + .unwrap(); + fs.set_status_for_repo( + path!("/project/.git").as_ref(), + &[( + "conflict.txt", + FileStatus::Unmerged(UnmergedStatus { + first_head: UnmergedStatusCode::Updated, + second_head: UnmergedStatusCode::Updated, + }), + )], + ); + fs.set_merge_base_content_for_repo( + path!("/project/.git").as_ref(), + &[ + ("conflict.txt", build_conflict_text(1)), + ("helper.txt", "same\n".to_string()), + ], + ); + + let project = Project::test(fs.clone(), [path!("/project").as_ref()], cx).await; + let (multi_workspace, cx) = + cx.add_window_view(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx)); + let workspace = multi_workspace.read_with(cx, |mw, _| mw.workspace().clone()); + let _project_diff = cx + .update(|window, cx| { + ProjectDiff::new_with_default_branch(project.clone(), workspace, window, cx) + }) + .await + .unwrap(); + cx.run_until_parked(); + + let buffer = project + .update(cx, |project, cx| { + project.open_local_buffer(path!("/project/conflict.txt"), cx) + }) + .await + .unwrap(); + buffer.update(cx, |buffer, cx| buffer.edit([(0..0, "dirty\n")], None, cx)); + assert!(buffer.read_with(cx, |buffer, _| buffer.is_dirty())); + cx.run_until_parked(); + + cx.update(|window, cx| { + let fs = fs.clone(); + window + .spawn(cx, async move |cx| { + cx.background_executor().simulate_random_delay().await; + fs.with_git_state(path!("/project/.git").as_ref(), true, |state| { + state.refs.insert("HEAD".into(), "head-1".into()); + state.refs.remove("MERGE_HEAD"); + }) + .unwrap(); + fs.set_status_for_repo( + path!("/project/.git").as_ref(), + &[ + ( + "conflict.txt", + FileStatus::Tracked(TrackedStatus { + index_status: git::status::StatusCode::Modified, + worktree_status: git::status::StatusCode::Modified, + }), + ), + ( + "helper.txt", + FileStatus::Tracked(TrackedStatus { + index_status: git::status::StatusCode::Modified, + worktree_status: git::status::StatusCode::Modified, + }), + ), + ], + ); + // FakeFs assigns deterministic OIDs by entry position; flipping order churns + // conflict diff identity without reaching into ProjectDiff internals. + fs.set_merge_base_content_for_repo( + path!("/project/.git").as_ref(), + &[ + ("helper.txt", "helper-base\n".to_string()), + ("conflict.txt", build_conflict_text(2)), + ], + ); + }) + .detach(); + }); + + cx.update(|window, cx| { + let buffer = buffer.clone(); + window + .spawn(cx, async move |cx| { + cx.background_executor().simulate_random_delay().await; + for edit_index in 0..10 { + if edit_index > 0 { + cx.background_executor().simulate_random_delay().await; + } + buffer.update(cx, |buffer, cx| { + let len = buffer.len(); + if edit_index % 2 == 0 { + buffer.edit( + [(0..0, format!("status-burst-head-{edit_index}\n"))], + None, + cx, + ); + } else { + buffer.edit( + [(len..len, format!("status-burst-tail-{edit_index}\n"))], + None, + cx, + ); + } + }); + } + }) + .detach(); + }); + + cx.run_until_parked(); + } + #[gpui::test] async fn test_new_hunk_in_modified_file(cx: &mut TestAppContext) { init_test(cx); diff --git a/crates/multi_buffer/src/multi_buffer.rs b/crates/multi_buffer/src/multi_buffer.rs index c1764ff25034da6fa6c6218e0de33f6bd63d24bb..4f89dfa6faf7aec30eefc99c96334c8e1286e177 100644 --- a/crates/multi_buffer/src/multi_buffer.rs +++ b/crates/multi_buffer/src/multi_buffer.rs @@ -522,7 +522,7 @@ struct BufferState { struct DiffState { diff: Entity, - is_inverted: bool, + main_buffer: Option>, _subscription: gpui::Subscription, } @@ -530,7 +530,7 @@ impl DiffState { fn snapshot(&self, cx: &App) -> DiffStateSnapshot { DiffStateSnapshot { diff: self.diff.read(cx).snapshot(cx), - is_inverted: self.is_inverted, + main_buffer: self.main_buffer.as_ref().map(|b| b.read(cx).snapshot()), } } } @@ -538,7 +538,7 @@ impl DiffState { #[derive(Clone)] struct DiffStateSnapshot { diff: BufferDiffSnapshot, - is_inverted: bool, + main_buffer: Option, } impl std::ops::Deref for DiffStateSnapshot { @@ -573,32 +573,47 @@ impl DiffState { _ => {} }), diff, - is_inverted: false, + main_buffer: None, } } - fn new_inverted(diff: Entity, cx: &mut Context) -> Self { + fn new_inverted( + diff: Entity, + main_buffer: Entity, + cx: &mut Context, + ) -> Self { + let weak_main_buffer = main_buffer.downgrade(); DiffState { _subscription: cx.subscribe(&diff, { - move |this, diff, event, cx| match event { - BufferDiffEvent::DiffChanged(DiffChanged { - changed_range: _, - base_text_changed_range, - extended_range: _, - }) => { - if let Some(base_text_changed_range) = base_text_changed_range.clone() { - this.inverted_buffer_diff_changed(diff, base_text_changed_range, cx) + move |this, diff, event, cx| { + let Some(main_buffer) = weak_main_buffer.upgrade() else { + return; + }; + match event { + BufferDiffEvent::DiffChanged(DiffChanged { + changed_range: _, + base_text_changed_range, + extended_range: _, + }) => { + if let Some(base_text_changed_range) = base_text_changed_range.clone() { + this.inverted_buffer_diff_changed( + diff, + main_buffer, + base_text_changed_range, + cx, + ) + } + cx.emit(Event::BufferDiffChanged); } - cx.emit(Event::BufferDiffChanged); - } - BufferDiffEvent::LanguageChanged => { - this.inverted_buffer_diff_language_changed(diff, cx) + BufferDiffEvent::LanguageChanged => { + this.inverted_buffer_diff_language_changed(diff, main_buffer, cx) + } + _ => {} } - _ => {} } }), diff, - is_inverted: true, + main_buffer: Some(main_buffer), } } } @@ -2297,7 +2312,10 @@ impl MultiBuffer { // Recalculate has_inverted_diff after removing diffs if !removed_buffer_ids.is_empty() { - snapshot.has_inverted_diff = snapshot.diffs.iter().any(|(_, diff)| diff.is_inverted); + snapshot.has_inverted_diff = snapshot + .diffs + .iter() + .any(|(_, diff)| diff.main_buffer.is_some()); } if changed_trailing_excerpt { @@ -2399,7 +2417,7 @@ impl MultiBuffer { let buffer_id = diff.buffer_id; let diff = DiffStateSnapshot { diff: diff.snapshot(cx), - is_inverted: false, + main_buffer: None, }; self.snapshot.get_mut().diffs.insert(buffer_id, diff); } @@ -2407,13 +2425,15 @@ impl MultiBuffer { fn inverted_buffer_diff_language_changed( &mut self, diff: Entity, + main_buffer: Entity, cx: &mut Context, ) { let base_text_buffer_id = diff.read(cx).base_text_buffer().read(cx).remote_id(); + let main_buffer_snapshot = main_buffer.read(cx).snapshot(); let diff = diff.read(cx); let diff = DiffStateSnapshot { diff: diff.snapshot(cx), - is_inverted: true, + main_buffer: Some(main_buffer_snapshot), }; self.snapshot .get_mut() @@ -2437,7 +2457,7 @@ impl MultiBuffer { }; let new_diff = DiffStateSnapshot { diff: diff.snapshot(cx), - is_inverted: false, + main_buffer: None, }; let mut snapshot = self.snapshot.get_mut(); let base_text_changed = snapshot @@ -2468,20 +2488,22 @@ impl MultiBuffer { fn inverted_buffer_diff_changed( &mut self, diff: Entity, + main_buffer: Entity, diff_change_range: Range, cx: &mut Context, ) { self.sync_mut(cx); - let diff = diff.read(cx); - let base_text_buffer_id = diff.base_text_buffer().read(cx).remote_id(); + let base_text_buffer_id = diff.read(cx).base_text_buffer().read(cx).remote_id(); let Some(buffer_state) = self.buffers.get(&base_text_buffer_id) else { return; }; + let main_buffer_snapshot = main_buffer.read(cx).snapshot(); + let diff = diff.read(cx); let new_diff = DiffStateSnapshot { diff: diff.snapshot(cx), - is_inverted: true, + main_buffer: Some(main_buffer_snapshot), }; let mut snapshot = self.snapshot.get_mut(); snapshot @@ -2673,14 +2695,21 @@ impl MultiBuffer { self.diffs.insert(buffer_id, DiffState::new(diff, cx)); } - pub fn add_inverted_diff(&mut self, diff: Entity, cx: &mut Context) { + pub fn add_inverted_diff( + &mut self, + diff: Entity, + main_buffer: Entity, + cx: &mut Context, + ) { let snapshot = diff.read(cx).base_text(cx); let base_text_buffer_id = snapshot.remote_id(); let diff_change_range = 0..snapshot.len(); self.snapshot.get_mut().has_inverted_diff = true; - self.inverted_buffer_diff_changed(diff.clone(), diff_change_range, cx); - self.diffs - .insert(base_text_buffer_id, DiffState::new_inverted(diff, cx)); + self.inverted_buffer_diff_changed(diff.clone(), main_buffer.clone(), diff_change_range, cx); + self.diffs.insert( + base_text_buffer_id, + DiffState::new_inverted(diff, main_buffer, cx), + ); } pub fn diff_for(&self, buffer_id: BufferId) -> Option> { @@ -3092,7 +3121,7 @@ impl MultiBuffer { // Those base text ranges are usize, so make sure if the base text changed // we also update the diff snapshot so that we don't use stale offsets if buffer_diff.get(id).is_none_or(|existing_diff| { - if !existing_diff.is_inverted { + if existing_diff.main_buffer.is_none() { return false; } let base_text = diff.diff.read(cx).base_text_buffer().read(cx); @@ -3297,7 +3326,7 @@ impl MultiBuffer { .. }) => excerpts.item().is_some_and(|excerpt| { if let Some(diff) = snapshot.diffs.get(&excerpt.buffer_id) - && diff.is_inverted + && diff.main_buffer.is_some() { return true; } @@ -3409,10 +3438,10 @@ impl MultiBuffer { excerpt_buffer_start + edit.new.end.saturating_sub(excerpt_start); let edit_buffer_end = edit_buffer_end.min(excerpt_buffer_end); - if diff.is_inverted { + if let Some(main_buffer) = &diff.main_buffer { for hunk in diff.hunks_intersecting_base_text_range( edit_buffer_start..edit_buffer_end, - diff.original_buffer_snapshot(), + main_buffer, ) { did_expand_hunks = true; let hunk_buffer_range = hunk.diff_base_byte_range.clone(); @@ -3966,15 +3995,12 @@ impl MultiBufferSnapshot { let query_range = range.start.to_point(self)..range.end.to_point(self); self.lift_buffer_metadata(query_range.clone(), move |buffer, buffer_range| { let diff = self.diffs.get(&buffer.remote_id())?; - let iter = if diff.is_inverted { + let iter = if let Some(main_buffer) = &diff.main_buffer { let buffer_start = buffer.point_to_offset(buffer_range.start); let buffer_end = buffer.point_to_offset(buffer_range.end); itertools::Either::Left( - diff.hunks_intersecting_base_text_range( - buffer_start..buffer_end, - diff.original_buffer_snapshot(), - ) - .map(move |hunk| (hunk, buffer, true)), + diff.hunks_intersecting_base_text_range(buffer_start..buffer_end, main_buffer) + .map(move |hunk| (hunk, buffer, true)), ) } else { let buffer_start = buffer.anchor_before(buffer_range.start); @@ -4552,11 +4578,10 @@ impl MultiBufferSnapshot { .to_offset(&excerpt.buffer); if let Some(diff) = self.diffs.get(&excerpt.buffer_id) { - if diff.is_inverted { - for hunk in diff.hunks_intersecting_base_text_range_rev( - excerpt_start..excerpt_end, - diff.original_buffer_snapshot(), - ) { + if let Some(main_buffer) = &diff.main_buffer { + for hunk in diff + .hunks_intersecting_base_text_range_rev(excerpt_start..excerpt_end, main_buffer) + { if hunk.diff_base_byte_range.end >= current_position { continue; } @@ -4590,11 +4615,11 @@ impl MultiBufferSnapshot { let Some(diff) = self.diffs.get(&excerpt.buffer_id) else { continue; }; - if diff.is_inverted { + if let Some(main_buffer) = &diff.main_buffer { let Some(hunk) = diff .hunks_intersecting_base_text_range_rev( excerpt.range.context.to_offset(&excerpt.buffer), - diff.original_buffer_snapshot(), + main_buffer, ) .next() else { diff --git a/crates/multi_buffer/src/multi_buffer_tests.rs b/crates/multi_buffer/src/multi_buffer_tests.rs index 9bf10a270bf7e8f54543d5a530a3e5fc947eb97c..5e028e60f13034e73c0c2cb6ae05c6bf56911c87 100644 --- a/crates/multi_buffer/src/multi_buffer_tests.rs +++ b/crates/multi_buffer/src/multi_buffer_tests.rs @@ -528,7 +528,7 @@ async fn test_inverted_diff_hunks_in_range(cx: &mut TestAppContext) { }); multibuffer.update(cx, |multibuffer, cx| { - multibuffer.add_inverted_diff(diff, cx); + multibuffer.add_inverted_diff(diff, buffer.clone(), cx); }); assert_new_snapshot( @@ -2336,7 +2336,7 @@ async fn test_diff_hunks_with_multiple_excerpts(cx: &mut TestAppContext) { struct ReferenceMultibuffer { excerpts: Vec, diffs: HashMap>, - inverted_diffs: HashMap>, + inverted_diffs: HashMap, Entity)>, } #[derive(Debug)] @@ -2483,13 +2483,14 @@ impl ReferenceMultibuffer { let buffer_id = buffer.remote_id(); let buffer_range = excerpt.range.to_offset(buffer); - if let Some(diff) = self.inverted_diffs.get(&buffer_id) { + if let Some((diff, main_buffer)) = self.inverted_diffs.get(&buffer_id) { let diff_snapshot = diff.read(cx).snapshot(cx); + let main_buffer_snapshot = main_buffer.read(cx).snapshot(); let mut offset = buffer_range.start; for hunk in diff_snapshot.hunks_intersecting_base_text_range( buffer_range.clone(), - diff_snapshot.original_buffer_snapshot(), + &main_buffer_snapshot.text, ) { let mut hunk_base_range = hunk.diff_base_byte_range.clone(); @@ -2788,9 +2789,15 @@ impl ReferenceMultibuffer { self.diffs.insert(buffer_id, diff); } - fn add_inverted_diff(&mut self, diff: Entity, cx: &App) { + fn add_inverted_diff( + &mut self, + diff: Entity, + main_buffer: Entity, + cx: &App, + ) { let base_text_buffer_id = diff.read(cx).base_text(cx).remote_id(); - self.inverted_diffs.insert(base_text_buffer_id, diff); + self.inverted_diffs + .insert(base_text_buffer_id, (diff, main_buffer)); } } @@ -3162,9 +3169,9 @@ async fn test_random_multibuffer(cx: &mut TestAppContext, mut rng: StdRng) { excerpt_buffer.read_with(cx, |buffer, _| buffer.remote_id()); multibuffer.update(cx, |multibuffer, cx| { if multibuffer.diff_for(excerpt_buffer_id).is_none() { - if inverted_main_buffer.is_some() { - reference.add_inverted_diff(diff.clone(), cx); - multibuffer.add_inverted_diff(diff, cx); + if let Some(main_buffer) = inverted_main_buffer { + reference.add_inverted_diff(diff.clone(), main_buffer.clone(), cx); + multibuffer.add_inverted_diff(diff, main_buffer, cx); } else { reference.add_diff(diff.clone(), cx); multibuffer.add_diff(diff, cx); @@ -3938,7 +3945,7 @@ async fn test_singleton_with_inverted_diff(cx: &mut TestAppContext) { let multibuffer = cx.new(|cx| { let mut multibuffer = MultiBuffer::singleton(base_text_buffer.clone(), cx); multibuffer.set_all_diff_hunks_expanded(cx); - multibuffer.add_inverted_diff(diff.clone(), cx); + multibuffer.add_inverted_diff(diff.clone(), buffer.clone(), cx); multibuffer }); @@ -4089,7 +4096,7 @@ async fn test_inverted_diff_base_text_change(cx: &mut TestAppContext) { let multibuffer = cx.new(|cx| { let mut multibuffer = MultiBuffer::singleton(base_text_buffer.clone(), cx); multibuffer.set_all_diff_hunks_expanded(cx); - multibuffer.add_inverted_diff(diff.clone(), cx); + multibuffer.add_inverted_diff(diff.clone(), buffer.clone(), cx); multibuffer }); @@ -4134,6 +4141,63 @@ async fn test_inverted_diff_base_text_change(cx: &mut TestAppContext) { .collect(); } +#[gpui::test] +async fn test_inverted_diff_secondary_version_mismatch(cx: &mut TestAppContext) { + let base_text = "one\ntwo\nthree\nfour\nfive\n"; + let index_text = "one\nTWO\nthree\nfour\nfive\n"; + let buffer_text = "one\nTWO\nthree\nFOUR\nfive\n"; + + let buffer = cx.new(|cx| Buffer::local(buffer_text, cx)); + + let unstaged_diff = cx + .new(|cx| BufferDiff::new_with_base_text(index_text, &buffer.read(cx).text_snapshot(), cx)); + cx.run_until_parked(); + + let uncommitted_diff = cx.new(|cx| { + let mut diff = + BufferDiff::new_with_base_text(base_text, &buffer.read(cx).text_snapshot(), cx); + diff.set_secondary_diff(unstaged_diff.clone()); + diff + }); + cx.run_until_parked(); + + buffer.update(cx, |buffer, cx| { + buffer.edit([(0..0, "ZERO\n")], None, cx); + }); + + let update = unstaged_diff + .update(cx, |diff, cx| { + diff.update_diff( + buffer.read(cx).text_snapshot(), + Some(index_text.into()), + None, + None, + cx, + ) + }) + .await; + unstaged_diff + .update(cx, |diff, cx| { + diff.set_snapshot(update, &buffer.read(cx).text_snapshot(), cx) + }) + .await; + + let base_text_buffer = + uncommitted_diff.read_with(cx, |diff, _| diff.base_text_buffer().clone()); + + let multibuffer = cx.new(|cx| { + let mut multibuffer = MultiBuffer::singleton(base_text_buffer.clone(), cx); + multibuffer.set_all_diff_hunks_expanded(cx); + multibuffer.add_inverted_diff(uncommitted_diff.clone(), buffer.clone(), cx); + multibuffer + }); + + let _hunks: Vec<_> = multibuffer + .read_with(cx, |multibuffer, cx| multibuffer.snapshot(cx)) + .diff_hunks() + .collect(); +} + #[track_caller] fn assert_excerpts_match( multibuffer: &Entity,