From 89b8ca108874f93403dce5b2c79fe496d5c3e750 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 2 Nov 2026 17:08:08 -0800 Subject: [PATCH] Normalize the paths validate, gv and for-file are given `validate `, `gv ` and `for-file` matched each path as typed. The only cleanup was stripping the project root from an absolute path, and the CLI canonicalizes the root. So `./app/x.rb`, `app//x.rb`, `app/./x.rb`, `app/../app/x.rb` and an absolute path through a symlink to the project, as macOS's /tmp is, were matched against owned_globs, unowned_globs and CODEOWNERS as different strings. With `**/*.rb`, a file that CODEOWNERS lists was reported as unowned. With `{app,config}/**/*.rb`, a file with no owner was left out and passed, and `for-file` ignored the annotation of a file named through a symlink. A new path_utils::project_relative_path finds the project-relative path that a given path names. It joins the path onto the absolute root and cleans it lexically, as cli.rs does for `--config-path`. If that isn't under the root, it resolves the root, which a library caller's RunConfig may give unresolved, and then the path's ancestors from the shortest until one is in the project, keeping the rest of the path as written. So a path through a symlink to the project names what its plain form does. Nothing inside the project is resolved: a symlink there keeps its own path, and a deleted file, or one in a directory that doesn't exist or can't be read, is still checked rather than skipped. A path that names nothing under the root is matched as before: as given, without the root if it's absolute. validate_files uses it for the glob filters and the optionally_owned_globs check, and the CODEOWNERS query for its lookup, which covers `for-file --from-codeowners` and the code_ownership gem's lookups. That replaces plain_relative and the second CODEOWNERS lookup from #135, so a path with a `..` inside the project, or through a symlink, now gets the same answer there as the plain path rather than being reported as unowned. find_file_owners uses it for `for-file` and the library calls behind it. runner::teams_for_files_from_codeowners is now keyed by each path as given, so two paths that name the same file stay separate entries. An absolute path used to come back under its stripped relative form, although the gem stores each result under the path it passed. Results and messages still show each path as given. The README's CLI reference now says that `validate` and `gv` take files, and which path forms work. Fixes #145. --- README.md | 6 +- src/ownership/codeowners_file_parser.rs | 32 ++- src/ownership/codeowners_query.rs | 11 +- src/ownership/file_owner_resolver.rs | 16 +- src/path_utils.rs | 209 +++++++++++++++++--- src/runner.rs | 44 ++--- src/runner/api.rs | 2 +- tests/common/mod.rs | 23 ++- tests/optionally_owned_globs_test.rs | 50 +++-- tests/path_normalization_test.rs | 249 ++++++++++++++++++++++++ 10 files changed, 542 insertions(+), 100 deletions(-) create mode 100644 tests/path_normalization_test.rs 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(()) +}