refactor: remove the orphaned reviews and comments modules
commit
647ea2crefactor: remove the orphaned reviews and comments modules
Both were fully built and tested but never wired to a caller: no CLI command, no web endpoint, no template references either module. Reviews in particular has nothing to attach readiness to until a proposal or merge flow exists. Removing them now rather than leaving fully-tested dead code that looks load-bearing; either can come back alongside the feature that actually consumes it.
refactor: delete comments::* (refs/meta/comments/<issue_id>/<comment_id>) refactor: delete reviews::* (refs/meta/reviews/<target_id>/<principal>) Assisted-by: Claude:claude-sonnet-4-6
Reviews
No reviews of this commit yet — record a verdict below.
Start a review
crates/git-ents/src/lib.rs
@@ -2,11 +2,9 @@
pub mod account;
pub mod checks;
-pub mod comments;
pub mod config;
pub mod issues;
pub mod members;
-pub mod reviews;
pub mod revocations;
#[cfg(test)]
mod testutil;
crates/git-ents/src/comments.rs
@@ -1,170 +1,0 @@
-//! Comments on issues, sourced from the
-//! `refs/meta/comments/<issue_id>/<comment_id>` refs.
-//!
-//! Each comment is its own document nested under the issue's stable genesis
-//! key — never its friendly number, so a comment filed before promotion still
-//! resolves after. The comment's own id is the ref's last segment: the hash of
-//! its own content, content-addressed exactly like an issue's genesis key
-//! ([`git_ents::issues`](crate::issues)), so concurrent additions never
-//! collide and no counter is needed.
-
-use std::path::Path;
-
-use facet::Facet;
-
-/// The namespace under which comments are recorded: one ref,
-/// `refs/meta/comments/<issue_id>/<comment_id>`, per comment.
-pub const COMMENTS_NS: &str = "refs/meta/comments";
-
-/// One comment on an issue, stored at
-/// `refs/meta/comments/<issue_id>/<comment_id>`.
-#[derive(Debug, Clone, PartialEq, Eq, Facet)]
-pub struct Comment {
- /// The comment's body text.
- pub body: String,
- /// The identity that wrote the comment.
- pub author: String,
- /// The commented-on issue's stable genesis key — never its friendly
- /// number, so a comment filed before promotion still resolves after.
- pub issue_id: String,
-}
-
-/// The ref namespace holding every comment on `issue_id`.
-fn issue_comments_ns(issue_id: &str) -> String {
- format!("{COMMENTS_NS}/{issue_id}")
-}
-
-/// Derive a comment's stable id: the hash of its own content, since a comment
-/// derives from nothing upstream and so always originates itself — the same
-/// content-addressing [`issues::new_id`](crate::issues::new_id) uses when an
-/// issue has no origin either.
-pub fn new_id(content: &Comment) -> Result<String, git_store::Error> {
- git_store::content_hash(content)
-}
-
-/// Load the comment `comment_id` on `issue_id` in `repo`.
-pub fn load(
- repo: &Path,
- issue_id: &str,
- comment_id: &str,
-) -> Result<Option<Comment>, git_store::Error> {
- git_store::Store::open(repo)?.load_item(&issue_comments_ns(issue_id), comment_id)
-}
-
-/// Write `comment` at `refs/meta/comments/<issue_id>/<comment_id>` in `repo`,
-/// where `comment_id` is [`new_id`] of `comment`.
-pub fn store(
- repo: &Path,
- issue_id: &str,
- comment_id: &str,
- comment: &Comment,
-) -> Result<(), git_store::Error> {
- git_store::Store::open(repo)?.store_item(
- &issue_comments_ns(issue_id),
- comment_id,
- comment,
- "Add comment",
- )
-}
-
-/// List every comment on `issue_id` in `repo`, as `(comment_id, comment)`
-/// pairs, newest first.
-pub fn list(repo: &Path, issue_id: &str) -> Result<Vec<(String, Comment)>, git_store::Error> {
- git_store::Store::open(repo)?.list_items(&issue_comments_ns(issue_id))
-}
-
-#[cfg(test)]
-mod tests {
- #![allow(
- clippy::unwrap_used,
- clippy::let_underscore_must_use,
- reason = "unit test"
- )]
-
- use super::*;
- use crate::testutil::unique_repo as new_repo;
-
- fn unique_repo() -> std::path::PathBuf {
- new_repo("comments")
- }
-
- fn comment(issue_id: &str, body: &str) -> Comment {
- Comment {
- body: body.to_owned(),
- author: "alice".to_owned(),
- issue_id: issue_id.to_owned(),
- }
- }
-
- #[test]
- fn store_then_load_round_trips_a_comment() {
- let repo = unique_repo();
- let written = comment("issue-1", "A comment");
- let id = new_id(&written).unwrap();
- store(&repo, "issue-1", &id, &written).unwrap();
- assert_eq!(load(&repo, "issue-1", &id).unwrap(), Some(written));
- let _ = std::fs::remove_dir_all(&repo);
- }
-
- #[test]
- fn none_when_the_comment_is_absent() {
- let repo = unique_repo();
- assert_eq!(load(&repo, "issue-1", "missing").unwrap(), None);
- let _ = std::fs::remove_dir_all(&repo);
- }
-
- #[test]
- fn lists_only_the_comments_on_the_named_issue() {
- let repo = unique_repo();
- let a = comment("issue-1", "First");
- let b = comment("issue-1", "Second");
- let other = comment("issue-2", "Unrelated");
- store(&repo, "issue-1", &new_id(&a).unwrap(), &a).unwrap();
- store(&repo, "issue-1", &new_id(&b).unwrap(), &b).unwrap();
- store(&repo, "issue-2", &new_id(&other).unwrap(), &other).unwrap();
-
- let mut bodies: Vec<String> = list(&repo, "issue-1")
- .unwrap()
- .into_iter()
- .map(|(_id, comment)| comment.body)
- .collect();
- bodies.sort();
- assert_eq!(bodies, vec!["First".to_owned(), "Second".to_owned()]);
- let _ = std::fs::remove_dir_all(&repo);
- }
-
- #[test]
- fn new_id_is_content_addressed() {
- let a = comment("issue-1", "Same text");
- let b = comment("issue-1", "Same text");
- let different = comment("issue-1", "Different text");
- assert_eq!(new_id(&a).unwrap(), new_id(&b).unwrap());
- assert_ne!(new_id(&a).unwrap(), new_id(&different).unwrap());
- }
-
- #[test]
- fn a_comment_filed_before_promotion_still_resolves_after() {
- let repo = unique_repo();
- let issue = crate::issues::Issue {
- title: "A bug".to_owned(),
- body: "A body".to_owned(),
- state: "open".to_owned(),
- labels: vec![],
- author: "alice".to_owned(),
- id: None,
- };
- let issue_id = crate::issues::new_id(None, &issue).unwrap();
- crate::issues::store(&repo, &issue_id, &issue).unwrap();
-
- let written = comment(&issue_id, "Filed before promotion");
- let comment_id = new_id(&written).unwrap();
- store(&repo, &issue_id, &comment_id, &written).unwrap();
-
- crate::issues::promote(&repo, &issue_id).unwrap();
-
- // The comment is keyed off the stable genesis id, which promotion
- // never renames, so it still resolves.
- assert_eq!(load(&repo, &issue_id, &comment_id).unwrap(), Some(written));
- let _ = std::fs::remove_dir_all(&repo);
- }
-}
crates/git-ents/src/reviews.rs
@@ -1,200 +1,0 @@
-//! Reviews on a target (an issue, or eventually a proposal), sourced from the
-//! `refs/meta/reviews/<target_id>/<reviewer_principal>` refs.
-//!
-//! One ref per reviewer per target: a reviewer's second look replaces their
-//! own verdict on the same ref rather than appending a new one, while two
-//! reviewers' verdicts never collide. `target_id` is the target's stable
-//! genesis key ([`git_ents::issues::new_id`](crate::issues::new_id)), so a
-//! review survives promotion exactly like a comment does.
-//!
-//! Merge readiness is computed at read time by aggregating review verdicts
-//! with check runs — it is never stored, so there is no cached status to fall
-//! out of sync with the reviews and runs it summarizes.
-
-use std::path::Path;
-
-use facet::Facet;
-
-/// The namespace under which reviews are recorded: one ref,
-/// `refs/meta/reviews/<target_id>/<reviewer_principal>`, per reviewer per
-/// target.
-pub const REVIEWS_NS: &str = "refs/meta/reviews";
-
-/// A reviewer's verdict on a target.
-#[derive(Debug, Clone, PartialEq, Eq, Facet)]
-#[repr(u8)]
-pub enum Verdict {
- /// The reviewer approves the target as-is.
- Approve,
- /// The reviewer asks for changes before the target can land.
- RequestChanges,
- /// A non-blocking remark, neither an approval nor a change request.
- Comment,
-}
-
-/// One reviewer's review of a target, stored at
-/// `refs/meta/reviews/<target_id>/<reviewer_principal>`.
-#[derive(Debug, Clone, PartialEq, Eq, Facet)]
-pub struct Review {
- /// The reviewing member's principal — the ref's last segment.
- pub principal: String,
- /// The reviewer's verdict.
- pub verdict: Verdict,
- /// The reviewer's remarks.
- pub body: String,
-}
-
-/// The ref namespace holding every review of `target_id`.
-fn target_reviews_ns(target_id: &str) -> String {
- format!("{REVIEWS_NS}/{target_id}")
-}
-
-/// Load `principal`'s review of `target_id` in `repo`.
-pub fn load(
- repo: &Path,
- target_id: &str,
- principal: &str,
-) -> Result<Option<Review>, git_store::Error> {
- git_store::Store::open(repo)?.load_item(&target_reviews_ns(target_id), principal)
-}
-
-/// Write `review` at `refs/meta/reviews/<target_id>/<review.principal>` in
-/// `repo`, replacing that reviewer's prior verdict on `target_id` as a new
-/// commit.
-pub fn store(repo: &Path, target_id: &str, review: &Review) -> Result<(), git_store::Error> {
- git_store::Store::open(repo)?.store_item(
- &target_reviews_ns(target_id),
- &review.principal,
- review,
- "Add review",
- )
-}
-
-/// List every review of `target_id` in `repo`, as `(principal, review)` pairs,
-/// newest first.
-pub fn list(repo: &Path, target_id: &str) -> Result<Vec<(String, Review)>, git_store::Error> {
- git_store::Store::open(repo)?.list_items(&target_reviews_ns(target_id))
-}
-
-/// Whether `target_id` is ready to merge: aggregated at read time from its
-/// recorded reviews and check runs, never stored.
-///
-/// Ready requires at least one [`Verdict::Approve`], no
-/// [`Verdict::RequestChanges`], and every check in `runs`'s most recent run
-/// (if any) having passed. A target with no runs recorded is judged on
-/// reviews alone — checks that were never configured cannot block it.
-#[must_use]
-pub fn is_ready(reviews: &[Review], latest_run: Option<&crate::checks::Run>) -> bool {
- let approved = reviews.iter().any(|r| r.verdict == Verdict::Approve);
- let blocked = reviews.iter().any(|r| r.verdict == Verdict::RequestChanges);
- let checks_pass =
- latest_run.is_none_or(|run| run.results.iter().all(|outcome| outcome.outcome == "pass"));
- approved && !blocked && checks_pass
-}
-
-#[cfg(test)]
-mod tests {
- #![allow(
- clippy::unwrap_used,
- clippy::let_underscore_must_use,
- reason = "unit test"
- )]
-
- use super::*;
- use crate::checks::{Run, RunOutcome};
- use crate::testutil::unique_repo as new_repo;
-
- fn unique_repo() -> std::path::PathBuf {
- new_repo("reviews")
- }
-
- fn review(principal: &str, verdict: Verdict) -> Review {
- Review {
- principal: principal.to_owned(),
- verdict,
- body: "Looks good".to_owned(),
- }
- }
-
- #[test]
- fn store_then_load_round_trips_a_review() {
- let repo = unique_repo();
- let written = review("alice", Verdict::Approve);
- store(&repo, "target-1", &written).unwrap();
- assert_eq!(load(&repo, "target-1", "alice").unwrap(), Some(written));
- let _ = std::fs::remove_dir_all(&repo);
- }
-
- #[test]
- fn a_second_review_from_the_same_reviewer_replaces_the_first() {
- let repo = unique_repo();
- store(&repo, "target-1", &review("alice", Verdict::RequestChanges)).unwrap();
- store(&repo, "target-1", &review("alice", Verdict::Approve)).unwrap();
- assert_eq!(
- load(&repo, "target-1", "alice").unwrap(),
- Some(review("alice", Verdict::Approve))
- );
- let _ = std::fs::remove_dir_all(&repo);
- }
-
- #[test]
- fn lists_only_the_reviews_on_the_named_target() {
- let repo = unique_repo();
- store(&repo, "target-1", &review("alice", Verdict::Approve)).unwrap();
- store(&repo, "target-1", &review("bob", Verdict::Comment)).unwrap();
- store(&repo, "target-2", &review("carol", Verdict::Approve)).unwrap();
-
- let mut principals: Vec<String> = list(&repo, "target-1")
- .unwrap()
- .into_iter()
- .map(|(principal, _review)| principal)
- .collect();
- principals.sort();
- assert_eq!(principals, vec!["alice".to_owned(), "bob".to_owned()]);
- let _ = std::fs::remove_dir_all(&repo);
- }
-
- fn outcome(name: &str, outcome: &str) -> RunOutcome {
- RunOutcome {
- name: name.to_owned(),
- outcome: outcome.to_owned(),
- duration_secs: None,
- log_url: None,
- }
- }
-
- #[test]
- fn ready_requires_an_approval() {
- assert!(!is_ready(&[], None));
- assert!(is_ready(&[review("alice", Verdict::Approve)], None));
- }
-
- #[test]
- fn a_requested_change_blocks_readiness_even_with_an_approval() {
- let reviews = [
- review("alice", Verdict::Approve),
- review("bob", Verdict::RequestChanges),
- ];
- assert!(!is_ready(&reviews, None));
- }
-
- #[test]
- fn a_failing_check_blocks_readiness() {
- let reviews = [review("alice", Verdict::Approve)];
- let run = Run {
- at: 0,
- results: vec![outcome("fmt", "pass"), outcome("test", "fail")],
- };
- assert!(!is_ready(&reviews, Some(&run)));
- }
-
- #[test]
- fn passing_checks_and_an_approval_are_ready() {
- let reviews = [review("alice", Verdict::Approve)];
- let run = Run {
- at: 0,
- results: vec![outcome("fmt", "pass"), outcome("test", "pass")],
- };
- assert!(is_ready(&reviews, Some(&run)));
- }
-}