vinzenz/hyperhive
vinzenz/hyperhive
Watch
0
0
Fork
You've already forked hyperhive
 0
Code Activity

Compare commits

base: vinzenz:9bbb28731af833ae2b62e68c290182c58d1d6be9
Branches Tags
vinzenz:main
..
compare: vinzenz:0f3da0a496a6533b77ec577ce910f89c59a3822f
Branches Tags
vinzenz:main
2 changed files with 19 additions and 77 deletions
Download patch file Download diff file Expand all files Collapse all files

13
docs/swarm/review-classifier.md
View file

@ -308,14 +308,11 @@ merge base and at its head, with 25 lines of context around each change
rather than the 3 of the forge's `.diff`. An enclosing gate, such as a
`config = lib.mkIf …` or an option's `default`, is in view when it's within
25 lines of the change, so a move that restates it doesn't read as adding one.
A gate further away is still out of view. At review time the controller takes
the merge base from the PR's commits: the one parent that isn't a PR commit.
A rebased or force-pushed PR therefore gets the diff against the base it sits on
now, not the base it started from. The controller sends the forge's `.diff`
instead when the PR changes more than 100 files, when the commits don't name
a single merge base (the head merges the base branch in), when reading a
changed file fails, or when the wide diff is over 256 KiB. The review then says
so, with the reason.
A gate further away is still out of view. The
controller sends the forge's `.diff` instead when the PR changes more than
100 files, when the forge names no merge base, when reading a changed file
fails, or when the wide diff is over 256 KiB. The review then says so, with
the reason.
On the API key runtime the commits and diff are one content block of the user
message and the question another, with a cache breakpoint at the end of the

83
swarm-controller/src/forge.rs
View file

@ -105,29 +105,6 @@ pub struct OpenPull {
pub head_sha: Option<String>,
}
/// The merge base of a PR's head and base branch, read off its commits (the
/// forge lists them from the current head and base): the one parent that is
/// not itself a PR commit. `None` when the PR has no commits or its commits
/// reach more than one such parent (a merge of the base branch into the head).
///
/// The PR's own `merge_base` field is not used: the forge stores it and
/// refreshes it asynchronously, so right after a rebase or force-push it
/// still names the old base.
fn merge_base_of(commits: &[forgejo_api::structs::Commit]) -> Option<String> {
let own: HashSet<&str> = commits.iter().filter_map(|c| c.sha.as_deref()).collect();
let outside: HashSet<&str> = commits
.iter()
.flat_map(|c| c.parents.iter().flatten())
.filter_map(|p| p.sha.as_deref())
.filter(|sha| !own.contains(sha))
.collect();
let mut outside = outside.into_iter();
match (outside.next(), outside.next()) {
(Some(base), None) => Some(base.to_owned()),
_ => None,
}
}
/// `pulls` as the dropdown's rows, newest (highest number) first. A PR
/// without a usable number cannot be named, so it is left out.
fn open_pulls(pulls: Vec<forgejo_api::structs::PullRequest>) -> Vec<OpenPull> {
@ -1276,7 +1253,7 @@ impl Client {
/// What the review classifier reads of `owner/repo`'s PR `number`: its
/// head commit, its commits oldest first with their full messages, and
/// its unified diff from the merge base ([`merge_base_of`]): [`pull_diff::wide`]'s, or the
/// its unified diff from the merge base: [`pull_diff::wide`]'s, or the
/// forge's `.diff` when the wide one is not read, cannot be read, or is
/// too large.
///
@ -1295,12 +1272,12 @@ impl Client {
.call(|api| api.repo_get_pull_request(owner, repo, number).into_future())
.await
.with_context(|| format!("read PR {number} of {owner}/{repo}"))?;
let changed_files = pull.changed_files;
let (merge_base, changed_files) = (pull.merge_base, pull.changed_files);
let head_sha = pull
.head
.and_then(|h| h.sha)
.with_context(|| format!("PR {number} of {owner}/{repo} names no head commit"))?;
let listed_commits = self
let commits = self
.api
.call(|api| {
Box::pin(
@ -1317,9 +1294,7 @@ impl Client {
)
})
.await
.with_context(|| format!("list the commits of PR {number} of {owner}/{repo}"))?;
let merge_base = merge_base_of(&listed_commits);
let commits = listed_commits
.with_context(|| format!("list the commits of PR {number} of {owner}/{repo}"))?
.into_iter()
.map(|c| Commit {
sha: c.sha.unwrap_or_default(),
@ -1329,7 +1304,7 @@ impl Client {
let count = changed_files.map_or(0, |n| usize::try_from(n).unwrap_or(usize::MAX));
let wide = match (pull_diff::too_many_files(count), merge_base) {
(Some(reason), _) => Err(reason),
(None, None) => Err("the PR's commits do not name a single merge base".to_owned()),
(None, None) => Err("the forge names no merge base".to_owned()),
(None, Some(base)) => {
match self
.changed_files(owner, repo, number, &base, &head_sha)
@ -2599,15 +2574,11 @@ mod tests {
(Client { api, url }, seen)
}
/// One PR commit whose parent is `mb`.
const ON_MB: &str = r#"[{"sha":"c1","parents":[{"sha":"mb"}]}]"#;
/// A stand-in forge holding PR 7 of `o/r`, head `h1`, which changes
/// `m.nix` from `old` at merge base `mb` to `new`. `pull` is the PR's own
/// JSON, `commits` its commit list. Records every request's path.
/// JSON. Records every request's path.
async fn pull_forge(
pull: &'static str,
commits: &'static str,
old: &'static str,
new: &'static str,
) -> (Client, Arc<std::sync::Mutex<Vec<String>>>) {
@ -2632,7 +2603,7 @@ mod tests {
let recorded = Arc::clone(&recorded);
let body = match uri.path() {
"/api/v1/repos/o/r/pulls/7" => Some(pull.to_owned()),
"/api/v1/repos/o/r/pulls/7/commits" => Some(commits.to_owned()),
"/api/v1/repos/o/r/pulls/7/commits" => Some("[]".to_owned()),
"/api/v1/repos/o/r/pulls/7/files" => {
Some(r#"[{"filename":"m.nix","status":"modified"}]"#.to_owned())
}
@ -2663,31 +2634,12 @@ mod tests {
(Client { api, url }, seen)
}
fn commit(sha: &str, parents: &[&str]) -> forgejo_api::structs::Commit {
let parents = parents
.iter()
.map(|p| format!(r#"{{"sha":"{p}"}}"#))
.collect::<Vec<_>>()
.join(",");
serde_json::from_str(&format!(r#"{{"sha":"{sha}","parents":[{parents}]}}"#)).unwrap()
}
#[test]
fn the_merge_base_is_the_one_parent_outside_the_pr() {
let linear = [commit("c2", &["c1"]), commit("c1", &["mb"])];
assert_eq!(merge_base_of(&linear).as_deref(), Some("mb"));
// A merge of the base branch into the head reaches two outside parents.
let merged = [commit("c2", &["c1", "main-tip"]), commit("c1", &["mb"])];
assert_eq!(merge_base_of(&merged), None);
assert_eq!(merge_base_of(&[]), None);
}
#[tokio::test]
async fn a_review_reads_the_wide_diff_from_the_merge_base_or_says_why_not() {
let old = "{\n config = lib.mkIf gate {\n a = 1;\n b = 2;\n c = 3;\n d = 4;\n e = 5;\n };\n}\n";
let new = "{\n config = lib.mkIf gate {\n a = 1;\n b = 2;\n c = 3;\n d = 4;\n };\n}\n";
let wide = r#"{"number":7,"requested_reviewers":[],"head":{"sha":"h1"},"merge_base":"mb","changed_files":1}"#;
let (client, _) = pull_forge(wide, ON_MB, old, new).await;
let (client, _) = pull_forge(wide, old, new).await;
let read = client.pull_for_review("o", "r", 7).await.unwrap();
assert_eq!(read.narrowed, None);
assert!(
@ -2704,7 +2656,7 @@ mod tests {
assert!(read.diff.contains("\n- e = 5;\n"), "{}", read.diff);
let many = r#"{"number":7,"requested_reviewers":[],"head":{"sha":"h1"},"merge_base":"mb","changed_files":101}"#;
let (client, seen) = pull_forge(many, ON_MB, old, new).await;
let (client, seen) = pull_forge(many, old, new).await;
let read = client.pull_for_review("o", "r", 7).await.unwrap();
assert_eq!(read.diff, "forge diff\n");
assert_eq!(
@ -2716,25 +2668,18 @@ mod tests {
"no file is read for a PR past the limit"
);
// The PR record still names the base it was opened on; the commits
// name the current one.
let stale = r#"{"number":7,"requested_reviewers":[],"head":{"sha":"h1"},"merge_base":"old-base","changed_files":1}"#;
let (client, _) = pull_forge(stale, ON_MB, old, new).await;
let read = client.pull_for_review("o", "r", 7).await.unwrap();
assert_eq!(read.narrowed, None, "the diff is read from `mb`");
assert!(read.diff.contains("\n- e = 5;\n"), "{}", read.diff);
let no_commits = r#"{"number":7,"requested_reviewers":[],"head":{"sha":"h1"},"merge_base":"mb","changed_files":1}"#;
let (client, _) = pull_forge(no_commits, "[]", old, new).await;
let no_base =
r#"{"number":7,"requested_reviewers":[],"head":{"sha":"h1"},"changed_files":1}"#;
let (client, _) = pull_forge(no_base, old, new).await;
let read = client.pull_for_review("o", "r", 7).await.unwrap();
assert_eq!(read.diff, "forge diff\n");
assert_eq!(
read.narrowed.as_deref(),
Some("the PR's commits do not name a single merge base")
Some("the forge names no merge base")
);
let unreadable = r#"{"number":7,"requested_reviewers":[],"head":{"sha":"h2"},"merge_base":"mb","changed_files":1}"#;
let (client, _) = pull_forge(unreadable, ON_MB, old, new).await;
let (client, _) = pull_forge(unreadable, old, new).await;
let read = client.pull_for_review("o", "r", 7).await.unwrap();
assert_eq!(read.diff, "forge diff\n");
let reason = read.narrowed.expect("a failed file read falls back");