From 6863694293e6df2e16e3a6f8a8de3c425ed2d027 Mon Sep 17 00:00:00 2001 From: Brian Picciano Date: Tue, 4 Jul 2023 19:33:03 +0200 Subject: [PATCH] Pass descr around as a reference, generally --- src/domain/manager.rs | 7 +++--- src/origin.rs | 1 - src/origin/store.rs | 15 ++++-------- src/origin/store/git.rs | 46 +++++++++++------------------------ src/origin/store/mux.rs | 54 +++++++++++++++++++++-------------------- 5 files changed, 50 insertions(+), 73 deletions(-) diff --git a/src/domain/manager.rs b/src/domain/manager.rs index 5f2ee0f..716dfd5 100644 --- a/src/domain/manager.rs +++ b/src/domain/manager.rs @@ -193,7 +193,7 @@ impl ManagerImpl { } } .for_each(|descr| { - if let Err(err) = self.origin_store.sync(descr.clone(), origin::store::Limits {}) { + if let Err(err) = self.origin_store.sync(&descr) { log::error!("Failed to sync store for {:?}: {err}", descr); return; } @@ -217,7 +217,7 @@ impl Manager for ManagerImpl { let config = self.domain_config_store.get(domain)?; let origin = self .origin_store - .get(config.origin_descr) + .get(&config.origin_descr) // if there's a config there should be an origin, any error here is unexpected .or_unexpected()?; Ok(origin) @@ -252,8 +252,7 @@ impl Manager for ManagerImpl { .check_domain(&domain, &config_hash) .await?; - self.origin_store - .sync(config.origin_descr.clone(), origin::store::Limits {})?; + self.origin_store.sync(&config.origin_descr)?; self.domain_config_store.set(&domain, &config)?; diff --git a/src/origin.rs b/src/origin.rs index b0b448e..2dfdda1 100644 --- a/src/origin.rs +++ b/src/origin.rs @@ -17,7 +17,6 @@ pub enum ReadFileIntoError { /// Describes an origin which has already been synced locally and is available for reading files /// from. pub trait Origin { - fn descr(&self) -> &Descr; fn read_file_into( &self, path: &str, diff --git a/src/origin/store.rs b/src/origin/store.rs index db5a279..a420ca4 100644 --- a/src/origin/store.rs +++ b/src/origin/store.rs @@ -5,11 +5,6 @@ use std::sync; pub mod git; pub mod mux; -#[derive(Clone, Copy)] -pub struct Limits { - // TODO storage limits -} - #[derive(thiserror::Error, Clone, Debug, PartialEq)] pub enum SyncError { #[error("invalid url")] @@ -45,9 +40,9 @@ pub enum AllDescrsError { pub trait Store { /// If the origin is of a kind which can be updated, sync will pull down the latest version of /// the origin into the storage. - fn sync(&self, descr: origin::Descr, limits: Limits) -> Result<(), SyncError>; + fn sync(&self, descr: &origin::Descr) -> Result<(), SyncError>; - fn get(&self, descr: origin::Descr) -> Result, GetError>; + fn get(&self, descr: &origin::Descr) -> Result, GetError>; fn all_descrs(&self) -> Result, AllDescrsError>; } @@ -56,11 +51,11 @@ pub fn new_mock() -> sync::Arc> { } impl Store for sync::Arc> { - fn sync(&self, descr: origin::Descr, limits: Limits) -> Result<(), SyncError> { - self.lock().unwrap().sync(descr, limits) + fn sync(&self, descr: &origin::Descr) -> Result<(), SyncError> { + self.lock().unwrap().sync(descr) } - fn get(&self, descr: origin::Descr) -> Result, GetError> { + fn get(&self, descr: &origin::Descr) -> Result, GetError> { self.lock().unwrap().get(descr) } diff --git a/src/origin/store/git.rs b/src/origin/store/git.rs index b362642..1aeb9f4 100644 --- a/src/origin/store/git.rs +++ b/src/origin/store/git.rs @@ -6,16 +6,11 @@ use std::{collections, fs, io, sync}; #[derive(Clone)] struct Origin { - descr: origin::Descr, repo: sync::Arc, tree_object_id: gix::ObjectId, } impl origin::Origin for Origin { - fn descr(&self) -> &origin::Descr { - &self.descr - } - fn read_file_into( &self, path: &str, @@ -96,7 +91,7 @@ impl FSStore { fn get_origin( &self, repo: gix::Repository, - descr: origin::Descr, + descr: &origin::Descr, ) -> Result { let origin::Descr::Git { ref branch_name, .. @@ -120,17 +115,12 @@ impl FSStore { .tree(); Ok(Origin { - descr, repo: sync::Arc::new(repo.into()), tree_object_id, }) } - fn sync_inner( - &self, - descr: &origin::Descr, - _limits: store::Limits, - ) -> Result { + fn sync_inner(&self, descr: &origin::Descr) -> Result { use gix::clone::Error as gixCloneErr; use gix::progress::Discard; @@ -209,7 +199,7 @@ impl FSStore { } impl super::Store for FSStore { - fn sync(&self, descr: origin::Descr, limits: store::Limits) -> Result<(), store::SyncError> { + fn sync(&self, descr: &origin::Descr) -> Result<(), store::SyncError> { // attempt to lock this descr for syncing, doing so within a new scope so the mutex // isn't actually being held for the whole method duration. let is_already_syncing = { @@ -224,7 +214,7 @@ impl super::Store for FSStore { return Err(store::SyncError::AlreadyInProgress); } - let res = self.sync_inner(&descr, limits); + let res = self.sync_inner(descr); self.sync_guard.lock().unwrap().remove(&descr); @@ -242,21 +232,21 @@ impl super::Store for FSStore { // calling this while the sync lock is held isn't ideal, but it's convenient and // shouldn't be too terrible generally - let origin = self.get_origin(repo, descr.clone()).map_err(|e| match e { + let origin = self.get_origin(repo, &descr).map_err(|e| match e { GetOriginError::InvalidBranchName => store::SyncError::InvalidBranchName, GetOriginError::Unexpected(e) => store::SyncError::Unexpected(e), })?; let mut origins = self.origins.write().unwrap(); - (*origins).insert(descr, sync::Arc::new(origin)); + (*origins).insert(descr.clone(), sync::Arc::new(origin)); Ok(()) } - fn get(&self, descr: origin::Descr) -> Result, store::GetError> { + fn get(&self, descr: &origin::Descr) -> Result, store::GetError> { { let origins = self.origins.read().unwrap(); - if let Some(origin) = origins.get(&descr) { + if let Some(origin) = origins.get(descr) { return Ok(origin.clone()); } } @@ -273,7 +263,7 @@ impl super::Store for FSStore { let repo = gix::open(&repo_path) .map_unexpected_while(|| format!("opening {} as git repo", repo_path.display()))?; - let origin = self.get_origin(repo, descr.clone()).map_err(|e| match e { + let origin = self.get_origin(repo, descr).map_err(|e| match e { // it's not expected that the branch name is invalid at this point, it must have // existed for sync to have been successful. GetOriginError::InvalidBranchName => e.into_unexpected().into(), @@ -284,7 +274,7 @@ impl super::Store for FSStore { let mut origins = self.origins.write().unwrap(); - (*origins).insert(descr, origin.clone()); + (*origins).insert(descr.clone(), origin.clone()); Ok(origin) } @@ -342,25 +332,17 @@ mod tests { branch_name: String::from("some_other_branch"), }; - let limits = store::Limits {}; - let store = super::FSStore::new(tmp_dir.path().to_path_buf()).expect("store created"); - store - .sync(descr.clone(), limits) - .expect("sync should succeed"); - store - .sync(descr.clone(), limits) - .expect("second sync should succeed"); + store.sync(&descr).expect("sync should succeed"); + store.sync(&descr).expect("second sync should succeed"); assert!(matches!( - store.get(other_descr), + store.get(&other_descr), Err::<_, store::GetError>(store::GetError::NotFound), )); - let origin = store.get(descr.clone()).expect("origin retrieved"); - - assert_eq!(&descr, origin.descr()); + let origin = store.get(&descr).expect("origin retrieved"); let assert_write = |path: &str| { let mut into: Vec = vec![]; diff --git a/src/origin/store/mux.rs b/src/origin/store/mux.rs index f8fda8a..4f156e2 100644 --- a/src/origin/store/mux.rs +++ b/src/origin/store/mux.rs @@ -26,14 +26,14 @@ where S: store::Store + 'static, F: Fn(&origin::Descr) -> Option + Sync + Send, { - fn sync(&self, descr: origin::Descr, limits: store::Limits) -> Result<(), store::SyncError> { - (self.mapping_fn)(&descr) + fn sync(&self, descr: &origin::Descr) -> Result<(), store::SyncError> { + (self.mapping_fn)(descr) .or_unexpected_while(format!("mapping {:?} to store", &descr))? - .sync(descr, limits) + .sync(descr) } - fn get(&self, descr: origin::Descr) -> Result, store::GetError> { - (self.mapping_fn)(&descr) + fn get(&self, descr: &origin::Descr) -> Result, store::GetError> { + (self.mapping_fn)(descr) .or_unexpected_while(format!("mapping {:?} to store", &descr))? .get(descr) } @@ -52,7 +52,6 @@ where #[cfg(test)] mod tests { use crate::origin::{self, store}; - use mockall::predicate; use std::sync; struct Harness { @@ -112,30 +111,33 @@ mod tests { fn sync() { let h = Harness::new(); - h.store_a - .lock() - .unwrap() - .expect_sync() - .with(predicate::eq(h.descr_a.clone()), predicate::always()) - .times(1) - .return_const(Ok::<(), store::SyncError>(())); + { + let descr_a = h.descr_a.clone(); + h.store_a + .lock() + .unwrap() + .expect_sync() + .withf(move |descr: &origin::Descr| descr == &descr_a) + .times(1) + .return_const(Ok::<(), store::SyncError>(())); + } - assert_eq!(Ok(()), h.store.sync(h.descr_a.clone(), store::Limits {})); + assert_eq!(Ok(()), h.store.sync(&h.descr_a)); - h.store_b - .lock() - .unwrap() - .expect_sync() - .with(predicate::eq(h.descr_b.clone()), predicate::always()) - .times(1) - .return_const(Ok::<(), store::SyncError>(())); + { + let descr_b = h.descr_b.clone(); + h.store_b + .lock() + .unwrap() + .expect_sync() + .withf(move |descr: &origin::Descr| descr == &descr_b) + .times(1) + .return_const(Ok::<(), store::SyncError>(())); + } - assert_eq!(Ok(()), h.store.sync(h.descr_b.clone(), store::Limits {})); + assert_eq!(Ok(()), h.store.sync(&h.descr_b)); - assert!(h - .store - .sync(h.descr_unknown.clone(), store::Limits {}) - .is_err()); + assert!(h.store.sync(&h.descr_unknown).is_err()); } #[test]