first stab at optimizing for-file

perryqh · perryqh · commit b76e8bd62f44 · 2025-08-09T15:07:05.000-05:00
diff --git a/ai-prompts/for-file-optimization.md b/ai-prompts/for-file-optimization.md
@@ -0,0 +1,21 @@
+## Re-architect `codeowners-rs`
+
+The CLI behaviors live in `src/cli.rs`.
+
+### What this tool does
+- **Generate CODEOWNERS**: Builds `.github/CODEOWNERS` from multiple ownership sources. See mappers in `src/ownership/mapper/`.
+- **Answer per-file ownership**: The `for-file` command returns the owner of a given file even if the checked-in `CODEOWNERS` is not up to date.
+
+### Current implementations
+- **Fast but potentially inaccurate**: `Runner::for_file_from_codeowners` (in `src/runner.rs`) parses the existing `CODEOWNERS` file to find a file’s owner. It’s very fast, but can be wrong if `CODEOWNERS` is stale.
+- **Accurate but slow**: `Runner::for_file` is correct but can take up to ~4 seconds on large repositories because it effectively determines ownership for many files and then returns the single result needed.
+
+Ownership resolution is complex because definitions often live in other files (packages, team configs, globs, etc.).
+
+## Assignment
+Implement a faster `for_file` that remains accurate.
+
+### Requirements
+- **Performance**: Under 1 second on large codebases (e.g., `$HOME/workspace/large`).
+- **Correctness**: All existing tests must continue to pass.
+- **Compatibility**: No changes to the external behavior of the `for-file` CLI command.
diff --git a/src/runner.rs b/src/runner.rs
@@ -39,7 +39,7 @@ pub fn for_file(run_config: &RunConfig, file_path: &str, fast: bool) -> RunResul
     if fast {
         for_file_from_codeowners(run_config, file_path)
     } else {
-        run_with_runner(run_config, |runner| runner.for_file(file_path))
+        for_file_optimized(run_config, file_path)
     }
 }
 
@@ -83,6 +83,311 @@ pub fn team_for_file_from_codeowners(run_config: &RunConfig, file_path: &str) ->
         .map_err(|e| Error::Io(e.to_string()))?)
 }
 
+use std::collections::{HashMap, HashSet};
+use std::fs;
+use fast_glob::glob_match;
+use glob::glob;
+use lazy_static::lazy_static;
+use regex::Regex;
+
+fn for_file_optimized(run_config: &RunConfig, file_path: &str) -> RunResult {
+    let config = match config_from_path(&run_config.config_path) {
+        Ok(c) => c,
+        Err(err) => {
+            return RunResult {
+                io_errors: vec![err.to_string()],
+                ..Default::default()
+            }
+        }
+    };
+
+    let absolute_file_path = std::path::Path::new(file_path);
+    let relative_file_path = absolute_file_path
+        .strip_prefix(&run_config.project_root)
+        .unwrap_or(absolute_file_path)
+        .to_path_buf();
+
+    let teams = match load_teams(&run_config.project_root, &config.team_file_glob) {
+        Ok(t) => t,
+        Err(err) => {
+            return RunResult {
+                io_errors: vec![err.to_string()],
+                ..Default::default()
+            }
+        }
+    };
+    let teams_by_name = build_teams_by_name_map(&teams);
+
+    let mut sources_by_team: HashMap<String, Vec<crate::ownership::mapper::Source>> = HashMap::new();
+
+    if let Some(team_name) = read_top_of_file_team(&absolute_file_path.to_path_buf()) {
+        if let Some(team) = teams_by_name.get(&team_name) {
+            sources_by_team.entry(team.name.clone()).or_default().push(crate::ownership::mapper::Source::TeamFile);
+        }
+    }
+
+    if let Some((owner_team_name, dir_source)) = most_specific_directory_owner(
+        &run_config.project_root,
+        &relative_file_path,
+        &teams_by_name,
+    ) {
+        sources_by_team.entry(owner_team_name).or_default().push(dir_source);
+    }
+
+    if let Some((owner_team_name, package_source)) = nearest_package_owner(
+        &run_config.project_root,
+        &relative_file_path,
+        &config,
+        &teams_by_name,
+    ) {
+        sources_by_team.entry(owner_team_name).or_default().push(package_source);
+    }
+
+    if let Some((owner_team_name, gem_source)) = vendored_gem_owner(&relative_file_path, &config, &teams) {
+        sources_by_team.entry(owner_team_name).or_default().push(gem_source);
+    }
+
+    if let Some(rel_str) = relative_file_path.to_str() {
+        for team in &teams {
+            let subtracts: HashSet<&str> = team.subtracted_globs.iter().map(|s| s.as_str()).collect();
+            for owned_glob in &team.owned_globs {
+                if glob_match(owned_glob, rel_str) && !subtracts.iter().any(|sub| glob_match(sub, rel_str)) {
+                    sources_by_team
+                        .entry(team.name.clone())
+                        .or_default()
+                        .push(crate::ownership::mapper::Source::TeamGlob(owned_glob.clone()));
+                }
+            }
+        }
+    }
+
+    for team in &teams {
+        let team_rel = team
+            .path
+            .strip_prefix(&run_config.project_root)
+            .unwrap_or(&team.path)
+            .to_path_buf();
+        if team_rel == relative_file_path {
+            sources_by_team.entry(team.name.clone()).or_default().push(crate::ownership::mapper::Source::TeamYml);
+        }
+    }
+
+    let mut file_owners: Vec<FileOwner> = Vec::new();
+    for (team_name, sources) in sources_by_team.into_iter() {
+        if let Some(team) = teams_by_name.get(&team_name) {
+            let relative_team_yml_path = team
+                .path
+                .strip_prefix(&run_config.project_root)
+                .unwrap_or(&team.path)
+                .to_string_lossy()
+                .to_string();
+            file_owners.push(FileOwner {
+                team: team.clone(),
+                team_config_file_path: relative_team_yml_path,
+                sources,
+            });
+        }
+    }
+
+    let info_messages: Vec<String> = match file_owners.len() {
+        0 => vec![format!("{}", FileOwner::default())],
+        1 => vec![format!("{}", file_owners[0])],
+        _ => {
+            let mut error_messages = vec!["Error: file is owned by multiple teams!".to_string()];
+            for file_owner in file_owners {
+                error_messages.push(format!("\n{}", file_owner));
+            }
+            return RunResult {
+                validation_errors: error_messages,
+                ..Default::default()
+            };
+        }
+    };
+    RunResult { info_messages, ..Default::default() }
+}
+
+fn build_teams_by_name_map(teams: &[Team]) -> HashMap<String, Team> {
+    let mut map = HashMap::new();
+    for team in teams {
+        map.insert(team.name.clone(), team.clone());
+        map.insert(team.github_team.clone(), team.clone());
+    }
+    map
+}
+
+fn load_teams(project_root: &std::path::Path, team_file_globs: &[String]) -> std::result::Result<Vec<Team>, String> {
+    let mut teams: Vec<Team> = Vec::new();
+    for glob_str in team_file_globs {
+        let absolute_glob = format!("{}/{}", project_root.display(), glob_str);
+        let paths = glob(&absolute_glob).map_err(|e| e.to_string())?;
+        for path in paths.flatten() {
+            match Team::from_team_file_path(path.clone()) {
+                Ok(team) => teams.push(team),
+                Err(e) => {
+                    eprintln!("Error parsing team file: {}", e);
+                    continue;
+                }
+            }
+        }
+    }
+    Ok(teams)
+}
+
+lazy_static! {
+    static ref TOP_OF_FILE_TEAM_REGEX: Regex = Regex::new(r#"^(?:#|//) @team (.*)$"#).expect("error compiling regular expression");
+}
+
+fn read_top_of_file_team(path: &std::path::PathBuf) -> Option<String> {
+    let content = fs::read_to_string(path).ok()?;
+    let first_line = content.lines().next()?;
+    TOP_OF_FILE_TEAM_REGEX
+        .captures(first_line)
+        .and_then(|cap| cap.get(1))
+        .map(|m| m.as_str().to_string())
+}
+
+fn most_specific_directory_owner(
+    project_root: &std::path::Path,
+    relative_file_path: &std::path::Path,
+    teams_by_name: &HashMap<String, Team>,
+) -> Option<(String, crate::ownership::mapper::Source)> {
+    let mut current = project_root.join(relative_file_path);
+    let mut best: Option<(String, crate::ownership::mapper::Source)> = None;
+    loop {
+        let parent_opt = current.parent().map(|p| p.to_path_buf());
+        let Some(parent) = parent_opt else { break };
+        let codeowner_path = parent.join(".codeowner");
+        if let Ok(owner_str) = fs::read_to_string(&codeowner_path) {
+            let owner = owner_str.trim();
+            if let Some(team) = teams_by_name.get(owner) {
+                let relative_dir = parent
+                    .strip_prefix(project_root)
+                    .unwrap_or(parent.as_path())
+                    .to_string_lossy()
+                    .to_string();
+                let candidate = (
+                    team.name.clone(),
+                    crate::ownership::mapper::Source::Directory(relative_dir),
+                );
+                match &best {
+                    None => best = Some(candidate),
+                    Some((_, existing_source)) => {
+                        let existing_len = source_directory_depth(existing_source);
+                        let candidate_len = source_directory_depth(&candidate.1);
+                        if candidate_len > existing_len {
+                            best = Some(candidate);
+                        }
+                    }
+                }
+            }
+        }
+        if parent == project_root { break; }
+        current = parent.clone();
+    }
+    best
+}
+
+fn nearest_package_owner(
+    project_root: &std::path::Path,
+    relative_file_path: &std::path::Path,
+    config: &Config,
+    teams_by_name: &HashMap<String, Team>,
+) -> Option<(String, crate::ownership::mapper::Source)> {
+    let mut current = project_root.join(relative_file_path);
+    loop {
+        let parent_opt = current.parent().map(|p| p.to_path_buf());
+        let Some(parent) = parent_opt else { break };
+        let parent_rel = parent.strip_prefix(project_root).unwrap_or(parent.as_path());
+        if let Some(rel_str) = parent_rel.to_str() {
+            if glob_list_matches(rel_str, &config.ruby_package_paths) {
+                let pkg_yml = parent.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(),
+                                crate::ownership::mapper::Source::Package(
+                                    package_path.to_string_lossy().to_string(),
+                                    package_glob,
+                                ),
+                            ));
+                        }
+                    }
+                }
+            }
+            if glob_list_matches(rel_str, &config.javascript_package_paths) {
+                let pkg_json = parent.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(),
+                                crate::ownership::mapper::Source::Package(
+                                    package_path.to_string_lossy().to_string(),
+                                    package_glob,
+                                ),
+                            ));
+                        }
+                    }
+                }
+            }
+        }
+        if parent == project_root { break; }
+        current = parent;
+    }
+    None
+}
+
+fn source_directory_depth(source: &crate::ownership::mapper::Source) -> usize {
+    match source {
+        crate::ownership::mapper::Source::Directory(path) => path.matches('/').count(),
+        _ => 0,
+    }
+}
+
+fn glob_list_matches(path: &str, globs: &[String]) -> bool {
+    globs.iter().any(|g| glob_match(g, path))
+}
+
+fn read_ruby_package_owner(path: &std::path::Path) -> std::result::Result<String, String> {
+    let file = std::fs::File::open(path).map_err(|e| e.to_string())?;
+    let deserializer: crate::project::deserializers::RubyPackage = serde_yaml::from_reader(file).map_err(|e| e.to_string())?;
+    deserializer.owner.ok_or_else(|| "Missing owner".to_string())
+}
+
+fn read_js_package_owner(path: &std::path::Path) -> std::result::Result<String, String> {
+    let file = std::fs::File::open(path).map_err(|e| e.to_string())?;
+    let deserializer: crate::project::deserializers::JavascriptPackage = serde_json::from_reader(file).map_err(|e| e.to_string())?;
+    deserializer
+        .metadata
+        .and_then(|m| m.owner)
+        .ok_or_else(|| "Missing owner".to_string())
+}
+
+fn vendored_gem_owner(
+    relative_file_path: &std::path::Path,
+    config: &Config,
+    teams: &[Team],
+) -> Option<(String, crate::ownership::mapper::Source)> {
+    use std::path::Component;
+    let mut comps = relative_file_path.components();
+    let first = comps.next()?;
+    let second = comps.next()?;
+    let first_str = match first { Component::Normal(s) => s.to_string_lossy(), _ => return None };
+    if first_str != config.vendored_gems_path { return None; }
+    let gem_name = match second { Component::Normal(s) => s.to_string_lossy().to_string(), _ => return None };
+    for team in teams {
+        if team.owned_gems.iter().any(|g| g == &gem_name) {
+            return Some((team.name.clone(), crate::ownership::mapper::Source::TeamGem));
+        }
+    }
+    None
+}
+
 pub fn for_team(run_config: &RunConfig, team_name: &str) -> RunResult {
     run_with_runner(run_config, |runner| runner.for_team(team_name))
 }
