vinzenz/hyperhive
vinzenz/hyperhive
Watch
0
0
Fork
You've already forked hyperhive
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

Compare commits

base: vinzenz:0e64f46221bea046fd59698590de059a656e6aeb
Branches Tags
vinzenz:main
..
compare: vinzenz:f4c470881e1eb0082f5e5e5f20beae8ad7c8560e
Branches Tags
vinzenz:main
1 changed files with 2 additions and 67 deletions
Download patch file Download diff file Expand all files Collapse all files

69
hive-forge/src/verbs/mod.rs
View file

@ -237,14 +237,6 @@ fn edit_distance(a: &str, b: &str) -> usize {
/// string still reads `APPROVED` / `REQUEST_CHANGES` — so surfacing them
/// stops the CLI from reporting a no-longer-valid review as still-good, and
/// [`ReviewInfo::superseded`] rolls both into one "doesn't count" check.
///
/// ⚠️ **`stale` here is not forgejo's flag alone.** That flag is eventually
/// consistent: seconds after a push it still reports the pre-push answer, so
/// a verdict against the previous head reads as current in exactly the window
/// where someone runs the CLI right after pushing. [`latest_reviews`] therefore
/// ORs it with a direct comparison of the review's own `commit_id` against the
/// PR head — the flag is right *eventually*, the comparison is right
/// *immediately*, and either alone is worse than both.
pub(crate) struct ReviewInfo {
pub login: String,
pub state: String,
@ -261,26 +253,6 @@ impl ReviewInfo {
}
}
/// Whether this review was submitted against a commit that is no longer the
/// PR head — the race-free half of the staleness check.
///
/// Fails **closed on unknowns**, i.e. "not stale": either side missing means
/// we cannot show the head moved, and the cost of guessing wrong in that
/// direction is one redundant re-review, where the other direction would
/// silently void every verdict on the PR (blocking nothing, but reporting a
/// ready PR as unreviewed and inviting a re-request that dismisses a real
/// approval).
fn reviewed_older_head(reviewed_sha: Option<&str>, head_sha: Option<&str>) -> bool {
let (Some(head), Some(reviewed)) = (head_sha, reviewed_sha) else {
return false;
};
// Both emptiness checks matter, and for the same reason: a blank string
// is a value the forge sent, not a sha it has. Treating one as real would
// make every review compare unequal and mark the whole PR stale — the
// direction this whole function exists to avoid.
!head.is_empty() && !reviewed.is_empty() && reviewed != head
}
/// Latest non-comment review per reviewer on a PR. Reviews come
/// oldest-first, so a later verdict from the same user supersedes an
/// earlier one; `COMMENT` / `PENDING` reviews carry no verdict and are
@ -294,16 +266,6 @@ fn reviewed_older_head(reviewed_sha: Option<&str>, head_sha: Option<&str>) -> bo
pub(crate) fn latest_reviews(client: &Client, repo: &str, pr: u64) -> Result<Vec<ReviewInfo>> {
let (owner, name) = crate::client::split_repo(repo)?;
let pr = index(pr)?;
// The head this PR currently points at, used to age out verdicts forgejo
// has not marked stale yet (see `ReviewInfo`). Best-effort: on any failure
// we fall back to forgejo's flag alone, which is today's behaviour — a
// missing head must never make every review look superseded.
let head_sha = client
.api()
.repo_get_pull_request(owner, name, pr)
.send()
.ok()
.and_then(|pull| pull.head.as_ref().and_then(|h| h.sha.clone()));
// Paginate (50/page, 10-page runaway cap — same ceiling the raw
// client used) so a heavily re-reviewed PR doesn't truncate.
let mut reviews = Vec::new();
@ -329,8 +291,7 @@ pub(crate) fn latest_reviews(client: &Client, repo: &str, pr: u64) -> Result<Vec
if st == "COMMENT" || st == "PENDING" || st.is_empty() {
continue;
}
let stale = r.stale.unwrap_or(false)
|| reviewed_older_head(r.commit_id.as_deref(), head_sha.as_deref());
let stale = r.stale.unwrap_or(false);
let dismissed = r.dismissed.unwrap_or(false);
if let Some(slot) = latest.iter_mut().find(|info| info.login == login) {
st.clone_into(&mut slot.state);
@ -377,33 +338,7 @@ pub(crate) fn dependency_summaries(
#[cfg(test)]
mod tests {
use super::{pct_encode, reviewed_older_head};
#[test]
fn review_on_an_older_commit_is_stale() {
// The bug this exists for: forgejo still reports `stale: false` here
// in the seconds after a push, so the comparison has to catch it.
assert!(reviewed_older_head(Some("81292f14"), Some("f4c47088")));
}
#[test]
fn review_on_the_current_head_is_not_stale() {
assert!(!reviewed_older_head(Some("f4c47088"), Some("f4c47088")));
}
/// Every unknown fails *toward* keeping the verdict. Voiding every review
/// on a forge that stopped reporting one of these would be a far louder
/// wrong answer than one redundant re-review.
#[test]
fn unknown_commit_or_head_is_not_stale() {
assert!(!reviewed_older_head(None, Some("f4c47088")));
assert!(!reviewed_older_head(Some("81292f14"), None));
assert!(!reviewed_older_head(Some(""), Some("f4c47088")));
// Absent and blank have to behave the same on BOTH sides — a blank
// head that counted as real would mark every review on the PR stale.
assert!(!reviewed_older_head(Some("81292f14"), Some("")));
assert!(!reviewed_older_head(Some(""), Some("")));
}
use super::pct_encode;
#[test]
fn pct_encode_passes_unreserved_through() {