Cache notes v3

README.md

@@ -151,6 +151,13 @@ That means Limmat won't re-run tests unless the actual repository contents
 change - for example changes to the commit message won't invalidate cache
 results.
 
+There is also `cache = "by_commit_with_notes"`. This is like `by_commit` (the
+default), but it also incorporates the hash of the git note attached to the
+commit under `refs/notes/limmat`. This allows you to attach configuration to
+specific commits without modifying the commit itself (e.g. using `git notes --ref
+refs/notes/limmat add -m ...`). The hash of the note object is passed to the
+test script in the `LIMMAT_NOTES_OBJECT` environment variable.
+
 If the test is terminated by a signal, it isn't considered to have produced a
 result: instead of "success" or "failure" it's an "error". Errors aren't cached.
 

limmat.schema.json

@@ -32,7 +32,8 @@
       "enum": [
         "no_caching",
         "by_commit",
-        "by_tree"
+        "by_tree",
+        "by_commit_with_notes"
       ]
     },
     "Command": {

src/git.rs

@@ -172,6 +172,10 @@ impl Worktree for PersistentWorktree {
 pub struct Commit {
     pub hash: CommitHash,
     pub tree: TreeHash,
+    /// Hash of the blob at refs/notes/limmat:<hash>, if it exists.
+    /// This is not a standard part of a Commit but is used by Limmat to identify
+    /// the test configuration.
+    pub limmat_notes_hash: Option<Hash>,
 }
 
 impl Commit {
@@ -180,6 +184,7 @@ impl Commit {
         Self {
             hash: CommitHash::new("080b8ecbad3e34e55c5a035af80100f73b742a8d"),
             tree: TreeHash::new("6366d790125291272542a6b40f6fd3400e080821"),
+            limmat_notes_hash: None,
         }
     }
 }
@@ -204,23 +209,23 @@ static COMMAND_SEM: LazyLock<Semaphore> = LazyLock::new(|| Semaphore::new(64));
 // This exists to try and avoid running into file descriptor exhaustion, without
 // needing any retry logic that would risk creating livelocks.
 #[derive(Debug)]
-struct GitCommand {
+pub struct GitCommand {
     _permit: SemaphorePermit<'static>,
     command: Command,
 }
 
 impl GitCommand {
-    fn arg(&mut self, arg: impl AsRef<OsStr>) -> &mut GitCommand {
+    pub fn arg(&mut self, arg: impl AsRef<OsStr>) -> &mut GitCommand {
         self.command.arg(arg);
         self
     }
 
-    fn args(&mut self, args: impl IntoIterator<Item = impl AsRef<OsStr>>) -> &mut GitCommand {
+    pub fn args(&mut self, args: impl IntoIterator<Item = impl AsRef<OsStr>>) -> &mut GitCommand {
         self.command.args(args);
         self
     }
 
-    async fn execute(&mut self) -> anyhow::Result<process::Output> {
+    pub async fn execute(&mut self) -> anyhow::Result<process::Output> {
         self.command.execute().await
     }
 
@@ -233,7 +238,7 @@ impl GitCommand {
 // inheritance-brained idea to use this Worktree kinda like a superclass was not
 // a very good one.  This trait is a workaround for that, to avoid linter
 // warnings from having a public method return a private type.
-trait WorktreePriv: Worktree {
+pub trait WorktreePriv: Worktree {
     // Convenience function to create a git command with some pre-filled args.
     // Returns a BoxFuture as an utterly mysterious workaround for what I
     // believe is a compiler bug:
@@ -488,9 +493,29 @@ pub trait Worktree: Debug + Sync {
                 String::from_utf8_lossy(&output.stderr)
             );
         }
+        let hash = parts[0];
+        let tree = parts[1];
+
+        // Now fetch notes.
+        let notes_ref = format!("refs/notes/limmat:{}", hash);
+        let output = self
+            .git(["rev-parse", &notes_ref])
+            .await
+            .output()
+            .await
+            .context("failed to run 'git rev-parse' for notes")?;
+
+        // 128 means not found (ref doesn't exist).
+        let limmat_notes_hash = if output.status.success() {
+            Some(Hash::new(String::from_utf8(output.stdout)?.trim()))
+        } else {
+            None
+        };
+
         Ok(Some(Commit {
-            hash: CommitHash::new(parts[0]),
-            tree: TreeHash::new(parts[1]),
+            hash: CommitHash::new(hash),
+            tree: TreeHash::new(tree),
+            limmat_notes_hash,
         }))
     }
 }

src/test.rs

@@ -44,6 +44,7 @@ pub enum CachePolicy {
     NoCaching,
     ByCommit,
     ByTree,
+    ByCommitWithNotes,
 }
 
 impl CachePolicy {
@@ -54,6 +55,18 @@ impl CachePolicy {
             CachePolicy::NoCaching => None::<Hash>,
             CachePolicy::ByCommit => Some(commit.hash.clone().into()),
             CachePolicy::ByTree => Some(commit.tree.clone().into()),
+            CachePolicy::ByCommitWithNotes => {
+                let mut hasher = sha3::Sha3_256::new();
+                use sha3::Digest;
+                let h: &str = commit.hash.as_ref();
+                hasher.update(h.as_bytes());
+                if let Some(notes) = &commit.limmat_notes_hash {
+                    hasher.update(b"\0");
+                    let n: &str = notes.as_ref();
+                    hasher.update(n.as_bytes());
+                }
+                Some(Hash::new(hex::encode(hasher.finalize())))
+            }
         }
     }
 }
@@ -741,6 +754,9 @@ impl<'a> TestJob {
         dep_db_entries: &DepDatabaseEntries,
     ) {
         cmd.env("LIMMAT_COMMIT", &self.test_case.commit_hash);
+        if let Some(h) = &self.test_case.notes_hash {
+            cmd.env("LIMMAT_NOTES_OBJECT", h);
+        }
         cmd.env("LIMMAT_ARTIFACTS", artifacts_dir);
         for (k, v) in self.base_env.iter() {
             cmd.env(k, v);
@@ -865,8 +881,8 @@ impl<'a> TestJob {
 pub struct TestCaseId(String);
 
 impl TestCaseId {
-    fn new(commit_hash: &CommitHash, test_name: &TestName) -> Self {
-        Self(format!("{}:{}", commit_hash, test_name))
+    fn new(commit_hash: &CommitHash, test_name: &TestName, storage_hash: &Hash) -> Self {
+        Self(format!("{}:{}:{}", commit_hash, test_name, storage_hash))
     }
 }
 
@@ -879,6 +895,7 @@ pub struct TestCase {
     // enabled. Might be a tree hash, otherwise it matches the commit hash.
     pub cache_hash: Option<Hash>,
     pub test: Arc<Test>,
+    pub notes_hash: Option<Hash>,
 }
 
 impl Debug for TestCase {
@@ -898,6 +915,7 @@ impl TestCase {
             cache_hash: test.cache_policy.cache_hash(&commit),
             test,
             commit_hash: commit.hash,
+            notes_hash: commit.limmat_notes_hash,
         }
     }
 
@@ -911,8 +929,7 @@ impl TestCase {
     // TODO: this is always getting built on-demand all over the place, it
     // doesn't really need to be.
     fn id(&self) -> TestCaseId {
-        // The hash_cache is redundant information here so we don't need to include it.
-        TestCaseId::new(&self.commit_hash, &self.test.name)
+        TestCaseId::new(&self.commit_hash, &self.test.name, self.storage_hash())
     }
 }
 
@@ -927,7 +944,7 @@ impl GraphNode for TestCase {
         self.test
             .depends_on
             .iter()
-            .map(|test_name| TestCaseId::new(&self.commit_hash, test_name))
+            .map(|test_name| TestCaseId::new(&self.commit_hash, test_name, self.storage_hash()))
             .collect()
     }
 }
@@ -1120,7 +1137,7 @@ mod tests {
     use crate::{
         git::{
             test_utils::{TempRepo, WorktreeExt},
-            CommitHash, TempWorktree,
+            CommitHash, TempWorktree, WorktreePriv,
         },
         resource::Resource,
         test_utils::{path_exists, timeout_5s},
@@ -1843,6 +1860,69 @@ mod tests {
         assert_eq!(f.scripts[2].num_runs(&orig_commit.hash), 1);
     }
 
+    #[tokio::test]
+    async fn should_cache_results_with_notes() {
+        let f = TestScriptFixture::builder()
+            .cache_policies([CachePolicy::ByCommitWithNotes, CachePolicy::ByCommit])
+            .build()
+            .await;
+
+        // Initial commit
+        let commit = f
+            .repo
+            .commit("initial")
+            .await
+            .expect("couldn't create test commit");
+
+        // Add a note to the commit
+        f.repo
+            .git(["notes", "--ref=refs/notes/limmat", "add", "-m", "note1"])
+            .await
+            .arg(&commit.hash)
+            .execute()
+            .await
+            .expect("failed to add note");
+
+        // Run tests
+        f.manager.set_revisions(vec![commit.clone()]).await.unwrap();
+        f.manager.settled().await;
+        assert_eq!(f.scripts[0].num_runs(&commit.hash), 1);
+        assert_eq!(f.scripts[1].num_runs(&commit.hash), 1);
+
+        // Change the note (update to note2)
+        f.repo
+            .git([
+                "notes",
+                "--ref=refs/notes/limmat",
+                "add",
+                "-f",
+                "-m",
+                "note2",
+            ])
+            .await
+            .arg(&commit.hash)
+            .execute()
+            .await
+            .expect("failed to update note");
+
+        // Run tests again
+        f.manager.set_revisions(vec![commit.clone()]).await.unwrap();
+        f.manager.settled().await;
+
+        // ByCommitWithNotes should re-run because notes changed
+        assert_eq!(f.scripts[0].num_runs(&commit.hash), 2);
+        // ByCommit should NOT re-run because commit hash is same
+        assert_eq!(f.scripts[1].num_runs(&commit.hash), 1);
+
+        // Run again with same note
+        f.manager.set_revisions(vec![commit.clone()]).await.unwrap();
+        f.manager.settled().await;
+
+        // Neither should re-run
+        assert_eq!(f.scripts[0].num_runs(&commit.hash), 2);
+        assert_eq!(f.scripts[1].num_runs(&commit.hash), 1);
+    }
+
     #[test_case(1, 1 ; "single worktree, one test")]
     #[test_case(4, 1 ; "multiple worktrees, one test")]
     #[test_case(4, 4 ; "multiple worktrees, multiple tests")]
@@ -1940,6 +2020,119 @@ mod tests {
         }
     }
 
+    #[tokio::test]
+    async fn test_notes_env() {
+        let temp_dir = TempDir::new().unwrap();
+        let repo = Arc::new(TempRepo::new().await.unwrap());
+        let commit_with_note = repo
+            .commit("hello,")
+            .await
+            .expect("couldn't create test commit");
+
+        // Add a note to the commit
+        repo.git(["notes", "--ref=refs/notes/limmat", "add", "-m", "note1"])
+            .await
+            .arg(&commit_with_note.hash)
+            .execute()
+            .await
+            .expect("failed to add note");
+
+        let commit_without_note = repo
+            .commit("bye,")
+            .await
+            .expect("couldn't create test commit");
+
+        // Get the note hash to verify against
+        let note_hash_output = repo
+            .git(["rev-parse"])
+            .await
+            .arg(format!("refs/notes/limmat:{}", commit_with_note.hash))
+            .output()
+            .await
+            .expect("failed to get note hash");
+        let note_hash = String::from_utf8(note_hash_output.stdout)
+            .unwrap()
+            .trim()
+            .to_string();
+
+        let db_dir = TempDir::new().expect("couldn't make temp dir for result DB");
+
+        // Set up a test that dumps environment
+        let tests = Dag::new([Arc::new(
+            TestBuilder::new(
+                "env_test",
+                "bash",
+                [
+                    "-c".into(),
+                    OsString::from(format!(
+                        "env >> {0:?}/${{LIMMAT_COMMIT}}_env.txt",
+                        temp_dir.path()
+                    )),
+                ],
+            )
+            .needs_resources([(ResourceKey::Worktree, 1)])
+            .cache_policy(CachePolicy::ByCommitWithNotes)
+            .build(),
+        )])
+        .expect("couldn't build test DAG");
+
+        let resource_pools =
+            Pools::new([(ResourceKey::Worktree, worktree_resources(&repo, 1).await)].into_iter());
+
+        let m = Manager::new(
+            repo.clone(),
+            "/fake/config/path",
+            Arc::new(Database::create_or_open(db_dir.path()).expect("couldn't setup result DB")),
+            Arc::new(resource_pools),
+            tests,
+        );
+
+        m.set_revisions([commit_with_note.clone(), commit_without_note.clone()])
+            .await
+            .expect("set_revisions failed");
+        m.settled().await;
+
+        // Verify commit WITH note
+        let env_path_with = temp_dir
+            .path()
+            .join(format!("{}_env.txt", commit_with_note.hash));
+        let env_dump_with = fs::read_to_string(&env_path_with).unwrap_or_else(|_| {
+            panic!(
+                "couldn't read env dumped from test script at {}",
+                env_path_with.display()
+            )
+        });
+
+        let notes_env = env_dump_with
+            .lines()
+            .find(|l| l.starts_with("LIMMAT_NOTES_OBJECT="))
+            .expect("LIMMAT_NOTES_OBJECT not found in env for commit with note");
+
+        assert_eq!(
+            notes_env.split('=').nth(1).unwrap(),
+            note_hash,
+            "LIMMAT_NOTES_OBJECT didn't match expected hash"
+        );
+
+        // Verify commit WITHOUT note
+        let env_path_without = temp_dir
+            .path()
+            .join(format!("{}_env.txt", commit_without_note.hash));
+        let env_dump_without = fs::read_to_string(&env_path_without).unwrap_or_else(|_| {
+            panic!(
+                "couldn't read env dumped from test script at {}",
+                env_path_without.display()
+            )
+        });
+
+        assert!(
+            !env_dump_without
+                .lines()
+                .any(|l| l.starts_with("LIMMAT_NOTES_OBJECT=")),
+            "LIMMAT_NOTES_OBJECT should NOT be present for commit without note"
+        );
+    }
+
     #[tokio::test]
     async fn test_job_env() {
         let temp_dir = TempDir::new().unwrap();