Escape globs built from paths and fix them at the project root

src/common_test.rs

@@ -185,6 +185,53 @@ pub mod tests {
         )
     }
 
+    pub fn build_ownership_with_directory_codeowners_with_glob_metacharacters() -> Result<Ownership, Box<dyn Error>> {
+        ownership!(
+            TestProjectFile {
+                relative_path: "app/{old}/.codeowner".to_owned(),
+                content: "Bar\n".to_owned(),
+            },
+            TestProjectFile {
+                relative_path: "!legacy/.codeowner".to_owned(),
+                content: "Foo\n".to_owned(),
+            }
+        )
+    }
+
+    #[cfg(unix)]
+    pub fn build_ownership_with_directory_codeowner_with_an_asterisk() -> Result<Ownership, Box<dyn Error>> {
+        ownership!(TestProjectFile {
+            relative_path: "app/*/.codeowner".to_owned(),
+            content: "Baz\n".to_owned(),
+        })
+    }
+
+    pub fn build_ownership_with_directory_codeowners_at_the_project_root() -> Result<Ownership, Box<dyn Error>> {
+        ownership!(
+            TestProjectFile {
+                relative_path: ".codeowner".to_owned(),
+                content: "Bam\n".to_owned(),
+            },
+            TestProjectFile {
+                relative_path: "app/.codeowner".to_owned(),
+                content: "Bar\n".to_owned(),
+            }
+        )
+    }
+
+    pub fn build_ownership_with_directory_codeowners_naming_a_github_team_and_an_unknown_team() -> Result<Ownership, Box<dyn Error>> {
+        ownership!(
+            TestProjectFile {
+                relative_path: "app/by_handle/.codeowner".to_owned(),
+                content: "@Bar\n".to_owned(),
+            },
+            TestProjectFile {
+                relative_path: "app/typo/.codeowner".to_owned(),
+                content: "Typo\n".to_owned(),
+            }
+        )
+    }
+
     pub fn build_ownership_with_all_mappers() -> Result<Ownership, Box<dyn Error>> {
         ownership!(
             TestProjectFile {
@@ -249,6 +296,10 @@ pub mod tests {
                     relative_path: "packs/[admin]/comp.ts".to_owned(),
                     content: "// @team Bar\n".to_owned(),
                 },
+                TestProjectFile {
+                    relative_path: "packs/{admin}/comp.ts".to_owned(),
+                    content: "// @team Foo\n".to_owned(),
+                },
                 TestProjectFile {
                     relative_path: "packs/bar/comp.rb".to_owned(),
                     content: "# @team Bar\n".to_owned(),
@@ -307,6 +358,32 @@ pub mod tests {
         let test_config = TestConfig::new(temp_dir.path().to_path_buf(), vec![]);
         build_ownership(test_config)
     }
+
+    pub fn build_ownership_with_team_yml_and_gem_codeowners_with_glob_metacharacters() -> Result<Ownership, Box<dyn Error>> {
+        let temp_dir = tempdir()?;
+
+        let test_config = TestConfig {
+            team_names: vec![],
+            ..TestConfig::new(
+                temp_dir.path().to_path_buf(),
+                vec![
+                    TestProjectFile {
+                        relative_path: "config/teams/[ops].yml".to_owned(),
+                        content: "name: Ops\ngithub:\n  team: \"@Ops\"\nruby:\n  owned_gems:\n    - \"{taco}\"\n".to_owned(),
+                    },
+                    TestProjectFile {
+                        relative_path: "config/teams/{dev}.yml".to_owned(),
+                        content: "name: Dev\ngithub:\n  team: \"@Dev\"\n".to_owned(),
+                    },
+                    TestProjectFile {
+                        relative_path: "gems/{taco}/sauce.rb".to_owned(),
+                        content: "class Taco::Sauce\nend\n".to_owned(),
+                    },
+                ],
+            )
+        };
+        build_ownership(test_config)
+    }
     pub fn build_ownership_with_package_codeowners() -> Result<Ownership, Box<dyn Error>> {
         ownership!(
             TestProjectFile {
@@ -328,6 +405,41 @@ pub mod tests {
         )
     }
 
+    pub fn build_ownership_with_package_codeowners_with_glob_metacharacters() -> Result<Ownership, Box<dyn Error>> {
+        ownership!(
+            TestProjectFile {
+                relative_path: "packs/[ab]/package.yml".to_owned(),
+                content: "owner: Foo\n".to_owned(),
+            },
+            TestProjectFile {
+                relative_path: "packs/{legacy}/package.yml".to_owned(),
+                content: "owner: Bar\n".to_owned(),
+            }
+        )
+    }
+
+    pub fn build_ownership_with_package_codeowners_at_the_project_root() -> Result<Ownership, Box<dyn Error>> {
+        let temp_dir = tempdir()?;
+
+        let test_config = TestConfig {
+            code_ownership_config_yml: format!("{DEFAULT_CODE_OWNERSHIP_YML}ruby_package_paths:\n  - \"**/*\"\n"),
+            ..TestConfig::new(
+                temp_dir.path().to_path_buf(),
+                vec![
+                    TestProjectFile {
+                        relative_path: "package.yml".to_owned(),
+                        content: "owner: Foo\n".to_owned(),
+                    },
+                    TestProjectFile {
+                        relative_path: "packs/inner/package.yml".to_owned(),
+                        content: "owner: Bar\n".to_owned(),
+                    },
+                ],
+            )
+        };
+        build_ownership(test_config)
+    }
+
     pub fn vecs_match<T: PartialEq + std::fmt::Debug>(a: &Vec<T>, b: &Vec<T>) {
         // First check lengths match
         assert_eq!(a.len(), b.len(), "Vectors have different lengths");

src/ownership.rs

@@ -179,37 +179,18 @@ impl Ownership {
 
     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 package mappers drop
-        // such a package, and the directory mapper's glob misses a `.codeowner` at the project root and reads braces as alternatives,
-        // matching `app/legacy` for one in `app/{legacy}`. So the directory is matched as a path prefix, which replaces that glob
-        // for such a `.codeowner`: brackets and braces are literal and the root covers every file.
+        // 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());
-        let directory_roots = self
-            .project
-            .directory_codeowner_files
-            .iter()
-            .filter(|directory_codeowner_file| !self.project.teams_by_name.contains_key(&directory_codeowner_file.owner))
-            .filter_map(|directory_codeowner_file| directory_codeowner_file.directory_root());
         OwnerLookup {
-            owner_matchers: self
-                .mappers()
-                .iter()
-                .flat_map(|mapper| mapper.owner_matchers())
-                .filter(|owner_matcher| match owner_matcher {
-                    OwnerMatcher::Glob {
-                        source: Source::Directory(_),
-                        team_name,
-                        ..
-                    } => self.project.teams_by_name.contains_key(team_name),
-                    _ => true,
-                })
-                .collect(),
-            unknown_team_roots: package_roots.chain(directory_roots).map(Path::to_path_buf).collect(),
+            owner_matchers: self.mappers().iter().flat_map(|mapper| mapper.owner_matchers()).collect(),
+            unknown_team_roots: package_roots.map(Path::to_path_buf).collect(),
         }
     }
 

src/ownership/file_owner_finder.rs

@@ -97,4 +97,18 @@ mod tests {
             Some((&team_name_longest, &source_longest))
         );
     }
+
+    #[test]
+    fn test_directory_overrider_ranks_the_project_root_below_a_top_level_directory() {
+        // The `.codeowner` files come from a parallel walk, so either can be processed first.
+        let (root_team, root) = ("root_team".to_string(), Source::Directory("".to_string()));
+        let (app_team, app) = ("app_team".to_string(), Source::Directory("app".to_string()));
+        for order in [[(&root_team, &root), (&app_team, &app)], [(&app_team, &app), (&root_team, &root)]] {
+            let mut directory_overrider = DirectoryOverrider::default();
+            for (team_name, source) in order {
+                directory_overrider.process(team_name, source);
+            }
+            assert_eq!(directory_overrider.specific_directory_owner(), Some((&app_team, &app)));
+        }
+    }
 }

src/ownership/file_owner_resolver.rs

@@ -10,7 +10,10 @@ use glob::glob;
 
 use crate::{config::Config, project::Team, project_file_builder::build_project_file_without_cache, yaml::NodeLimit};
 
-use super::{FileOwner, mapper::Source};
+use super::{
+    FileOwner,
+    mapper::{Source, directory_glob},
+};
 
 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() {
@@ -278,7 +281,7 @@ fn nearest_package_owner(
                         Ok(owner) => {
                             if let Some(team) = owner.and_then(|owner| teams_by_name.get(&owner)) {
                                 let package_path = parent_rel.join("package.yml");
-                                let package_glob = format!("{rel_str}/**/**");
+                                let package_glob = directory_glob(rel_str);
                                 return Some((
                                     team.name.clone(),
                                     Source::Package(package_path.to_string_lossy().to_string(), package_glob),
@@ -300,7 +303,7 @@ fn nearest_package_owner(
                     && let Some(team) = teams_by_name.get(&owner)
                 {
                     let package_path = parent_rel.join("package.json");
-                    let package_glob = format!("{rel_str}/**/**");
+                    let package_glob = directory_glob(rel_str);
                     return Some((
                         team.name.clone(),
                         Source::Package(package_path.to_string_lossy().to_string(), package_glob),
@@ -584,6 +587,36 @@ mod tests {
         }
     }
 
+    #[test]
+    fn test_nearest_package_owner_describes_a_package_by_the_glob_validate_matches() {
+        let td = tempdir().unwrap();
+        let project_root = td.path();
+        let config = build_config_for_temp("**/*", "packs/**/*", "vendored");
+
+        let ruby_pkg = project_root.join("packs/{payroll}");
+        std::fs::create_dir_all(&ruby_pkg).unwrap();
+        std::fs::write(ruby_pkg.join("package.yml"), "---\nowner: Payroll\n").unwrap();
+        let js_pkg = project_root.join("frontend/[flow]");
+        std::fs::create_dir_all(&js_pkg).unwrap();
+        std::fs::write(js_pkg.join("package.json"), r#"{"metadata": {"owner": "UX"}}"#).unwrap();
+        std::fs::write(project_root.join("package.json"), r#"{"metadata": {"owner": "UX"}}"#).unwrap();
+
+        let mut tbn: HashMap<String, Team> = HashMap::new();
+        for name in ["Payroll", "UX"] {
+            let t = team_named(name);
+            tbn.insert(t.name.clone(), t);
+        }
+
+        for (file, package_path, glob) in [
+            ("packs/{payroll}/a.rb", "packs/{payroll}/package.yml", "packs/\\{payroll\\}/**/**"),
+            ("frontend/[flow]/a.ts", "frontend/[flow]/package.json", "frontend/\\[flow\\]/**/**"),
+            ("app/a.ts", "package.json", "**/**"),
+        ] {
+            let (_, source) = nearest_package_owner(project_root, Path::new(file), &config, &tbn).unwrap();
+            assert_eq!(source, Source::Package(package_path.to_string(), glob.to_string()));
+        }
+    }
+
     #[test]
     fn test_vendored_gem_owner() {
         let config = build_config_for_temp("frontend/**/*", "packs/**/*", "vendored");

src/ownership/mapper.rs

@@ -15,6 +15,7 @@ mod team_yml_mapper;
 
 pub use annotated_file_mapper::TeamFileMapper;
 pub use directory_mapper::DirectoryMapper;
+pub use escaper::directory_glob;
 pub use package_mapper::JavascriptPackageMapper;
 pub use package_mapper::RubyPackageMapper;
 pub use team_gem_mapper::TeamGemMapper;
@@ -26,6 +27,8 @@ use super::Entry;
 pub trait Mapper {
     fn name(&self) -> String;
     fn entries(&self) -> Vec<Entry>;
+    /// A matcher names a team that exists by its `name`, even where a file names it by GitHub team, since `validate` counts
+    /// each name matching a file as a separate owner.
     fn owner_matchers(&self) -> Vec<OwnerMatcher>;
 }
 pub type TeamName = String;
@@ -43,7 +46,7 @@ pub enum Source {
 impl Display for Source {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
-            Source::Directory(path) => write!(f, "Owner specified in `{}/.codeowner`", path),
+            Source::Directory(path) => write!(f, "Owner specified in `{}`", Path::new(path).join(".codeowner").display()),
             Source::AnnotatedFile => write!(f, "Owner annotation at the top of the file"),
             Source::TeamGem => write!(f, "Owner specified in Team YML's `owned_gems`"),
             Source::TeamGlob(glob) => write!(f, "Owner specified in Team YML as an owned_glob `{}`", glob),
@@ -58,7 +61,8 @@ impl Display for Source {
 impl Source {
     pub fn len(&self) -> usize {
         match self {
-            Source::Directory(path) => path.matches('/').count(),
+            // Counting components rather than separators ranks the project root, "", below a top-level directory.
+            Source::Directory(path) => Path::new(path).components().count(),
             _ => 0,
         }
     }
@@ -200,6 +204,7 @@ mod tests {
             Source::Directory("packs/bam".to_string()).to_string(),
             "Owner specified in `packs/bam/.codeowner`"
         );
+        assert_eq!(Source::Directory("".to_string()).to_string(), "Owner specified in `.codeowner`");
         assert_eq!(Source::AnnotatedFile.to_string(), "Owner annotation at the top of the file");
         assert_eq!(Source::TeamGem.to_string(), "Owner specified in Team YML's `owned_gems`");
         assert_eq!(

src/ownership/mapper/annotated_file_mapper.rs

@@ -3,7 +3,7 @@ use std::path::PathBuf;
 use std::sync::Arc;
 
 use super::Entry;
-use super::escaper::escape_brackets;
+use super::escaper::escape_glob;
 use super::{Mapper, OwnerMatcher};
 use crate::ownership::mapper::Source;
 use crate::project::Project;
@@ -31,7 +31,7 @@ impl Mapper for TeamFileMapper {
                     let relative_path = self.project.relative_path(&owned_file.path);
 
                     entries.push(Entry {
-                        path: escape_brackets(&relative_path.to_string_lossy()),
+                        path: escape_glob(&relative_path.to_string_lossy()),
                         github_team: team.github_team.to_owned(),
                         team_name: team.name.to_owned(),
                         disabled: team.avoid_ownership,
@@ -87,6 +87,12 @@ mod tests {
                     team_name: "Bar".to_owned(),
                     disabled: false,
                 },
+                Entry {
+                    path: "packs/\\{admin\\}/comp.ts".to_owned(),
+                    github_team: "@Foo".to_owned(),
+                    team_name: "Foo".to_owned(),
+                    disabled: false,
+                },
                 Entry {
                     path: "packs/jscomponents/comp.ts".to_owned(),
                     github_team: "@Foo".to_owned(),
@@ -142,6 +148,7 @@ mod tests {
         let expected_owner_matchers = vec![OwnerMatcher::ExactMatches(
             HashMap::from([
                 (PathBuf::from("packs/[admin]/comp.ts"), "Bar".to_owned()),
+                (PathBuf::from("packs/{admin}/comp.ts"), "Foo".to_owned()),
                 (PathBuf::from("packs/bar/comp.rb"), "Bar".to_owned()),
                 (PathBuf::from("packs/jscomponents/comp.ts"), "Foo".to_owned()),
                 (PathBuf::from("packs/jscomponents/comp-colon.ts"), "Foo".to_owned()),