Unowned team globs (#58)

* DRY iter_team_globs

* the behavior we want

* introducing concept of subtracted globs

* subtracting team globs

* bumping version

Author: perryqh

Cargo.lock

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

Cargo.toml

@@ -279,6 +279,13 @@ pub mod tests {
         })
     }
 
+    pub fn build_ownership_with_subtracted_globs_team_glob_codeowners() -> Result<Ownership, Box<dyn Error>> {
+        ownership!(TestProjectFile {
+            relative_path: "config/teams/baz.yml".to_owned(),
+            content: "name: Baz\ngithub:\n  team: \"@Baz\"\n  members:\n    - Baz member\nowned_globs:\n  - \"packs/bar/**\"\nunowned_globs:\n  - \"packs/bar/excluded/**\"\n".to_owned(),
+        })
+    }
+
     pub fn build_ownership_with_team_yml_codeowners() -> Result<Ownership, Box<dyn Error>> {
         let temp_dir = tempdir()?;
 

src/common_test.rs

@@ -67,19 +67,56 @@ impl Source {
 #[derive(Debug, PartialEq)]
 pub enum OwnerMatcher {
     ExactMatches(HashMap<PathBuf, TeamName>, Source),
-    Glob { glob: String, team_name: TeamName, source: Source },
+    Glob {
+        glob: String,
+        subtracted_globs: Vec<String>,
+        team_name: TeamName,
+        source: Source,
+    },
 }
 
 impl OwnerMatcher {
+    pub fn new_glob_with_candidate_subtracted_globs(
+        glob: String,
+        candidate_subtracted_globs: &[String],
+        team_name: TeamName,
+        source: Source,
+    ) -> Self {
+        let subtracted_globs = candidate_subtracted_globs
+            .iter()
+            .filter(|candidate_subtracted_glob| {
+                glob_match(candidate_subtracted_glob, &glob) || glob_match(&glob, candidate_subtracted_glob)
+            })
+            .cloned()
+            .collect();
+        OwnerMatcher::Glob {
+            glob,
+            subtracted_globs,
+            team_name,
+            source,
+        }
+    }
+
+    pub fn new_glob(glob: String, team_name: TeamName, source: Source) -> Self {
+        OwnerMatcher::Glob {
+            glob,
+            subtracted_globs: vec![],
+            team_name,
+            source,
+        }
+    }
+
     pub fn owner_for(&self, relative_path: &Path) -> (Option<&TeamName>, &Source) {
         match self {
-            OwnerMatcher::Glob { glob, team_name, source } => {
-                if glob_match(glob, relative_path.to_str().unwrap()) {
-                    (Some(team_name), source)
-                } else {
-                    (None, source)
-                }
-            }
+            OwnerMatcher::Glob {
+                glob,
+                subtracted_globs,
+                team_name,
+                source,
+            } => relative_path
+                .to_str()
+                .filter(|path| glob_match(glob, path) && !subtracted_globs.iter().any(|subtracted| glob_match(subtracted, path)))
+                .map_or((None, source), |_| (Some(team_name), source)),
             OwnerMatcher::ExactMatches(path_to_team, source) => (path_to_team.get(relative_path), source),
         }
     }
@@ -89,14 +126,15 @@ impl OwnerMatcher {
 mod tests {
     use super::*;
 
-    fn assert_owner_for(glob: &str, relative_path: &str, expect_match: bool) {
+    fn assert_owner_for(glob: &str, subtracted_globs: &[&str], relative_path: &str, expect_match: bool) {
         let source = Source::Directory("packs/bam".to_string());
         let team_name = "team1".to_string();
-        let owner_matcher = OwnerMatcher::Glob {
-            glob: glob.to_string(),
-            team_name: team_name.clone(),
-            source: source.clone(),
-        };
+        let owner_matcher = OwnerMatcher::new_glob_with_candidate_subtracted_globs(
+            glob.to_string(),
+            &subtracted_globs.iter().map(|s| s.to_string()).collect::<Vec<String>>(),
+            team_name.clone(),
+            source.clone(),
+        );
         let response = owner_matcher.owner_for(Path::new(relative_path));
         if expect_match {
             assert_eq!(response, (Some(&team_name), &source));
@@ -107,26 +145,50 @@ mod tests {
 
     #[test]
     fn owner_for_without_brackets_in_glob() {
-        assert_owner_for("packs/bam/**/**", "packs/bam/app/components/sidebar.jsx", true);
-        assert_owner_for("packs/bam/**/**", "packs/baz/app/components/sidebar.jsx", false);
-        assert_owner_for("packs/bam/**/**", "packs/bam/app/[components]/gadgets/sidebar.jsx", true);
-        assert_owner_for("packs/bam/**/**", "packs/bam/app/sidebar_[component].jsx", true);
+        assert_owner_for("packs/bam/**/**", &[], "packs/bam/app/components/sidebar.jsx", true);
+        assert_owner_for("packs/bam/**/**", &[], "packs/baz/app/components/sidebar.jsx", false);
+        assert_owner_for("packs/bam/**/**", &[], "packs/bam/app/[components]/gadgets/sidebar.jsx", true);
+        assert_owner_for("packs/bam/**/**", &[], "packs/bam/app/sidebar_[component].jsx", true);
+        assert_owner_for(
+            "packs/bam/**/**",
+            &["packs/bam/app/excluded/**"],
+            "packs/bam/app/excluded/sidebar_[component].jsx",
+            false,
+        );
+    }
+
+    #[test]
+    fn subtracted_globs() {
+        assert_owner_for(
+            "packs/bam/**/**",
+            &["packs/bam/app/excluded/**"],
+            "packs/bam/app/excluded/some_file.rb",
+            false,
+        );
+        assert_owner_for(
+            "packs/bam/**/**",
+            &["packs/bam/app/excluded/**"],
+            "packs/bam/app/not_excluded/some_file.rb",
+            true,
+        );
     }
 
     #[test]
     fn owner_for_with_brackets_in_glob() {
         assert_owner_for(
             "packs/bam/app/\\[components\\]/**/**",
+            &[],
             "packs/bam/app/[components]/gadgets/sidebar.jsx",
             true,
         );
-        assert_owner_for("packs/\\[bam\\]/**/**", "packs/[bam]/app/components/sidebar.jsx", true);
+        assert_owner_for("packs/\\[bam\\]/**/**", &[], "packs/[bam]/app/components/sidebar.jsx", true);
     }
 
     #[test]
     fn owner_for_with_multiple_brackets_in_glob() {
         assert_owner_for(
             "packs/\\[bam\\]/bar/\\[foo\\]/**/**",
+            &[],
             "packs/[bam]/bar/[foo]/app/components/sidebar.jsx",
             true,
         );
@@ -150,4 +212,38 @@ mod tests {
         );
         assert_eq!(Source::TeamYml.to_string(), "Teams own their configuration files");
     }
+
+    #[test]
+    fn test_new_glob_with_candidate_subtracted_globs() {
+        assert_new_glob_with_candidate_subtracted_globs("packs/bam/**/**", &[], &[]);
+        assert_new_glob_with_candidate_subtracted_globs("packs/bam/**/**", &["packs/bam/app/**/**"], &["packs/bam/app/**/**"]);
+        assert_new_glob_with_candidate_subtracted_globs(
+            "packs/bam/**/**",
+            &["packs/bam/app/an/exceptional/path/it.rb"],
+            &["packs/bam/app/an/exceptional/path/it.rb"],
+        );
+        assert_new_glob_with_candidate_subtracted_globs("packs/bam/**/**", &["packs/bam.rb"], &[]);
+        assert_new_glob_with_candidate_subtracted_globs("packs/bam/**/**", &["packs/nope/app/**/**"], &[]);
+        assert_new_glob_with_candidate_subtracted_globs("packs/**", &["packs/yep/app/**/**"], &["packs/yep/app/**/**"]);
+        assert_new_glob_with_candidate_subtracted_globs("packs/foo.yml", &["packs/foo/**/**"], &[]);
+    }
+
+    fn assert_new_glob_with_candidate_subtracted_globs(
+        glob: &str,
+        candidate_subtracted_globs: &[&str],
+        expected_subtracted_globs: &[&str],
+    ) {
+        let owner_matcher = OwnerMatcher::new_glob_with_candidate_subtracted_globs(
+            glob.to_string(),
+            &candidate_subtracted_globs.iter().map(|s| s.to_string()).collect::<Vec<String>>(),
+            "team1".to_string(),
+            Source::TeamGlob(glob.to_string()),
+        );
+
+        if let OwnerMatcher::Glob { subtracted_globs, .. } = owner_matcher {
+            assert_eq!(subtracted_globs, expected_subtracted_globs);
+        } else {
+            panic!("Expected a Glob matcher");
+        }
+    }
 }

src/ownership/mapper.rs

@@ -40,11 +40,11 @@ impl Mapper for DirectoryMapper {
         let mut owner_matchers = Vec::new();
 
         for file in &self.project.directory_codeowner_files {
-            owner_matchers.push(OwnerMatcher::Glob {
-                glob: format!("{}/**/**", escape_brackets(&file.directory_root().to_string_lossy())),
-                team_name: file.owner.to_owned(),
-                source: Source::Directory(file.directory_root().to_string_lossy().to_string()),
-            });
+            owner_matchers.push(OwnerMatcher::new_glob(
+                format!("{}/**/**", escape_brackets(&file.directory_root().to_string_lossy())),
+                file.owner.to_owned(),
+                Source::Directory(file.directory_root().to_string_lossy().to_string()),
+            ));
         }
 
         owner_matchers
@@ -125,21 +125,21 @@ mod tests {
         vecs_match(
             &mapper.owner_matchers(),
             &vec![
-                OwnerMatcher::Glob {
-                    glob: "app/consumers/**/**".to_owned(),
-                    team_name: "Bar".to_owned(),
-                    source: Source::Directory("app/consumers".to_string()),
-                },
-                OwnerMatcher::Glob {
-                    glob: "app/services/**/**".to_owned(),
-                    team_name: "Foo".to_owned(),
-                    source: Source::Directory("app/services".to_owned()),
-                },
-                OwnerMatcher::Glob {
-                    glob: "app/services/exciting/**/**".to_owned(),
-                    team_name: "Bar".to_owned(),
-                    source: Source::Directory("app/services/exciting".to_owned()),
-                },
+                OwnerMatcher::new_glob(
+                    "app/consumers/**/**".to_owned(),
+                    "Bar".to_owned(),
+                    Source::Directory("app/consumers".to_string()),
+                ),
+                OwnerMatcher::new_glob(
+                    "app/services/**/**".to_owned(),
+                    "Foo".to_owned(),
+                    Source::Directory("app/services".to_owned()),
+                ),
+                OwnerMatcher::new_glob(
+                    "app/services/exciting/**/**".to_owned(),
+                    "Bar".to_owned(),
+                    Source::Directory("app/services/exciting".to_owned()),
+                ),
             ],
         );
         Ok(())
@@ -152,16 +152,16 @@ mod tests {
         vecs_match(
             &mapper.owner_matchers(),
             &vec![
-                OwnerMatcher::Glob {
-                    glob: "app/\\[consumers\\]/**/**".to_string(),
-                    team_name: "Bar".to_string(),
-                    source: Source::Directory("app/[consumers]".to_string()),
-                },
-                OwnerMatcher::Glob {
-                    glob: "app/\\[consumers\\]/deep/nesting/\\[nestdir\\]/**/**".to_string(),
-                    team_name: "Foo".to_string(),
-                    source: Source::Directory("app/[consumers]/deep/nesting/[nestdir]".to_string()),
-                },
+                OwnerMatcher::new_glob(
+                    "app/\\[consumers\\]/**/**".to_string(),
+                    "Bar".to_string(),
+                    Source::Directory("app/[consumers]".to_string()),
+                ),
+                OwnerMatcher::new_glob(
+                    "app/\\[consumers\\]/deep/nesting/\\[nestdir\\]/**/**".to_string(),
+                    "Foo".to_string(),
+                    Source::Directory("app/[consumers]/deep/nesting/[nestdir]".to_string()),
+                ),
             ],
         );
         Ok(())

src/ownership/mapper/directory_mapper.rs

@@ -101,11 +101,11 @@ impl PackageMapper {
             let team = team_by_name.get(&package.owner);
 
             if let Some(team) = team {
-                owner_matchers.push(OwnerMatcher::Glob {
-                    glob: format!("{}/**/**", package_root),
-                    team_name: team.name.to_owned(),
-                    source: Source::Package(package.path.to_string_lossy().to_string(), format!("{}/**/**", package_root)),
-                });
+                owner_matchers.push(OwnerMatcher::new_glob(
+                    format!("{}/**/**", package_root),
+                    team.name.to_owned(),
+                    Source::Package(package.path.to_string_lossy().to_string(), format!("{}/**/**", package_root)),
+                ));
             }
         }
 
@@ -198,16 +198,16 @@ mod tests {
         vecs_match(
             &mapper.owner_matchers(&PackageType::Ruby),
             &vec![
-                OwnerMatcher::Glob {
-                    glob: "packs/bam/**/**".to_owned(),
-                    team_name: "Bam".to_owned(),
-                    source: Source::Package("packs/bam/package.yml".to_owned(), "packs/bam/**/**".to_owned()),
-                },
-                OwnerMatcher::Glob {
-                    glob: "packs/foo/**/**".to_owned(),
-                    team_name: "Baz".to_owned(),
-                    source: Source::Package("packs/foo/package.yml".to_owned(), "packs/foo/**/**".to_owned()),
-                },
+                OwnerMatcher::new_glob(
+                    "packs/bam/**/**".to_owned(),
+                    "Bam".to_owned(),
+                    Source::Package("packs/bam/package.yml".to_owned(), "packs/bam/**/**".to_owned()),
+                ),
+                OwnerMatcher::new_glob(
+                    "packs/foo/**/**".to_owned(),
+                    "Baz".to_owned(),
+                    Source::Package("packs/foo/package.yml".to_owned(), "packs/foo/**/**".to_owned()),
+                ),
             ],
         );
         Ok(())

src/ownership/mapper/package_mapper.rs

@@ -46,11 +46,11 @@ impl Mapper for TeamGemMapper {
                 let vendored_gem = vendored_gem_by_name.get(owned_gem);
 
                 if let Some(vendored_gem) = vendored_gem {
-                    owner_matchers.push(OwnerMatcher::Glob {
-                        glob: format!("{}/**/*", self.project.relative_path(&vendored_gem.path).to_string_lossy()),
-                        team_name: team.name.clone(),
-                        source: Source::TeamGem,
-                    });
+                    owner_matchers.push(OwnerMatcher::new_glob(
+                        format!("{}/**/*", self.project.relative_path(&vendored_gem.path).to_string_lossy()),
+                        team.name.clone(),
+                        Source::TeamGem,
+                    ));
                 }
             }
         }
@@ -92,11 +92,11 @@ mod tests {
         let mapper = TeamGemMapper::build(ownership.project.clone());
         vecs_match(
             &mapper.owner_matchers(),
-            &vec![OwnerMatcher::Glob {
-                glob: "gems/globbing/**/*".to_owned(),
-                team_name: "Bam".to_owned(),
-                source: Source::TeamGem,
-            }],
+            &vec![OwnerMatcher::new_glob(
+                "gems/globbing/**/*".to_owned(),
+                "Bam".to_owned(),
+                Source::TeamGem,
+            )],
         );
         Ok(())
     }