e353a987
Fix 3 cleanup issues: O(1) max_clock, ReviewVerdict Display, dedup refname_to_id
a73x 2026-03-22 09:49
- max_clock reads only tip commit instead of walking entire DAG (c673d28f) - Add as_str/Display/FromStr for ReviewVerdict, remove 3 duplicated string tables (c342e36b) - Remove redundant second refname_to_id call in dag::append_event (e9ce0591) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
diff --git a/src/dag.rs b/src/dag.rs index 179f370..57f2df7 100644 --- a/src/dag.rs +++ b/src/dag.rs @@ -29,43 +29,33 @@ fn build_event_tree( Ok(tree_oid) } /// Walk the entire DAG reachable from `tip` and return the maximum clock value. /// Returns 0 if the DAG is empty or all events have clock 0 (pre-migration). /// Read the clock value from the tip commit of a DAG. /// Since clocks are monotonically increasing, the tip always has the max clock. /// Returns 0 if the event has clock 0 (pre-migration). pub fn max_clock(repo: &Repository, tip: Oid) -> Result<u64, Error> { let mut revwalk = repo.revwalk()?; revwalk.set_sorting(Sort::TOPOLOGICAL)?; revwalk.push(tip)?; let mut max = 0u64; for oid_result in revwalk { let oid = oid_result?; let commit = repo.find_commit(oid)?; let tree = commit.tree()?; let entry = tree .get_name("event.json") .ok_or_else(|| git2::Error::from_str("missing event.json in commit tree"))?; if entry.kind() != Some(ObjectType::Blob) { return Err(Error::Git(git2::Error::from_str( "event.json entry is not a blob", ))); } let blob = repo.find_blob(entry.id())?; let content = blob.content(); if content.len() > MAX_EVENT_BLOB_SIZE { return Err(Error::PayloadTooLarge { actual: content.len(), limit: MAX_EVENT_BLOB_SIZE, }); } let commit = repo.find_commit(tip)?; let tree = commit.tree()?; let entry = tree .get_name("event.json") .ok_or_else(|| git2::Error::from_str("missing event.json in commit tree"))?; if entry.kind() != Some(ObjectType::Blob) { return Err(Error::Git(git2::Error::from_str( "event.json entry is not a blob", ))); } let event: Event = serde_json::from_slice(content)?; if event.clock > max { max = event.clock; } let blob = repo.find_blob(entry.id())?; let content = blob.content(); if content.len() > MAX_EVENT_BLOB_SIZE { return Err(Error::PayloadTooLarge { actual: content.len(), limit: MAX_EVENT_BLOB_SIZE, }); } Ok(max) let event: Event = serde_json::from_slice(content)?; Ok(event.clock) } /// Create an orphan commit (no parents) with the given event. @@ -125,8 +115,7 @@ pub fn append_event( let sig = author_signature(&event.author)?; let message = commit_message(&event.action); let parent_oid = repo.refname_to_id(ref_name)?; let parent = repo.find_commit(parent_oid)?; let parent = repo.find_commit(tip)?; let oid = repo.commit(Some(ref_name), &sig, &sig, &message, &tree, &[&parent])?; Ok(oid) @@ -367,7 +356,7 @@ fn commit_message(action: &Action) -> String { Action::IssueReopen => "issue: reopen".to_string(), Action::PatchCreate { title, .. } => format!("patch: create \"{}\"", title), Action::PatchRevision { .. } => "patch: revision".to_string(), Action::PatchReview { verdict, .. } => format!("patch: review ({:?})", verdict), Action::PatchReview { verdict, .. } => format!("patch: review ({})", verdict), Action::PatchComment { .. } => "patch: comment".to_string(), Action::PatchInlineComment { ref file, line, .. } => { format!("patch: inline comment on {}:{}", file, line) diff --git a/src/event.rs b/src/event.rs index 97083f9..061c6b5 100644 --- a/src/event.rs +++ b/src/event.rs @@ -112,3 +112,34 @@ pub enum ReviewVerdict { Comment, Reject, } impl ReviewVerdict { pub fn as_str(&self) -> &'static str { match self { ReviewVerdict::Approve => "approve", ReviewVerdict::RequestChanges => "request-changes", ReviewVerdict::Comment => "comment", ReviewVerdict::Reject => "reject", } } } impl std::fmt::Display for ReviewVerdict { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.write_str(self.as_str()) } } impl std::str::FromStr for ReviewVerdict { type Err = String; fn from_str(s: &str) -> Result<Self, Self::Err> { match s { "approve" => Ok(ReviewVerdict::Approve), "request-changes" => Ok(ReviewVerdict::RequestChanges), "comment" => Ok(ReviewVerdict::Comment), "reject" => Ok(ReviewVerdict::Reject), other => Err(format!("unknown verdict: {}", other)), } } } diff --git a/src/lib.rs b/src/lib.rs index 2922efe..8cec6a6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -272,7 +272,7 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> { for r in &reviews { let rev_label = r.revision.map(|n| format!(" (r{})", n)).unwrap_or_default(); println!( "\n{} ({:?}) - {}{}:\n{}", "\n{} ({}) - {}{}:\n{}", r.author.name, r.verdict, r.timestamp, rev_label, r.body ); } @@ -333,18 +333,11 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> { Ok(()) } PatchCmd::Review { id, verdict, body, revision } => { let v = match verdict.as_str() { "approve" => ReviewVerdict::Approve, "request-changes" => ReviewVerdict::RequestChanges, "comment" => ReviewVerdict::Comment, "reject" => ReviewVerdict::Reject, _ => { return Err(git2::Error::from_str( "verdict must be: approve, request-changes, comment, or reject", ) .into()); } }; let v: ReviewVerdict = verdict.parse().map_err(|_| { git2::Error::from_str( "verdict must be: approve, request-changes, comment, or reject", ) })?; patch::review(repo, &id, v, &body, revision)?; println!("Review submitted."); Ok(()) diff --git a/src/state.rs b/src/state.rs index baa0787..a08289a 100644 --- a/src/state.rs +++ b/src/state.rs @@ -32,27 +32,12 @@ fn deserialize_oid_option<'de, D: serde::Deserializer<'de>>(d: D) -> Result<Opti } fn serialize_verdict<S: serde::Serializer>(v: &ReviewVerdict, s: S) -> Result<S::Ok, S::Error> { let str_val = match v { ReviewVerdict::Approve => "approve", ReviewVerdict::RequestChanges => "request-changes", ReviewVerdict::Comment => "comment", ReviewVerdict::Reject => "reject", }; s.serialize_str(str_val) s.serialize_str(v.as_str()) } fn deserialize_verdict<'de, D: serde::Deserializer<'de>>(d: D) -> Result<ReviewVerdict, D::Error> { let s = String::deserialize(d)?; match s.as_str() { "approve" => Ok(ReviewVerdict::Approve), "request-changes" => Ok(ReviewVerdict::RequestChanges), "comment" => Ok(ReviewVerdict::Comment), "reject" => Ok(ReviewVerdict::Reject), other => Err(serde::de::Error::custom(format!( "unknown verdict: {}", other ))), } s.parse().map_err(serde::de::Error::custom) } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] diff --git a/src/tui/widgets.rs b/src/tui/widgets.rs index 4bf517f..fcfdc69 100644 --- a/src/tui/widgets.rs +++ b/src/tui/widgets.rs @@ -631,12 +631,7 @@ fn build_patch_detail_text(app: &App) -> Text<'static> { .add_modifier(Modifier::BOLD), )); for review in &patch.reviews { let verdict_str = match review.verdict { ReviewVerdict::Approve => "approve", ReviewVerdict::RequestChanges => "request-changes", ReviewVerdict::Comment => "comment", ReviewVerdict::Reject => "reject", }; let verdict_str = review.verdict.as_str(); let verdict_color = match review.verdict { ReviewVerdict::Approve => Color::Green, ReviewVerdict::RequestChanges => Color::Yellow, diff --git a/tests/cli_test.rs b/tests/cli_test.rs index 33952c8..1ca1b58 100644 --- a/tests/cli_test.rs +++ b/tests/cli_test.rs @@ -382,7 +382,7 @@ fn test_patch_review_approve() { let out = repo.run_ok(&["patch", "show", &id]); assert!(out.contains("LGTM!")); assert!(out.contains("Approve")); assert!(out.contains("(approve)")); assert!(out.contains("Reviews")); } @@ -402,7 +402,7 @@ fn test_patch_review_request_changes() { ]); let out = repo.run_ok(&["patch", "show", &id]); assert!(out.contains("RequestChanges")); assert!(out.contains("(request-changes)")); assert!(out.contains("Needs error handling")); } @@ -723,8 +723,8 @@ fn test_full_patch_review_cycle() { assert!(out.contains("[merged]")); assert!(out.contains("Added documentation")); assert!(out.contains("LGTM now")); assert!(out.contains("RequestChanges")); assert!(out.contains("Approve")); assert!(out.contains("(request-changes)")); assert!(out.contains("(approve)")); assert!(out.contains("Missing doc comment")); assert!(out.contains("Otherwise looks good")); assert!(out.contains("Inline Comments"));