diff --git a/Cargo.lock b/Cargo.lock index 7f8c509..deeeaf4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -184,7 +184,7 @@ checksum = "1c133bc6a41be0d194c306b5506d15e6feeea7b1d6604bd3f8310dfb2ca96486" [[package]] name = "codeowners" -version = "0.7.0" +version = "0.7.1" dependencies = [ "assert_cmd", "clap", diff --git a/Cargo.toml b/Cargo.toml index a286f02..7e98759 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "codeowners" -version = "0.7.0" +version = "0.7.1" edition = "2024" license = "MIT" diff --git a/src/path_utils.rs b/src/path_utils.rs index 50369ae..87c1f0e 100644 --- a/src/path_utils.rs +++ b/src/path_utils.rs @@ -64,10 +64,31 @@ pub(crate) fn matches_globs(path: &Path, globs: &[String]) -> bool { } } +/// The leading components of `glob` with no glob syntax in them. Every path the glob can match is under this one. +pub(crate) fn literal_glob_prefix(glob: &str) -> PathBuf { + Path::new(glob) + .components() + .take_while(|component| !component.as_os_str().to_string_lossy().contains(['*', '?', '[', '{', '\\', '!'])) + .collect() +} + #[cfg(test)] mod tests { use super::*; + #[test] + fn test_literal_glob_prefix() { + for (glob, prefix) in [ + ("config/teams/**/*.yml", "config/teams"), + ("config/teams/*.yml", "config/teams"), + ("config/{teams,groups}/*.yml", "config"), + ("**/teams/*.yml", ""), + ("config/teams/payroll.yml", "config/teams/payroll.yml"), + ] { + assert_eq!(literal_glob_prefix(glob), PathBuf::from(prefix), "{glob}"); + } + } + const OWNED_GLOB: &str = "{app,components,config,frontend,lib,packs,spec,danger,script}/**/*.{rb,arb,erb,rake,js,jsx,ts,tsx}"; #[test] diff --git a/src/project_builder.rs b/src/project_builder.rs index a3e1b2d..82ff8ad 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -1,4 +1,5 @@ use std::{ + collections::HashSet, fs::File, path::{Path, PathBuf}, sync::{Arc, Mutex}, @@ -12,7 +13,7 @@ use tracing::instrument; use crate::{ cache::Cache, config::Config, - path_utils::matches_globs, + path_utils::{literal_glob_prefix, matches_globs}, project::{DirectoryCodeownersFile, Error, Package, PackageType, Project, ProjectFile, Team, VendoredGem, deserializers}, project_file_builder::ProjectFileBuilder, tracked_files, @@ -77,6 +78,11 @@ impl<'a> ProjectBuilder<'a> { let team_file_glob = self.config.team_file_glob.clone(); let base_path = self.base_path.clone(); let tracked_files = tracked_files::find_tracked_files(&self.base_path); + // git's ignore rules are off, so without this the walk reads every ignored directory, like build output or worktrees. + let tracked_directories = tracked_files + .as_ref() + .map(|files| tracked_files::tracked_directories(&base_path, files)); + let team_file_roots: Vec = team_file_glob.iter().map(|glob| literal_glob_prefix(glob)).collect(); builder.filter_entry(move |entry: &DirEntry| { let path = entry.path(); @@ -101,6 +107,14 @@ impl<'a> ProjectBuilder<'a> { { return false; } + if let Some(tracked_directories) = &tracked_directories + && let Some(ft) = entry.file_type() + && ft.is_dir() + && let Ok(rel) = path.strip_prefix(&base_path) + && !walk_enters(rel, tracked_directories, &team_file_roots) + { + return false; + } if let Some(ft) = entry.file_type() && ft.is_dir() && let Ok(rel) = path.strip_prefix(&base_path) @@ -332,6 +346,15 @@ impl<'a> ProjectBuilder<'a> { } } +/// Whether the walk has to enter `relative_directory`. Only a directory that holds a tracked file can hold a file the project +/// includes, apart from team files, which are included untracked. +fn walk_enters(relative_directory: &Path, tracked_directories: &HashSet, team_file_roots: &[PathBuf]) -> bool { + tracked_directories.contains(relative_directory) + || team_file_roots + .iter() + .any(|root| relative_directory.starts_with(root) || root.starts_with(relative_directory)) +} + pub(crate) fn ruby_package_owner(path: &Path, node_limit: &NodeLimit) -> Result, Report> { let bytes = std::fs::read(path).change_context(Error::Io)?; // An empty, comment-only or `---`-only file is a null document, which serde-saphyr 2.1 no longer reads as a struct. @@ -369,6 +392,35 @@ mod tests { use super::*; + #[test] + fn test_walk_enters() { + let tracked_directories: HashSet = ["", "app", "app/models"].iter().map(PathBuf::from).collect(); + let team_file_roots = vec![PathBuf::from("config/teams")]; + for (directory, entered) in [ + ("", true), + ("app", true), + ("app/models", true), + ("app/assets", false), + ("node_modules", false), + ("config", true), + ("config/teams", true), + ("config/teams/new_area", true), + ("config/initializers", false), + ] { + assert_eq!( + walk_enters(Path::new(directory), &tracked_directories, &team_file_roots), + entered, + "{directory}" + ); + } + } + + #[test] + fn test_walk_enters_every_directory_when_a_team_file_glob_starts_with_a_wildcard() { + let tracked_directories = HashSet::from([PathBuf::new()]); + assert!(walk_enters(Path::new("node_modules"), &tracked_directories, &[PathBuf::new()])); + } + #[test] fn test_ruby_package_owner_top_level() { let yaml = "owner: TeamA\n"; diff --git a/src/tracked_files.rs b/src/tracked_files.rs index 9d6717c..038fb05 100644 --- a/src/tracked_files.rs +++ b/src/tracked_files.rs @@ -1,5 +1,5 @@ use std::{ - collections::HashMap, + collections::{HashMap, HashSet}, path::{Path, PathBuf}, process::Command, }; @@ -25,6 +25,21 @@ pub(crate) fn find_tracked_files(base_path: &Path) -> Option) -> HashSet { + let mut directories = HashSet::from([PathBuf::new()]); + for relative_file in tracked_files.keys().filter_map(|file| file.strip_prefix(base_path).ok()) { + for directory in relative_file.ancestors() { + // Its ancestors were added along with it. + if !directories.insert(directory.to_path_buf()) { + break; + } + } + } + directories +} + #[cfg(test)] mod tests { use super::*; @@ -87,4 +102,25 @@ mod tests { assert!(tracked.get(&tracked_file).unwrap()); assert!(!tracked.contains_key(&backend_dir.join("backend/app/models/foo.rb"))); } + + #[test] + fn test_tracked_directories() { + let base_path = Path::new("/project"); + let tracked: HashMap = ["app/models/user.rb", "app/models/concerns/named.rb", "vendored/submodule_gem"] + .iter() + .map(|file| (base_path.join(file), true)) + .collect(); + + let directories = tracked_directories(base_path, &tracked); + for directory in ["", "app", "app/models", "app/models/concerns", "vendored", "vendored/submodule_gem"] { + assert!(directories.contains(Path::new(directory)), "{directory}"); + } + assert!(!directories.contains(Path::new("app/assets"))); + } + + #[test] + fn test_tracked_directories_always_include_the_root() { + let expected: HashSet = HashSet::from([PathBuf::new()]); + assert_eq!(tracked_directories(Path::new("/project"), &HashMap::new()), expected); + } } diff --git a/tests/untracked_directory_test.rs b/tests/untracked_directory_test.rs new file mode 100644 index 0000000..3e5c55d --- /dev/null +++ b/tests/untracked_directory_test.rs @@ -0,0 +1,85 @@ +use assert_cmd::prelude::*; +use std::{error::Error, fs, path::Path, process::Command}; + +mod common; +use common::{git_add_all_files, setup_fixture_repo}; + +const FIXTURE: &str = "tests/fixtures/valid_project"; + +fn generate(project_root: &Path) -> Result> { + let codeowners_path = project_root.join("tmp/CODEOWNERS"); + fs::create_dir_all(codeowners_path.parent().unwrap())?; + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--codeowners-file-path") + .arg(&codeowners_path) + .arg("--no-cache") + .arg("generate") + .arg("--skip-stage") + .assert() + .success(); + Ok(fs::read_to_string(&codeowners_path)?) +} + +/// The walk doesn't enter a directory that holds no tracked file, so a vendored gem that exists only in one checkout doesn't +/// change the CODEOWNERS file that checkout generates. +#[test] +fn test_generate_leaves_out_a_vendored_gem_with_no_tracked_files() -> Result<(), Box> { + let temp_dir = setup_fixture_repo(Path::new(FIXTURE)); + let project_root = temp_dir.path(); + git_add_all_files(project_root); + let untrack = Command::new("git") + .args(["rm", "-r", "-q", "--cached", "gems/pets"]) + .current_dir(project_root) + .output()?; + assert!(untrack.status.success(), "{}", String::from_utf8_lossy(&untrack.stderr)); + + let codeowners = generate(project_root)?; + assert!(codeowners.contains("/gems/payroll_calculator/**/** @PayrollTeam"), "{codeowners}"); + assert!(!codeowners.contains("/gems/pets/"), "{codeowners}"); + Ok(()) +} + +/// `git ls-files` lists a submodule as one path, without its contents, so a vendored gem that's a submodule is tracked too. +#[test] +fn test_generate_keeps_a_vendored_gem_that_is_a_submodule() -> Result<(), Box> { + let temp_dir = setup_fixture_repo(Path::new(FIXTURE)); + let project_root = temp_dir.path(); + git_add_all_files(project_root); + for args in [ + vec!["rm", "-r", "-q", "--cached", "gems/pets"], + vec![ + "update-index", + "--add", + "--cacheinfo", + "160000,0123456789abcdef0123456789abcdef01234567,gems/pets", + ], + ] { + let output = Command::new("git").args(&args).current_dir(project_root).output()?; + assert!(output.status.success(), "{}", String::from_utf8_lossy(&output.stderr)); + } + + let codeowners = generate(project_root)?; + assert!(codeowners.contains("/gems/pets/**/** @UX"), "{codeowners}"); + Ok(()) +} + +/// Team files count before they're tracked (rubyatscale/code_ownership#149), so the walk still enters an untracked directory +/// that the team file glob can match in. +#[test] +fn test_generate_reads_an_untracked_team_file_in_a_new_directory() -> Result<(), Box> { + let temp_dir = setup_fixture_repo(Path::new(FIXTURE)); + let project_root = temp_dir.path(); + git_add_all_files(project_root); + fs::create_dir_all(project_root.join("config/teams/docs"))?; + fs::write( + project_root.join("config/teams/docs/docs.yml"), + "name: Docs\ngithub:\n team: '@DocsTeam'\nowned_globs:\n - ruby/app/models/bank_account.rb\n", + )?; + + let codeowners = generate(project_root)?; + assert!(codeowners.contains("/config/teams/docs/docs.yml @DocsTeam"), "{codeowners}"); + assert!(codeowners.contains("/ruby/app/models/bank_account.rb @DocsTeam"), "{codeowners}"); + Ok(()) +}