diff --git a/src/cache/file.rs b/src/cache/file.rs index ab0045b..36fc52d 100644 --- a/src/cache/file.rs +++ b/src/cache/file.rs @@ -21,11 +21,13 @@ const DEFAULT_CACHE_CAPACITY: usize = 10000; impl Caching for GlobalCache { fn get_file_owner(&self, path: &Path) -> Result, Error> { - if let Ok(cache) = self.file_owner_cache.as_ref().unwrap().lock() { - if let Some(cached_entry) = cache.get(path) { - let timestamp = get_file_timestamp(path)?; - if cached_entry.timestamp == timestamp { - return Ok(Some(cached_entry.clone())); + if let Some(cache_mutex) = self.file_owner_cache.as_ref() { + if let Ok(cache) = cache_mutex.lock() { + if let Some(cached_entry) = cache.get(path) { + let timestamp = get_file_timestamp(path)?; + if cached_entry.timestamp == timestamp { + return Ok(Some(cached_entry.clone())); + } } } } @@ -33,9 +35,11 @@ impl Caching for GlobalCache { } fn write_file_owner(&self, path: &Path, owner: Option) { - if let Ok(mut cache) = self.file_owner_cache.as_ref().unwrap().lock() { - if let Ok(timestamp) = get_file_timestamp(path) { - cache.insert(path.to_path_buf(), FileOwnerCacheEntry { timestamp, owner }); + if let Some(cache_mutex) = self.file_owner_cache.as_ref() { + if let Ok(mut cache) = cache_mutex.lock() { + if let Ok(timestamp) = get_file_timestamp(path) { + cache.insert(path.to_path_buf(), FileOwnerCacheEntry { timestamp, owner }); + } } } } @@ -50,8 +54,12 @@ impl Caching for GlobalCache { .change_context(Error::Io)?; let writer = BufWriter::new(file); - let cache = self.file_owner_cache.as_ref().unwrap().lock().map_err(|_| Error::Io)?; - serde_json::to_writer(writer, &*cache).change_context(Error::SerdeJson) + if let Some(cache_mutex) = self.file_owner_cache.as_ref() { + let cache = cache_mutex.lock().map_err(|_| Error::Io)?; + serde_json::to_writer(writer, &*cache).change_context(Error::SerdeJson) + } else { + Ok(()) + } } fn delete_cache(&self) -> Result<(), Error> { @@ -91,7 +99,7 @@ impl GlobalCache { fn get_cache_path(&self) -> PathBuf { let cache_dir = self.base_path.join(PathBuf::from(&self.cache_directory)); - fs::create_dir_all(&cache_dir).unwrap(); + let _ = fs::create_dir_all(&cache_dir); cache_dir.join("project-file-cache.json") } diff --git a/src/ownership/mapper/directory_mapper.rs b/src/ownership/mapper/directory_mapper.rs index 4916ef1..295e842 100644 --- a/src/ownership/mapper/directory_mapper.rs +++ b/src/ownership/mapper/directory_mapper.rs @@ -21,7 +21,10 @@ impl Mapper for DirectoryMapper { let team_by_name = self.project.teams_by_name.clone(); for directory_codeowner_file in &self.project.directory_codeowner_files { - let dir_root = directory_codeowner_file.directory_root().to_string_lossy(); + let dir_root = directory_codeowner_file + .directory_root() + .map(|p| p.to_string_lossy()) + .unwrap_or_default(); let team = team_by_name.get(&directory_codeowner_file.owner); if let Some(team) = team { entries.push(Entry { @@ -41,9 +44,12 @@ impl Mapper for DirectoryMapper { for file in &self.project.directory_codeowner_files { owner_matchers.push(OwnerMatcher::new_glob( - format!("{}/**/**", escape_brackets(&file.directory_root().to_string_lossy())), + format!( + "{}/**/**", + escape_brackets(&file.directory_root().map(|p| p.to_string_lossy()).unwrap_or_default()) + ), file.owner.to_owned(), - Source::Directory(file.directory_root().to_string_lossy().to_string()), + Source::Directory(file.directory_root().map(|p| p.to_string_lossy().to_string()).unwrap_or_default()), )); } diff --git a/src/ownership/mapper/package_mapper.rs b/src/ownership/mapper/package_mapper.rs index 2279779..5d7dc76 100644 --- a/src/ownership/mapper/package_mapper.rs +++ b/src/ownership/mapper/package_mapper.rs @@ -69,16 +69,18 @@ impl PackageMapper { let team_by_name = self.project.teams_by_name.clone(); for package in self.project.packages.iter().filter(|package| &package.package_type == package_type) { - let package_root = package.package_root().to_string_lossy(); - let team = team_by_name.get(&package.owner); - - if let Some(team) = team { - entries.push(Entry { - path: format!("{}/**/**", package_root), - github_team: team.github_team.to_owned(), - team_name: team.name.to_owned(), - disabled: team.avoid_ownership, - }); + if let Some(package_root) = package.package_root() { + let package_root = package_root.to_string_lossy(); + let team = team_by_name.get(&package.owner); + + if let Some(team) = team { + entries.push(Entry { + path: format!("{}/**/**", package_root), + github_team: team.github_team.to_owned(), + team_name: team.name.to_owned(), + disabled: team.avoid_ownership, + }); + } } } @@ -97,15 +99,17 @@ impl PackageMapper { let packages = remove_nested_packages(&packages); for package in packages { - let package_root = package.package_root().to_string_lossy(); - let team = team_by_name.get(&package.owner); - - if let Some(team) = team { - 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)), - )); + if let Some(package_root) = package.package_root() { + let package_root = package_root.to_string_lossy(); + let team = team_by_name.get(&package.owner); + + if let Some(team) = team { + 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)), + )); + } } } @@ -118,9 +122,10 @@ fn remove_nested_packages<'a>(packages: &'a [&'a Package]) -> Vec<&'a Package> { for package in packages.iter().sorted_by_key(|package| package.package_root()) { if let Some(last_package) = top_level_packages.last() { - let last_package_root = last_package.package_root(); - if !package.package_root().starts_with(last_package_root) { - top_level_packages.push(package); + if let (Some(current_root), Some(last_root)) = (package.package_root(), last_package.package_root()) { + if !current_root.starts_with(last_root) { + top_level_packages.push(package); + } } } else { top_level_packages.push(package); diff --git a/src/project.rs b/src/project.rs index db47e10..e405e90 100644 --- a/src/project.rs +++ b/src/project.rs @@ -66,8 +66,8 @@ pub struct Package { } impl Package { - pub fn package_root(&self) -> &Path { - self.path.parent().unwrap() + pub fn package_root(&self) -> Option<&Path> { + self.path.parent() } } @@ -78,8 +78,8 @@ pub struct DirectoryCodeownersFile { } impl DirectoryCodeownersFile { - pub fn directory_root(&self) -> &Path { - self.path.parent().unwrap() + pub fn directory_root(&self) -> Option<&Path> { + self.path.parent() } } diff --git a/src/project_builder.rs b/src/project_builder.rs index c9820b2..54f3ac3 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -78,10 +78,18 @@ impl<'a> ProjectBuilder<'a> { .to_lowercase(); match file_name.as_str() { - name if name == "package.yml" && matches_globs(relative_path.parent().unwrap(), &self.config.ruby_package_paths) => { + name if name == "package.yml" + && relative_path + .parent() + .is_some_and(|parent| matches_globs(parent, &self.config.ruby_package_paths)) => + { Ok(EntryType::RubyPackage(absolute_path.to_owned(), relative_path.to_owned())) } - name if name == "package.json" && matches_globs(relative_path.parent().unwrap(), &self.config.javascript_package_paths) => { + name if name == "package.json" + && relative_path + .parent() + .is_some_and(|parent| matches_globs(parent, &self.config.javascript_package_paths)) => + { Ok(EntryType::JavascriptPackage(absolute_path.to_owned(), relative_path.to_owned())) } ".codeowner" => Ok(EntryType::CodeownerFile(absolute_path.to_owned(), relative_path.to_owned())), @@ -124,31 +132,47 @@ impl<'a> ProjectBuilder<'a> { } } EntryType::RubyPackage(absolute_path, relative_path) => { - if let Some(owner) = ruby_package_owner(&absolute_path).unwrap() { - pkgs.push(Package { - path: relative_path.clone(), - owner, - package_type: PackageType::Ruby, - }); + match ruby_package_owner(&absolute_path) { + Ok(Some(owner)) => { + pkgs.push(Package { + path: relative_path.clone(), + owner, + package_type: PackageType::Ruby, + }); + } + Ok(None) => { /* No owner, do nothing */ } + Err(e) => { + warn!("Error reading ruby package owner for {:?}: {:?}", absolute_path, e); + } } } EntryType::JavascriptPackage(absolute_path, relative_path) => { - if let Some(owner) = javascript_package_owner(&absolute_path).unwrap() { - pkgs.push(Package { + match javascript_package_owner(&absolute_path) { + Ok(Some(owner)) => { + pkgs.push(Package { + path: relative_path.clone(), + owner, + package_type: PackageType::Javascript, + }); + } + Ok(None) => { /* No owner, do nothing */ } + Err(e) => { + warn!("Error reading javascript package owner for {:?}: {:?}", absolute_path, e); + } + } + } + EntryType::CodeownerFile(absolute_path, relative_path) => match std::fs::read_to_string(&absolute_path) { + Ok(owner) => { + let owner = owner.trim().to_owned(); + codeowners.push(DirectoryCodeownersFile { path: relative_path.clone(), owner, - package_type: PackageType::Javascript, }); } - } - EntryType::CodeownerFile(absolute_path, relative_path) => { - let owner = std::fs::read_to_string(absolute_path).unwrap(); - let owner = owner.trim().to_owned(); - codeowners.push(DirectoryCodeownersFile { - path: relative_path.clone(), - owner, - }); - } + Err(e) => { + warn!("Error reading codeowner file for {:?}: {:?}", absolute_path, e); + } + }, EntryType::TeamFile(absolute_path, _relative_path) => match Team::from_team_file_path(absolute_path) { Ok(team) => { team_files.push(team);