for-file --fast fixes (#57)

perryqh · web-flow · commit 5dae93776e25 · 2025-04-10T12:56:57.000-05:00
* create codeowners parent dir

* for-file absolute path
diff --git a/rust-toolchain.toml b/rust-toolchain.toml
@@ -1,4 +1,4 @@
 [toolchain]
-channel = "1.85.0"
+channel = "1.86.0"
 components = ["clippy", "rustfmt"]
 targets = ["x86_64-apple-darwin", "aarch64-apple-darwin", "x86_64-unknown-linux-gnu"]
diff --git a/src/ownership.rs b/src/ownership.rs
@@ -125,8 +125,8 @@ impl Ownership {
     }
 
     #[instrument(level = "debug", skip_all)]
-    pub fn for_file(&self, file_path: &str) -> Result<Vec<FileOwner>, ValidatorErrors> {
-        info!("getting file ownership for {}", file_path);
+    pub fn for_file(&self, file_path: &Path) -> Result<Vec<FileOwner>, ValidatorErrors> {
+        info!("getting file ownership for {}", file_path.display());
         let owner_matchers: Vec<OwnerMatcher> = self.mappers().iter().flat_map(|mapper| mapper.owner_matchers()).collect();
         let file_owner_finder = FileOwnerFinder {
             owner_matchers: &owner_matchers,
@@ -186,7 +186,7 @@ mod tests {
     #[test]
     fn test_for_file_owner() -> Result<(), Box<dyn Error>> {
         let ownership = build_ownership_with_all_mappers()?;
-        let file_owners = ownership.for_file("app/consumers/directory_owned.rb").unwrap();
+        let file_owners = ownership.for_file(Path::new("app/consumers/directory_owned.rb")).unwrap();
         assert_eq!(file_owners.len(), 1);
         assert_eq!(file_owners[0].team.name, "Bar");
         assert_eq!(file_owners[0].team_config_file_path, "config/teams/bar.yml");
@@ -196,7 +196,7 @@ mod tests {
     #[test]
     fn test_for_file_no_owner() -> Result<(), Box<dyn Error>> {
         let ownership = build_ownership_with_all_mappers()?;
-        let file_owners = ownership.for_file("app/madeup/foo.rb").unwrap();
+        let file_owners = ownership.for_file(Path::new("app/madeup/foo.rb")).unwrap();
         assert_eq!(file_owners.len(), 0);
         Ok(())
     }
diff --git a/src/ownership/file_generator.rs b/src/ownership/file_generator.rs
@@ -43,23 +43,25 @@ impl FileGenerator {
 
     fn to_sorted_lines(entries: &[Entry]) -> Vec<String> {
         let mut lines: Vec<String> = entries.iter().map(|entry| entry.to_row()).collect();
-        lines.sort_by(|a, b| {
-            if let Some((prefix, _)) = a.split_once("**") {
-                if b.starts_with(prefix) {
-                    return Ordering::Less;
-                }
-            }
-            if let Some((prefix, _)) = b.split_once("**") {
-                if a.starts_with(prefix) {
-                    return Ordering::Greater;
-                }
-            }
-            a.cmp(b)
-        });
+        lines.sort_by(compare_lines);
         lines
     }
 }
 
+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, _)) = b.split_once("**") {
+        if a.starts_with(prefix) {
+            return Ordering::Greater;
+        }
+    }
+    a.cmp(b)
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
diff --git a/src/ownership/parser.rs b/src/ownership/parser.rs
@@ -4,6 +4,7 @@ use crate::{
 };
 use fast_glob::glob_match;
 use memoize::memoize;
+use regex::Regex;
 use std::{
     collections::HashMap,
     error::Error,
@@ -12,6 +13,8 @@ use std::{
     path::{Path, PathBuf},
 };
 
+use super::file_generator::compare_lines;
+
 pub struct Parser {
     pub project_root: PathBuf,
     pub codeowners_file_path: PathBuf,
@@ -54,7 +57,6 @@ impl Parser {
 
 #[memoize]
 fn teams_by_github_team_name(team_file_glob: Vec<String>) -> HashMap<String, Team> {
-    dbg!("in teams_by_github_team_name");
     let mut teams = HashMap::new();
     for glob in team_file_glob {
         let paths = glob::glob(&glob).expect("Failed to read glob pattern").filter_map(Result::ok);
@@ -76,8 +78,6 @@ fn teams_by_github_team_name(team_file_glob: Vec<String>) -> HashMap<String, Tea
 
 #[memoize]
 fn build_codeowners_lines_in_priority(codeowners_file_path: String) -> Vec<String> {
-    dbg!("in build_codeowners_lines_in_priority");
-    dbg!(&codeowners_file_path);
     let codeowners_file = match fs::read_to_string(codeowners_file_path) {
         Ok(codeowners_file) => codeowners_file,
         Err(e) => {
@@ -89,13 +89,67 @@ fn build_codeowners_lines_in_priority(codeowners_file_path: String) -> Vec<Strin
     stripped_lines_by_priority(&codeowners_file)
 }
 
+#[derive(Debug, Clone, PartialEq, Eq)]
+struct Section {
+    heading: String,
+    lines: Vec<String>,
+}
+
+impl Section {
+    fn new(heading: String, lines: Vec<String>) -> Self {
+        let mut sorted_lines = lines.clone();
+        sorted_lines.sort_by(compare_lines);
+        Self {
+            heading,
+            lines: sorted_lines,
+        }
+    }
+}
+
+fn codeowner_sections(codeowners_file: &str) -> Result<Vec<Section>, Box<dyn Error>> {
+    let un_ignore = Regex::new(r"^# \/")?;
+    let mut iter = codeowners_file.lines().peekable();
+    let mut sections = Vec::new();
+    let mut current_section = None;
+    let mut current_lines = Vec::new();
+
+    while let Some(line) = iter.next() {
+        let line = un_ignore.replace(line, "/").to_string();
+        if line.is_empty() {
+            continue;
+        }
+
+        if line.starts_with('#') {
+            if iter
+                .peek()
+                .map(|next| next.starts_with('/') || next.starts_with("# /"))
+                .unwrap_or(false)
+            {
+                if let Some(section_name) = current_section.take() {
+                    sections.push(Section::new(section_name, std::mem::take(&mut current_lines)));
+                }
+                current_section = Some(line);
+            }
+        } else {
+            current_lines.push(line);
+        }
+    }
+
+    if let Some(section_name) = current_section {
+        sections.push(Section::new(section_name, current_lines));
+    }
+
+    Ok(sections)
+}
+
 fn stripped_lines_by_priority(codeowners_file: &str) -> Vec<String> {
-    codeowners_file
-        .lines()
-        .filter(|line| !line.trim().is_empty() && !line.trim().starts_with("#"))
-        .map(|line| line.trim().to_string())
-        .rev()
-        .collect()
+    let mut lines = Vec::new();
+    let sections = codeowner_sections(codeowners_file).unwrap_or_default();
+    for section in sections {
+        lines.extend(section.lines);
+    }
+    lines.reverse();
+    lines
 }
 
 pub fn parse_for_team(team_name: String, codeowners_file: &str) -> Result<Vec<TeamOwnership>, Box<dyn Error>> {
@@ -136,6 +190,7 @@ pub fn parse_for_team(team_name: String, codeowners_file: &str) -> Result<Vec<Te
 
 #[cfg(test)]
 mod tests {
+
     use crate::common_test::tests::vecs_match;
 
     use super::*;
@@ -336,4 +391,90 @@ mod tests {
         assert_eq!(stripped_lines, vec!["/another/path/to/owned @Bar", "/path/to/owned @Foo"]);
         Ok(())
     }
+
+    #[test]
+    fn test_stripped_lines_by_priority_with_ignored_teams() -> Result<(), Box<dyn Error>> {
+        let codeownership_file = indoc! {"
+            # STOP! - DO NOT EDIT THIS FILE MANUALLY
+            # This file was automatically generated by \"bin/codeownership validate\".
+            #
+            # CODEOWNERS is used for GitHub to suggest code/file owners to various GitHub
+            # teams. This is useful when developers create Pull Requests since the
+            # code/file owner is notified. Reference GitHub docs for more details:
+            # https://help.github.com/en/articles/about-code-owners
+
+
+            # Annotations at the top of file
+            # /app/assets/config/manifest.js @Prefix/team-foo
+            # /config/application.rb @Prefix/team-bar
+            /config/i18n-tasks.yml.erb @Prefix/language-team
+
+            # Team-specific owned globs
+            # /.github/workflows/pull-translations.yml @Prefix/infra
+            # /.github/workflows/push-sources.yml @Prefix/infra
+            # /Dockerfile @Prefix/docker-team
+            # /components/create.rb @Prefix/component-team
+            /.codeclimate.yml @Prefix/climate-team
+            /.vscode/extensions/z/**/* @Prefix/zteam
+            /bin/brakeman @Prefix/psecurity
+            /config/brakeman.ignore @Prefix/security
+        "};
+
+        // build up each sections lines
+        // resort the lines without the '#'
+        // re-assemble the sections
+        // reverse sort
+        let codeowner_sections = codeowner_sections(codeownership_file)?;
+        assert_eq!(
+            codeowner_sections,
+            vec![
+                Section {
+                    heading: "# Annotations at the top of file".to_string(),
+                    lines: vec![
+                        "/app/assets/config/manifest.js @Prefix/team-foo".to_string(),
+                        "/config/application.rb @Prefix/team-bar".to_string(),
+                        "/config/i18n-tasks.yml.erb @Prefix/language-team".to_string()
+                    ]
+                },
+                Section {
+                    heading: "# Team-specific owned globs".to_string(),
+                    lines: vec![
+                        "/.codeclimate.yml @Prefix/climate-team".to_string(),
+                        "/.github/workflows/pull-translations.yml @Prefix/infra".to_string(),
+                        "/.github/workflows/push-sources.yml @Prefix/infra".to_string(),
+                        "/.vscode/extensions/z/**/* @Prefix/zteam".to_string(),
+                        "/Dockerfile @Prefix/docker-team".to_string(),
+                        "/bin/brakeman @Prefix/psecurity".to_string(),
+                        "/components/create.rb @Prefix/component-team".to_string(),
+                        "/config/brakeman.ignore @Prefix/security".to_string()
+                    ]
+                },
+            ]
+        );
+        let stripped_lines = stripped_lines_by_priority(codeownership_file);
+        assert_eq!(
+            stripped_lines,
+            vec![
+                "/config/brakeman.ignore @Prefix/security",
+                "/components/create.rb @Prefix/component-team",
+                "/bin/brakeman @Prefix/psecurity",
+                "/Dockerfile @Prefix/docker-team",
+                "/.vscode/extensions/z/**/* @Prefix/zteam",
+                "/.github/workflows/push-sources.yml @Prefix/infra",
+                "/.github/workflows/pull-translations.yml @Prefix/infra",
+                "/.codeclimate.yml @Prefix/climate-team",
+                "/config/i18n-tasks.yml.erb @Prefix/language-team",
+                "/config/application.rb @Prefix/team-bar",
+                "/app/assets/config/manifest.js @Prefix/team-foo"
+            ]
+        );
+        Ok(())
+    }
+
+    #[test]
+    fn test_unignore_regex() -> Result<(), Box<dyn Error>> {
+        let un_ignore = Regex::new(r"^# \/")?;
+        assert_eq!(un_ignore.replace("# /path/to/owned", "/"), "/path/to/owned");
+        Ok(())
+    }
 }
diff --git a/src/project_builder.rs b/src/project_builder.rs
@@ -7,7 +7,7 @@ use error_stack::{Result, ResultExt};
 use fast_glob::glob_match;
 use ignore::WalkBuilder;
 use rayon::iter::{IntoParallelIterator, ParallelIterator};
-use tracing::instrument;
+use tracing::{instrument, warn};
 
 use crate::{
     cache::Cache,
@@ -149,10 +149,14 @@ impl<'a> ProjectBuilder<'a> {
                                 owner,
                             });
                         }
-                        EntryType::TeamFile(absolute_path, _relative_path) => {
-                            let team = Team::from_team_file_path(absolute_path).unwrap();
-                            team_files.push(team);
-                        }
+                        EntryType::TeamFile(absolute_path, _relative_path) => match Team::from_team_file_path(absolute_path) {
+                            Ok(team) => {
+                                team_files.push(team);
+                            }
+                            Err(e) => {
+                                warn!("Error building team from team file path: {}", e);
+                            }
+                        },
                         EntryType::NullEntry() => {}
                     }
                     (project_files, pkgs, gems, codeowners, team_files)
@@ -169,7 +173,10 @@ impl<'a> ProjectBuilder<'a> {
                     acc
                 },
             );
-        let teams_by_name = teams.iter().map(|team| (team.name.clone(), team.clone())).collect();
+        let teams_by_name = teams
+            .iter()
+            .flat_map(|team| vec![(team.name.clone(), team.clone()), (team.github_team.clone(), team.clone())])
+            .collect();
 
         Ok(Project {
             base_path: self.base_path.to_owned(),
diff --git a/src/runner.rs b/src/runner.rs
@@ -69,14 +69,17 @@ fn for_file_from_codeowners(run_config: &RunConfig, file_path: &str) -> RunResul
 
 pub fn team_for_file_from_codeowners(run_config: &RunConfig, file_path: &str) -> Result<Option<Team>, Error> {
     let config = config_from_path(&run_config.config_path)?;
+    let relative_file_path = Path::new(file_path)
+        .strip_prefix(&run_config.project_root)
+        .unwrap_or(Path::new(file_path));
 
     let parser = crate::ownership::parser::Parser {
         project_root: run_config.project_root.clone(),
         codeowners_file_path: run_config.codeowners_file_path.clone(),
         team_file_globs: config.team_file_glob.clone(),
     };
     Ok(parser
-        .team_from_file_path(Path::new(file_path))
+        .team_from_file_path(Path::new(relative_file_path))
         .map_err(|e| Error::Io(e.to_string()))?)
 }
 
@@ -198,7 +201,11 @@ impl Runner {
     }
 
     pub fn generate(&self) -> RunResult {
-        match std::fs::write(&self.run_config.codeowners_file_path, self.ownership.generate_file()) {
+        let content = self.ownership.generate_file();
+        if let Some(parent) = &self.run_config.codeowners_file_path.parent() {
+            let _ = std::fs::create_dir_all(parent);
+        }
+        match std::fs::write(&self.run_config.codeowners_file_path, content) {
             Ok(_) => RunResult::default(),
             Err(err) => RunResult {
                 io_errors: vec![err.to_string()],
@@ -216,7 +223,10 @@ impl Runner {
     }
 
     pub fn for_file(&self, file_path: &str) -> RunResult {
-        let file_owners = match self.ownership.for_file(file_path) {
+        let relative_file_path = Path::new(file_path)
+            .strip_prefix(&self.run_config.project_root)
+            .unwrap_or(Path::new(file_path));
+        let file_owners = match self.ownership.for_file(relative_file_path) {
             Ok(file_owners) => file_owners,
             Err(err) => {
                 return RunResult {
diff --git a/tests/valid_project_test.rs b/tests/valid_project_test.rs
