diff --git a/crates/mergify-cli/src/main.rs b/crates/mergify-cli/src/main.rs index c8969d7a..b37a9448 100644 --- a/crates/mergify-cli/src/main.rs +++ b/crates/mergify-cli/src/main.rs @@ -212,8 +212,8 @@ enum NativeCommand { InternalStackLocalCommits(InternalStackLocalCommitsOpts), /// `_internal stack-remote-changes --github-server URL --token T /// --user U --repo R --stack-prefix P --author A` — Python - /// migration helper. Searches GitHub for the open + merged PRs - /// belonging to the stack, groups them by Change-Id, prints a + /// migration helper. Looks up the open + merged PRs on the + /// stack's live branches, groups them by Change-Id, prints a /// JSON array of `{change_id, pull}` records. Wire format is /// not stable. InternalStackRemoteChanges(InternalStackRemoteChangesOpts), @@ -2712,7 +2712,7 @@ fn run_native(cmd: NativeCommand) -> ExitCode { Ok(mergify_core::ExitCode::Success) } NativeCommand::InternalStackRemoteChanges(opts) => { - // Search GitHub for PRs belonging to the stack and + // Look up the PRs on the stack's live branches and // group them by Change-Id, printed as a JSON array of // `{change_id, pull}` records for whoever invoked this // hidden command. @@ -2734,6 +2734,7 @@ fn run_native(cmd: NativeCommand) -> ExitCode { &opts.repo, &opts.stack_prefix, Some(opts.author.as_str()), + &[], ) .await?; let json = serde_json::to_string(&changes).map_err(|e| { @@ -3068,11 +3069,10 @@ enum InternalSubcommand { /// extraction. Not a stable user-facing surface. #[command(name = "stack-local-commits")] StackLocalCommits(InternalStackLocalCommitsArgs), - /// Search GitHub for the open + merged PRs belonging to a - /// stack and group them by `Change-Id`. Used by the Python - /// side of `mergify stack ` during migration to - /// centralise the GitHub search + per-PR fetch + change-id - /// regrouping. Not a stable user-facing surface. + /// Look up the open + merged PRs on a stack's live branches + /// and group them by `Change-Id`. A debugging aid: it has no + /// local stack, so a merged PR whose branch was deleted is not + /// found. Not a stable user-facing surface. #[command(name = "stack-remote-changes")] StackRemoteChanges(InternalStackRemoteChangesArgs), /// Self-invocation target for the rebase-family stack @@ -3821,12 +3821,12 @@ struct InternalStackRemoteChangesArgs { /// Repository name. #[arg(long)] repo: String, - /// Stack branch prefix (e.g. `stack/main` — the search query - /// becomes `head:/`). + /// Stack branch prefix (e.g. `stack/main` — the branches + /// looked up are `/--`). #[arg(long = "stack-prefix")] stack_prefix: String, - /// PR author to filter on. Limits the search to PRs the - /// current user owns — `mergify stack` only manages its own. + /// PR author to filter on. Keeps only PRs the current user + /// opened — `mergify stack` only manages its own. #[arg(long)] author: String, } diff --git a/crates/mergify-cli/tests/stack_list.rs b/crates/mergify-cli/tests/stack_list.rs new file mode 100644 index 00000000..3b797cfa --- /dev/null +++ b/crates/mergify-cli/tests/stack_list.rs @@ -0,0 +1,230 @@ +//! End-to-end tests for `mergify stack list`. Spawns the real binary +//! against a wiremock GitHub server and a real git repo. + +use std::path::{Path, PathBuf}; +use std::process::Command; + +use wiremock::matchers::{method, path, query_param}; +use wiremock::{Mock, MockServer, ResponseTemplate}; + +fn mergify_binary() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_mergify")) +} + +fn isolated_git() -> Command { + let mut cmd = Command::new("git"); + cmd.env("GIT_CONFIG_GLOBAL", "/dev/null"); + cmd.env("GIT_CONFIG_NOSYSTEM", "1"); + cmd +} + +fn run_in(dir: &Path, args: &[&str]) { + let ok = isolated_git() + .arg("-C") + .arg(dir) + .args(args) + .status() + .unwrap() + .success(); + assert!(ok, "git -C {}: {args:?} failed", dir.display()); +} + +fn capture(dir: &Path, args: &[&str]) -> String { + let out = isolated_git() + .arg("-C") + .arg(dir) + .args(args) + .output() + .unwrap(); + String::from_utf8(out.stdout).unwrap().trim().to_string() +} + +/// `feature` branch with `n_commits` commits on top of a pushed +/// `main`, each carrying a Change-Id distinct in its first 8 hex — +/// the part a stack branch name carries. +fn build_stack_repo(n_commits: usize) -> (tempfile::TempDir, Vec<(String, String)>) { + let workdir = tempfile::tempdir().unwrap(); + let upstream = workdir.path().join("up.git"); + isolated_git() + .args([ + "init", + "-q", + "--bare", + "-b", + "main", + upstream.to_str().unwrap(), + ]) + .status() + .unwrap(); + let local = workdir.path().join("local"); + std::fs::create_dir(&local).unwrap(); + for args in [ + &["init", "-q", "-b", "main"][..], + &["config", "user.email", "t@e.com"], + &["config", "user.name", "T"], + ] { + run_in(&local, args); + } + std::fs::write(local.join("root.txt"), "root").unwrap(); + run_in(&local, &["add", "root.txt"]); + run_in(&local, &["commit", "-q", "-m", "root"]); + run_in( + &local, + &["remote", "add", "origin", upstream.to_str().unwrap()], + ); + run_in(&local, &["push", "-q", "origin", "main"]); + run_in(&local, &["remote", "set-head", "origin", "main"]); + run_in(&local, &["checkout", "-q", "-b", "feature"]); + + let mut commits = Vec::new(); + for i in 0..n_commits { + let label = (b'A' + u8::try_from(i).expect("test stack stays under 26 commits")) as char; + let fname = format!("{}.txt", label.to_lowercase()); + std::fs::write(local.join(&fname), format!("content {label}")).unwrap(); + run_in(&local, &["add", &fname]); + let cid = format!("I{:08x}{}", i + 1, "0".repeat(32)); + let msg = format!("Commit {label}\n\nChange-Id: {cid}"); + run_in(&local, &["commit", "-q", "-m", &msg]); + commits.push((capture(&local, &["rev-parse", "HEAD"]), cid)); + } + (workdir, commits) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn list_reads_mergeable_from_the_pull_request_itself() { + // Stack discovery goes through the pull-request *list* endpoint, + // which never carries `mergeable`. `stack list` has to ask the PR + // for it, or it could no longer flag a conflicting one. + let (work, commits) = build_stack_repo(2); + let local = work.path().join("local"); + let head_ref = format!("stack/tester/feature/commit--{}", &commits[0].1[1..9]); + + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path( + "/repos/myorg/myrepo/git/matching-refs/heads/stack/tester/feature/", + )) + .respond_with( + ResponseTemplate::new(200) + .set_body_json(serde_json::json!([{"ref": format!("refs/heads/{head_ref}")}])), + ) + .expect(1) + .mount(&server) + .await; + let listed = serde_json::json!({ + "number": 101, + "state": "open", + "draft": false, + "merged_at": null, + "title": "Commit A", + "user": {"login": "tester"}, + "updated_at": "2026-01-01T00:00:00Z", + "head": {"ref": head_ref, "sha": commits[0].0}, + "base": {"ref": "main"}, + "html_url": "https://github.com/myorg/myrepo/pull/101", + }); + Mock::given(method("GET")) + .and(path("/repos/myorg/myrepo/pulls")) + .and(query_param("head", format!("myorg:{head_ref}"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([listed]))) + .expect(1) + .mount(&server) + .await; + // Commit B has no PR yet: its branch is looked up and holds none. + Mock::given(method("GET")) + .and(path("/repos/myorg/myrepo/pulls")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .with_priority(10) + .expect(1) + .mount(&server) + .await; + let mut full = listed.clone(); + full["mergeable"] = serde_json::json!(false); + Mock::given(method("GET")) + .and(path("/repos/myorg/myrepo/pulls/101")) + .respond_with(ResponseTemplate::new(200).set_body_json(full)) + .expect(1) + .mount(&server) + .await; + mount_no_checks_or_reviews(&server, &commits[0].0, 101).await; + + let output = run_list(&local, &server.uri()); + assert!( + output.status.success(), + "stdout: {}\nstderr: {}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + + let payload: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + let entries: Vec<_> = payload["entries"] + .as_array() + .unwrap() + .iter() + .map(|e| { + ( + e["pull_number"].clone(), + e["status"].clone(), + e["mergeable"].clone(), + ) + }) + .collect(); + assert_eq!( + entries, + vec![ + ( + serde_json::json!(101), + serde_json::json!("open"), + serde_json::json!(false) + ), + ( + serde_json::Value::Null, + serde_json::json!("no_pr"), + serde_json::Value::Null + ), + ], + ); +} + +fn run_list(local: &Path, server_uri: &str) -> std::process::Output { + Command::new(mergify_binary()) + .args([ + "stack", + "list", + "--json", + "--trunk", + "origin/main", + "--author", + "tester", + "--branch-prefix", + "stack/tester", + // Bypass slug discovery — `origin` is a local tempdir path. + "--repository", + "myorg/myrepo", + ]) + .current_dir(local) + .env("MERGIFY_TOKEN", "test-token") + .env("MERGIFY_GITHUB_SERVER", server_uri) + .env("GIT_CONFIG_GLOBAL", "/dev/null") + .env("GIT_CONFIG_NOSYSTEM", "1") + .output() + .unwrap() +} + +/// The CI and review columns: nothing to report. +async fn mount_no_checks_or_reviews(server: &MockServer, head_sha: &str, number: u64) { + Mock::given(method("GET")) + .and(path(format!( + "/repos/myorg/myrepo/commits/{head_sha}/check-runs" + ))) + .respond_with( + ResponseTemplate::new(200).set_body_json(serde_json::json!({"check_runs": []})), + ) + .mount(server) + .await; + Mock::given(method("GET")) + .and(path(format!("/repos/myorg/myrepo/pulls/{number}/reviews"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .mount(server) + .await; +} diff --git a/crates/mergify-cli/tests/stack_push_github_native.rs b/crates/mergify-cli/tests/stack_push_github_native.rs index 94323467..2f0feff2 100644 --- a/crates/mergify-cli/tests/stack_push_github_native.rs +++ b/crates/mergify-cli/tests/stack_push_github_native.rs @@ -25,7 +25,7 @@ use std::path::{Path, PathBuf}; use std::process::Command; -use wiremock::matchers::{method, path as wm_path, path_regex}; +use wiremock::matchers::{method, path as wm_path, path_regex, query_param}; use wiremock::{Mock, MockServer, ResponseTemplate}; fn mergify_binary() -> PathBuf { @@ -120,18 +120,49 @@ fn build_stack_repo(n_commits: usize) -> (tempfile::TempDir, Vec) { (workdir, change_ids) } +/// Mock the stack discovery a push starts with: `pulls` are the PRs +/// already open, each listed as a live branch under the stack prefix +/// and returned by the lookup on its own `head.ref`. Any other lookup +/// — a local commit with no PR yet — finds nothing. Each PR is given +/// the `user.login` the push filters on (`--author tester`). +async fn mount_discovery(server: &MockServer, pulls: &[serde_json::Value]) { + let head_ref = |pull: &serde_json::Value| pull["head"]["ref"].as_str().unwrap().to_string(); + let refs: Vec<_> = pulls + .iter() + .map(|pull| serde_json::json!({"ref": format!("refs/heads/{}", head_ref(pull))})) + .collect(); + Mock::given(method("GET")) + .and(wm_path( + "/repos/myorg/myrepo/git/matching-refs/heads/stack/tester/feature/", + )) + .respond_with(ResponseTemplate::new(200).set_body_json(refs)) + .mount(server) + .await; + for pull in pulls { + let mut listed = pull.clone(); + listed["user"] = serde_json::json!({"login": "tester"}); + Mock::given(method("GET")) + .and(wm_path("/repos/myorg/myrepo/pulls")) + .and(query_param("head", format!("myorg:{}", head_ref(pull)))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([listed]))) + .mount(server) + .await; + } + Mock::given(method("GET")) + .and(wm_path("/repos/myorg/myrepo/pulls")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .with_priority(10) + .mount(server) + .await; +} + /// Mock GitHub for a push that creates `pr_numbers.len()` brand-new -/// PRs: an empty search (nothing on the remote yet), a POST that -/// hands out the numbers in order, and the comment endpoints the -/// stack comment needs. +/// PRs: nothing on the remote yet, a POST that hands out the numbers +/// in order, and the comment endpoints the stack comment needs. async fn mock_github_creating(pr_numbers: &[u64]) -> MockServer { let server = MockServer::start().await; - Mock::given(method("GET")) - .and(wm_path("/search/issues")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({"items": []}))) - .mount(&server) - .await; + mount_discovery(&server, &[]).await; // One POST mock per PR, mounted newest-first so wiremock's // last-mounted-wins ordering hands out the numbers bottom-to-top @@ -198,22 +229,11 @@ async fn mock_github_updating_a_stacked_pair( head_sha: &str, ) -> MockServer { let server = MockServer::start().await; - Mock::given(method("GET")) - .and(wm_path("/search/issues")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "items": [{"number": 101}, {"number": 102}], - }))) - .mount(&server) - .await; - for (i, number) in [101_u64, 102].iter().enumerate() { - let (head, base) = if i == 0 { - (bottom_ref, "main") - } else { - (top_ref, top_base) - }; - Mock::given(method("GET")) - .and(wm_path(format!("/repos/myorg/myrepo/pulls/{number}"))) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + let pulls: Vec<_> = [(101_u64, bottom_ref, "main"), (102, top_ref, top_base)] + .iter() + .enumerate() + .map(|(i, (number, head, base))| { + serde_json::json!({ "number": number, "state": "open", "merged_at": null, @@ -224,9 +244,11 @@ async fn mock_github_updating_a_stacked_pair( "base": {"ref": base}, "html_url": format!("https://github.com/myorg/myrepo/pull/{number}"), "stack": {"id": 162_170, "number": 7, "position": i + 1, "size": 2}, - }))) - .mount(&server) - .await; + }) + }) + .collect(); + mount_discovery(&server, &pulls).await; + for number in [101_u64, 102] { Mock::given(method("GET")) .and(wm_path(format!( "/repos/myorg/myrepo/pulls/{number}/reviews" @@ -561,16 +583,9 @@ async fn a_registered_stack_down_to_one_live_pull_request_still_loses_its_commen // the removal at all. let (_work, local, change_ids, remote_head) = repo_with_pushed_branches(1, 1); let server = MockServer::start().await; - Mock::given(method("GET")) - .and(wm_path("/search/issues")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "items": [{"number": 101}], - }))) - .mount(&server) - .await; - Mock::given(method("GET")) - .and(wm_path("/repos/myorg/myrepo/pulls/101")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + mount_discovery( + &server, + &[serde_json::json!({ "number": 101, "state": "open", "merged_at": null, @@ -583,9 +598,9 @@ async fn a_registered_stack_down_to_one_live_pull_request_still_loses_its_commen // What is left of a stack that was bigger: GitHub keeps // the registration and infers the merged prefix itself. "stack": {"id": 162_170, "number": 7, "position": 1, "size": 1}, - }))) - .mount(&server) - .await; + })], + ) + .await; Mock::given(method("GET")) .and(wm_path("/repos/myorg/myrepo/pulls/101/reviews")) .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) diff --git a/crates/mergify-cli/tests/stack_sync.rs b/crates/mergify-cli/tests/stack_sync.rs index b50fb6c5..a83781cc 100644 --- a/crates/mergify-cli/tests/stack_sync.rs +++ b/crates/mergify-cli/tests/stack_sync.rs @@ -105,12 +105,44 @@ async fn start_mock_with_no_prs() -> wiremock::MockServer { }))) .mount(&server) .await; + mount_discovery(&server, None).await; + server +} + +/// Mock the stack discovery. `merged` is a PR merged from the bottom +/// commit, whose branch GitHub then deleted — the usual state after a +/// merge: no branch is left under the stack prefix, and the PR is +/// only found by the branch name its local commit implies +/// (`commit--` for "Commit A"). Every other lookup finds nothing. +async fn mount_discovery(server: &wiremock::MockServer, merged: Option) { + use wiremock::matchers::{method, path, query_param}; + use wiremock::{Mock, ResponseTemplate}; + Mock::given(method("GET")) - .and(path("/search/issues")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({"items": []}))) - .mount(&server) + .and(path( + "/repos/myorg/myrepo/git/matching-refs/heads/stack/tester/feature/", + )) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .mount(server) + .await; + if let Some(mut pull) = merged { + let head = format!("myorg:{}", pull["head"]["ref"].as_str().unwrap()); + pull["user"] = serde_json::json!({"login": "tester"}); + Mock::given(method("GET")) + .and(path("/repos/myorg/myrepo/pulls")) + .and(query_param("head", head)) + .and(query_param("state", "all")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([pull]))) + .expect(1) + .mount(server) + .await; + } + Mock::given(method("GET")) + .and(path("/repos/myorg/myrepo/pulls")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .with_priority(10) + .mount(server) .await; - server } fn run_mergify(local: &Path, server_uri: &str, args: &[&str]) -> std::process::Output { @@ -168,8 +200,7 @@ async fn sync_dry_run_reports_up_to_date_when_no_merged_prs() { #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn sync_dry_run_lists_merged_commits() { - use wiremock::matchers::{method, path}; - use wiremock::{Mock, MockServer, ResponseTemplate}; + use wiremock::MockServer; let (work, commits) = build_stack_repo(2); let local = work.path().join("local"); @@ -177,26 +208,19 @@ async fn sync_dry_run_lists_merged_commits() { let first_cid = commits[0].1.clone(); let server = MockServer::start().await; - Mock::given(method("GET")) - .and(path("/search/issues")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "items": [{"number": 42}], - }))) - .mount(&server) - .await; - Mock::given(method("GET")) - .and(path("/repos/myorg/myrepo/pulls/42")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + mount_discovery( + &server, + Some(serde_json::json!({ "number": 42, "title": "Commit A", "state": "closed", "merged_at": "2025-01-01T00:00:00Z", - "head": {"sha": first_sha, "ref": format!("stack/tester/feature/feat-a--{}", &first_cid[1..9])}, + "head": {"sha": first_sha, "ref": format!("stack/tester/feature/commit--{}", &first_cid[1..9])}, "base": {"ref": "main"}, "html_url": "https://github.com/myorg/myrepo/pull/42", - }))) - .mount(&server) - .await; + })), + ) + .await; let output = run_mergify( &local, @@ -233,8 +257,7 @@ async fn sync_dry_run_lists_merged_commits() { /// remove — the abort we guard against with `--reapply-cherry-picks`. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn sync_drops_squash_merged_commit_with_matching_patch() { - use wiremock::matchers::{method, path}; - use wiremock::{Mock, MockServer, ResponseTemplate}; + use wiremock::MockServer; let (work, commits) = build_stack_repo(2); let local = work.path().join("local"); @@ -253,26 +276,19 @@ async fn sync_drops_squash_merged_commit_with_matching_patch() { run_in(&local, &["checkout", "-q", "feature"]); let server = MockServer::start().await; - Mock::given(method("GET")) - .and(path("/search/issues")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "items": [{"number": 42}], - }))) - .mount(&server) - .await; - Mock::given(method("GET")) - .and(path("/repos/myorg/myrepo/pulls/42")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + mount_discovery( + &server, + Some(serde_json::json!({ "number": 42, "title": "Commit A", "state": "closed", "merged_at": "2025-01-01T00:00:00Z", - "head": {"sha": a_sha, "ref": format!("stack/tester/feature/feat-a--{}", &a_cid[1..9])}, + "head": {"sha": a_sha, "ref": format!("stack/tester/feature/commit--{}", &a_cid[1..9])}, "base": {"ref": "main"}, "html_url": "https://github.com/myorg/myrepo/pull/42", - }))) - .mount(&server) - .await; + })), + ) + .await; let output = run_mergify( &local, diff --git a/crates/mergify-stack/src/commands/checkout.rs b/crates/mergify-stack/src/commands/checkout.rs index 016d3df3..0e1f3c8e 100644 --- a/crates/mergify-stack/src/commands/checkout.rs +++ b/crates/mergify-stack/src/commands/checkout.rs @@ -20,9 +20,11 @@ //! //! 1. Resolve the target into a stack branch (+ `owner/repo`, //! which a PR URL carries). -//! 2. Search GitHub for the stack's PRs (via +//! 2. Look up the stack's PRs (via //! [`crate::remote_changes::get_remote_changes`], with no -//! `author:` filter — a branch name is unique within a repo). +//! author filter — a branch name is unique within a repo). Only +//! open PRs make the chain, and an open PR's branch always +//! exists, so no local commits are needed to find them. //! 3. Link open PRs into a single chain via their `head.ref` → //! `base.ref` pointers, find the root (the PR whose `base.ref` //! is *outside* the stack — i.e. doesn't start with the stack @@ -192,7 +194,8 @@ pub async fn run(opts: &Options<'_>) -> Result { }; let remote_changes = - remote_changes::get_remote_changes(opts.client, &owner, &repo, &stack_branch, None).await?; + remote_changes::get_remote_changes(opts.client, &owner, &repo, &stack_branch, None, &[]) + .await?; let chain = build_chain(&remote_changes, &stack_branch)?; if chain.is_empty() { @@ -458,17 +461,10 @@ mod tests { async fn run_no_stacked_prs_returns_no_stacked_prs() { use mergify_core::ApiFlavor; use url::Url; - use wiremock::matchers::{method, path}; - use wiremock::{Mock, MockServer, ResponseTemplate}; + use wiremock::MockServer; let server = MockServer::start().await; - Mock::given(method("GET")) - .and(path("/search/issues")) - .respond_with( - ResponseTemplate::new(200).set_body_json(serde_json::json!({"items": []})), - ) - .mount(&server) - .await; + mount_discovery(&server, "stack/author/my-branch", &[]).await; let client = HttpClient::new( Url::parse(&server.uri()).unwrap(), @@ -496,46 +492,37 @@ mod tests { async fn run_dry_run_returns_chain_without_touching_git() { use mergify_core::ApiFlavor; use url::Url; - use wiremock::matchers::{method, path}; - use wiremock::{Mock, MockServer, ResponseTemplate}; + use wiremock::MockServer; let server = MockServer::start().await; - Mock::given(method("GET")) - .and(path("/search/issues")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "items": [{"number": 1}, {"number": 2}], - }))) - .mount(&server) - .await; // Head refs use the new-format `--` shape so // `extract_from_branch_segment` accepts them and the // remote_changes pipeline doesn't filter them out. - Mock::given(method("GET")) - .and(path("/repos/user/repo/pulls/1")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "number": 1, - "title": "feat: A", - "html_url": "https://github.com/user/repo/pull/1", - "state": "open", - "base": {"ref": "main"}, - "head": {"ref": "stack/author/my-branch/feat-a--aaaaaaaa"}, - "merged_at": null, - }))) - .mount(&server) - .await; - Mock::given(method("GET")) - .and(path("/repos/user/repo/pulls/2")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "number": 2, - "title": "feat: B", - "html_url": "https://github.com/user/repo/pull/2", - "state": "open", - "base": {"ref": "stack/author/my-branch/feat-a--aaaaaaaa"}, - "head": {"ref": "stack/author/my-branch/feat-b--bbbbbbbb"}, - "merged_at": null, - }))) - .mount(&server) - .await; + mount_discovery( + &server, + "stack/author/my-branch", + &[ + serde_json::json!({ + "number": 1, + "title": "feat: A", + "html_url": "https://github.com/user/repo/pull/1", + "state": "open", + "base": {"ref": "main"}, + "head": {"ref": "stack/author/my-branch/feat-a--aaaaaaaa"}, + "merged_at": null, + }), + serde_json::json!({ + "number": 2, + "title": "feat: B", + "html_url": "https://github.com/user/repo/pull/2", + "state": "open", + "base": {"ref": "stack/author/my-branch/feat-a--aaaaaaaa"}, + "head": {"ref": "stack/author/my-branch/feat-b--bbbbbbbb"}, + "merged_at": null, + }), + ], + ) + .await; let client = HttpClient::new( Url::parse(&server.uri()).unwrap(), @@ -577,22 +564,14 @@ mod tests { #[tokio::test] async fn run_strips_changeid_suffix_from_a_pasted_leaf_ref() { // The user pastes a leaf branch ref verbatim, current - // `--<8 hex>` naming included. The search has to be - // issued against the stack stem, not the leaf. + // `--<8 hex>` naming included. The branches have to be + // listed under the stack stem, not the leaf. use mergify_core::ApiFlavor; use url::Url; - use wiremock::matchers::{method, path, query_param_contains}; - use wiremock::{Mock, MockServer, ResponseTemplate}; + use wiremock::MockServer; let server = MockServer::start().await; - Mock::given(method("GET")) - .and(path("/search/issues")) - .and(query_param_contains("q", "head:stack/author/my-branch/")) - .respond_with( - ResponseTemplate::new(200).set_body_json(serde_json::json!({"items": []})), - ) - .mount(&server) - .await; + mount_discovery(&server, "stack/author/my-branch", &[]).await; let client = HttpClient::new( Url::parse(&server.uri()).unwrap(), @@ -719,7 +698,7 @@ mod tests { // the full root→leaf chain, not just that PR and below. use mergify_core::ApiFlavor; use url::Url; - use wiremock::matchers::{method, path, query_param}; + use wiremock::matchers::{method, path}; use wiremock::{Mock, MockServer, ResponseTemplate}; fn pr_body(number: u64, base: &str, head: &str) -> serde_json::Value { @@ -731,6 +710,7 @@ mod tests { "base": {"ref": base}, "head": {"ref": head}, "merged_at": null, + "user": {"login": "someone-else"}, }) } @@ -747,18 +727,10 @@ mod tests { ]; let server = MockServer::start().await; - // No `author:` qualifier — checkout reaches anyone's stack. - Mock::given(method("GET")) - .and(path("/search/issues")) - .and(query_param( - "q", - "repo:user/repo is:pull-request head:stack/author/my-branch/", - )) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "items": [{"number": 1}, {"number": 2}, {"number": 3}], - }))) - .mount(&server) - .await; + // No author filter — checkout reaches anyone's stack, so PRs + // opened by someone else still make the chain. + mount_discovery(&server, stack, &bodies).await; + // The PR URL is resolved to its stack branch first. for (i, body) in bodies.iter().enumerate() { Mock::given(method("GET")) .and(path(format!("/repos/user/repo/pulls/{}", i + 1))) @@ -807,6 +779,42 @@ mod tests { } } + /// Mount the stack discovery for `stack_branch`: each of `pulls` + /// listed as a live branch under it, and returned by the lookup + /// on its own `head.ref`. The branches must be listed exactly + /// once, under the stack stem. + async fn mount_discovery( + server: &wiremock::MockServer, + stack_branch: &str, + pulls: &[serde_json::Value], + ) { + use wiremock::matchers::{method, path, query_param}; + use wiremock::{Mock, ResponseTemplate}; + + let head_ref = |pull: &serde_json::Value| pull["head"]["ref"].as_str().unwrap().to_string(); + let refs: Vec<_> = pulls + .iter() + .map(|pull| serde_json::json!({"ref": format!("refs/heads/{}", head_ref(pull))})) + .collect(); + Mock::given(method("GET")) + .and(path(format!( + "/repos/user/repo/git/matching-refs/heads/{stack_branch}/" + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(refs)) + .expect(1) + .mount(server) + .await; + for pull in pulls { + Mock::given(method("GET")) + .and(path("/repos/user/repo/pulls")) + .and(query_param("head", format!("user:{}", head_ref(pull)))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([pull]))) + .expect(1) + .mount(server) + .await; + } + } + /// Mount a two-PR stack under `stack_branch` and return the /// client pointed at it. The server has to outlive the call, /// so it comes back with the client. @@ -834,13 +842,9 @@ mod tests { ]; let server = MockServer::start().await; - Mock::given(method("GET")) - .and(path("/search/issues")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "items": [{"number": 1}, {"number": 2}], - }))) - .mount(&server) - .await; + mount_discovery(&server, stack_branch, &bodies).await; + // A pull-request URL target is resolved to its stack branch + // through the PR itself. for (i, body) in bodies.iter().enumerate() { Mock::given(method("GET")) .and(path(format!("/repos/user/repo/pulls/{}", i + 1))) diff --git a/crates/mergify-stack/src/commands/list.rs b/crates/mergify-stack/src/commands/list.rs index e151d06f..4cfd28ad 100644 --- a/crates/mergify-stack/src/commands/list.rs +++ b/crates/mergify-stack/src/commands/list.rs @@ -107,21 +107,22 @@ pub async fn run(opts: &Options<'_>) -> Result { ))); } + let local = local_commits::read(&repo_dir, &base_commit_sha, "HEAD")?; let remote_changes = remote_changes::get_remote_changes( opts.client, opts.user, opts.repo, &stack_prefix, Some(opts.author), + &local, ) .await?; - let local = local_commits::read(&repo_dir, &base_commit_sha, "HEAD")?; let classified = changes::classify(&local, remote_changes)?; let mut entries = Vec::with_capacity(classified.locals.len()); for local in &classified.locals { - let (status, pull_number, pull_url, mergeable) = match &local.pull { - None => ("no_pr".to_string(), None, None, None), + let (status, pull_number, pull_url) = match &local.pull { + None => ("no_pr".to_string(), None, None), Some(pull) => { let merged = pull.get("merged_at").is_some_and(|v| !v.is_null()); let draft = pull.get("draft").and_then(Value::as_bool).unwrap_or(false); @@ -137,8 +138,7 @@ pub async fn run(opts: &Options<'_>) -> Result { .get("html_url") .and_then(Value::as_str) .map(str::to_owned); - let mergeable = pull.get("mergeable").and_then(Value::as_bool); - (status.to_string(), pull_number, pull_url, mergeable) + (status.to_string(), pull_number, pull_url) } }; entries.push(StackListEntry { @@ -152,7 +152,7 @@ pub async fn run(opts: &Options<'_>) -> Result { ci_checks: Vec::new(), review_status: "unknown".to_string(), reviews: Vec::new(), - mergeable, + mergeable: None, }); } @@ -176,7 +176,10 @@ pub async fn run(opts: &Options<'_>) -> Result { /// Per-PR fan-out: for each entry with a pull, fetch /// `/check-runs` + `/reviews` and fold them into the entry's -/// `ci_*` / `review_*` fields. Sequential rather than concurrent +/// `ci_*` / `review_*` fields, plus the PR itself for `mergeable` +/// unless it merged — the discovery payloads come from the +/// pull-request list endpoint, which never carries it (`None` while +/// GitHub is still computing it). Sequential rather than concurrent /// since GitHub's secondary rate limit kicks in around ~80 /// concurrent calls on the same endpoint pool — and a typical /// stack has well under that. @@ -215,6 +218,13 @@ async fn fetch_pr_details( let (review_status, review_list) = compute_review_status(&reviews); entries[i].review_status = review_status.to_string(); entries[i].reviews = review_list; + + if entries[i].status != "merged" { + let full: Value = client + .get(&format!("/repos/{user}/{repo}/pulls/{pull_number}")) + .await?; + entries[i].mergeable = full.get("mergeable").and_then(Value::as_bool); + } } Ok(()) } diff --git a/crates/mergify-stack/src/commands/push.rs b/crates/mergify-stack/src/commands/push.rs index 1a017ab3..8953f948 100644 --- a/crates/mergify-stack/src/commands/push.rs +++ b/crates/mergify-stack/src/commands/push.rs @@ -126,7 +126,7 @@ pub struct Options<'a> { /// `(remote, branch)` of the trunk. pub trunk: (&'a str, &'a str), /// GitHub login of the author whose PRs the stack belongs - /// to. Used both for the search filter in `remote_changes` + /// to. Used both for the author filter in `remote_changes` /// and for `branch_prefix` fallback resolution. pub author: &'a str, /// PR branch prefix (typically `stack/`); empty @@ -203,7 +203,7 @@ pub async fn run(opts: &Options<'_>) -> Result { format!("{prefix}/{dest_branch}", prefix = opts.branch_prefix) }; - // The pre-flight — trunk + notes fetch, the PR search, the rebase + // The pre-flight — trunk + notes fetch, the PR lookup, the rebase // decision — is several network round-trips with nothing to show, // the stretch that used to look frozen. Drive one spinner row // through it, resolving to a kept summary line so it never blinks @@ -232,10 +232,15 @@ pub async fn run(opts: &Options<'_>) -> Result { let trunk_ref = format!("{remote}/{base_branch}"); let base_commit_sha = compute_base_commit_sha(&repo_dir, &trunk_ref, &dest_branch)?; - // `get_remote_changes` fetches each PR sequentially. Publish the - // number it's on into a shared cell; the spinner's tick loop reads - // it each frame so the label tracks each pull (owner/repo#id) - // while the glyph keeps spinning smoothly. + // Read before the discovery: a merged PR whose branch was deleted + // is only found under the branch name a local commit implies. + let local = local_commits::read(&repo_dir, &base_commit_sha, "HEAD")?; + + // `get_remote_changes` looks each stack branch up sequentially. + // Publish how far it is into a shared cell (`done << 32 | total`, + // one atomic so the pair is never torn); the spinner's tick loop + // reads it each frame so the label tracks the lookups while the + // glyph keeps spinning smoothly. let reading = Arc::new(AtomicU64::new(0)); let reading_writer = Arc::clone(&reading); let fetch_pulls = remote_changes::get_remote_changes_reporting( @@ -244,16 +249,22 @@ pub async fn run(opts: &Options<'_>) -> Result { opts.repo, &stack_prefix, Some(opts.author), - move |number| reading_writer.store(number, Ordering::Relaxed), + &local, + move |done, total| { + let packed = + (u64::try_from(done).unwrap_or(0) << 32) | u64::try_from(total).unwrap_or(0); + reading_writer.store(packed, Ordering::Relaxed); + }, ); let (owner, repo_name) = (opts.user, opts.repo); let remote_changes_data = prog .run_reporting(pf, "reading pull requests", fetch_pulls, move || { - let n = reading.load(Ordering::Relaxed); - (n != 0).then(|| format!("reading {owner}/{repo_name}#{n}")) + let packed = reading.load(Ordering::Relaxed); + let (done, total) = (packed >> 32, packed & u64::from(u32::MAX)); + (total != 0) + .then(|| format!("reading {owner}/{repo_name} pull requests ({done}/{total})")) }) .await?; - let local = local_commits::read(&repo_dir, &base_commit_sha, "HEAD")?; let planner_opts = PlannerOpts { stack_prefix: &stack_prefix, @@ -326,7 +337,7 @@ pub async fn run(opts: &Options<'_>) -> Result { quiet: true, // The pre-flight already fetched the trunk and the remote // PRs; hand them through so the rebase doesn't repeat the - // search + per-PR GETs. + // per-branch PR lookups. prefetched_remote_changes: Some(remote_changes_data.clone()), skip_trunk_fetch: true, }; diff --git a/crates/mergify-stack/src/commands/sync.rs b/crates/mergify-stack/src/commands/sync.rs index 2dcf7424..9cff75c2 100644 --- a/crates/mergify-stack/src/commands/sync.rs +++ b/crates/mergify-stack/src/commands/sync.rs @@ -63,7 +63,7 @@ pub struct Options<'a> { pub quiet: bool, /// Remote changes already fetched by the caller (the `stack push` /// pre-flight). When `Some`, `run` reuses them instead of - /// re-running the search + per-PR GETs that `get_remote_changes` + /// re-running the per-branch PR lookups that `get_remote_changes` /// does. `None` for the standalone `stack sync` command. pub prefetched_remote_changes: Option>, /// Skip the `git fetch ` pre-rebase. Set by @@ -96,6 +96,7 @@ pub async fn run(opts: &Options<'_>) -> Result { let trunk_ref = format!("{remote}/{base_branch}"); let base_commit_sha = compute_base_commit_sha(&repo_dir, &trunk_ref, &dest_branch)?; + let local = local_commits::read(&repo_dir, &base_commit_sha, "HEAD")?; let remote_changes = match &opts.prefetched_remote_changes { Some(changes) => changes.clone(), None => { @@ -105,12 +106,11 @@ pub async fn run(opts: &Options<'_>) -> Result { opts.repo, &stack_prefix, Some(opts.author), + &local, ) .await? } }; - - let local = local_commits::read(&repo_dir, &base_commit_sha, "HEAD")?; let status = sync_status::classify( dest_branch.clone(), trunk_ref.clone(), diff --git a/crates/mergify-stack/src/remote_changes.rs b/crates/mergify-stack/src/remote_changes.rs index 39d1719b..bfe935e8 100644 --- a/crates/mergify-stack/src/remote_changes.rs +++ b/crates/mergify-stack/src/remote_changes.rs @@ -1,126 +1,234 @@ //! Discover the open + recently merged PRs belonging to a stack. //! //! Every `mergify stack ` that needs to know which PRs are -//! already on GitHub for the current branch runs this: +//! already on GitHub for the current branch runs this. It only +//! calls repository-scoped endpoints (`/repos/{owner}/{repo}/...`): +//! some environments proxy the GitHub API and refuse everything +//! else (Claude Code on the web answers `/search/issues` with a +//! 403, Mergifyio/mergify-cli#1861), and the Search API has its +//! own small rate limit on top. //! -//! 1. `GET /search/issues?q=repo:owner/repo author:author -//! is:pull-request head:prefix/` — GitHub search, sorted by -//! `updated`, capped at 100 results (the same `per_page` cap -//! the Python CLI used). Only one search call regardless of -//! stack depth. -//! 2. For each result, `GET /repos/owner/repo/pulls/` to -//! fetch the full PR payload (the search index returns a -//! pared-down shape; downstream code needs `head.sha`, -//! `merged_at`, `draft`, etc.). -//! 3. Group by [`change_id::extract_from_branch_segment`] applied -//! to the last segment of `head.ref`. Closed-but-not-merged -//! PRs are dropped. When two PRs share the same Change-Id, -//! open beats closed — and two open PRs on the same Change-Id -//! is a hard error (the user has a duplicate that the rest of -//! the orchestration can't reconcile). +//! 1. `GET /repos/owner/repo/git/matching-refs/heads//` — +//! every branch that exists under the stack prefix, in one +//! unpaginated call. Kept: branches one segment below the prefix +//! whose last segment carries a Change-Id, i.e. the shape +//! `stack push` creates. +//! 2. Add the branch each local commit would be pushed to +//! (`/`) when no live branch carries its +//! Change-Id. That is how a merged PR is still found after its +//! head branch was deleted: `SkipMerged` needs the PR's +//! `head.sha` to equal the local commit, and a commit that +//! unchanged has the same title, so the same slug. A commit +//! reworded since has a different SHA and is re-created +//! whether its merged PR is found or not. +//! 3. For each of those branches, +//! `GET /repos/owner/repo/pulls?head=owner:&state=all`. +//! That filter reads the PR's recorded head, so it answers for +//! closed and merged PRs whose branch is gone. +//! 4. Sort everything by `updated_at`, newest first, and group by +//! [`change_id::extract_from_branch_segment`] applied to the +//! last segment of `head.ref`. Closed-but-not-merged PRs are +//! dropped. When two PRs share the same Change-Id, open beats +//! closed — and two open PRs on the same Change-Id is a hard +//! error (the user has a duplicate that the rest of the +//! orchestration can't reconcile). //! -//! The output is order-preserving — GitHub's `sort=updated` -//! ordering carries through to downstream consumers (the orphan -//! list in `changes.py`'s `get_changes` iterates this map and the -//! order matters for how orphan PRs are presented). +//! The payloads are the pull-request *list* shape: everything the +//! single-PR endpoint returns except the computed fields +//! (`mergeable`, `mergeable_state`, `merged`, diff stats). A +//! consumer that needs one of those fetches the PR itself. //! -//! Exposed via the hidden `_internal stack-remote-changes` -//! subcommand on the `mergify` binary as a no-op-from-Python's- -//! perspective: the Rust impl is callable but `changes.py` still -//! does the HTTP work itself today. Migration of the Python call -//! site lands in a follow-up PR — that change requires updating -//! roughly 50 `respx`-mocked tests across the `mergify_cli/tests/stack/` -//! tree to drive the bridge stub instead, which is too much -//! churn to bundle here. +//! What the Search API found and this does not: a merged PR whose +//! branch was deleted and whose Change-Id is no longer in the local +//! stack. Consumers only report *open* orphans, so nothing reads it. +//! +//! The output is order-preserving — the `updated_at` ordering +//! carries through to downstream consumers (the orphan list is +//! presented in that order). use mergify_core::http::Client; use mergify_core::{ApiFlavor, CliError}; use serde::{Deserialize, Serialize}; use crate::change_id; +use crate::local_commits::LocalCommit; -/// One `{change_id, pull}` entry in the order GitHub returned it -/// from `/search/issues`. Emitted as a flat array (not a JSON -/// object) so consumers can rebuild an order-preserving dict -/// without relying on serde's optional `preserve_order` feature. +/// One `{change_id, pull}` entry, most recently updated first. +/// Emitted as a flat array (not a JSON object) so consumers can +/// rebuild an order-preserving dict without relying on serde's +/// optional `preserve_order` feature. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct RemoteChange { pub change_id: String, - /// Raw PR payload from `/repos/owner/repo/pulls/`. - /// Passed through as a typeless JSON value — the downstream - /// Python orchestrator consumes many fields (`head.ref`, - /// `head.sha`, `state`, `draft`, `merged_at`, `merge_commit_sha`, - /// `html_url`, …); typing them all here would be a - /// translation tax with no real correctness win. + /// Raw PR payload from `/repos/owner/repo/pulls?head=...` (the + /// list shape — no `mergeable`). Passed through as a typeless + /// JSON value — the downstream orchestrator consumes many + /// fields (`head.ref`, `head.sha`, `state`, `draft`, + /// `merged_at`, `merge_commit_sha`, `html_url`, `stack`, …); + /// typing them all here would be a translation tax with no + /// real correctness win. pub pull: serde_json::Value, } -/// Run the search → fetch → group pipeline. +/// Run the list-branches → look-up → group pipeline. /// -/// `user` / `repo` form the search scope (`repo:user/repo`). -/// `stack_prefix` filters the search to PRs whose head branch -/// starts with `prefix/`. `author` restricts the search to PRs -/// one user opened — `Some(me)` for the commands that only ever -/// manage the local user's own stack, `None` for `stack checkout`, -/// which has to reach anyone's stack. Branch names are unique -/// within a repo, so the branch filter alone already pins the -/// result set. +/// `user` / `repo` name the repository the stack lives in. +/// `stack_prefix` is the branch prefix the stack's PR branches sit +/// under. `author` keeps only PRs one user opened — `Some(me)` for +/// the commands that only ever manage the local user's own stack, +/// `None` for `stack checkout`, which has to reach anyone's stack. +/// `local_commits` supplies the branch names a merged PR may still +/// be recorded under after its branch was deleted; pass `&[]` when +/// there is no local stack (then only live branches are searched). pub async fn get_remote_changes( client: &Client, user: &str, repo: &str, stack_prefix: &str, author: Option<&str>, + local_commits: &[LocalCommit], ) -> Result, CliError> { - get_remote_changes_reporting(client, user, repo, stack_prefix, author, |_| {}).await + get_remote_changes_reporting( + client, + user, + repo, + stack_prefix, + author, + local_commits, + |_, _| {}, + ) + .await } -/// Same as [`get_remote_changes`], but calls `on_pull` with each -/// pull-request number just before fetching it — so a caller can -/// surface per-pull progress through the otherwise-silent -/// sequential fetch loop. +/// Same as [`get_remote_changes`], but calls `on_lookup(n, total)` +/// just before the `n`-th (1-based) of `total` per-branch lookups — +/// so a caller can surface progress through the otherwise-silent +/// sequential loop. pub async fn get_remote_changes_reporting( client: &Client, user: &str, repo: &str, stack_prefix: &str, author: Option<&str>, - mut on_pull: impl FnMut(u64), + local_commits: &[LocalCommit], + mut on_lookup: impl FnMut(usize, usize), ) -> Result, CliError> { - // Omitted entirely rather than left empty when there's no - // author — GitHub rejects a bare `author:` qualifier. - let author_filter = author.map_or_else(String::new, |a| format!("author:{a} ")); - let q = format!("repo:{user}/{repo} {author_filter}is:pull-request head:{stack_prefix}/"); - let search: SearchResponse = client - .get_with_query( - "/search/issues", - &[("q", &q), ("per_page", "100"), ("sort", "updated")], - ) - .await?; - - // Per-PR fetches run sequentially. The Python orchestrator - // uses `asyncio.gather` for fan-out, but the underlying - // reqwest client we wrap doesn't memoize connections by - // host the same way httpx does — and GitHub's secondary - // rate-limit kicks in around ~80 concurrent requests on the - // search-resource pool. Sequential keeps it under the - // threshold and the latency budget is small (~50 PRs × ~100ms - // each = a few seconds for the largest realistic stacks). - let mut pulls: Vec = Vec::with_capacity(search.items.len()); - for item in &search.items { - on_pull(item.number); - let path = format!("/repos/{user}/{repo}/pulls/{number}", number = item.number); - let pull: serde_json::Value = client.get(&path).await?; - pulls.push(pull); + let live = live_stack_branches(client, user, repo, stack_prefix).await?; + let branches = branches_to_look_up(live, stack_prefix, local_commits); + + // Sequential on purpose: GitHub's secondary rate limit kicks in + // on bursts of concurrent requests, and a stack is a handful of + // branches (~100ms each). + let pulls_path = format!("/repos/{user}/{repo}/pulls"); + let mut pulls: Vec = Vec::new(); + for (idx, branch) in branches.iter().enumerate() { + on_lookup(idx + 1, branches.len()); + let head = format!("{user}:{branch}"); + let found: Vec = client + .get_with_query( + &pulls_path, + &[("head", &head), ("state", "all"), ("per_page", "100")], + ) + .await?; + pulls.extend( + found + .into_iter() + .filter(|pull| author.is_none_or(|a| opened_by(pull, a))), + ); } + // ISO-8601 UTC timestamps sort chronologically as strings. A + // stable sort keeps lookup order between equal timestamps. + pulls.sort_by(|a, b| updated_at(b).cmp(updated_at(a))); group_by_change_id(pulls) } +/// Branches that exist right now one segment below `stack_prefix` +/// and end in a Change-Id — the branches `stack push` creates. +/// A nested stack (`//...`) or a hand-made branch +/// under the prefix is not this stack's. +async fn live_stack_branches( + client: &Client, + user: &str, + repo: &str, + stack_prefix: &str, +) -> Result, CliError> { + // The trailing slash matters: `matching-refs` is a plain string + // prefix match, so without it `stack/foo` would also list + // `stack/foobar/...`. + let path = format!("/repos/{user}/{repo}/git/matching-refs/heads/{stack_prefix}/"); + let refs: Vec = client.get(&path).await?; + let ref_prefix = format!("refs/heads/{stack_prefix}/"); + Ok(refs + .into_iter() + .filter_map(|r| { + let leaf = r.ref_name.strip_prefix(&ref_prefix)?; + let managed = + !leaf.contains('/') && change_id::extract_from_branch_segment(leaf).is_some(); + managed.then(|| format!("{stack_prefix}/{leaf}")) + }) + .collect()) +} + +/// `live`, plus the branch each local commit would be pushed to +/// when no live branch already carries its Change-Id. +fn branches_to_look_up( + live: Vec, + stack_prefix: &str, + local_commits: &[LocalCommit], +) -> Vec { + let live_change_ids: Vec = live + .iter() + .filter_map(|b| change_id::extract_from_branch_segment(last_segment(b))) + .map(str::to_owned) + .collect(); + let mut branches = live; + for local in local_commits { + if live_change_ids + .iter() + .any(|remote| same_change(remote, &local.change_id)) + { + continue; + } + let expected = format!("{stack_prefix}/{slug}", slug = local.slug); + if !branches.contains(&expected) { + branches.push(expected); + } + } + branches +} + +/// Whether a branch's Change-Id (the short 8-hex form or the full +/// `I` + 40 hex) and a local commit's full Change-Id name the same +/// change. The same prefix comparison the classifiers pair commits +/// with PRs by. +fn same_change(remote: &str, local: &str) -> bool { + let remote_hex = remote.strip_prefix('I').unwrap_or(remote); + let local_hex = local.strip_prefix('I').unwrap_or(local); + remote_hex.starts_with(local_hex) || local_hex.starts_with(remote_hex) +} + +fn last_segment(branch: &str) -> &str { + branch.rsplit('/').next().unwrap_or(branch) +} + +/// GitHub logins are case-insensitive, as the Search API's +/// `author:` qualifier this replaces was. +fn opened_by(pull: &serde_json::Value, author: &str) -> bool { + pull.pointer("/user/login") + .and_then(serde_json::Value::as_str) + .is_some_and(|login| login.eq_ignore_ascii_case(author)) +} + +fn updated_at(pull: &serde_json::Value) -> &str { + pull.get("updated_at") + .and_then(serde_json::Value::as_str) + .unwrap_or("") +} + /// Pure, network-free regrouping of the per-PR payloads. Exposed -/// so the search + fetch + group split can be unit-tested in -/// isolation without spinning up a mock server for every parser -/// edge case. +/// so the lookup + group split can be unit-tested in isolation +/// without spinning up a mock server for every parser edge case. fn group_by_change_id(pulls: Vec) -> Result, CliError> { let mut out: Vec = Vec::new(); for pull in pulls { @@ -139,8 +247,7 @@ fn group_by_change_id(pulls: Vec) -> Result .ok_or_else(|| { CliError::GitHubApi("PR payload missing required `head.ref` field".to_string()) })?; - let last_segment = head_ref.rsplit('/').next().unwrap_or(head_ref); - let Some(change_id) = change_id::extract_from_branch_segment(last_segment) else { + let Some(change_id) = change_id::extract_from_branch_segment(last_segment(head_ref)) else { continue; }; let change_id = change_id.to_string(); @@ -165,8 +272,8 @@ fn group_by_change_id(pulls: Vec) -> Result ))); } // Open-existing + closed-new, or both closed: - // keep the existing entry. GitHub returned the - // search sorted by `updated`, so the first-seen + // keep the existing entry. The input is sorted by + // `updated_at`, newest first, so the first-seen // one is the most recent. _ => {} } @@ -178,13 +285,9 @@ fn group_by_change_id(pulls: Vec) -> Result } #[derive(Debug, Deserialize)] -struct SearchResponse { - items: Vec, -} - -#[derive(Debug, Deserialize)] -struct SearchItem { - number: u64, +struct GitRef { + #[serde(rename = "ref")] + ref_name: String, } /// Build an [`ApiFlavor::GitHub`] client pre-configured for the @@ -198,6 +301,10 @@ pub fn default_client(github_server: url::Url, token: &str) -> Result) -> serde_json::Value { serde_json::json!({ @@ -257,9 +364,9 @@ mod tests { #[test] fn group_drops_pr_whose_branch_has_no_recognisable_change_id() { // Some user manually pushed a branch under the prefix - // without going through `mergify stack`. The search query - // can still surface it (the prefix substring matches); - // dropping it keeps the result hermetic to managed PRs. + // without going through `mergify stack`, and opened a PR + // on it. Dropping it keeps the result hermetic to managed + // PRs. let pulls = vec![pr(1, "open", "prefix/random-branch", None)]; let out = group_by_change_id(pulls).unwrap(); assert!(out.is_empty(), "got: {out:?}"); @@ -282,9 +389,9 @@ mod tests { #[test] fn group_preserves_first_seen_order_across_distinct_change_ids() { - // GitHub returns search results in `sort=updated` - // order; the orphan loop in `get_changes` observes that - // order. Confirm it's not silently re-sorted. + // The lookup sorts by `updated_at` before grouping, and + // the orphan list observes that order. Confirm grouping + // doesn't silently re-sort it. let pulls = vec![ pr(1, "open", "prefix/feat-a--aaaaaaaa", None), pr(2, "open", "prefix/feat-b--bbbbbbbb", None), @@ -337,104 +444,331 @@ mod tests { } } - #[tokio::test] - async fn get_remote_changes_searches_then_fetches_each_pull() { - // End-to-end smoke: the search query gets composed - // correctly, each result triggers a per-PR fetch, the - // grouped output matches. - use url::Url; - use wiremock::matchers::{method, path, query_param}; - use wiremock::{Mock, MockServer, ResponseTemplate}; + const ID_A: &str = "Iaaaaaaaa0000000000000000000000000000000a"; + const ID_B: &str = "Ibbbbbbbb0000000000000000000000000000000b"; + + fn local(change_id: &str, slug: &str) -> LocalCommit { + LocalCommit { + commit_sha: format!("sha-{slug}"), + title: slug.to_string(), + message: String::new(), + change_id: change_id.to_string(), + slug: slug.to_string(), + note: String::new(), + } + } - let server = MockServer::start().await; + /// A list-endpoint PR payload, with the fields the lookup + /// filters and sorts on. + fn listed(number: u64, state: &str, head_ref: &str, login: &str, updated: &str) -> Value { + let mut pull = pr(number, state, head_ref, None); + pull["user"] = json!({ "login": login }); + pull["updated_at"] = json!(updated); + pull + } + fn refs(names: &[&str]) -> Value { + Value::Array( + names + .iter() + .map(|n| json!({ "ref": format!("refs/heads/{n}"), "object": { "sha": "x" } })) + .collect(), + ) + } + + async fn mount_refs(server: &MockServer, prefix: &str, names: &[&str]) { Mock::given(method("GET")) - .and(path("/search/issues")) - .and(query_param( - "q", - "repo:user/repo author:author is:pull-request head:prefix/", - )) - .and(query_param("per_page", "100")) - .and(query_param("sort", "updated")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "items": [ - { "number": 11 }, - { "number": 22 }, - ], - }))) - .mount(&server) + .and(path(format!( + "/repos/user/repo/git/matching-refs/heads/{prefix}/" + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(refs(names))) + .expect(1) + .mount(server) .await; + } + + async fn mount_lookup(server: &MockServer, branch: &str, pulls: Value) { Mock::given(method("GET")) - .and(path("/repos/user/repo/pulls/11")) - .respond_with(ResponseTemplate::new(200).set_body_json(pr( + .and(path("/repos/user/repo/pulls")) + .and(query_param("head", format!("user:{branch}"))) + .and(query_param("state", "all")) + .and(query_param("per_page", "100")) + .respond_with(ResponseTemplate::new(200).set_body_json(pulls)) + .expect(1) + .mount(server) + .await; + } + + /// The endpoint this module used to call. Mounted with + /// `expect(0)` so any regression back to it fails the test. + async fn forbid_search(server: &MockServer) { + Mock::given(path("/search/issues")) + .respond_with(ResponseTemplate::new(403)) + .expect(0) + .mount(server) + .await; + } + + fn client(server: &MockServer) -> Client { + Client::new( + Url::parse(&server.uri()).unwrap(), + "tok".to_string(), + ApiFlavor::GitHub, + ) + .unwrap() + } + + #[tokio::test] + async fn looks_up_each_live_stack_branch_without_the_search_api() { + let server = MockServer::start().await; + forbid_search(&server).await; + mount_refs( + &server, + "prefix", + &["prefix/feat-a--aaaaaaaa", "prefix/feat-b--bbbbbbbb"], + ) + .await; + mount_lookup( + &server, + "prefix/feat-a--aaaaaaaa", + json!([listed( 11, "open", "prefix/feat-a--aaaaaaaa", - None, - ))) - .mount(&server) - .await; - Mock::given(method("GET")) - .and(path("/repos/user/repo/pulls/22")) - .respond_with(ResponseTemplate::new(200).set_body_json(pr( + "author", + "2026-01-01T00:00:00Z" + )]), + ) + .await; + mount_lookup( + &server, + "prefix/feat-b--bbbbbbbb", + json!([listed( 22, "open", "prefix/feat-b--bbbbbbbb", - None, - ))) - .mount(&server) - .await; + "author", + "2026-01-02T00:00:00Z" + )]), + ) + .await; + + let mut progress = Vec::new(); + let got = get_remote_changes_reporting( + &client(&server), + "user", + "repo", + "prefix", + Some("author"), + &[], + |n, total| progress.push((n, total)), + ) + .await + .unwrap(); - let client = Client::new( - Url::parse(&server.uri()).unwrap(), - "tok".to_string(), - ApiFlavor::GitHub, + // Most recently updated first, as the Search API's + // `sort=updated` used to return them. + assert_eq!( + got.iter() + .map(|c| (c.change_id.as_str(), c.pull["number"].as_u64())) + .collect::>(), + vec![("bbbbbbbb", Some(22)), ("aaaaaaaa", Some(11))], + ); + assert_eq!(progress, vec![(1, 2), (2, 2)]); + } + + #[tokio::test] + async fn finds_a_merged_pull_whose_branch_was_deleted_through_the_local_slug() { + // The bottom PR merged and GitHub deleted its branch, so + // `matching-refs` no longer lists it. `stack sync` still + // has to see that PR as merged to drop the local commit. + let server = MockServer::start().await; + forbid_search(&server).await; + mount_refs(&server, "prefix", &["prefix/feat-b--bbbbbbbb"]).await; + mount_lookup( + &server, + "prefix/feat-b--bbbbbbbb", + json!([listed( + 22, + "open", + "prefix/feat-b--bbbbbbbb", + "author", + "2026-01-02T00:00:00Z" + )]), ) + .await; + let mut merged = listed( + 11, + "closed", + "prefix/feat-a--aaaaaaaa", + "author", + "2026-01-01T00:00:00Z", + ); + merged["merged_at"] = json!("2026-01-01T00:00:00Z"); + mount_lookup(&server, "prefix/feat-a--aaaaaaaa", json!([merged])).await; + + let locals = [ + local(ID_A, "feat-a--aaaaaaaa"), + local(ID_B, "feat-b--bbbbbbbb"), + ]; + let got = get_remote_changes( + &client(&server), + "user", + "repo", + "prefix", + Some("author"), + &locals, + ) + .await .unwrap(); - let got = get_remote_changes(&client, "user", "repo", "prefix", Some("author")) - .await - .unwrap(); + let merged = got + .iter() + .find(|c| c.change_id == "aaaaaaaa") + .expect("merged pull found"); + assert_eq!(merged.pull["number"], 11); + assert_eq!(merged.pull["merged_at"], "2026-01-01T00:00:00Z"); assert_eq!(got.len(), 2); - assert_eq!(got[0].change_id, "aaaaaaaa"); - assert_eq!(got[0].pull["number"], 11); - assert_eq!(got[1].change_id, "bbbbbbbb"); - assert_eq!(got[1].pull["number"], 22); } #[tokio::test] - async fn get_remote_changes_omits_author_filter_when_author_is_none() { - // `stack checkout` works on anyone's stack, so it searches - // the branch alone. The `author:` qualifier has to be - // absent entirely — an empty one (`author:`) is a search - // syntax error on GitHub's side. - use url::Url; - use wiremock::matchers::{method, path, query_param}; - use wiremock::{Mock, MockServer, ResponseTemplate}; - + async fn skips_the_slug_lookup_for_a_change_already_on_a_live_branch() { + // The commit was reworded after its PR opened: the PR keeps + // its original branch, the local slug now differs. The live + // branch already answers for that Change-Id, so the + // reworded slug is not looked up. let server = MockServer::start().await; + forbid_search(&server).await; + mount_refs(&server, "prefix", &["prefix/old-title--aaaaaaaa"]).await; + mount_lookup( + &server, + "prefix/old-title--aaaaaaaa", + json!([listed( + 11, + "open", + "prefix/old-title--aaaaaaaa", + "author", + "2026-01-01T00:00:00Z" + )]), + ) + .await; Mock::given(method("GET")) - .and(path("/search/issues")) - .and(query_param( - "q", - "repo:user/repo is:pull-request head:prefix/", - )) - .respond_with( - ResponseTemplate::new(200).set_body_json(serde_json::json!({"items": []})), - ) + .and(path("/repos/user/repo/pulls")) + .and(query_param("head", "user:prefix/new-title--aaaaaaaa")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!([]))) + .expect(0) .mount(&server) .await; - let client = Client::new( - Url::parse(&server.uri()).unwrap(), - "tok".to_string(), - ApiFlavor::GitHub, + let got = get_remote_changes( + &client(&server), + "user", + "repo", + "prefix", + Some("author"), + &[local(ID_A, "new-title--aaaaaaaa")], ) + .await .unwrap(); - let got = get_remote_changes(&client, "user", "repo", "prefix", None) + + assert_eq!(got.len(), 1); + assert_eq!(got[0].pull["number"], 11); + } + + #[tokio::test] + async fn ignores_nested_and_unmanaged_branches_under_the_prefix() { + // `prefix/sub/...` is another stack; `prefix/random` was + // never pushed by `mergify stack`. Neither is looked up. + let server = MockServer::start().await; + forbid_search(&server).await; + mount_refs( + &server, + "prefix", + &["prefix/sub/feat-x--cccccccc", "prefix/random-branch"], + ) + .await; + Mock::given(method("GET")) + .and(path("/repos/user/repo/pulls")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!([]))) + .expect(0) + .mount(&server) + .await; + + let got = get_remote_changes(&client(&server), "user", "repo", "prefix", None, &[]) + .await + .unwrap(); + + assert!(got.is_empty(), "got: {got:?}"); + } + + #[tokio::test] + async fn author_filter_is_case_insensitive_and_absent_without_an_author() { + // `stack checkout` passes no author and must see anyone's + // PR; the owner-only commands keep just their own, matching + // the login the way GitHub does (case-insensitively). + for (author, expected) in [ + (Some("AUTHOR"), vec![11]), + (Some("someone-else"), vec![]), + (None, vec![11]), + ] { + let server = MockServer::start().await; + mount_refs(&server, "prefix", &["prefix/feat-a--aaaaaaaa"]).await; + mount_lookup( + &server, + "prefix/feat-a--aaaaaaaa", + json!([listed( + 11, + "open", + "prefix/feat-a--aaaaaaaa", + "author", + "2026-01-01T00:00:00Z" + )]), + ) + .await; + + let got = get_remote_changes(&client(&server), "user", "repo", "prefix", author, &[]) + .await + .unwrap(); + + assert_eq!( + got.iter() + .filter_map(|c| c.pull["number"].as_u64()) + .collect::>(), + expected, + "author {author:?}", + ); + } + } + + #[tokio::test] + async fn keeps_the_most_recently_updated_of_two_merged_pulls_on_one_change() { + // Both merged, one Change-Id (re-opened and merged twice): + // the newest wins, whatever order the lookups ran in. + let server = MockServer::start().await; + mount_refs(&server, "prefix", &["prefix/feat-a--aaaaaaaa"]).await; + let mut older = listed( + 11, + "closed", + "prefix/feat-a--aaaaaaaa", + "author", + "2026-01-01T00:00:00Z", + ); + older["merged_at"] = json!("2026-01-01T00:00:00Z"); + let mut newer = listed( + 12, + "closed", + "prefix/feat-a--aaaaaaaa", + "author", + "2026-02-01T00:00:00Z", + ); + newer["merged_at"] = json!("2026-02-01T00:00:00Z"); + mount_lookup(&server, "prefix/feat-a--aaaaaaaa", json!([older, newer])).await; + + let got = get_remote_changes(&client(&server), "user", "repo", "prefix", None, &[]) .await .unwrap(); - assert!(got.is_empty()); + assert_eq!(got.len(), 1); + assert_eq!(got[0].pull["number"], 12); } }