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

@@ -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<MessageToken> {
-    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<Commit> {
+pub fn find_next_commit_on_dev(
+    next: &Commit,
+    main: &Commit,
+    dev_commit_history: &[Commit],
+) -> (Option<Commit>, 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'

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<AdvanceNext> 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,

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<ValidateRepo> 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 {

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<Commit>): "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<Commit>,
+}
+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");

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,

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"
+    );
 }

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();
 

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(())
 }
 

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<git::Commit>,
+    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(

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]