From 3337a4e3fcf89257ecd1658671539d134eaef655 Mon Sep 17 00:00:00 2001 From: Brendan Jackman Date: Fri, 19 Dec 2025 10:58:35 +0000 Subject: [PATCH 1/4] feat: Add by_commit_with_notes cache policy --- src/git.rs | 39 +++++++++++++++++++---- src/test.rs | 92 +++++++++++++++++++++++++++++++++++++++++++++++++---- src/ui.rs | 1 + 3 files changed, 119 insertions(+), 13 deletions(-) diff --git a/src/git.rs b/src/git.rs index fcd9bfa..1f567ef 100644 --- a/src/git.rs +++ b/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:, 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, } 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 = 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) -> &mut GitCommand { + pub fn arg(&mut self, arg: impl AsRef) -> &mut GitCommand { self.command.arg(arg); self } - fn args(&mut self, args: impl IntoIterator>) -> &mut GitCommand { + pub fn args(&mut self, args: impl IntoIterator>) -> &mut GitCommand { self.command.args(args); self } - async fn execute(&mut self) -> anyhow::Result { + pub async fn execute(&mut self) -> anyhow::Result { 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", ¬es_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, })) } } diff --git a/src/test.rs b/src/test.rs index 4757638..fe0d658 100644 --- a/src/test.rs +++ b/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::, 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, pub test: Arc, + pub notes_hash: Option, } 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")] diff --git a/src/ui.rs b/src/ui.rs index 6153764..e8cf347 100644 --- a/src/ui.rs +++ b/src/ui.rs @@ -489,6 +489,7 @@ mod tests { commit_hash: commit_hash.clone(), cache_hash: Some(commit_hash.clone().into()), test: test.clone(), + notes_hash: None, }, status, } From 0b4dba778b6e437514e4cce5fe74fd4173db2c61 Mon Sep 17 00:00:00 2001 From: Brendan Jackman Date: Fri, 19 Dec 2025 11:09:07 +0000 Subject: [PATCH 2/4] test: Add test for LIMMAT_NOTES_OBJECT environment variable --- src/test.rs | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 113 insertions(+) diff --git a/src/test.rs b/src/test.rs index fe0d658..311b86b 100644 --- a/src/test.rs +++ b/src/test.rs @@ -2020,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(); From 1506735ac2df815846f22a68b999728ee6a7b078 Mon Sep 17 00:00:00 2001 From: Brendan Jackman Date: Fri, 19 Dec 2025 11:11:06 +0000 Subject: [PATCH 3/4] docs: Update README with by_commit_with_notes info --- README.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/README.md b/README.md index 85d1336..836ab92 100644 --- a/README.md +++ b/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. From 2bf33fc659305f8955f807d610d4649f73cec946 Mon Sep 17 00:00:00 2001 From: Brendan Jackman Date: Fri, 19 Dec 2025 11:12:07 +0000 Subject: [PATCH 4/4] chore: Update json schema for new cache policy --- limmat.schema.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/limmat.schema.json b/limmat.schema.json index 3bd8d33..ef69b1e 100644 --- a/limmat.schema.json +++ b/limmat.schema.json @@ -32,7 +32,8 @@ "enum": [ "no_caching", "by_commit", - "by_tree" + "by_tree", + "by_commit_with_notes" ] }, "Command": {