Normalize the paths validate, gv and for-file are given

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 <path>` (`f`): Print the owner of a file.
   - Flags: `--from-codeowners` to resolve using only the CODEOWNERS rules
 - `for-team <name>` (`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

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<HashMap<String, Option<Team>>, Box<dyn Error>> {
-        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<Vec<Option<Team>>, Box<dyn Error>> {
+        let prefixed_file_paths: Vec<String> = 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::<Result<Vec<_>, 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<String, Option<Team>> = 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<String> {
@@ -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()));

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())
 }

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<Vec<FileOwner>, 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),

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<PathBuf> {
-    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<PathBuf> {
+    // `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<PathBuf> {
+    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<String> {
-        plain_relative(Path::new(path)).map(|plain| plain.to_string_lossy().into_owned())
+    fn relative(project_root: &Path, path: impl AsRef<Path>) -> Option<String> {
+        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());
+        }
     }
 }