refactor: add type to capture the "run or don't run" decision

The existing logic is faulty, and hard for me to follow. The new type
has tests, to hopefully making it harder for me to break things again.

Signed-off-by: Lars Wirzenius <liw@liw.fi>
Sponsored-by: author

2 files changed, 293 insertions, 42 deletions

diff --git a/ambient-driver.md b/ambient-driver.md
index d78c2cb..073ab9c 100644
--- a/ambient-driver.md
+++ b/ambient-driver.md
@@ -167,9 +167,9 @@ given an Ambient VM image ambient.qcow2
 given file multi.yaml
 given a directory foo
 given a directory bar
-when I run env AMBIENT_LOG=info ambient-driver run --dry-run multi.yaml foo
-then stderr contains "foo"
-then stderr doesn't contain "bar"
+when I run env AMBIENT_LOG=debug ambient-driver run --dry-run multi.yaml foo
+then stderr contains "project foo: NOT running CI"
+then stderr doesn't contain "project bar:"
 ~~~
 
 ~~~{#multi.yaml .file .yaml}
diff --git a/src/run.rs b/src/run.rs
index b3acb86..5426f18 100644
--- a/src/run.rs
+++ b/src/run.rs
@@ -45,7 +45,7 @@ fn cmd_run(config: &EffectiveConfig, run: &RunCommand) -> Result<(), RunError> {
         let (do_run, mut state) = should_run(run, statedir, name, project)?;
 
         if do_run {
-            info!("{}: running CI on {}", name, project.source().display());
+            info!("project {}: running CI", name);
 
             let artifactsdir = state.artifactsdir();
             recreate_dir(&artifactsdir)?;
@@ -131,6 +131,8 @@ fn cmd_run(config: &EffectiveConfig, run: &RunCommand) -> Result<(), RunError> {
                 state.set_latest_commot(None);
             };
             state.write_to_file()?;
+        } else {
+            info!("project {}: NOT running CI", name);
         }
     }
 
@@ -143,49 +145,60 @@ fn should_run(
     name: &str,
     project: &Project,
 ) -> Result<(bool, State), RunError> {
+    let mut decision = Decision::default();
+
     let state = State::from_file(statedir, name)?;
-    let head = if is_git(project.source()) {
-        Some(git_head(project.source())?)
+    if let Some(latest_commit) = state.latest_commit() {
+        debug!("latest commit: {:?}", latest_commit);
+        decision = decision.latest_commit(latest_commit);
     } else {
-        None
-    };
-    debug!("latest commit: {:?}", state.latest_commit());
-    debug!("HEAD commit:   {:?}", head);
-
-    let mut do_run = false;
-
-    if !is_git(project.source()) {
-        debug!("not a git repository: {}", project.source().display());
-        do_run = true;
-    } else if !git_is_clean(project.source()) {
-        debug!(
-            "git repository is not clean: {}",
-            project.source().display()
-        );
-        do_run = true;
-    } else if state.latest_commit() != head.as_deref() {
-        debug!("git repository has changed");
-        do_run = true;
-    }
-
-    if run.force() && run.dry_run() {
-        debug!("both --dry-run and --force used, --dry run wins");
-        do_run = false;
-    } else if run.force() {
-        debug!("--force used");
-        do_run = true;
-    }
-
-    if do_run {
-        if run.dry_run() {
-            info!("would run CI on {} but --dry-run used", name);
-        } else {
-            info!("running CI on {}", name);
-        }
+        debug!("no latest commit stored");
+    }
+
+    let is_git = is_git(project.source());
+    if is_git {
+        debug!("is a git repository");
+        decision = decision.is_git();
+    } else {
+        debug!("is not a git repository");
+        decision = decision.is_not_git();
+    }
+
+    if !git_is_clean(project.source()) {
+        debug!("git repository is clean");
+        decision = decision.is_clean();
+    } else {
+        debug!("git repository is dirty");
+        decision = decision.is_dirty();
+    }
+
+    if is_git {
+        let head = git_head(project.source())?;
+        debug!("current (HEAD) commit: {head}");
+        decision = decision.current_commit(&head);
+    } else {
+        debug!("no current commit due to not git repository");
+    }
+
+    if run.dry_run() {
+        debug!("dry run requested");
+        decision = decision.dry_run();
+    } else {
+        debug!("no dry run requested");
+        decision = decision.no_dry_run();
+    }
+
+    if run.force() {
+        debug!("forced run requested");
+        decision = decision.force();
     } else {
-        info!("skipping {}", name);
+        debug!("no forced run requested");
+        decision = decision.no_force();
     }
 
+    let do_run = decision.should_run() == ShouldRun::Run;
+    debug!("run? {do_run}");
+
     Ok((do_run, state))
 }
 
@@ -340,3 +353,241 @@ pub enum RunError {
     #[error("no state directory specified")]
     NoState,
 }
+
+#[derive(Debug, Default)]
+struct Decision {
+    dry_run: Option<bool>,
+    force_run: Option<bool>,
+    is_git: Option<bool>,
+    latest_commit: Option<String>,
+    current_commit: Option<String>,
+    source_is_dirty: Option<bool>,
+}
+
+impl Decision {
+    fn dry_run(mut self) -> Self {
+        self.dry_run = Some(true);
+        self
+    }
+
+    fn no_dry_run(mut self) -> Self {
+        self.dry_run = Some(false);
+        self
+    }
+
+    fn force(mut self) -> Self {
+        self.force_run = Some(true);
+        self
+    }
+
+    fn no_force(mut self) -> Self {
+        self.force_run = Some(false);
+        self
+    }
+
+    fn is_git(mut self) -> Self {
+        self.is_git = Some(true);
+        self
+    }
+
+    fn is_not_git(mut self) -> Self {
+        self.is_git = Some(false);
+        self
+    }
+
+    fn latest_commit(mut self, commit: &str) -> Self {
+        self.latest_commit = Some(commit.into());
+        self
+    }
+
+    fn current_commit(mut self, commit: &str) -> Self {
+        self.current_commit = Some(commit.into());
+        self
+    }
+
+    fn is_clean(mut self) -> Self {
+        self.source_is_dirty = Some(false);
+        self
+    }
+
+    fn is_dirty(mut self) -> Self {
+        self.source_is_dirty = Some(true);
+        self
+    }
+
+    fn should_run(&self) -> ShouldRun {
+        let dry_run = self.dry_run.unwrap();
+        let force = self.force_run.unwrap();
+        let is_git = self.is_git.unwrap();
+        let dirty = self.source_is_dirty.unwrap();
+
+        if dry_run {
+            Self::log("dry run");
+            ShouldRun::DontRun
+        } else if force {
+            Self::log("force");
+            ShouldRun::Run
+        } else if !is_git {
+            Self::log("not git");
+            ShouldRun::Run
+        } else if dirty {
+            Self::log("dirty");
+            ShouldRun::Run
+        } else if self.current_commit == self.latest_commit {
+            Self::log("commits are equal");
+            ShouldRun::DontRun
+        } else {
+            Self::log("nothing prevents run");
+            ShouldRun::Run
+        }
+    }
+
+    #[allow(unused_variables)]
+    fn log(msg: &str) {
+        #[cfg(test)]
+        println!("{}", msg);
+    }
+}
+
+// Use a custom enum to avoid confusing reader with interpreting
+// boolean values, specially in tests.
+#[derive(Debug, Eq, PartialEq, Copy, Clone)]
+enum ShouldRun {
+    Run,
+    DontRun,
+}
+
+#[cfg(test)]
+mod test_run_decision {
+    use super::{Decision, ShouldRun};
+
+    #[test]
+    fn is_not_git() {
+        let d = Decision::default()
+            .no_dry_run()
+            .no_force()
+            .is_not_git()
+            .is_clean();
+        assert_eq!(d.should_run(), ShouldRun::Run);
+    }
+
+    #[test]
+    fn unchanged() {
+        let d = Decision::default()
+            .no_dry_run()
+            .no_force()
+            .is_git()
+            .is_clean()
+            .latest_commit("abcd")
+            .current_commit("abcd");
+        assert_eq!(d.should_run(), ShouldRun::DontRun);
+    }
+
+    #[test]
+    fn unchanged_with_force() {
+        let d = Decision::default()
+            .no_dry_run()
+            .force()
+            .is_git()
+            .is_clean()
+            .latest_commit("abcd")
+            .current_commit("abcd");
+        assert_eq!(d.should_run(), ShouldRun::Run);
+    }
+
+    #[test]
+    fn unchanged_commit_but_dirty() {
+        let d = Decision::default()
+            .no_dry_run()
+            .no_force()
+            .is_git()
+            .is_dirty()
+            .latest_commit("abcd")
+            .current_commit("abcd");
+        assert_eq!(d.should_run(), ShouldRun::Run);
+    }
+
+    #[test]
+    fn commit_changed() {
+        let d = Decision::default()
+            .no_dry_run()
+            .no_force()
+            .is_git()
+            .is_clean()
+            .latest_commit("abcd")
+            .current_commit("efgh");
+        assert_eq!(d.should_run(), ShouldRun::Run);
+    }
+
+    #[test]
+    fn dry_run_for_unchanged() {
+        let d = Decision::default()
+            .dry_run()
+            .no_force()
+            .is_git()
+            .is_clean()
+            .latest_commit("abcd")
+            .current_commit("abcd");
+        assert_eq!(d.should_run(), ShouldRun::DontRun);
+    }
+
+    #[test]
+    fn dry_run_for_unchanged_but_dirty() {
+        let d = Decision::default()
+            .dry_run()
+            .no_force()
+            .is_git()
+            .is_dirty()
+            .latest_commit("abcd")
+            .current_commit("efgh");
+        assert_eq!(d.should_run(), ShouldRun::DontRun);
+    }
+
+    #[test]
+    fn dry_run_for_commit_changed() {
+        let d = Decision::default()
+            .dry_run()
+            .no_force()
+            .is_git()
+            .is_clean()
+            .latest_commit("abcd")
+            .current_commit("efgh");
+        assert_eq!(d.should_run(), ShouldRun::DontRun);
+    }
+
+    #[test]
+    fn dry_run_for_unchanged_with_force() {
+        let d = Decision::default()
+            .dry_run()
+            .no_force()
+            .is_git()
+            .is_clean()
+            .latest_commit("abcd")
+            .current_commit("abcd");
+        assert_eq!(d.should_run(), ShouldRun::DontRun);
+    }
+
+    #[test]
+    fn dry_run_for_unchanged_but_dirty_with_force() {
+        let d = Decision::default()
+            .dry_run()
+            .no_force()
+            .is_git()
+            .is_dirty()
+            .latest_commit("abcd")
+            .current_commit("efgh");
+        assert_eq!(d.should_run(), ShouldRun::DontRun);
+    }
+
+    #[test]
+    fn dry_run_for_commit_changed_with_force() {
+        let d = Decision::default()
+            .dry_run()
+            .no_force()
+            .is_git()
+            .is_clean()
+            .latest_commit("abcd")
+            .current_commit("efgh");
+        assert_eq!(d.should_run(), ShouldRun::DontRun);
+    }
+}