Improving test tooling before tackling overrides

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "codeowners"
-version = "0.2.7"
+version = "0.2.8"
 edition = "2024"
 
 [profile.release]

rust-toolchain.toml

@@ -1,4 +1,4 @@
 [toolchain]
-channel = "1.86.0"
+channel = "1.89.0"
 components = ["clippy", "rustfmt"]
 targets = ["x86_64-apple-darwin", "aarch64-apple-darwin", "x86_64-unknown-linux-gnu"]

src/cache/file.rs

@@ -21,26 +21,24 @@ const DEFAULT_CACHE_CAPACITY: usize = 10000;
 
 impl Caching for GlobalCache {
     fn get_file_owner(&self, path: &Path) -> Result<Option<FileOwnerCacheEntry>, Error> {
-        if let Some(cache_mutex) = self.file_owner_cache.as_ref() {
-            if let Ok(cache) = cache_mutex.lock() {
-                if let Some(cached_entry) = cache.get(path) {
-                    let timestamp = get_file_timestamp(path)?;
-                    if cached_entry.timestamp == timestamp {
-                        return Ok(Some(cached_entry.clone()));
-                    }
-                }
+        if let Some(cache_mutex) = self.file_owner_cache.as_ref()
+            && let Ok(cache) = cache_mutex.lock()
+            && let Some(cached_entry) = cache.get(path)
+        {
+            let timestamp = get_file_timestamp(path)?;
+            if cached_entry.timestamp == timestamp {
+                return Ok(Some(cached_entry.clone()));
             }
         }
         Ok(None)
     }
 
     fn write_file_owner(&self, path: &Path, owner: Option<String>) {
-        if let Some(cache_mutex) = self.file_owner_cache.as_ref() {
-            if let Ok(mut cache) = cache_mutex.lock() {
-                if let Ok(timestamp) = get_file_timestamp(path) {
-                    cache.insert(path.to_path_buf(), FileOwnerCacheEntry { timestamp, owner });
-                }
-            }
+        if let Some(cache_mutex) = self.file_owner_cache.as_ref()
+            && let Ok(mut cache) = cache_mutex.lock()
+            && let Ok(timestamp) = get_file_timestamp(path)
+        {
+            cache.insert(path.to_path_buf(), FileOwnerCacheEntry { timestamp, owner });
         }
     }
 

src/cli.rs

@@ -16,7 +16,7 @@ enum Command {
             default_value = "false",
             help = "Find the owner from the CODEOWNERS file and just return the team name and yml path"
         )]
-        fast: bool,
+        from_codeowners: bool,
         name: String,
     },
 
@@ -46,6 +46,9 @@ enum Command {
 
     #[clap(about = "Delete the cache file.", visible_alias = "d")]
     DeleteCache,
+
+    #[clap(about = "Compare the CODEOWNERS file to the for-file command.", hide = true)]
+    CrosscheckOwners,
 }
 
 /// A CLI to validate and generate Github's CODEOWNERS file.
@@ -110,9 +113,10 @@ pub fn cli() -> Result<RunResult, RunnerError> {
         Command::Validate => runner::validate(&run_config, vec![]),
         Command::Generate { skip_stage } => runner::generate(&run_config, !skip_stage),
         Command::GenerateAndValidate { skip_stage } => runner::generate_and_validate(&run_config, vec![], !skip_stage),
-        Command::ForFile { name, fast: _ } => runner::for_file(&run_config, &name),
+        Command::ForFile { name, from_codeowners } => runner::for_file(&run_config, &name, from_codeowners),
         Command::ForTeam { name } => runner::for_team(&run_config, &name),
         Command::DeleteCache => runner::delete_cache(&run_config),
+        Command::CrosscheckOwners => runner::crosscheck_owners(&run_config),
     };
 
     Ok(runner_result)

src/crosscheck.rs

@@ -0,0 +1,90 @@
+use std::path::Path;
+
+use crate::{
+    cache::Cache,
+    config::Config,
+    ownership::for_file_fast::find_file_owners,
+    project::Project,
+    project_builder::ProjectBuilder,
+    runner::{RunConfig, RunResult, config_from_path, team_for_file_from_codeowners},
+};
+
+pub fn crosscheck_owners(run_config: &RunConfig, cache: &Cache) -> RunResult {
+    match do_crosscheck_owners(run_config, cache) {
+        Ok(mismatches) if mismatches.is_empty() => RunResult {
+            info_messages: vec!["Success! All files match between CODEOWNERS and for-file command.".to_string()],
+            ..Default::default()
+        },
+        Ok(mismatches) => RunResult {
+            validation_errors: mismatches,
+            ..Default::default()
+        },
+        Err(err) => RunResult {
+            io_errors: vec![err],
+            ..Default::default()
+        },
+    }
+}
+
+fn do_crosscheck_owners(run_config: &RunConfig, cache: &Cache) -> Result<Vec<String>, String> {
+    let config = load_config(run_config)?;
+    let project = build_project(&config, run_config, cache)?;
+
+    let mut mismatches: Vec<String> = Vec::new();
+    for file in &project.files {
+        let (codeowners_team, fast_display) = owners_for_file(&file.path, run_config, &config)?;
+        let codeowners_display = codeowners_team.clone().unwrap_or_else(|| "Unowned".to_string());
+        if !is_match(codeowners_team.as_deref(), &fast_display) {
+            mismatches.push(format_mismatch(&project, &file.path, &codeowners_display, &fast_display));
+        }
+    }
+
+    Ok(mismatches)
+}
+
+fn load_config(run_config: &RunConfig) -> Result<Config, String> {
+    config_from_path(&run_config.config_path).map_err(|e| e.to_string())
+}
+
+fn build_project(config: &Config, run_config: &RunConfig, cache: &Cache) -> Result<Project, String> {
+    let mut project_builder = ProjectBuilder::new(
+        config,
+        run_config.project_root.clone(),
+        run_config.codeowners_file_path.clone(),
+        cache,
+    );
+    project_builder.build().map_err(|e| e.to_string())
+}
+
+fn owners_for_file(path: &Path, run_config: &RunConfig, config: &Config) -> Result<(Option<String>, String), String> {
+    let file_path_str = path.to_string_lossy().to_string();
+
+    let codeowners_team = team_for_file_from_codeowners(run_config, &file_path_str)
+        .map_err(|e| e.to_string())?
+        .map(|t| t.name);
+
+    let fast_owners = find_file_owners(&run_config.project_root, config, Path::new(&file_path_str))?;
+    let fast_display = match fast_owners.len() {
+        0 => "Unowned".to_string(),
+        1 => fast_owners[0].team.name.clone(),
+        _ => {
+            let names: Vec<String> = fast_owners.into_iter().map(|fo| fo.team.name).collect();
+            format!("Multiple: {}", names.join(", "))
+        }
+    };
+
+    Ok((codeowners_team, fast_display))
+}
+
+fn is_match(codeowners_team: Option<&str>, fast_display: &str) -> bool {
+    match (codeowners_team, fast_display) {
+        (None, "Unowned") => true,
+        (Some(t), fd) if fd == t => true,
+        _ => false,
+    }
+}
+
+fn format_mismatch(project: &Project, file_path: &Path, codeowners_display: &str, fast_display: &str) -> String {
+    let rel = project.relative_path(file_path).to_string_lossy().to_string();
+    format!("- {}: CODEOWNERS={} fast={}", rel, codeowners_display, fast_display)
+}

src/lib.rs

@@ -1,6 +1,7 @@
 pub mod cache;
 pub(crate) mod common_test;
 pub mod config;
+pub mod crosscheck;
 pub mod ownership;
 pub(crate) mod project;
 pub mod project_builder;

src/ownership/file_generator.rs

@@ -49,15 +49,15 @@ impl FileGenerator {
 }
 
 pub fn compare_lines(a: &String, b: &String) -> Ordering {
-    if let Some((prefix, _)) = a.split_once("**") {
-        if b.starts_with(prefix) {
-            return Ordering::Less;
-        }
+    if let Some((prefix, _)) = a.split_once("**")
+        && b.starts_with(prefix)
+    {
+        return Ordering::Less;
     }
-    if let Some((prefix, _)) = b.split_once("**") {
-        if a.starts_with(prefix) {
-            return Ordering::Greater;
-        }
+    if let Some((prefix, _)) = b.split_once("**")
+        && a.starts_with(prefix)
+    {
+        return Ordering::Greater;
     }
     a.cmp(b)
 }

src/ownership/for_file_fast.rs

@@ -32,10 +32,11 @@ pub fn find_file_owners(project_root: &Path, config: &Config, file_path: &Path)
         if let Some(rel_str) = relative_file_path.to_str() {
             let is_config_owned = glob_list_matches(rel_str, &config.owned_globs);
             let is_config_unowned = glob_list_matches(rel_str, &config.unowned_globs);
-            if is_config_owned && !is_config_unowned {
-                if let Some(team) = teams_by_name.get(&team_name) {
-                    sources_by_team.entry(team.name.clone()).or_default().push(Source::TeamFile);
-                }
+            if is_config_owned
+                && !is_config_unowned
+                && let Some(team) = teams_by_name.get(&team_name)
+            {
+                sources_by_team.entry(team.name.clone()).or_default().push(Source::TeamFile);
             }
         }
     }
@@ -194,32 +195,30 @@ fn nearest_package_owner(
         if let Some(rel_str) = parent_rel.to_str() {
             if glob_list_matches(rel_str, &config.ruby_package_paths) {
                 let pkg_yml = current.join("package.yml");
-                if pkg_yml.exists() {
-                    if let Ok(owner) = read_ruby_package_owner(&pkg_yml) {
-                        if let Some(team) = teams_by_name.get(&owner) {
-                            let package_path = parent_rel.join("package.yml");
-                            let package_glob = format!("{rel_str}/**/**");
-                            return Some((
-                                team.name.clone(),
-                                Source::Package(package_path.to_string_lossy().to_string(), package_glob),
-                            ));
-                        }
-                    }
+                if pkg_yml.exists()
+                    && let Ok(owner) = read_ruby_package_owner(&pkg_yml)
+                    && let Some(team) = teams_by_name.get(&owner)
+                {
+                    let package_path = parent_rel.join("package.yml");
+                    let package_glob = format!("{rel_str}/**/**");
+                    return Some((
+                        team.name.clone(),
+                        Source::Package(package_path.to_string_lossy().to_string(), package_glob),
+                    ));
                 }
             }
             if glob_list_matches(rel_str, &config.javascript_package_paths) {
                 let pkg_json = current.join("package.json");
-                if pkg_json.exists() {
-                    if let Ok(owner) = read_js_package_owner(&pkg_json) {
-                        if let Some(team) = teams_by_name.get(&owner) {
-                            let package_path = parent_rel.join("package.json");
-                            let package_glob = format!("{rel_str}/**/**");
-                            return Some((
-                                team.name.clone(),
-                                Source::Package(package_path.to_string_lossy().to_string(), package_glob),
-                            ));
-                        }
-                    }
+                if pkg_json.exists()
+                    && let Ok(owner) = read_js_package_owner(&pkg_json)
+                    && let Some(team) = teams_by_name.get(&owner)
+                {
+                    let package_path = parent_rel.join("package.json");
+                    let package_glob = format!("{rel_str}/**/**");
+                    return Some((
+                        team.name.clone(),
+                        Source::Package(package_path.to_string_lossy().to_string(), package_glob),
+                    ));
                 }
             }
         }

src/ownership/mapper/package_mapper.rs

@@ -122,10 +122,10 @@ fn remove_nested_packages<'a>(packages: &'a [&'a Package]) -> Vec<&'a Package> {
 
     for package in packages.iter().sorted_by_key(|package| package.package_root()) {
         if let Some(last_package) = top_level_packages.last() {
-            if let (Some(current_root), Some(last_root)) = (package.package_root(), last_package.package_root()) {
-                if !current_root.starts_with(last_root) {
-                    top_level_packages.push(package);
-                }
+            if let (Some(current_root), Some(last_root)) = (package.package_root(), last_package.package_root())
+                && !current_root.starts_with(last_root)
+            {
+                top_level_packages.push(package);
             }
         } else {
             top_level_packages.push(package);

src/ownership/validator.rs

@@ -74,13 +74,13 @@ impl Validator {
             .files
             .par_iter()
             .flat_map(|file| {
-                if let Some(owner) = &file.owner {
-                    if !team_names.contains(owner) {
-                        return Some(Error::InvalidTeam {
-                            name: owner.clone(),
-                            path: project.relative_path(&file.path).to_owned(),
-                        });
-                    }
+                if let Some(owner) = &file.owner
+                    && !team_names.contains(owner)
+                {
+                    return Some(Error::InvalidTeam {
+                        name: owner.clone(),
+                        path: project.relative_path(&file.path).to_owned(),
+                    });
                 }
 
                 None

src/project_builder.rs

@@ -61,14 +61,13 @@ impl<'a> ProjectBuilder<'a> {
         builder.filter_entry(move |entry: &DirEntry| {
             let path = entry.path();
             let file_name = entry.file_name().to_str().unwrap_or("");
-            if let Some(ft) = entry.file_type() {
-                if ft.is_dir() {
-                    if let Ok(rel) = path.strip_prefix(&base_path) {
-                        if rel.components().count() == 1 && ignore_dirs.iter().any(|d| *d == file_name) {
-                            return false;
-                        }
-                    }
-                }
+            if let Some(ft) = entry.file_type()
+                && ft.is_dir()
+                && let Ok(rel) = path.strip_prefix(&base_path)
+                && rel.components().count() == 1
+                && ignore_dirs.iter().any(|d| *d == file_name)
+            {
+                return false;
             }
 
             true
@@ -92,10 +91,10 @@ impl<'a> ProjectBuilder<'a> {
                             let _ = tx.send(entry_type);
                         }
                         Err(report) => {
-                            if let Ok(mut slot) = error_holder.lock() {
-                                if slot.is_none() {
-                                    *slot = Some(report);
-                                }
+                            if let Ok(mut slot) = error_holder.lock()
+                                && slot.is_none()
+                            {
+                                *slot = Some(report);
                             }
                         }
                     }