From 73b416e3a010f9cd9522c01bca5e7b10dde1cb86 Mon Sep 17 00:00:00 2001 From: Paul Campbell Date: Sun, 30 Jun 2024 13:12:43 +0100 Subject: [PATCH] refactor: git: replace Mutex with RwLock in Repository --- crates/git/src/repository/mod.rs | 6 ++--- crates/git/src/repository/open/mod.rs | 6 ++--- crates/git/src/repository/open/ofake.rs | 22 ++++++++-------- crates/git/src/repository/open/oreal.rs | 35 ++++++++++++++----------- crates/git/src/repository/open/otest.rs | 4 +-- 5 files changed, 39 insertions(+), 34 deletions(-) diff --git a/crates/git/src/repository/mod.rs b/crates/git/src/repository/mod.rs index 8b0c043..edb8219 100644 --- a/crates/git/src/repository/mod.rs +++ b/crates/git/src/repository/mod.rs @@ -1,5 +1,5 @@ // -use std::sync::{atomic::AtomicBool, Arc, Mutex}; +use std::sync::{atomic::AtomicBool, Arc, RwLock}; use derive_more::Deref as _; @@ -80,7 +80,7 @@ struct RealRepositoryFactory; impl RepositoryFactory for RealRepositoryFactory { fn open(&self, gitdir: &GitDir) -> Result> { let gix_repo = gix::ThreadSafeRepository::open(gitdir.to_path_buf())?.to_thread_local(); - let repo = RealOpenRepository::new(Arc::new(Mutex::new(gix_repo))); + let repo = RealOpenRepository::new(Arc::new(RwLock::new(gix_repo.into()))); Ok(Box::new(repo)) } @@ -93,7 +93,7 @@ impl RepositoryFactory for RealRepositoryFactory { )? .fetch_only(gix::progress::Discard, &AtomicBool::new(false))?; tracing::info!("created"); - let repo = RealOpenRepository::new(Arc::new(Mutex::new(gix_repo))); + let repo = RealOpenRepository::new(Arc::new(RwLock::new(gix_repo.into()))); Ok(Box::new(repo)) } diff --git a/crates/git/src/repository/open/mod.rs b/crates/git/src/repository/open/mod.rs index fd5c9e2..da471f4 100644 --- a/crates/git/src/repository/open/mod.rs +++ b/crates/git/src/repository/open/mod.rs @@ -10,7 +10,7 @@ pub mod ofake; use std::{ path::Path, - sync::{Arc, Mutex}, + sync::{Arc, RwLock}, }; use crate as git; @@ -39,8 +39,8 @@ pub enum OpenRepository { #[cfg(not(tarpaulin_include))] pub fn real(gix_repo: gix::Repository) -> OpenRepository { - OpenRepository::Real(oreal::RealOpenRepository::new(Arc::new(Mutex::new( - gix_repo, + OpenRepository::Real(oreal::RealOpenRepository::new(Arc::new(RwLock::new( + gix_repo.into(), )))) } diff --git a/crates/git/src/repository/open/ofake.rs b/crates/git/src/repository/open/ofake.rs index bc1325c..148970d 100644 --- a/crates/git/src/repository/open/ofake.rs +++ b/crates/git/src/repository/open/ofake.rs @@ -4,14 +4,14 @@ use git_next_config as config; use std::{ path::Path, - sync::{Arc, Mutex}, + sync::{Arc, RwLock}, }; #[derive(Clone, Debug, Default)] pub struct FakeOpenRepository { - default_push_remote: Arc>>, - default_fetch_remote: Arc>>, - operations: Arc>>, + default_push_remote: Arc>>, + default_fetch_remote: Arc>>, + operations: Arc>>, } impl FakeOpenRepository { pub fn new() -> Self { @@ -26,7 +26,7 @@ impl FakeOpenRepository { match direction { git::repository::Direction::Push => self .default_push_remote - .lock() + .write() .map(|mut o| match remote { Some(gr) => o.replace(gr), None => o.take(), @@ -34,7 +34,7 @@ impl FakeOpenRepository { .unwrap(), git::repository::Direction::Fetch => self .default_fetch_remote - .lock() + .write() .map(|mut o| match remote { Some(gr) => o.replace(gr), None => o.take(), @@ -45,7 +45,7 @@ impl FakeOpenRepository { pub fn operations(&self) -> Vec { self.operations - .lock() + .read() .map(|operations| operations.clone()) .unwrap_or_default() } @@ -60,12 +60,12 @@ impl git::repository::OpenRepositoryLike for FakeOpenRepository { match direction { git::repository::Direction::Push => self .default_push_remote - .lock() + .read() .map(|r| r.clone()) .unwrap_or(None), git::repository::Direction::Fetch => self .default_fetch_remote - .lock() + .read() .map(|r| r.clone()) .unwrap_or(None), } @@ -73,7 +73,7 @@ impl git::repository::OpenRepositoryLike for FakeOpenRepository { fn fetch(&self) -> core::result::Result<(), crate::fetch::Error> { self.operations - .lock() + .write() .map_err(|_| crate::fetch::Error::Lock) .map(|mut operations| operations.push("fetch".to_string()))?; Ok(()) @@ -89,7 +89,7 @@ impl git::repository::OpenRepositoryLike for FakeOpenRepository { let forge_alias = repo_details.forge.forge_alias(); let repo_alias = &repo_details.repo_alias; self.operations - .lock() + .write() .map_err(|_| crate::fetch::Error::Lock) .map(|mut operations| { operations.push(format!( diff --git a/crates/git/src/repository/open/oreal.rs b/crates/git/src/repository/open/oreal.rs index 486659c..2f9f6f7 100644 --- a/crates/git/src/repository/open/oreal.rs +++ b/crates/git/src/repository/open/oreal.rs @@ -7,20 +7,20 @@ use git_next_config as config; use gix::bstr::BStr; use std::{ path::Path, - sync::{Arc, Mutex}, + sync::{Arc, RwLock}, }; use tracing::{info, warn}; #[derive(Clone, Debug, Constructor)] -pub struct RealOpenRepository(Arc>); +pub struct RealOpenRepository(Arc>); impl super::OpenRepositoryLike for RealOpenRepository { fn remote_branches(&self) -> git::push::Result> { let refs = self .0 - .lock() + .read() .map_err(|_| git::push::Error::Lock) .and_then(|repo| { - Ok(repo.references()?).and_then(|refs| { + Ok(repo.to_thread_local().references()?).and_then(|refs| { Ok(refs.remote_branches().map(|rb| { rb.filter_map(|rbi| rbi.ok()) .map(|r| r.name().to_owned()) @@ -37,11 +37,12 @@ impl super::OpenRepositoryLike for RealOpenRepository { Ok(refs) } fn find_default_remote(&self, direction: git::repository::Direction) -> Option { - let Ok(repository) = self.0.lock() else { + let Ok(repository) = self.0.read() else { #[cfg(not(tarpaulin_include))] // don't test mutex lock failure return None; }; - let Some(Ok(remote)) = repository.find_default_remote(direction.into()) else { + let thread_local = repository.to_thread_local(); + let Some(Ok(remote)) = thread_local.find_default_remote(direction.into()) else { #[cfg(not(tarpaulin_include))] // test is on local repo - should always have remotes return None; }; @@ -51,12 +52,13 @@ impl super::OpenRepositoryLike for RealOpenRepository { #[tracing::instrument(skip_all)] #[cfg(not(tarpaulin_include))] // would require writing to external service fn fetch(&self) -> Result<(), git::fetch::Error> { - let Ok(repository) = self.0.lock() else { + let Ok(repository) = self.0.read() else { #[cfg(not(tarpaulin_include))] // don't test mutex lock failure return Err(git::fetch::Error::Lock); }; + let thread_local = repository.to_thread_local(); let Some(Ok(remote)) = - repository.find_default_remote(git::repository::Direction::Fetch.into()) + thread_local.find_default_remote(git::repository::Direction::Fetch.into()) else { #[cfg(not(tarpaulin_include))] // test is on local repo - should always have remotes return Err(git::fetch::Error::NoFetchRemoteFound); @@ -98,7 +100,7 @@ impl super::OpenRepositoryLike for RealOpenRepository { .into(); let git_dir = self .0 - .lock() + .read() .map_err(|_| git::push::Error::Lock) .map(|r| r.git_dir().to_path_buf())?; let ctx = gix::diff::command::Context { @@ -125,12 +127,13 @@ impl super::OpenRepositoryLike for RealOpenRepository { false => 50, }; self.0 - .lock() + .read() .map_err(|_| git::commit::log::Error::Lock) .map(|repo| { let branch = format!("remotes/origin/{branch_name}"); let branch = BStr::new(&branch); - let branch_head = repo + let thread_local = repo.to_thread_local(); + let branch_head = thread_local .rev_parse_single(branch) .map_err(|e| e.to_string()) .map_err(as_gix_error(branch_name.clone()))?; @@ -142,7 +145,7 @@ impl super::OpenRepositoryLike for RealOpenRepository { .try_into_commit() .map_err(|e| e.to_string()) .map_err(as_gix_error(branch_name.clone()))?; - let walk = repo + let walk = thread_local .rev_walk([commit.id]) .all() .map_err(|e| e.to_string()) @@ -183,13 +186,15 @@ impl super::OpenRepositoryLike for RealOpenRepository { file_name: &Path, ) -> git::file::Result { self.0 - .lock() + .read() .map_err(|_| git::file::Error::Lock) .and_then(|repo| { - let fref = repo.find_reference(format!("origin/{}", branch_name).as_str())?; + let thread_local = repo.to_thread_local(); + let fref = + thread_local.find_reference(format!("origin/{}", branch_name).as_str())?; let id = fref.try_id().ok_or(git::file::Error::TryId)?; let oid = id.detach(); - let obj = repo.find_object(oid)?; + let obj = thread_local.find_object(oid)?; let commit = obj.into_commit(); let tree = commit.tree()?; let ent = tree diff --git a/crates/git/src/repository/open/otest.rs b/crates/git/src/repository/open/otest.rs index 4a44548..9a6f5de 100644 --- a/crates/git/src/repository/open/otest.rs +++ b/crates/git/src/repository/open/otest.rs @@ -5,7 +5,7 @@ use git_next_config as config; use std::{ path::Path, - sync::{Arc, Mutex, RwLock}, + sync::{Arc, RwLock}, }; pub type OnFetchFn = @@ -154,7 +154,7 @@ impl TestOpenRepository { fetch_counter: Arc::new(RwLock::new(0)), on_push, push_counter: Arc::new(RwLock::new(0)), - real: git::repository::RealOpenRepository::new(Arc::new(Mutex::new(gix))), + real: git::repository::RealOpenRepository::new(Arc::new(RwLock::new(gix.into()))), } }