From 6a183a66f77c1e66a6abf106bbe292c2431463f8 Mon Sep 17 00:00:00 2001 From: sophalter Date: Sun, 30 Aug 2026 16:14:04 +0200 Subject: [PATCH] perf(core): prune package discovery walks --- fixtures/issue-334/.package.json | 6 + fixtures/issue-334/apps/test/.package.json | 6 + src/disk.rs | 163 +++++++++++++++++++-- src/disk_test.rs | 84 ++++++++++- tests/glob_matching_test.rs | 58 ++++++++ 5 files changed, 299 insertions(+), 18 deletions(-) create mode 100644 fixtures/issue-334/.package.json create mode 100644 fixtures/issue-334/apps/test/.package.json diff --git a/fixtures/issue-334/.package.json b/fixtures/issue-334/.package.json new file mode 100644 index 00000000..7efb04d2 --- /dev/null +++ b/fixtures/issue-334/.package.json @@ -0,0 +1,6 @@ +{ + "name": "hidden-root", + "dependencies": { + "root-dependency": "1.0.0" + } +} diff --git a/fixtures/issue-334/apps/test/.package.json b/fixtures/issue-334/apps/test/.package.json new file mode 100644 index 00000000..1a58ea67 --- /dev/null +++ b/fixtures/issue-334/apps/test/.package.json @@ -0,0 +1,6 @@ +{ + "name": "hidden-nested", + "dependencies": { + "nested-dependency": "1.0.0" + } +} diff --git a/src/disk.rs b/src/disk.rs index 1fa35a72..280dcf48 100644 --- a/src/disk.rs +++ b/src/disk.rs @@ -2,6 +2,10 @@ use { crate::{dependency::Strategy, instance::Instance, specifier::Specifier}, detect_indent::detect_indent, detect_newline_style::LineEnding, + ignore::{ + overrides::{Override, OverrideBuilder}, + types::TypesBuilder, + }, serde_json::Value as JsonValue, std::{ fs, @@ -283,6 +287,128 @@ impl DiskDirEntry { } } +#[derive(Clone)] +struct SourcePatternMatcher { + root: PathBuf, + file_overrides: Override, + directory_overrides: Override, + prune_directories: bool, +} + +impl SourcePatternMatcher { + fn new(root: &Path, patterns: &[String]) -> Self { + let mut file_builder = OverrideBuilder::new(root); + let mut directory_builder = OverrideBuilder::new(root); + let mut can_prune_directories = true; + + for pattern in patterns { + if let Err(err) = file_builder.add(pattern) { + log::debug!("Invalid source pattern '{pattern}': {err}"); + continue; + } + if !pattern.starts_with('!') { + can_prune_directories = can_prune_directories && add_directory_patterns(&mut directory_builder, pattern); + } + } + + let Ok(file_overrides) = file_builder.build() else { + log::debug!("Failed to build source pattern overrides"); + return Self::unrestricted(root); + }; + let has_positive_pattern = file_overrides.num_whitelists() > 0; + let (directory_overrides, prune_directories) = match (has_positive_pattern, can_prune_directories) { + (true, true) => match directory_builder.build() { + Ok(overrides) => (overrides, true), + Err(err) => { + log::debug!("Failed to build source directory overrides: {err}"); + (Override::empty(), false) + } + }, + _ => (Override::empty(), false), + }; + + Self { + root: root.to_path_buf(), + file_overrides, + directory_overrides, + prune_directories, + } + } + + fn unrestricted(root: &Path) -> Self { + Self { + root: root.to_path_buf(), + file_overrides: Override::empty(), + directory_overrides: Override::empty(), + prune_directories: false, + } + } + + fn matches_file(&self, path: &Path) -> bool { + !self.file_overrides.matched(path, false).is_ignore() + } + + fn has_positive_patterns(&self) -> bool { + self.file_overrides.num_whitelists() > 0 + } + + fn matches_directory(&self, path: &Path) -> bool { + if !self.prune_directories { + return true; + } + let Ok(relative_path) = path.strip_prefix(&self.root) else { + return true; + }; + relative_path.as_os_str().is_empty() || self.directory_overrides.matched(relative_path, true).is_whitelist() + } +} + +fn add_directory_patterns(builder: &mut OverrideBuilder, pattern: &str) -> bool { + let Some(directory_patterns) = get_directory_patterns(pattern) else { + return false; + }; + for directory_pattern in directory_patterns { + if let Err(err) = builder.add(&directory_pattern) { + log::debug!("Invalid source directory pattern '{directory_pattern}': {err}"); + return false; + } + } + true +} + +fn get_directory_patterns(pattern: &str) -> Option> { + let pattern = pattern.trim_start_matches('/'); + let mut patterns = Vec::new(); + let mut brace_depth = 0; + + for (index, character) in pattern.char_indices() { + match character { + '\\' => return None, + '{' => brace_depth += 1, + '}' => { + if brace_depth == 0 { + return None; + } + brace_depth -= 1; + } + '[' | ']' => { + // Character classes can match separators through ranges, so their directory prefixes are not safe to infer. + return None; + } + '/' => { + if brace_depth > 0 { + return None; + } + // Preserve trailing whitespace in the prefix by keeping it internal to a directory-only glob. + patterns.push(format!("/{}/", &pattern[..index])); + } + _ => {} + } + } + + (brace_depth == 0).then_some(patterns) +} + /// Allow DI of the disk IO layer pub trait DiskIo { /// Execute a NodeJS command and return the stdout @@ -441,23 +567,26 @@ impl DiskIo for LiveDiskIo { } fn find_package_jsons(&self, root: &Path, patterns: &[String]) -> Vec { - let mut builder = ignore::overrides::OverrideBuilder::new(root); - for pattern in patterns { - if let Err(err) = builder.add(pattern) { - log::debug!("Invalid source pattern '{pattern}': {err}"); - } + let matcher = SourcePatternMatcher::new(root, patterns); + let directory_matcher = matcher.clone(); + let mut walk_builder = ignore::WalkBuilder::new(root); + walk_builder.require_git(false); + if matcher.has_positive_patterns() { + let mut types_builder = TypesBuilder::new(); + types_builder.add("json", "*.json").expect("the JSON file type glob is valid"); + types_builder.select("json"); + let json_types = types_builder.build().expect("the JSON file type matcher is valid"); + // Type whitelists are applied after ignore files but before hidden-file filtering. Install one only when positive sources will reject + // unmatched files; exclusion-only sources must retain the walker's default hidden-file behavior. + walk_builder.types(json_types); } - let overrides = builder.build().unwrap_or_else(|err| { - log::debug!("Failed to build source pattern overrides: {err}"); - ignore::overrides::Override::empty() - }); - ignore::WalkBuilder::new(root) - .require_git(false) - .overrides(overrides) - .filter_entry(|entry| { + walk_builder + .filter_entry(move |entry| { let name = entry.file_name(); - name != "node_modules" && name != ".git" + name != "node_modules" + && name != ".git" + && (!entry.file_type().is_some_and(|file_type| file_type.is_dir()) || directory_matcher.matches_directory(entry.path())) }) .build() .filter_map(|result| match result { @@ -467,7 +596,11 @@ impl DiskIo for LiveDiskIo { None } }) - .filter(|entry| entry.file_type().is_some_and(|t| t.is_file()) && entry.file_name().to_string_lossy().ends_with(".json")) + .filter(|entry| { + entry.file_type().is_some_and(|file_type| file_type.is_file()) + && entry.file_name().to_string_lossy().ends_with(".json") + && matcher.matches_file(entry.path()) + }) .map(|entry| entry.into_path()) .collect() } diff --git a/src/disk_test.rs b/src/disk_test.rs index 38f27dea..52badfca 100644 --- a/src/disk_test.rs +++ b/src/disk_test.rs @@ -1,16 +1,94 @@ use { crate::{ disk::{ - Disk, File, PackageManager, PendingYamlOp, empty_yaml_file, ensure_object_path, insert_catalog_definition, json_view, - parse_yaml_file, remove_catalog_definition, remove_prop, render_yaml_bytes, set_nested_prop, set_prop, + Disk, File, PackageManager, PendingYamlOp, SourcePatternMatcher, empty_yaml_file, ensure_object_path, insert_catalog_definition, + json_view, parse_yaml_file, remove_catalog_definition, remove_prop, render_yaml_bytes, set_nested_prop, set_prop, }, specifier::Specifier, test::mock_disk::MockDiskIo, }, serde_json::json, - std::path::PathBuf, + std::path::{Path, PathBuf}, }; +fn source_pattern_matcher(patterns: &[&str]) -> SourcePatternMatcher { + SourcePatternMatcher::new( + Path::new("/repo"), + &patterns.iter().map(|pattern| pattern.to_string()).collect::>(), + ) +} + +#[test] +fn source_patterns_prune_unrelated_and_too_deep_directories() { + let matcher = source_pattern_matcher(&["/package.json", "apps/*/package.json"]); + + assert!(matcher.matches_directory(Path::new("/repo"))); + assert!(matcher.matches_directory(Path::new("/repo/apps"))); + assert!(matcher.matches_directory(Path::new("/repo/apps/one"))); + assert!(!matcher.matches_directory(Path::new("/repo/cache"))); + assert!(!matcher.matches_directory(Path::new("/repo/apps/one/cache"))); +} + +#[test] +fn recursive_source_patterns_allow_matching_descendants_only() { + let matcher = source_pattern_matcher(&["apps/*/package.json", "libs/**/package.json"]); + + assert!(matcher.matches_directory(Path::new("/repo/apps"))); + assert!(matcher.matches_directory(Path::new("/repo/apps/one"))); + assert!(!matcher.matches_directory(Path::new("/repo/apps/one/nested"))); + assert!(matcher.matches_directory(Path::new("/repo/libs"))); + assert!(matcher.matches_directory(Path::new("/repo/libs/one/nested"))); + assert!(!matcher.matches_directory(Path::new("/repo/cache"))); + + let root_recursive_matcher = source_pattern_matcher(&["**/package.json"]); + assert!(root_recursive_matcher.matches_directory(Path::new("/repo/anything/nested"))); +} + +#[test] +fn source_patterns_keep_file_matching_separate_from_traversal() { + let matcher = source_pattern_matcher(&[ + "/package.json", + "packages/*/package.public.json", + "!packages/private/package.public.json", + ]); + + assert!(matcher.matches_file(Path::new("/repo/package.json"))); + assert!(matcher.matches_file(Path::new("/repo/packages/public/package.public.json"))); + assert!(!matcher.matches_file(Path::new("/repo/packages/private/package.public.json"))); + assert!(!matcher.matches_file(Path::new("/repo/packages/public/package.json"))); +} + +#[test] +fn ambiguous_source_patterns_disable_directory_pruning() { + let matcher = source_pattern_matcher(&["{apps/one,libs/two}/package.json"]); + + assert!(matcher.matches_directory(Path::new("/repo/cache"))); +} + +#[test] +fn character_class_source_patterns_disable_directory_pruning() { + let matcher = source_pattern_matcher(&["a[.-0]b/c/package.json"]); + + assert!(matcher.matches_file(Path::new("/repo/a/b/c/package.json"))); + assert!(matcher.matches_directory(Path::new("/repo/a"))); + assert!(matcher.matches_directory(Path::new("/repo/cache"))); +} + +#[test] +fn source_patterns_preserve_trailing_whitespace_in_directory_names() { + for whitespace in [" ", "\t"] { + let directory_name = format!("foo{whitespace}"); + let pattern = format!("{directory_name}/package.json"); + let directory_path = Path::new("/repo").join(&directory_name); + let matcher = source_pattern_matcher(&[pattern.as_str()]); + + assert!(matcher.matches_directory(&directory_path)); + assert!(!matcher.matches_directory(Path::new("/repo/foo"))); + assert!(matcher.matches_file(&directory_path.join("package.json"))); + assert!(!matcher.matches_file(Path::new("/repo/foo/package.json"))); + } +} + fn json_file_from_value(value: serde_json::Value) -> File { File { filepath: PathBuf::from("/test/package.json"), diff --git a/tests/glob_matching_test.rs b/tests/glob_matching_test.rs index 16e1f09f..3d71fc30 100644 --- a/tests/glob_matching_test.rs +++ b/tests/glob_matching_test.rs @@ -116,6 +116,64 @@ fn issue_334_double_star_matches_deeply_nested() { ); } +#[test] +fn issue_334_double_star_keeps_gitignore_exclusions() { + let paths = package_paths(&run_json("issue-334", &["--source", "apps/**/package.json"])); + for component in [".next", "dist", "build"] { + assert!( + !has_component(&paths, component), + "expected '{component}' directory to be excluded by .gitignore, got: {paths:?}", + ); + } +} + +#[test] +fn issue_334_separator_matching_character_class_is_discovered() { + let paths = package_paths(&run_json("issue-334", &["--source", "apps[.-0]test/nested/package.json"])); + assert!( + ends_with(&paths, "apps/test/nested/package.json"), + "expected a character class matching '/' to reach the nested package, got: {paths:?}", + ); +} + +#[test] +fn issue_334_root_dotfile_source_is_discovered() { + let paths = package_paths(&run_json("issue-334", &["--source", ".package.json"])); + assert!( + ends_with(&paths, "issue-334/.package.json"), + "expected root '.package.json' to be matched, got: {paths:?}", + ); +} + +#[test] +fn issue_334_nested_dotfile_source_is_discovered() { + let paths = package_paths(&run_json("issue-334", &["--source", "apps/test/.package.json"])); + assert!( + ends_with(&paths, "issue-334/apps/test/.package.json"), + "expected nested '.package.json' to be matched, got: {paths:?}", + ); +} + +#[test] +fn issue_334_exclusion_only_sources_keep_dotfiles_hidden() { + let paths = package_paths(&run_json("issue-334", &["--source", "!apps/other/package.json"])); + + assert!( + ends_with(&paths, "apps/test/package.json"), + "expected unmatched files to remain included, got: {paths:?}" + ); + assert!( + !ends_with(&paths, "apps/other/package.json"), + "expected the negated package to remain excluded, got: {paths:?}", + ); + for hidden_path in ["issue-334/.package.json", "apps/test/.package.json"] { + assert!( + !ends_with(&paths, hidden_path), + "expected exclusion-only sources to keep '{hidden_path}' hidden, got: {paths:?}", + ); + } +} + #[test] fn issue_334_bare_package_json_matches_root_only() { let paths = package_paths(&run_json("issue-334", &["--source", "package.json"]));