From d2ea93f05ec81f7b9af4e2a347fc0b324eb3770f Mon Sep 17 00:00:00 2001 From: Paul Campbell Date: Sun, 28 Jul 2024 18:50:27 +0100 Subject: [PATCH] feat: avoid resetting next to main when dev is ahead of main When dev is not based on next, next is reset to main, however, it should reset to the next commit towards dev when when is ahead of main. Closes kemitix/git-next#111 --- crates/cli/src/repo/branch.rs | 22 ++++-- crates/cli/src/repo/handlers/advance_next.rs | 11 ++- crates/cli/src/repo/handlers/validate_repo.rs | 11 ++- crates/cli/src/repo/messages.rs | 9 ++- .../cli/src/repo/tests/branch/advance_next.rs | 10 +++ crates/cli/src/repo/tests/branch/mod.rs | 32 +++++++- .../src/repo/tests/handlers/advance_next.rs | 15 ++-- .../src/repo/tests/handlers/validate_repo.rs | 74 +++++++++++++++++-- crates/core/src/git/validation/positions.rs | 15 +++- crates/core/src/git/validation/tests.rs | 62 +++++++++++----- 10 files changed, 214 insertions(+), 47 deletions(-) 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]