fix: run the real ownership checks when validate/gv are given paths

README.md

@@ -170,7 +170,7 @@ 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 [files...]` (`v`): Validate the CODEOWNERS file and configuration. Given files, it checks only their ownership, a fast mode for git hooks.
+- `validate [files...]` (`v`): Validate the CODEOWNERS file and configuration. Given files, a fast mode for git hooks, it runs the same checks on just those files and the packages and `.codeowner` files whose directories contain them, and checks that CODEOWNERS gives each file its owner rather than comparing the whole file.
 - `generate-and-validate [files...]` (`gv`): Run `generate` then `validate`.
   - Flags: `--skip-stage, -s`
 - `for-file <path>` (`f`): Print the owner of a file.
@@ -215,7 +215,7 @@ codeowners gv --no-cache
 - `js_package_paths` / `javascript_package_paths` (default: `['frontend/**/*']`)
 - `team_file_glob` (default: `['config/teams/**/*.yml']`)
 - `unowned_globs` (default: `['frontend/**/node_modules/**/*', 'frontend/**/__generated__/**/*']`): Files matched here are left out of the project: they don't need an owner, validation doesn't check them, and file annotations in them are ignored. A `.codeowner`, `package.yml` or `package.json` among them is still read, though, so its owner still goes into CODEOWNERS and an invalid team in it is still reported. A team file among them is still read too, so it still defines its team and keeps its own line in CODEOWNERS. Use this for third-party or generated code.
-- `optionally_owned_globs` (default: `[]`, added in 0.6.0): Files in `owned_globs` that match these globs don't need an owner. It doesn't add to `owned_globs`, so a file must match both. Unlike `unowned_globs`, file annotations in them still count, and a full `validate` or `gv` still runs every other check, so a file there can't have more than one owner or name an invalid team. Given a list of files, as in a git hook, `validate` and `gv` look each one up in CODEOWNERS and don't check for more than one owner. An optionally owned file with no line passes only if nothing names an owner for it, so one whose annotation, `.codeowner` or package names a team that doesn't exist, or whose owner CODEOWNERS doesn't list yet, is reported under "Files matching optionally_owned_globs name an owner that CODEOWNERS doesn't list". As in a full `validate`, an annotation, `.codeowner` or package file doesn't count if the file is untracked or in a top-level directory named in `ignore_dirs`, so a new file's own annotation counts once the file is staged. Use `['**/*']` if only some of your code has owners, or scope it, e.g. `['**/deprecated/**/*']`. `unowned_globs` takes precedence, so a file that matches both is left out. If you use `unowned_globs: ['**/*']` only so that files can go unowned, replace it with `optionally_owned_globs: ['**/*']` to make annotations work, and run `codeowners gv` to add a CODEOWNERS line for each annotated file. The files that `unowned_globs` left out are then read and checked like any other owned file, which takes longer and may turn up problems that were hidden before. Keep third-party and generated code in `unowned_globs`. Versions before 0.6.0 ignore the key without a warning, so they still report unowned files in these globs.
+- `optionally_owned_globs` (default: `[]`, added in 0.6.0): Files in `owned_globs` that match these globs don't need an owner. It doesn't add to `owned_globs`, so a file must match both. Unlike `unowned_globs`, file annotations in them still count, and `validate` and `gv` still run every other check, with or without a list of files, so a file there can't have more than one owner or name an invalid team, and given a list of files, `validate` reports one whose owner CODEOWNERS doesn't list yet. An annotation, `.codeowner` or package file doesn't count if the file is untracked or in a top-level directory named in `ignore_dirs`, so a new file's own annotation counts once the file is staged. Use `['**/*']` if only some of your code has owners, or scope it, e.g. `['**/deprecated/**/*']`. `unowned_globs` takes precedence, so a file that matches both is left out. If you use `unowned_globs: ['**/*']` only so that files can go unowned, replace it with `optionally_owned_globs: ['**/*']` to make annotations work, and run `codeowners gv` to add a CODEOWNERS line for each annotated file. The files that `unowned_globs` left out are then read and checked like any other owned file, which takes longer and may turn up problems that were hidden before. Keep third-party and generated code in `unowned_globs`. Versions before 0.6.0 ignore the key without a warning, so they still report unowned files in these globs.
 - `vendored_gems_path` (default: `'vendored/'`)
 - `cache_directory` (default: `'tmp/cache/codeowners'`)
 - `ignore_dirs` (default includes: `.git`, `node_modules`, `tmp`, etc.)

perf/README.md

@@ -113,12 +113,12 @@ a hook that runs `gv` sees the smaller of the two wins, and quoting the
 Spans are inclusive of children, so sibling spans can be summed and nested ones
 cannot. `config_load`, `cache_init`, `project_build`, `cache_persist` and
 `per_file_query` are disjoint — percentages across those are sound. But
-`ownership_validate` ⊃ `validator_validate` ⊃ `validate_file_ownership` ⊃
-`file_to_owners`: quoting those together as shares of one total double-counts.
+`ownership_validate` ⊃ `validator_validate` ⊃ `path_to_owners`: quoting those
+together as shares of one total double-counts.
 - The **phase breakdown** comes from `tracing` spans inside the library. Phases
   are **inclusive of nested children**, so they do not sum to the total —
   `project_build` contains its own sub-work, and `ownership_validate` contains
-  `validator_validate`, which contains `file_to_owners`. Use them to attribute a
+  `validator_validate`, which contains `path_to_owners`. Use them to attribute a
   win, not to reconstruct a total.
 - `mapper_build` is **accumulated across all calls in a run**. That is
   deliberate: it is currently invoked more than once per validate, and the sum is

src/cli.rs

@@ -39,15 +39,23 @@ enum Command {
         visible_alias = "v"
     )]
     Validate {
-        #[arg(help = "Optional list of files to validate ownership for (fast mode for git hooks)")]
+        #[arg(
+            help = "Optional list of files to validate ownership for (fast mode for git hooks). Checks just these \
+                   files, the packages and `.codeowner` files whose directories contain them, and that the \
+                   CODEOWNERS file gives each file its owner."
+        )]
         files: Vec<String>,
     },
 
     #[clap(about = "Chains both `generate` and `validate` commands.", visible_alias = "gv")]
     GenerateAndValidate {
         #[arg(long, short, default_value = "false", help = "Skip staging the CODEOWNERS file")]
         skip_stage: bool,
-        #[arg(help = "Optional list of files to validate ownership for (fast mode for git hooks)")]
+        #[arg(
+            help = "Optional list of files to validate ownership for (fast mode for git hooks). Regenerates the \
+                   CODEOWNERS file, then checks just these files and the packages and `.codeowner` files whose \
+                   directories contain them."
+        )]
         files: Vec<String>,
     },
 

src/ownership.rs

@@ -2,6 +2,7 @@ use file_owner_finder::FileOwnerFinder;
 use itertools::Itertools;
 use mapper::{OwnerMatcher, Source, TeamName};
 use std::{
+    collections::HashMap,
     error::Error,
     fmt::{self, Display},
     path::{Path, PathBuf},
@@ -23,6 +24,7 @@ use crate::{
 };
 
 pub use validator::Errors as ValidatorErrors;
+pub(crate) use validator::SuppliedPath;
 
 use self::{
     codeowners_file_parser::parse_for_team,
@@ -109,27 +111,6 @@ impl Entry {
     }
 }
 
-/// Tells whether anything names an owner for a project-relative path, even a team that doesn't exist: an annotation naming one
-/// still counts for its file, and a `.codeowner`, `package.yml` or `package.json` naming one for every path under its directory.
-/// It sees only files the project reads, 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`. Building one goes through every project
-/// file, so reuse it.
-pub(crate) struct OwnerLookup {
-    owner_matchers: Vec<OwnerMatcher>,
-    unknown_team_roots: Vec<PathBuf>,
-}
-
-impl OwnerLookup {
-    pub(crate) fn has_owner(&self, relative_path: &Path) -> bool {
-        // FileOwnerFinder::find returns an owner exactly when some matcher names a team, but builds every owner to do it.
-        self.unknown_team_roots.iter().any(|root| relative_path.starts_with(root))
-            || self
-                .owner_matchers
-                .iter()
-                .any(|owner_matcher| owner_matcher.owner_for(relative_path).0.is_some())
-    }
-}
-
 impl Ownership {
     pub fn build(project: Project) -> Self {
         Self {
@@ -143,13 +124,29 @@ impl Ownership {
         let validator = Validator {
             project: self.project.clone(),
             mappers: self.mappers(),
-            file_generator: FileGenerator {
-                mappers: self.codeowners_file_mappers(),
-            },
             executable_name: self.project.executable_name.clone(),
         };
 
-        validator.validate()
+        validator.validate(&FileGenerator {
+            mappers: self.codeowners_file_mappers(),
+        })
+    }
+
+    #[instrument(name = "ownership_validate_files", level = "debug", skip_all)]
+    pub(crate) fn validate_files(
+        &self,
+        supplied: &[SuppliedPath],
+        selectors: &[PathBuf],
+        listed: Option<&HashMap<String, Option<Team>>>,
+    ) -> Result<(), ValidatorErrors> {
+        info!("validating file ownership for {} supplied paths", supplied.len());
+        let validator = Validator {
+            project: self.project.clone(),
+            mappers: self.mappers(),
+            executable_name: self.project.executable_name.clone(),
+        };
+
+        validator.validate_files(supplied, selectors, listed)
     }
 
     #[instrument(level = "debug", skip_all)]
@@ -177,23 +174,6 @@ impl Ownership {
             .collect())
     }
 
-    pub(crate) fn owner_lookup(&self) -> OwnerLookup {
-        // `validate` reports a package.yml, package.json or `.codeowner` naming a team that doesn't exist, but a list of files
-        // isn't checked for that, so here each still names an owner for the files under its directory. The directory mapper
-        // matches a `.codeowner` whatever team it names, but the package mappers drop such a package, so its directory is matched
-        // as a path prefix.
-        let package_roots = self
-            .project
-            .packages
-            .iter()
-            .filter(|package| !self.project.teams_by_name.contains_key(&package.owner))
-            .filter_map(|package| package.package_root());
-        OwnerLookup {
-            owner_matchers: self.mappers().iter().flat_map(|mapper| mapper.owner_matchers()).collect(),
-            unknown_team_roots: package_roots.map(Path::to_path_buf).collect(),
-        }
-    }
-
     #[instrument(level = "debug", skip_all)]
     pub fn for_team(&self, team_name: &str) -> Result<Vec<TeamOwnership>, Box<dyn Error>> {
         info!("getting team ownership for {}", team_name);