From 3642b2cdd11de2bf49c1214c9938a86517d6a7fd Mon Sep 17 00:00:00 2001 From: Paul Campbell Date: Sat, 25 May 2024 11:29:08 +0100 Subject: [PATCH] fix: new commit_log matches original from API request The original was including a lot of extra commits, those are now trimmed to match the expected. --- .../src/branch/validate_positions.rs | 25 +++++++++++-------- crates/git/src/repository/open/oreal.rs | 19 +++++++++----- crates/git/src/validation.rs | 12 +++++++++ 3 files changed, 39 insertions(+), 17 deletions(-) diff --git a/crates/forge-forgejo/src/branch/validate_positions.rs b/crates/forge-forgejo/src/branch/validate_positions.rs index bc3bc0c..3879483 100644 --- a/crates/forge-forgejo/src/branch/validate_positions.rs +++ b/crates/forge-forgejo/src/branch/validate_positions.rs @@ -14,6 +14,7 @@ pub async fn validate_positions( ) -> git::validation::Result { let repo_details = &forge.repo_details; // Collect Commit Histories for `main`, `next` and `dev` branches + repository.fetch()?; let commit_histories = get_commit_histories(repository, repo_details, &repo_config, &forge.net).await; let commit_histories = match commit_histories { @@ -23,7 +24,6 @@ pub async fn validate_positions( return Err(git::validation::Error::Network(Box::new(err))); } }; - // Validations let Some(main) = commit_histories.main.first().cloned() else { warn!( @@ -60,7 +60,6 @@ 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(), @@ -77,11 +76,12 @@ pub async fn validate_positions( repo_config.branches().next(), )); } - let next_commits = commit_histories .next .into_iter() - .take(2) + .take(2) // next should never be more than one commit ahead of main, so this should be + // either be next and main on two adjacent commits, or next and main on the same commit, + // plus the parent of main. .collect::>(); if !next_commits.contains(&main) { warn!( @@ -128,7 +128,6 @@ pub async fn validate_positions( repo_config.branches().next(), )); // dev is not based on next } - let Some(dev) = commit_histories.dev.first().cloned() else { warn!( "No commits on dev branch '{}'", @@ -138,7 +137,6 @@ pub async fn validate_positions( repo_config.branches().dev(), )); }; - Ok(git::validation::Positions { main, next, @@ -170,12 +168,11 @@ async fn get_commit_histories( net, ) .await)?; - let next_head = next[0].clone(); let dev = (get_commit_history( repository, repo_details, &repo_config.branches().dev(), - &[next_head, main_head], + &[main_head], net, ) .await)?; @@ -189,6 +186,7 @@ async fn get_commit_histories( Ok(histories) } +#[tracing::instrument(skip_all, fields(%branch_name, ?find_commits))] async fn get_commit_history( repository: &git::repository::OpenRepository, repo_details: &git::RepoDetails, @@ -204,7 +202,7 @@ async fn get_commit_history( info!("new version matches"); Ok(updated) } else { - error!(?updated, ?original, "new version doesn't match original"); + error!("new version doesn't match original"); Ok(original) } } @@ -269,8 +267,13 @@ async fn original_get_commit_history( || find_commits .iter() .any(|find_commit| commits.iter().any(|commit| commit == find_commit)); - let at_end = commits.len() < limit; - all_commits.extend(commits); + let at_end = commits.len() < limit; // i.e. less than the number of commits requested + for commit in commits { + all_commits.push(commit.clone()); + if find_commits.contains(&commit) { + break; + } + } if found || at_end { break; } diff --git a/crates/git/src/repository/open/oreal.rs b/crates/git/src/repository/open/oreal.rs index 61c5e8d..08f9102 100644 --- a/crates/git/src/repository/open/oreal.rs +++ b/crates/git/src/repository/open/oreal.rs @@ -22,6 +22,7 @@ impl super::OpenRepositoryLike for RealOpenRepository { remote.url(direction.into()).map(Into::into) } + #[tracing::instrument(skip_all)] fn fetch(&self) -> Result<(), git::fetch::Error> { let Ok(repository) = self.0.lock() else { #[cfg(not(tarpaulin_include))] // don't test mutex lock failure @@ -44,12 +45,13 @@ impl super::OpenRepositoryLike for RealOpenRepository { } Err(e) => Err(git::fetch::Error::Fetch(e.to_string()))?, } - + info!("Fetch okay"); Ok(()) } // TODO: (#72) reimplement using `gix` #[cfg(not(tarpaulin_include))] // would require writing to external service + #[tracing::instrument(skip_all)] fn push( &self, repo_details: &git::RepoDetails, @@ -71,13 +73,11 @@ impl super::OpenRepositoryLike for RealOpenRepository { origin.expose_secret() ) .into(); - let git_dir = self .0 .lock() .map_err(|_| git::push::Error::Lock) .map(|r| r.git_dir().to_path_buf())?; - let ctx = gix::diff::command::Context { git_dir: Some(git_dir.clone()), ..Default::default() @@ -91,7 +91,7 @@ impl super::OpenRepositoryLike for RealOpenRepository { { Ok(mut child) => match child.wait() { Ok(_) => { - info!("Branch updated"); + info!("Push okay"); Ok(()) } Err(err) => { @@ -111,11 +111,15 @@ impl super::OpenRepositoryLike for RealOpenRepository { branch_name: &config::BranchName, find_commits: &[git::Commit], ) -> Result, git::commit::log::Error> { + let limit = match find_commits.is_empty() { + true => 1, + false => 50, + }; self.0 .lock() .map_err(|_| git::commit::log::Error::Lock) .map(|repo| { - let branch_name = branch_name.to_string(); + let branch_name = format!("remotes/origin/{branch_name}"); let branch_name = BStr::new(&branch_name); let branch_head = repo .rev_parse_single(branch_name) @@ -127,7 +131,7 @@ impl super::OpenRepositoryLike for RealOpenRepository { .all() .map_err(|e| e.to_string())?; let mut commits = vec![]; - for item in walk { + for item in walk.take(limit) { 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(); @@ -136,12 +140,15 @@ impl super::OpenRepositoryLike for RealOpenRepository { git::commit::Sha::new(id), git::commit::Message::new(message), ); + info!(?commit, "found"); if find_commits.contains(&commit) { + info!("Is in find_commits"); commits.push(commit); break; } commits.push(commit); } + info!("finished walkfing"); Ok(commits) })? } diff --git a/crates/git/src/validation.rs b/crates/git/src/validation.rs index da7ea82..2b7a8ae 100644 --- a/crates/git/src/validation.rs +++ b/crates/git/src/validation.rs @@ -13,13 +13,25 @@ pub struct Positions { #[derive(Debug, derive_more::Display)] pub enum Error { Network(Box), + + Fetch(git::fetch::Error), + #[display("Failed to Reset Branch {branch} to {commit}")] FailedToResetBranch { branch: BranchName, commit: git::Commit, }, + BranchReset(BranchName), + BranchHasNoCommits(BranchName), + DevBranchNotBasedOn(BranchName), } impl std::error::Error for Error {} + +impl From for Error { + fn from(value: git::fetch::Error) -> Self { + Self::Fetch(value) + } +}