From 91b88f2b00071b954d037fe834e3293a5fa1e6f3 Mon Sep 17 00:00:00 2001 From: Edmondo Porcu Date: Wed, 4 Mar 2026 09:16:23 -0500 Subject: [PATCH 1/6] fix: include error source chain in error messages (#1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: include error source chain in error messages octocrab's Error::GitHub variant uses snafu without a custom display attribute, so its Display impl outputs just "GitHub" โ€” the variant name. The actual API error message lives in the source field but was never surfaced because From only used format!("{}", error). Walk std::error::Error::source() chain so wrapped errors include the underlying message. Also add context to the merge call in land.rs with PR number and head SHA for better diagnostics. Before: ๐Ÿ›‘ GitHub After: ๐Ÿ›‘ GitHub: * trigger CI * style: cargo fmt --- spr/src/commands/land.rs | 4 +++ spr/src/error.rs | 78 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 81 insertions(+), 1 deletion(-) diff --git a/spr/src/commands/land.rs b/spr/src/commands/land.rs index f4c709c..6e53387 100644 --- a/spr/src/commands/land.rs +++ b/spr/src/commands/land.rs @@ -207,6 +207,10 @@ pub async fn land( .send() .await .convert() + .context(format!( + "squash-merging PR #{} (head {})", + pull_request_number, pr_head_oid + )) .and_then(|merge| { if merge.merged { Ok(merge) diff --git a/spr/src/error.rs b/spr/src/error.rs index edc155d..392d1df 100644 --- a/spr/src/error.rs +++ b/spr/src/error.rs @@ -46,8 +46,17 @@ where E: std::error::Error, { fn from(error: E) -> Self { + // Walk the error source chain so that wrapped errors (e.g. + // octocrab::Error::GitHub whose Display is just "GitHub") include + // the underlying message. + let mut msg = format!("{}", error); + let mut source = error.source(); + while let Some(s) = source { + msg = format!("{}: {}", msg, s); + source = s.source(); + } Self { - messages: vec![format!("{}", error)], + messages: vec![msg], } } } @@ -161,3 +170,70 @@ pub fn add_error(result: &mut Result, other: Result) -> Option { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[derive(Debug)] + struct InnerError(String); + + impl std::fmt::Display for InnerError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) + } + } + + impl std::error::Error for InnerError {} + + #[derive(Debug)] + struct OuterError { + msg: String, + source: InnerError, + } + + impl std::fmt::Display for OuterError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.msg) + } + } + + impl std::error::Error for OuterError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + Some(&self.source) + } + } + + #[test] + fn test_from_error_includes_source_chain() { + let outer = OuterError { + msg: "GitHub".into(), + source: InnerError("PR is not mergeable".into()), + }; + let error: Error = outer.into(); + assert_eq!(error.messages()[0], "GitHub: PR is not mergeable"); + } + + #[test] + fn test_from_error_without_source() { + let inner = InnerError("simple error".into()); + let error: Error = inner.into(); + assert_eq!(error.messages()[0], "simple error"); + } + + #[test] + fn test_context_appends_message() { + let result: Result<()> = Err(Error::new("original")); + let result = result.context("added context".into()); + let err = result.unwrap_err(); + assert_eq!(err.messages(), &["original", "added context"]); + } + + #[test] + fn test_reword_replaces_last_message() { + let result: Result<()> = Err(Error::new("original")); + let result = result.reword("reworded".into()); + let err = result.unwrap_err(); + assert_eq!(err.messages(), &["reworded"]); + } +} From f70ee605ba9085f5fef38dd01b678e2b5149c2dd Mon Sep 17 00:00:00 2001 From: Edmondo Porcu Date: Wed, 4 Mar 2026 09:16:35 -0500 Subject: [PATCH 2/6] feat: add dry run mode to spr diff (#2) * Adding dry run mode * Improve dry run output formatting Show richer details (base/head branches, draft status, reviewers) and suppress commit title output during dry run mode. * trigger CI * style: cargo fmt --- spr/src/commands/diff.rs | 436 ++++++++++++++++++++++++++------------- spr/src/jj.rs | 19 ++ 2 files changed, 309 insertions(+), 146 deletions(-) diff --git a/spr/src/commands/diff.rs b/spr/src/commands/diff.rs index 43375b6..a0f5a27 100644 --- a/spr/src/commands/diff.rs +++ b/spr/src/commands/diff.rs @@ -52,6 +52,10 @@ pub struct DiffOptions { /// If a range is provided, behaves like --all mode. If not specified, uses '@-'. #[clap(short = 'r', long)] revision: Option, + + /// Preview what would happen without pushing or creating PRs + #[clap(long)] + pub dry_run: bool, } pub async fn diff( @@ -117,7 +121,9 @@ pub async fn diff( None }; - write_commit_title(prepared_commit)?; + if !opts.dry_run { + write_commit_title(prepared_commit)?; + } // The further implementation of the diff command is in a separate function. // This makes it easier to run the code to update the local commit message @@ -138,10 +144,78 @@ pub async fn diff( // This updates the commit message in the local Jujutsu repository (if it was // changed by the implementation) - add_error( - &mut result, - jj.rewrite_commit_messages(prepared_commits.as_mut_slice()), - ); + if !opts.dry_run { + add_error( + &mut result, + jj.rewrite_commit_messages(prepared_commits.as_mut_slice()), + ); + } + + if opts.dry_run { + let actions: Vec<_> = prepared_commits + .iter() + .enumerate() + .filter(|(_, c)| c.dry_run_action.is_some()) + .collect(); + + output( + "\n๐Ÿ“‹", + &format!( + "Dry run complete. Would process {} change(s):\n", + actions.len() + ), + )?; + + for (idx, pc) in &actions { + let title = pc + .message + .get(&crate::message::MessageSection::Title) + .map(|t| &t[..]) + .unwrap_or(""); + let pos = idx + 1; + + let (action_label, base, head, reviewers_list) = match &pc.dry_run_action { + Some(crate::jj::DryRunAction::Create { + base, + head, + draft, + reviewers, + .. + }) => { + let label = if *draft { + "CREATE (draft)".to_string() + } else { + "CREATE".to_string() + }; + (label, base.as_str(), head.as_str(), reviewers.clone()) + } + Some(crate::jj::DryRunAction::Update { + pr_number, + base, + head, + .. + }) => { + let label = format!("UPDATE PR #{pr_number}"); + (label, base.as_str(), head.as_str(), vec![]) + } + None => continue, + }; + + output( + &format!(" #{pos}"), + &format!("{action_label} {} \"{title}\"", pc.short_id), + )?; + output(" ", &format!("head: {head}"))?; + output(" ", &format!("base: {base}"))?; + if !reviewers_list.is_empty() { + output( + " ", + &format!("reviewers: {}", reviewers_list.join(", ")), + )?; + } + output("", "")?; + } + } result } @@ -355,10 +429,17 @@ async fn diff_impl( pull_request_updates.update_message(pull_request, message); if !pull_request_updates.is_empty() { - // ...and there are actual changes to the message - gh.update_pull_request(pull_request.number, pull_request_updates) - .await?; - output("โœ", "Updated commit message on GitHub")?; + if opts.dry_run { + output( + " ", + &format!("Would update PR #{} title/body", pull_request.number), + )?; + } else { + // ...and there are actual changes to the message + gh.update_pull_request(pull_request.number, pull_request_updates) + .await?; + output("โœ", "Updated commit message on GitHub")?; + } } } @@ -440,23 +521,28 @@ async fn diff_impl( parents.push(master_base_oid); } - let new_base_branch_commit = jj.create_derived_commit( - local_commit.parent_oid, - &format!( - "[spr] {}\n\nCreated using jj-spr {}\n\n[skip ci]", - if pull_request.is_some() { - "changes introduced through rebase".to_string() - } else { - format!( - "changes to {} this commit is based on", - config.master_ref.branch_name() - ) - }, - env!("CARGO_PKG_VERSION"), - ), - new_base_tree, - &parents[..], - )?; + let new_base_branch_commit = if opts.dry_run { + // Use a placeholder OID โ€” this won't be pushed + pr_base_oid + } else { + jj.create_derived_commit( + local_commit.parent_oid, + &format!( + "[spr] {}\n\nCreated using jj-spr {}\n\n[skip ci]", + if pull_request.is_some() { + "changes introduced through rebase".to_string() + } else { + format!( + "changes to {} this commit is based on", + config.master_ref.branch_name() + ) + }, + env!("CARGO_PKG_VERSION"), + ), + new_base_tree, + &parents[..], + )? + }; // If `base_branch` is `None` (which means a base branch does not exist // yet), then make a `GitHubBranch` with a new name for a base branch @@ -470,7 +556,7 @@ async fn diff_impl( }; let mut github_commit_message = opts.message.clone(); - if pull_request.is_some() && github_commit_message.is_none() { + if pull_request.is_some() && github_commit_message.is_none() && !opts.dry_run { let input = { let message_on_prompt = message_on_prompt.clone(); @@ -508,144 +594,179 @@ async fn diff_impl( } // Create the new commit - let pr_commit = jj.create_derived_commit( - local_commit.oid, - &format!( - "{}\n\nCreated using jj-spr {}", - github_commit_message - .as_ref() - .map(|s| &s[..]) - .unwrap_or("[jj-spr] initial version"), - env!("CARGO_PKG_VERSION"), - ), - new_head_tree, - &pr_commit_parents[..], - )?; - - let mut cmd = tokio::process::Command::new("git"); - cmd.arg("push") - .arg("--atomic") - .arg("--no-verify") - .arg("--") - .arg(&config.remote_name) - .arg(format!("{}:{}", pr_commit, pull_request_branch.on_github())); - - if let Some(pull_request) = pull_request { - // We are updating an existing Pull Request - - if needs_merging_master { - output( - "โšพ", - &format!( - "Commit was rebased - updating Pull Request #{}", - pull_request.number - ), - )?; + let pr_commit = if opts.dry_run { + // Use a placeholder OID โ€” this won't be pushed + pr_head_oid + } else { + jj.create_derived_commit( + local_commit.oid, + &format!( + "{}\n\nCreated using jj-spr {}", + github_commit_message + .as_ref() + .map(|s| &s[..]) + .unwrap_or("[jj-spr] initial version"), + env!("CARGO_PKG_VERSION"), + ), + new_head_tree, + &pr_commit_parents[..], + )? + }; + + if opts.dry_run { + let base_ref = base_branch.as_ref().unwrap_or(&config.master_ref); + let base_branch_name = base_ref.branch_name(); + let head_branch_name = pull_request_branch.branch_name(); + let is_stacked = !base_ref.is_master_branch(); + + local_commit.dry_run_action = if let Some(ref pr) = pull_request { + Some(crate::jj::DryRunAction::Update { + pr_number: pr.number, + base: base_branch_name.to_string(), + head: head_branch_name.to_string(), + is_stacked, + }) } else { - output( - "๐Ÿ”", - &format!( - "Commit was changed - updating Pull Request #{}", - pull_request.number - ), - )?; - } + let all_reviewers: Vec = requested_reviewers + .reviewers + .iter() + .chain(requested_reviewers.team_reviewers.iter()) + .cloned() + .collect(); + Some(crate::jj::DryRunAction::Create { + base: base_branch_name.to_string(), + head: head_branch_name.to_string(), + is_stacked, + draft: opts.draft, + reviewers: all_reviewers, + }) + }; + } else { + let mut cmd = tokio::process::Command::new("git"); + cmd.arg("push") + .arg("--atomic") + .arg("--no-verify") + .arg("--") + .arg(&config.remote_name) + .arg(format!("{}:{}", pr_commit, pull_request_branch.on_github())); + + if let Some(pull_request) = pull_request { + // We are updating an existing Pull Request + + if needs_merging_master { + output( + "โšพ", + &format!( + "Commit was rebased - updating Pull Request #{}", + pull_request.number + ), + )?; + } else { + output( + "๐Ÿ”", + &format!( + "Commit was changed - updating Pull Request #{}", + pull_request.number + ), + )?; + } - // Things we want to update in the Pull Request on GitHub - let mut pull_request_updates: PullRequestUpdate = Default::default(); + // Things we want to update in the Pull Request on GitHub + let mut pull_request_updates: PullRequestUpdate = Default::default(); - if opts.update_message { - pull_request_updates.update_message(&pull_request, message); - } + if opts.update_message { + pull_request_updates.update_message(&pull_request, message); + } + + if let Some(base_branch) = base_branch { + // We are using a base branch. + + if let Some(base_branch_commit) = pr_base_parent { + // ...and we prepared a new commit for it, so we need to push an + // update of the base branch. + cmd.arg(format!( + "{}:{}", + base_branch_commit, + base_branch.on_github() + )); + } + + // Push the new commit onto the Pull Request branch (and also the + // new base commit, if we added that to cmd above). + run_command(&mut cmd) + .await + .reword("git push failed".to_string())?; + + // If the Pull Request's base is not set to the base branch yet, + // change that now. + if pull_request.base.branch_name() != base_branch.branch_name() { + pull_request_updates.base = Some(base_branch.branch_name().to_string()); + } + } else { + // The Pull Request is against the master branch. In that case we + // only need to push the update to the Pull Request branch. + run_command(&mut cmd) + .await + .reword("git push failed".to_string())?; + } - if let Some(base_branch) = base_branch { - // We are using a base branch. + if !pull_request_updates.is_empty() { + gh.update_pull_request(pull_request.number, pull_request_updates) + .await?; + } + } else { + // We are creating a new Pull Request. - if let Some(base_branch_commit) = pr_base_parent { - // ...and we prepared a new commit for it, so we need to push an - // update of the base branch. + // If there's a base branch, add it to the push + if let (Some(base_branch), Some(base_branch_commit)) = (&base_branch, pr_base_parent) { cmd.arg(format!( "{}:{}", base_branch_commit, base_branch.on_github() )); } - - // Push the new commit onto the Pull Request branch (and also the - // new base commit, if we added that to cmd above). - run_command(&mut cmd) - .await - .reword("git push failed".to_string())?; - - // If the Pull Request's base is not set to the base branch yet, - // change that now. - if pull_request.base.branch_name() != base_branch.branch_name() { - pull_request_updates.base = Some(base_branch.branch_name().to_string()); - } - } else { - // The Pull Request is against the master branch. In that case we - // only need to push the update to the Pull Request branch. + // Push the pull request branch and the base branch if present run_command(&mut cmd) .await .reword("git push failed".to_string())?; - } - if !pull_request_updates.is_empty() { - gh.update_pull_request(pull_request.number, pull_request_updates) + // Then call GitHub to create the Pull Request. + let pull_request_number = gh + .create_pull_request( + message, + base_branch + .as_ref() + .unwrap_or(&config.master_ref) + .branch_name() + .to_string(), + pull_request_branch.branch_name().to_string(), + opts.draft, + ) .await?; - } - } else { - // We are creating a new Pull Request. - - // If there's a base branch, add it to the push - if let (Some(base_branch), Some(base_branch_commit)) = (&base_branch, pr_base_parent) { - cmd.arg(format!( - "{}:{}", - base_branch_commit, - base_branch.on_github() - )); - } - // Push the pull request branch and the base branch if present - run_command(&mut cmd) - .await - .reword("git push failed".to_string())?; - - // Then call GitHub to create the Pull Request. - let pull_request_number = gh - .create_pull_request( - message, - base_branch - .as_ref() - .unwrap_or(&config.master_ref) - .branch_name() - .to_string(), - pull_request_branch.branch_name().to_string(), - opts.draft, - ) - .await?; - - let pull_request_url = config.pull_request_url(pull_request_number); - output( - "โœจ", - &format!( - "Created new Pull Request #{}: {}", - pull_request_number, &pull_request_url, - ), - )?; + let pull_request_url = config.pull_request_url(pull_request_number); - message.insert(MessageSection::PullRequest, pull_request_url); - local_commit.message_changed = true; + output( + "โœจ", + &format!( + "Created new Pull Request #{}: {}", + pull_request_number, &pull_request_url, + ), + )?; - let result = gh - .request_reviewers(pull_request_number, requested_reviewers) - .await; - match result { - Ok(()) => (), - Err(error) => { - output("โš ๏ธ", "Requesting reviewers failed")?; - for message in error.messages() { - output(" ", message)?; + message.insert(MessageSection::PullRequest, pull_request_url); + local_commit.message_changed = true; + + let result = gh + .request_reviewers(pull_request_number, requested_reviewers) + .await; + match result { + Ok(()) => (), + Err(error) => { + output("โš ๏ธ", "Requesting reviewers failed")?; + for message in error.messages() { + output(" ", message)?; + } } } } @@ -747,6 +868,7 @@ mod tests { cherry_pick: false, base: None, revision: None, + dry_run: false, }; assert!(!opts.all); @@ -767,6 +889,7 @@ mod tests { cherry_pick: false, base: Some("main".to_string()), revision: None, + dry_run: false, }; assert_eq!(opts.base, Some("main".to_string())); @@ -792,6 +915,7 @@ mod tests { cherry_pick: false, base: Some("main".to_string()), revision: None, + dry_run: false, }; assert_eq!(opts_with_base.base.as_deref(), Some("main")); @@ -805,6 +929,7 @@ mod tests { cherry_pick: false, base: Some("trunk()".to_string()), revision: None, + dry_run: false, }; assert_eq!(opts_with_trunk.base.as_deref(), Some("trunk()")); @@ -820,6 +945,7 @@ mod tests { cherry_pick: false, base: Some("trunk()".to_string()), revision: None, + dry_run: false, }; // When --all is specified, it should work with base revisions @@ -838,6 +964,7 @@ mod tests { cherry_pick: false, base: Some("trunk()".to_string()), revision: None, + dry_run: false, }; assert!(opts.all); @@ -848,6 +975,23 @@ mod tests { assert_eq!(opts.base.as_deref(), Some("trunk()")); } + #[test] + fn test_diff_options_dry_run_flag() { + let opts = DiffOptions { + all: false, + update_message: false, + draft: false, + message: None, + cherry_pick: false, + base: None, + revision: None, + dry_run: true, + }; + + assert!(opts.dry_run); + assert!(!opts.all); + } + // Integration tests would require more complex setup with actual Git repositories // and proper mocking of GitHub API calls. The tests above focus on: // 1. Option parsing and validation diff --git a/spr/src/jj.rs b/spr/src/jj.rs index b78d474..cbef8aa 100644 --- a/spr/src/jj.rs +++ b/spr/src/jj.rs @@ -18,6 +18,23 @@ use crate::{ }; use git2::Oid; +#[derive(Debug, Clone)] +pub enum DryRunAction { + Create { + base: String, + head: String, + is_stacked: bool, + draft: bool, + reviewers: Vec, + }, + Update { + pr_number: u64, + base: String, + head: String, + is_stacked: bool, + }, +} + #[derive(Debug)] pub struct PreparedCommit { pub oid: Oid, @@ -26,6 +43,7 @@ pub struct PreparedCommit { pub message: MessageSectionsMap, pub pull_request_number: Option, pub message_changed: bool, + pub dry_run_action: Option, } pub struct Jujutsu { @@ -279,6 +297,7 @@ impl Jujutsu { message, pull_request_number, message_changed: false, + dry_run_action: None, }) } From cf9ce076935a1b4847d11307a6c4d2a346d7ddef Mon Sep 17 00:00:00 2001 From: Edmondo Porcu Date: Wed, 4 Mar 2026 09:16:57 -0500 Subject: [PATCH 3/6] feat: add jj spr cleanup command (#3) * Add jj spr cleanup command List and delete orphan SPR branches on the remote that are no longer associated with any open pull request. Dry-run by default; pass --confirm to actually delete. Extract branch filtering logic into testable functions with unit tests. * trigger CI * style: cargo fmt --- spr/src/commands/cleanup.rs | 229 ++++++++++++++++++ spr/src/commands/mod.rs | 1 + spr/src/github.rs | 60 +++++ .../gql/open_pull_request_branches.graphql | 15 ++ spr/src/main.rs | 4 + 5 files changed, 309 insertions(+) create mode 100644 spr/src/commands/cleanup.rs create mode 100644 spr/src/gql/open_pull_request_branches.graphql diff --git a/spr/src/commands/cleanup.rs b/spr/src/commands/cleanup.rs new file mode 100644 index 0000000..00b67d1 --- /dev/null +++ b/spr/src/commands/cleanup.rs @@ -0,0 +1,229 @@ +/* + * Copyright (c) Radical HQ Limited + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +use std::collections::HashSet; +use std::process::Stdio; + +use crate::{error::Result, output::output}; + +#[derive(Debug, clap::Parser)] +pub struct CleanupOptions { + /// Actually delete the orphan branches (default is list-only) + #[clap(long)] + confirm: bool, +} + +/// Extract branch names from refs that match the SPR remote prefix. +/// +/// Given refs like `refs/remotes/origin/spr/user/my-feature`, extracts +/// `spr/user/my-feature`. +fn extract_spr_branch_names( + all_refs: &HashSet, + remote_name: &str, + branch_prefix: &str, +) -> Vec { + let remote_prefix = format!("refs/remotes/{}/{}", remote_name, branch_prefix); + let strip_len = "refs/remotes/".len() + remote_name.len() + 1; + + all_refs + .iter() + .filter(|r| r.starts_with(&remote_prefix)) + .map(|r| r[strip_len..].to_string()) + .collect() +} + +/// Find SPR branches that are not referenced by any open PR. +fn find_orphan_branches<'a>( + spr_branches: &'a [String], + open_pr_branches: &HashSet, +) -> Vec<&'a String> { + spr_branches + .iter() + .filter(|b| !open_pr_branches.contains(*b)) + .collect() +} + +pub async fn cleanup( + opts: CleanupOptions, + jj: &crate::jj::Jujutsu, + gh: &crate::github::GitHub, + config: &crate::config::Config, +) -> Result<()> { + output("๐Ÿ”", "Finding orphan SPR branches...")?; + + let all_refs = jj.get_all_ref_names()?; + let spr_branches = + extract_spr_branch_names(&all_refs, &config.remote_name, &config.branch_prefix); + + if spr_branches.is_empty() { + output("โœจ", "No SPR branches found. Nothing to clean up.")?; + return Ok(()); + } + + let open_pr_branches = gh.get_open_pr_branch_names().await?; + let orphan_branches = find_orphan_branches(&spr_branches, &open_pr_branches); + + if orphan_branches.is_empty() { + output( + "โœจ", + &format!( + "All {} SPR branch(es) belong to open PRs. Nothing to clean up.", + spr_branches.len() + ), + )?; + return Ok(()); + } + + output( + "๐Ÿ—‘๏ธ", + &format!( + "Found {} orphan SPR branch(es) (out of {} total):", + orphan_branches.len(), + spr_branches.len() + ), + )?; + + let term = console::Term::stdout(); + for branch in &orphan_branches { + term.write_line(&format!(" {}", console::style(*branch).dim()))?; + } + + if !opts.confirm { + output("๐Ÿ’ก", "Run with --confirm to delete these branches.")?; + return Ok(()); + } + + output("๐Ÿงน", "Deleting orphan branches...")?; + + for branch in &orphan_branches { + let result = tokio::process::Command::new("git") + .arg("push") + .arg("--no-verify") + .arg("--delete") + .arg("--") + .arg(&config.remote_name) + .arg(format!("refs/heads/{}", branch)) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .output() + .await; + + match result { + Ok(status) if status.status.success() => { + output("โœ…", &format!("Deleted {}", branch))?; + } + _ => { + output( + "โš ๏ธ", + &format!("Failed to delete {} (may already be gone)", branch), + )?; + } + } + } + + output("โœจ", "Cleanup complete.")?; + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_extract_spr_branch_names_filters_by_prefix() { + let refs: HashSet = [ + "refs/remotes/origin/spr/user/my-feature", + "refs/remotes/origin/spr/user/main.my-feature", + "refs/remotes/origin/main", + "refs/remotes/origin/other-branch", + "refs/heads/local-branch", + ] + .iter() + .map(|s| s.to_string()) + .collect(); + + let mut branches = extract_spr_branch_names(&refs, "origin", "spr/user/"); + branches.sort(); + + assert_eq!( + branches, + vec!["spr/user/main.my-feature", "spr/user/my-feature"] + ); + } + + #[test] + fn test_extract_spr_branch_names_empty_when_no_match() { + let refs: HashSet = ["refs/remotes/origin/main", "refs/remotes/origin/feature"] + .iter() + .map(|s| s.to_string()) + .collect(); + + let branches = extract_spr_branch_names(&refs, "origin", "spr/user/"); + assert!(branches.is_empty()); + } + + #[test] + fn test_extract_spr_branch_names_respects_remote_name() { + let refs: HashSet = [ + "refs/remotes/origin/spr/user/feat", + "refs/remotes/upstream/spr/user/feat", + ] + .iter() + .map(|s| s.to_string()) + .collect(); + + let branches = extract_spr_branch_names(&refs, "upstream", "spr/user/"); + assert_eq!(branches, vec!["spr/user/feat"]); + } + + #[test] + fn test_find_orphan_branches_identifies_orphans() { + let spr_branches: Vec = vec![ + "spr/user/feat-a".into(), + "spr/user/main.feat-a".into(), + "spr/user/feat-b".into(), + "spr/user/feat-c".into(), + ]; + + let open_pr_branches: HashSet = ["spr/user/feat-a", "spr/user/main.feat-a"] + .iter() + .map(|s| s.to_string()) + .collect(); + + let mut orphans: Vec<&str> = find_orphan_branches(&spr_branches, &open_pr_branches) + .into_iter() + .map(|s| s.as_str()) + .collect(); + orphans.sort(); + + assert_eq!(orphans, vec!["spr/user/feat-b", "spr/user/feat-c"]); + } + + #[test] + fn test_find_orphan_branches_none_when_all_active() { + let spr_branches: Vec = + vec!["spr/user/feat-a".into(), "spr/user/main.feat-a".into()]; + + let open_pr_branches: HashSet = ["spr/user/feat-a", "spr/user/main.feat-a"] + .iter() + .map(|s| s.to_string()) + .collect(); + + let orphans = find_orphan_branches(&spr_branches, &open_pr_branches); + assert!(orphans.is_empty()); + } + + #[test] + fn test_find_orphan_branches_all_orphans_when_no_open_prs() { + let spr_branches: Vec = vec!["spr/user/feat-a".into(), "spr/user/feat-b".into()]; + + let open_pr_branches: HashSet = HashSet::new(); + + let orphans = find_orphan_branches(&spr_branches, &open_pr_branches); + assert_eq!(orphans.len(), 2); + } +} diff --git a/spr/src/commands/mod.rs b/spr/src/commands/mod.rs index b166970..fbb9a28 100644 --- a/spr/src/commands/mod.rs +++ b/spr/src/commands/mod.rs @@ -6,6 +6,7 @@ */ pub mod amend; +pub mod cleanup; pub mod close; pub mod diff; pub mod format; diff --git a/spr/src/github.rs b/spr/src/github.rs index 1b37f94..6360c0f 100644 --- a/spr/src/github.rs +++ b/spr/src/github.rs @@ -119,6 +119,14 @@ type GitObjectID = String; )] pub struct PullRequestMergeabilityQuery; +#[derive(GraphQLQuery)] +#[graphql( + schema_path = "src/gql/schema.docs.graphql", + query_path = "src/gql/open_pull_request_branches.graphql", + response_derives = "Debug" +)] +pub struct OpenPullRequestBranchesQuery; + impl GitHub { pub fn new(config: crate::config::Config, graphql_client: reqwest::Client) -> Self { Self { @@ -453,6 +461,58 @@ impl GitHub { .and_then(|sha| git2::Oid::from_str(&sha.oid).ok()), }) } + + pub async fn get_open_pr_branch_names(&self) -> Result> { + let mut branch_names = HashSet::new(); + let mut after: Option = None; + + loop { + let variables = open_pull_request_branches_query::Variables { + owner: self.config.owner.clone(), + name: self.config.repo.clone(), + first: 100, + after: after.clone(), + }; + let request_body = OpenPullRequestBranchesQuery::build_query(variables); + let res = self + .graphql_client + .post("https://api.github.com/graphql") + .json(&request_body) + .send() + .await?; + let response_body: Response = + res.json().await?; + + if let Some(errors) = response_body.errors { + let error = Err(Error::new("fetching open PR branches failed".to_string())); + return errors + .into_iter() + .fold(error, |err, e| err.context(e.to_string())); + } + + let prs = response_body + .data + .ok_or_else(|| Error::new("failed to fetch open PRs"))? + .repository + .ok_or_else(|| Error::new("failed to find repository"))? + .pull_requests; + + if let Some(nodes) = prs.nodes { + for node in nodes.into_iter().flatten() { + branch_names.insert(node.head_ref_name); + branch_names.insert(node.base_ref_name); + } + } + + if prs.page_info.has_next_page { + after = prs.page_info.end_cursor; + } else { + break; + } + } + + Ok(branch_names) + } } #[derive(Debug, Clone)] diff --git a/spr/src/gql/open_pull_request_branches.graphql b/spr/src/gql/open_pull_request_branches.graphql new file mode 100644 index 0000000..cc68c72 --- /dev/null +++ b/spr/src/gql/open_pull_request_branches.graphql @@ -0,0 +1,15 @@ +query OpenPullRequestBranchesQuery($owner: String!, $name: String!, $first: Int!, $after: String) { + repository(owner: $owner, name: $name) { + pullRequests(states: [OPEN], first: $first, after: $after) { + nodes { + number + headRefName + baseRefName + } + pageInfo { + hasNextPage + endCursor + } + } + } +} diff --git a/spr/src/main.rs b/spr/src/main.rs index 05cd87a..d78fbd0 100644 --- a/spr/src/main.rs +++ b/spr/src/main.rs @@ -72,6 +72,9 @@ enum Commands { /// Close a Pull request Close(commands::close::CloseOptions), + + /// Remove orphan SPR branches from the remote + Cleanup(commands::cleanup::CleanupOptions), } #[derive(Debug, thiserror::Error)] @@ -196,6 +199,7 @@ pub async fn spr() -> Result<()> { Commands::List => commands::list::list(graphql_client, &config).await?, Commands::Patch(opts) => commands::patch::patch(opts, &jj, &mut gh, &config).await?, Commands::Close(opts) => commands::close::close(opts, &jj, &mut gh, &config).await?, + Commands::Cleanup(opts) => commands::cleanup::cleanup(opts, &jj, &gh, &config).await?, // The following commands are executed above and return from this // function before it reaches this match. Commands::Init | Commands::Format(_) => (), From d9166c3fe515124467073aadcdf84616134d8dfb Mon Sep 17 00:00:00 2001 From: Edmondo Porcu Date: Tue, 3 Mar 2026 13:21:04 -0500 Subject: [PATCH 4/6] Retarget PR to master when commit is now directly on master After the bottom of a stack is landed and the remaining stack is rebased onto master, jj spr diff now detects that the next PR's commit is directly on master and retargets the PR from its synthetic base to master. The old synthetic base branch is deleted as part of cleanup. Previously, diff would always keep an existing synthetic base even when the commit was rebased directly onto master, leaving the PR targeting a stale synthetic branch. Extract determine_base_branch() for testability. --- spr/src/commands/diff.rs | 186 +++++++++++++++++++++++++++++++++------ 1 file changed, 158 insertions(+), 28 deletions(-) diff --git a/spr/src/commands/diff.rs b/spr/src/commands/diff.rs index a0f5a27..c3f3660 100644 --- a/spr/src/commands/diff.rs +++ b/spr/src/commands/diff.rs @@ -6,6 +6,7 @@ */ use std::iter::zip; +use std::process::Stdio; use crate::{ error::{Error, Result, ResultExt, add_error}, @@ -220,6 +221,43 @@ pub async fn diff( result } +/// Determine whether an existing PR's base branch should be kept, dropped, or +/// is absent. Returns `(base_branch, old_synthetic_base)`. +/// +/// - `base_branch`: `Some` if the PR should keep (or gain) a synthetic base, +/// `None` if the PR should target master. +/// - `old_synthetic_base`: `Some` if the PR currently has a synthetic base that +/// may need cleanup (retarget + delete). +fn determine_base_branch( + pull_request: Option<&PullRequest>, + directly_based_on_master: bool, + cherry_pick: bool, +) -> ( + Option, + Option, +) { + let old_synthetic_base = pull_request.and_then(|pr| { + if !pr.base.is_master_branch() { + Some(pr.base.clone()) + } else { + None + } + }); + + let base_branch = pull_request.and_then(|pr| { + if pr.base.is_master_branch() { + None + } else if directly_based_on_master || cherry_pick { + // Commit is now directly on master โ€” drop the synthetic base + None + } else { + Some(pr.base.clone()) + } + }); + + (base_branch, old_synthetic_base) +} + #[allow(clippy::too_many_arguments)] async fn diff_impl( opts: &DiffOptions, @@ -447,17 +485,11 @@ async fn diff_impl( } } - // Check if there is a base branch on GitHub already. That's the case when - // there is an existing Pull Request, and its base is not the master branch. - let base_branch = if let Some(ref pr) = pull_request { - if pr.base.is_master_branch() { - None - } else { - Some(pr.base.clone()) - } - } else { - None - }; + let (base_branch, old_synthetic_base) = determine_base_branch( + pull_request.as_ref(), + directly_based_on_master, + opts.cherry_pick, + ); // We are going to construct `pr_base_parent: Option`. // The value will be the commit we have to merge into the new Pull Request @@ -472,11 +504,12 @@ async fn diff_impl( // not rebased. We don't need to merge anything into the Pull Request // branch. // (2) the parent tree has changed, but the parent of the local commit is on - // master (or we are cherry-picking) and we are not already using a base - // branch: in this case we can merge the master commit we are based on - // into the PR branch, without going via a base branch. Thus, we don't - // introduce a base branch here and the PR continues to target the - // master branch. + // master (or we are cherry-picking) and we are not using a base branch: + // in this case we can merge the master commit we are based on into the + // PR branch, without going via a base branch. This also applies when + // the PR previously had a synthetic base but the commit is now directly + // on master (e.g. after the bottom of a stack was landed). In that case + // the synthetic base is dropped and the PR is retargeted to master. // (3) the parent tree has changed, and we need to use a base branch (either // because one was already created earlier, or we find that we are not // directly based on master now): we need to construct a new commit for @@ -703,11 +736,40 @@ async fn diff_impl( pull_request_updates.base = Some(base_branch.branch_name().to_string()); } } else { - // The Pull Request is against the master branch. In that case we - // only need to push the update to the Pull Request branch. + // The Pull Request is against the master branch (or we are + // retargeting it to master). In that case we only need to push the + // update to the Pull Request branch. run_command(&mut cmd) .await .reword("git push failed".to_string())?; + + // If the PR previously had a synthetic base, retarget to master + // and clean up the old synthetic base branch. + if let Some(ref old_base) = old_synthetic_base { + pull_request_updates.base = Some(config.master_ref.branch_name().to_string()); + + output( + "๐ŸŽฏ", + &format!( + "Retargeting Pull Request #{} to {}", + pull_request.number, + config.master_ref.branch_name() + ), + )?; + + // Delete the old synthetic base branch (best-effort) + let _ = tokio::process::Command::new("git") + .arg("push") + .arg("--no-verify") + .arg("--delete") + .arg("--") + .arg(&config.remote_name) + .arg(old_base.on_github()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .output() + .await; + } } if !pull_request_updates.is_empty() { @@ -992,14 +1054,82 @@ mod tests { assert!(!opts.all); } - // Integration tests would require more complex setup with actual Git repositories - // and proper mocking of GitHub API calls. The tests above focus on: - // 1. Option parsing and validation - // 2. Data structure correctness - // 3. Basic logic flow verification - // - // For full integration testing, consider: - // - Mocking GitHub API responses - // - Creating test repositories with specific commit structures - // - Testing the interaction between revision specification and commit preparation + fn make_test_pr(base_branch_name: &str) -> PullRequest { + PullRequest { + number: 42, + state: PullRequestState::Open, + title: "test".to_string(), + body: None, + sections: Default::default(), + base: crate::github::GitHubBranch::new_from_branch_name( + base_branch_name, + "origin", + "main", + ), + head: crate::github::GitHubBranch::new_from_branch_name( + "spr/test/my-feature", + "origin", + "main", + ), + base_oid: git2::Oid::zero(), + head_oid: git2::Oid::zero(), + merge_commit: None, + reviewers: Default::default(), + review_status: None, + } + } + + #[test] + fn test_determine_base_branch_no_pr_returns_none() { + let (base, old) = determine_base_branch(None, true, false); + assert!(base.is_none()); + assert!(old.is_none()); + } + + #[test] + fn test_determine_base_branch_pr_on_master_returns_none() { + let pr = make_test_pr("main"); + let (base, old) = determine_base_branch(Some(&pr), true, false); + assert!(base.is_none()); + assert!(old.is_none()); + } + + #[test] + fn test_determine_base_branch_stacked_pr_not_on_master_keeps_synthetic() { + let pr = make_test_pr("spr/test/main.parent-feature"); + let (base, old) = determine_base_branch(Some(&pr), false, false); + assert_eq!(base.unwrap().branch_name(), "spr/test/main.parent-feature"); + assert_eq!(old.unwrap().branch_name(), "spr/test/main.parent-feature"); + } + + #[test] + fn test_determine_base_branch_drops_synthetic_when_directly_on_master() { + let pr = make_test_pr("spr/test/main.parent-feature"); + let (base, old) = determine_base_branch(Some(&pr), true, false); + assert!( + base.is_none(), + "should drop synthetic base when directly on master" + ); + assert_eq!( + old.unwrap().branch_name(), + "spr/test/main.parent-feature", + "should remember old synthetic for cleanup" + ); + } + + #[test] + fn test_determine_base_branch_drops_synthetic_when_cherry_pick() { + let pr = make_test_pr("spr/test/main.parent-feature"); + let (base, old) = determine_base_branch(Some(&pr), false, true); + assert!(base.is_none(), "should drop synthetic base on cherry-pick"); + assert!(old.is_some(), "should remember old synthetic for cleanup"); + } + + #[test] + fn test_determine_base_branch_pr_on_master_not_directly_on_master() { + let pr = make_test_pr("main"); + let (base, old) = determine_base_branch(Some(&pr), false, false); + assert!(base.is_none(), "PR already on master stays on master"); + assert!(old.is_none(), "no synthetic base to clean up"); + } } From 798f349116eb78462a84d9cd0630a7cbc0e09f16 Mon Sep 17 00:00:00 2001 From: Edmondo Porcu Date: Wed, 4 Mar 2026 07:25:51 -0500 Subject: [PATCH 5/6] trigger CI From 4068da5b12e09aa9e3dfb817b2d7d10af1a03a25 Mon Sep 17 00:00:00 2001 From: Edmondo Porcu Date: Thu, 5 Mar 2026 17:47:48 -0500 Subject: [PATCH 6/6] fix: stop deleting old synthetic base branch on retarget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting the synthetic base branch before retargeting the PR to master causes GitHub to close the PR, since its base branch no longer exists. Remove the deletion entirely โ€” orphan branches can be cleaned up via `jj spr cleanup` instead. Co-Authored-By: Claude Opus 4.6 --- spr/src/commands/diff.rs | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/spr/src/commands/diff.rs b/spr/src/commands/diff.rs index c3f3660..b30465f 100644 --- a/spr/src/commands/diff.rs +++ b/spr/src/commands/diff.rs @@ -6,7 +6,6 @@ */ use std::iter::zip; -use std::process::Stdio; use crate::{ error::{Error, Result, ResultExt, add_error}, @@ -227,7 +226,7 @@ pub async fn diff( /// - `base_branch`: `Some` if the PR should keep (or gain) a synthetic base, /// `None` if the PR should target master. /// - `old_synthetic_base`: `Some` if the PR currently has a synthetic base that -/// may need cleanup (retarget + delete). +/// needs retargeting to master. fn determine_base_branch( pull_request: Option<&PullRequest>, directly_based_on_master: bool, @@ -743,9 +742,8 @@ async fn diff_impl( .await .reword("git push failed".to_string())?; - // If the PR previously had a synthetic base, retarget to master - // and clean up the old synthetic base branch. - if let Some(ref old_base) = old_synthetic_base { + // If the PR previously had a synthetic base, retarget to master. + if old_synthetic_base.is_some() { pull_request_updates.base = Some(config.master_ref.branch_name().to_string()); output( @@ -756,19 +754,6 @@ async fn diff_impl( config.master_ref.branch_name() ), )?; - - // Delete the old synthetic base branch (best-effort) - let _ = tokio::process::Command::new("git") - .arg("push") - .arg("--no-verify") - .arg("--delete") - .arg("--") - .arg(&config.remote_name) - .arg(old_base.on_github()) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .output() - .await; } }