From 62bee38c85397bb4fcde8e05cca5872eed24843b Mon Sep 17 00:00:00 2001 From: Paul Campbell Date: Thu, 9 May 2024 21:53:50 +0100 Subject: [PATCH] feat(server): use cached Repository from RepoActor --- src/server/actors/repo/branch.rs | 4 ++ src/server/actors/repo/mod.rs | 50 ++++++++++++------- src/server/config/mod.rs | 22 ++++---- src/server/config/tests.rs | 32 ++++-------- src/server/git/reset.rs | 9 ++-- src/server/gitforge/forgejo/branch/fetch.rs | 10 ++-- .../forgejo/branch/validate_positions.rs | 4 ++ src/server/gitforge/forgejo/mod.rs | 20 +++++--- src/server/gitforge/mock_forge.rs | 2 + src/server/gitforge/mod.rs | 2 + src/server/gitforge/types.rs | 7 +++ 11 files changed, 93 insertions(+), 69 deletions(-) diff --git a/src/server/actors/repo/branch.rs b/src/server/actors/repo/branch.rs index b771964..159e1a1 100644 --- a/src/server/actors/repo/branch.rs +++ b/src/server/actors/repo/branch.rs @@ -17,6 +17,7 @@ pub async fn advance_next( dev_commit_history: Vec, repo_config: config::RepoConfig, forge: gitforge::Forge, + repository: gitforge::Repository, addr: Addr, message_token: super::MessageToken, ) { @@ -31,6 +32,7 @@ pub async fn advance_next( } info!("Advancing next to commit '{}'", commit); if let Err(err) = forge.branch_reset( + &repository, repo_config.branches().next(), commit.into(), gitforge::Force::No, @@ -79,11 +81,13 @@ pub async fn advance_main( next: gitforge::Commit, repo_config: config::RepoConfig, forge: gitforge::Forge, + repository: gitforge::Repository, addr: Addr, message_token: super::MessageToken, ) { info!("Advancing main to next"); if let Err(err) = forge.branch_reset( + &repository, repo_config.branches().main(), next.into(), gitforge::Force::No, diff --git a/src/server/actors/repo/mod.rs b/src/server/actors/repo/mod.rs index 4656fea..19101be 100644 --- a/src/server/actors/repo/mod.rs +++ b/src/server/actors/repo/mod.rs @@ -191,13 +191,15 @@ impl Handler for RepoActor { .into_actor(self) .wait(ctx); } - if let Some(repo_config) = self.details.repo_config.clone() { + if let (Some(repository), Some(repo_config)) = + (self.repository.clone(), self.details.repo_config.clone()) + { let forge = self.forge.clone(); let addr = ctx.address(); let message_token = self.message_token; async move { forge - .branches_validate_positions(repo_config, addr, message_token) + .branches_validate_positions(repository, repo_config, addr, message_token) .await } .in_current_span() @@ -238,17 +240,20 @@ impl Handler for RepoActor { .into_actor(self) .wait(ctx); } else if dev_ahead_of_next { - branch::advance_next( - msg.next, - msg.dev_commit_history, - repo_config, - forge, - addr, - self.message_token, - ) - .in_current_span() - .into_actor(self) - .wait(ctx); + if let Some(repository) = self.repository.clone() { + branch::advance_next( + msg.next, + msg.dev_commit_history, + repo_config, + forge, + repository, + addr, + self.message_token, + ) + .in_current_span() + .into_actor(self) + .wait(ctx); + } } } } @@ -278,11 +283,22 @@ impl Handler for RepoActor { warn!("No config loaded"); return; }; + let Some(repository) = self.repository.clone() else { + warn!("No repository opened"); + return; + }; let forge = self.forge.clone(); let addr = ctx.address(); - branch::advance_main(msg.0, repo_config, forge, addr, self.message_token) - .in_current_span() - .into_actor(self) - .wait(ctx); + branch::advance_main( + msg.0, + repo_config, + forge, + repository, + addr, + self.message_token, + ) + .in_current_span() + .into_actor(self) + .wait(ctx); } } diff --git a/src/server/config/mod.rs b/src/server/config/mod.rs index 3b1e263..7555c2f 100644 --- a/src/server/config/mod.rs +++ b/src/server/config/mod.rs @@ -14,7 +14,7 @@ use serde::Deserialize; use kxio::fs::FileSystem; use tracing::info; -use crate::server::types::ServerGeneration; +use crate::server::{gitforge::Repository, types::ServerGeneration}; #[derive(Debug, derive_more::From, derive_more::Display)] pub enum Error { @@ -418,11 +418,6 @@ impl RepoDetails { origin.into() } - #[allow(dead_code)] - pub fn validate_repo(&self) -> ValidationResult<()> { - self.gitdir.validate(self) - } - pub fn git_remote(&self) -> GitRemote { GitRemote::new(self.forge.hostname.clone(), self.repo_path.clone()) } @@ -516,11 +511,15 @@ impl GitDir { } #[tracing::instrument(skip_all)] - pub fn validate(&self, repo_details: &RepoDetails) -> ValidationResult<()> { + pub fn validate( + &self, + repository: &Repository, + repo_details: &RepoDetails, + ) -> ValidationResult<()> { let git_remote = repo_details.git_remote(); use gix::remote::Direction; - let push_remote = self.find_default_remote(Direction::Push)?; - let fetch_remote = self.find_default_remote(Direction::Fetch)?; + let push_remote = self.find_default_remote(repository, Direction::Push)?; + let fetch_remote = self.find_default_remote(repository, Direction::Fetch)?; info!(config = %git_remote, push = %push_remote, fetch = %fetch_remote, "Check remotes match"); if git_remote != push_remote { return Err(RepoValidationError::MismatchDefaultPushRemote { @@ -540,11 +539,10 @@ impl GitDir { #[tracing::instrument(skip_all, fields(?direction))] fn find_default_remote( &self, + repository: &Repository, direction: gix::remote::Direction, ) -> ValidationResult { - let repository = gix::ThreadSafeRepository::open(self.deref()) - .map_err(|e| RepoValidationError::UnableToOpenRepo(e.to_string()))? - .to_thread_local(); + let repository = repository.deref().to_thread_local(); let Some(Ok(remote)) = repository.find_default_remote(gix::remote::Direction::Push) else { return Err(RepoValidationError::NoDefaultPushRemote); }; diff --git a/src/server/config/tests.rs b/src/server/config/tests.rs index 284dd60..5877e9a 100644 --- a/src/server/config/tests.rs +++ b/src/server/config/tests.rs @@ -166,7 +166,8 @@ fn repo_details_find_default_push_remote_finds_correct_remote() -> Result<()> { repo_details.forge.hostname = Hostname("git.kemitix.net".to_string()); repo_details.repo_path = RepoPath("kemitix/git-next".to_string()); let gitdir = &repo_details.gitdir; - let found_git_remote = gitdir.find_default_remote(Direction::Push)?; + let repository = gix::ThreadSafeRepository::open(gitdir.to_path_buf())?; + let found_git_remote = gitdir.find_default_remote(&repository.into(), Direction::Push)?; let config_git_remote = repo_details.git_remote(); assert_eq!( @@ -189,29 +190,15 @@ fn gitdir_validate_should_pass_a_valid_git_repo() -> Result<()> { ); repo_details.forge.hostname = Hostname("git.kemitix.net".to_string()); repo_details.repo_path = RepoPath("kemitix/git-next".to_string()); - let git_dir = &repo_details.gitdir; - git_dir.validate(&repo_details)?; + let gitdir = &repo_details.gitdir; + let repository = gix::ThreadSafeRepository::open(gitdir.to_path_buf())?; + gitdir.validate(&repository.into(), &repo_details)?; Ok(()) } #[test] -fn gitdir_validate_should_fail_a_non_git_dir() { - let_assert!(Ok(fs) = kxio::fs::temp()); - let cwd = fs.base(); - let repo_details = common::repo_details( - 1, - ServerGeneration::new(), - common::forge_details(1, ForgeType::MockForge), - None, - GitDir::new(cwd), // Server GitDir - should be ignored - ); - let git_dir = &repo_details.gitdir; - let_assert!(Err(_) = git_dir.validate(&repo_details)); -} - -#[test] -fn gitdir_validate_should_fail_a_git_repo_with_wrong_remote() { +fn gitdir_validate_should_fail_a_git_repo_with_wrong_remote() -> Result<()> { let_assert!(Ok(cwd) = std::env::current_dir().map_err(RepoValidationError::Io)); let mut repo_details = common::repo_details( 1, @@ -222,8 +209,11 @@ fn gitdir_validate_should_fail_a_git_repo_with_wrong_remote() { ); repo_details.forge.hostname = Hostname("localhost".to_string()); repo_details.repo_path = RepoPath("hello/world".to_string()); - let git_dir = &repo_details.gitdir; - let_assert!(Err(_) = git_dir.validate(&repo_details)); + let gitdir = &repo_details.gitdir; + let repository = gix::ThreadSafeRepository::open(gitdir.to_path_buf())?; + let_assert!(Err(_) = gitdir.validate(&repository.into(), &repo_details)); + + Ok(()) } #[test] diff --git a/src/server/git/reset.rs b/src/server/git/reset.rs index 4fe6a89..3a46bab 100644 --- a/src/server/git/reset.rs +++ b/src/server/git/reset.rs @@ -5,22 +5,19 @@ use tracing::{info, warn}; use crate::server::{ config::{BranchName, RepoDetails}, - gitforge::{BranchResetError, BranchResetResult, Force}, + gitforge::{BranchResetError, BranchResetResult, Force, Repository}, types::GitRef, }; // TODO: (#72) reimplement using `gix` #[tracing::instrument(skip_all, fields(branch = %branch_name, to = %to_commit, force = %force))] pub fn reset( + repository: &Repository, repo_details: &RepoDetails, branch_name: BranchName, to_commit: GitRef, force: Force, ) -> BranchResetResult { - let repository = gix::ThreadSafeRepository::open(repo_details.gitdir.deref()) - .map_err(Box::new)? - .to_thread_local(); - let gitdir = repository.git_dir(); let origin = repo_details.origin(); let force = match force { Force::No => "".to_string(), @@ -33,7 +30,7 @@ pub fn reset( ) .into(); let ctx = gix::diff::command::Context { - git_dir: Some(gitdir.to_path_buf()), + git_dir: Some(repository.deref().git_dir().to_path_buf()), ..Default::default() }; match gix::command::prepare(command.expose_secret()) diff --git a/src/server/gitforge/forgejo/branch/fetch.rs b/src/server/gitforge/forgejo/branch/fetch.rs index 6d8c01c..0124920 100644 --- a/src/server/gitforge/forgejo/branch/fetch.rs +++ b/src/server/gitforge/forgejo/branch/fetch.rs @@ -2,7 +2,7 @@ use std::ops::Deref; use tracing::{debug, info}; -use crate::server::config::RepoDetails; +use crate::server::{config::RepoDetails, gitforge::Repository}; #[derive(Debug, derive_more::From, derive_more::Display)] pub enum Error { @@ -14,12 +14,8 @@ pub enum Error { impl std::error::Error for Error {} #[tracing::instrument(skip_all, fields(repo = %repo_details))] -pub fn fetch(repo_details: &RepoDetails) -> Result<(), Error> { - // INFO: gitdir validate tests that the default fetch remote matches the configured remote - let repository = gix::ThreadSafeRepository::open(repo_details.gitdir.deref()) - .map_err(Box::new)? - .to_thread_local(); - debug!(?repository, "opened repo"); +pub fn fetch(repository: &Repository, repo_details: &RepoDetails) -> Result<(), Error> { + let repository = repository.deref().to_thread_local(); let Some(remote) = repository.find_default_remote(gix::remote::Direction::Fetch) else { return Err(Error::NoFetchRemoteFound); }; diff --git a/src/server/gitforge/forgejo/branch/validate_positions.rs b/src/server/gitforge/forgejo/branch/validate_positions.rs index dc519d2..d9e4647 100644 --- a/src/server/gitforge/forgejo/branch/validate_positions.rs +++ b/src/server/gitforge/forgejo/branch/validate_positions.rs @@ -30,6 +30,7 @@ pub struct ValidatedPositions { pub async fn validate_positions( forge: &gitforge::forgejo::ForgeJoEnv, + repository: &gitforge::Repository, repo_config: RepoConfig, ) -> Result { let repo_details = &forge.repo_details; @@ -73,7 +74,9 @@ pub async fn validate_positions( let next_is_ancestor_of_dev = commit_histories.dev.iter().any(|dev| dev == &next); if !next_is_ancestor_of_dev { info!("Next is not an ancestor of dev - resetting next to main"); + if let Err(err) = forge.branch_reset( + repository, repo_config.branches().next(), main.into(), gitforge::Force::From(next.clone().into()), @@ -99,6 +102,7 @@ pub async fn validate_positions( repo_config.branches().next() ); if let Err(err) = forge.branch_reset( + repository, repo_config.branches().next(), main.into(), gitforge::Force::From(next.clone().into()), diff --git a/src/server/gitforge/forgejo/mod.rs b/src/server/gitforge/forgejo/mod.rs index 983f841..bbe6b20 100644 --- a/src/server/gitforge/forgejo/mod.rs +++ b/src/server/gitforge/forgejo/mod.rs @@ -48,11 +48,12 @@ impl super::ForgeLike for ForgeJoEnv { async fn branches_validate_positions( &self, + repository: Repository, repo_config: RepoConfig, addr: Addr, message_token: MessageToken, ) { - match branch::validate_positions(self, repo_config).await { + match branch::validate_positions(self, &repository, repo_config).await { Ok(ValidatedPositions { main, next, @@ -76,12 +77,19 @@ impl super::ForgeLike for ForgeJoEnv { fn branch_reset( &self, + repository: &Repository, branch_name: BranchName, to_commit: GitRef, force: gitforge::Force, ) -> gitforge::BranchResetResult { - branch::fetch(&self.repo_details)?; - git::reset(&self.repo_details, branch_name, to_commit, force) + branch::fetch(repository, &self.repo_details)?; + git::reset( + repository, + &self.repo_details, + branch_name, + to_commit, + force, + ) } async fn commit_status(&self, commit: &gitforge::Commit) -> gitforge::CommitStatus { @@ -129,7 +137,7 @@ impl super::ForgeLike for ForgeJoEnv { } fn repo_clone(&self, gitdir: GitDir) -> Result { - let repo = if !gitdir.exists() { + let repository = if !gitdir.exists() { info!("Local copy not found - cloning..."); repo::clone(&self.repo_details, gitdir.clone())? } else { @@ -137,10 +145,10 @@ impl super::ForgeLike for ForgeJoEnv { }; info!("Validating..."); gitdir - .validate(&self.repo_details) + .validate(&repository, &self.repo_details) .map_err(|e| RepoCloneError::Validation(e.to_string())) .inspect(|_| info!("Validation - OK"))?; - Ok(repo) + Ok(repository) } } diff --git a/src/server/gitforge/mock_forge.rs b/src/server/gitforge/mock_forge.rs index 0003322..f51c899 100644 --- a/src/server/gitforge/mock_forge.rs +++ b/src/server/gitforge/mock_forge.rs @@ -33,6 +33,7 @@ impl super::ForgeLike for MockForgeEnv { async fn branches_validate_positions( &self, + _repository: Repository, _repo_config: RepoConfig, _addr: actix::prelude::Addr, _message_token: MessageToken, @@ -42,6 +43,7 @@ impl super::ForgeLike for MockForgeEnv { fn branch_reset( &self, + _repository: &Repository, _branch_name: BranchName, _to_commit: GitRef, _force: gitforge::Force, diff --git a/src/server/gitforge/mod.rs b/src/server/gitforge/mod.rs index ad9b9fb..0d57b62 100644 --- a/src/server/gitforge/mod.rs +++ b/src/server/gitforge/mod.rs @@ -39,6 +39,7 @@ pub trait ForgeLike { /// positions as needed. async fn branches_validate_positions( &self, + repository: Repository, repo_config: RepoConfig, addr: actix::prelude::Addr, message_token: MessageToken, @@ -47,6 +48,7 @@ pub trait ForgeLike { /// Moves a branch to a new commit. fn branch_reset( &self, + repository: &Repository, branch_name: BranchName, to_commit: GitRef, force: Force, diff --git a/src/server/gitforge/types.rs b/src/server/gitforge/types.rs index 9d4c3f7..1570bbb 100644 --- a/src/server/gitforge/types.rs +++ b/src/server/gitforge/types.rs @@ -99,3 +99,10 @@ impl std::fmt::Display for Message { #[derive(Debug, Clone, derive_more::From)] pub struct Repository(gix::ThreadSafeRepository); +impl std::ops::Deref for Repository { + type Target = gix::ThreadSafeRepository; + + fn deref(&self) -> &Self::Target { + &self.0 + } +}