Fix bug in status detection when removing a directory

Mikayla Maki created

Change summary

crates/collab/src/tests/integration_tests.rs | 32 +++---
crates/fs/src/fs.rs                          |  8 
crates/fs/src/repository.rs                  | 62 ++++++--------
crates/project/src/worktree.rs               | 94 ++++++++++++---------
crates/project_panel/src/project_panel.rs    | 17 +--
5 files changed, 108 insertions(+), 105 deletions(-)

Detailed changes

crates/collab/src/tests/integration_tests.rs 🔗

@@ -10,7 +10,7 @@ use editor::{
     ConfirmRename, Editor, ExcerptRange, MultiBuffer, Redo, Rename, ToOffset, ToggleCodeActions,
     Undo,
 };
-use fs::{repository::GitStatus, FakeFs, Fs as _, LineEnding, RemoveOptions};
+use fs::{repository::GitFileStatus, FakeFs, Fs as _, LineEnding, RemoveOptions};
 use futures::StreamExt as _;
 use gpui::{
     executor::Deterministic, geometry::vector::vec2f, test::EmptyView, AppContext, ModelHandle,
@@ -2728,8 +2728,8 @@ async fn test_git_status_sync(
         .set_status_for_repo(
             Path::new("/dir/.git"),
             &[
-                (&Path::new(A_TXT), GitStatus::Added),
-                (&Path::new(B_TXT), GitStatus::Added),
+                (&Path::new(A_TXT), GitFileStatus::Added),
+                (&Path::new(B_TXT), GitFileStatus::Added),
             ],
         )
         .await;
@@ -2748,7 +2748,7 @@ async fn test_git_status_sync(
     deterministic.run_until_parked();
 
     #[track_caller]
-    fn assert_status(file: &impl AsRef<Path>, status: Option<GitStatus>, project: &Project, cx: &AppContext) {
+    fn assert_status(file: &impl AsRef<Path>, status: Option<GitFileStatus>, project: &Project, cx: &AppContext) {
         let file = file.as_ref();
         let worktrees = project.visible_worktrees(cx).collect::<Vec<_>>();
         assert_eq!(worktrees.len(), 1);
@@ -2760,12 +2760,12 @@ async fn test_git_status_sync(
 
     // Smoke test status reading
     project_local.read_with(cx_a, |project, cx| {
-        assert_status(&Path::new(A_TXT), Some(GitStatus::Added), project, cx);
-        assert_status(&Path::new(B_TXT), Some(GitStatus::Added), project, cx);
+        assert_status(&Path::new(A_TXT), Some(GitFileStatus::Added), project, cx);
+        assert_status(&Path::new(B_TXT), Some(GitFileStatus::Added), project, cx);
     });
     project_remote.read_with(cx_b, |project, cx| {
-        assert_status(&Path::new(A_TXT), Some(GitStatus::Added), project, cx);
-        assert_status(&Path::new(B_TXT), Some(GitStatus::Added), project, cx);
+        assert_status(&Path::new(A_TXT), Some(GitFileStatus::Added), project, cx);
+        assert_status(&Path::new(B_TXT), Some(GitFileStatus::Added), project, cx);
     });
 
     client_a
@@ -2774,8 +2774,8 @@ async fn test_git_status_sync(
         .set_status_for_repo(
             Path::new("/dir/.git"),
             &[
-                (&Path::new(A_TXT), GitStatus::Modified),
-                (&Path::new(B_TXT), GitStatus::Modified),
+                (&Path::new(A_TXT), GitFileStatus::Modified),
+                (&Path::new(B_TXT), GitFileStatus::Modified),
             ],
         )
         .await;
@@ -2785,19 +2785,19 @@ async fn test_git_status_sync(
 
     // Smoke test status reading
     project_local.read_with(cx_a, |project, cx| {
-        assert_status(&Path::new(A_TXT), Some(GitStatus::Added), project, cx);
-        assert_status(&Path::new(B_TXT), Some(GitStatus::Added), project, cx);
+        assert_status(&Path::new(A_TXT), Some(GitFileStatus::Added), project, cx);
+        assert_status(&Path::new(B_TXT), Some(GitFileStatus::Added), project, cx);
     });
     project_remote.read_with(cx_b, |project, cx| {
-        assert_status(&Path::new(A_TXT), Some(GitStatus::Added), project, cx);
-        assert_status(&Path::new(B_TXT), Some(GitStatus::Added), project, cx);
+        assert_status(&Path::new(A_TXT), Some(GitFileStatus::Added), project, cx);
+        assert_status(&Path::new(B_TXT), Some(GitFileStatus::Added), project, cx);
     });
 
     // And synchronization while joining
     let project_remote_c = client_c.build_remote_project(project_id, cx_c).await;
     project_remote_c.read_with(cx_c, |project, cx| {
-        assert_status(&Path::new(A_TXT), Some(GitStatus::Added), project, cx);
-        assert_status(&Path::new(B_TXT), Some(GitStatus::Added), project, cx);
+        assert_status(&Path::new(A_TXT), Some(GitFileStatus::Added), project, cx);
+        assert_status(&Path::new(B_TXT), Some(GitFileStatus::Added), project, cx);
     });
 }
 

crates/fs/src/fs.rs 🔗

@@ -7,7 +7,7 @@ use git2::Repository as LibGitRepository;
 use lazy_static::lazy_static;
 use parking_lot::Mutex;
 use regex::Regex;
-use repository::{GitRepository, GitStatus};
+use repository::{GitRepository, GitFileStatus};
 use rope::Rope;
 use smol::io::{AsyncReadExt, AsyncWriteExt};
 use std::borrow::Cow;
@@ -654,10 +654,10 @@ impl FakeFs {
         });
     }
 
-    pub async fn set_status_for_repo(&self, dot_git: &Path, statuses: &[(&Path, GitStatus)]) {
+    pub async fn set_status_for_repo(&self, dot_git: &Path, statuses: &[(&Path, GitFileStatus)]) {
         self.with_git_state(dot_git, |state| {
-            state.git_statuses.clear();
-            state.git_statuses.extend(
+            state.worktree_statuses.clear();
+            state.worktree_statuses.extend(
                 statuses
                     .iter()
                     .map(|(path, content)| {

crates/fs/src/repository.rs 🔗

@@ -1,6 +1,5 @@
 use anyhow::Result;
 use collections::HashMap;
-use git2::Status;
 use parking_lot::Mutex;
 use std::{
     ffi::OsStr,
@@ -21,9 +20,9 @@ pub trait GitRepository: Send {
 
     fn branch_name(&self) -> Option<String>;
 
-    fn statuses(&self) -> Option<TreeMap<RepoPath, GitStatus>>;
+    fn worktree_statuses(&self) -> Option<TreeMap<RepoPath, GitFileStatus>>;
 
-    fn file_status(&self, path: &RepoPath) -> Option<GitStatus>;
+    fn worktree_status(&self, path: &RepoPath) -> Option<GitFileStatus>;
 }
 
 impl std::fmt::Debug for dyn GitRepository {
@@ -70,7 +69,7 @@ impl GitRepository for LibGitRepository {
         Some(branch.to_string())
     }
 
-    fn statuses(&self) -> Option<TreeMap<RepoPath, GitStatus>> {
+    fn worktree_statuses(&self) -> Option<TreeMap<RepoPath, GitFileStatus>> {
         let statuses = self.statuses(None).log_err()?;
 
         let mut map = TreeMap::default();
@@ -80,17 +79,31 @@ impl GitRepository for LibGitRepository {
             .filter(|status| !status.status().contains(git2::Status::IGNORED))
         {
             let path = RepoPath(PathBuf::from(OsStr::from_bytes(status.path_bytes())));
+            let Some(status) = read_status(status.status()) else {
+                continue
+            };
 
-            map.insert(path, status.status().into())
+            map.insert(path, status)
         }
 
         Some(map)
     }
 
-    fn file_status(&self, path: &RepoPath) -> Option<GitStatus> {
+    fn worktree_status(&self, path: &RepoPath) -> Option<GitFileStatus> {
         let status = self.status_file(path).log_err()?;
+        read_status(status)
+    }
+}
 
-        Some(status.into())
+fn read_status(status: git2::Status) -> Option<GitFileStatus> {
+    if status.contains(git2::Status::CONFLICTED) {
+        Some(GitFileStatus::Conflict)
+    } else if status.intersects(git2::Status::WT_MODIFIED | git2::Status::WT_RENAMED) {
+        Some(GitFileStatus::Modified)
+    } else if status.intersects(git2::Status::WT_NEW) {
+        Some(GitFileStatus::Added)
+    } else {
+        None
     }
 }
 
@@ -102,7 +115,7 @@ pub struct FakeGitRepository {
 #[derive(Debug, Clone, Default)]
 pub struct FakeGitRepositoryState {
     pub index_contents: HashMap<PathBuf, String>,
-    pub git_statuses: HashMap<RepoPath, GitStatus>,
+    pub worktree_statuses: HashMap<RepoPath, GitFileStatus>,
     pub branch_name: Option<String>,
 }
 
@@ -126,18 +139,18 @@ impl GitRepository for FakeGitRepository {
         state.branch_name.clone()
     }
 
-    fn statuses(&self) -> Option<TreeMap<RepoPath, GitStatus>> {
+    fn worktree_statuses(&self) -> Option<TreeMap<RepoPath, GitFileStatus>> {
         let state = self.state.lock();
         let mut map = TreeMap::default();
-        for (repo_path, status) in state.git_statuses.iter() {
+        for (repo_path, status) in state.worktree_statuses.iter() {
             map.insert(repo_path.to_owned(), status.to_owned());
         }
         Some(map)
     }
 
-    fn file_status(&self, path: &RepoPath) -> Option<GitStatus> {
+    fn worktree_status(&self, path: &RepoPath) -> Option<GitFileStatus> {
         let state = self.state.lock();
-        state.git_statuses.get(path).cloned()
+        state.worktree_statuses.get(path).cloned()
     }
 }
 
@@ -170,32 +183,11 @@ fn check_path_to_repo_path_errors(relative_file_path: &Path) -> Result<()> {
     }
 }
 
-#[derive(Debug, Clone, Default, PartialEq, Eq)]
-pub enum GitStatus {
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum GitFileStatus {
     Added,
     Modified,
     Conflict,
-    #[default]
-    Untracked,
-}
-
-impl From<Status> for GitStatus {
-    fn from(value: Status) -> Self {
-        if value.contains(git2::Status::CONFLICTED) {
-            GitStatus::Conflict
-        } else if value.intersects(
-            git2::Status::INDEX_MODIFIED
-                | git2::Status::WT_MODIFIED
-                | git2::Status::INDEX_RENAMED
-                | git2::Status::WT_RENAMED,
-        ) {
-            GitStatus::Modified
-        } else if value.intersects(git2::Status::INDEX_NEW | git2::Status::WT_NEW) {
-            GitStatus::Added
-        } else {
-            GitStatus::Untracked
-        }
-    }
 }
 
 #[derive(Clone, Debug, Ord, Hash, PartialOrd, Eq, PartialEq)]

crates/project/src/worktree.rs 🔗

@@ -7,7 +7,7 @@ use client::{proto, Client};
 use clock::ReplicaId;
 use collections::{HashMap, VecDeque};
 use fs::{
-    repository::{GitRepository, GitStatus, RepoPath},
+    repository::{GitRepository, GitFileStatus, RepoPath},
     Fs, LineEnding,
 };
 use futures::{
@@ -143,7 +143,7 @@ impl Snapshot {
 pub struct RepositoryEntry {
     pub(crate) work_directory: WorkDirectoryEntry,
     pub(crate) branch: Option<Arc<str>>,
-    pub(crate) statuses: TreeMap<RepoPath, GitStatus>,
+    pub(crate) worktree_statuses: TreeMap<RepoPath, GitFileStatus>,
 }
 
 impl RepositoryEntry {
@@ -165,10 +165,10 @@ impl RepositoryEntry {
         self.work_directory.contains(snapshot, path)
     }
 
-    pub fn status_for(&self, snapshot: &Snapshot, path: &Path) -> Option<GitStatus> {
+    pub fn status_for(&self, snapshot: &Snapshot, path: &Path) -> Option<GitFileStatus> {
         self.work_directory
             .relativize(snapshot, path)
-            .and_then(|repo_path| self.statuses.get(&repo_path))
+            .and_then(|repo_path| self.worktree_statuses.get(&repo_path))
             .cloned()
     }
 }
@@ -1445,7 +1445,7 @@ impl Snapshot {
                 work_directory: ProjectEntryId::from_proto(repository.work_directory_id).into(),
                 branch: repository.branch.map(Into::into),
                 // TODO: status
-                statuses: Default::default(),
+                worktree_statuses: Default::default(),
             };
             if let Some(entry) = self.entry_for_id(repository.work_directory_id()) {
                 self.repository_entries
@@ -1864,7 +1864,7 @@ impl LocalSnapshot {
                 RepositoryEntry {
                     work_directory: work_dir_id.into(),
                     branch: repo_lock.branch_name().map(Into::into),
-                    statuses: repo_lock.statuses().unwrap_or_default(),
+                    worktree_statuses: repo_lock.worktree_statuses().unwrap_or_default(),
                 },
             );
             drop(repo_lock);
@@ -2896,9 +2896,12 @@ impl BackgroundScanner {
                 .git_repositories
                 .update(&work_dir_id, |entry| entry.scan_id = scan_id);
 
+            // TODO: Status Replace linear scan with smarter sum tree traversal
             snapshot
                 .repository_entries
-                .update(&work_dir, |entry| entry.statuses.remove(&repo_path));
+                .update(&work_dir, |entry| entry.worktree_statuses.retain(|stored_path, _| {
+                    !stored_path.starts_with(&repo_path)
+                }));
         }
 
         Some(())
@@ -2920,7 +2923,7 @@ impl BackgroundScanner {
             let repo = repo.lock();
             repo.reload_index();
             let branch = repo.branch_name();
-            let statuses = repo.statuses().unwrap_or_default();
+            let statuses = repo.worktree_statuses().unwrap_or_default();
 
             snapshot.git_repositories.update(&entry_id, |entry| {
                 entry.scan_id = scan_id;
@@ -2929,7 +2932,7 @@ impl BackgroundScanner {
 
             snapshot.repository_entries.update(&work_dir, |entry| {
                 entry.branch = branch.map(Into::into);
-                entry.statuses = statuses;
+                entry.worktree_statuses = statuses;
             });
         } else {
             let repo = snapshot.repo_for(&path)?;
@@ -2945,21 +2948,19 @@ impl BackgroundScanner {
                 }
 
                 let git_ptr = local_repo.repo_ptr.lock();
-                git_ptr.file_status(&repo_path)?
+                git_ptr.worktree_status(&repo_path)?
             };
 
-            if status != GitStatus::Untracked {
-                let work_dir = repo.work_directory(snapshot)?;
-                let work_dir_id = repo.work_directory;
+            let work_dir = repo.work_directory(snapshot)?;
+            let work_dir_id = repo.work_directory;
 
-                snapshot
-                    .git_repositories
-                    .update(&work_dir_id, |entry| entry.scan_id = scan_id);
+            snapshot
+                .git_repositories
+                .update(&work_dir_id, |entry| entry.scan_id = scan_id);
 
-                snapshot
-                    .repository_entries
-                    .update(&work_dir, |entry| entry.statuses.insert(repo_path, status));
-            }
+            snapshot
+                .repository_entries
+                .update(&work_dir, |entry| entry.worktree_statuses.insert(repo_path, status));
         }
 
         Some(())
@@ -3848,6 +3849,11 @@ mod tests {
             "project": {
                 "a.txt": "a",
                 "b.txt": "bb",
+                "c": {
+                    "d": {
+                        "e.txt": "eee"
+                    }
+                }
             },
 
         }));
@@ -3865,18 +3871,22 @@ mod tests {
         .await
         .unwrap();
 
+        cx.read(|cx| tree.read(cx).as_local().unwrap().scan_complete())
+            .await;
+
         const A_TXT: &'static str = "a.txt";
         const B_TXT: &'static str = "b.txt";
+        const E_TXT: &'static str = "c/d/e.txt";
+
         let work_dir = root.path().join("project");
 
         let mut repo = git_init(work_dir.as_path());
         git_add(Path::new(A_TXT), &repo);
+        git_add(Path::new(E_TXT), &repo);
         git_commit("Initial commit", &repo);
 
         std::fs::write(work_dir.join(A_TXT), "aa").unwrap();
 
-        cx.read(|cx| tree.read(cx).as_local().unwrap().scan_complete())
-            .await;
         tree.flush_fs_events(cx).await;
 
         // Check that the right git state is observed on startup
@@ -3886,14 +3896,14 @@ mod tests {
             let (dir, repo) = snapshot.repository_entries.iter().next().unwrap();
             assert_eq!(dir.0.as_ref(), Path::new("project"));
 
-            assert_eq!(repo.statuses.iter().count(), 2);
+            assert_eq!(repo.worktree_statuses.iter().count(), 2);
             assert_eq!(
-                repo.statuses.get(&Path::new(A_TXT).into()),
-                Some(&GitStatus::Modified)
+                repo.worktree_statuses.get(&Path::new(A_TXT).into()),
+                Some(&GitFileStatus::Modified)
             );
             assert_eq!(
-                repo.statuses.get(&Path::new(B_TXT).into()),
-                Some(&GitStatus::Added)
+                repo.worktree_statuses.get(&Path::new(B_TXT).into()),
+                Some(&GitFileStatus::Added)
             );
         });
 
@@ -3907,14 +3917,15 @@ mod tests {
             let snapshot = tree.snapshot();
             let (_, repo) = snapshot.repository_entries.iter().next().unwrap();
 
-            assert_eq!(repo.statuses.iter().count(), 0);
-            assert_eq!(repo.statuses.get(&Path::new(A_TXT).into()), None);
-            assert_eq!(repo.statuses.get(&Path::new(B_TXT).into()), None);
+            assert_eq!(repo.worktree_statuses.iter().count(), 0);
+            assert_eq!(repo.worktree_statuses.get(&Path::new(A_TXT).into()), None);
+            assert_eq!(repo.worktree_statuses.get(&Path::new(B_TXT).into()), None);
         });
 
         git_reset(0, &repo);
         git_remove_index(Path::new(B_TXT), &repo);
         git_stash(&mut repo);
+        std::fs::write(work_dir.join(E_TXT), "eeee").unwrap();
         tree.flush_fs_events(cx).await;
 
         // Check that more complex repo changes are tracked
@@ -3922,17 +3933,21 @@ mod tests {
             let snapshot = tree.snapshot();
             let (_, repo) = snapshot.repository_entries.iter().next().unwrap();
 
-            dbg!(&repo.statuses);
-
-            assert_eq!(repo.statuses.iter().count(), 1);
-            assert_eq!(repo.statuses.get(&Path::new(A_TXT).into()), None);
+            assert_eq!(repo.worktree_statuses.iter().count(), 2);
+            assert_eq!(repo.worktree_statuses.get(&Path::new(A_TXT).into()), None);
             assert_eq!(
-                repo.statuses.get(&Path::new(B_TXT).into()),
-                Some(&GitStatus::Added)
+                repo.worktree_statuses.get(&Path::new(B_TXT).into()),
+                Some(&GitFileStatus::Added)
+            );
+            assert_eq!(
+                repo.worktree_statuses.get(&Path::new(E_TXT).into()),
+                Some(&GitFileStatus::Modified)
             );
         });
 
         std::fs::remove_file(work_dir.join(B_TXT)).unwrap();
+        std::fs::remove_dir_all(work_dir.join("c")).unwrap();
+
         tree.flush_fs_events(cx).await;
 
         // Check that non-repo behavior is tracked
@@ -3940,9 +3955,10 @@ mod tests {
             let snapshot = tree.snapshot();
             let (_, repo) = snapshot.repository_entries.iter().next().unwrap();
 
-            assert_eq!(repo.statuses.iter().count(), 0);
-            assert_eq!(repo.statuses.get(&Path::new(A_TXT).into()), None);
-            assert_eq!(repo.statuses.get(&Path::new(B_TXT).into()), None);
+            assert_eq!(repo.worktree_statuses.iter().count(), 0);
+            assert_eq!(repo.worktree_statuses.get(&Path::new(A_TXT).into()), None);
+            assert_eq!(repo.worktree_statuses.get(&Path::new(B_TXT).into()), None);
+            assert_eq!(repo.worktree_statuses.get(&Path::new(E_TXT).into()), None);
         });
     }
 

crates/project_panel/src/project_panel.rs 🔗

@@ -17,7 +17,7 @@ use gpui::{
 };
 use menu::{Confirm, SelectNext, SelectPrev};
 use project::{
-    repository::GitStatus, Entry, EntryKind, Project, ProjectEntryId, ProjectPath, Worktree,
+    repository::GitFileStatus, Entry, EntryKind, Project, ProjectEntryId, ProjectPath, Worktree,
     WorktreeId,
 };
 use settings::Settings;
@@ -89,7 +89,7 @@ pub struct EntryDetails {
     is_editing: bool,
     is_processing: bool,
     is_cut: bool,
-    git_status: Option<GitStatus>,
+    git_status: Option<GitFileStatus>,
 }
 
 actions!(
@@ -1081,19 +1081,14 @@ impl ProjectPanel {
 
         // Prepare colors for git statuses
         let editor_theme = &cx.global::<Settings>().theme.editor;
-        let color_for_added = Some(editor_theme.diff.inserted);
-        let color_for_modified = Some(editor_theme.diff.modified);
-        let color_for_conflict = Some(editor_theme.diff.deleted);
-        let color_for_untracked = None;
         let mut filename_text_style = style.text.clone();
         filename_text_style.color = details
             .git_status
             .as_ref()
-            .and_then(|status| match status {
-                GitStatus::Added => color_for_added,
-                GitStatus::Modified => color_for_modified,
-                GitStatus::Conflict => color_for_conflict,
-                GitStatus::Untracked => color_for_untracked,
+            .map(|status| match status {
+                GitFileStatus::Added => editor_theme.diff.inserted,
+                GitFileStatus::Modified => editor_theme.diff.modified,
+                GitFileStatus::Conflict => editor_theme.diff.deleted,
             })
             .unwrap_or(style.text.color);