fix(#3129): treat a blank head sha as unknown too, not as a real value
argus's review note: the emptiness guard was one-sided. A reviewed sha of "" was already treated as unknown, but a head of "" was not — so a forge returning an empty string rather than omitting the field would make every review compare unequal and mark the whole PR stale. That is the wrong-direction failure this function exists to prevent, and the asymmetry contradicted its own doc comment. Both sides now check emptiness, for the same reason: a blank string is a value the forge sent, not a sha it has.
This commit is contained in:
parent
178eb13993
commit
0e64f46221
1 changed files with 9 additions and 1 deletions
|
|
@ -274,7 +274,11 @@ fn reviewed_older_head(reviewed_sha: Option<&str>, head_sha: Option<&str>) -> bo
|
||||||
let (Some(head), Some(reviewed)) = (head_sha, reviewed_sha) else {
|
let (Some(head), Some(reviewed)) = (head_sha, reviewed_sha) else {
|
||||||
return false;
|
return false;
|
||||||
};
|
};
|
||||||
!reviewed.is_empty() && reviewed != head
|
// 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
|
/// Latest non-comment review per reviewer on a PR. Reviews come
|
||||||
|
|
@ -395,6 +399,10 @@ mod tests {
|
||||||
assert!(!reviewed_older_head(None, Some("f4c47088")));
|
assert!(!reviewed_older_head(None, Some("f4c47088")));
|
||||||
assert!(!reviewed_older_head(Some("81292f14"), None));
|
assert!(!reviewed_older_head(Some("81292f14"), None));
|
||||||
assert!(!reviewed_older_head(Some(""), Some("f4c47088")));
|
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("")));
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue