diff --git a/README.md b/README.md index d99237b..47269f8 100644 --- a/README.md +++ b/README.md @@ -170,14 +170,16 @@ js_package_paths: - `generate` (`g`): Generate the CODEOWNERS file and write it to `--codeowners-file-path`. - Flags: `--skip-stage, -s` to avoid `git add` after writing -- `validate` (`v`): Validate the CODEOWNERS file and configuration. -- `generate-and-validate` (`gv`): Run `generate` then `validate`. +- `validate [files...]` (`v`): Validate the CODEOWNERS file and configuration. Given files, it checks only their ownership, a fast mode for git hooks. +- `generate-and-validate [files...]` (`gv`): Run `generate` then `validate`. - Flags: `--skip-stage, -s` - `for-file ` (`f`): Print the owner of a file. - Flags: `--from-codeowners` to resolve using only the CODEOWNERS rules - `for-team ` (`t`): Print ownership report for a team. - `delete-cache` (`d`): Delete the persisted cache. +A path given to `validate`, `gv` or `for-file` may be relative to the project root or absolute. + ### Examples #### Find the owner of a file diff --git a/src/ownership/codeowners_file_parser.rs b/src/ownership/codeowners_file_parser.rs index a59d855..a9b2200 100644 --- a/src/ownership/codeowners_file_parser.rs +++ b/src/ownership/codeowners_file_parser.rs @@ -19,43 +19,39 @@ pub struct Parser { } impl Parser { - pub fn teams_from_files_paths(&self, file_paths: &[PathBuf]) -> Result>, Box> { - let file_inputs: Vec<(String, String)> = file_paths + /// The team CODEOWNERS gives each path, in the order given. + pub fn teams_from_files_paths(&self, file_paths: &[PathBuf]) -> Result>, Box> { + let prefixed_file_paths: Vec = file_paths .iter() .map(|path| { let file_path_str = path .to_str() .ok_or(IoError::new(std::io::ErrorKind::InvalidInput, "Invalid file path"))?; - let original = file_path_str.to_string(); - let prefixed = if file_path_str.starts_with('/') { - original.clone() + Ok(if file_path_str.starts_with('/') { + file_path_str.to_string() } else { format!("/{}", file_path_str) - }; - Ok((original, prefixed)) + }) }) .collect::, IoError>>()?; - if file_inputs.is_empty() { - return Ok(HashMap::new()); + if prefixed_file_paths.is_empty() { + return Ok(Vec::new()); } let codeowners_entries = parse_codeowners_entries(self.codeowners_file_path.to_string_lossy().into_owned()); let teams_by_name = teams_by_github_team_name(self.absolute_team_files_globs(), self.node_limit.clone()); - let result: HashMap> = file_inputs + Ok(prefixed_file_paths .par_iter() - .map(|(key, prefixed)| { - let team = codeowners_entries + .map(|prefixed| { + codeowners_entries .iter() .find(|(glob, _)| glob_match(glob, prefixed)) - .and_then(|(_, team_name)| teams_by_name.get(team_name).cloned()); - (key.clone(), team) + .and_then(|(_, team_name)| teams_by_name.get(team_name).cloned()) }) - .collect(); - - Ok(result) + .collect()) } fn absolute_team_files_globs(&self) -> Vec { @@ -518,7 +514,7 @@ mod tests { }, }; let teams = parser.teams_from_files_paths(&[PathBuf::from("app/a.rb")])?; - Ok(teams["app/a.rb"].as_ref().map(|team| team.name.clone())) + Ok(teams[0].as_ref().map(|team| team.name.clone())) }; assert_eq!(owner_of(crate::yaml::DEFAULT_MAX_NODES)?, Some("Payroll".to_string())); diff --git a/src/ownership/codeowners_query.rs b/src/ownership/codeowners_query.rs index eb58ee2..9c54209 100644 --- a/src/ownership/codeowners_query.rs +++ b/src/ownership/codeowners_query.rs @@ -15,11 +15,8 @@ pub(crate) fn teams_for_files_from_codeowners( .iter() .map(Path::new) .map(|path| { - if path.is_absolute() { - crate::path_utils::relative_to_buf(project_root, path) - } else { - path.to_path_buf() - } + crate::path_utils::project_relative_path(project_root, path) + .unwrap_or_else(|| crate::path_utils::unplaced_path(project_root, path).to_path_buf()) }) .collect(); @@ -30,5 +27,7 @@ pub(crate) fn teams_for_files_from_codeowners( node_limit, }; - parser.teams_from_files_paths(&relative_file_paths).map_err(|e| e.to_string()) + // Keyed by each path as given, so two paths that name the same file are separate entries. + let teams = parser.teams_from_files_paths(&relative_file_paths).map_err(|e| e.to_string())?; + Ok(file_paths.iter().cloned().zip(teams).collect()) } diff --git a/src/ownership/file_owner_resolver.rs b/src/ownership/file_owner_resolver.rs index 23a5fcc..deadaf1 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -13,12 +13,18 @@ use crate::{config::Config, project::Team, project_file_builder::build_project_f use super::{FileOwner, mapper::Source}; pub fn find_file_owners(project_root: &Path, config: &Config, file_path: &Path) -> Result, String> { - let absolute_file_path = if file_path.is_absolute() { - file_path.to_path_buf() - } else { - project_root.join(file_path) + let (absolute_file_path, relative_file_path) = match crate::path_utils::project_relative_path(project_root, file_path) { + Some(relative_file_path) => (project_root.join(&relative_file_path), relative_file_path), + None => { + let absolute_file_path = if file_path.is_absolute() { + file_path.to_path_buf() + } else { + project_root.join(file_path) + }; + let relative_file_path = crate::path_utils::relative_to_buf(project_root, &absolute_file_path); + (absolute_file_path, relative_file_path) + } }; - let relative_file_path = crate::path_utils::relative_to_buf(project_root, &absolute_file_path); let loaded = loaded_teams( teams_cache_root(project_root), diff --git a/src/path_utils.rs b/src/path_utils.rs index dc98e00..50369ae 100644 --- a/src/path_utils.rs +++ b/src/path_utils.rs @@ -1,4 +1,6 @@ -use std::path::{Component, Path, PathBuf}; +use std::path::{Path, PathBuf}; + +use path_clean::PathClean; /// Return `path` relative to `root` if possible; otherwise return `path` unchanged. pub fn relative_to<'a>(root: &'a Path, path: &'a Path) -> &'a Path { @@ -10,18 +12,48 @@ pub fn relative_to_buf(root: &Path, path: &Path) -> PathBuf { relative_to(root, path).to_path_buf() } -/// `path` without `.` components or repeated separators, or `None` if it has a `..`, a root or a prefix. Owners are matched -/// against plain project-relative paths, and no other path can be made into one without asking the file system. -pub(crate) fn plain_relative(path: &Path) -> Option { - let mut plain = PathBuf::new(); - for component in path.components() { - match component { - Component::CurDir => {} - Component::Normal(name) => plain.push(name), - Component::ParentDir | Component::RootDir | Component::Prefix(_) => return None, +/// The path under `project_root` that `path` names, relative to the root, or `None` if it names nothing under it. A relative +/// `path` is relative to the root. Owners are matched against these paths, so another form of one, with a `./`, a repeated `/`, +/// a `.` or `..` segment, or through a symlink, would miss them. `..` is resolved lexically, as for `--config-path`, so after a +/// symlinked directory it names the link's parent. +pub(crate) fn project_relative_path(project_root: &Path, path: &Path) -> Option { + // `absolute` fails only for an empty root, or a relative one with no working directory, and neither can be resolved. + let root = std::path::absolute(project_root).ok()?.clean(); + let cleaned = root.join(path).clean(); + if let Some(relative) = path_under(&root, &cleaned) { + return Some(relative); + } + // The path may reach the project through a symlink, or the root may be the unresolved side, as a library caller's + // RunConfig can be, so both are resolved. Its ancestors are resolved from the shortest until one is in the project, and the + // rest of the path is kept as written, so it names what its plain form does: a symlink inside the project keeps its own + // path, and a file or directory that doesn't exist, or can't be read, is still named. The file itself is never resolved, + // since CODEOWNERS and the project walk name a symlinked file by its own path, not its target's. + let canonical_root = root.canonicalize().ok()?; + let ancestors: Vec<&Path> = cleaned.ancestors().skip(1).collect(); + for ancestor in ancestors.into_iter().rev() { + if let Ok(resolved) = ancestor.canonicalize() + && resolved.starts_with(&canonical_root) + { + return path_under(&canonical_root, &resolved.join(cleaned.strip_prefix(ancestor).ok()?)); } } - Some(plain) + None +} + +/// How owners are matched against a path that `project_relative_path` can't place: as given, but without the root if it's +/// absolute, so it matches as its relative form does. +pub(crate) fn unplaced_path<'a>(project_root: &'a Path, path: &'a Path) -> &'a Path { + if path.is_absolute() { + relative_to(project_root, path) + } else { + path + } +} + +// The root itself names nothing under it. +fn path_under(root: &Path, path: &Path) -> Option { + let relative = path.strip_prefix(root).ok()?; + (!relative.as_os_str().is_empty()).then(|| relative.to_path_buf()) } /// Returns true if `path` matches any of the provided glob patterns. @@ -89,25 +121,158 @@ mod tests { } // Compared as strings, as globs see them, since Path equality ignores a repeated separator or a `.` past the start. - fn plain(path: &str) -> Option { - plain_relative(Path::new(path)).map(|plain| plain.to_string_lossy().into_owned()) + fn relative(project_root: &Path, path: impl AsRef) -> Option { + project_relative_path(project_root, path.as_ref()).map(|relative| relative.to_string_lossy().into_owned()) + } + + #[test] + fn project_relative_path_returns_a_plain_path_unchanged() { + assert_eq!(relative(Path::new("/proj"), "app/a.rb").as_deref(), Some("app/a.rb")); + } + + #[test] + fn project_relative_path_drops_dot_components_and_repeated_separators() { + for path in ["./app/a.rb", "app//a.rb", "app/./a.rb"] { + assert_eq!(relative(Path::new("/proj"), path).as_deref(), Some("app/a.rb"), "{path}"); + } + } + + #[test] + fn project_relative_path_resolves_dot_dot_inside_the_project() { + for path in ["app/../app/a.rb", "app/models/../a.rb", "../proj/app/a.rb", "/proj/app/../app/a.rb"] { + assert_eq!(relative(Path::new("/proj"), path).as_deref(), Some("app/a.rb"), "{path}"); + } + } + + #[test] + fn project_relative_path_returns_none_for_dot_dot_out_of_the_project() { + // A real root, so the paths are also resolved. + let temp_dir = tempfile::tempdir().unwrap(); + let root = temp_dir.path().join("proj"); + std::fs::create_dir_all(root.join("app")).unwrap(); + let absolute = |path: &str| root.join(path).to_string_lossy().into_owned(); + for path in [ + "../a.rb", + "../../app/a.rb", + "app/../../a.rb", + &absolute("../a.rb"), + &absolute("../../app/a.rb"), + ] { + assert_eq!(relative(&root, path), None, "{path}"); + } } #[test] - fn plain_relative_returns_a_plain_path_unchanged() { - assert_eq!(plain("a/b").as_deref(), Some("a/b")); + fn project_relative_path_strips_the_root_from_an_absolute_path() { + assert_eq!(relative(Path::new("/proj"), "/proj/app/a.rb").as_deref(), Some("app/a.rb")); + assert_eq!(relative(Path::new("/proj/"), "/proj//app/a.rb").as_deref(), Some("app/a.rb")); + assert_eq!(relative(Path::new("/proj"), "/elsewhere/app/a.rb"), None); } #[test] - fn plain_relative_drops_dot_components_and_repeated_separators() { - assert_eq!(plain("./a/b").as_deref(), Some("a/b")); - assert_eq!(plain("a/./b").as_deref(), Some("a/b")); - assert_eq!(plain("a//b").as_deref(), Some("a/b")); + fn project_relative_path_resolves_dot_dot_in_the_root() { + assert_eq!(relative(Path::new("/x/../proj"), "app/a.rb").as_deref(), Some("app/a.rb")); + assert_eq!(relative(Path::new("/x/../proj"), "/proj/app/a.rb").as_deref(), Some("app/a.rb")); } #[test] - fn plain_relative_returns_none_for_parent_dirs_and_roots() { - assert_eq!(plain("a/../b"), None); - assert_eq!(plain("/a/b"), None); + fn project_relative_path_takes_a_relative_root_from_the_working_directory() { + let cwd = std::env::current_dir().unwrap(); + assert_eq!(relative(Path::new("."), "./src/lib.rs").as_deref(), Some("src/lib.rs")); + assert_eq!(relative(Path::new("."), cwd.join("src/lib.rs")).as_deref(), Some("src/lib.rs")); + assert_eq!(relative(Path::new("src"), "./lib.rs").as_deref(), Some("lib.rs")); + assert_eq!(relative(Path::new("src"), cwd.join("src/lib.rs")).as_deref(), Some("lib.rs")); + assert_eq!(relative(Path::new("src"), "src/lib.rs").as_deref(), Some("src/lib.rs")); + // One that doesn't exist can't be resolved, so only the working directory places it. + assert_eq!(relative(Path::new("."), "./no/such/dir/a.rb").as_deref(), Some("no/such/dir/a.rb")); + } + + // A project root with no symlink on its path, the same root through a symlink, and the temp dir holding both. In the + // project, app/lib_link is a symlink to its lib directory. + #[cfg(unix)] + fn symlinked_project() -> (PathBuf, PathBuf, tempfile::TempDir) { + let temp_dir = tempfile::tempdir().unwrap(); + // On macOS the temp dir is itself reached through a symlink, from /var to /private/var. + let base = temp_dir.path().canonicalize().unwrap(); + let root = base.join("real/proj"); + std::fs::create_dir_all(root.join("app")).unwrap(); + std::fs::create_dir_all(root.join("lib")).unwrap(); + std::fs::write(root.join("app/a.rb"), "").unwrap(); + std::os::unix::fs::symlink("../lib", root.join("app/lib_link")).unwrap(); + std::os::unix::fs::symlink(base.join("real"), base.join("link")).unwrap(); + (root, base.join("link/proj"), temp_dir) + } + + #[cfg(unix)] + #[test] + fn project_relative_path_resolves_a_path_through_a_symlink_to_the_project() { + let (root, linked_root, _temp_dir) = symlinked_project(); + assert_eq!(relative(&root, linked_root.join("app/a.rb")).as_deref(), Some("app/a.rb")); + assert_eq!(relative(&root, linked_root.join("a.rb")).as_deref(), Some("a.rb")); + assert_eq!(relative(&root, linked_root.join("app/../app/./a.rb")).as_deref(), Some("app/a.rb")); + assert_eq!(relative(&root, linked_root.join("../a.rb")), None); + } + + #[cfg(unix)] + #[test] + fn project_relative_path_resolves_a_root_given_through_a_symlink() { + let (root, linked_root, _temp_dir) = symlinked_project(); + assert_eq!(relative(&linked_root, root.join("app/a.rb")).as_deref(), Some("app/a.rb")); + assert_eq!(relative(&linked_root, "./app/a.rb").as_deref(), Some("app/a.rb")); + } + + #[cfg(unix)] + #[test] + fn project_relative_path_keeps_the_path_of_a_symlinked_file() { + let (root, linked_root, _temp_dir) = symlinked_project(); + std::os::unix::fs::symlink("a.rb", root.join("app/link.rb")).unwrap(); + assert_eq!(relative(&root, linked_root.join("app/link.rb")).as_deref(), Some("app/link.rb")); + assert_eq!(relative(&linked_root, root.join("app/link.rb")).as_deref(), Some("app/link.rb")); + // So one outside the project names nothing in it. + let outside_link = root.ancestors().nth(2).unwrap().join("link.rb"); + std::os::unix::fs::symlink(root.join("app/a.rb"), &outside_link).unwrap(); + assert_eq!(relative(&root, &outside_link), None); + } + + #[cfg(unix)] + #[test] + fn project_relative_path_resolves_a_path_through_a_symlink_only_as_far_as_the_project() { + let (root, linked_root, _temp_dir) = symlinked_project(); + // Symlinks to a directory out of the project and to itself, which can't be resolved, and a directory that doesn't exist. + std::os::unix::fs::symlink(root.ancestors().nth(2).unwrap(), root.join("app/out_link")).unwrap(); + std::os::unix::fs::symlink("loop", root.join("app/loop")).unwrap(); + for path in [ + "app/lib_link/b.rb", + "app/out_link/b.rb", + "app/loop/b.rb", + "app/new/b.rb", + "app/deleted.rb", + ] { + assert_eq!(relative(&root, path).as_deref(), Some(path), "{path}"); + assert_eq!(relative(&root, linked_root.join(path)).as_deref(), Some(path), "{path}"); + } + } + + #[cfg(unix)] + #[test] + fn project_relative_path_resolves_dot_dot_before_any_symlink() { + let (root, linked_root, _temp_dir) = symlinked_project(); + let base = root.ancestors().nth(2).unwrap(); + std::os::unix::fs::symlink(root.join("app"), base.join("app_link")).unwrap(); + // Each `..` names the link's parent, not its target's. + assert_eq!( + relative(&root, linked_root.join("app/lib_link/../a.rb")).as_deref(), + Some("app/a.rb") + ); + assert_eq!(relative(&root, base.join("app_link/../a.rb")), None); + } + + #[test] + fn project_relative_path_returns_none_for_the_root_itself() { + let temp_dir = tempfile::tempdir().unwrap(); + let root = temp_dir.path(); + for path in [Path::new(""), Path::new("."), Path::new("app/.."), root, &root.join(".")] { + assert_eq!(relative(root, path), None, "{}", path.display()); + } } } diff --git a/src/runner.rs b/src/runner.rs index f4397e5..d79fcce 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -10,7 +10,7 @@ use crate::{ cache::{Cache, Caching, file::GlobalCache, noop::NoopCache}, config::Config, ownership::{FileOwner, Ownership}, - path_utils::{matches_globs, plain_relative, relative_to_buf}, + path_utils::{matches_globs, project_relative_path, unplaced_path}, project_builder::ProjectBuilder, }; @@ -150,32 +150,23 @@ impl Runner { // Filter files based on owned_globs and unowned_globs configuration // Only validate files that match owned_globs and don't match unowned_globs - let filtered_paths: Vec<(String, PathBuf)> = file_paths + let filtered_paths: Vec<(String, Option)> = file_paths .into_iter() .map(|file_path| { - let path = Path::new(&file_path); - // As in the CODEOWNERS lookup, a relative path is already relative to the project root, even when that root is relative. - let relative_path = if path.is_absolute() { - relative_to_buf(&self.run_config.project_root, path) - } else { - path.to_path_buf() - }; + let relative_path = project_relative_path(&self.run_config.project_root, Path::new(&file_path)); (file_path, relative_path) }) // Mirror the filtering applied by ProjectBuilder when walking the project - .filter(|(_, relative_path)| { - matches_globs(relative_path, &self.config.owned_globs) && !matches_globs(relative_path, &self.config.unowned_globs) + .filter(|(file_path, relative_path)| { + let path = relative_path + .as_deref() + .unwrap_or_else(|| unplaced_path(&self.run_config.project_root, Path::new(file_path))); + matches_globs(path, &self.config.owned_globs) && !matches_globs(path, &self.config.unowned_globs) }) .collect(); // Built only if an optionally owned file needs it, and then once. let owner_lookup = LazyCell::new(|| self.ownership.owner_lookup()); - let listed_in_codeowners = |path: &Path| { - matches!( - team_for_file_from_codeowners(&self.run_config, &path.to_string_lossy()), - Ok(Some(_)) - ) - }; debug_span!("per_file_query").in_scope(|| { for (file_path, relative_path) in filtered_paths { @@ -185,18 +176,13 @@ impl Runner { // package names a team that doesn't exist, or whose owner CODEOWNERS doesn't list yet, is still reported, // but not as unowned, which the key allows. Only files the project reads count, though, so an annotated // file, `.codeowner` or package file names no owner here if it is untracked or under a top-level directory - // named in ignore_dirs, as in `validate`. The lookup matches plain project-relative paths only, so a path - // with a `..`, or an absolute one not under the project root, is never excused and is reported as unowned, - // as without the key. - Ok(None) => { - match plain_relative(&relative_path).filter(|path| matches_globs(path, &self.config.optionally_owned_globs)) { - Some(path) if !owner_lookup.has_owner(&path) => {} - // The CODEOWNERS lookup above read the path as given, so a `./`, `//` or inner `.` missed the file's line. - Some(path) if path.as_os_str() != relative_path.as_os_str() && listed_in_codeowners(&path) => {} - Some(_) => optionally_owned_files_with_unlisted_owners.push(file_path), - None => unowned_files.push(file_path), - } - } + // named in ignore_dirs, as in `validate`. A path that names nothing under the project root is never + // excused and is reported as unowned, as without the key. + Ok(None) => match relative_path.filter(|path| matches_globs(path, &self.config.optionally_owned_globs)) { + Some(path) if !owner_lookup.has_owner(&path) => {} + Some(_) => optionally_owned_files_with_unlisted_owners.push(file_path), + None => unowned_files.push(file_path), + }, Err(err) => io_errors.push(format!("{}: {}", file_path, err)), } } diff --git a/src/runner/api.rs b/src/runner/api.rs index f9073ef..a03814c 100644 --- a/src/runner/api.rs +++ b/src/runner/api.rs @@ -58,7 +58,7 @@ pub fn team_for_file(run_config: &RunConfig, file_path: &str) -> Result]) -> Result> { + Ok(Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .args(args) + .assert()) +} + +// A symlink to `target`, in a temp dir that is removed when the returned TempDir is dropped. +#[cfg(unix)] +#[allow(dead_code)] +pub fn symlink_in_temp_dir(target: &Path) -> (std::path::PathBuf, TempDir) { + let temp_dir = tempfile::tempdir().expect("failed to create tempdir"); + let link = temp_dir.path().join("link"); + std::os::unix::fs::symlink(target, &link).expect("failed to create symlink"); + (link, temp_dir) +} + #[allow(dead_code)] pub fn teardown() { glob::glob("tests/fixtures/*/tmp/cache/codeowners") diff --git a/tests/optionally_owned_globs_test.rs b/tests/optionally_owned_globs_test.rs index c916837..a9a929e 100644 --- a/tests/optionally_owned_globs_test.rs +++ b/tests/optionally_owned_globs_test.rs @@ -1,11 +1,11 @@ -use assert_cmd::{assert::Assert, prelude::*}; +use assert_cmd::assert::Assert; use indoc::indoc; use predicates::prelude::*; -use std::{error::Error, ffi::OsStr, fs, path::Path, process::Command}; +use std::{error::Error, ffi::OsStr, fs, path::Path}; use tempfile::TempDir; mod common; -use common::{OutputStream, git_add_all_files, run_codeowners, setup_fixture_repo}; +use common::{OutputStream, codeowners, git_add_all_files, run_codeowners, setup_fixture_repo}; const FIXTURE: &str = "optionally_owned_globs"; @@ -111,15 +111,6 @@ fn modified_fixture(modify: impl FnOnce(&Path) -> std::io::Result<()>) -> Result Ok(temp_dir) } -fn codeowners(project_root: &Path, args: &[impl AsRef]) -> Result> { - Ok(Command::cargo_bin("codeowners")? - .arg("--project-root") - .arg(project_root) - .arg("--no-cache") - .args(args) - .assert()) -} - fn run_on_modified_fixture(modify: impl FnOnce(&Path) -> std::io::Result<()>, args: &[&str]) -> Result> { let temp_dir = modified_fixture(modify)?; codeowners(temp_dir.path(), args) @@ -270,20 +261,37 @@ fn test_validate_files_reports_an_optionally_owned_file_whose_annotation_names_a #[test] fn test_validate_files_reports_an_optionally_owned_file_whose_annotation_names_an_unknown_team_by_path_with_dot_dot() -> Result<(), Box> { - // The owner lookup can't see the annotation without resolving the `..`, so the file mustn't be excused. It's reported - // as unowned, as without the key. for command in ["validate", "gv"] { run_on_modified_fixture( add_an_annotation_naming_an_unknown_team_in_an_optionally_owned_glob, &[command, "app/deprecated/../deprecated/typo.rb"], )? .failure() - .stdout(predicate::str::contains("Unowned files detected:")) - .stdout(predicate::str::contains("app/deprecated/../deprecated/typo.rb")); + .stdout(predicate::eq(format!( + "{}\n app/deprecated/../deprecated/typo.rb\n", + unlisted_owner_heading(command) + ))); } Ok(()) } +#[cfg(unix)] +#[test] +fn test_validate_files_reports_an_optionally_owned_file_whose_annotation_names_an_unknown_team_by_path_through_a_symlink() +-> Result<(), Box> { + let temp_dir = modified_fixture(add_an_annotation_naming_an_unknown_team_in_an_optionally_owned_glob)?; + let (link, _link_dir) = common::symlink_in_temp_dir(temp_dir.path()); + let file = link.join("app/deprecated/typo.rb"); + codeowners(temp_dir.path(), &[OsStr::new("validate"), file.as_os_str()])? + .failure() + .stdout(predicate::eq(format!( + "{}\n {}\n", + unlisted_owner_heading("validate"), + file.display() + ))); + Ok(()) +} + fn own_every_ruby_file_and_annotate_an_optionally_owned_one_with_an_unknown_team(project_root: &Path) -> std::io::Result<()> { add_an_annotation_naming_an_unknown_team_in_an_optionally_owned_glob(project_root)?; Config { @@ -343,6 +351,16 @@ fn test_validate_files_looks_up_an_optionally_owned_file_in_codeowners_by_its_pl Ok(()) } +#[test] +fn test_validate_files_does_not_excuse_a_path_outside_the_project() -> Result<(), Box> { + for command in ["validate", "gv"] { + run_on_modified_fixture(make_every_file_optionally_owned, &[command, "../outside.rb"])? + .failure() + .stdout(predicate::eq("Unowned files detected:\n ../outside.rb\n")); + } + Ok(()) +} + fn add_a_package_naming_an_unknown_team_in_an_optionally_owned_glob(project_root: &Path) -> std::io::Result<()> { fs::create_dir_all(project_root.join("packs/typo/app"))?; fs::write(project_root.join("packs/typo/package.yml"), "owner: Typo\n")?; diff --git a/tests/path_normalization_test.rs b/tests/path_normalization_test.rs new file mode 100644 index 0000000..2dc9f9a --- /dev/null +++ b/tests/path_normalization_test.rs @@ -0,0 +1,249 @@ +use indoc::indoc; +use predicates::prelude::*; +use std::{error::Error, fs, path::Path}; +use tempfile::TempDir; + +mod common; +use common::{codeowners, git_add_all_files, init_git_repo}; + +// The project from #145, with CODEOWNERS generated: app/valid.rb names its team in an annotation, and app/unowned.rb has no owner. +fn project(owned_glob: &str) -> Result> { + let temp_dir = tempfile::tempdir()?; + project_at(temp_dir.path(), &format!("owned_globs:\n - \"{owned_glob}\"\n"))?; + Ok(temp_dir) +} + +// The same project at `root`, with `config` as its config/code_ownership.yml. +fn project_at(root: &Path, config: &str) -> Result<(), Box> { + fs::create_dir_all(root.join("app"))?; + fs::create_dir_all(root.join("config/teams"))?; + fs::write(root.join("config/code_ownership.yml"), config)?; + fs::write(root.join("config/teams/valid.yml"), "name: Valid\ngithub:\n team: '@org/valid'\n")?; + fs::write(root.join("app/valid.rb"), "# @team Valid\n")?; + fs::write(root.join("app/unowned.rb"), "puts 1\n")?; + init_git_repo(root); + git_add_all_files(root); + codeowners(root, &["generate"])?.success(); + Ok(()) +} + +const FOR_FILE_FROM_ANNOTATION: &str = indoc! {" + Team: Valid + Github Team: @org/valid + Team YML: config/teams/valid.yml + Description: + - Owner annotation at the top of the file +"}; + +const FOR_FILE_FROM_CODEOWNERS: &str = indoc! {" + Team: Valid + Github Team: @org/valid + Team YML: config/teams/valid.yml + Description: + - Owner inferred from codeowners file +"}; + +#[test] +fn test_validate_files_finds_an_owned_file_by_any_form_of_its_path() -> Result<(), Box> { + // `**/*.rb` matches each of these as given, so each reaches the CODEOWNERS lookup, which has a line for the plain path only. + let project = project("**/*.rb")?; + for file in [ + "app/valid.rb", + "./app/valid.rb", + "app//valid.rb", + "app/./valid.rb", + "app/../app/valid.rb", + ] { + for command in ["validate", "gv"] { + codeowners(project.path(), &[command, file])?.success().stdout(predicate::eq("")); + } + } + Ok(()) +} + +#[cfg(unix)] +#[test] +fn test_validate_files_finds_an_owned_file_by_a_path_through_a_symlink() -> Result<(), Box> { + // The CLI canonicalizes the project root, which a path through a symlink to the project doesn't start with. + let project = project("**/*.rb")?; + let (link, _link_dir) = common::symlink_in_temp_dir(project.path()); + codeowners(project.path(), &["validate", link.join("app/valid.rb").to_str().unwrap()])? + .success() + .stdout(predicate::eq("")); + Ok(()) +} + +#[test] +fn test_validate_files_reports_an_unowned_file_by_a_dot_slash_path() -> Result<(), Box> { + // `{app,config}/**/*.rb` doesn't match the path as given, so matched that way, the file would be left out and pass. + let project = project("{app,config}/**/*.rb")?; + for command in ["validate", "gv"] { + codeowners(project.path(), &[command, "./app/unowned.rb"])? + .failure() + .stdout(predicate::eq("Unowned files detected:\n ./app/unowned.rb\n")); + } + Ok(()) +} + +#[cfg(unix)] +#[test] +fn test_validate_files_reports_an_unowned_file_by_a_path_through_a_symlink() -> Result<(), Box> { + // A file that doesn't exist, in a directory that doesn't either, is reported as its plain path would be. + let project = project("{app,config}/**/*.rb")?; + let (link, _link_dir) = common::symlink_in_temp_dir(project.path()); + for file in ["app/unowned.rb", "app/deleted/unowned.rb"] { + let path = link.join(file); + codeowners(project.path(), &["validate", path.to_str().unwrap()])? + .failure() + .stdout(predicate::eq(format!("Unowned files detected:\n {}\n", path.display()))); + } + Ok(()) +} + +#[test] +fn test_validate_files_leaves_out_a_file_in_unowned_globs_by_any_form_of_its_path() -> Result<(), Box> { + // `**/*.rb` matches each of these as given, but `app/vendor/**/*` only the plain path. + let project = tempfile::tempdir()?; + fs::create_dir_all(project.path().join("app/vendor"))?; + fs::write(project.path().join("app/vendor/lib.rb"), "puts 1\n")?; + project_at( + project.path(), + indoc! {r#" + owned_globs: + - "**/*.rb" + unowned_globs: + - "app/vendor/**/*" + "#}, + )?; + for file in ["app/vendor/lib.rb", "./app/vendor/lib.rb", "app//vendor/lib.rb"] { + for command in ["validate", "gv"] { + codeowners(project.path(), &[command, file])?.success().stdout(predicate::eq("")); + } + } + Ok(()) +} + +#[test] +fn test_validate_files_reports_a_path_outside_the_project_as_unowned() -> Result<(), Box> { + // `**/*.rb` matches these as given, and CODEOWNERS has no line for them. + let project = project("**/*.rb")?; + let outside = project.path().parent().ok_or("temp dir has no parent")?.join("outside.rb"); + for file in ["../outside.rb", outside.to_str().unwrap()] { + codeowners(project.path(), &["validate", file])? + .failure() + .stdout(predicate::eq(format!("Unowned files detected:\n {file}\n"))); + } + Ok(()) +} + +#[test] +fn test_validate_files_reports_a_path_that_leaves_the_project_by_either_form() -> Result<(), Box> { + // Without the root, the absolute form matches `{app,config}/**/*.rb`, as the relative one does. + let project = project("{app,config}/**/*.rb")?; + let absolute = project.path().canonicalize()?.join("app/../../outside.rb"); + for file in ["app/../../outside.rb", absolute.to_str().unwrap()] { + codeowners(project.path(), &["validate", file])? + .failure() + .stdout(predicate::eq(format!("Unowned files detected:\n {file}\n"))); + } + Ok(()) +} + +#[test] +fn test_teams_for_files_from_codeowners_looks_up_a_path_that_leaves_the_project_alike_by_either_form() -> Result<(), Box> { + let project = project("**/*.rb")?; + fs::create_dir_all(project.path().join("app/legacy"))?; + fs::write(project.path().join("app/legacy/.codeowner"), "Valid\n")?; + git_add_all_files(project.path()); + codeowners(project.path(), &["generate"])?.success(); + let run_config = common::build_run_config(project.path(), ".github/CODEOWNERS"); + let relative = "app/legacy/../../../outside.rb".to_string(); + let absolute = run_config.project_root.join(&relative).to_string_lossy().into_owned(); + + let teams = codeowners::runner::teams_for_files_from_codeowners(&run_config, &[relative.clone(), absolute.clone()])?; + + let team_name = |file_path: &String| teams[file_path].as_ref().map(|team| team.name.clone()); + // CODEOWNERS has `/app/legacy/**/**`, which the relative form matches, since `**` matches `..`. + assert_eq!(team_name(&relative).as_deref(), Some("Valid")); + assert_eq!(team_name(&absolute), team_name(&relative)); + Ok(()) +} + +#[cfg(unix)] +#[test] +fn test_validate_files_and_for_file_read_dot_dot_after_a_symlinked_directory_as_its_parent() -> Result<(), Box> { + // `..` is resolved before any symlink, so this names app/valid.rb, not config/valid.rb. + let project = project("**/*.rb")?; + std::os::unix::fs::symlink("../config/teams", project.path().join("app/teams"))?; + codeowners(project.path(), &["validate", "app/teams/../valid.rb"])? + .success() + .stdout(predicate::eq("")); + codeowners(project.path(), &["for-file", "app/teams/../valid.rb"])? + .success() + .stdout(predicate::str::starts_with("Team: Valid\n")); + Ok(()) +} + +#[test] +fn test_for_file_from_codeowners_finds_the_owner_by_a_dot_slash_path() -> Result<(), Box> { + let project = project("**/*.rb")?; + codeowners(project.path(), &["for-file", "--from-codeowners", "./app/valid.rb"])? + .success() + .stdout(predicate::eq(FOR_FILE_FROM_CODEOWNERS)); + Ok(()) +} + +#[cfg(unix)] +#[test] +fn test_for_file_finds_the_owner_by_a_path_through_a_symlink() -> Result<(), Box> { + let project = project("{app,config}/**/*.rb")?; + let (link, _link_dir) = common::symlink_in_temp_dir(project.path()); + let file = link.join("app/valid.rb"); + let file = file.to_str().unwrap(); + codeowners(project.path(), &["for-file", file])? + .success() + .stdout(predicate::eq(FOR_FILE_FROM_ANNOTATION)); + codeowners(project.path(), &["for-file", "--from-codeowners", file])? + .success() + .stdout(predicate::eq(FOR_FILE_FROM_CODEOWNERS)); + Ok(()) +} + +#[test] +fn test_for_file_reads_the_annotation_of_a_file_outside_the_project() -> Result<(), Box> { + // The code_ownership gem asks about files outside the project too, such as one in another gem, by an absolute or a `../` path. + let temp_dir = tempfile::tempdir()?; + let root = temp_dir.path().join("proj"); + project_at(&root, "owned_globs:\n - \"**/*.rb\"\n")?; + let outside = temp_dir.path().join("outside.rb"); + fs::write(&outside, "# @team Valid\n")?; + for file in ["../outside.rb", outside.to_str().unwrap()] { + codeowners(&root, &["for-file", file])? + .success() + .stdout(predicate::eq(FOR_FILE_FROM_ANNOTATION)); + } + Ok(()) +} + +#[cfg(unix)] +#[test] +fn test_teams_for_files_from_codeowners_keys_each_path_as_given() -> Result<(), Box> { + let project = project("**/*.rb")?; + let run_config = common::build_run_config(project.path(), ".github/CODEOWNERS"); + let (link, _link_dir) = common::symlink_in_temp_dir(project.path()); + let file_paths = [ + "app/valid.rb".to_string(), + "./app/valid.rb".to_string(), + run_config.project_root.join("app/valid.rb").to_string_lossy().into_owned(), + link.join("app/valid.rb").to_string_lossy().into_owned(), + ]; + + let teams = codeowners::runner::teams_for_files_from_codeowners(&run_config, &file_paths)?; + + assert_eq!(teams.len(), file_paths.len(), "{teams:?}"); + for file_path in &file_paths { + let team = teams.get(file_path).ok_or(format!("no entry for {file_path}"))?; + assert_eq!(team.as_ref().map(|team| team.name.as_str()), Some("Valid"), "{file_path}"); + } + Ok(()) +}