git ents: refactor: collapse checks/revocations onto Store::load

crates/git-ents-server/src/checks.rs @@ -29,7 +29,7 @@ use std::sync::Arc; use std::time::Duration; -use git_ents::checks::{self, Check, RunOutcome}; +use git_ents::checks::{self, Check, RunOutcome, Status}; use tokio::sync::Mutex; /// Where the pushed tree is unpacked inside the Sprite. @@ -77,7 +77,7 @@ // Record the run as `queued` straight away so it shows up on the Checks // tab the moment the push lands, before the worker picks it up; a // recording hiccup is reported but never fails the hook. - let queued = statuses(&runnable, "queued"); + let queued = statuses(&runnable, Status::Queued); if let Err(e) = checks::record(&repo, update.new, &queued) { eprintln!( "checks: could not record queued run for {}: {e}", @@ -95,12 +95,12 @@ /// Every check's [`RunOutcome`] set to one shared `status` — the queued/running /// snapshot a run starts from before per-check results land. -fn statuses(checks: &[Check], status: &str) -> Vec<RunOutcome> { +fn statuses(checks: &[Check], status: Status) -> Vec<RunOutcome> { checks .iter() .map(|check| RunOutcome { name: check.name.clone(), - outcome: status.to_owned(), + status, duration_secs: None, log_url: None, }) @@ -193,7 +193,7 @@ return Ok(()); } - let mut outcomes = statuses(&runnable, "running"); + let mut outcomes = statuses(&runnable, Status::Running); let sprite = sprite_name(&job.repo); if let Err(e) = ensure_auth().and_then(|()| ensure_sprite(&sprite)) { finalize_error(&job.repo, &job.new, &mut outcomes); @@ -214,7 +214,7 @@ for (index, check) in runnable.iter().enumerate() { let result = run_one(&sprite, check); if let Some(outcome) = outcomes.get_mut(index) { - outcome.outcome = result.to_owned(); + outcome.status = result; } advance(&job.repo, &job.new, &outcomes); } @@ -233,7 +233,7 @@ /// a run the worker could not carry out. fn finalize_error(repo: &Path, new: &str, outcomes: &mut [RunOutcome]) { for outcome in outcomes.iter_mut() { - outcome.outcome = "error".to_owned(); + outcome.status = Status::Error; } advance(repo, new, outcomes); } @@ -416,9 +416,9 @@ const CHECK_TIMEOUT: Duration = Duration::from_secs(30 * 60); /// Run one check in the Sprite's [`WORKDIR`], logging a `PASS`/`FAIL` line and -/// echoing the output on failure. Returns its outcome (`pass`/`fail`/`error`); a -/// check that exceeds [`CHECK_TIMEOUT`] or cannot be captured is `error`. -fn run_one(sprite: &str, check: &Check) -> &'static str { +/// echoing the output on failure. Returns its outcome; a check that exceeds +/// [`CHECK_TIMEOUT`] or cannot be captured is [`Status::Error`]. +fn run_one(sprite: &str, check: &Check) -> Status { let child = Command::new("sprite") .args([ "exec", @@ -438,7 +438,7 @@ Ok(child) => child, Err(e) => { eprintln!("checks: ERROR {} (could not run: {e})", check.name); - return "error"; + return Status::Error; } }; let Some(output) = wait_bounded(child, CHECK_TIMEOUT) else { @@ -446,11 +446,11 @@ "checks: ERROR {} (timed out after {:?} or could not be captured)", check.name, CHECK_TIMEOUT ); - return "error"; + return Status::Error; }; if output.status.success() { eprintln!("checks: PASS {}", check.name); - "pass" + Status::Pass } else { eprintln!("checks: FAIL {} ({})", check.name, check.command); let logs = String::from_utf8_lossy(&output.stderr); @@ -462,7 +462,7 @@ for line in logs.lines() { eprintln!("checks: {line}"); } - "fail" + Status::Fail } }

crates/git-ents/src/checks.rs @@ -2,7 +2,7 @@ //! //! A check is anything a server runs against a push — CI, CD, linting, //! versioning gates, and so on. Their definitions live in exactly one place: -//! the `refs/meta/checks` ref. Its tree is a [`Checks`] document mapping each +//! the `refs/meta/checks` ref, whose tree is a scalar-keyed map from each //! check name to the [`CheckBody`] that runs it. The document is read and //! written through [`git_store`], so the check set is a typed value that //! lives in git — versioned, auditable, and itself pushable. Keeping it on a @@ -11,15 +11,13 @@ //! //! # Migration note //! -//! `Checks`/`RunResults` moved their map values from a bare `String` to a -//! struct (`CheckBody`/[`Outcome`]) so a run's outcome can carry more than one -//! field (a duration, a log URL). This turns `checks/<name>` and -//! `results/<name>` from blobs into subtrees on disk, an incompatible format -//! change: data written in the prior flat-string layout no longer loads and -//! must be re-recorded. Acceptable pre-1.0 (see the format compatibility -//! rules in `git_store`'s module docs). +//! `checks/<name>` and `results/<name>` moved from bare blobs to subtrees +//! (`CheckBody`/[`Outcome`]) so a run's outcome can carry more than one field +//! (a duration, a log URL), and a run's [`Status`] moved from a bare string to +//! a closed enum. Each is an incompatible format change: data written in a +//! prior layout no longer loads and must be re-recorded. Acceptable pre-1.0 +//! (see the format compatibility rules in `git_store`'s module docs). -use std::collections::BTreeMap; use std::path::Path; use facet::Facet; @@ -35,13 +33,6 @@ command: String, } -/// The document stored at [`CHECKS_REF`]: its `checks/` subtree maps each -/// check name to its [`CheckBody`]. -#[derive(Debug, Clone, PartialEq, Eq, Facet)] -struct Checks { - checks: BTreeMap<String, CheckBody>, -} - /// One configured check, assembled from its map key and [`CheckBody`] at load. #[derive(Debug, Clone, PartialEq, Eq, Facet)] pub struct Check { @@ -51,45 +42,34 @@ pub command: String, } -/// Load the configured checks recorded at [`CHECKS_REF`] from an already-open -/// `store`. +/// Load the configured checks recorded at [`CHECKS_REF`] in `repo`. /// /// An absent ref yields an empty set, as on a server whose check set has not /// been pushed yet. A present but unreadable ref is an error so callers can /// distinguish corruption from "no checks configured". pub fn load(repo: &Path) -> Result<Vec<Check>, git_store::Error> { - Ok(git_store::Store::open(repo)? - .load::<Checks>(CHECKS_REF)? - .map(|doc| { - doc.checks - .into_iter() - .map(|(name, body)| Check { - name, - command: body.command, - }) - .collect() - }) - .unwrap_or_default()) + git_store::Store::open(repo)?.load_map(CHECKS_REF, |name, body: CheckBody| Check { + name, + command: body.command, + }) } /// Write `checks` to [`CHECKS_REF`] in `repo`, replacing any existing set as a /// new commit. pub fn store(repo: &Path, checks: &[Check]) -> Result<(), git_store::Error> { - let doc = Checks { - checks: checks - .iter() - .cloned() - .map(|check| { - ( - check.name, - CheckBody { - command: check.command, - }, - ) - }) - .collect(), - }; - git_store::Store::open(repo)?.store(CHECKS_REF, &doc, "Update checks") + git_store::Store::open(repo)?.store_map( + CHECKS_REF, + checks, + |check| { + ( + check.name.clone(), + CheckBody { + command: check.command.clone(), + }, + ) + }, + "Update checks", + ) } /// The namespace under which a commit's check runs are recorded: one ref, @@ -97,37 +77,58 @@ /// run against it. Definitions live on [`CHECKS_REF`]; this is their history. pub const RUNS_NS: &str = "refs/meta/runs"; +/// A check run's status, progressing `Queued` → `Running` → a terminal +/// outcome. Closed set — the only values a run legitimately takes, in place +/// of a `String` that every caller had to trust held one of five values. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Facet)] +#[repr(u8)] +pub enum Status { + /// Enqueued by `post-receive`, not yet picked up by the worker. + Queued, + /// The worker has started this run. + Running, + /// The check exited successfully. + Pass, + /// The check exited with a failure. + Fail, + /// An infrastructure failure (an unreachable sandbox, a timeout) kept the + /// check from completing. + Error, +} + +impl std::fmt::Display for Status { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(match self { + Self::Queued => "queued", + Self::Running => "running", + Self::Pass => "pass", + Self::Fail => "fail", + Self::Error => "error", + }) + } +} + /// One check's on-disk outcome. The map key (the check's name) is not /// duplicated inside it. Optional fields absent from an older record load as /// unset, so a run recorded before a field existed still loads. #[derive(Debug, Clone, PartialEq, Eq, Facet)] struct Outcome { /// `queued`, `running`, then `pass`, `fail`, or `error`. - outcome: String, + status: Status, /// How long the check took to run, when known. duration_secs: Option<u64>, /// Where to read the check's full log, when the runner published one. log_url: Option<String>, } -/// One run's outcomes, stored as the tree of a commit on the run ref: -/// `results/<name>` maps each check to its [`Outcome`]. Each commit on the ref -/// is one run and the commit's date is when it ran, so no timestamp is -/// duplicated in the tree — the run history is the ref's commit chain. -#[derive(Debug, Clone, PartialEq, Eq, Facet)] -struct RunResults { - results: BTreeMap<String, Outcome>, -} - /// One check's outcome within a [`Run`], assembled from its map key and /// [`Outcome`] at load. #[derive(Debug, Clone, PartialEq, Eq, Facet)] pub struct RunOutcome { /// The check's name (its `checks/<name>` in [`CHECKS_REF`]). pub name: String, - /// The outcome recorded for it as a run progresses — `queued`, `running`, - /// then `pass`, `fail`, or `error`. - pub outcome: String, + /// The outcome recorded for it as a run progresses. + pub status: Status, /// How long the check took to run, when known. pub duration_secs: Option<u64>, /// Where to read the check's full log, when the runner published one. @@ -158,9 +159,11 @@ /// `refs/meta/runs/<commit>`, parented on the prior run so the ref's commit /// chain is the run history. The commit's date is the run time. pub fn record(repo: &Path, commit: &str, outcomes: &[RunOutcome]) -> Result<(), git_store::Error> { - git_store::Store::open(repo)?.store( + let store = git_store::Store::open(repo)?; + store.store_map( &format!("{RUNS_NS}/{commit}"), - &run_doc(outcomes), + outcomes, + outcome_split, "Record check run", ) } @@ -178,11 +181,10 @@ commit: &str, outcomes: &[RunOutcome], ) -> Result<(), git_store::Error> { - git_store::Store::open(repo)?.amend( - &format!("{RUNS_NS}/{commit}"), - &run_doc(outcomes), - "Record check run", - ) + let refname = format!("{RUNS_NS}/{commit}"); + let doc: std::collections::BTreeMap<String, Outcome> = + outcomes.iter().map(outcome_split).collect(); + git_store::Store::open(repo)?.amend(&refname, &doc, "Record check run") } /// List the recorded runs per commit in `repo`, newest commit first. Each @@ -197,12 +199,11 @@ continue; }; let runs = store - .history::<RunResults>(&refname)? + .history::<std::collections::BTreeMap<String, Outcome>>(&refname)? .into_iter() .map(|(at, doc)| Run { at, results: doc - .results .into_iter() .map(|(name, outcome)| assemble_outcome(name, outcome)) .collect(), @@ -216,31 +217,23 @@ Ok(commits) } -/// Build a [`RunResults`] from a run's `outcomes`. -fn run_doc(outcomes: &[RunOutcome]) -> RunResults { - RunResults { - results: outcomes - .iter() - .cloned() - .map(|outcome| { - ( - outcome.name, - Outcome { - outcome: outcome.outcome, - duration_secs: outcome.duration_secs, - log_url: outcome.log_url, - }, - ) - }) - .collect(), - } +/// Split a public [`RunOutcome`] into its map key and on-disk [`Outcome`]. +fn outcome_split(outcome: &RunOutcome) -> (String, Outcome) { + ( + outcome.name.clone(), + Outcome { + status: outcome.status, + duration_secs: outcome.duration_secs, + log_url: outcome.log_url.clone(), + }, + ) } /// Assemble a public [`RunOutcome`] from its map key and on-disk [`Outcome`]. fn assemble_outcome(name: String, outcome: Outcome) -> RunOutcome { RunOutcome { name, - outcome: outcome.outcome, + status: outcome.status, duration_secs: outcome.duration_secs, log_url: outcome.log_url, } @@ -306,9 +299,9 @@ #[test] fn loads_the_on_disk_checks_format() { // A fixture written as the real `checks/<name>/command` subtree layout - // (the 2c migration: a struct value, not a bare blob) must keep - // loading, guarding the Checks document's shape against an - // incompatible change to data already on a ref. + // (a struct value, not a bare blob) must keep loading, guarding the + // checks document's shape against an incompatible change to data + // already on a ref. let repo = unique_repo(); write_checks_doc( &repo, @@ -328,15 +321,15 @@ #[test] fn loads_the_on_disk_runs_format() { - // A fixture written as the real `results/<name>/outcome` subtree - // layout (the 2c migration) with `duration_secs`/`log_url` omitted - // must keep loading, with the missing optional fields unset. + // A fixture written as the real `results/<name>/status/<Variant>` + // subtree layout, with `duration_secs`/`log_url` omitted, must keep + // loading, with the missing optional fields unset. let repo = unique_repo(); let commit = "0123456789012345678901234567890123456789"; write_runs_doc( &repo, &format!("{RUNS_NS}/{commit}"), - &[("fmt", "pass"), ("test", "fail")], + &[("fmt", "Pass"), ("test", "Fail")], ); let commits = runs(&repo).unwrap(); assert_eq!(commits.len(), 1); @@ -344,15 +337,15 @@ assert_eq!(commits[0].runs.len(), 1); assert_eq!( commits[0].runs[0].results, - vec![outcome("fmt", "pass"), outcome("test", "fail")] + vec![outcome("fmt", Status::Pass), outcome("test", Status::Fail)] ); let _ = std::fs::remove_dir_all(&repo); } - fn outcome(name: &str, outcome: &str) -> RunOutcome { + fn outcome(name: &str, status: Status) -> RunOutcome { RunOutcome { name: name.to_owned(), - outcome: outcome.to_owned(), + status, duration_secs: None, log_url: None, } @@ -365,7 +358,7 @@ record( &repo, commit, - &[outcome("fmt", "pass"), outcome("test", "fail")], + &[outcome("fmt", Status::Pass), outcome("test", Status::Fail)], ) .unwrap(); @@ -375,7 +368,7 @@ assert_eq!(commits[0].runs.len(), 1); assert_eq!( commits[0].runs[0].results, - vec![outcome("fmt", "pass"), outcome("test", "fail")] + vec![outcome("fmt", Status::Pass), outcome("test", Status::Fail)] ); let _ = std::fs::remove_dir_all(&repo); } @@ -384,14 +377,20 @@ fn recording_a_commit_again_appends_a_run() { let repo = unique_repo(); let commit = "0123456789012345678901234567890123456789"; - record(&repo, commit, &[outcome("fmt", "fail")]).unwrap(); - record(&repo, commit, &[outcome("fmt", "pass")]).unwrap(); + record(&repo, commit, &[outcome("fmt", Status::Fail)]).unwrap(); + record(&repo, commit, &[outcome("fmt", Status::Pass)]).unwrap(); let commits = runs(&repo).unwrap(); assert_eq!(commits.len(), 1); assert_eq!(commits[0].runs.len(), 2); // Newest first: the second run (pass) leads, the first (fail) follows. - assert_eq!(commits[0].runs[0].results, vec![outcome("fmt", "pass")]); - assert_eq!(commits[0].runs[1].results, vec![outcome("fmt", "fail")]); + assert_eq!( + commits[0].runs[0].results, + vec![outcome("fmt", Status::Pass)] + ); + assert_eq!( + commits[0].runs[1].results, + vec![outcome("fmt", Status::Fail)] + ); let _ = std::fs::remove_dir_all(&repo); } @@ -408,7 +407,7 @@ let commit = "0123456789012345678901234567890123456789"; let rich = RunOutcome { name: "fmt".to_owned(), - outcome: "pass".to_owned(), + status: Status::Pass, duration_secs: Some(12), log_url: Some("https://example.com/log".to_owned()), }; @@ -417,4 +416,26 @@ assert_eq!(commits[0].runs[0].results, vec![rich]); let _ = std::fs::remove_dir_all(&repo); } + + #[test] + fn update_run_advances_in_place_rather_than_appending() { + let repo = unique_repo(); + let commit = "0123456789012345678901234567890123456789"; + record(&repo, commit, &[outcome("fmt", Status::Queued)]).unwrap(); + update_run(&repo, commit, &[outcome("fmt", Status::Running)]).unwrap(); + update_run(&repo, commit, &[outcome("fmt", Status::Pass)]).unwrap(); + let commits = runs(&repo).unwrap(); + assert_eq!(commits[0].runs.len(), 1); + assert_eq!( + commits[0].runs[0].results, + vec![outcome("fmt", Status::Pass)] + ); + let _ = std::fs::remove_dir_all(&repo); + } + + #[test] + fn displays_lowercase_status_words() { + assert_eq!(Status::Queued.to_string(), "queued"); + assert_eq!(Status::Pass.to_string(), "pass"); + } }

crates/git-ents/src/issues.rs @@ -37,6 +37,17 @@ /// advances it, so filing an issue never contends it. pub const ISSUE_NUMBER_REF: &str = "refs/meta/issue-number"; +/// An issue's state — the closed set `Issue.state` legitimately takes, in +/// place of a `String` every caller had to trust held one of two values. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Facet)] +#[repr(u8)] +pub enum State { + /// The issue is being tracked. + Open, + /// The issue has been resolved or dismissed. + Closed, +} + /// One issue stored at `refs/meta/issues/<id>`. #[derive(Debug, Clone, PartialEq, Eq, Facet)] pub struct Issue { @@ -44,8 +55,8 @@ pub title: String, /// The issue's body text. pub body: String, - /// The issue's state — `open` or `closed`. - pub state: String, + /// The issue's state. + pub state: State, /// The labels applied to the issue, as plain strings. pub labels: Vec<String>, /// The identity that opened the issue. @@ -57,10 +68,10 @@ } impl Issue { - /// Whether the issue is open (any state other than `closed`). + /// Whether the issue is open (any state other than [`State::Closed`]). #[must_use] pub fn is_open(&self) -> bool { - self.state != "closed" + self.state != State::Closed } } @@ -179,11 +190,11 @@ new_repo("issues") } - fn issue(title: &str, state: &str, labels: &[&str]) -> Issue { + fn issue(title: &str, state: State, labels: &[&str]) -> Issue { Issue { title: title.to_owned(), body: "A body".to_owned(), - state: state.to_owned(), + state, labels: labels.iter().map(|l| (*l).to_owned()).collect(), author: "alice".to_owned(), id: None, @@ -193,7 +204,7 @@ #[test] fn store_then_load_round_trips_an_issue() { let repo = unique_repo(); - let written = issue("A bug", "open", &["bug", "p1"]); + let written = issue("A bug", State::Open, &["bug", "p1"]); store(&repo, "1", &written).unwrap(); assert_eq!(load(&repo, "1").unwrap(), Some(written)); let _ = std::fs::remove_dir_all(&repo); @@ -209,8 +220,8 @@ #[test] fn lists_issues_and_counts_the_open_ones() { let repo = unique_repo(); - store(&repo, "1", &issue("Open one", "open", &["bug"])).unwrap(); - store(&repo, "2", &issue("Closed one", "closed", &[])).unwrap(); + store(&repo, "1", &issue("Open one", State::Open, &["bug"])).unwrap(); + store(&repo, "2", &issue("Closed one", State::Closed, &[])).unwrap(); let mut ids: Vec<String> = list(&repo).unwrap().into_iter().map(|(id, _)| id).collect(); ids.sort(); assert_eq!(ids, vec!["1".to_owned(), "2".to_owned()]); @@ -221,36 +232,37 @@ #[test] fn loads_the_on_disk_issue_format() { // A fixture written as the real on-disk layout — `title`, `body`, - // `state`, `author` blobs plus an index-keyed `labels/` subtree — must - // keep loading, guarding the Issue document's shape against an - // incompatible change to data already on a ref. + // `author` blobs, a `state/<Variant>` subtree, and an index-keyed + // `labels/` subtree — must keep loading, guarding the Issue + // document's shape against an incompatible change to data already on + // a ref. let repo = unique_repo(); write_issue_doc( &repo, &format!("{ISSUES_NS}/1"), "A bug", "A body", - "open", + "Open", &["bug", "p1"], "alice", ); assert_eq!( load(&repo, "1").unwrap(), - Some(issue("A bug", "open", &["bug", "p1"])) + Some(issue("A bug", State::Open, &["bug", "p1"])) ); let _ = std::fs::remove_dir_all(&repo); } #[test] fn new_id_uses_the_origin_when_one_is_given() { - let content = issue("A bug", "open", &[]); + let content = issue("A bug", State::Open, &[]); assert_eq!(new_id(Some("deadbeef"), &content).unwrap(), "deadbeef"); } #[test] fn new_id_hashes_its_own_content_with_no_origin() { - let a = issue("A bug", "open", &[]); - let b = issue("A different bug", "open", &[]); + let a = issue("A bug", State::Open, &[]); + let b = issue("A different bug", State::Open, &[]); let a_id = new_id(None, &a).unwrap(); let b_id = new_id(None, &b).unwrap(); // Content-addressed: same content yields the same id, different @@ -262,7 +274,7 @@ #[test] fn filing_an_issue_leaves_its_friendly_number_unset() { let repo = unique_repo(); - let content = issue("A bug", "open", &[]); + let content = issue("A bug", State::Open, &[]); let id = new_id(None, &content).unwrap(); store(&repo, &id, &content).unwrap(); assert_eq!(load(&repo, &id).unwrap().unwrap().id, None); @@ -272,7 +284,7 @@ #[test] fn promotion_assigns_a_number_and_advances_the_counter_without_renaming_the_ref() { let repo = unique_repo(); - let content = issue("A bug", "open", &[]); + let content = issue("A bug", State::Open, &[]); let id = new_id(None, &content).unwrap(); store(&repo, &id, &content).unwrap(); @@ -283,7 +295,7 @@ // A second issue promotes to the next number; the first issue's ref // — keyed by its stable genesis hash — still resolves. - let other = issue("Another bug", "open", &[]); + let other = issue("Another bug", State::Open, &[]); let other_id = new_id(None, &other).unwrap(); store(&repo, &other_id, &other).unwrap(); assert_eq!(promote(&repo, &other_id).unwrap(), "2");

crates/git-ents/src/revocations.rs @@ -23,7 +23,7 @@ //! must be re-recorded. Acceptable pre-1.0 (see the format compatibility //! rules in `git_store`'s module docs). -use std::collections::{BTreeMap, BTreeSet}; +use std::collections::BTreeSet; use std::path::Path; use facet::Facet; @@ -40,13 +40,6 @@ reason: String, } -/// The revocation document stored at [`REVOKED_REF`]: its `revoked/` subtree -/// maps each revoked fingerprint to its [`RevocationBody`]. -#[derive(Debug, Clone, PartialEq, Eq, Facet)] -struct Revocations { - revoked: BTreeMap<String, RevocationBody>, -} - /// One revoked key, assembled from its map key and [`RevocationBody`] at load. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Revocation { @@ -59,47 +52,41 @@ /// Load the revocations recorded at [`REVOKED_REF`] in `repo`. An absent ref /// yields an empty list — nothing is revoked. pub fn load(repo: &Path) -> Result<Vec<Revocation>, git_store::Error> { - Ok(git_store::Store::open(repo)? - .load::<Revocations>(REVOKED_REF)? - .map(|doc| { - doc.revoked - .into_iter() - .map(|(fingerprint, body)| Revocation { - fingerprint, - reason: body.reason, - }) - .collect() - }) - .unwrap_or_default()) + git_store::Store::open(repo)?.load_map(REVOKED_REF, |fingerprint, body: RevocationBody| { + Revocation { + fingerprint, + reason: body.reason, + } + }) } /// Write `revocations` to [`REVOKED_REF`] in `repo`, replacing any existing /// list as a new commit. pub fn store(repo: &Path, revocations: &[Revocation]) -> Result<(), git_store::Error> { - let doc = Revocations { - revoked: revocations - .iter() - .cloned() - .map(|revocation| { - ( - revocation.fingerprint, - RevocationBody { - reason: revocation.reason, - }, - ) - }) - .collect(), - }; - git_store::Store::open(repo)?.store(REVOKED_REF, &doc, "Update revocations") + git_store::Store::open(repo)?.store_map( + REVOKED_REF, + revocations, + |revocation| { + ( + revocation.fingerprint.clone(), + RevocationBody { + reason: revocation.reason.clone(), + }, + ) + }, + "Update revocations", + ) } /// The set of revoked fingerprints recorded at [`REVOKED_REF`] from an /// already-open `store`, for the verifier to subtract from the trust set. pub fn fingerprints_with(store: &git_store::Store) -> Result<BTreeSet<String>, git_store::Error> { Ok(store - .load::<Revocations>(REVOKED_REF)? - .map(|doc| doc.revoked.into_keys().collect()) - .unwrap_or_default()) + .load_map(REVOKED_REF, |fingerprint, _body: RevocationBody| { + fingerprint + })? + .into_iter() + .collect()) } /// The set of revoked fingerprints recorded at [`REVOKED_REF`] in `repo`. See @@ -181,4 +168,12 @@ ); let _ = std::fs::remove_dir_all(&repo); } + + #[test] + fn store_rejects_a_fingerprint_that_is_not_a_safe_ref_segment() { + let repo = unique_repo(); + let result = store(&repo, &[revocation("aa/bb", "slash is not a safe segment")]); + assert!(matches!(result, Err(git_store::Error::InvalidKey(_)))); + let _ = std::fs::remove_dir_all(&repo); + } }

crates/git-ents/src/testutil.rs @@ -234,9 +234,12 @@ } /// Lay an `Issue` document out at `refname` as the real on-disk format: -/// `title`, `body`, `state`, and `author` blobs plus an index-keyed (`0000`, -/// `0001`, …) `labels/` subtree, committed and pointed to by the ref. Asserts -/// the loader still reads the format independent of the writer. +/// `title`, `body`, and `author` blobs, a `state/<Variant>` subtree (the +/// `State` enum's unit variant resolving to an empty tree, exactly like +/// `Member`'s `provenance`), and an index-keyed (`0000`, `0001`, …) `labels/` +/// subtree, committed and pointed to by the ref. `state` is the `State` +/// variant's name (`"Open"`, `"Closed"`). Asserts the loader still reads the +/// format independent of the writer. pub(crate) fn write_issue_doc( repo: &Path, refname: &str, @@ -249,8 +252,13 @@ let blob = |value: &str| git_with_stdin(repo, &["hash-object", "-w", "--stdin"], value); let title_blob = blob(title); let body_blob = blob(body); - let state_blob = blob(state); let author_blob = blob(author); + let empty_tree = git_with_stdin(repo, &["mktree"], ""); + let state_tree = git_with_stdin( + repo, + &["mktree"], + &format!("040000 tree {empty_tree}\t{state}\n"), + ); let mut label_entries = String::new(); for (index, label) in labels.iter().enumerate() { label_entries.push_str(&format!("100644 blob {}\t{index:04}\n", blob(label))); @@ -262,7 +270,7 @@ &format!( "100644 blob {title_blob}\ttitle\n\ 100644 blob {body_blob}\tbody\n\ - 100644 blob {state_blob}\tstate\n\ + 040000 tree {state_tree}\tstate\n\ 040000 tree {labels_tree}\tlabels\n\ 100644 blob {author_blob}\tauthor\n" ), @@ -277,11 +285,12 @@ assert!(status.success()); } -/// Lay a `Checks` document out at [`crate::checks::CHECKS_REF`] as the real -/// on-disk format after the 2c migration: a `checks/<name>/command` blob per -/// configured check (the map value is a `CheckBody` subtree, not a bare -/// blob). Asserts the loader still reads the format independent of the -/// writer. +/// Lay the checks document out at [`crate::checks::CHECKS_REF`] as the real +/// on-disk format: a bare scalar-keyed map at the ref's tree root, one +/// `<name>/command` blob per configured check (the map value is a `CheckBody` +/// subtree, not a bare blob, and the map itself is the whole document — no +/// wrapper struct). Asserts the loader still reads the format independent of +/// the writer. pub(crate) fn write_checks_doc(repo: &Path, checks: &[(&str, &str)]) { let mut entries = String::new(); for (name, command) in checks { @@ -294,12 +303,7 @@ entries.push_str(&format!("040000 tree {check_tree}\t{name}\n")); } let checks_tree = git_with_stdin(repo, &["mktree"], &entries); - let root = git_with_stdin( - repo, - &["mktree"], - &format!("040000 tree {checks_tree}\tchecks\n"), - ); - let commit = git_with_stdin(repo, &["commit-tree", &root, "-m", "fixture"], ""); + let commit = git_with_stdin(repo, &["commit-tree", &checks_tree, "-m", "fixture"], ""); let status = Command::new("git") .arg("-C") .arg(repo) @@ -309,29 +313,31 @@ assert!(status.success()); } -/// Lay a `RunResults` document out at `refname` as the real on-disk format -/// after the 2c migration: a `results/<name>/outcome` blob per outcome (the -/// map value is an `Outcome` subtree), with `duration_secs`/`log_url` omitted -/// entirely — asserting the loader fills a record's missing optional fields -/// as unset, independent of the writer. +/// Lay a `results/<name>` run-outcomes document out at `refname` as the real +/// on-disk format: a `results/<name>/status/<Variant>` subtree per outcome +/// (the `Status` enum's unit variant resolving to an empty tree, exactly like +/// `Member`'s `provenance`), with `duration_secs`/`log_url` omitted entirely — +/// asserting the loader fills a record's missing optional fields as unset, +/// independent of the writer. `variant` is the `Status` variant's name +/// (`"Pass"`, `"Fail"`, …). pub(crate) fn write_runs_doc(repo: &Path, refname: &str, outcomes: &[(&str, &str)]) { + let empty_tree = git_with_stdin(repo, &["mktree"], ""); let mut entries = String::new(); - for (name, outcome) in outcomes { - let outcome_blob = git_with_stdin(repo, &["hash-object", "-w", "--stdin"], outcome); + for (name, variant) in outcomes { + let variant_tree = git_with_stdin( + repo, + &["mktree"], + &format!("040000 tree {empty_tree}\t{variant}\n"), + ); let outcome_tree = git_with_stdin( repo, &["mktree"], - &format!("100644 blob {outcome_blob}\toutcome\n"), + &format!("040000 tree {variant_tree}\tstatus\n"), ); entries.push_str(&format!("040000 tree {outcome_tree}\t{name}\n")); } let results_tree = git_with_stdin(repo, &["mktree"], &entries); - let root = git_with_stdin( - repo, - &["mktree"], - &format!("040000 tree {results_tree}\tresults\n"), - ); - let commit = git_with_stdin(repo, &["commit-tree", &root, "-m", "fixture"], ""); + let commit = git_with_stdin(repo, &["commit-tree", &results_tree, "-m", "fixture"], ""); let status = Command::new("git") .arg("-C") .arg(repo) @@ -343,9 +349,10 @@ /// Lay a `Revocations` document out at /// [`crate::revocations::REVOKED_REF`] as the real on-disk format after the -/// revocations migration: a `revoked/<fingerprint>/reason` blob per entry -/// (the map value is a `RevocationBody` subtree, not a bare blob). Asserts -/// the loader still reads the format independent of the writer. +/// revocations migration, at the ref's tree root — a bare scalar-keyed map, +/// no wrapper struct: a `<fingerprint>/reason` blob per entry (the map value +/// is a `RevocationBody` subtree, not a bare blob). Asserts the loader still +/// reads the format independent of the writer. pub(crate) fn write_revocations_doc(repo: &Path, revoked: &[(&str, &str)]) { let mut entries = String::new(); for (fingerprint, reason) in revoked { @@ -358,12 +365,7 @@ entries.push_str(&format!("040000 tree {body_tree}\t{fingerprint}\n")); } let revoked_tree = git_with_stdin(repo, &["mktree"], &entries); - let root = git_with_stdin( - repo, - &["mktree"], - &format!("040000 tree {revoked_tree}\trevoked\n"), - ); - let commit = git_with_stdin(repo, &["commit-tree", &root, "-m", "fixture"], ""); + let commit = git_with_stdin(repo, &["commit-tree", &revoked_tree, "-m", "fixture"], ""); let status = Command::new("git") .arg("-C") .arg(repo)

crates/git-ents-server/src/web/render.rs @@ -151,7 +151,7 @@ fn run_summary(run: &Run) -> String { run.results .iter() - .map(|result| format!("{} {}", result.name, result.outcome)) + .map(|result| format!("{} {}", result.name, result.status)) .collect::<Vec<_>>() .join(" · ") }

refactor: collapse checks/revocations onto Store::load_map and close run/issue state to enums

Reviews

Start a review