From 134049de57d201eae3786f5918d75a9c85a6e5f1 Mon Sep 17 00:00:00 2001 From: Paul Campbell Date: Wed, 19 Jun 2024 16:40:10 +0100 Subject: [PATCH] WIP RepositoryFactory trait --- Cargo.toml | 1 + crates/cli/src/main.rs | 2 +- crates/git/Cargo.toml | 1 + crates/git/src/push.rs | 2 +- crates/git/src/repository/mod.rs | 52 +++++++++++++++++++--- crates/git/src/repository/open/mod.rs | 30 +++++++------ crates/git/src/repository/open/oreal.rs | 6 ++- crates/git/src/repository/open/otest.rs | 6 ++- crates/git/src/validation/positions.rs | 14 +++--- crates/git/src/validation/repo.rs | 6 +-- crates/repo-actor/src/branch.rs | 8 ++-- crates/repo-actor/src/lib.rs | 58 ++++++++++++++----------- crates/repo-actor/src/load.rs | 8 ++-- crates/server/src/actors/server.rs | 12 ++--- crates/server/src/lib.rs | 4 +- 15 files changed, 135 insertions(+), 75 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 11b3361..f703b9a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -96,3 +96,4 @@ tokio = { version = "1.37", features = ["rt", "macros"] } assert2 = "0.3" pretty_assertions = "1.4" rand = "0.8" +mockall = "0.12" diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index f8eb9da..a1023fb 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -30,7 +30,7 @@ enum Server { async fn main() { let fs = fs::new(PathBuf::default()); let net = Network::new_real(); - let repo = git_next_git::repository::new(); + let repo = git_next_git::repository::real(); let commands = Commands::parse(); match commands.command { diff --git a/crates/git/Cargo.toml b/crates/git/Cargo.toml index 59345cb..c49172e 100644 --- a/crates/git/Cargo.toml +++ b/crates/git/Cargo.toml @@ -56,6 +56,7 @@ actix = { workspace = true } assert2 = { workspace = true } rand = { workspace = true } pretty_assertions = { workspace = true } +mockall = { workspace = true } [lints.clippy] nursery = { level = "warn", priority = -1 } diff --git a/crates/git/src/push.rs b/crates/git/src/push.rs index 7c4161d..9e3d987 100644 --- a/crates/git/src/push.rs +++ b/crates/git/src/push.rs @@ -47,7 +47,7 @@ pub enum Error { } pub fn reset( - repository: &git::OpenRepository, + repository: &dyn git::repository::OpenRepositoryLike, repo_details: &git::RepoDetails, branch_name: &config::BranchName, to_commit: &git::GitRef, diff --git a/crates/git/src/repository/mod.rs b/crates/git/src/repository/mod.rs index 14e1b1a..0a5ac79 100644 --- a/crates/git/src/repository/mod.rs +++ b/crates/git/src/repository/mod.rs @@ -1,6 +1,10 @@ // #[cfg(test)] mod mock; +use std::sync::{atomic::AtomicBool, Arc, Mutex}; + +use derive_more::Deref as _; + #[cfg(test)] pub use mock::MockRepository; #[cfg(test)] @@ -26,11 +30,11 @@ pub use real::RealRepository; use tracing::info; use crate::repository::test::TestRepository; - use crate::validation::repo::validate_repo; use super::RepoDetails; +// TODO: #[deprecated] #[derive(Clone, Debug)] #[allow(clippy::large_enum_variant)] pub enum Repository { @@ -62,11 +66,11 @@ pub const fn test(fs: kxio::fs::FileSystem) -> TestRepository { #[tracing::instrument(skip_all)] #[cfg(not(tarpaulin_include))] // requires network access to either clone new and/or fetch. pub fn open( - repository: &Repository, + repository: &dyn RepositoryFactory, repo_details: &RepoDetails, gitdir: config::GitDir, -) -> Result { - let repository = if !gitdir.exists() { +) -> Result> { + let open_repository = if !gitdir.exists() { info!("Local copy not found - cloning..."); repository.git_clone(repo_details)? } else { @@ -74,8 +78,44 @@ pub fn open( repository.open(&gitdir)? }; info!("Validating..."); - validate_repo(&repository, repo_details).map_err(|e| Error::Validation(e.to_string()))?; - Ok(repository) + validate_repo(&*open_repository, repo_details).map_err(|e| Error::Validation(e.to_string()))?; + Ok(open_repository) +} + +#[cfg_attr(test, mockall::automock)] +pub trait RepositoryFactory: std::fmt::Debug { + fn duplicate(&self) -> Box; + fn open(&self, gitdir: &GitDir) -> Result>; + fn git_clone(&self, repo_details: &RepoDetails) -> Result>; +} +pub fn real() -> Box { + Box::new(RealRepositoryFactory) +} +#[derive(Debug, Clone)] +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))); + Ok(Box::new(repo)) + } + + fn git_clone(&self, repo_details: &RepoDetails) -> Result> { + tracing::info!("creating"); + use secrecy::ExposeSecret; + let (gix_repo, _outcome) = gix::prepare_clone_bare( + repo_details.origin().expose_secret().as_str(), + repo_details.gitdir.deref(), + )? + .fetch_only(gix::progress::Discard, &AtomicBool::new(false))?; + tracing::info!("created"); + let repo = RealOpenRepository::new(Arc::new(Mutex::new(gix_repo))); + Ok(Box::new(repo)) + } + + fn duplicate(&self) -> Box { + Box::new(self.clone()) + } } pub trait RepositoryLike { diff --git a/crates/git/src/repository/open/mod.rs b/crates/git/src/repository/open/mod.rs index 0082fe0..75c9170 100644 --- a/crates/git/src/repository/open/mod.rs +++ b/crates/git/src/repository/open/mod.rs @@ -23,6 +23,7 @@ pub use omock::MockOpenRepository; pub use oreal::RealOpenRepository; pub use otest::TestOpenRepository; +// TODO: #[deprecated] #[derive(Clone, Debug)] pub enum OpenRepository { /// A real git repository. @@ -74,7 +75,8 @@ pub fn test_bare( OpenRepository::Test(TestOpenRepository::new_bare(gitdir, fs, on_fetch, on_push)) } -pub trait OpenRepositoryLike { +pub trait OpenRepositoryLike: std::fmt::Debug { + fn duplicate(&self) -> Box; fn remote_branches(&self) -> git::push::Result>; fn find_default_remote(&self, direction: Direction) -> Option; fn fetch(&self) -> Result<(), git::fetch::Error>; @@ -102,16 +104,16 @@ pub trait OpenRepositoryLike { file_name: &Path, ) -> git::file::Result; } -impl std::ops::Deref for OpenRepository { - type Target = dyn OpenRepositoryLike; - - fn deref(&self) -> &Self::Target { - match self { - Self::Real(real) => real, - Self::Test(test) => test, - - #[cfg(test)] - Self::Mock(mock) => mock, - } - } -} +// impl std::ops::Deref for OpenRepository { +// type Target = dyn OpenRepositoryLike; +// +// fn deref(&self) -> &Self::Target { +// match self { +// Self::Real(real) => real, +// Self::Test(test) => test, +// +// #[cfg(test)] +// Self::Mock(mock) => mock, +// } +// } +// } diff --git a/crates/git/src/repository/open/oreal.rs b/crates/git/src/repository/open/oreal.rs index 58426b8..486659c 100644 --- a/crates/git/src/repository/open/oreal.rs +++ b/crates/git/src/repository/open/oreal.rs @@ -1,5 +1,5 @@ // -use crate as git; +use crate::{self as git, repository::OpenRepositoryLike}; use config::BranchName; use derive_more::Constructor; use git_next_config as config; @@ -201,6 +201,10 @@ impl super::OpenRepositoryLike for RealOpenRepository { Ok(content) }) } + + fn duplicate(&self) -> Box { + Box::new(self.clone()) + } } fn as_gix_error(branch: BranchName) -> impl FnOnce(String) -> git::commit::log::Error { diff --git a/crates/git/src/repository/open/otest.rs b/crates/git/src/repository/open/otest.rs index 7720011..337dfc6 100644 --- a/crates/git/src/repository/open/otest.rs +++ b/crates/git/src/repository/open/otest.rs @@ -1,5 +1,5 @@ // -use crate as git; +use crate::{self as git, repository::OpenRepositoryLike}; use derive_more::{Constructor, Deref}; use git_next_config as config; @@ -132,6 +132,10 @@ impl git::repository::OpenRepositoryLike for TestOpenRepository { ) -> git::file::Result { self.real.read_file(branch_name, file_name) } + + fn duplicate(&self) -> Box { + Box::new(self.clone()) + } } impl TestOpenRepository { pub fn new( diff --git a/crates/git/src/validation/positions.rs b/crates/git/src/validation/positions.rs index 61ee563..f8acd64 100644 --- a/crates/git/src/validation/positions.rs +++ b/crates/git/src/validation/positions.rs @@ -16,7 +16,7 @@ pub struct Positions { #[allow(clippy::cognitive_complexity)] // TODO: (#83) reduce complexity pub fn validate_positions( - repository: &git::OpenRepository, + open_repository: &dyn git::repository::OpenRepositoryLike, repo_details: &git::RepoDetails, repo_config: config::RepoConfig, ) -> Result { @@ -24,9 +24,9 @@ pub fn validate_positions( let next_branch = repo_config.branches().next(); let dev_branch = repo_config.branches().dev(); // Collect Commit Histories for `main`, `next` and `dev` branches - repository.fetch()?; + open_repository.fetch()?; let commit_histories = - get_commit_histories(repository, &repo_config).map_err(Error::CommitLog)?; + get_commit_histories(open_repository, &repo_config).map_err(Error::CommitLog)?; // branch tips let main = commit_histories .main @@ -55,12 +55,12 @@ pub fn validate_positions( // verify that next is on main or at most one commit on top of main, else reset it back to main if is_not_based_on(&commit_histories.next[0..=1], &main) { info!("Main not on same commit as next, or it's parent - resetting next to main",); - return reset_next_to_main(repository, repo_details, &main, &next, &next_branch); + return reset_next_to_main(open_repository, repo_details, &main, &next, &next_branch); } // verify that next is an ancestor of dev, else reset it back to main if is_not_based_on(&commit_histories.dev, &next) { info!("Next is not an ancestor of dev - resetting next to main"); - return reset_next_to_main(repository, repo_details, &main, &next, &next_branch); + return reset_next_to_main(open_repository, repo_details, &main, &next, &next_branch); } Ok(git::validation::positions::Positions { @@ -72,7 +72,7 @@ pub fn validate_positions( } fn reset_next_to_main( - repository: &crate::OpenRepository, + repository: &dyn crate::repository::OpenRepositoryLike, repo_details: &crate::RepoDetails, main: &crate::Commit, next: &crate::Commit, @@ -100,7 +100,7 @@ fn is_not_based_on(commits: &[crate::commit::Commit], needle: &crate::Commit) -> } fn get_commit_histories( - repository: &git::repository::OpenRepository, + repository: &dyn git::repository::OpenRepositoryLike, repo_config: &config::RepoConfig, ) -> git::commit::log::Result { let main = (repository.commit_log(&repo_config.branches().main(), &[]))?; diff --git a/crates/git/src/validation/repo.rs b/crates/git/src/validation/repo.rs index 8052838..46c9560 100644 --- a/crates/git/src/validation/repo.rs +++ b/crates/git/src/validation/repo.rs @@ -4,13 +4,13 @@ use crate as git; #[tracing::instrument(skip_all)] pub fn validate_repo( - repository: &git::OpenRepository, + open_repository: &dyn git::repository::OpenRepositoryLike, repo_details: &git::RepoDetails, ) -> Result<()> { - let push_remote = repository + let push_remote = open_repository .find_default_remote(git::repository::Direction::Push) .ok_or_else(|| Error::NoDefaultPushRemote)?; - let fetch_remote = repository + let fetch_remote = open_repository .find_default_remote(git::repository::Direction::Fetch) .ok_or_else(|| Error::NoDefaultFetchRemote)?; let git_remote = repo_details.git_remote(); diff --git a/crates/repo-actor/src/branch.rs b/crates/repo-actor/src/branch.rs index d514b82..482b827 100644 --- a/crates/repo-actor/src/branch.rs +++ b/crates/repo-actor/src/branch.rs @@ -16,7 +16,7 @@ pub async fn advance_next( dev_commit_history: Vec, repo_details: git::RepoDetails, repo_config: config::RepoConfig, - repository: git::OpenRepository, + open_repository: &dyn git::repository::OpenRepositoryLike, addr: Addr, message_token: MessageToken, ) { @@ -31,7 +31,7 @@ pub async fn advance_next( } info!("Advancing next to commit '{}'", commit); if let Err(err) = git::push::reset( - &repository, + open_repository, &repo_details, &repo_config.branches().next(), &commit.into(), @@ -81,11 +81,11 @@ pub async fn advance_main( next: git::Commit, repo_details: &git::RepoDetails, repo_config: &config::RepoConfig, - repository: &git::OpenRepository, + open_repository: &dyn git::repository::OpenRepositoryLike, ) { info!("Advancing main to next"); if let Err(err) = git::push::reset( - repository, + &*open_repository, repo_details, &repo_config.branches().main(), &next.into(), diff --git a/crates/repo-actor/src/lib.rs b/crates/repo-actor/src/lib.rs index 3bf1815..be59827 100644 --- a/crates/repo-actor/src/lib.rs +++ b/crates/repo-actor/src/lib.rs @@ -32,8 +32,8 @@ pub struct RepoActor { last_main_commit: Option, last_next_commit: Option, last_dev_commit: Option, - repository: git::Repository, - open_repository: Option, + repository: Box, + open_repository: Option>, net: Network, forge: forge::Forge, } @@ -43,7 +43,7 @@ impl RepoActor { webhook: config::server::Webhook, generation: git::Generation, net: Network, - repo: git::Repository, + repository: Box, ) -> Self { let forge = forge::Forge::new(details.clone(), net.clone()); debug!(?forge, "new"); @@ -57,7 +57,7 @@ impl RepoActor { last_main_commit: None, last_next_commit: None, last_dev_commit: None, - repository: repo, + repository, open_repository: None, net, forge, @@ -96,7 +96,7 @@ impl Handler for RepoActor { #[tracing::instrument(name = "RepoActor::CloneRepo", skip_all, fields(repo = %self.repo_details /*, gitdir = %self.repo_details.gitdir */))] fn handle(&mut self, _msg: CloneRepo, ctx: &mut Self::Context) -> Self::Result { let gitdir = self.repo_details.gitdir.clone(); - match git::repository::open(&self.repository, &self.repo_details, gitdir) { + match git::repository::open(&*self.repository, &self.repo_details, gitdir) { Ok(repository) => { self.open_repository.replace(repository); if self.repo_details.repo_config.is_none() { @@ -121,14 +121,15 @@ impl Handler for RepoActor { fn handle(&mut self, _msg: LoadConfigFromRepo, ctx: &mut Self::Context) -> Self::Result { let details = self.repo_details.clone(); let addr = ctx.address(); - let Some(open_repository) = self.open_repository.clone() else { + let Some(open_repository) = &self.open_repository else { warn!("missing open repository - can't load configuration"); return; }; - repo_actor::load::load_file(details, addr, open_repository) + let open_repository = open_repository.duplicate(); + async move { repo_actor::load::load_file(details, addr, &*open_repository).await } .in_current_span() .into_actor(self) - .wait(ctx); + .wait(ctx) } } @@ -195,15 +196,15 @@ impl Handler for RepoActor { .into_actor(self) .wait(ctx); } - if let (Some(repository), Some(repo_config)) = ( - self.open_repository.clone(), - self.repo_details.repo_config.clone(), - ) { + if let (Some(open_repository), Some(repo_config)) = + (&self.open_repository, self.repo_details.repo_config.clone()) + { let repo_details = self.repo_details.clone(); let addr = ctx.address(); let message_token = self.message_token; + let open_repository = open_repository.duplicate(); async move { - match validate_positions(&repository, &repo_details, repo_config) { + match validate_positions(&*open_repository, &repo_details, repo_config) { Ok(Positions { main, next, @@ -258,16 +259,22 @@ impl Handler for RepoActor { .into_actor(self) .wait(ctx); } else if dev_ahead_of_next { - if let Some(repository) = self.open_repository.clone() { - branch::advance_next( - msg.next, - msg.dev_commit_history, - self.repo_details.clone(), - repo_config, - repository, - addr, - self.message_token, - ) + if let Some(open_repository) = &self.open_repository { + let open_repository = open_repository.duplicate(); + let repo_details = self.repo_details.clone(); + let message_token = self.message_token; + async move { + branch::advance_next( + msg.next, + msg.dev_commit_history, + repo_details, + repo_config, + &*open_repository, + addr, + message_token, + ) + .await + } .in_current_span() .into_actor(self) .wait(ctx); @@ -304,15 +311,16 @@ impl Handler for RepoActor { warn!("No config loaded"); return; }; - let Some(repository) = self.open_repository.clone() else { + let Some(open_repository) = &self.open_repository else { warn!("No repository opened"); return; }; let repo_details = self.repo_details.clone(); let addr = ctx.address(); let message_token = self.message_token; + let open_repository = open_repository.duplicate(); async move { - branch::advance_main(msg.0, &repo_details, &repo_config, &repository).await; + branch::advance_main(msg.0, &repo_details, &repo_config, &*open_repository).await; match repo_config.source() { git_next_config::RepoConfigSource::Repo => addr.do_send(LoadConfigFromRepo), git_next_config::RepoConfigSource::Server => { diff --git a/crates/repo-actor/src/load.rs b/crates/repo-actor/src/load.rs index be389d1..ff64e32 100644 --- a/crates/repo-actor/src/load.rs +++ b/crates/repo-actor/src/load.rs @@ -15,10 +15,10 @@ use super::{LoadedConfig, RepoActor}; pub async fn load_file( repo_details: git::RepoDetails, addr: Addr, - open_repository: git::OpenRepository, + open_repository: &dyn git::repository::OpenRepositoryLike, ) { info!("Loading .git-next.toml from repo"); - let repo_config = match load(&repo_details, &open_repository).await { + let repo_config = match load(&repo_details, open_repository).await { Ok(repo_config) => repo_config, Err(err) => { error!(?err, "Failed to load config"); @@ -31,7 +31,7 @@ pub async fn load_file( async fn load( details: &git::RepoDetails, - open_repository: &git::OpenRepository, + open_repository: &dyn git::repository::OpenRepositoryLike, ) -> Result { let contents = open_repository.read_file(&details.branch, &PathBuf::from(".git-next.toml"))?; let config = config::RepoConfig::parse(&contents)?; @@ -50,7 +50,7 @@ pub enum Error { pub async fn validate( config: config::RepoConfig, - open_repository: &git::OpenRepository, + open_repository: &dyn git::repository::OpenRepositoryLike, ) -> Result { let branches = open_repository.remote_branches()?; if !branches diff --git a/crates/server/src/actors/server.rs b/crates/server/src/actors/server.rs index 68df2b9..8a1f549 100644 --- a/crates/server/src/actors/server.rs +++ b/crates/server/src/actors/server.rs @@ -7,7 +7,7 @@ use config::server::{ServerConfig, ServerStorage, Webhook}; use git_next_config::{ self as config, ForgeAlias, ForgeConfig, GitDir, RepoAlias, ServerRepoConfig, }; -use git_next_git::{Generation, RepoDetails, Repository}; +use git_next_git::{repository::RepositoryFactory, Generation, RepoDetails}; use git_next_repo_actor::{CloneRepo, RepoActor}; use kxio::{fs::FileSystem, network::Network}; use tracing::{error, info, warn}; @@ -38,7 +38,7 @@ pub struct Server { webhook: Option>, fs: FileSystem, net: Network, - repo: Repository, + repository_factory: Box, } impl Actor for Server { type Context = Context; @@ -114,14 +114,14 @@ impl Handler for Server { } } impl Server { - pub fn new(fs: FileSystem, net: Network, repo: Repository) -> Self { + pub fn new(fs: FileSystem, net: Network, repo: Box) -> Self { let generation = Generation::new(); Self { generation, webhook: None, fs, net, - repo, + repository_factory: repo, } } fn create_forge_data_directories( @@ -180,7 +180,7 @@ impl Server { let server_storage = server_storage.clone(); let webhook = webhook.clone(); let net = self.net.clone(); - let repo = self.repo.clone(); + let repository_factory = self.repository_factory.duplicate(); let generation = self.generation; move |(repo_alias, server_repo_config)| { let span = tracing::info_span!("create_actor", alias = %repo_alias, config = %server_repo_config); @@ -213,7 +213,7 @@ impl Server { webhook.clone(), generation, net.clone(), - repo.clone(), + repository_factory.duplicate(), ); (forge_name.clone(), repo_alias, actor) } diff --git a/crates/server/src/lib.rs b/crates/server/src/lib.rs index e264869..f0930b6 100644 --- a/crates/server/src/lib.rs +++ b/crates/server/src/lib.rs @@ -3,7 +3,7 @@ mod config; // use actix::prelude::*; -use git_next_git::Repository; +use git_next_git::repository::RepositoryFactory; use kxio::{fs::FileSystem, network::Network}; use std::path::PathBuf; @@ -37,7 +37,7 @@ pub fn init(fs: FileSystem) { } } -pub async fn start(fs: FileSystem, net: Network, repo: Repository) { +pub async fn start(fs: FileSystem, net: Network, repo: Box) { init_logging(); info!("Starting Server...");