diff --git a/crates/cli/src/repo/branch.rs b/crates/cli/src/repo/branch.rs index 20fa057e..55b8ecd1 100644 --- a/crates/cli/src/repo/branch.rs +++ b/crates/cli/src/repo/branch.rs @@ -6,7 +6,7 @@ use git_next_core::{ commit::Message, push::{reset, Force}, repository::open::OpenRepositoryLike, - Commit, RepoDetails, + Commit, GitRef, RepoDetails, }, RepoConfig, }; @@ -18,14 +18,15 @@ use tracing::{info, instrument, warn}; #[instrument(fields(next), skip_all)] pub fn advance_next( next: &Commit, + main: &Commit, dev_commit_history: &[Commit], repo_details: RepoDetails, repo_config: RepoConfig, open_repository: &dyn OpenRepositoryLike, message_token: MessageToken, ) -> Result { - let commit = - find_next_commit_on_dev(next, dev_commit_history).ok_or_else(|| Error::NextAtDev)?; + let (commit, force) = find_next_commit_on_dev(next, main, dev_commit_history); + let commit = commit.ok_or_else(|| Error::NextAtDev)?; validate_commit_message(commit.message())?; info!("Advancing next to commit '{}'", commit); reset( @@ -33,7 +34,7 @@ pub fn advance_next( &repo_details, &repo_config.branches().next(), &commit.into(), - &Force::No, + &force, )?; Ok(message_token) } @@ -58,15 +59,24 @@ fn validate_commit_message(message: &Message) -> Result<()> { } } -pub fn find_next_commit_on_dev(next: &Commit, dev_commit_history: &[Commit]) -> Option { +pub fn find_next_commit_on_dev( + next: &Commit, + main: &Commit, + dev_commit_history: &[Commit], +) -> (Option, Force) { let mut next_commit: Option<&Commit> = None; + let mut force = Force::No; for commit in dev_commit_history.iter() { if commit == next { break; }; + if commit == main { + force = Force::From(GitRef::from(next.sha().clone())); + break; + }; next_commit.replace(commit); } - next_commit.cloned() + (next_commit.cloned(), force) } // advance main branch to the commit 'next' diff --git a/crates/cli/src/repo/handlers/advance_next.rs b/crates/cli/src/repo/handlers/advance_next.rs index c74c9170..cdcd2f99 100644 --- a/crates/cli/src/repo/handlers/advance_next.rs +++ b/crates/cli/src/repo/handlers/advance_next.rs @@ -6,7 +6,7 @@ use tracing::warn; use crate::repo::{ branch::advance_next, do_send, - messages::{AdvanceNext, ValidateRepo}, + messages::{AdvanceNext, AdvanceNextPayload, ValidateRepo}, RepoActor, }; @@ -20,13 +20,18 @@ impl Handler for RepoActor { let Some(open_repository) = &self.open_repository else { return; }; - let (next_commit, dev_commit_history) = msg.unwrap(); + let AdvanceNextPayload { + next, + main, + dev_commit_history, + } = msg.unwrap(); let repo_details = self.repo_details.clone(); let repo_config = repo_config.clone(); let addr = ctx.address(); match advance_next( - &next_commit, + &next, + &main, &dev_commit_history, repo_details, repo_config, diff --git a/crates/cli/src/repo/handlers/validate_repo.rs b/crates/cli/src/repo/handlers/validate_repo.rs index 1224fed6..0cb18a6e 100644 --- a/crates/cli/src/repo/handlers/validate_repo.rs +++ b/crates/cli/src/repo/handlers/validate_repo.rs @@ -6,7 +6,7 @@ use tracing::{debug, instrument, Instrument as _}; use crate::repo::{ do_send, logger, - messages::{AdvanceNext, CheckCIStatus, MessageToken, ValidateRepo}, + messages::{AdvanceNext, AdvanceNextPayload, CheckCIStatus, MessageToken, ValidateRepo}, notify_user, RepoActor, }; @@ -60,14 +60,19 @@ impl Handler for RepoActor { next, dev, dev_commit_history, + next_is_valid, }) => { debug!(%main, %next, %dev, "positions"); - if next != main { + if next_is_valid && next != main { do_send(ctx.address(), CheckCIStatus::new(next), self.log.as_ref()); } else if next != dev { do_send( ctx.address(), - AdvanceNext::new((next, dev_commit_history)), + AdvanceNext::new(AdvanceNextPayload { + next, + main, + dev_commit_history, + }), self.log.as_ref(), ) } else { diff --git a/crates/cli/src/repo/messages.rs b/crates/cli/src/repo/messages.rs index a1c551ad..63cb067d 100644 --- a/crates/cli/src/repo/messages.rs +++ b/crates/cli/src/repo/messages.rs @@ -60,7 +60,14 @@ Contains the commit from the tip of the `next` branch."#); // next commit message!(ReceiveCIStatus: (Commit, Status): r#"Notification of the status of the CI checks for the commit. Contains a tuple of the commit that was checked (the tip of the `next` branch) and the status."#); // commit and it's status -message!(AdvanceNext: (Commit, Vec): "Request to advance the `next` branch on to the next commit on the `dev branch."); // next commit and the dev commit history + +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct AdvanceNextPayload { + pub next: Commit, + pub main: Commit, + pub dev_commit_history: Vec, +} +message!(AdvanceNext: AdvanceNextPayload: "Request to advance the `next` branch on to the next commit on the `dev branch."); // next commit and the dev commit history message!(AdvanceMain: Commit: "Request to advance the `main` branch on to same commit as the `next` branch."); // next commit message!(WebhookNotification: ForgeNotification: "Notification of a webhook message from the forge."); message!(NotifyUser: UserNotification: "Request to send the message payload to the notification webhook"); diff --git a/crates/cli/src/repo/tests/branch/advance_next.rs b/crates/cli/src/repo/tests/branch/advance_next.rs index 8818fb29..32c443ca 100644 --- a/crates/cli/src/repo/tests/branch/advance_next.rs +++ b/crates/cli/src/repo/tests/branch/advance_next.rs @@ -7,6 +7,7 @@ mod when_at_dev { #[test] fn should_not_push() -> TestResult { let next = given::a_commit(); + let main = &next; let dev_commit_history = &[next.clone()]; let fs = given::a_filesystem(); let repo_config = given::a_repo_config(); @@ -16,6 +17,7 @@ mod when_at_dev { let_assert!( Err(err) = branch::advance_next( &next, + main, dev_commit_history, repo_details, repo_config, @@ -40,6 +42,7 @@ mod can_advance { #[test] fn should_not_push() -> TestResult { let next = given::a_commit(); + let main = &next; let dev = given::a_commit_with_message("wip: test: message".to_string()); let dev_commit_history = &[dev, next.clone()]; let fs = given::a_filesystem(); @@ -50,6 +53,7 @@ mod can_advance { let_assert!( Err(err) = branch::advance_next( &next, + main, dev_commit_history, repo_details, repo_config, @@ -70,6 +74,7 @@ mod can_advance { #[test] fn should_not_push_and_error() -> TestResult { let next = given::a_commit(); + let main = &next; let dev = given::a_commit(); let dev_commit_history = &[dev, next.clone()]; let fs = given::a_filesystem(); @@ -80,6 +85,7 @@ mod can_advance { let_assert!( Err(err) = branch::advance_next( &next, + main, dev_commit_history, repo_details, repo_config, @@ -108,6 +114,7 @@ mod can_advance { #[test] fn should_error() -> TestResult { let next = given::a_commit(); + let main = &next; let dev = given::a_commit_with_message("test: message".to_string()); let dev_commit_history = &[dev, next.clone()]; let fs = given::a_filesystem(); @@ -119,6 +126,7 @@ mod can_advance { let_assert!( Err(err) = branch::advance_next( &next, + main, dev_commit_history, repo_details, repo_config, @@ -139,6 +147,7 @@ mod can_advance { #[test] fn should_ok() -> TestResult { let next = given::a_commit(); + let main = &next; let dev = given::a_commit_with_message("test: message".to_string()); let dev_commit_history = &[dev, next.clone()]; let fs = given::a_filesystem(); @@ -150,6 +159,7 @@ mod can_advance { let_assert!( Ok(mt) = branch::advance_next( &next, + main, dev_commit_history, repo_details, repo_config, diff --git a/crates/cli/src/repo/tests/branch/mod.rs b/crates/cli/src/repo/tests/branch/mod.rs index 8634fc44..0341e7df 100644 --- a/crates/cli/src/repo/tests/branch/mod.rs +++ b/crates/cli/src/repo/tests/branch/mod.rs @@ -1,3 +1,6 @@ +use git_next_core::git::push::Force; +use git_next_core::git::GitRef; + // use super::*; @@ -8,8 +11,8 @@ use crate::git; use crate::repo::branch; #[actix_rt::test] -async fn test_find_next_commit_on_dev() { - let next = given::a_commit(); +async fn test_find_next_commit_on_dev_when_next_is_at_main() { + let next = given::a_commit(); // and main let expected = given::a_commit(); let dev_commit_history = vec![ given::a_commit(), // dev HEAD @@ -18,7 +21,30 @@ async fn test_find_next_commit_on_dev() { given::a_commit(), // parent of next ]; - let next_commit = branch::find_next_commit_on_dev(&next, &dev_commit_history); + let (next_commit, force) = branch::find_next_commit_on_dev(&next, &next, &dev_commit_history); assert_eq!(next_commit, Some(expected), "Found the wrong commit"); + assert_eq!(force, Force::No, "should not try to force"); +} + +#[actix_rt::test] +async fn test_find_next_commit_on_dev_when_next_is_not_on_dev() { + let next = given::a_commit(); + let main = given::a_commit(); + let expected = given::a_commit(); + let dev_commit_history = vec![ + given::a_commit(), // dev HEAD + expected.clone(), + main.clone(), // main - advancing towards dev HEAD + given::a_commit(), // parent of next + ]; + + let (next_commit, force) = branch::find_next_commit_on_dev(&next, &main, &dev_commit_history); + + assert_eq!(next_commit, Some(expected), "Found the wrong commit"); + assert_eq!( + force, + Force::From(GitRef::from(next.sha().clone())), + "should force back onto dev branch" + ); } diff --git a/crates/cli/src/repo/tests/handlers/advance_next.rs b/crates/cli/src/repo/tests/handlers/advance_next.rs index 2f2f7411..f068de3b 100644 --- a/crates/cli/src/repo/tests/handlers/advance_next.rs +++ b/crates/cli/src/repo/tests/handlers/advance_next.rs @@ -1,3 +1,5 @@ +use crate::repo::messages::AdvanceNextPayload; + // use super::*; @@ -7,7 +9,7 @@ async fn should_fetch_then_push_then_revalidate() -> TestResult { let fs = given::a_filesystem(); let (mut open_repository, repo_details) = given::an_open_repository(&fs); let next_commit = given::a_commit_with_message("feat: next".to_string()); - let dev_commit_log = vec![ + let dev_commit_history = vec![ given::a_commit_with_message("feat: dev".to_string()), given::a_commit_with_message("feat: target".to_string()), next_commit.clone(), @@ -31,10 +33,13 @@ async fn should_fetch_then_push_then_revalidate() -> TestResult { repo_details, given::a_forge(), ); - addr.send(crate::repo::messages::AdvanceNext::new(( - next_commit.clone(), - dev_commit_log, - ))) + addr.send(crate::repo::messages::AdvanceNext::new( + AdvanceNextPayload { + next: next_commit.clone(), + main: next_commit.clone(), + dev_commit_history, + }, + )) .await?; System::current().stop(); diff --git a/crates/cli/src/repo/tests/handlers/validate_repo.rs b/crates/cli/src/repo/tests/handlers/validate_repo.rs index ef25e914..8fce6184 100644 --- a/crates/cli/src/repo/tests/handlers/validate_repo.rs +++ b/crates/cli/src/repo/tests/handlers/validate_repo.rs @@ -1,8 +1,11 @@ +use crate::repo::messages::{AdvanceNext, AdvanceNextPayload}; + // use super::*; #[test_log::test(actix::test)] -async fn repo_with_next_not_an_ancestor_of_dev_should_be_reset() -> TestResult { +async fn repo_with_next_not_an_ancestor_of_dev_and_dev_on_main_should_be_reset_to_main( +) -> TestResult { //given let fs = given::a_filesystem(); let (mut open_repository, repo_details) = given::an_open_repository(&fs); @@ -15,7 +18,7 @@ async fn repo_with_next_not_an_ancestor_of_dev_should_be_reset() -> TestResult { // next - based on main let next_branch_log = vec![given::a_commit(), main_commit.clone()]; // dev - based on main, but not on next - let dev_branch_log = vec![given::a_commit(), main_commit.clone()]; + let dev_branch_log = vec![main_commit.clone()]; // commit_log next - based on main, but not a parent of dev open_repository .expect_commit_log() @@ -49,6 +52,62 @@ async fn repo_with_next_not_an_ancestor_of_dev_should_be_reset() -> TestResult { Ok(()) } +#[test_log::test(actix::test)] +async fn repo_with_next_not_an_ancestor_of_dev_and_dev_ahead_of_main_should_be_reset_to_dev( +) -> TestResult { + //given + let fs = given::a_filesystem(); + let (mut open_repository, repo_details) = given::an_open_repository(&fs); + #[allow(clippy::unwrap_used)] + let repo_config = repo_details.repo_config.clone().unwrap(); + expect::fetch_ok(&mut open_repository); + let branches = repo_config.branches(); + // commit_log main + let main_commit = expect::main_commit_log(&mut open_repository, branches.main()); + // next - based on main + let next_commit = given::a_commit(); + let next_branch_log = vec![next_commit.clone(), main_commit.clone()]; + // dev - based on main, but not on next + let dev_branch_log = vec![given::a_commit(), main_commit.clone()]; + // commit_log next - based on main, but not a parent of dev + open_repository + .expect_commit_log() + .times(1) + .with(eq(branches.next()), eq([main_commit.clone()])) + .return_once(move |_, _| Ok(next_branch_log)); + // commit_log dev + let dev_branch_log_clone = dev_branch_log.clone(); + open_repository + .expect_commit_log() + .times(1) + .with(eq(branches.dev()), eq([main_commit.clone()])) + .return_once(|_, _| Ok(dev_branch_log_clone)); + // expect to reset the branch + expect::fetch_ok(&mut open_repository); + expect::push_ok(&mut open_repository); + + //when + let (addr, log) = when::start_actor_with_open_repository( + Box::new(open_repository), + repo_details, + given::a_forge(), + ); + addr.send(crate::repo::messages::ValidateRepo::new( + MessageToken::default(), + )) + .await?; + System::current().stop(); + + //then + let expected = AdvanceNext::new(AdvanceNextPayload { + next: next_commit, + main: main_commit, + dev_commit_history: dev_branch_log, + }); + log.require_message_containing(format!("send: {expected:?}",))?; + Ok(()) +} + #[test_log::test(actix::test)] async fn repo_with_next_not_on_or_near_main_should_be_reset() -> TestResult { //given @@ -269,7 +328,7 @@ async fn repo_with_dev_ahead_of_next_should_advance_next() -> TestResult { open_repository .expect_commit_log() .times(1) - .with(eq(branches.dev()), eq([main_commit])) + .with(eq(branches.dev()), eq([main_commit.clone()])) .return_once(move |_, _| Ok(dev_commit_log)); //when @@ -285,9 +344,12 @@ async fn repo_with_dev_ahead_of_next_should_advance_next() -> TestResult { System::current().stop(); //then - log.require_message_containing(format!( - "send: AdvanceNext(({next_commit:?}, {dev_branch_log:?}))" - ))?; + let expected = AdvanceNext::new(AdvanceNextPayload { + next: next_commit, + main: main_commit, + dev_commit_history: dev_branch_log, + }); + log.require_message_containing(format!("send: {expected:?}"))?; Ok(()) } diff --git a/crates/core/src/git/validation/positions.rs b/crates/core/src/git/validation/positions.rs index 3ed8b9ec..390d5dbc 100644 --- a/crates/core/src/git/validation/positions.rs +++ b/crates/core/src/git/validation/positions.rs @@ -12,6 +12,7 @@ pub struct Positions { pub next: git::Commit, pub dev: git::Commit, pub dev_commit_history: Vec, + pub next_is_valid: bool, } pub fn validate_positions( @@ -65,16 +66,20 @@ pub fn validate_positions( tracing::info!("Main not on same commit as next, or it's parent - resetting next to main",); 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) { + // verify that next is an ancestor of dev, else reset it back to main if dev not ahead of main + if is_not_based_on(&commit_histories.dev, &next) + && commit_histories.main.first() == commit_histories.dev.first() + { tracing::info!("Next is not an ancestor of dev - resetting next to main"); return reset_next_to_main(open_repository, repo_details, &main, &next, &next_branch); } + let next_is_valid = is_based_on(&commit_histories.dev, &next); Ok(git::validation::positions::Positions { main, next, dev, dev_commit_history: commit_histories.dev, + next_is_valid, }) } @@ -103,7 +108,11 @@ fn reset_next_to_main( } fn is_not_based_on(commits: &[git::commit::Commit], needle: &git::Commit) -> bool { - commits.iter().filter(|commit| *commit == needle).count() == 0 + !is_based_on(commits, needle) +} + +fn is_based_on(commits: &[git::commit::Commit], needle: &git::Commit) -> bool { + commits.iter().any(|commit| commit == needle) } fn get_commit_histories( diff --git a/crates/core/src/git/validation/tests.rs b/crates/core/src/git/validation/tests.rs index 902324c5..92d024f0 100644 --- a/crates/core/src/git/validation/tests.rs +++ b/crates/core/src/git/validation/tests.rs @@ -410,9 +410,10 @@ mod positions { git::validation::positions::Error::NonRetryable(_) )); } - #[test] - fn where_dev_branch_is_not_based_on_next_should_reset_next_branch_to_main_and_then_error() { + #[allow(clippy::expect_used)] + fn where_dev_branch_is_on_main_and_next_is_not_should_reset_next_branch_to_main_and_retryable_error( + ) { //given let_assert!(Ok(fs) = kxio::fs::temp(), "temp fs"); let gitdir = GitDir::new(fs.base().to_path_buf(), StoragePathType::Internal); @@ -424,12 +425,11 @@ mod positions { fs.clone(), |branches, gitdir, fs| { // /--- 3 next - // 0 --- 1 main - // \--- 2 dev + // 0 --- 1 main & dev // add a commit to main (0 -> 1) then::create_a_commit_on_branch(fs, gitdir, &branches.main())?; - // add a commit to dev (1 -> 2) - then::create_a_commit_on_branch(fs, gitdir, &branches.dev())?; + // create dev branch on main (1) + then::git_switch(&branches.dev(), gitdir)?; // switch back to main (1) then::git_switch(&branches.main(), gitdir)?; // add a commit to next (1 -> 3) @@ -496,7 +496,9 @@ mod positions { } #[test] - fn where_dev_branch_is_not_based_on_next_and_reset_of_next_fails_should_error() { + #[allow(clippy::expect_used)] + fn where_dev_branch_is_not_based_on_next_should_reset_next_branch_to_next_commit_on_dev_and_retryable_error( + ) { //given let_assert!(Ok(fs) = kxio::fs::temp(), "temp fs"); let gitdir = GitDir::new(fs.base().to_path_buf(), StoragePathType::Internal); @@ -537,8 +539,25 @@ mod positions { repo_config.branches().clone(), gitdir.clone(), fs.clone(), - |_repo_details, _branch_name, _gitref, _force, _repo_branches, _gitdir, _fs| { - git::push::Result::Err(git::push::Error::Lock) + |_repo_details, branch_name, gitref, force, repo_branches, gitdir, fs| { + assert_eq!( + branch_name, + &repo_branches.next(), + "branch to reset should be 'next'" + ); + let sha_dev = then::get_sha_for_branch(fs, gitdir, &repo_branches.dev())?; + assert_eq!( + gitref, + &git::GitRef::from(sha_dev), + "should reset to the sha of the next commit on 'dev' branch" + ); + let sha_next = then::get_sha_for_branch(fs, gitdir, &repo_branches.next())?; + assert_eq!( + force, + &git::push::Force::From(sha_next.into()), + "should force push only if next is on expected sha" + ); + git::push::Result::Ok(()) }, )); let_assert!( @@ -549,21 +568,30 @@ mod positions { //when let_assert!( - Err(err) = + Ok(positions) = validate_positions(&*open_repository, &repo_details, repo_config.clone()), "validate" ); //then - println!("Got: {err:?}"); + println!("positions: {positions:#?}"); + let_assert!( - Ok(_) = then::get_sha_for_branch(&fs, &gitdir, &repo_config.branches().next()), - "load next branch sha" + Ok(main_sha) = + then::get_sha_for_branch(&fs, &gitdir, &repo_config.branches().main()) ); - assert!(matches!( - err, - git::validation::positions::Error::NonRetryable(_) - )); + assert_eq!(positions.main.sha(), &main_sha); + + let_assert!( + Ok(next_sha) = + then::get_sha_for_branch(&fs, &gitdir, &repo_config.branches().next()) + ); + assert_eq!(positions.next.sha(), &next_sha); + + let_assert!( + Ok(dev_sha) = then::get_sha_for_branch(&fs, &gitdir, &repo_config.branches().dev()) + ); + assert_eq!(positions.dev.sha(), &dev_sha); } #[test]