From 7a0247ea03b61b3b93a712db1db4aad52778d1c7 Mon Sep 17 00:00:00 2001 From: Paul Campbell Date: Fri, 24 May 2024 08:47:34 +0100 Subject: [PATCH] refactor: get commit log from local repo (step 1) Avoid using a forge-specific API to get a commit log when the information is already available locally in the cloned repo through a generic git command. The commit adds the new method of getting the commit log and compares it with the original methods, logging if they match or not. The updated results are returned only if they match. --- .../src/branch/validate_positions.rs | 63 +++++++++++-- crates/forge-forgejo/src/tests/mod.rs | 45 ++++++++- crates/git/src/commit.rs | 11 +++ crates/git/src/repository/mock.rs | 34 +++++-- crates/git/src/repository/open/mod.rs | 42 +++++---- crates/git/src/repository/open/oreal.rs | 94 ++++++++++++++----- 6 files changed, 228 insertions(+), 61 deletions(-) diff --git a/crates/forge-forgejo/src/branch/validate_positions.rs b/crates/forge-forgejo/src/branch/validate_positions.rs index 286fcaa0..bc3bc0c8 100644 --- a/crates/forge-forgejo/src/branch/validate_positions.rs +++ b/crates/forge-forgejo/src/branch/validate_positions.rs @@ -14,7 +14,8 @@ pub async fn validate_positions( ) -> git::validation::Result { let repo_details = &forge.repo_details; // Collect Commit Histories for `main`, `next` and `dev` branches - let commit_histories = get_commit_histories(repo_details, &repo_config, &forge.net).await; + let commit_histories = + get_commit_histories(repository, repo_details, &repo_config, &forge.net).await; let commit_histories = match commit_histories { Ok(commit_histories) => commit_histories, Err(err) => { @@ -147,25 +148,34 @@ pub async fn validate_positions( } async fn get_commit_histories( + repository: &git::repository::OpenRepository, repo_details: &git::RepoDetails, repo_config: &config::RepoConfig, net: &network::Network, ) -> Result { - let main = - (get_commit_history(repo_details, &repo_config.branches().main(), vec![], net).await)?; + let main = (get_commit_history( + repository, + repo_details, + &repo_config.branches().main(), + &[], + net, + ) + .await)?; let main_head = main[0].clone(); let next = (get_commit_history( + repository, repo_details, &repo_config.branches().next(), - vec![main_head.clone()], + &[main_head.clone()], net, ) .await)?; let next_head = next[0].clone(); let dev = (get_commit_history( + repository, repo_details, &repo_config.branches().dev(), - vec![next_head, main_head], + &[next_head, main_head], net, ) .await)?; @@ -179,11 +189,45 @@ async fn get_commit_histories( Ok(histories) } -#[tracing::instrument(fields(%branch_name),skip_all)] async fn get_commit_history( + repository: &git::repository::OpenRepository, repo_details: &git::RepoDetails, branch_name: &config::BranchName, - find_commits: Vec, + find_commits: &[git::Commit], + net: &kxio::network::Network, +) -> Result, network::NetworkError> { + let original = original_get_commit_history(repo_details, branch_name, find_commits, net).await; + let updated = repository.commit_log(branch_name, find_commits); + match (original, updated) { + (Ok(original), Ok(updated)) => { + if updated == original { + info!("new version matches"); + Ok(updated) + } else { + error!(?updated, ?original, "new version doesn't match original"); + Ok(original) + } + } + (Ok(original), Err(err_updated)) => { + warn!(?err_updated, "original ok, updated error"); + Ok(original) + } + (Err(err_original), Ok(updated)) => { + warn!(?err_original, "original err, updated ok"); + Ok(updated) + } + (Err(err_orignal), Err(err_updated)) => { + error!(?err_orignal, ?err_updated, "original err, updated err"); + Err(err_orignal) + } + } +} + +#[tracing::instrument(fields(%branch_name),skip_all)] +async fn original_get_commit_history( + repo_details: &git::RepoDetails, + branch_name: &config::BranchName, + find_commits: &[git::Commit], net: &kxio::network::Network, ) -> Result, network::NetworkError> { let hostname = &repo_details.forge.hostname(); @@ -223,9 +267,8 @@ async fn get_commit_history( let found = find_commits.is_empty() || find_commits - .clone() - .into_iter() - .any(|find_commit| commits.iter().any(|commit| commit == &find_commit)); + .iter() + .any(|find_commit| commits.iter().any(|commit| commit == find_commit)); let at_end = commits.len() < limit; all_commits.extend(commits); if found || at_end { diff --git a/crates/forge-forgejo/src/tests/mod.rs b/crates/forge-forgejo/src/tests/mod.rs index 30223ca6..90e52d51 100644 --- a/crates/forge-forgejo/src/tests/mod.rs +++ b/crates/forge-forgejo/src/tests/mod.rs @@ -20,7 +20,7 @@ mod branch { let mut net = MockNetwork::new(); //given given_forgejo_has_branches(&mut net, 1); - let repo_details = given_a_repo(fs.base(), 1); + let repo_details = given_repo_details(fs.base(), 1); let net = Network::from(net); let (repo, _reality) = git::repository::mock(); @@ -35,6 +35,47 @@ mod branch { assert_eq!(branches, vec![config::BranchName::new("string")]); } } + // mod validate_positions { + // + // use git::ForgeLike; + // + // use super::*; + // + // #[test] + // fn should_ok_all_branches_on_same_commit() { + // let_assert!(Ok(fs) = kxio::fs::temp()); + // let mut net = MockNetwork::new(); + // //given + // let repo_details = given_repo_details(fs.base(), 1); + // let net = Network::from(net); + // let (repo, _reality) = git::repository::mock(); + // let forge = given_a_forge(repo_details, &net, repo); + // let open_repository = given_an_open_repository(); + // let repo_config = given_a_repo_config(); + // + // let forge = forgejo::ForgeJo::new(repo_details, net.clone(), repo); + // + // let_assert!( + // Ok(positions) = forge + // .branches_validate_positions(open_repository, repo_config) + // .await + // ); + // } + // + // fn given_a_forge( + // repo_details: git::RepoDetails, + // net: &Network, + // repo: git::Repository, + // ) -> forgejo::ForgeJo { + // forgejo::ForgeJo::new(repo_details, net.clone(), repo) + // } + // fn given_an_open_repository() -> git::OpenRepository { + // todo!() + // } + // fn given_a_repo_config() -> config::RepoConfig { + // todo!() + // } + // } } fn given_forgejo_has_branches(net: &mut MockNetwork, i: u32) { @@ -48,7 +89,7 @@ fn given_forgejo_has_branches(net: &mut MockNetwork, i: u32) { net.add_get_response(&url, StatusCode::OK, body); } -fn given_a_repo(path: &Path, i: u32) -> git::RepoDetails { +fn given_repo_details(path: &Path, i: u32) -> git::RepoDetails { git::common::repo_details( i, git::Generation::new(), diff --git a/crates/git/src/commit.rs b/crates/git/src/commit.rs index 59693e15..e78e4868 100644 --- a/crates/git/src/commit.rs +++ b/crates/git/src/commit.rs @@ -32,3 +32,14 @@ pub struct Histories { pub next: Vec, pub dev: Vec, } + +pub mod log { + use derive_more::{Display, From}; + + #[derive(Debug, Display, From)] + pub enum Error { + Gix(String), + Lock, + } + impl std::error::Error for Error {} +} diff --git a/crates/git/src/repository/mock.rs b/crates/git/src/repository/mock.rs index 6975b3e6..fc266151 100644 --- a/crates/git/src/repository/mock.rs +++ b/crates/git/src/repository/mock.rs @@ -6,8 +6,8 @@ use std::{ sync::{Arc, Mutex}, }; -use git_next_config::GitDir; - +use super::Error; +use crate as git; use crate::{ repository::{ open::{OpenRepository, OpenRepositoryLike}, @@ -15,8 +15,7 @@ use crate::{ }, GitRemote, RepoDetails, }; - -use super::Error; +use git_next_config::GitDir; #[derive(Debug, Default, Clone)] pub struct MockRepository(Arc>); @@ -117,7 +116,7 @@ impl OpenRepositoryLike for MockOpenRepository { .unwrap() } - fn fetch(&self) -> std::prelude::v1::Result<(), crate::fetch::Error> { + fn fetch(&self) -> core::result::Result<(), crate::fetch::Error> { self.inner.lock().map(|inner| inner.fetch()).unwrap() } @@ -127,12 +126,23 @@ impl OpenRepositoryLike for MockOpenRepository { branch_name: git_next_config::BranchName, to_commit: crate::GitRef, force: crate::push::Force, - ) -> std::prelude::v1::Result<(), crate::push::Error> { + ) -> core::result::Result<(), crate::push::Error> { self.inner .lock() .map(|inner| inner.push(repo_details, branch_name, to_commit, force)) .unwrap() } + + fn commit_log( + &self, + branch_name: &git_next_config::BranchName, + find_commits: &[git::Commit], + ) -> core::result::Result, git::commit::log::Error> { + self.inner + .lock() + .map(|inner| inner.commit_log(branch_name, find_commits)) + .unwrap() + } } impl OpenRepositoryLike for InnerMockOpenRepository { fn find_default_remote(&self, direction: Direction) -> Option { @@ -142,7 +152,7 @@ impl OpenRepositoryLike for InnerMockOpenRepository { } } - fn fetch(&self) -> std::prelude::v1::Result<(), crate::fetch::Error> { + fn fetch(&self) -> core::result::Result<(), crate::fetch::Error> { todo!() } @@ -152,7 +162,15 @@ impl OpenRepositoryLike for InnerMockOpenRepository { _branch_name: git_next_config::BranchName, _to_commit: crate::GitRef, _force: crate::push::Force, - ) -> std::prelude::v1::Result<(), crate::push::Error> { + ) -> core::result::Result<(), crate::push::Error> { + todo!() + } + + fn commit_log( + &self, + _branch_name: &git_next_config::BranchName, + _find_commits: &[git::Commit], + ) -> core::result::Result, crate::commit::log::Error> { todo!() } } diff --git a/crates/git/src/repository/open/mod.rs b/crates/git/src/repository/open/mod.rs index 2a86c43b..b2fcd65b 100644 --- a/crates/git/src/repository/open/mod.rs +++ b/crates/git/src/repository/open/mod.rs @@ -6,38 +6,44 @@ pub mod oreal; use std::sync::{Arc, Mutex}; -use git_next_config::BranchName; - -use crate::{ - fetch, push, - repository::{mock::MockOpenRepository, open::oreal::RealOpenRepository, Direction}, - GitRef, GitRemote, RepoDetails, -}; +use crate as git; +use git::repository::open::oreal::RealOpenRepository; +use git::repository::Direction; +use git_next_config as config; #[derive(Clone, Debug)] pub enum OpenRepository { - Real(oreal::RealOpenRepository), - Mock(MockOpenRepository), // TODO: (#38) contain a mock model of a repo + Real(RealOpenRepository), + Mock(git::repository::mock::MockOpenRepository), // TODO: (#38) contain a mock model of a repo } impl OpenRepository { pub fn real(gix_repo: gix::Repository) -> Self { - Self::Real(RealOpenRepository::new(Arc::new(Mutex::new(gix_repo)))) + Self::Real(oreal::RealOpenRepository::new(Arc::new(Mutex::new( + gix_repo, + )))) } #[cfg(not(tarpaulin_include))] // don't test mocks - pub const fn mock(mock: MockOpenRepository) -> Self { + pub const fn mock(mock: git::repository::mock::MockOpenRepository) -> Self { Self::Mock(mock) } } pub trait OpenRepositoryLike { - fn find_default_remote(&self, direction: Direction) -> Option; - fn fetch(&self) -> Result<(), fetch::Error>; + fn find_default_remote(&self, direction: Direction) -> Option; + fn fetch(&self) -> Result<(), git::fetch::Error>; fn push( &self, - repo_details: &RepoDetails, - branch_name: BranchName, - to_commit: GitRef, - force: push::Force, - ) -> Result<(), push::Error>; + repo_details: &git::RepoDetails, + branch_name: config::BranchName, + to_commit: git::GitRef, + force: git::push::Force, + ) -> Result<(), git::push::Error>; + + /// List of commits in a branch, optionally up-to any specified commit. + fn commit_log( + &self, + branch_name: &config::BranchName, + find_commits: &[git::Commit], + ) -> Result, git::commit::log::Error>; } impl std::ops::Deref for OpenRepository { type Target = dyn OpenRepositoryLike; diff --git a/crates/git/src/repository/open/oreal.rs b/crates/git/src/repository/open/oreal.rs index b03f4b87..61c5e8d3 100644 --- a/crates/git/src/repository/open/oreal.rs +++ b/crates/git/src/repository/open/oreal.rs @@ -1,15 +1,16 @@ // + +use crate as git; +use git_next_config as config; +use gix::bstr::BStr; + use std::sync::{Arc, Mutex}; - -use git_next_config::{BranchName, Hostname, RepoPath}; use tracing::{info, warn}; -use crate::{fetch, push, repository::Direction, GitRef, GitRemote, RepoDetails}; - #[derive(Clone, Debug, derive_more::Constructor)] pub struct RealOpenRepository(Arc>); impl super::OpenRepositoryLike for RealOpenRepository { - fn find_default_remote(&self, direction: Direction) -> Option { + fn find_default_remote(&self, direction: git::repository::Direction) -> Option { let Ok(repository) = self.0.lock() else { #[cfg(not(tarpaulin_include))] // don't test mutex lock failure return None; @@ -21,14 +22,16 @@ impl super::OpenRepositoryLike for RealOpenRepository { remote.url(direction.into()).map(Into::into) } - fn fetch(&self) -> Result<(), fetch::Error> { + fn fetch(&self) -> Result<(), git::fetch::Error> { let Ok(repository) = self.0.lock() else { #[cfg(not(tarpaulin_include))] // don't test mutex lock failure - return Err(fetch::Error::Lock); + return Err(git::fetch::Error::Lock); }; - let Some(Ok(remote)) = repository.find_default_remote(Direction::Fetch.into()) else { + let Some(Ok(remote)) = + repository.find_default_remote(git::repository::Direction::Fetch.into()) + else { #[cfg(not(tarpaulin_include))] // test is on local repo - should always have remotes - return Err(fetch::Error::NoFetchRemoteFound); + return Err(git::fetch::Error::NoFetchRemoteFound); }; let prepared_fetch = remote .connect(gix::remote::Direction::Fetch)? @@ -37,9 +40,9 @@ impl super::OpenRepositoryLike for RealOpenRepository { Ok(fetch) => { fetch .receive(gix::progress::Discard, &Default::default()) - .map_err(|e| fetch::Error::Fetch(e.to_string()))?; + .map_err(|e| git::fetch::Error::Fetch(e.to_string()))?; } - Err(e) => Err(fetch::Error::Fetch(e.to_string()))?, + Err(e) => Err(git::fetch::Error::Fetch(e.to_string()))?, } Ok(()) @@ -49,15 +52,17 @@ impl super::OpenRepositoryLike for RealOpenRepository { #[cfg(not(tarpaulin_include))] // would require writing to external service fn push( &self, - repo_details: &RepoDetails, - branch_name: BranchName, - to_commit: GitRef, - force: push::Force, - ) -> Result<(), push::Error> { + repo_details: &git::RepoDetails, + branch_name: config::BranchName, + to_commit: git::GitRef, + force: git::push::Force, + ) -> Result<(), git::push::Error> { let origin = repo_details.origin(); let force = match force { - push::Force::No => "".to_string(), - push::Force::From(old_ref) => format!("--force-with-lease={branch_name}:{old_ref}"), + git::push::Force::No => "".to_string(), + git::push::Force::From(old_ref) => { + format!("--force-with-lease={branch_name}:{old_ref}") + } }; // INFO: never log the command as it contains the API token within the 'origin' use secrecy::ExposeSecret; @@ -70,7 +75,7 @@ impl super::OpenRepositoryLike for RealOpenRepository { let git_dir = self .0 .lock() - .map_err(|_| push::Error::Lock) + .map_err(|_| git::push::Error::Lock) .map(|r| r.git_dir().to_path_buf())?; let ctx = gix::diff::command::Context { @@ -91,23 +96,66 @@ impl super::OpenRepositoryLike for RealOpenRepository { } Err(err) => { warn!(?err, ?git_dir, "Failed (wait)"); - Err(push::Error::Push) + Err(git::push::Error::Push) } }, Err(err) => { warn!(?err, ?git_dir, "Failed (spawn)"); - Err(push::Error::Push) + Err(git::push::Error::Push) } } } + + fn commit_log( + &self, + branch_name: &config::BranchName, + find_commits: &[git::Commit], + ) -> Result, git::commit::log::Error> { + self.0 + .lock() + .map_err(|_| git::commit::log::Error::Lock) + .map(|repo| { + let branch_name = branch_name.to_string(); + let branch_name = BStr::new(&branch_name); + let branch_head = repo + .rev_parse_single(branch_name) + .map_err(|e| e.to_string())?; + let object = branch_head.object().map_err(|e| e.to_string())?; + let commit = object.try_into_commit().map_err(|e| e.to_string())?; + let walk = repo + .rev_walk([commit.id]) + .all() + .map_err(|e| e.to_string())?; + let mut commits = vec![]; + for item in walk { + let item = item.map_err(|e| e.to_string())?; + let commit = item.object().map_err(|e| e.to_string())?; + let id = commit.id().to_string(); + let message = commit.message_raw().map_err(|e| e.to_string())?.to_string(); + let commit = git::Commit::new( + git::commit::Sha::new(id), + git::commit::Message::new(message), + ); + if find_commits.contains(&commit) { + commits.push(commit); + break; + } + commits.push(commit); + } + Ok(commits) + })? + } } -impl From<&gix::Url> for GitRemote { +impl From<&gix::Url> for git::GitRemote { fn from(url: &gix::Url) -> Self { let host = url.host().unwrap_or_default(); let path = url.path.to_string(); let path = path.strip_prefix('/').map_or(path.as_str(), |path| path); let path = path.strip_suffix(".git").map_or(path, |path| path); - Self::new(Hostname::new(host), RepoPath::new(path.to_string())) + Self::new( + config::Hostname::new(host), + config::RepoPath::new(path.to_string()), + ) } }