git: Fix .git directory being removed from watcher when excluded via file_scan_exclusions

[AlvaroParker]

Self-Review Checklist:

• I've reviewed my own diff for quality, security, and reliability
• Unsafe blocks (if any) have justifying comments
• The content is consistent with the UI/UX checklist
• Tests cover the new/changed behavior
• Performance impact has been considered and is acceptable

Closes #57888

Commit c19cc4c51e0f64eec42168943050f2deeccaa076 introduced the bug on #50412 . This change modified the remove_path function to also remove the path from the watcher:

zed/crates/worktree/src/worktree.rs

Lines 3218 to 3220 in f0341c9

	for removed_dir_abs_path in removed_dir_abs_paths {
	watcher.remove(&removed_dir_abs_path).log_err();
	}

And when a user modified the global setting and doesn't include .git in it, like:

// ~/.config/zed/settings.json
{
  "file_scan_exclusions": ["foo"]
}

But then includes it on their local project settings:

// ~/my/local/project/.zed/settings.json
{
  "file_scan_exclusions": ["**/.git"]
}

It causes zed to stop watching for changes on .git

Release Notes:

• Fixed bug where zed stopped watching change on .git directory if it was added to the project local file_scan_exclusions

[AlvaroParker]

I think this PR fixes the immediate bug, but the shape of the bug is more like a design issue because the watcher ownership is implicit.

Currently .git is detected and registered for git metadata watching before file_scan_exclusions are applied:

zed/crates/worktree/src/worktree.rs

Lines 4857 to 4894 in 8734d01

	if self.track_git_repositories {
	if child_name == DOT_GIT {
	let mut state = self.state.lock().await;
	state
	.insert_git_repository(
	child_path.clone(),
	self.fs.as_ref(),
	self.watcher.as_ref(),
	)
	.await;
	} else if child_name == GITIGNORE {
	match build_gitignore(&child_abs_path, self.fs.as_ref()).await {
	Ok(ignore) => {
	let ignore = Arc::new(ignore);
	ignore_stack = ignore_stack.append(
	IgnoreKind::Gitignore(job.abs_path.clone()),
	ignore.clone(),
	);
	new_ignore = Some(ignore);
	}
	Err(error) => {
	log::error!(
	"error loading .gitignore file {:?} - {:?}",
	child_name,
	error
	);
	}
	}
	}
	}
	if self.settings.is_path_excluded(&child_path) {
	log::debug!("skipping excluded child entry {child_path:?}");
	self.state
	.lock()
	.await
	.remove_path(&child_path, self.watcher.as_ref());
	continue;

The repository-specific watches are added here:

zed/crates/worktree/src/worktree.rs

Lines 3271 to 3301 in 8734d01

	async fn insert_git_repository_for_path(
	&mut self,
	work_directory: WorkDirectory,
	dot_git_abs_path: Arc<Path>,
	fs: &dyn Fs,
	watcher: &dyn Watcher,
	) -> Result<LocalRepositoryEntry> {
	let work_dir_entry = self
	.snapshot
	.entry_for_path(&work_directory.path_key().0)
	.with_context(|| {
	format!(
	"working directory `{}` not indexed",
	work_directory
	.path_key()
	.0
	.display(self.snapshot.path_style)
	)
	})?;
	let work_directory_abs_path = self.snapshot.work_directory_abs_path(&work_directory);
	let (repository_dir_abs_path, common_dir_abs_path) =
	discover_git_paths(&dot_git_abs_path, fs).await;
	watcher
	.add(&common_dir_abs_path)
	.context("failed to add common directory to watcher")
	.log_err();
	watcher
	.add(&repository_dir_abs_path)
	.context("failed to add repository directory to watcher")
	.log_err();

But remove_path removes directory watches when removing entries from the worktree snapshot:

zed/crates/worktree/src/worktree.rs

Lines 3156 to 3220 in 8734d01

	fn remove_path(&mut self, path: &RelPath, watcher: &dyn Watcher) {
	log::trace!("background scanner removing path {path:?}");
	let mut new_entries;
	let removed_entries;
	{
	let mut cursor = self
	.snapshot
	.entries_by_path
	.cursor::<TraversalProgress>(());
	new_entries = cursor.slice(&TraversalTarget::path(path), Bias::Left);
	removed_entries = cursor.slice(&TraversalTarget::successor(path), Bias::Left);
	new_entries.append(cursor.suffix(), ());
	}
	self.snapshot.entries_by_path = new_entries;
	let mut removed_ids = Vec::with_capacity(removed_entries.summary().count);
	let mut removed_dir_abs_paths = Vec::new();
	for entry in removed_entries.cursor::<()>(()) {
	if entry.is_dir() {
	let watch_path = self
	.watched_dir_abs_paths_by_entry_id
	.remove(&entry.id)
	.map(|path| path.as_ref().to_path_buf())
	.unwrap_or_else(|| self.snapshot.absolutize(&entry.path));
	removed_dir_abs_paths.push(watch_path);
	}
	match self.removed_entries.entry(entry.inode) {
	hash_map::Entry::Occupied(mut e) => {
	let prev_removed_entry = e.get_mut();
	if entry.id > prev_removed_entry.id {
	*prev_removed_entry = entry.clone();
	}
	}
	hash_map::Entry::Vacant(e) => {
	e.insert(entry.clone());
	}
	}
	if entry.path.file_name() == Some(GITIGNORE) {
	let abs_parent_path = self.snapshot.absolutize(&entry.path.parent().unwrap());
	if let Some((_, needs_update)) = self
	.snapshot
	.ignores_by_parent_abs_path
	.get_mut(abs_parent_path.as_path())
	{
	*needs_update = true;
	}
	}
	if let Err(ix) = removed_ids.binary_search(&entry.id) {
	removed_ids.insert(ix, entry.id);
	}
	}
	self.snapshot
	.entries_by_id
	.edit(removed_ids.iter().map(|&id| Edit::Remove(id)).collect(), ());
	self.snapshot
	.git_repositories
	.retain(|id, _| removed_ids.binary_search(id).is_err());
	for removed_dir_abs_path in removed_dir_abs_paths {
	watcher.remove(&removed_dir_abs_path).log_err();
	}

This matters because remove_path is not only called when a file or directory is deleted. It also runs when settings change and a path that was previously scanned now matches file_scan_exclusions. For example, a user can have global file_scan_exclusions without **/.git, then add **/.git in project-local .zed/settings.json. On the rescan, .git becomes newly excluded, so remove_path removes it from the worktree snapshot (this causes zed to stop detecting git changes like staging, branch switches, etc).

So the same path can effectively be watched for two reasons:

• as a normal worktree directory
• as metadata needed by a subsystem (currently git)

The current fix special-cases git repository paths, which is probably fine for this PR. My point (and concern) is that this coupling is easy to miss, so if we later add another metadata directory with similar behavior, for example .jj, a developer might add the equivalent of if child_name == DOT_JJ { ... watcher.add(...) } in scan_dir, but not realize they also need a matching exception in remove_path. Then the same type of bug can happen again.

If zed decides to integrate something like .jj where file watching would also be required, this would be less fragile if watcher ownership were explicit, meaning, remove_path should remove only watches owned by worktree entries, while git/jj/others metadata watches should be owned and released by their own tracking state. That would make future implementations that have specially watched directories safer by construction instead of relying on devs remembering this extra remove_path interaction :)

But given that currently the only exception is git, I don't think this refactoring is needed for now to fix this bug.

[cole-miller]

@AlvaroParker Thanks for the PR, and for the thorough analysis of the issue! This seems like a good fix. We just merged a related change in #58692 that split up "remove from snapshot" from "remove watcher", so if you merge main you should be able to use that to unconditionally remove from the snapshot and then conditionally remove the watcher in the "newly excluded" case. Once that's done, happy to merge this!

[AlvaroParker]

@cole-miller sorry for the ping :) I did a rebase and updated the fix to be more inline to what has been done on #58692

Let me know what you think!

[cole-miller]

Thanks!