git-ents.gitmain
⌘K
foforge

refactor: use gix::ObjectId for check-run commit ids

commit 3d9a2fd
refactor: use gix::ObjectId for check-run commit ids

CommitRuns.commit, and the commit parameters threaded through record/update_run/runs, checks_page, check_recording_page, and the post-receive/worker queue (Update.new, Job.new), were all bare hex Strings passed around unvalidated. None of these are Facet-persisted (check runs key off the ref name, job files are plain text), so they convert to gix::ObjectId directly with no on-disk format concern.

parse_updates and read_job now reject a malformed oid up front via ObjectId::from_hex instead of only checking for emptiness, and the zero-oid deletion check uses ObjectId::is_null instead of a string compare against git_ents::ZERO_OID.

Assisted-by: Claude:claude-sonnet-5

Joseph D. Carpinelli · 1 month ago
tree browse at HEAD · parents: ad82e48 · comment on this commit

Reviews

No reviews of this commit yet — record a verdict below.

Start a review

verdict

Cargo.lock @@ -1417,6 +1417,7 @@ "git-anchor", "git-comment", "git-store", + "gix", "iddqd", "inquire", "signal-hook 0.3.18",
crates/git-ents/Cargo.toml @@ -14,6 +14,7 @@ git-anchor = { workspace = true } git-comment = { workspace = true } git-store = { workspace = true } +gix = { workspace = true } iddqd = { workspace = true } inquire = { version = "0.9.4", default-features = false, features = ["crossterm"] } signal-hook = { version = "0.3", features = ["iterator"] }
crates/git-ents-server/src/checks.rs @@ -31,6 +31,7 @@ use std::time::{Duration, Instant}; use git_ents::checks::{self, Check, RunOutcome, Status}; +use gix_hash::ObjectId; use portable_pty::{CommandBuilder, PtySize, native_pty_system}; use tokio::sync::Mutex; @@ -198,7 +199,7 @@ 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); + finalize_error(&job.repo, job.new, &mut outcomes); return Err(e); } @@ -207,9 +208,9 @@ runnable.len(), job.ref_name ); - advance(&job.repo, &job.new, &outcomes); - if let Err(e) = sync_tree(&job.repo, &sprite, &job.new) { - finalize_error(&job.repo, &job.new, &mut outcomes); + advance(&job.repo, job.new, &outcomes); + if let Err(e) = sync_tree(&job.repo, &sprite, job.new) { + finalize_error(&job.repo, job.new, &mut outcomes); return Err(e); } @@ -220,14 +221,14 @@ outcome.duration_secs = Some(result.duration_secs); outcome.recording = Some(result.recording); } - advance(&job.repo, &job.new, &outcomes); + advance(&job.repo, job.new, &outcomes); } Ok(()) } /// Advance the recorded run for `new` to `outcomes`; a recording hiccup is /// logged but never derails the worker. -fn advance(repo: &Path, new: &str, outcomes: &[RunOutcome]) { +fn advance(repo: &Path, new: ObjectId, outcomes: &[RunOutcome]) { if let Err(e) = checks::update_run(repo, new, outcomes) { eprintln!("checks: could not record run for {new}: {e}"); } @@ -235,7 +236,7 @@ /// Mark every check in `outcomes` `error` and record it — the terminal state for /// a run the worker could not carry out. -fn finalize_error(repo: &Path, new: &str, outcomes: &mut [RunOutcome]) { +fn finalize_error(repo: &Path, new: ObjectId, outcomes: &mut [RunOutcome]) { for outcome in outcomes.iter_mut() { outcome.status = Status::Error; } @@ -246,7 +247,7 @@ /// it updated (carried only for logging). struct Job { repo: PathBuf, - new: String, + new: ObjectId, ref_name: String, } @@ -277,11 +278,8 @@ let contents = std::fs::read_to_string(path).ok()?; let mut lines = contents.lines(); let repo = PathBuf::from(lines.next()?); - let new = lines.next()?.to_owned(); + let new = ObjectId::from_hex(lines.next()?.as_bytes()).ok()?; let ref_name = lines.next()?.to_owned(); - if new.is_empty() { - return None; - } Some(Job { repo, new, @@ -291,7 +289,7 @@ /// One ref git reported as updated by the push. struct Update<'a> { - new: &'a str, + new: ObjectId, ref_name: &'a str, } @@ -307,7 +305,8 @@ let _old = fields.next()?; let new = fields.next()?; let ref_name = fields.next()?; - if new == git_ents::ZERO_OID || ref_name.starts_with("refs/meta/") { + let new = ObjectId::from_hex(new.as_bytes()).ok()?; + if new.is_null() || ref_name.starts_with("refs/meta/") { None } else { Some(Update { new, ref_name }) @@ -382,11 +381,11 @@ /// previous contents while leaving the rest of the persistent filesystem (build /// caches and the like) intact. `git archive` emits the tree as a tar that the /// Sprite unpacks over stdin. -fn sync_tree(repo: &Path, sprite: &str, new: &str) -> Result<(), String> { +fn sync_tree(repo: &Path, sprite: &str, new: ObjectId) -> Result<(), String> { let archive = Command::new("git") .arg("-C") .arg(repo) - .args(["archive", "--format=tar", new]) + .args(["archive", "--format=tar", &new.to_string()]) .output() .map_err(|e| format!("could not run git archive: {e}"))?; if !archive.status.success() { @@ -631,11 +630,18 @@ let write = |name: &str, body: &str| { std::fs::write(queue.path().join(name), body).unwrap(); }; - write("a.job", "/repos/one\naaa\nrefs/heads/main\n"); - write("b.job", "/repos/one\nbbb\nrefs/heads/dev\n"); - write("c.job", "/repos/two\nccc\nrefs/heads/main\n"); + let oid_a = "a".repeat(40); + let oid_b = "b".repeat(40); + let oid_c = "c".repeat(40); + let oid_d = "d".repeat(40); + write("a.job", &format!("/repos/one\n{oid_a}\nrefs/heads/main\n")); + write("b.job", &format!("/repos/one\n{oid_b}\nrefs/heads/dev\n")); + write("c.job", &format!("/repos/two\n{oid_c}\nrefs/heads/main\n")); write("d.job", "garbage\n"); - write("ignored.tmp", "/repos/one\nddd\nrefs/heads/main\n"); + write( + "ignored.tmp", + &format!("/repos/one\n{oid_d}\nrefs/heads/main\n"), + ); let groups = pending_jobs(queue.path()); assert_eq!(groups.len(), 2);
crates/git-ents/src/checks.rs @@ -21,6 +21,7 @@ use std::path::Path; use facet::Facet; +use gix::ObjectId; /// The ref whose tree holds the configured check set. pub const CHECKS_REF: &str = "refs/meta/checks"; @@ -152,7 +153,7 @@ #[derive(Debug, Clone, PartialEq, Eq)] pub struct CommitRuns { /// The checked commit's object id. - pub commit: String, + pub commit: ObjectId, /// Every run against it, newest first. pub runs: Vec<Run>, } @@ -160,7 +161,11 @@ /// Record a run of `outcomes` for `commit` in `repo`, as a new commit on /// `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> { +pub fn record( + repo: &Path, + commit: ObjectId, + outcomes: &[RunOutcome], +) -> Result<(), git_store::Error> { let store = git_store::Store::open(repo)?; store.store_map( &format!("{RUNS_NS}/{commit}"), @@ -180,7 +185,7 @@ /// advances a run is self-healing even if the `queued` record never landed. pub fn update_run( repo: &Path, - commit: &str, + commit: ObjectId, outcomes: &[RunOutcome], ) -> Result<(), git_store::Error> { let refname = format!("{RUNS_NS}/{commit}"); @@ -192,12 +197,20 @@ /// List the recorded runs per commit in `repo`, newest commit first. Each /// commit's runs are the ref's commit chain, newest first, with the run time /// taken from each commit's date. +/// +/// A ref whose last segment is not a valid hex object id cannot have been +/// written by [`record`]/[`update_run`], so it is skipped rather than +/// surfaced as an error — the same tolerance [`runs`] already gives a foreign +/// ref under [`RUNS_NS`]. pub fn runs(repo: &Path) -> Result<Vec<CommitRuns>, git_store::Error> { let store = git_store::Store::open(repo)?; let prefix = format!("{RUNS_NS}/"); let mut commits = Vec::new(); for refname in store.list(&prefix)? { - let Some(commit) = refname.strip_prefix(&prefix) else { + let Some(commit) = refname + .strip_prefix(&prefix) + .and_then(|hex| ObjectId::from_hex(hex.as_bytes()).ok()) + else { continue; }; let runs = store @@ -211,10 +224,7 @@ .collect(), }) .collect(); - commits.push(CommitRuns { - commit: commit.to_owned(), - runs, - }); + commits.push(CommitRuns { commit, runs }); } Ok(commits) } @@ -327,7 +337,7 @@ // subtree layout, with `duration_secs`/`recording` omitted, must keep // loading, with the missing optional fields unset. let repo = unique_repo(); - let commit = "0123456789012345678901234567890123456789"; + let commit = ObjectId::from_hex(b"0123456789012345678901234567890123456789").unwrap(); write_runs_doc( &repo, &format!("{RUNS_NS}/{commit}"), @@ -356,7 +366,7 @@ #[test] fn record_then_runs_round_trips_a_run() { let repo = unique_repo(); - let commit = "0123456789012345678901234567890123456789"; + let commit = ObjectId::from_hex(b"0123456789012345678901234567890123456789").unwrap(); record( &repo, commit, @@ -378,7 +388,7 @@ #[test] fn recording_a_commit_again_appends_a_run() { let repo = unique_repo(); - let commit = "0123456789012345678901234567890123456789"; + let commit = ObjectId::from_hex(b"0123456789012345678901234567890123456789").unwrap(); record(&repo, commit, &[outcome("fmt", Status::Fail)]).unwrap(); record(&repo, commit, &[outcome("fmt", Status::Pass)]).unwrap(); let commits = runs(&repo).unwrap(); @@ -406,7 +416,7 @@ #[test] fn round_trips_an_outcomes_duration_and_recording() { let repo = unique_repo(); - let commit = "0123456789012345678901234567890123456789"; + let commit = ObjectId::from_hex(b"0123456789012345678901234567890123456789").unwrap(); let rich = RunOutcome { name: "fmt".to_owned(), status: Status::Pass, @@ -422,7 +432,7 @@ #[test] fn update_run_advances_in_place_rather_than_appending() { let repo = unique_repo(); - let commit = "0123456789012345678901234567890123456789"; + let commit = ObjectId::from_hex(b"0123456789012345678901234567890123456789").unwrap(); 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();
crates/git-ents/src/main.rs @@ -369,7 +369,10 @@ .map(|outcome| format!("{}={}", outcome.name, outcome.status)) .collect::<Vec<_>>() .join(" "); - println!("{} {when} {results}", short_id(&commit_runs.commit)); + println!( + "{} {when} {results}", + short_id(&commit_runs.commit.to_string()) + ); } } Ok(())
crates/git-ents-server/src/web/pages.rs @@ -860,10 +860,13 @@ .await .map(|out| out.trim().to_owned()) .filter(|head| !head.is_empty()); - let head_run = head.as_deref().and_then(|head| { + let head_oid = head + .as_deref() + .and_then(|head| ObjectId::from_hex(head.as_bytes()).ok()); + let head_run = head_oid.and_then(|head_oid| { runs.as_ref() .ok() - .and_then(|commits| commits.iter().find(|commit| commit.commit == head)) + .and_then(|commits| commits.iter().find(|commit| commit.commit == head_oid)) .and_then(|commit| commit.runs.first()) }); repo_shell( @@ -908,7 +911,7 @@ @for commit in commits.iter().take(25) { @for run in &commit.runs { div.card-row.signer-row { - code.key { (commit.commit.get(..8).unwrap_or(&commit.commit)) } + code.key { (short_oid(&commit.commit)) } (run.render()) } } @@ -977,11 +980,14 @@ commit: &str, name: &str, ) -> Response { + let Some(commit_oid) = ObjectId::from_hex(commit.as_bytes()).ok() else { + return not_found().into_response(); + }; let runs = load_runs(repo).await; let recording = runs.ok().and_then(|commits| { commits .into_iter() - .find(|commit_runs| commit_runs.commit == commit) + .find(|commit_runs| commit_runs.commit == commit_oid) .and_then(|commit_runs| commit_runs.runs.into_iter().next()) .and_then(|run| run.results.into_iter().find(|result| result.name == name)) .and_then(|outcome| outcome.recording)