Fix some syncing issues with git statuses (#25535)

ConradIrwin · maxbrunsfeld · web-flow · commit 08539b32d0ae · 2025-02-25T22:09:02.000-07:00
Like the real app, this one infinite loops if you have a diff in an
UnsharedFile.

Release Notes:

- N/A *or* Added/Fixed/Improved ...

---------

Co-authored-by: Max Brunsfeld &lt;maxbrunsfeld@gmail.com&gt;
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/crates/collab/Cargo.toml b/crates/collab/Cargo.toml
@@ -97,6 +97,7 @@ extension.workspace = true
 file_finder.workspace = true
 fs = { workspace = true, features = ["test-support"] }
 git = { workspace = true, features = ["test-support"] }
+git_ui = { workspace = true, features = ["test-support"] }
 git_hosting_providers.workspace = true
 gpui = { workspace = true, features = ["test-support"] }
 hyper.workspace = true
diff --git a/crates/collab/src/tests.rs b/crates/collab/src/tests.rs
@@ -13,6 +13,7 @@ mod channel_message_tests;
 mod channel_tests;
 mod editor_tests;
 mod following_tests;
+mod git_tests;
 mod integration_tests;
 mod notification_tests;
 mod random_channel_buffer_tests;
diff --git a/crates/collab/src/tests/git_tests.rs b/crates/collab/src/tests/git_tests.rs
@@ -0,0 +1,130 @@
+use std::{
+    path::{Path, PathBuf},
+    sync::Arc,
+};
+
+use call::ActiveCall;
+use git::status::{FileStatus, StatusCode, TrackedStatus};
+use git_ui::project_diff::ProjectDiff;
+use gpui::{TestAppContext, VisualTestContext};
+use project::ProjectPath;
+use serde_json::json;
+use workspace::Workspace;
+
+//
+use crate::tests::TestServer;
+
+#[gpui::test]
+async fn test_project_diff(cx_a: &mut TestAppContext, cx_b: &mut TestAppContext) {
+    let mut server = TestServer::start(cx_a.background_executor.clone()).await;
+    let client_a = server.create_client(cx_a, "user_a").await;
+    let client_b = server.create_client(cx_b, "user_b").await;
+    cx_a.set_name("cx_a");
+    cx_b.set_name("cx_b");
+
+    server
+        .create_room(&mut [(&client_a, cx_a), (&client_b, cx_b)])
+        .await;
+
+    client_a
+        .fs()
+        .insert_tree(
+            "/a",
+            json!({
+                ".git": {},
+                "changed.txt": "after\n",
+                "unchanged.txt": "unchanged\n",
+                "created.txt": "created\n",
+                "secret.pem": "secret-changed\n",
+            }),
+        )
+        .await;
+
+    client_a.fs().set_git_content_for_repo(
+        Path::new("/a/.git"),
+        &[
+            ("changed.txt".into(), "before\n".to_string(), None),
+            ("unchanged.txt".into(), "unchanged\n".to_string(), None),
+            ("deleted.txt".into(), "deleted\n".to_string(), None),
+            ("secret.pem".into(), "shh\n".to_string(), None),
+        ],
+    );
+    let (project_a, worktree_id) = client_a.build_local_project("/a", cx_a).await;
+    let active_call_a = cx_a.read(ActiveCall::global);
+    let project_id = active_call_a
+        .update(cx_a, |call, cx| call.share_project(project_a.clone(), cx))
+        .await
+        .unwrap();
+
+    cx_b.update(editor::init);
+    cx_b.update(git_ui::init);
+    let project_b = client_b.join_remote_project(project_id, cx_b).await;
+    let workspace_b = cx_b.add_window(|window, cx| {
+        Workspace::new(
+            None,
+            project_b.clone(),
+            client_b.app_state.clone(),
+            window,
+            cx,
+        )
+    });
+    let cx_b = &mut VisualTestContext::from_window(*workspace_b, cx_b);
+    let workspace_b = workspace_b.root(cx_b).unwrap();
+
+    cx_b.update(|window, cx| {
+        window
+            .focused(cx)
+            .unwrap()
+            .dispatch_action(&git_ui::project_diff::Diff, window, cx)
+    });
+    let diff = workspace_b.update(cx_b, |workspace, cx| {
+        workspace.active_item(cx).unwrap().act_as::<ProjectDiff>(cx)
+    });
+    let diff = diff.unwrap();
+    cx_b.run_until_parked();
+
+    diff.update(cx_b, |diff, cx| {
+        assert_eq!(
+            diff.excerpt_paths(cx),
+            vec!["changed.txt", "deleted.txt", "created.txt"]
+        );
+    });
+
+    client_a
+        .fs()
+        .insert_tree(
+            "/a",
+            json!({
+                ".git": {},
+                "changed.txt": "before\n",
+                "unchanged.txt": "changed\n",
+                "created.txt": "created\n",
+                "secret.pem": "secret-changed\n",
+            }),
+        )
+        .await;
+    client_a.fs().recalculate_git_status(Path::new("/a/.git"));
+    cx_b.run_until_parked();
+
+    project_b.update(cx_b, |project, cx| {
+        let project_path = ProjectPath {
+            worktree_id,
+            path: Arc::from(PathBuf::from("unchanged.txt")),
+        };
+        let status = project.project_path_git_status(&project_path, cx);
+        assert_eq!(
+            status.unwrap(),
+            FileStatus::Tracked(TrackedStatus {
+                worktree_status: StatusCode::Modified,
+                index_status: StatusCode::Unmodified,
+            })
+        );
+    });
+
+    diff.update(cx_b, |diff, cx| {
+        assert_eq!(
+            diff.excerpt_paths(cx),
+            vec!["deleted.txt", "unchanged.txt", "created.txt"]
+        );
+    });
+}
diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs
@@ -5,12 +5,20 @@ mod mac_watcher;
 pub mod fs_watcher;
 
 use anyhow::{anyhow, Context as _, Result};
+#[cfg(any(test, feature = "test-support"))]
+use collections::HashMap;
+#[cfg(any(test, feature = "test-support"))]
+use git::status::StatusCode;
+#[cfg(any(test, feature = "test-support"))]
+use git::status::TrackedStatus;
 use git::GitHostingProviderRegistry;
 #[cfg(any(test, feature = "test-support"))]
 use git::{repository::RepoPath, status::FileStatus};
 
 #[cfg(any(target_os = "linux", target_os = "freebsd"))]
 use ashpd::desktop::trash;
+#[cfg(any(test, feature = "test-support"))]
+use std::collections::HashSet;
 #[cfg(unix)]
 use std::os::fd::AsFd;
 #[cfg(unix)]
@@ -1292,6 +1300,105 @@ impl FakeFs {
         });
     }
 
+    pub fn set_git_content_for_repo(
+        &self,
+        dot_git: &Path,
+        head_state: &[(RepoPath, String, Option<String>)],
+    ) {
+        self.with_git_state(dot_git, true, |state| {
+            state.head_contents.clear();
+            state.head_contents.extend(
+                head_state
+                    .iter()
+                    .map(|(path, head_content, _)| (path.clone(), head_content.clone())),
+            );
+            state.index_contents.clear();
+            state.index_contents.extend(head_state.iter().map(
+                |(path, head_content, index_content)| {
+                    (
+                        path.clone(),
+                        index_content.as_ref().unwrap_or(head_content).clone(),
+                    )
+                },
+            ));
+        });
+        self.recalculate_git_status(dot_git);
+    }
+
+    pub fn recalculate_git_status(&self, dot_git: &Path) {
+        let git_files: HashMap<_, _> = self
+            .files()
+            .iter()
+            .filter_map(|path| {
+                let repo_path =
+                    RepoPath::new(path.strip_prefix(dot_git.parent().unwrap()).ok()?.into());
+                let content = self
+                    .read_file_sync(path)
+                    .ok()
+                    .map(|content| String::from_utf8(content).unwrap());
+                Some((repo_path, content?))
+            })
+            .collect();
+        self.with_git_state(dot_git, false, |state| {
+            state.statuses.clear();
+            let mut paths: HashSet<_> = state.head_contents.keys().collect();
+            paths.extend(state.index_contents.keys());
+            paths.extend(git_files.keys());
+            for path in paths {
+                let head = state.head_contents.get(path);
+                let index = state.index_contents.get(path);
+                let fs = git_files.get(path);
+                let status = match (head, index, fs) {
+                    (Some(head), Some(index), Some(fs)) => FileStatus::Tracked(TrackedStatus {
+                        index_status: if head == index {
+                            StatusCode::Unmodified
+                        } else {
+                            StatusCode::Modified
+                        },
+                        worktree_status: if fs == index {
+                            StatusCode::Unmodified
+                        } else {
+                            StatusCode::Modified
+                        },
+                    }),
+                    (Some(head), Some(index), None) => FileStatus::Tracked(TrackedStatus {
+                        index_status: if head == index {
+                            StatusCode::Unmodified
+                        } else {
+                            StatusCode::Modified
+                        },
+                        worktree_status: StatusCode::Deleted,
+                    }),
+                    (Some(_), None, Some(_)) => FileStatus::Tracked(TrackedStatus {
+                        index_status: StatusCode::Deleted,
+                        worktree_status: StatusCode::Added,
+                    }),
+                    (Some(_), None, None) => FileStatus::Tracked(TrackedStatus {
+                        index_status: StatusCode::Deleted,
+                        worktree_status: StatusCode::Deleted,
+                    }),
+                    (None, Some(index), Some(fs)) => FileStatus::Tracked(TrackedStatus {
+                        index_status: StatusCode::Added,
+                        worktree_status: if fs == index {
+                            StatusCode::Unmodified
+                        } else {
+                            StatusCode::Modified
+                        },
+                    }),
+                    (None, Some(_), None) => FileStatus::Tracked(TrackedStatus {
+                        index_status: StatusCode::Added,
+                        worktree_status: StatusCode::Deleted,
+                    }),
+                    (None, None, Some(_)) => FileStatus::Untracked,
+                    (None, None, None) => {
+                        unreachable!();
+                    }
+                };
+                state.statuses.insert(path.clone(), status);
+            }
+        });
+    }
+
     pub fn set_blame_for_repo(&self, dot_git: &Path, blames: Vec<(RepoPath, git::blame::Blame)>) {
         self.with_git_state(dot_git, true, |state| {
             state.blames.clear();
diff --git a/crates/git_ui/Cargo.toml b/crates/git_ui/Cargo.toml
@@ -49,3 +49,4 @@ windows.workspace = true
 
 [features]
 default = []
+test-support = ["multi_buffer/test-support"]
diff --git a/crates/git_ui/src/project_diff.rs b/crates/git_ui/src/project_diff.rs
@@ -33,7 +33,7 @@ use crate::git_panel::{GitPanel, GitPanelAddon, GitStatusEntry};
 
 actions!(git, [Diff]);
 
-pub(crate) struct ProjectDiff {
+pub struct ProjectDiff {
     multibuffer: Entity<MultiBuffer>,
     editor: Entity<Editor>,
     project: Entity<Project>,
@@ -438,6 +438,15 @@ impl ProjectDiff {
 
         Ok(())
     }
+
+    #[cfg(any(test, feature = "test-support"))]
+    pub fn excerpt_paths(&self, cx: &App) -> Vec<String> {
+        self.multibuffer
+            .read(cx)
+            .excerpt_paths()
+            .map(|key| key.path().to_string_lossy().to_string())
+            .collect()
+    }
 }
 
 impl EventEmitter<EditorEvent> for ProjectDiff {}
diff --git a/crates/multi_buffer/src/multi_buffer.rs b/crates/multi_buffer/src/multi_buffer.rs
@@ -165,6 +165,10 @@ impl PathKey {
     pub fn namespaced(namespace: &'static str, path: Arc<Path>) -> Self {
         Self { namespace, path }
     }
+
+    pub fn path(&self) -> &Arc<Path> {
+        &self.path
+    }
 }
 
 pub type MultiBufferPoint = Point;
@@ -1453,6 +1457,11 @@ impl MultiBuffer {
             excerpt.range.context.start,
         ))
     }
+
+    pub fn excerpt_paths(&self) -> impl Iterator<Item = &PathKey> {
+        self.buffers_by_path.keys()
+    }
+
     /// Sets excerpts, returns `true` if at least one new excerpt was added.
     pub fn set_excerpts_for_path(
         &mut self,
diff --git a/crates/project/src/git.rs b/crates/project/src/git.rs
@@ -88,6 +88,7 @@ pub enum Message {
     Fetch(GitRepo),
 }
 
+#[derive(Debug)]
 pub enum GitEvent {
     ActiveRepositoryChanged,
     FileSystemUpdated,
diff --git a/crates/project/src/worktree_store.rs b/crates/project/src/worktree_store.rs
@@ -377,7 +377,7 @@ impl WorktreeStore {
             match event {
                 worktree::Event::UpdatedEntries(changes) => {
                     cx.emit(WorktreeStoreEvent::WorktreeUpdatedEntries(
-                        worktree.read(cx).id(),
+                        worktree_id,
                         changes.clone(),
                     ));
                 }
diff --git a/crates/worktree/src/worktree.rs b/crates/worktree/src/worktree.rs
@@ -827,17 +827,35 @@ impl Worktree {
             cx.spawn(|this, mut cx| async move {
                 while (snapshot_updated_rx.recv().await).is_some() {
                     this.update(&mut cx, |this, cx| {
+                        let mut git_repos_changed = false;
+                        let mut entries_changed = false;
                         let this = this.as_remote_mut().unwrap();
                         {
                             let mut lock = this.background_snapshot.lock();
                             this.snapshot = lock.0.clone();
-                            if let Some(tx) = &this.update_observer {
-                                for update in lock.1.drain(..) {
+                            for update in lock.1.drain(..) {
+                                if !update.updated_entries.is_empty()
+                                    || !update.removed_entries.is_empty()
+                                {
+                                    entries_changed = true;
+                                }
+                                if !update.updated_repositories.is_empty()
+                                    || !update.removed_repositories.is_empty()
+                                {
+                                    git_repos_changed = true;
+                                }
+                                if let Some(tx) = &this.update_observer {
                                     tx.unbounded_send(update).ok();
                                 }
                             }
                         };
-                        cx.emit(Event::UpdatedEntries(Arc::default()));
+
+                        if entries_changed {
+                            cx.emit(Event::UpdatedEntries(Arc::default()));
+                        }
+                        if git_repos_changed {
+                            cx.emit(Event::UpdatedGitRepositories(Arc::default()));
+                        }
                         cx.notify();
                         while let Some((scan_id, _)) = this.snapshot_subscriptions.front() {
                             if this.observed_snapshot(*scan_id) {
@@ -5451,7 +5469,6 @@ impl BackgroundScanner {
                 else {
                     return;
                 };
-
                 log::trace!(
                     "computed git statuses for repo {repository_name} in {:?}",
                     t0.elapsed()

Original file line number	Diff line number	Diff line change
`@@ -49,3 +49,4 @@ windows.workspace = true`
`49`	`49`
`50`	`50`	`[features]`
`51`	`51`	`default = []`
	`52`	`+test-support = ["multi_buffer/test-support"]`
Original file line number	Diff line number	Diff line change
`@@ -88,6 +88,7 @@ pub enum Message {`
`88`	`88`	`Fetch(GitRepo),`
`89`	`89`	`}`
`90`	`90`
	`91`	`+#[derive(Debug)]`
`91`	`92`	`pub enum GitEvent {`
`92`	`93`	`ActiveRepositoryChanged,`
`93`	`94`	`FileSystemUpdated,`
Original file line number	Diff line number	Diff line change
`@@ -377,7 +377,7 @@ impl WorktreeStore {`
`377`	`377`	`match event {`
`378`	`378`	`worktree::Event::UpdatedEntries(changes) => {`
`379`	`379`	`cx.emit(WorktreeStoreEvent::WorktreeUpdatedEntries(`
`380`		`- worktree.read(cx).id(),`
	`380`	`+ worktree_id,`
`381`	`381`	`changes.clone(),`
`382`	`382`	`));`
`383`	`383`	`}`