From 652f1fa3b08e0ef906f017ba9e474fb357d9f8e0 Mon Sep 17 00:00:00 2001 From: Cole Miller Date: Tue, 14 Apr 2026 17:08:16 -0400 Subject: [PATCH] git: Fix a race that caused incorrect hunk staging information (#53929) In `GitStore::open_diff_internal`, when opening a `DiffKind::Unstaged` diff, we previously always overwrote the `unstaged_diff` field of `BufferGitState`. This causes a bug in the following situation: - We call `open_unstaged_diff`, and it hits an await point where it is loading the index text - While that task is suspended, we call `open_uncommitted_diff`. It reaches `open_diff_internal` and sets `buffer_git_state.uncommitted_diff` and `buffer_git_state.unstaged_diff`. It also sets the secondary diff for the uncommitted diff to be the unstaged diff that it just opened. - The `open_unstaged_diff` task wakes up and enters `open_diff_internal`. It creates a new entity for the unstaged diff and overwrites `buffer_git_state.unstaged_diff` with it, but it doesn't update the secondary diff of the uncommitted diff, which still has the entity that was produced in the previous step. - Now the uncommitted diff's secondary diff will never receive updates from the `GitStore`, causing staging information to be incorrect. The fix is for `open_diff_internal` to not overwrite an existing unstaged diff. Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - Fixed a bug that could cause diff hunks to have an incorrect staged status. --- crates/project/src/git_store.rs | 4 +- .../tests/integration/project_tests.rs | 62 +++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index ef81f8030eb87366738db55850e401bbaa3d716f..e917f7bd3a7167f37980e6ef581a81a70530b56e 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -998,7 +998,9 @@ impl GitStore { diff_state.language_registry = language_registry; match kind { - DiffKind::Unstaged => diff_state.unstaged_diff = Some(diff.downgrade()), + DiffKind::Unstaged => { + diff_state.unstaged_diff.get_or_insert(diff.downgrade()); + } DiffKind::Uncommitted => { let unstaged_diff = if let Some(diff) = diff_state.unstaged_diff() { diff diff --git a/crates/project/tests/integration/project_tests.rs b/crates/project/tests/integration/project_tests.rs index f680ccee78e997064af2647f68d8aa3631fa4bd3..a26e95fdb9de20a35d92b85f5d7abb21047d2520 100644 --- a/crates/project/tests/integration/project_tests.rs +++ b/crates/project/tests/integration/project_tests.rs @@ -9090,6 +9090,68 @@ async fn test_staging_hunks(cx: &mut gpui::TestAppContext) { }); } +#[gpui::test(iterations = 10)] +async fn test_uncommitted_diff_opened_before_unstaged_diff(cx: &mut gpui::TestAppContext) { + use DiffHunkSecondaryStatus::*; + init_test(cx); + + let committed_contents = "one\ntwo\nthree\n"; + let file_contents = "one\nTWO\nthree\n"; + + let fs = FakeFs::new(cx.background_executor.clone()); + fs.insert_tree( + "/dir", + json!({ + ".git": {}, + "file.txt": file_contents, + }), + ) + .await; + fs.set_head_and_index_for_repo( + path!("/dir/.git").as_ref(), + &[("file.txt", committed_contents.into())], + ); + + let project = Project::test(fs.clone(), ["/dir".as_ref()], cx).await; + let buffer = project + .update(cx, |project, cx| { + project.open_local_buffer("/dir/file.txt", cx) + }) + .await + .unwrap(); + + let uncommitted_diff_task = project.update(cx, |project, cx| { + project.open_uncommitted_diff(buffer.clone(), cx) + }); + let unstaged_diff_task = project.update(cx, |project, cx| { + project.open_unstaged_diff(buffer.clone(), cx) + }); + let (uncommitted_diff, _unstaged_diff) = + futures::future::join(uncommitted_diff_task, unstaged_diff_task).await; + let uncommitted_diff = uncommitted_diff.unwrap(); + let _unstaged_diff = _unstaged_diff.unwrap(); + + cx.run_until_parked(); + + uncommitted_diff.read_with(cx, |diff, cx| { + let snapshot = buffer.read(cx).snapshot(); + assert_hunks( + diff.snapshot(cx).hunks_intersecting_range( + Anchor::min_max_range_for_buffer(snapshot.remote_id()), + &snapshot, + ), + &snapshot, + &diff.base_text_string(cx).unwrap(), + &[( + 1..2, + "two\n", + "TWO\n", + DiffHunkStatus::modified(HasSecondaryHunk), + )], + ); + }); +} + #[gpui::test(seeds(340, 472))] async fn test_staging_hunks_with_delayed_fs_event(cx: &mut gpui::TestAppContext) { use DiffHunkSecondaryStatus::*;