From b9c79a145a05258c275545ac436398919ccbd811 Mon Sep 17 00:00:00 2001 From: flupkede Date: Sat, 17 Oct 2026 22:40:22 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20include-list=20review=20fixes=20?= =?UTF-8?q?=E2=80=94=20last-match=20negation,=20empty=20file,=20shared=20h?= =?UTF-8?q?elper?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on the .codesearchinclude feature (PR #346): - !-rules were dead under a decisively-included ancestor: the component loop returned at the FIRST decisive match, so src/ + !src/gen.rs still indexed gen.rs while the docs promise last-match-wins. The loop now tracks the last decisive match and decides after it. - a present-but-empty (comments-only) .codesearchinclude built a matcher with zero rules and indexed NOTHING, contradicting the shipped 'absent or empty = historical behavior' claim. A zero-rule build is now inactive (num_ignores + num_whitelists both zero). - the include semantics existed as two ~45-line copies (walker + watcher); the watcher now delegates to the shared pub(crate) helpers in file/mod.rs so the two gates cannot drift. - two new pins: ancestor+negation un-includes, present-but-empty indexes everything. --- src/file/mod.rs | 71 +++++++++++++++++++++++++++++++++++++++++++----- src/watch/mod.rs | 53 ++++-------------------------------- 2 files changed, 69 insertions(+), 55 deletions(-) diff --git a/src/file/mod.rs b/src/file/mod.rs index f9b0403f..96f2aacd 100644 --- a/src/file/mod.rs +++ b/src/file/mod.rs @@ -85,12 +85,15 @@ pub struct FileWalker { /// Build the whitelist matcher from `.codesearchinclude` (repo root) and the /// global `~/.codesearch/.codesearchinclude` (lowest precedence). `None` when -/// neither exists — the walker then indexes everything the ignore rules +/// neither exists — or when both exist but carry no rules (empty or +/// comments-only) — the caller then indexes everything the ignore rules /// allow, exactly as before. When a matcher EXISTS, it is a whitelist: a /// file is indexed only when the include rules match it (the file itself or /// an ancestor — `src/` covers `src/a/b.rs`), and ignore rules still win /// first: an include can never resurrect an ignored file. -fn build_include_matcher(root: &Path) -> Option { +/// +/// Shared by the walker and the watcher so the two gates cannot drift. +pub(crate) fn build_include_matcher(root: &Path) -> Option { let mut builder = GitignoreBuilder::new(root); let mut added_any = false; @@ -116,25 +119,34 @@ fn build_include_matcher(root: &Path) -> Option { if !added_any { return None; } - builder.build().ok() + let matcher = builder.build().ok()?; + // A present-but-empty file (no rules — e.g. only `#` comments) is not a + // whitelist: `matched` would answer None for every path and index + // nothing, contradicting the "absent or empty = historical behavior" + // contract. + if matcher.num_ignores() == 0 && matcher.num_whitelists() == 0 { + return None; + } + Some(matcher) } /// Whitelist verdict for one path against the include matcher: `true` when /// the file is included (the path itself or an ancestor matches an include /// rule; a `!`-whitelist rule inside the include file un-includes again — /// last match along the component chain wins, mirroring gitignore order). -fn is_included(matcher: &Gitignore, root: &Path, path: &Path) -> bool { +pub(crate) fn is_included(matcher: &Gitignore, root: &Path, path: &Path) -> bool { let relative = path.strip_prefix(root).unwrap_or(path); let mut current = PathBuf::new(); + let mut included = false; for component in relative.components() { current.push(component); match matcher.matched(¤t, true) { - m if m.is_ignore() => return true, - m if m.is_whitelist() => return false, + m if m.is_ignore() => included = true, + m if m.is_whitelist() => included = false, _ => {} } } - false + included } impl FileWalker { @@ -537,4 +549,49 @@ mod tests { let (files, _) = walker.walk().unwrap(); assert_eq!(files.len(), 2, "absent include list = historical behavior"); } + + #[test] + fn include_list_ancestor_negation_unincludes_again() { + let dir = TempDir::new().unwrap(); + fs::create_dir_all(dir.path().join("src")).unwrap(); + fs::write(dir.path().join("src").join("keep.rs"), "fn a() {}").unwrap(); + fs::write(dir.path().join("src").join("gen.rs"), "generated").unwrap(); + fs::write(dir.path().join("drop.py"), "print(1)").unwrap(); + // The ancestor include decides first, the deeper `!` rule must win: + // last decisive match along the component chain. + fs::write(dir.path().join(".codesearchinclude"), "src/\n!src/gen.rs\n").unwrap(); + + let walker = FileWalker::new(dir.path()); + let (files, stats) = walker.walk().unwrap(); + + let names: Vec = files + .iter() + .map(|f| f.path.file_name().unwrap().to_string_lossy().to_string()) + .collect(); + assert!(names.contains(&"keep.rs".to_string())); + assert!( + !names.contains(&"gen.rs".to_string()), + "the !-rule under an included ancestor must un-include" + ); + assert!(!names.contains(&"drop.py".to_string())); + assert_eq!(stats.indexable_files, 1); + assert_eq!(stats.skipped_ignored, 2); + } + + #[test] + fn include_list_present_but_empty_indexes_everything() { + let dir = TempDir::new().unwrap(); + fs::write(dir.path().join("a.rs"), "fn a() {}").unwrap(); + fs::write(dir.path().join("b.py"), "print(1)").unwrap(); + // Comments only: no rules, so this is not a whitelist. + fs::write(dir.path().join(".codesearchinclude"), "# nothing yet\n").unwrap(); + + let walker = FileWalker::new(dir.path()); + let (files, _) = walker.walk().unwrap(); + assert_eq!( + files.len(), + 2, + "a present-but-empty include list = historical behavior" + ); + } } diff --git a/src/watch/mod.rs b/src/watch/mod.rs index e0af48a1..17e939f1 100644 --- a/src/watch/mod.rs +++ b/src/watch/mod.rs @@ -12,8 +12,8 @@ use tokio::sync::Mutex; use crate::cache::normalize_path; use crate::constants::{ - global_codesearchignore_path, global_codesearchinclude_path, ALWAYS_EXCLUDED, - ALWAYS_SKIP_EXTENSIONS, ALWAYS_SKIP_FILENAME_SUFFIXES, + global_codesearchignore_path, ALWAYS_EXCLUDED, ALWAYS_SKIP_EXTENSIONS, + ALWAYS_SKIP_FILENAME_SUFFIXES, }; use crate::file::Language; @@ -79,7 +79,7 @@ impl FileWatcher { /// Create a new file watcher for the given root directory pub fn new(root: PathBuf) -> Self { let gitignore = Self::build_gitignore(&root); - let include_matcher = Self::build_include_matcher(&root); + let include_matcher = crate::file::build_include_matcher(&root); Self { root, debouncer: None, @@ -185,58 +185,15 @@ impl FileWatcher { } } - /// Build the whitelist matcher from `.codesearchinclude` (repo root) and - /// the global `~/.codesearch/.codesearchinclude`. `None` when neither - /// exists — the watcher then accepts everything the ignore rules allow. - /// Same loading order as `build_gitignore` (global lowest, local highest). - fn build_include_matcher(root: &Path) -> Option { - let mut builder = GitignoreBuilder::new(root); - let mut added_any = false; - - if let Some(global_path) = global_codesearchinclude_path() { - if global_path.exists() { - if let Some(e) = builder.add(&global_path) { - tracing::debug!("Failed to add global codesearchinclude: {}", e); - } else { - added_any = true; - } - } - } - - let local_path = root.join(crate::constants::CODESEARCHINCLUDE_FILE); - if local_path.exists() { - if let Some(e) = builder.add(&local_path) { - tracing::debug!("Failed to add .codesearchinclude: {}", e); - } else { - added_any = true; - } - } - - if !added_any { - return None; - } - builder.build().ok() - } - /// Whitelist verdict for one path: included when the path itself or an /// ancestor matches an include rule (last match along the component /// chain wins, so a `!` rule inside the include file un-includes). - /// Mirrors `is_gitignored`'s ancestor walk, inverted. + /// Delegates to the shared walker helper so both gates cannot drift. fn is_included(&self, path: &Path) -> bool { let Some(ref inc) = self.include_matcher else { return true; // no include list = everything is included }; - let relative = path.strip_prefix(&self.root).unwrap_or(path); - let mut current = PathBuf::new(); - for component in relative.components() { - current.push(component); - match inc.matched(¤t, true) { - m if m.is_ignore() => return true, - m if m.is_whitelist() => return false, - _ => {} - } - } - false + crate::file::is_included(inc, &self.root, path) } /// Start watching for file changes