From 4d3cf0bf19817500a37c786e413e03d8dd17e994 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Tue, 12 Aug 2025 15:11:33 -0500 Subject: [PATCH 1/4] removing unused for-file code --- src/bin/compare_for_file.rs | 179 ---------------------------------- src/cli.rs | 2 +- src/runner.rs | 68 +------------ tests/invalid_project_test.rs | 18 ---- tests/valid_project_test.rs | 13 ++- 5 files changed, 13 insertions(+), 267 deletions(-) delete mode 100644 src/bin/compare_for_file.rs diff --git a/src/bin/compare_for_file.rs b/src/bin/compare_for_file.rs deleted file mode 100644 index 7bce8ca..0000000 --- a/src/bin/compare_for_file.rs +++ /dev/null @@ -1,179 +0,0 @@ -// This is a tool to compare the output of the original codeowners CLI with the optimized version. -// It's useful for verifying that the optimized version is correct. -// -// It's not used in CI, but it's useful for debugging. -// -// To run it, use `cargo run --bin compare_for_file ` -// -// It will compare the output of the original codeowners CLI with the optimized version for all files in the project. - -use std::{ - fs::File, - io::{self, Write}, - path::{Path, PathBuf}, - process::Command, -}; - -use codeowners::config::Config as OwnershipConfig; -use codeowners::ownership::{FileOwner, for_file_fast}; -use codeowners::runner::{RunConfig, Runner}; -use ignore::WalkBuilder; - -fn main() { - let project_root = std::env::args().nth(1).expect("usage: compare_for_file "); - let project_root = PathBuf::from(project_root); - if !project_root.is_absolute() { - eprintln!("Project root must be absolute"); - std::process::exit(2); - } - - let codeowners_file_path = project_root.join(".github/CODEOWNERS"); - let config_path = project_root.join("config/code_ownership.yml"); - - let run_config = RunConfig { - project_root: project_root.clone(), - codeowners_file_path, - config_path: config_path.clone(), - no_cache: false, - }; - - // Build the original, accurate-but-slower runner once - let runner = match Runner::new(&run_config) { - Ok(r) => r, - Err(e) => { - eprintln!("Failed to initialize Runner: {}", e); - std::process::exit(1); - } - }; - - // Load config once for the optimized path - let config_file = match File::open(&config_path) { - Ok(f) => f, - Err(e) => { - eprintln!("Can't open config file {}: {}", config_path.display(), e); - std::process::exit(1); - } - }; - let optimized_config: OwnershipConfig = match serde_yaml::from_reader(config_file) { - Ok(c) => c, - Err(e) => { - eprintln!("Can't parse config file {}: {}", config_path.display(), e); - std::process::exit(1); - } - }; - - let mut total_files: usize = 0; - let mut diff_count: usize = 0; - - // Prefer tracked files from git; fall back to walking the FS if git is unavailable - let tracked_files_output = Command::new("git").arg("-C").arg(&project_root).arg("ls-files").arg("-z").output(); - - match tracked_files_output { - Ok(output) if output.status.success() => { - let bytes = output.stdout; - for rel in bytes.split(|b| *b == 0u8) { - if rel.is_empty() { - continue; - } - let rel_str = match std::str::from_utf8(rel) { - Ok(s) => s, - Err(_) => continue, - }; - let abs_path = project_root.join(rel_str); - // Only process regular files that currently exist - if !abs_path.is_file() { - continue; - } - - total_files += 1; - let original = run_original(&runner, &abs_path); - let optimized = run_optimized(&project_root, &optimized_config, &abs_path); - - if original != optimized { - diff_count += 1; - println!("\n==== {} ====", abs_path.display()); - println!("ORIGINAL:\n{}", original); - println!("OPTIMIZED:\n{}", optimized); - let _ = io::stdout().flush(); - } - - if total_files % 1000 == 0 { - eprintln!("Processed {} files... diffs so far: {}", total_files, diff_count); - } - } - } - _ => { - eprintln!("git ls-files failed; falling back to filesystem walk (untracked files may be included)"); - let walker = WalkBuilder::new(&project_root) - .hidden(false) - .git_ignore(true) - .git_exclude(true) - .follow_links(false) - .build(); - - for result in walker { - let entry = match result { - Ok(e) => e, - Err(err) => { - eprintln!("walk error: {}", err); - continue; - } - }; - if !entry.file_type().map(|t| t.is_file()).unwrap_or(false) { - continue; - } - let path = entry.path(); - total_files += 1; - - let original = run_original(&runner, path); - let optimized = run_optimized(&project_root, &optimized_config, path); - - if original != optimized { - diff_count += 1; - println!("\n==== {} ====", path.display()); - println!("ORIGINAL:\n{}", original); - println!("OPTIMIZED:\n{}", optimized); - let _ = io::stdout().flush(); - } - - if total_files % 1000 == 0 { - eprintln!("Processed {} files... diffs so far: {}", total_files, diff_count); - } - } - } - } - - println!("Checked {} files. Diffs: {}", total_files, diff_count); - if diff_count > 0 { - std::process::exit(3); - } -} - -fn run_original(runner: &Runner, file_path: &Path) -> String { - let result = runner.for_file(&file_path.to_string_lossy()); - if !result.validation_errors.is_empty() { - return result.validation_errors.join("\n"); - } - if !result.io_errors.is_empty() { - return format!("IO_ERROR: {}", result.io_errors.join(" | ")); - } - result.info_messages.join("\n") -} - -fn run_optimized(project_root: &Path, config: &OwnershipConfig, file_path: &Path) -> String { - let owners: Vec = match for_file_fast::find_file_owners(project_root, config, file_path) { - Ok(v) => v, - Err(e) => return format!("IO_ERROR: {}", e), - }; - match owners.len() { - 0 => format!("{}", FileOwner::default()), - 1 => format!("{}", owners[0]), - _ => { - let mut lines = vec!["Error: file is owned by multiple teams!".to_string()]; - for owner in owners { - lines.push(format!("\n{}", owner)); - } - lines.join("\n") - } - } -} diff --git a/src/cli.rs b/src/cli.rs index 714487c..dae6e40 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -109,7 +109,7 @@ pub fn cli() -> Result { 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, fast), + Command::ForFile { name, fast: _ } => runner::for_file(&run_config, &name), Command::ForTeam { name } => runner::for_team(&run_config, &name), Command::DeleteCache => runner::delete_cache(&run_config), }; diff --git a/src/runner.rs b/src/runner.rs index bfbe2fb..30fc0b1 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -36,37 +36,8 @@ pub struct Runner { cache: Cache, } -pub fn for_file(run_config: &RunConfig, file_path: &str, fast: bool) -> RunResult { - 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) - } -} - -fn for_file_from_codeowners(run_config: &RunConfig, file_path: &str) -> RunResult { - match team_for_file_from_codeowners(run_config, file_path) { - Ok(Some(team)) => { - let relative_team_yml_path = team.path.strip_prefix(&run_config.project_root).unwrap_or(&team.path); - - RunResult { - info_messages: vec![ - format!("Team: {}", team.name), - format!("Team YML: {}", relative_team_yml_path.display()), - ], - ..Default::default() - } - } - Ok(None) => RunResult { - info_messages: vec!["Team: Unowned".to_string(), "Team YML:".to_string()], - ..Default::default() - }, - Err(err) => RunResult { - io_errors: vec![err.to_string()], - ..Default::default() - }, - } +pub fn for_file(run_config: &RunConfig, file_path: &str) -> RunResult { + for_file_optimized(run_config, file_path) } pub fn team_for_file_from_codeowners(run_config: &RunConfig, file_path: &str) -> Result, Error> { @@ -289,41 +260,6 @@ impl Runner { .output(); } - // TODO: remove this once we've verified the fast path is working - #[allow(dead_code)] - pub fn for_file(&self, file_path: &str) -> RunResult { - 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 { - io_errors: vec![err.to_string()], - ..Default::default() - }; - } - }; - let info_messages: Vec = 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() - } - } - pub fn for_team(&self, team_name: &str) -> RunResult { let mut info_messages = vec![]; let mut io_errors = vec![]; diff --git a/tests/invalid_project_test.rs b/tests/invalid_project_test.rs index 9ed2192..824fa63 100644 --- a/tests/invalid_project_test.rs +++ b/tests/invalid_project_test.rs @@ -61,24 +61,6 @@ fn test_for_file() -> Result<(), Box> { Ok(()) } -#[test] -fn test_fast_for_file() -> Result<(), Box> { - Command::cargo_bin("codeowners")? - .arg("--project-root") - .arg("tests/fixtures/invalid_project") - .arg("--no-cache") - .arg("for-file") - .arg("--fast") - .arg("ruby/app/models/blockchain.rb") - .assert() - .success() - .stdout(predicate::eq(indoc! {" - Team: Unowned - Team YML: - "})); - Ok(()) -} - #[test] fn test_for_file_multiple_owners() -> Result<(), Box> { Command::cargo_bin("codeowners")? diff --git a/tests/valid_project_test.rs b/tests/valid_project_test.rs index d8b4e81..790f9f2 100644 --- a/tests/valid_project_test.rs +++ b/tests/valid_project_test.rs @@ -86,13 +86,15 @@ fn test_fast_for_file() -> Result<(), Box> { .arg("tests/fixtures/valid_project") .arg("--no-cache") .arg("for-file") - .arg("--fast") .arg("ruby/app/models/payroll.rb") .assert() .success() .stdout(predicate::eq(indoc! {" Team: Payroll + Github Team: @PayrollTeam Team YML: config/teams/payroll.yml + Description: + - Owner annotation at the top of the file "})); Ok(()) } @@ -127,13 +129,15 @@ fn test_fast_for_file_full_path() -> Result<(), Box> { .arg(project_root) .arg("--no-cache") .arg("for-file") - .arg("--fast") .arg(for_file_absolute_path.to_str().unwrap()) .assert() .success() .stdout(predicate::eq(indoc! {" Team: Payroll + Github Team: @PayrollTeam Team YML: config/teams/payroll.yml + Description: + - Owner annotation at the top of the file "})); Ok(()) } @@ -186,13 +190,16 @@ fn test_fast_for_file_same_team_multiple_ownerships() -> Result<(), Box Date: Tue, 12 Aug 2025 16:11:04 -0500 Subject: [PATCH 2/4] show version --- src/cli.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/cli.rs b/src/cli.rs index dae6e40..f0e45ec 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -6,6 +6,7 @@ use path_clean::PathClean; use std::path::{Path, PathBuf}; #[derive(Subcommand, Debug)] +#[command(version)] enum Command { #[clap(about = "Finds the owner of a given file.", visible_alias = "f")] ForFile { From 664631181527b456835bfbd50495ee637a1dedd2 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Tue, 12 Aug 2025 16:16:28 -0500 Subject: [PATCH 3/4] adding the version --- src/runner.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/runner.rs b/src/runner.rs index 30fc0b1..b1d71d0 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -108,6 +108,10 @@ fn for_file_optimized(run_config: &RunConfig, file_path: &str) -> RunResult { } } +pub fn version() -> String { + env!("CARGO_PKG_VERSION").to_string() +} + pub fn for_team(run_config: &RunConfig, team_name: &str) -> RunResult { run_with_runner(run_config, |runner| runner.for_team(team_name)) } @@ -296,3 +300,13 @@ impl Runner { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_version() { + assert_eq!(version(), env!("CARGO_PKG_VERSION").to_string()); + } +} From 08514aa0f2d7e38cfaeba4b55439514bb4b15609 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Tue, 12 Aug 2025 17:57:20 -0500 Subject: [PATCH 4/4] avoiding panics --- dev/run_benchmarks_for_file.sh | 1 - dev/run_benchmarks_for_gv.sh | 1 - src/ownership/parser.rs | 26 ++++++++++++-------- src/project_builder.rs | 43 ++++++++++++++++++++++++---------- 4 files changed, 47 insertions(+), 24 deletions(-) diff --git a/dev/run_benchmarks_for_file.sh b/dev/run_benchmarks_for_file.sh index bb60317..a3bfa7f 100755 --- a/dev/run_benchmarks_for_file.sh +++ b/dev/run_benchmarks_for_file.sh @@ -11,5 +11,4 @@ echo "To run these benchmarks on your application, you can place this repo next hyperfine --warmup=2 --runs=3 --export-markdown tmp/codeowners_for_file_benchmarks.md \ "../rubyatscale/codeowners-rs/target/release/codeowners for-file \"$1\"" \ - "bin/codeowners for_file \"$1\"" \ "bin/codeownership for_file \"$1\"" \ No newline at end of file diff --git a/dev/run_benchmarks_for_gv.sh b/dev/run_benchmarks_for_gv.sh index 3892238..1277351 100755 --- a/dev/run_benchmarks_for_gv.sh +++ b/dev/run_benchmarks_for_gv.sh @@ -9,5 +9,4 @@ echo "To run these benchmarks on your application, you can place this repo next hyperfine --warmup=2 --runs=3 --export-markdown tmp/codeowners_benchmarks_gv.md \ '../rubyatscale/codeowners-rs/target/release/codeowners gv' \ - 'bin/codeowners validate' \ 'bin/codeownership validate' \ No newline at end of file diff --git a/src/ownership/parser.rs b/src/ownership/parser.rs index bd60bfa..9681420 100644 --- a/src/ownership/parser.rs +++ b/src/ownership/parser.rs @@ -59,17 +59,23 @@ impl Parser { fn teams_by_github_team_name(team_file_glob: Vec) -> HashMap { 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); - - for path in paths { - let team = match Team::from_team_file_path(path) { - Ok(team) => team, - Err(e) => { - eprintln!("Error parsing team file: {}", e); - continue; + match glob::glob(&glob) { + Ok(paths) => { + for path in paths.filter_map(Result::ok) { + let team = match Team::from_team_file_path(path) { + Ok(team) => team, + Err(e) => { + eprintln!("Error parsing team file: {}", e); + continue; + } + }; + teams.insert(team.github_team.clone(), team); } - }; - teams.insert(team.github_team.clone(), team); + } + Err(e) => { + eprintln!("Failed to read glob pattern '{}': {}", glob, e); + continue; + } } } diff --git a/src/project_builder.rs b/src/project_builder.rs index c0bb1ad..41bdc54 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -71,8 +71,25 @@ impl<'a> ProjectBuilder<'a> { }) }); - // Process sequentially with &mut self - let collected_entries = Arc::try_unwrap(collected).unwrap().into_inner().unwrap(); + // Process sequentially with &mut self without panicking on Arc/Mutex unwraps + let collected_entries = match Arc::try_unwrap(collected) { + // We are the sole owner of the Arc + Ok(mutex) => match mutex.into_inner() { + // Mutex not poisoned + Ok(entries) => entries, + // Recover entries even if the mutex was poisoned + Err(poisoned) => poisoned.into_inner(), + }, + // There are still other Arc references; lock and take the contents + Err(arc) => match arc.lock() { + Ok(mut guard) => std::mem::take(&mut *guard), + // Recover guard even if poisoned, then take contents + Err(poisoned) => { + let mut guard = poisoned.into_inner(); + std::mem::take(&mut *guard) + } + }, + }; for entry in collected_entries { entry_types.push(self.build_entry_type(entry)?); } @@ -89,11 +106,10 @@ impl<'a> ProjectBuilder<'a> { if is_dir { return Ok(EntryType::Directory(absolute_path.to_owned(), relative_path.to_owned())); } - let file_name = relative_path - .file_name() - .expect("expected a file_name") - .to_string_lossy() - .to_lowercase(); + let file_name = match relative_path.file_name() { + Some(name) => name.to_string_lossy().to_lowercase(), + None => return Ok(EntryType::NullEntry()), + }; match file_name.as_str() { name if name == "package.yml" @@ -142,11 +158,14 @@ impl<'a> ProjectBuilder<'a> { } EntryType::Directory(absolute_path, relative_path) => { if relative_path.parent() == Some(Path::new(&self.config.vendored_gems_path)) { - let file_name = relative_path.file_name().expect("expected a file_name"); - gems.push(VendoredGem { - path: absolute_path, - name: file_name.to_string_lossy().to_string(), - }); + if let Some(file_name) = relative_path.file_name() { + gems.push(VendoredGem { + path: absolute_path, + name: file_name.to_string_lossy().to_string(), + }); + } else { + warn!("Vendored gem path without file name: {:?}", relative_path); + } } } EntryType::RubyPackage(absolute_path, relative_path) => {