From 23a19d85b817a70474f7ac2f9588191b46b7449a Mon Sep 17 00:00:00 2001 From: Mikayla Maki Date: Wed, 10 May 2023 09:55:10 -0700 Subject: [PATCH] Fix bug in status detection when removing a directory --- 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(-) diff --git a/crates/collab/src/tests/integration_tests.rs b/crates/collab/src/tests/integration_tests.rs index 764f070f0b..185e6c6354 100644 --- a/crates/collab/src/tests/integration_tests.rs +++ b/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, status: Option, project: &Project, cx: &AppContext) { + fn assert_status(file: &impl AsRef, status: Option, project: &Project, cx: &AppContext) { let file = file.as_ref(); let worktrees = project.visible_worktrees(cx).collect::>(); 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); }); } diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index efc24553c4..fd094160f5 100644 --- a/crates/fs/src/fs.rs +++ b/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)| { diff --git a/crates/fs/src/repository.rs b/crates/fs/src/repository.rs index 7fa20bddcb..3fb562570e 100644 --- a/crates/fs/src/repository.rs +++ b/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; - fn statuses(&self) -> Option>; + fn worktree_statuses(&self) -> Option>; - fn file_status(&self, path: &RepoPath) -> Option; + fn worktree_status(&self, path: &RepoPath) -> Option; } impl std::fmt::Debug for dyn GitRepository { @@ -70,7 +69,7 @@ impl GitRepository for LibGitRepository { Some(branch.to_string()) } - fn statuses(&self) -> Option> { + fn worktree_statuses(&self) -> Option> { 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 { + fn worktree_status(&self, path: &RepoPath) -> Option { let status = self.status_file(path).log_err()?; + read_status(status) + } +} - Some(status.into()) +fn read_status(status: git2::Status) -> Option { + 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, - pub git_statuses: HashMap, + pub worktree_statuses: HashMap, pub branch_name: Option, } @@ -126,18 +139,18 @@ impl GitRepository for FakeGitRepository { state.branch_name.clone() } - fn statuses(&self) -> Option> { + fn worktree_statuses(&self) -> Option> { 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 { + fn worktree_status(&self, path: &RepoPath) -> Option { 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 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)] diff --git a/crates/project/src/worktree.rs b/crates/project/src/worktree.rs index 82c719f31e..1fc4fcf5a8 100644 --- a/crates/project/src/worktree.rs +++ b/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>, - pub(crate) statuses: TreeMap, + pub(crate) worktree_statuses: TreeMap, } impl RepositoryEntry { @@ -165,10 +165,10 @@ impl RepositoryEntry { self.work_directory.contains(snapshot, path) } - pub fn status_for(&self, snapshot: &Snapshot, path: &Path) -> Option { + pub fn status_for(&self, snapshot: &Snapshot, path: &Path) -> Option { 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); }); } diff --git a/crates/project_panel/src/project_panel.rs b/crates/project_panel/src/project_panel.rs index 16b6232e8b..49741ea49f 100644 --- a/crates/project_panel/src/project_panel.rs +++ b/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, + git_status: Option, } actions!( @@ -1081,19 +1081,14 @@ impl ProjectPanel { // Prepare colors for git statuses let editor_theme = &cx.global::().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);