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.
This commit is contained in:
Paul Campbell 2024-05-24 08:47:34 +01:00
parent 7818b25a5c
commit 7a0247ea03
6 changed files with 228 additions and 61 deletions

View file

@ -14,7 +14,8 @@ pub async fn validate_positions(
) -> git::validation::Result { ) -> git::validation::Result {
let repo_details = &forge.repo_details; let repo_details = &forge.repo_details;
// Collect Commit Histories for `main`, `next` and `dev` branches // 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 { let commit_histories = match commit_histories {
Ok(commit_histories) => commit_histories, Ok(commit_histories) => commit_histories,
Err(err) => { Err(err) => {
@ -147,25 +148,34 @@ pub async fn validate_positions(
} }
async fn get_commit_histories( async fn get_commit_histories(
repository: &git::repository::OpenRepository,
repo_details: &git::RepoDetails, repo_details: &git::RepoDetails,
repo_config: &config::RepoConfig, repo_config: &config::RepoConfig,
net: &network::Network, net: &network::Network,
) -> Result<git::commit::Histories, network::NetworkError> { ) -> Result<git::commit::Histories, network::NetworkError> {
let main = let main = (get_commit_history(
(get_commit_history(repo_details, &repo_config.branches().main(), vec![], net).await)?; repository,
repo_details,
&repo_config.branches().main(),
&[],
net,
)
.await)?;
let main_head = main[0].clone(); let main_head = main[0].clone();
let next = (get_commit_history( let next = (get_commit_history(
repository,
repo_details, repo_details,
&repo_config.branches().next(), &repo_config.branches().next(),
vec![main_head.clone()], &[main_head.clone()],
net, net,
) )
.await)?; .await)?;
let next_head = next[0].clone(); let next_head = next[0].clone();
let dev = (get_commit_history( let dev = (get_commit_history(
repository,
repo_details, repo_details,
&repo_config.branches().dev(), &repo_config.branches().dev(),
vec![next_head, main_head], &[next_head, main_head],
net, net,
) )
.await)?; .await)?;
@ -179,11 +189,45 @@ async fn get_commit_histories(
Ok(histories) Ok(histories)
} }
#[tracing::instrument(fields(%branch_name),skip_all)]
async fn get_commit_history( async fn get_commit_history(
repository: &git::repository::OpenRepository,
repo_details: &git::RepoDetails, repo_details: &git::RepoDetails,
branch_name: &config::BranchName, branch_name: &config::BranchName,
find_commits: Vec<git::Commit>, find_commits: &[git::Commit],
net: &kxio::network::Network,
) -> Result<Vec<git::Commit>, 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, net: &kxio::network::Network,
) -> Result<Vec<git::Commit>, network::NetworkError> { ) -> Result<Vec<git::Commit>, network::NetworkError> {
let hostname = &repo_details.forge.hostname(); let hostname = &repo_details.forge.hostname();
@ -223,9 +267,8 @@ async fn get_commit_history(
let found = find_commits.is_empty() let found = find_commits.is_empty()
|| find_commits || find_commits
.clone() .iter()
.into_iter() .any(|find_commit| commits.iter().any(|commit| commit == find_commit));
.any(|find_commit| commits.iter().any(|commit| commit == &find_commit));
let at_end = commits.len() < limit; let at_end = commits.len() < limit;
all_commits.extend(commits); all_commits.extend(commits);
if found || at_end { if found || at_end {

View file

@ -20,7 +20,7 @@ mod branch {
let mut net = MockNetwork::new(); let mut net = MockNetwork::new();
//given //given
given_forgejo_has_branches(&mut net, 1); 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 net = Network::from(net);
let (repo, _reality) = git::repository::mock(); let (repo, _reality) = git::repository::mock();
@ -35,6 +35,47 @@ mod branch {
assert_eq!(branches, vec![config::BranchName::new("string")]); 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) { 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); 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( git::common::repo_details(
i, i,
git::Generation::new(), git::Generation::new(),

View file

@ -32,3 +32,14 @@ pub struct Histories {
pub next: Vec<Commit>, pub next: Vec<Commit>,
pub dev: Vec<Commit>, pub dev: Vec<Commit>,
} }
pub mod log {
use derive_more::{Display, From};
#[derive(Debug, Display, From)]
pub enum Error {
Gix(String),
Lock,
}
impl std::error::Error for Error {}
}

View file

@ -6,8 +6,8 @@ use std::{
sync::{Arc, Mutex}, sync::{Arc, Mutex},
}; };
use git_next_config::GitDir; use super::Error;
use crate as git;
use crate::{ use crate::{
repository::{ repository::{
open::{OpenRepository, OpenRepositoryLike}, open::{OpenRepository, OpenRepositoryLike},
@ -15,8 +15,7 @@ use crate::{
}, },
GitRemote, RepoDetails, GitRemote, RepoDetails,
}; };
use git_next_config::GitDir;
use super::Error;
#[derive(Debug, Default, Clone)] #[derive(Debug, Default, Clone)]
pub struct MockRepository(Arc<Mutex<Reality>>); pub struct MockRepository(Arc<Mutex<Reality>>);
@ -117,7 +116,7 @@ impl OpenRepositoryLike for MockOpenRepository {
.unwrap() .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() self.inner.lock().map(|inner| inner.fetch()).unwrap()
} }
@ -127,12 +126,23 @@ impl OpenRepositoryLike for MockOpenRepository {
branch_name: git_next_config::BranchName, branch_name: git_next_config::BranchName,
to_commit: crate::GitRef, to_commit: crate::GitRef,
force: crate::push::Force, force: crate::push::Force,
) -> std::prelude::v1::Result<(), crate::push::Error> { ) -> core::result::Result<(), crate::push::Error> {
self.inner self.inner
.lock() .lock()
.map(|inner| inner.push(repo_details, branch_name, to_commit, force)) .map(|inner| inner.push(repo_details, branch_name, to_commit, force))
.unwrap() .unwrap()
} }
fn commit_log(
&self,
branch_name: &git_next_config::BranchName,
find_commits: &[git::Commit],
) -> core::result::Result<Vec<crate::Commit>, git::commit::log::Error> {
self.inner
.lock()
.map(|inner| inner.commit_log(branch_name, find_commits))
.unwrap()
}
} }
impl OpenRepositoryLike for InnerMockOpenRepository { impl OpenRepositoryLike for InnerMockOpenRepository {
fn find_default_remote(&self, direction: Direction) -> Option<GitRemote> { fn find_default_remote(&self, direction: Direction) -> Option<GitRemote> {
@ -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!() todo!()
} }
@ -152,7 +162,15 @@ impl OpenRepositoryLike for InnerMockOpenRepository {
_branch_name: git_next_config::BranchName, _branch_name: git_next_config::BranchName,
_to_commit: crate::GitRef, _to_commit: crate::GitRef,
_force: crate::push::Force, _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<Vec<crate::Commit>, crate::commit::log::Error> {
todo!() todo!()
} }
} }

View file

@ -6,38 +6,44 @@ pub mod oreal;
use std::sync::{Arc, Mutex}; use std::sync::{Arc, Mutex};
use git_next_config::BranchName; use crate as git;
use git::repository::open::oreal::RealOpenRepository;
use crate::{ use git::repository::Direction;
fetch, push, use git_next_config as config;
repository::{mock::MockOpenRepository, open::oreal::RealOpenRepository, Direction},
GitRef, GitRemote, RepoDetails,
};
#[derive(Clone, Debug)] #[derive(Clone, Debug)]
pub enum OpenRepository { pub enum OpenRepository {
Real(oreal::RealOpenRepository), Real(RealOpenRepository),
Mock(MockOpenRepository), // TODO: (#38) contain a mock model of a repo Mock(git::repository::mock::MockOpenRepository), // TODO: (#38) contain a mock model of a repo
} }
impl OpenRepository { impl OpenRepository {
pub fn real(gix_repo: gix::Repository) -> Self { 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 #[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) Self::Mock(mock)
} }
} }
pub trait OpenRepositoryLike { pub trait OpenRepositoryLike {
fn find_default_remote(&self, direction: Direction) -> Option<GitRemote>; fn find_default_remote(&self, direction: Direction) -> Option<git::GitRemote>;
fn fetch(&self) -> Result<(), fetch::Error>; fn fetch(&self) -> Result<(), git::fetch::Error>;
fn push( fn push(
&self, &self,
repo_details: &RepoDetails, repo_details: &git::RepoDetails,
branch_name: BranchName, branch_name: config::BranchName,
to_commit: GitRef, to_commit: git::GitRef,
force: push::Force, force: git::push::Force,
) -> Result<(), push::Error>; ) -> 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<Vec<git::Commit>, git::commit::log::Error>;
} }
impl std::ops::Deref for OpenRepository { impl std::ops::Deref for OpenRepository {
type Target = dyn OpenRepositoryLike; type Target = dyn OpenRepositoryLike;

View file

@ -1,15 +1,16 @@
// //
use crate as git;
use git_next_config as config;
use gix::bstr::BStr;
use std::sync::{Arc, Mutex}; use std::sync::{Arc, Mutex};
use git_next_config::{BranchName, Hostname, RepoPath};
use tracing::{info, warn}; use tracing::{info, warn};
use crate::{fetch, push, repository::Direction, GitRef, GitRemote, RepoDetails};
#[derive(Clone, Debug, derive_more::Constructor)] #[derive(Clone, Debug, derive_more::Constructor)]
pub struct RealOpenRepository(Arc<Mutex<gix::Repository>>); pub struct RealOpenRepository(Arc<Mutex<gix::Repository>>);
impl super::OpenRepositoryLike for RealOpenRepository { impl super::OpenRepositoryLike for RealOpenRepository {
fn find_default_remote(&self, direction: Direction) -> Option<GitRemote> { fn find_default_remote(&self, direction: git::repository::Direction) -> Option<git::GitRemote> {
let Ok(repository) = self.0.lock() else { let Ok(repository) = self.0.lock() else {
#[cfg(not(tarpaulin_include))] // don't test mutex lock failure #[cfg(not(tarpaulin_include))] // don't test mutex lock failure
return None; return None;
@ -21,14 +22,16 @@ impl super::OpenRepositoryLike for RealOpenRepository {
remote.url(direction.into()).map(Into::into) 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 { let Ok(repository) = self.0.lock() else {
#[cfg(not(tarpaulin_include))] // don't test mutex lock failure #[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 #[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 let prepared_fetch = remote
.connect(gix::remote::Direction::Fetch)? .connect(gix::remote::Direction::Fetch)?
@ -37,9 +40,9 @@ impl super::OpenRepositoryLike for RealOpenRepository {
Ok(fetch) => { Ok(fetch) => {
fetch fetch
.receive(gix::progress::Discard, &Default::default()) .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(()) Ok(())
@ -49,15 +52,17 @@ impl super::OpenRepositoryLike for RealOpenRepository {
#[cfg(not(tarpaulin_include))] // would require writing to external service #[cfg(not(tarpaulin_include))] // would require writing to external service
fn push( fn push(
&self, &self,
repo_details: &RepoDetails, repo_details: &git::RepoDetails,
branch_name: BranchName, branch_name: config::BranchName,
to_commit: GitRef, to_commit: git::GitRef,
force: push::Force, force: git::push::Force,
) -> Result<(), push::Error> { ) -> Result<(), git::push::Error> {
let origin = repo_details.origin(); let origin = repo_details.origin();
let force = match force { let force = match force {
push::Force::No => "".to_string(), git::push::Force::No => "".to_string(),
push::Force::From(old_ref) => format!("--force-with-lease={branch_name}:{old_ref}"), 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' // INFO: never log the command as it contains the API token within the 'origin'
use secrecy::ExposeSecret; use secrecy::ExposeSecret;
@ -70,7 +75,7 @@ impl super::OpenRepositoryLike for RealOpenRepository {
let git_dir = self let git_dir = self
.0 .0
.lock() .lock()
.map_err(|_| push::Error::Lock) .map_err(|_| git::push::Error::Lock)
.map(|r| r.git_dir().to_path_buf())?; .map(|r| r.git_dir().to_path_buf())?;
let ctx = gix::diff::command::Context { let ctx = gix::diff::command::Context {
@ -91,23 +96,66 @@ impl super::OpenRepositoryLike for RealOpenRepository {
} }
Err(err) => { Err(err) => {
warn!(?err, ?git_dir, "Failed (wait)"); warn!(?err, ?git_dir, "Failed (wait)");
Err(push::Error::Push) Err(git::push::Error::Push)
} }
}, },
Err(err) => { Err(err) => {
warn!(?err, ?git_dir, "Failed (spawn)"); 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<Vec<crate::Commit>, 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 { fn from(url: &gix::Url) -> Self {
let host = url.host().unwrap_or_default(); let host = url.host().unwrap_or_default();
let path = url.path.to_string(); let path = url.path.to_string();
let path = path.strip_prefix('/').map_or(path.as_str(), |path| path); let path = path.strip_prefix('/').map_or(path.as_str(), |path| path);
let path = path.strip_suffix(".git").map_or(path, |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()),
)
} }
} }