fix(watcher): skip hidden dirs and exclude_dirs in fs events - #54
Conversation
The walker uses hidden(true) by default and respects --exclude-dir, but handle_fs_event only filtered the .tgrep index dir. As a result, writes to .git/index.lock, .git/HEAD, etc. produced spurious '[trace] reindex: modified .git/...' lines (and pointless file I/O + trigram extraction) during normal git operations. Mirror the walker's filtering: skip any path with a dot-prefixed component, and skip any path whose components match the configured exclude_dirs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Aligns the server file watcher’s event handling with the index builder’s directory filtering to avoid reindexing noise and wasted work from paths that should not be indexed (notably .git/ activity).
Changes:
- Add watcher-side filtering to skip dot-prefixed path components (hidden paths) before reindexing.
- Add watcher-side filtering to skip paths whose components match the configured excluded directory names.
Show a summary per file
| File | Description |
|---|---|
| tgrep-cli/src/serve.rs | Adds hidden/exclude directory filtering to handle_fs_event to reduce spurious watcher reindexing work. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
tgrep-cli/src/serve.rs:769
- The
exclude_dirscheck currently matches any path segment, including the final filename segment. That means a file named the same as an excluded directory (e.g.,src/vendor) would be skipped by the watcher even though the walker only skips when the excluded name is a directory entry (tgrep-core/src/walker.rs). Consider applying the match only to directory components (e.g., ancestors / parent segments) so watcher and walker semantics stay consistent.
if !state.exclude_dirs.is_empty()
&& rel_path
.split('/')
.any(|seg| state.exclude_dirs.iter().any(|d| d == seg))
{
continue;
tgrep-cli/src/serve.rs:757
- The walker also applies gitignore rules by default (WalkBuilder::git_ignore/git_global/git_exclude), but the watcher-side filter here doesn’t. That means gitignored paths can still be indexed if they change after startup, diverging from the initial walk. If parity with the walker is the goal, consider applying the same ignore rules (or at least clarify in the comment that only hidden +
--excludeare mirrored).
// Mirror the walker's filtering so the watcher does not reindex
// files the initial walk would have skipped — most notably
// hidden directories like `.git/`, which fire frequent
// `index.lock`/HEAD/refs writes during normal git operations.
// The walker uses `hidden(true)` by default; replicate that by
// skipping any path component starting with `.`. Also honor the
// user's --exclude-dir list so the same names are skipped here.
- Files reviewed: 1/1 changed files
- Comments generated: 3
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Shengyu Fu <shengyfu@microsoft.com>
There was a problem hiding this comment.
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 2
Address review feedback: pull the watcher's hidden-component / exclude_dirs filtering out of handle_fs_event into a small pure helper, and cover it with focused unit tests so the behavior cannot silently regress. Cases covered: leading and inner dot components (.git, src/.cache), non-hidden paths (incl. names with a dot mid-segment), exact-match exclude_dirs (with substring negative case), and the . / .. / empty-string edge cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (3)
tgrep-cli/src/serve.rs:1707
- Tests cover directory exclusion (e.g.
target/debug/foo) but not the edge case where the excluded name appears as the file name (e.g.targetas a file in the repo root). Adding a regression test for that case will help ensure the watcher’s filtering stays aligned with the walker’s directory-only exclusion behavior.
#[test]
fn skip_watcher_path_honors_exclude_dirs() {
let exclude = vec!["target".to_string(), "node_modules".to_string()];
assert!(should_skip_watcher_path("target/debug/foo", &exclude));
assert!(should_skip_watcher_path(
"node_modules/react/index.js",
&exclude
));
// Excluded name appearing as an inner component still excludes.
assert!(should_skip_watcher_path("a/target/b", &exclude));
// Substring match should NOT trigger — "targets" != "target".
assert!(!should_skip_watcher_path("targets/foo", &exclude));
// Unrelated paths are not skipped.
assert!(!should_skip_watcher_path("src/main.rs", &exclude));
tgrep-cli/src/serve.rs:715
- PR description mentions
--exclude-dir, but the CLI flag in this repo (and this comment) is--exclude. To avoid user confusion, consider updating the PR description (or any related docs/release notes) to use the correct flag name.
/// * any path component starting with `.` (matches `WalkBuilder::hidden(true)`)
/// * any path component matching one of the configured `--exclude` names
tgrep-cli/src/serve.rs:729
should_skip_watcher_pathtreats any path segment equal to anexclude_dirsentry as excluded, including the final segment (the file name). The walker only appliesexclude_dirsto directory entries, so a file namedtarget(or similar) at the repo root would be indexed by the initial walk but then ignored by the watcher, leaving stale index content. Consider applyingexclude_dirschecks only to directory components (e.g., all segments except the last for file events), or otherwise matching the walker'sis_dirsemantics.
if seg.starts_with('.') {
return true;
}
if exclude_dirs.iter().any(|d| d == seg) {
return true;
}
- Files reviewed: 2/3 changed files
- Comments generated: 1
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Shengyu Fu <shengyfu@microsoft.com>
There was a problem hiding this comment.
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
tgrep-cli/src/serve.rs:1709
- Test coverage for
should_skip_watcher_pathdoesn't include the edge case where the file name (final segment) equals an excluded directory name (e.g."src/target"wheretargetis a file). Adding a regression test for that case would help ensure the watcher matches the walker's semantics (exclude directory names only).
#[test]
fn skip_watcher_path_honors_exclude_dirs() {
let exclude = vec!["target".to_string(), "node_modules".to_string()];
assert!(should_skip_watcher_path("target/debug/foo", &exclude));
assert!(should_skip_watcher_path(
"node_modules/react/index.js",
&exclude
));
// Excluded name appearing as an inner component still excludes.
assert!(should_skip_watcher_path("a/target/b", &exclude));
// Substring match should NOT trigger — "targets" != "target".
assert!(!should_skip_watcher_path("targets/foo", &exclude));
// Unrelated paths are not skipped.
assert!(!should_skip_watcher_path("src/main.rs", &exclude));
}
- Files reviewed: 2/3 changed files
- Comments generated: 2
…onents Address PR #54 review (comment 3107380665): the walker only treats --exclude names as directory subtree filters — a regular file whose basename happens to equal an excluded name (e.g. a file literally called vendor at the repo root, or src/target) is still indexed by the initial walk. The previous helper checked every component including the basename, so the watcher would skip such a file while the indexer would index it, leaving the in-memory and on-disk indexes inconsistent. Match only ancestor directory components; the hidden-component check still applies to every segment, since WalkBuilder::hidden(true) filters dotfiles too. Added a targeted test (skip_watcher_path_does_not_match_basename_against_exclude_dirs) for the regression and updated the doc-comment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address PR #54 review (comment 3107407968): the watcher only applied hidden / --exclude filtering; gitignored files (e.g. *.log under build/, target/release/foo) would still be upserted into the live overlay even though the indexer's WalkBuilder::git_ignore(true) walk skipped them. Over time the in-memory and on-disk indexes drift apart. Build a Gitignore matcher once at server startup (loads .git/info/exclude, the global gitignore, and every .gitignore in the tree via WalkBuilder so we naturally skip ignored subtrees and the .git dir during loading). Pass it to should_skip_watcher_path, which now consults the matcher in addition to the cheap hidden / --exclude checks. Added a unit test (skip_watcher_path_honors_gitignore_matcher) covering both ignore and non-ignore cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/4 changed files
- Comments generated: 3
Per review: don't fork the gitignore-loading logic between core and cli. Hoist build_gitignore_matcher into tgrep_core::walker (alongside walk_dir / walk_file_metadata) and re-export ignore::gitignore::Gitignore as tgrep_core::walker::Gitignore so the cli can consume it without a direct ignore-crate dep. Also fixes a discovery bug spotted by the new test: hidden(true) on the WalkBuilder used to enumerate .gitignore files filters dotfiles, including .gitignore itself, so no rules were ever loaded. Walk with hidden(false) and an explicit filter_entry to skip .git/ instead. tgrep-cli now: drops the ignore = 0.4 dep, calls tgrep_core::walker::build_gitignore_matcher, and the unit test exercises the shared loader by writing a .gitignore to a TempDir and round-tripping through it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two PR #54 review nits: 1. build_gitignore_matcher walks the whole tree once at startup. With --no-watch the matcher would just sit unused — gate the call on !no_watch so we don't pay the IO cost when the watcher is disabled. 2. should_skip_watcher_path used to allocate a Vec<&str> for every fs event just to compute path.len()-1. Replace with a single streaming Peekable pass: hidden check on every segment, exclude_dirs check only when peek().is_some() (i.e. not the basename). exclude_dirs is typically <= 3 entries so a HashSet would be overkill; left as a linear scan. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
tgrep-core/src/walker.rs:240
build_gitignore_matcheruses.hidden(false)and only filters out.git, so it will still recurse into other dot-directories (including.tgrep/if present) when searching for.gitignorefiles. To better match the indexer’shidden(true)behavior and avoid extra filesystem churn, consider afilter_entrythat skips dot-prefixed directories (except allowing.gitignorefiles to be visited).
.hidden(false)
.git_ignore(true)
.git_global(true)
.git_exclude(true)
.filter_entry(|entry| entry.file_name() != ".git")
- Files reviewed: 3/4 changed files
- Comments generated: 1
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The discovery walk used hidden(false) + only filtered .git. Now it skips all dot-prefixed directories (.tgrep, .vscode, etc.) to avoid unnecessary I/O into hidden subtrees that the indexer also skips. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/5 changed files
- Comments generated: 2
| // Walk to find every `.gitignore` file. We can't use `hidden(true)` | ||
| // because `.gitignore` itself starts with `.` and would be filtered. | ||
| // Instead, walk with hidden=false and use `filter_entry` to skip | ||
| // all dot-prefixed *directories* (`.git`, `.tgrep`, `.vscode`, …) — | ||
| // this avoids unnecessary I/O into hidden subtrees while still | ||
| // letting dot-prefixed *files* like `.gitignore` through, since | ||
| // `filter_entry` only controls directory descent for directories. | ||
| let walker = WalkBuilder::new(root) | ||
| .hidden(false) | ||
| .git_ignore(true) | ||
| .git_global(true) | ||
| .git_exclude(true) | ||
| .filter_entry(|entry| { | ||
| // Allow files (we only care about .gitignore among them). | ||
| // For directories, skip any that start with '.'. | ||
| if entry.file_type().is_some_and(|ft| ft.is_dir()) { | ||
| !entry | ||
| .file_name() | ||
| .to_str() | ||
| .is_some_and(|n| n.starts_with('.')) | ||
| } else { | ||
| true | ||
| } | ||
| }) | ||
| .build(); |
There was a problem hiding this comment.
build_matcher walks the entire tree to find .gitignore files but doesn't apply exclude_dirs-style subtree filtering. For repos that rely on --exclude to skip large directories that are not gitignored (or before gitignore rules are present), this can add a lot of unnecessary I/O at server startup. Consider adding an exclude_dirs parameter (or a more general filter) and skipping matching directory entries in the filter_entry closure, similar to walker.rs.
| /// Mirrors the file walker's hidden-path, `--exclude` directory filtering, | ||
| /// and `.gitignore` rules so the watcher does not reindex files that the | ||
| /// initial walk would never have indexed for those reasons: | ||
| /// * any path component starting with `.` (matches `WalkBuilder::hidden(true)`), | ||
| /// including the file name itself (e.g. `.envrc`). | ||
| /// * any *ancestor directory* component matching one of the configured | ||
| /// `--exclude` names. The walker only treats `--exclude` names as | ||
| /// directory subtree filters (it skips the whole subtree when the entry |
There was a problem hiding this comment.
PR description mentions --exclude-dir, but the CLI flag is --exclude (repeatable) for both index and serve (see tgrep-cli/src/main.rs). Updating the PR description to match the actual flag name would avoid confusion for reviewers/users.
The walker uses hidden(true) by default and respects
--exclude-dir, buthandle_fs_eventonly filtered the.tgrepindex dir. As a result, writes to.git/index.lock,.git/HEAD, etc. produced spurious[trace] reindex: modified .git/...lines (and pointless file I/O + trigram extraction) during normal git operations.Mirror the walker's filtering in the watcher:
exclude_dirsTested locally:
cargo fmt,cargo clippy --workspace --all-targets -- -D warnings, andcargo test --workspaceall pass.