From fa604153d937e45c1e7a6c3ca35827fc05e998fb Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Fri, 30 Jun 2023 11:40:49 -0700 Subject: [PATCH] Fix regression in handling git FS events (#2670) As part of an optimization in https://github.com/zed-industries/zed/pull/2663, I changed the way that the worktree ignores FS events within unloaded directories. But this accidentally prevented us from detecting some events that occur inside of `.git` directories. In this PR, I've made further tweaks to which FS events we can ignore. We now explicitly opt *in* to scanning `.git` (shallowly) directories (even though they are ignored). Note that we still don't recursively scan the git directory (including all of the files inside `objects` etc). This seems like the correct amount of work to do, and from my testing (and our unit tests that use the real FS and real git repositories), it seems to work correctly. Release Notes: - Fixed a bug where Zed would not detect some git repository changes (preview only). --- crates/project/src/worktree.rs | 80 +++++++++++++++------------- crates/project/src/worktree_tests.rs | 50 ++++++++--------- 2 files changed, 69 insertions(+), 61 deletions(-) diff --git a/crates/project/src/worktree.rs b/crates/project/src/worktree.rs index 20e693770f45f686a880b5aebd542eb74a96202e..2f793c84c97c9cf1d51071218e7c552641db4703 100644 --- a/crates/project/src/worktree.rs +++ b/crates/project/src/worktree.rs @@ -2140,6 +2140,7 @@ impl LocalSnapshot { impl BackgroundScannerState { fn should_scan_directory(&self, entry: &Entry) -> bool { (!entry.is_external && !entry.is_ignored) + || entry.path.file_name() == Some(&*DOT_GIT) || self.scanned_dirs.contains(&entry.id) // If we've ever scanned it, keep scanning || self .paths_to_scan @@ -2319,6 +2320,7 @@ impl BackgroundScannerState { .entry_for_id(entry_id) .map(|entry| RepositoryWorkDirectory(entry.path.clone())) else { continue }; + log::info!("reload git repository {:?}", dot_git_dir); let repository = repository.repo_ptr.lock(); let branch = repository.branch_name(); repository.reload_index(); @@ -2359,6 +2361,8 @@ impl BackgroundScannerState { } fn build_repository(&mut self, dot_git_path: Arc, fs: &dyn Fs) -> Option<()> { + log::info!("build git repository {:?}", dot_git_path); + let work_dir_path: Arc = dot_git_path.parent().unwrap().into(); // Guard against repositories inside the repository metadata @@ -3138,8 +3142,6 @@ impl BackgroundScanner { } async fn process_events(&mut self, mut abs_paths: Vec) { - log::debug!("received fs events {:?}", abs_paths); - let root_path = self.state.lock().snapshot.abs_path.clone(); let root_canonical_path = match self.fs.canonicalize(&root_path).await { Ok(path) => path, @@ -3150,7 +3152,6 @@ impl BackgroundScanner { }; let mut relative_paths = Vec::with_capacity(abs_paths.len()); - let mut unloaded_relative_paths = Vec::new(); abs_paths.sort_unstable(); abs_paths.dedup_by(|a, b| a.starts_with(&b)); abs_paths.retain(|abs_path| { @@ -3173,7 +3174,6 @@ impl BackgroundScanner { }); if !parent_dir_is_loaded { log::debug!("ignoring event {relative_path:?} within unloaded directory"); - unloaded_relative_paths.push(relative_path); return false; } @@ -3182,27 +3182,30 @@ impl BackgroundScanner { } }); - if !relative_paths.is_empty() { - let (scan_job_tx, scan_job_rx) = channel::unbounded(); - self.reload_entries_for_paths( - root_path, - root_canonical_path, - &relative_paths, - abs_paths, - Some(scan_job_tx.clone()), - ) - .await; - drop(scan_job_tx); - self.scan_dirs(false, scan_job_rx).await; - - let (scan_job_tx, scan_job_rx) = channel::unbounded(); - self.update_ignore_statuses(scan_job_tx).await; - self.scan_dirs(false, scan_job_rx).await; + if relative_paths.is_empty() { + return; } + log::debug!("received fs events {:?}", relative_paths); + + let (scan_job_tx, scan_job_rx) = channel::unbounded(); + self.reload_entries_for_paths( + root_path, + root_canonical_path, + &relative_paths, + abs_paths, + Some(scan_job_tx.clone()), + ) + .await; + drop(scan_job_tx); + self.scan_dirs(false, scan_job_rx).await; + + let (scan_job_tx, scan_job_rx) = channel::unbounded(); + self.update_ignore_statuses(scan_job_tx).await; + self.scan_dirs(false, scan_job_rx).await; + { let mut state = self.state.lock(); - relative_paths.extend(unloaded_relative_paths); state.reload_repositories(&relative_paths, self.fs.as_ref()); state.snapshot.completed_scan_id = state.snapshot.scan_id; for (_, entry_id) in mem::take(&mut state.removed_entry_ids) { @@ -3610,23 +3613,28 @@ impl BackgroundScanner { } } - let fs_entry = state.insert_entry(fs_entry, self.fs.as_ref()); - - if let Some(scan_queue_tx) = &scan_queue_tx { - let mut ancestor_inodes = state.snapshot.ancestor_inodes_for_path(&path); - if metadata.is_dir && !ancestor_inodes.contains(&metadata.inode) { - ancestor_inodes.insert(metadata.inode); - smol::block_on(scan_queue_tx.send(ScanJob { - abs_path, - path: path.clone(), - ignore_stack, - ancestor_inodes, - is_external: fs_entry.is_external, - scan_queue: scan_queue_tx.clone(), - })) - .unwrap(); + if let (Some(scan_queue_tx), true) = (&scan_queue_tx, fs_entry.is_dir()) { + if state.should_scan_directory(&fs_entry) { + let mut ancestor_inodes = + state.snapshot.ancestor_inodes_for_path(&path); + if !ancestor_inodes.contains(&metadata.inode) { + ancestor_inodes.insert(metadata.inode); + smol::block_on(scan_queue_tx.send(ScanJob { + abs_path, + path: path.clone(), + ignore_stack, + ancestor_inodes, + is_external: fs_entry.is_external, + scan_queue: scan_queue_tx.clone(), + })) + .unwrap(); + } + } else { + fs_entry.kind = EntryKind::UnloadedDir; } } + + state.insert_entry(fs_entry, self.fs.as_ref()); } Ok(None) => { self.remove_repo_path(&path, &mut state.snapshot); diff --git a/crates/project/src/worktree_tests.rs b/crates/project/src/worktree_tests.rs index f908d702eb22aeb7dfb02eb4300611b7d22fbd73..c9ed06dea723a5362652b97759e338e1e815c2c9 100644 --- a/crates/project/src/worktree_tests.rs +++ b/crates/project/src/worktree_tests.rs @@ -1654,37 +1654,37 @@ async fn test_git_status(deterministic: Arc, cx: &mut TestAppCont })); - let tree = Worktree::local( - build_client(cx), - root.path(), - true, - Arc::new(RealFs), - Default::default(), - &mut cx.to_async(), - ) - .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"; const F_TXT: &'static str = "f.txt"; const DOTGITIGNORE: &'static str = ".gitignore"; const BUILD_FILE: &'static str = "target/build_file"; - let project_path: &Path = &Path::new("project"); + let project_path = Path::new("project"); + // Set up git repository before creating the worktree. let work_dir = root.path().join("project"); let mut repo = git_init(work_dir.as_path()); repo.add_ignore_rule(IGNORE_RULE).unwrap(); - git_add(Path::new(A_TXT), &repo); - git_add(Path::new(E_TXT), &repo); - git_add(Path::new(DOTGITIGNORE), &repo); + git_add(A_TXT, &repo); + git_add(E_TXT, &repo); + git_add(DOTGITIGNORE, &repo); git_commit("Initial commit", &repo); + let tree = Worktree::local( + build_client(cx), + root.path(), + true, + Arc::new(RealFs), + Default::default(), + &mut cx.to_async(), + ) + .await + .unwrap(); + tree.flush_fs_events(cx).await; + cx.read(|cx| tree.read(cx).as_local().unwrap().scan_complete()) + .await; deterministic.run_until_parked(); // Check that the right git state is observed on startup @@ -1704,39 +1704,39 @@ async fn test_git_status(deterministic: Arc, cx: &mut TestAppCont ); }); + // Modify a file in the working copy. std::fs::write(work_dir.join(A_TXT), "aa").unwrap(); - tree.flush_fs_events(cx).await; deterministic.run_until_parked(); + // The worktree detects that the file's git status has changed. tree.read_with(cx, |tree, _cx| { let snapshot = tree.snapshot(); - assert_eq!( snapshot.status_for_file(project_path.join(A_TXT)), Some(GitFileStatus::Modified) ); }); - git_add(Path::new(A_TXT), &repo); - git_add(Path::new(B_TXT), &repo); + // Create a commit in the git repository. + git_add(A_TXT, &repo); + git_add(B_TXT, &repo); git_commit("Committing modified and added", &repo); tree.flush_fs_events(cx).await; deterministic.run_until_parked(); - // Check that repo only changes are tracked + // The worktree detects that the files' git status have changed. tree.read_with(cx, |tree, _cx| { let snapshot = tree.snapshot(); - assert_eq!( snapshot.status_for_file(project_path.join(F_TXT)), Some(GitFileStatus::Added) ); - assert_eq!(snapshot.status_for_file(project_path.join(B_TXT)), None); assert_eq!(snapshot.status_for_file(project_path.join(A_TXT)), None); }); + // Modify files in the working copy and perform git operations on other files. git_reset(0, &repo); git_remove_index(Path::new(B_TXT), &repo); git_stash(&mut repo);