git: Fix diff hunks not being removed on restore in remote projects (#54823)

Cole Miller created 4 days ago

Closes #48032 

When restoring a diff hunk, we first unstage it unconditionally. That
unstaging operation is a no-op in terms of the index text if the hunk
was already not staged, but previously we would still always do
`spawn_set_index_text_job` and bump the
`hunk_staging_operation_count_as_of_write`. Bumping that count in turn
causes us to skip a diff recalculation in response to the change in the
buffer's text. That works out fine in the local case, because when the
worktree picks up the write to `.git/index` we kick off another diff
recalculation which is not skipped. But in the remote case, we don't get
an `UpdateDiffBases` proto message if the index text didn't actually
change, so there is no subsequent diff calculation to do the cleanup,
and we end up with a stale no-op hunk.

This PR fixes the issue by skipping the write to the index and the
`hunk_staging_operation_count_as_of_write` bump if the new and old index
texts are the same.

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

Closes #ISSUE

Release Notes:

- Fixed a bug where restoring diff hunks in remote projects would leave
stale no-op hunks in the UI.

Change summary

crates/editor/src/folding_ranges.rs              |   8 
crates/editor/src/semantic_tokens.rs             |   8 +
crates/project/src/git_store.rs                  |   6 
crates/remote_server/src/remote_editing_tests.rs | 109 ++++++++++++++++++
4 files changed, 126 insertions(+), 5 deletions(-)

Detailed changes

crates/editor/src/folding_ranges.rs

@@ -16,7 +16,7 @@ impl Editor {
         if !self.lsp_data_enabled() || !self.use_document_folding_ranges {
             return;
         }
-        let Some(project) = self.project.clone() else {
+        let Some(project) = self.project.as_ref().map(|p| p.downgrade()) else {
             return;
         };
 
@@ -43,7 +43,8 @@ impl Editor {
 
             let Some(tasks) = editor
                 .update(cx, |_, cx| {
-                    project.read(cx).lsp_store().update(cx, |lsp_store, cx| {
+                    let project = project.upgrade()?;
+                    Some(project.read(cx).lsp_store().update(cx, |lsp_store, cx| {
                         buffers_to_query
                             .into_iter()
                             .map(|buffer| {
@@ -52,9 +53,10 @@ impl Editor {
                                 async move { (buffer_id, task.await) }
                             })
                             .collect::<Vec<_>>()
-                    })
+                    }))
                 })
                 .ok()
+                .flatten()
             else {
                 return;
             };

crates/editor/src/semantic_tokens.rs

@@ -142,7 +142,10 @@ impl Editor {
             );
         }
 
-        let Some((sema, project)) = self.semantics_provider.clone().zip(self.project.clone())
+        let Some((sema, project)) = self
+            .semantics_provider
+            .clone()
+            .zip(self.project.as_ref().map(|p| p.downgrade()))
         else {
             return;
         };
@@ -283,6 +286,9 @@ impl Editor {
                             .buffer(buffer_id)
                             .and_then(|buf| buf.read(cx).language().map(|l| l.name()));
 
+                        let Some(project) = project.upgrade() else {
+                            return;
+                        };
                         editor.display_map.update(cx, |display_map, cx| {
                             project.read(cx).lsp_store().update(cx, |lsp_store, cx| {
                                 let mut token_highlights = Vec::new();

crates/project/src/git_store.rs

@@ -1848,6 +1848,10 @@ impl GitStore {
         if let BufferDiffEvent::HunksStagedOrUnstaged(new_index_text) = event {
             let buffer_id = diff.read(cx).buffer_id;
             if let Some(diff_state) = self.diffs.get(&buffer_id) {
+                let new_index_text = new_index_text.as_ref().map(|rope| rope.to_string());
+                if new_index_text.as_deref() == diff_state.read(cx).index_text.as_deref() {
+                    return;
+                }
                 let hunk_staging_operation_count = diff_state.update(cx, |diff_state, _| {
                     diff_state.hunk_staging_operation_count += 1;
                     diff_state.hunk_staging_operation_count
@@ -1857,7 +1861,7 @@ impl GitStore {
                         log::debug!("hunks changed for {}", path.as_unix_str());
                         repo.spawn_set_index_text_job(
                             path,
-                            new_index_text.as_ref().map(|rope| rope.to_string()),
+                            new_index_text,
                             Some(hunk_staging_operation_count),
                             cx,
                         )

crates/remote_server/src/remote_editing_tests.rs

@@ -2624,6 +2624,115 @@ async fn test_remote_apply_code_action_skips_unadvertised_command(
     assert_eq!(transaction.0.len(), 0);
 }
 
+#[gpui::test]
+async fn test_remote_restore_unstaged_hunk_clears_diff(
+    cx: &mut TestAppContext,
+    server_cx: &mut TestAppContext,
+) {
+    cx.update(|cx| {
+        let settings_store = SettingsStore::test(cx);
+        cx.set_global(settings_store);
+        theme_settings::init(theme::LoadThemes::JustBase, cx);
+        release_channel::init(semver::Version::new(0, 0, 0), cx);
+        editor::init(cx);
+    });
+
+    use editor::Editor;
+    use gpui::VisualContext;
+
+    let base_text = "
+        fn one() -> usize {
+            1
+        }
+    "
+    .unindent();
+    let modified_text = "
+        fn one() -> usize {
+            100
+        }
+    "
+    .unindent();
+
+    let fs = FakeFs::new(server_cx.executor());
+    fs.insert_tree(
+        path!("/code"),
+        json!({
+            "project1": {
+                ".git": {},
+                "src": {
+                    "lib.rs": modified_text
+                },
+            },
+        }),
+    )
+    .await;
+    fs.set_index_for_repo(
+        Path::new(path!("/code/project1/.git")),
+        &[("src/lib.rs", base_text.clone())],
+    );
+    fs.set_head_for_repo(
+        Path::new(path!("/code/project1/.git")),
+        &[("src/lib.rs", base_text.clone())],
+        "deadbeef",
+    );
+
+    let (project, _headless) = init_test(&fs, cx, server_cx).await;
+    let worktree_id = {
+        let (worktree, _) = project
+            .update(cx, |project, cx| {
+                project.find_or_create_worktree(path!("/code/project1"), true, cx)
+            })
+            .await
+            .unwrap();
+        cx.update(|cx| worktree.read(cx).id())
+    };
+    cx.executor().run_until_parked();
+
+    let buffer = project
+        .update(cx, |project, cx| {
+            project.open_buffer((worktree_id, rel_path("src/lib.rs")), cx)
+        })
+        .await
+        .unwrap();
+
+    let cx = cx.add_empty_window();
+    let editor = cx.new_window_entity(|window, cx| {
+        Editor::for_buffer(buffer, Some(project.clone()), window, cx)
+    });
+    cx.executor().run_until_parked();
+
+    editor.update_in(cx, |editor, window, cx| {
+        let snapshot = editor.snapshot(window, cx);
+        let hunks: Vec<_> = editor
+            .diff_hunks_in_ranges(
+                &[editor::Anchor::Min..editor::Anchor::Max],
+                &snapshot.buffer_snapshot(),
+            )
+            .collect();
+        assert!(!hunks.is_empty(), "should have diff hunks before restore");
+    });
+
+    cx.update_window_entity(&editor, |editor, window, cx| {
+        editor.select_all(&editor::actions::SelectAll, window, cx);
+        editor.git_restore(&git::Restore, window, cx);
+    });
+    cx.executor().run_until_parked();
+
+    editor.update_in(cx, |editor, _window, cx| {
+        let snapshot = editor.buffer().read(cx).snapshot(cx);
+        assert_eq!(
+            snapshot.text(),
+            base_text,
+            "buffer text should match base after restoring all hunks"
+        );
+
+        let hunks: Vec<_> = editor
+            .diff_hunks_in_ranges(&[editor::Anchor::Min..editor::Anchor::Max], &snapshot)
+            .collect();
+        assert!(hunks.is_empty(), "should have no diff hunks after restore");
+    });
+}
+
 pub async fn init_test(
     server_fs: &Arc<FakeFs>,
     cx: &mut TestAppContext,