git: Fix a race that caused incorrect hunk staging information (#53929)

Cole Miller created 1 week ago

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.

Change summary

crates/project/src/git_store.rs                   |  4 
crates/project/tests/integration/project_tests.rs | 62 +++++++++++++++++
2 files changed, 65 insertions(+), 1 deletion(-)

Detailed changes

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

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::*;