diff --git a/docs/swarm/review-classifier.md b/docs/swarm/review-classifier.md index edb83bea..eb732898 100644 --- a/docs/swarm/review-classifier.md +++ b/docs/swarm/review-classifier.md @@ -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 diff --git a/swarm-controller/src/forge.rs b/swarm-controller/src/forge.rs index 45e8c46c..162ffedf 100644 --- a/swarm-controller/src/forge.rs +++ b/swarm-controller/src/forge.rs @@ -105,29 +105,6 @@ pub struct OpenPull { pub head_sha: Option, } -/// 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 { - 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) -> Vec { @@ -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>>) { @@ -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::>() - .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");