From eed9710bb03f5beb4cc05dec8cb1e5febd03be96 Mon Sep 17 00:00:00 2001 From: David Manca <1900179+medzernik@users.noreply.github.com> Date: Fri, 7 Aug 2026 16:12:52 +0200 Subject: [PATCH 01/11] add `squash_and_approve` command Added a new command that can squash and, if successful, approve a the latest resulting commit Signed-off-by: David Manca <1900179+medzernik@users.noreply.github.com> --- src/bors/command/mod.rs | 17 +++- src/bors/handlers/mod.rs | 108 +++++++++++++++++++++++- src/bors/handlers/review.rs | 5 +- src/bors/handlers/squash.rs | 30 ++++--- src/bors/handlers/squash_and_approve.rs | 40 +++++++++ src/bors/mod.rs | 1 + 6 files changed, 184 insertions(+), 17 deletions(-) create mode 100644 src/bors/handlers/squash_and_approve.rs diff --git a/src/bors/command/mod.rs b/src/bors/command/mod.rs index cf24bff3..80476e17 100644 --- a/src/bors/command/mod.rs +++ b/src/bors/command/mod.rs @@ -174,5 +174,20 @@ pub enum BorsCommand { /// Cancel an auto build currently running on a given PR (without removing it from the queue). Cancel, /// Squash all commits of a pull request into a single commit. - Squash { commit_message: SquashCommitMessage }, + Squash { + /// Squash message for the commit + commit_message: SquashCommitMessage, + }, + SquashAndApprove { + /// Squash message for the commit + commit_message: SquashCommitMessage, + /// Who is approving the commit. + approver: Approver, + /// Priority of the commit. + priority: Option, + /// Rollup status of the commit. + rollup: Option, + /// Optional note attached at the end of the command. + note: Option, + }, } diff --git a/src/bors/handlers/mod.rs b/src/bors/handlers/mod.rs index a7819ee9..8f849d40 100644 --- a/src/bors/handlers/mod.rs +++ b/src/bors/handlers/mod.rs @@ -15,6 +15,7 @@ use crate::bors::handlers::refresh::{ use crate::bors::handlers::review::{ TreeCloseArguments, command_approve, command_close_tree, command_open_tree, command_unapprove, }; +use crate::bors::handlers::squash::SquashResult; use crate::bors::handlers::trybuild::{command_try_build, command_try_cancel}; use crate::bors::handlers::workflow::{ AutoBuildCancelReason, handle_workflow_completed, handle_workflow_job_completed, @@ -55,6 +56,7 @@ mod pr_events; mod refresh; mod review; mod squash; +mod squash_and_approve; mod trybuild; mod workflow; @@ -487,6 +489,7 @@ async fn handle_comment( rollup, note, senders.merge_queue(), + pr.github.head.sha.clone(), ) .instrument(span) .await @@ -652,7 +655,8 @@ async fn handle_comment( senders.gitops_queue(), ) .instrument(span) - .await + .await?; + Ok(()) } else { repo.client .post_comment( @@ -668,6 +672,108 @@ async fn handle_comment( Ok(()) } } + BorsCommand::SquashAndApprove { + commit_message, + approver, + priority, + rollup, + note, + } => { + let span = tracing::info_span!("SquashAndApprove"); + + if ctx.local_git_available() { + let span_cloned = span.clone(); + let ctx_cloned = ctx.clone(); + let ctx2_cloned = ctx.clone(); + let comment_author_cloned = comment.author.clone(); + let comment2_author_cloned = comment.author.clone(); + let senders_cloned = senders.clone(); + let senders2_cloned = senders.clone(); + let db_cloned = database.clone(); + let db2_cloned = database.clone(); + let pr2_github_cloned = pr_github.clone(); + + let pr_cloned = PullRequestData { + github: &(pr_github.clone()), + db: { + &database + .clone() + .get_pull_request(repo.repository(), pr_number) + .await? + .unwrap() + }, + }; + + squash_and_approve::command_squash_and_approve( + repo.clone(), + db_cloned.clone(), + pr_cloned, + &comment_author_cloned.clone(), + commit_message, + ctx_cloned.parser.prefix(), + senders_cloned.gitops_queue(), + Box::new(move |squash_result: SquashResult| { + let ctx3_cloned = ctx2_cloned.clone(); + let span2_cloned = span_cloned.clone(); + let db3_cloned = db2_cloned.clone(); + let comment3_author_cloned = comment2_author_cloned.clone(); + let rollup_cloned = rollup; + let priority_cloned = priority; + let note_cloned = note.clone(); + let approver_cloned = approver.clone(); + let repo_cloned = repo.clone(); + let senders3_cloned = senders2_cloned.clone(); + let pr3_github_cloned = pr2_github_cloned.clone(); + + Box::pin(async move { + let pr_db_cloned = db3_cloned + .get_pull_request(repo_cloned.repository(), pr_number) + .await? + .unwrap(); + + let pr2_cloned = PullRequestData { + github: &pr3_github_cloned.clone(), + db: { &pr_db_cloned }, + }; + command_approve( + ctx3_cloned.clone(), + repo_cloned.clone(), + db3_cloned.clone(), + pr2_cloned, + &comment3_author_cloned.clone(), + &approver_cloned.clone(), + priority_cloned, + rollup_cloned, + note_cloned.clone(), + senders3_cloned.clone().merge_queue(), + squash_result.sha.unwrap_or(Err(anyhow::anyhow!( + "missing sha from squash" + ))?), + ) + .instrument(span2_cloned.clone()) + .await + }) + }), + ) + .instrument(span.clone()) + .await + } else { + repo.client + .post_comment( + pr_number, + Comment::new( + "`@bors squash` is not enabled in this bors instance.\ + Cancelling command, to just approve use the `r+`\ + command instead." + .to_string(), + ), + &ctx.db, + ) + .instrument(span) + .await?; + return Ok(()); + } + } }; if result.is_err() { return result.context("Cannot execute Bors command"); diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index 6575429c..3829e2f7 100644 --- a/src/bors/handlers/review.rs +++ b/src/bors/handlers/review.rs @@ -35,6 +35,7 @@ pub(super) async fn command_approve( rollup_mode: Option, note: Option, merge_queue_tx: &MergeQueueSender, + sha: CommitSha, ) -> anyhow::Result<()> { tracing::info!("Approving PR {}", pr.number()); if !has_permission(&repo_state, author, pr, PermissionType::Review).await? { @@ -79,7 +80,7 @@ pub(super) async fn command_approve( let approval_info = ApprovalInfo { approver: approver.clone(), - sha: pr.github.head.sha.to_string(), + sha: sha.to_string(), }; db.approve(pr.db, approval_info, priority, rollup_mode, note) @@ -125,7 +126,7 @@ pub(super) async fn command_approve( approved_comment( ctx.get_web_url(), repo_state.repository(), - &pr.github.head.sha, + &sha, &approver, unknown_reviewers, tree_state, diff --git a/src/bors/handlers/squash.rs b/src/bors/handlers/squash.rs index 83aa1da4..8888e987 100644 --- a/src/bors/handlers/squash.rs +++ b/src/bors/handlers/squash.rs @@ -14,7 +14,7 @@ use crate::bors::{ use crate::database::BuildStatus; use crate::github::api::CommitAuthor; use crate::github::api::operations::Commit; -use crate::github::{GithubRepoName, GithubUser}; +use crate::github::{CommitSha, GithubRepoName, GithubUser}; use crate::permissions::PermissionType; use std::collections::HashSet; use std::fmt::Write; @@ -22,6 +22,10 @@ use std::sync::Arc; const CO_AUTHORED_BY_TRAILER: &str = "Co-authored-by"; +pub struct SquashResult { + pub(crate) sha: Option, +} + /// Entry point for the squash command. /// This function validates the command and enqueues the actual work to the gitops queue. pub(super) async fn command_squash( @@ -32,7 +36,7 @@ pub(super) async fn command_squash( commit_message: SquashCommitMessage, bot_prefix: &CommandPrefix, gitops_queue: &GitOpsQueueSender, -) -> anyhow::Result<()> { +) -> anyhow::Result { let send_comment = async |text: String| { let comment = repo_state .client @@ -50,12 +54,12 @@ pub(super) async fn command_squash( if !is_author && !is_reviewer { send_comment(":key: Only the PR author or reviewers can squash commits.".to_string()) .await?; - return Ok(()); + return Ok(SquashResult { sha: None }); } if !pr.github.editable_by_maintainers { send_comment(":key: The `Allow edits by maintainers` option is not enabled on this PR. It is required for squashing to work.".to_string()).await?; - return Ok(()); + return Ok(SquashResult { sha: None }); } let fork_error = @@ -67,7 +71,7 @@ pub(super) async fn command_squash( .take_if(|repo| validate_fork(&pr.github.author, repo_state.repository(), repo)) else { send_comment(fork_error()).await?; - return Ok(()); + return Ok(SquashResult { sha: None }); }; let pr_model = pr.db; @@ -81,7 +85,7 @@ pub(super) async fn command_squash( format!(":exclamation: Cannot squash a PR that is currently being tested. Unapprove the PR first using `{bot_prefix} r-`."), ) .await?; - return Ok(()); + return Ok(SquashResult { sha: None }); } if pr.github.commit_count > 250 { @@ -90,7 +94,7 @@ pub(super) async fn command_squash( pr.github.commit_count )) .await?; - return Ok(()); + return Ok(SquashResult { sha: None }); } let pr_id = PullRequestId { @@ -99,7 +103,7 @@ pub(super) async fn command_squash( }; if gitops_queue.is_pending(&pr_id) { send_comment(":hourglass: This PR already has a pending git operation in progress, please wait until it is completed.".to_string()).await?; - return Ok(()); + return Ok(SquashResult { sha: None }); } let commits = repo_state @@ -108,7 +112,7 @@ pub(super) async fn command_squash( .await?; if commits.len() < 2 { send_comment(":exclamation: The PR has only one commit.".to_string()).await?; - return Ok(()); + return Ok(SquashResult { sha: None }); } let notify_comment = repo_state @@ -169,7 +173,7 @@ pub(super) async fn command_squash( ":exclamation: Failed to create squashed commit: {error}" )) .await?; - return Ok(()); + return Ok(SquashResult { sha: None }); } }; @@ -259,7 +263,7 @@ pub(super) async fn command_squash( source_repo: repo_state.repository().clone(), target_repo: fork_repository, target_branch, - commit, + commit: commit.clone(), token, on_finish, }); @@ -270,9 +274,9 @@ pub(super) async fn command_squash( .to_string(), ) .await?; - return Ok(()); + return Ok(SquashResult { sha: Some(commit) }); } - Ok(()) + Ok(SquashResult { sha: Some(commit) }) } /// Add "Co-authored-by: [name] <[email]>" trailer(s) to the commit message to properly reflect diff --git a/src/bors/handlers/squash_and_approve.rs b/src/bors/handlers/squash_and_approve.rs new file mode 100644 index 00000000..e74a30c2 --- /dev/null +++ b/src/bors/handlers/squash_and_approve.rs @@ -0,0 +1,40 @@ +use crate::PgDbClient; +use crate::bors::command::SquashCommitMessage; +use crate::bors::gitops_queue::GitOpsQueueSender; +use crate::bors::handlers::PullRequestData; +use crate::bors::handlers::squash::{SquashResult, command_squash}; +use crate::bors::{CommandPrefix, RepositoryState}; +use crate::github::GithubUser; +use std::pin::Pin; +use std::sync::Arc; + +type SquashCallback = + dyn Fn(SquashResult) -> Pin> + Send>> + Send; + +/// Entry point for the squash command. +/// This function validates the command and enqueues the actual work to the gitops queue. +#[allow(clippy::too_many_arguments)] +pub(super) async fn command_squash_and_approve( + repo_state: Arc, + db: Arc, + pr: PullRequestData<'_>, + author: &GithubUser, + commit_message: SquashCommitMessage, + bot_prefix: &CommandPrefix, + gitops_queue: &GitOpsQueueSender, + callback: Box, +) -> anyhow::Result<()> { + let squash_result = command_squash( + repo_state, + db, + pr, + author, + commit_message, + bot_prefix, + gitops_queue, + ) + .await?; + + callback(squash_result).await?; + Ok(()) +} diff --git a/src/bors/mod.rs b/src/bors/mod.rs index 96ccf6ef..9e3a20cf 100644 --- a/src/bors/mod.rs +++ b/src/bors/mod.rs @@ -106,6 +106,7 @@ pub fn format_help() -> &'static str { BorsCommand::Retry => {} BorsCommand::Cancel => {} BorsCommand::Squash { .. } => {} + BorsCommand::SquashAndApprove { .. } => {} } r#" From da8d18d9687847af6a436c60b492c4f7f356be43 Mon Sep 17 00:00:00 2001 From: David Manca <1900179+medzernik@users.noreply.github.com> Date: Sat, 22 Aug 2026 12:56:03 +0200 Subject: [PATCH 02/11] add test for `squash_and_approve` Signed-off-by: David Manca <1900179+medzernik@users.noreply.github.com> --- src/bors/handlers/squash_and_approve.rs | 90 +++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/src/bors/handlers/squash_and_approve.rs b/src/bors/handlers/squash_and_approve.rs index e74a30c2..11e56cdd 100644 --- a/src/bors/handlers/squash_and_approve.rs +++ b/src/bors/handlers/squash_and_approve.rs @@ -38,3 +38,93 @@ pub(super) async fn command_squash_and_approve( callback(squash_result).await?; Ok(()) } + +#[cfg(test)] +mod tests { + use crate::github::GithubRepoName; + use crate::tests::default_repo_name; + use crate::tests::{BorsTester, Commit, GitHub, Repo, User, run_test}; + use std::sync::Arc; + + async fn approve_add_label(pool: sqlx::PgPool) { + let gh = GitHub::default().append_to_default_config( + r#" +[labels] +approved = ["+approved"] +"#, + ); + run_test((pool, gh), async |ctx: &mut BorsTester| { + ctx.approve(()).await?; + ctx.pr(()).await.expect_added_labels(&["approved"]); + Ok(()) + }) + .await; + } + + #[sqlx::test(migrator = "crate::MIGRATOR")] + async fn squash_two_commits_and_approve(pool: sqlx::PgPool) { + let pool = Arc::new(pool); + let gh = run_test( + ( + ::clone(&*(pool.clone())), + squash_state(), + ), + async |ctx: &mut BorsTester| { + ctx.modify_pr_in_gh((), |pr| { + pr.title = "Foobar".to_string(); + pr.reset_to_single_commit(Commit::from_sha("sha1")); + pr.add_commits(vec![Commit::from_sha("sha2")]); + }); + ctx.post_comment("@bors squash").await?; + ctx.run_gitop_queue().await?; + insta::assert_snapshot!( + ctx.get_next_comment_text(()).await?, + @":construction: Squashing... this can take a few minutes." + ); + insta::assert_snapshot!( + ctx.get_next_comment_text(()).await?, + @":hammer: 2 commits were squashed into sha2-reauthored-to-git-user." + ); + let branch = ctx.pr(()).await.get_gh_pr().head_branch_copy(); + assert_eq!(branch.get_commits().len(), 1); + insta::assert_debug_snapshot!(branch.get_commit(), @r#" + Commit { + sha: "sha2-reauthored-to-git-user", + message: "Foobar\n\n* Commit sha1\n* Commit sha2\n", + author: GitUser { + name: "git-user", + email: "git-user@git.com", + }, + } + "#); + approve_add_label(::clone(&*(pool.clone()))).await; + + Ok(()) + }, + ) + .await; + insta::assert_snapshot!(gh.get_sha_history((), "pr/1"), @" + pr-1-sha + sha1 + sha2 + sha2-reauthored-to-git-user + "); + } + + fn squash_state() -> GitHub { + let gh = GitHub::default(); + let pr_author = User::default_pr_author(); + + // Create fork + let fork_repo = fork_repo(); + let mut repo = Repo::new(pr_author.clone(), fork_repo.name()); + repo.fork_of = Some(gh.default_repo()); + + // Set the default PR to be from the fork + gh.default_repo().lock().get_pr_mut(1).head_repository = Some(repo.full_name()); + gh.with_repo(repo) + } + fn fork_repo() -> GithubRepoName { + GithubRepoName::new(&User::default_pr_author().name, default_repo_name().name()) + } +} From cb0aab4db7d341fa4a59e556fe0a4ecb21660a0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Mon, 7 Sep 2026 11:01:44 +0200 Subject: [PATCH 03/11] Refactor squash and approve --- src/bors/handlers/mod.rs | 120 ++++++++-------------- src/bors/handlers/squash.rs | 88 +++++++++++++--- src/bors/handlers/squash_and_approve.rs | 130 ------------------------ 3 files changed, 119 insertions(+), 219 deletions(-) delete mode 100644 src/bors/handlers/squash_and_approve.rs diff --git a/src/bors/handlers/mod.rs b/src/bors/handlers/mod.rs index 8f849d40..94a12a69 100644 --- a/src/bors/handlers/mod.rs +++ b/src/bors/handlers/mod.rs @@ -15,7 +15,7 @@ use crate::bors::handlers::refresh::{ use crate::bors::handlers::review::{ TreeCloseArguments, command_approve, command_close_tree, command_open_tree, command_unapprove, }; -use crate::bors::handlers::squash::SquashResult; +use crate::bors::handlers::squash::AfterSquashCallback; use crate::bors::handlers::trybuild::{command_try_build, command_try_cancel}; use crate::bors::handlers::workflow::{ AutoBuildCancelReason, handle_workflow_completed, handle_workflow_job_completed, @@ -56,7 +56,6 @@ mod pr_events; mod refresh; mod review; mod squash; -mod squash_and_approve; mod trybuild; mod workflow; @@ -653,6 +652,7 @@ async fn handle_comment( commit_message, ctx.parser.prefix(), senders.gitops_queue(), + None, ) .instrument(span) .await?; @@ -682,81 +682,51 @@ async fn handle_comment( let span = tracing::info_span!("SquashAndApprove"); if ctx.local_git_available() { - let span_cloned = span.clone(); - let ctx_cloned = ctx.clone(); - let ctx2_cloned = ctx.clone(); - let comment_author_cloned = comment.author.clone(); - let comment2_author_cloned = comment.author.clone(); - let senders_cloned = senders.clone(); - let senders2_cloned = senders.clone(); - let db_cloned = database.clone(); - let db2_cloned = database.clone(); - let pr2_github_cloned = pr_github.clone(); - - let pr_cloned = PullRequestData { - github: &(pr_github.clone()), - db: { - &database - .clone() - .get_pull_request(repo.repository(), pr_number) + let ctx2 = ctx.clone(); + let repo2 = repo.clone(); + let db2 = database.clone(); + let pr_github = pr_github.clone(); + let comment_author = comment.author.clone(); + let merge_queue_tx = senders.merge_queue().clone(); + let callback: AfterSquashCallback = Box::new(move |sha: CommitSha| { + Box::pin(async move { + let pr_db = db2 + .get_pull_request(repo2.repository(), pr_github.number) .await? - .unwrap() - }, - }; - - squash_and_approve::command_squash_and_approve( - repo.clone(), - db_cloned.clone(), - pr_cloned, - &comment_author_cloned.clone(), + .expect("TODO"); + let pr2 = PullRequestData { + github: &pr_github, + db: &pr_db, + }; + + command_approve( + ctx2, + repo2, + db2, + pr2, + &comment_author, + &approver, + priority, + rollup, + note, + &merge_queue_tx, + sha, + ) + .await + }) + }); + squash::command_squash( + repo, + database, + pr, + &comment.author, commit_message, - ctx_cloned.parser.prefix(), - senders_cloned.gitops_queue(), - Box::new(move |squash_result: SquashResult| { - let ctx3_cloned = ctx2_cloned.clone(); - let span2_cloned = span_cloned.clone(); - let db3_cloned = db2_cloned.clone(); - let comment3_author_cloned = comment2_author_cloned.clone(); - let rollup_cloned = rollup; - let priority_cloned = priority; - let note_cloned = note.clone(); - let approver_cloned = approver.clone(); - let repo_cloned = repo.clone(); - let senders3_cloned = senders2_cloned.clone(); - let pr3_github_cloned = pr2_github_cloned.clone(); - - Box::pin(async move { - let pr_db_cloned = db3_cloned - .get_pull_request(repo_cloned.repository(), pr_number) - .await? - .unwrap(); - - let pr2_cloned = PullRequestData { - github: &pr3_github_cloned.clone(), - db: { &pr_db_cloned }, - }; - command_approve( - ctx3_cloned.clone(), - repo_cloned.clone(), - db3_cloned.clone(), - pr2_cloned, - &comment3_author_cloned.clone(), - &approver_cloned.clone(), - priority_cloned, - rollup_cloned, - note_cloned.clone(), - senders3_cloned.clone().merge_queue(), - squash_result.sha.unwrap_or(Err(anyhow::anyhow!( - "missing sha from squash" - ))?), - ) - .instrument(span2_cloned.clone()) - .await - }) - }), + ctx.parser.prefix(), + senders.gitops_queue(), + Some(callback), ) - .instrument(span.clone()) - .await + .instrument(span) + .await?; } else { repo.client .post_comment( @@ -771,8 +741,8 @@ async fn handle_comment( ) .instrument(span) .await?; - return Ok(()); } + Ok(()) } }; if result.is_err() { diff --git a/src/bors/handlers/squash.rs b/src/bors/handlers/squash.rs index 8888e987..fbec03b3 100644 --- a/src/bors/handlers/squash.rs +++ b/src/bors/handlers/squash.rs @@ -18,13 +18,13 @@ use crate::github::{CommitSha, GithubRepoName, GithubUser}; use crate::permissions::PermissionType; use std::collections::HashSet; use std::fmt::Write; +use std::pin::Pin; use std::sync::Arc; const CO_AUTHORED_BY_TRAILER: &str = "Co-authored-by"; -pub struct SquashResult { - pub(crate) sha: Option, -} +pub(super) type AfterSquashCallback = + Box Pin> + Send>> + Send>; /// Entry point for the squash command. /// This function validates the command and enqueues the actual work to the gitops queue. @@ -36,7 +36,8 @@ pub(super) async fn command_squash( commit_message: SquashCommitMessage, bot_prefix: &CommandPrefix, gitops_queue: &GitOpsQueueSender, -) -> anyhow::Result { + after_squash_callback: Option, +) -> anyhow::Result<()> { let send_comment = async |text: String| { let comment = repo_state .client @@ -54,12 +55,12 @@ pub(super) async fn command_squash( if !is_author && !is_reviewer { send_comment(":key: Only the PR author or reviewers can squash commits.".to_string()) .await?; - return Ok(SquashResult { sha: None }); + return Ok(()); } if !pr.github.editable_by_maintainers { send_comment(":key: The `Allow edits by maintainers` option is not enabled on this PR. It is required for squashing to work.".to_string()).await?; - return Ok(SquashResult { sha: None }); + return Ok(()); } let fork_error = @@ -71,7 +72,7 @@ pub(super) async fn command_squash( .take_if(|repo| validate_fork(&pr.github.author, repo_state.repository(), repo)) else { send_comment(fork_error()).await?; - return Ok(SquashResult { sha: None }); + return Ok(()); }; let pr_model = pr.db; @@ -85,7 +86,7 @@ pub(super) async fn command_squash( format!(":exclamation: Cannot squash a PR that is currently being tested. Unapprove the PR first using `{bot_prefix} r-`."), ) .await?; - return Ok(SquashResult { sha: None }); + return Ok(()); } if pr.github.commit_count > 250 { @@ -94,7 +95,7 @@ pub(super) async fn command_squash( pr.github.commit_count )) .await?; - return Ok(SquashResult { sha: None }); + return Ok(()); } let pr_id = PullRequestId { @@ -103,7 +104,7 @@ pub(super) async fn command_squash( }; if gitops_queue.is_pending(&pr_id) { send_comment(":hourglass: This PR already has a pending git operation in progress, please wait until it is completed.".to_string()).await?; - return Ok(SquashResult { sha: None }); + return Ok(()); } let commits = repo_state @@ -112,7 +113,7 @@ pub(super) async fn command_squash( .await?; if commits.len() < 2 { send_comment(":exclamation: The PR has only one commit.".to_string()).await?; - return Ok(SquashResult { sha: None }); + return Ok(()); } let notify_comment = repo_state @@ -173,7 +174,7 @@ pub(super) async fn command_squash( ":exclamation: Failed to create squashed commit: {error}" )) .await?; - return Ok(SquashResult { sha: None }); + return Ok(()); } }; @@ -244,6 +245,11 @@ pub(super) async fn command_squash( .await?; // Hide previous "squash started" comments. hide_tagged_comments(&repo_state, &db, &pr_model, CommentTag::SquashStarted).await?; + + if let Some(cb) = after_squash_callback { + cb(commit).await?; + } + Ok(()) }) }); @@ -274,9 +280,9 @@ pub(super) async fn command_squash( .to_string(), ) .await?; - return Ok(SquashResult { sha: Some(commit) }); + return Ok(()); } - Ok(SquashResult { sha: Some(commit) }) + Ok(()) } /// Add "Co-authored-by: [name] <[email]>" trailer(s) to the commit message to properly reflect @@ -401,6 +407,7 @@ mod tests { BorsTester, Comment, Commit, GitHub, GitUser, PullRequest, Repo, User, default_repo_name, run_test, }; + use std::sync::Arc; #[test] fn parse_coauthor_valid_trailer() { @@ -885,6 +892,59 @@ also include this pls .await; } + #[sqlx::test(migrator = "crate::MIGRATOR")] + async fn squash_two_commits_and_approve(pool: sqlx::PgPool) { + let pool = Arc::new(pool); + let gh = run_test( + ( + ::clone(&*(pool.clone())), + squash_state(), + ), + async |ctx: &mut BorsTester| { + ctx.modify_pr_in_gh((), |pr| { + pr.title = "Foobar".to_string(); + pr.reset_to_single_commit(Commit::from_sha("sha1")); + pr.add_commits(vec![Commit::from_sha("sha2")]); + }); + ctx.post_comment("@bors squash").await?; + ctx.run_gitop_queue().await?; + insta::assert_snapshot!( + ctx.get_next_comment_text(()).await?, + @":construction: Squashing... this can take a few minutes." + ); + insta::assert_snapshot!( + ctx.get_next_comment_text(()).await?, + @":hammer: 2 commits were squashed into sha2-reauthored-to-git-user." + ); + let branch = ctx.pr(()).await.get_gh_pr().head_branch_copy(); + assert_eq!(branch.get_commits().len(), 1); + insta::assert_debug_snapshot!(branch.get_commit(), @r#" + Commit { + sha: "sha2-reauthored-to-git-user", + message: "Foobar\n\n* Commit sha1\n* Commit sha2\n", + author: GitUser { + name: "git-user", + email: "git-user@git.com", + }, + } + "#); + crate::bors::handlers::squash_and_approve::tests::approve_add_label( + ::clone(&*(pool.clone())), + ) + .await; + + Ok(()) + }, + ) + .await; + insta::assert_snapshot!(gh.get_sha_history((), "pr/1"), @" + pr-1-sha + sha1 + sha2 + sha2-reauthored-to-git-user + "); + } + fn squash_state() -> GitHub { let gh = GitHub::default(); let pr_author = User::default_pr_author(); diff --git a/src/bors/handlers/squash_and_approve.rs b/src/bors/handlers/squash_and_approve.rs deleted file mode 100644 index 11e56cdd..00000000 --- a/src/bors/handlers/squash_and_approve.rs +++ /dev/null @@ -1,130 +0,0 @@ -use crate::PgDbClient; -use crate::bors::command::SquashCommitMessage; -use crate::bors::gitops_queue::GitOpsQueueSender; -use crate::bors::handlers::PullRequestData; -use crate::bors::handlers::squash::{SquashResult, command_squash}; -use crate::bors::{CommandPrefix, RepositoryState}; -use crate::github::GithubUser; -use std::pin::Pin; -use std::sync::Arc; - -type SquashCallback = - dyn Fn(SquashResult) -> Pin> + Send>> + Send; - -/// Entry point for the squash command. -/// This function validates the command and enqueues the actual work to the gitops queue. -#[allow(clippy::too_many_arguments)] -pub(super) async fn command_squash_and_approve( - repo_state: Arc, - db: Arc, - pr: PullRequestData<'_>, - author: &GithubUser, - commit_message: SquashCommitMessage, - bot_prefix: &CommandPrefix, - gitops_queue: &GitOpsQueueSender, - callback: Box, -) -> anyhow::Result<()> { - let squash_result = command_squash( - repo_state, - db, - pr, - author, - commit_message, - bot_prefix, - gitops_queue, - ) - .await?; - - callback(squash_result).await?; - Ok(()) -} - -#[cfg(test)] -mod tests { - use crate::github::GithubRepoName; - use crate::tests::default_repo_name; - use crate::tests::{BorsTester, Commit, GitHub, Repo, User, run_test}; - use std::sync::Arc; - - async fn approve_add_label(pool: sqlx::PgPool) { - let gh = GitHub::default().append_to_default_config( - r#" -[labels] -approved = ["+approved"] -"#, - ); - run_test((pool, gh), async |ctx: &mut BorsTester| { - ctx.approve(()).await?; - ctx.pr(()).await.expect_added_labels(&["approved"]); - Ok(()) - }) - .await; - } - - #[sqlx::test(migrator = "crate::MIGRATOR")] - async fn squash_two_commits_and_approve(pool: sqlx::PgPool) { - let pool = Arc::new(pool); - let gh = run_test( - ( - ::clone(&*(pool.clone())), - squash_state(), - ), - async |ctx: &mut BorsTester| { - ctx.modify_pr_in_gh((), |pr| { - pr.title = "Foobar".to_string(); - pr.reset_to_single_commit(Commit::from_sha("sha1")); - pr.add_commits(vec![Commit::from_sha("sha2")]); - }); - ctx.post_comment("@bors squash").await?; - ctx.run_gitop_queue().await?; - insta::assert_snapshot!( - ctx.get_next_comment_text(()).await?, - @":construction: Squashing... this can take a few minutes." - ); - insta::assert_snapshot!( - ctx.get_next_comment_text(()).await?, - @":hammer: 2 commits were squashed into sha2-reauthored-to-git-user." - ); - let branch = ctx.pr(()).await.get_gh_pr().head_branch_copy(); - assert_eq!(branch.get_commits().len(), 1); - insta::assert_debug_snapshot!(branch.get_commit(), @r#" - Commit { - sha: "sha2-reauthored-to-git-user", - message: "Foobar\n\n* Commit sha1\n* Commit sha2\n", - author: GitUser { - name: "git-user", - email: "git-user@git.com", - }, - } - "#); - approve_add_label(::clone(&*(pool.clone()))).await; - - Ok(()) - }, - ) - .await; - insta::assert_snapshot!(gh.get_sha_history((), "pr/1"), @" - pr-1-sha - sha1 - sha2 - sha2-reauthored-to-git-user - "); - } - - fn squash_state() -> GitHub { - let gh = GitHub::default(); - let pr_author = User::default_pr_author(); - - // Create fork - let fork_repo = fork_repo(); - let mut repo = Repo::new(pr_author.clone(), fork_repo.name()); - repo.fork_of = Some(gh.default_repo()); - - // Set the default PR to be from the fork - gh.default_repo().lock().get_pr_mut(1).head_repository = Some(repo.full_name()); - gh.with_repo(repo) - } - fn fork_repo() -> GithubRepoName { - GithubRepoName::new(&User::default_pr_author().name, default_repo_name().name()) - } -} From 34eb703e8e952ea075d4760b017d0203d27ae0e2 Mon Sep 17 00:00:00 2001 From: medzernik Date: Wed, 9 Sep 2026 19:32:00 +0200 Subject: [PATCH 04/11] try to fix test call in now gone module Signed-off-by: medzernik --- src/bors/handlers/squash.rs | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/bors/handlers/squash.rs b/src/bors/handlers/squash.rs index fbec03b3..2f43f362 100644 --- a/src/bors/handlers/squash.rs +++ b/src/bors/handlers/squash.rs @@ -892,6 +892,21 @@ also include this pls .await; } + async fn approve_add_label(pool: sqlx::PgPool) { + let gh = GitHub::default().append_to_default_config( + r#" +[labels] +approved = ["+approved"] +"#, + ); + run_test((pool, gh), async |ctx: &mut BorsTester| { + ctx.approve(()).await?; + ctx.pr(()).await.expect_added_labels(&["approved"]); + Ok(()) + }) + .await; + } + #[sqlx::test(migrator = "crate::MIGRATOR")] async fn squash_two_commits_and_approve(pool: sqlx::PgPool) { let pool = Arc::new(pool); @@ -928,11 +943,9 @@ also include this pls }, } "#); - crate::bors::handlers::squash_and_approve::tests::approve_add_label( - ::clone(&*(pool.clone())), - ) - .await; + approve_add_label(::clone(&*(pool.clone()))).await; + ctx.pr(()).await.expect_approved_by("default-user"); Ok(()) }, ) From 45a0aee27da0ec34126ca549951fe2c4c0692e15 Mon Sep 17 00:00:00 2001 From: medzernik Date: Wed, 9 Sep 2026 19:39:18 +0200 Subject: [PATCH 05/11] allow clippy argument limit lint on squash Signed-off-by: medzernik --- src/bors/handlers/squash.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/bors/handlers/squash.rs b/src/bors/handlers/squash.rs index 2f43f362..01ffa91c 100644 --- a/src/bors/handlers/squash.rs +++ b/src/bors/handlers/squash.rs @@ -28,6 +28,7 @@ pub(super) type AfterSquashCallback = /// Entry point for the squash command. /// This function validates the command and enqueues the actual work to the gitops queue. +#[allow(clippy::too_many_arguments)] pub(super) async fn command_squash( repo_state: Arc, db: Arc, From b1666a9e35a90c38d962a0961ecfa9ce52408439 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 10 Sep 2026 09:26:37 +0200 Subject: [PATCH 06/11] Handle race condition caused by concurrent push and approval --- ...a887809bf4c7d429ac242b8a3f21c91415499.json | 15 +++++ src/bors/handlers/mod.rs | 61 ++++++++++++++++--- src/bors/handlers/pr_events.rs | 6 +- src/bors/handlers/squash.rs | 36 ++++++++++- src/bors/merge_queue.rs | 10 +-- src/database/client.rs | 16 ++++- src/database/operations.rs | 26 ++++++++ 7 files changed, 151 insertions(+), 19 deletions(-) create mode 100644 .sqlx/query-323b623291bbf288bc65c6ce430a887809bf4c7d429ac242b8a3f21c91415499.json diff --git a/.sqlx/query-323b623291bbf288bc65c6ce430a887809bf4c7d429ac242b8a3f21c91415499.json b/.sqlx/query-323b623291bbf288bc65c6ce430a887809bf4c7d429ac242b8a3f21c91415499.json new file mode 100644 index 00000000..69549429 --- /dev/null +++ b/.sqlx/query-323b623291bbf288bc65c6ce430a887809bf4c7d429ac242b8a3f21c91415499.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "\n UPDATE pull_request\n SET approved_by = NULL,\n approved_sha = NULL,\n auto_build_id = NULL\n WHERE\n id = $1 AND\n (approved_by IS NULL OR approved_sha != $2)\n ", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Int4", + "Text" + ] + }, + "nullable": [] + }, + "hash": "323b623291bbf288bc65c6ce430a887809bf4c7d429ac242b8a3f21c91415499" +} diff --git a/src/bors/handlers/mod.rs b/src/bors/handlers/mod.rs index 94a12a69..45d45455 100644 --- a/src/bors/handlers/mod.rs +++ b/src/bors/handlers/mod.rs @@ -908,7 +908,9 @@ pub enum InvalidationReason { /// A new commit was pushed to the pull request. /// If it was approved, it will be unapproved. /// If it was contained in any rollups, they will be closed. - CommitShaChanged, + CommitShaChanged { sha: CommitSha }, + /// The base branch of the PR has changed. + BaseBranchChanged, /// The pull request was closed. /// If it was approved, it will be unapproved. /// If it was contained in any rollups, they will be closed. @@ -938,6 +940,20 @@ pub async fn unapprove_pr( Ok(()) } +/// Unapprove the given pull request if `new_sha` already wasn't the approved commit. +/// Returns true if the PR was actually unapproved. +pub async fn unapprove_pr_if_sha_changed( + repo_state: &RepositoryState, + db: &PgDbClient, + pr_db: &PullRequestModel, + pr_gh: &PullRequestInfo, + new_sha: &CommitSha, +) -> anyhow::Result { + let unapproved = db.unapprove_if_sha_changed(pr_db, new_sha).await?; + handle_label_trigger(repo_state, pr_gh, LabelTrigger::Unapproved).await?; + Ok(unapproved) +} + pub struct InvalidationComment { /// Start of the invalidation comment base_text: String, @@ -995,17 +1011,38 @@ pub async fn invalidate_pr( comment: Option, ) -> anyhow::Result { // Step 1: unapprove the pull request if it was approved - // This happens everytime the PR is invalidated, if it was approved before + // This happens everytime the PR is invalidated, if it was approved before with a different + // commit let pr_unapproved = if pr_db.is_approved() { - unapprove_pr(repo_state, db, pr_db, &pr_gh.clone().into()).await?; - true + // This is handling a potential race condition coming from `@bors r+ squash`. + // When we squash, we push a new commit to the PR, and then we approve it. + // GitHub will then send us a webhook about the push, which will call this function. + // If we then unapproved the PR, then we would essentially cancel the previous `r+ squash`. + // So instead, if the pushed commit SHA is the same as the one that is already approved in + // the DB, we do not unapprove it. We figure this out atomically, to avoid further race + // conditions in the DB. + match &info.reason { + InvalidationReason::CommitShaChanged { sha } => { + unapprove_pr_if_sha_changed(repo_state, db, pr_db, &pr_gh.clone().into(), sha) + .await? + } + InvalidationReason::Close + | InvalidationReason::BaseBranchChanged + | InvalidationReason::Unapproval { .. } + | InvalidationReason::RollupMemberInvalidated { .. } => { + unapprove_pr(repo_state, db, pr_db, &pr_gh.clone().into()).await?; + true + } + } } else { false }; fn get_cancel_reason(reason: &InvalidationReason) -> AutoBuildCancelReason { match reason { - InvalidationReason::CommitShaChanged => AutoBuildCancelReason::PushToPR, + InvalidationReason::CommitShaChanged { .. } | InvalidationReason::BaseBranchChanged => { + AutoBuildCancelReason::PushToPR + } InvalidationReason::Close => AutoBuildCancelReason::Close, InvalidationReason::Unapproval { .. } => AutoBuildCancelReason::Unapproval, InvalidationReason::RollupMemberInvalidated { reason, .. } => get_cancel_reason(reason), @@ -1025,7 +1062,9 @@ pub async fn invalidate_pr( // Note that we don't do this on `InvalidationReason::Close` itself, because that happens after // the PR has been closed already. let pr_closed = if let InvalidationReason::RollupMemberInvalidated { reason, .. } = &info.reason - && let InvalidationReason::Close | InvalidationReason::CommitShaChanged = &**reason + && let InvalidationReason::Close + | InvalidationReason::CommitShaChanged { .. } + | InvalidationReason::BaseBranchChanged = &**reason && matches!( pr_gh.status, PullRequestStatus::Open | PullRequestStatus::Draft @@ -1040,7 +1079,8 @@ pub async fn invalidate_pr( // Step 4: recursively invalidate all open rollups containing this PR let invalidate_rollups = match info.reason { - InvalidationReason::CommitShaChanged + InvalidationReason::CommitShaChanged { .. } + | InvalidationReason::BaseBranchChanged | InvalidationReason::Close | InvalidationReason::Unapproval { .. } => true, // We do not assume that rollups contain other rollups @@ -1168,7 +1208,12 @@ pub fn invalidation_comment( }; let action = match &**reason { - InvalidationReason::CommitShaChanged => format!("{} its commit SHA", wrap("changed")), + InvalidationReason::CommitShaChanged { sha } => { + format!("{} its commit SHA to {sha}", wrap("changed")) + } + InvalidationReason::BaseBranchChanged => { + format!("{} its base branch", wrap("changed")) + } InvalidationReason::Close => format!("was {}", wrap("closed")), InvalidationReason::Unapproval { .. } => format!("was {}", wrap("unapproved")), InvalidationReason::RollupMemberInvalidated { .. } => { diff --git a/src/bors/handlers/pr_events.rs b/src/bors/handlers/pr_events.rs index f85d2742..0f2906c0 100644 --- a/src/bors/handlers/pr_events.rs +++ b/src/bors/handlers/pr_events.rs @@ -41,7 +41,7 @@ pub(super) async fn handle_pull_request_edited( &db, &pr_model, pr, - InvalidationInfo::new(InvalidationReason::CommitShaChanged), + InvalidationInfo::new(InvalidationReason::BaseBranchChanged), Some(InvalidationComment::new(format!( ":warning: The base branch changed to `{base_name}`.", base_name = payload.pull_request.base.name @@ -71,7 +71,9 @@ pub(super) async fn handle_push_to_pull_request( &db, &pr_model, pr, - InvalidationInfo::new(InvalidationReason::CommitShaChanged), + InvalidationInfo::new(InvalidationReason::CommitShaChanged { + sha: pr.head.sha.clone(), + }), Some(InvalidationComment::new(format!( ":warning: A new commit `{}` was pushed.", pr.head.sha diff --git a/src/bors/handlers/squash.rs b/src/bors/handlers/squash.rs index 01ffa91c..978667fc 100644 --- a/src/bors/handlers/squash.rs +++ b/src/bors/handlers/squash.rs @@ -233,8 +233,10 @@ pub(super) async fn command_squash( &db, &pr_model, &pr_github, - InvalidationInfo::new(InvalidationReason::CommitShaChanged) - .with_comment_url(notify_comment.html_url.to_string()), + InvalidationInfo::new(InvalidationReason::CommitShaChanged { + sha: commit.clone(), + }) + .with_comment_url(notify_comment.html_url.to_string()), Some( InvalidationComment::new(format!( ":hammer: {} commits were squashed into {commit}.", @@ -959,6 +961,36 @@ approved = ["+approved"] "); } + #[sqlx::test(migrator = "crate::MIGRATOR")] + async fn squash_approve_push_webhook(pool: sqlx::PgPool) { + run_test((pool, squash_state()), async |ctx: &mut BorsTester| { + ctx.modify_pr_in_gh((), |pr| { + pr.title = "Foobar".to_string(); + pr.reset_to_single_commit(Commit::from_sha("sha1")); + pr.add_commits(vec![Commit::from_sha("sha2")]); + }); + ctx.post_comment("@bors r+ squash").await?; + ctx.expect_comments((), 1).await; + ctx.run_gitop_queue().await?; + insta::assert_snapshot!( + ctx.get_next_comment_text(()).await?, + @":hammer: 2 commits were squashed into sha2-reauthored-to-git-user." + ); + insta::assert_snapshot!( + ctx.get_next_comment_text(()).await?, + @":hammer: 2 commits were squashed into sha2-reauthored-to-git-user." + ); + let branch = ctx.pr(()).await.get_gh_pr().head_branch_copy(); + + // Check that this won't unapprove the PR + ctx.push_to_pr((), branch.get_commit().clone()).await?; + + ctx.pr(()).await.expect_approved_by("default-user"); + Ok(()) + }) + .await; + } + fn squash_state() -> GitHub { let gh = GitHub::default(); let pr_author = User::default_pr_author(); diff --git a/src/bors/merge_queue.rs b/src/bors/merge_queue.rs index 1c70dde1..baf4391b 100644 --- a/src/bors/merge_queue.rs +++ b/src/bors/merge_queue.rs @@ -464,14 +464,14 @@ async fn handle_start_auto_build( &ctx.db, pr, &gh_pr, - InvalidationInfo::new(InvalidationReason::CommitShaChanged), + InvalidationInfo::new(InvalidationReason::CommitShaChanged { + sha: actual.clone(), + }), Some( InvalidationComment::new(format!( r#"Commit SHA did not match the approved SHA during a merge attempt. -Approved commit SHA: {expected_sha} -Actual head SHA: {actual_sha}"#, - expected_sha = pr.approved_sha().unwrap_or("").to_owned(), - actual_sha = gh_pr.head.sha +Approved commit SHA: {approved} +Actual head SHA: {actual}"#, )) .post_always(), ), diff --git a/src/database/client.rs b/src/database/client.rs index 43076c0e..37e37a56 100644 --- a/src/database/client.rs +++ b/src/database/client.rs @@ -8,8 +8,9 @@ use super::operations::{ insert_repo_if_not_exists, is_rollup, record_tagged_bot_comment, set_pr_assignees, set_pr_mergeability_state, set_pr_priority, set_pr_rollup_mode, set_pr_status, set_rollup_member_unrolled_state, set_rollup_members_unrolled_state, - set_stale_mergeability_status_by_base_branch, unapprove_pull_request, undelegate_pull_request, - update_build, update_pr_try_build_id, update_pr_unrolled_build_id, update_workflow_status, + set_stale_mergeability_status_by_base_branch, unapprove_pull_request, + unapprove_pull_request_if_sha_changed, undelegate_pull_request, update_build, + update_pr_try_build_id, update_pr_unrolled_build_id, update_workflow_status, upsert_pull_request, upsert_repository, }; use super::{ @@ -114,6 +115,17 @@ impl PgDbClient { unapprove_pull_request(&self.pool, pr.id).await } + /// Unapprove a pull request and remove its auto build status, if there is any attached. + /// Only do it if `sha` wasn't already approved. + /// Returns true if the PR was actually unapproved. + pub async fn unapprove_if_sha_changed( + &self, + pr: &PullRequestModel, + sha: &CommitSha, + ) -> anyhow::Result { + unapprove_pull_request_if_sha_changed(&self.pool, pr.id, sha).await + } + pub async fn clear_auto_build(&self, pr: &PullRequestModel) -> anyhow::Result<()> { clear_auto_build(&self.pool, pr.id).await } diff --git a/src/database/operations.rs b/src/database/operations.rs index 21623973..8710c7b7 100644 --- a/src/database/operations.rs +++ b/src/database/operations.rs @@ -527,6 +527,32 @@ pub(crate) async fn unapprove_pull_request( .await } +pub(crate) async fn unapprove_pull_request_if_sha_changed( + executor: impl PgExecutor<'_>, + pr_id: i32, + sha: &CommitSha, +) -> anyhow::Result { + measure_db_query("unapprove_pull_request_if_sha_changed", || async { + let result = sqlx::query!( + r#" + UPDATE pull_request + SET approved_by = NULL, + approved_sha = NULL, + auto_build_id = NULL + WHERE + id = $1 AND + (approved_by IS NULL OR approved_sha != $2) + "#, + pr_id, + sha.0 + ) + .execute(executor) + .await?; + Ok(result.rows_affected() > 0) + }) + .await +} + pub(crate) async fn delegate_pull_request( executor: impl PgExecutor<'_>, pr_id: i32, From 8a02347d13f9d237f513f53bae7b056a329efc03 Mon Sep 17 00:00:00 2001 From: David Manca <1900179+medzernik@users.noreply.github.com> Date: Sat, 12 Sep 2026 14:04:32 +0200 Subject: [PATCH 07/11] add help text for `r+ squash` Signed-off-by: David Manca <1900179+medzernik@users.noreply.github.com> --- src/bors/handlers/help.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/bors/handlers/help.rs b/src/bors/handlers/help.rs index 4460b191..04452832 100644 --- a/src/bors/handlers/help.rs +++ b/src/bors/handlers/help.rs @@ -65,6 +65,9 @@ mod tests { - `squash [msg|message=|description]`: Squash the commits of a PR into a single commit. - Optionally, you can specify a `` for the created commit. If not specified, the commit messages of all squashed commits will be combined. - If you specify `msg=description`, then the PR body will be used as the squashed commit message. + - `r+ squash [msg|message=|description]`: Squash the commits of a PR into a single commit, then approve on your behalf. + - Optionally, you can specify a `` for the created commit. If not specified, the commit messages of all squashed commits will be combined. + - If you specify `msg=description`, then the PR body will be used as the squashed commit message. - `info`: Get information about the current PR ## Repository management From 9ff1811306a325a40d21a95d8adf63f72c27bf93 Mon Sep 17 00:00:00 2001 From: David Manca <1900179+medzernik@users.noreply.github.com> Date: Sat, 12 Sep 2026 14:07:23 +0200 Subject: [PATCH 08/11] `SquashAndApprove` -> `SquashApprove` Signed-off-by: David Manca <1900179+medzernik@users.noreply.github.com> --- src/bors/command/mod.rs | 2 +- src/bors/handlers/mod.rs | 2 +- src/bors/mod.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/bors/command/mod.rs b/src/bors/command/mod.rs index 80476e17..cd2cc172 100644 --- a/src/bors/command/mod.rs +++ b/src/bors/command/mod.rs @@ -178,7 +178,7 @@ pub enum BorsCommand { /// Squash message for the commit commit_message: SquashCommitMessage, }, - SquashAndApprove { + SquashApprove { /// Squash message for the commit commit_message: SquashCommitMessage, /// Who is approving the commit. diff --git a/src/bors/handlers/mod.rs b/src/bors/handlers/mod.rs index 45d45455..92a73cc6 100644 --- a/src/bors/handlers/mod.rs +++ b/src/bors/handlers/mod.rs @@ -672,7 +672,7 @@ async fn handle_comment( Ok(()) } } - BorsCommand::SquashAndApprove { + BorsCommand::SquashApprove { commit_message, approver, priority, diff --git a/src/bors/mod.rs b/src/bors/mod.rs index 9e3a20cf..4f0139ab 100644 --- a/src/bors/mod.rs +++ b/src/bors/mod.rs @@ -106,7 +106,7 @@ pub fn format_help() -> &'static str { BorsCommand::Retry => {} BorsCommand::Cancel => {} BorsCommand::Squash { .. } => {} - BorsCommand::SquashAndApprove { .. } => {} + BorsCommand::SquashApprove { .. } => {} } r#" From dbbf7f9be04fa44b3677375ee6e85d04763bcaf4 Mon Sep 17 00:00:00 2001 From: David Manca <1900179+medzernik@users.noreply.github.com> Date: Sat, 12 Sep 2026 14:09:41 +0200 Subject: [PATCH 09/11] add parsing for `SquashApprove` Signed-off-by: David Manca <1900179+medzernik@users.noreply.github.com> --- src/bors/command/parser.rs | 186 +++++++++++++++++++++++++++++++++++- src/bors/handlers/squash.rs | 67 ------------- src/github/rollup.rs | 4 +- 3 files changed, 187 insertions(+), 70 deletions(-) diff --git a/src/bors/command/parser.rs b/src/bors/command/parser.rs index 003bb69e..933effc5 100644 --- a/src/bors/command/parser.rs +++ b/src/bors/command/parser.rs @@ -23,7 +23,7 @@ pub enum CommandParseError { } /// Part of a command, either a bare string like `try` or a key value like `parent=`. -#[derive(PartialEq, Copy, Clone)] +#[derive(Debug, PartialEq, Copy, Clone)] enum CommandPart<'a> { Bare(&'a str), KeyValue { key: &'a str, value: &'a str }, @@ -267,6 +267,12 @@ fn parse_parts(input: &str) -> Result>, CommandParseError> { /// - "@bors r+ [p=] [rollup=] [note=]" /// - "@bors r= [p=] [rollup=] [note=]" fn parser_approval(command: &CommandPart<'_>, parts: &[CommandPart<'_>]) -> ParseResult { + if let CommandPart::Bare("r+") = command + && !parts.is_empty() + && parts.contains(&CommandPart::Bare("squash")) + { + return parser_squash_approve(command, parts); + } let approver = match command { CommandPart::Bare("r+") => Approver::Myself, CommandPart::KeyValue { key: "r", value } => { @@ -670,6 +676,53 @@ fn parser_squash(command: &CommandPart<'_>, parts: &[CommandPart<'_>]) -> ParseR } } +/// Parses `@bors r+ squash` command. +/// Supports specifying a commit message via `@bors r+ squash [msg|message]="message"`. +fn parser_squash_approve(_: &CommandPart<'_>, parts: &[CommandPart<'_>]) -> ParseResult { + match parts { + &[CommandPart::Bare("squash")] => Some(Ok(BorsCommand::SquashApprove { + commit_message: SquashCommitMessage::AutoGenerate, + approver: Approver::Myself, + priority: None, + rollup: None, + note: None, + })), + &[ + CommandPart::Bare("squash"), + CommandPart::KeyValue { + key: "msg" | "message", + value, + }, + .., + ] => { + if value == "description" { + Some(Ok(BorsCommand::SquashApprove { + commit_message: SquashCommitMessage::PullRequestDescription, + approver: Approver::Myself, + priority: None, + rollup: None, + note: None, + })) + } else { + Some(Ok(BorsCommand::SquashApprove { + commit_message: SquashCommitMessage::Explicit(value.to_owned()), + approver: Approver::Myself, + priority: None, + rollup: None, + note: None, + })) + } + } + [part, ..] => Some(Err(CommandParseError::UnknownArg { + arg: part.as_key().to_owned(), + did_you_mean: "r+ squash [msg|message=\"\"|description]".to_string(), + })), + part => Some(Err(CommandParseError::MissingArgValue { + arg: format!("{:#?}", part), + })), + } +} + #[cfg(test)] mod tests { use crate::bors::command::BorsCommand; @@ -2099,6 +2152,137 @@ for the crater", "#); } + #[test] + fn parse_squash_approve() { + let cmds = parse_commands("@bors r+ squash"); + insta::assert_debug_snapshot!(cmds, @" + [ + Ok( + SquashApprove { + commit_message: AutoGenerate, + approver: Myself, + priority: None, + rollup: None, + note: None, + }, + ), + ] + "); + } + + #[test] + fn parse_squash_approve_msg() { + let cmds = parse_commands("@bors r+ squash msg=foo"); + insta::assert_debug_snapshot!(cmds, @r#" + [ + Ok( + SquashApprove { + commit_message: Explicit( + "foo", + ), + approver: Myself, + priority: None, + rollup: None, + note: None, + }, + ), + ] + "#); + } + + #[test] + fn parse_squash_approve_message() { + let cmds = parse_commands("@bors r+ squash message=foo"); + insta::assert_debug_snapshot!(cmds, @r#" + [ + Ok( + SquashApprove { + commit_message: Explicit( + "foo", + ), + approver: Myself, + priority: None, + rollup: None, + note: None, + }, + ), + ] + "#); + } + + #[test] + fn parse_squash_approve_message_quoted() { + let cmds = parse_commands(r#"@bors r+ squash message="foo bar baz""#); + insta::assert_debug_snapshot!(cmds, @r#" + [ + Ok( + SquashApprove { + commit_message: Explicit( + "foo bar baz", + ), + approver: Myself, + priority: None, + rollup: None, + note: None, + }, + ), + ] + "#); + } + + #[test] + fn parse_squash_approve_msg_description() { + let cmds = parse_commands("@bors r+ squash msg=description"); + insta::assert_debug_snapshot!(cmds, @" + [ + Ok( + SquashApprove { + commit_message: PullRequestDescription, + approver: Myself, + priority: None, + rollup: None, + note: None, + }, + ), + ] + "); + } + + #[test] + fn parse_squash_approve_unknown_arg() { + let cmds = parse_commands("@bors r+ squash commit=foo"); + insta::assert_debug_snapshot!(cmds, @r#" + [ + Err( + UnknownArg { + arg: "squash", + did_you_mean: "r+ squash [msg|message=\"\"|description]", + }, + ), + ] + "#); + } + + #[test] + fn parse_squash_approve_extra_args() { + let cmds = parse_commands("@bors r+ squash message=foo baz"); + insta::assert_debug_snapshot!(cmds, @r#" + [ + Ok( + SquashApprove { + commit_message: Explicit( + "foo", + ), + approver: Myself, + priority: None, + rollup: None, + note: None, + }, + ), + ] + "#); + } + #[test] fn parse_in_html_command() { let cmds = parse_commands( diff --git a/src/bors/handlers/squash.rs b/src/bors/handlers/squash.rs index 978667fc..7d1a71f5 100644 --- a/src/bors/handlers/squash.rs +++ b/src/bors/handlers/squash.rs @@ -410,7 +410,6 @@ mod tests { BorsTester, Comment, Commit, GitHub, GitUser, PullRequest, Repo, User, default_repo_name, run_test, }; - use std::sync::Arc; #[test] fn parse_coauthor_valid_trailer() { @@ -895,72 +894,6 @@ also include this pls .await; } - async fn approve_add_label(pool: sqlx::PgPool) { - let gh = GitHub::default().append_to_default_config( - r#" -[labels] -approved = ["+approved"] -"#, - ); - run_test((pool, gh), async |ctx: &mut BorsTester| { - ctx.approve(()).await?; - ctx.pr(()).await.expect_added_labels(&["approved"]); - Ok(()) - }) - .await; - } - - #[sqlx::test(migrator = "crate::MIGRATOR")] - async fn squash_two_commits_and_approve(pool: sqlx::PgPool) { - let pool = Arc::new(pool); - let gh = run_test( - ( - ::clone(&*(pool.clone())), - squash_state(), - ), - async |ctx: &mut BorsTester| { - ctx.modify_pr_in_gh((), |pr| { - pr.title = "Foobar".to_string(); - pr.reset_to_single_commit(Commit::from_sha("sha1")); - pr.add_commits(vec![Commit::from_sha("sha2")]); - }); - ctx.post_comment("@bors squash").await?; - ctx.run_gitop_queue().await?; - insta::assert_snapshot!( - ctx.get_next_comment_text(()).await?, - @":construction: Squashing... this can take a few minutes." - ); - insta::assert_snapshot!( - ctx.get_next_comment_text(()).await?, - @":hammer: 2 commits were squashed into sha2-reauthored-to-git-user." - ); - let branch = ctx.pr(()).await.get_gh_pr().head_branch_copy(); - assert_eq!(branch.get_commits().len(), 1); - insta::assert_debug_snapshot!(branch.get_commit(), @r#" - Commit { - sha: "sha2-reauthored-to-git-user", - message: "Foobar\n\n* Commit sha1\n* Commit sha2\n", - author: GitUser { - name: "git-user", - email: "git-user@git.com", - }, - } - "#); - approve_add_label(::clone(&*(pool.clone()))).await; - - ctx.pr(()).await.expect_approved_by("default-user"); - Ok(()) - }, - ) - .await; - insta::assert_snapshot!(gh.get_sha_history((), "pr/1"), @" - pr-1-sha - sha1 - sha2 - sha2-reauthored-to-git-user - "); - } - #[sqlx::test(migrator = "crate::MIGRATOR")] async fn squash_approve_push_webhook(pool: sqlx::PgPool) { run_test((pool, squash_state()), async |ctx: &mut BorsTester| { diff --git a/src/github/rollup.rs b/src/github/rollup.rs index 1a44e7a2..d5f8a001 100644 --- a/src/github/rollup.rs +++ b/src/github/rollup.rs @@ -1353,7 +1353,7 @@ also include this pls" This PR was contained in a rollup (#4), which was closed. "); insta::assert_snapshot!(ctx.get_next_comment_text(4).await?, @" - PR #3, which is a member of this rollup, changed its commit SHA. + PR #3, which is a member of this rollup, changed its commit SHA to foo. This rollup was thus unapproved due to being closed. "); @@ -1434,7 +1434,7 @@ also include this pls" "); insta::assert_snapshot!(ctx.get_next_comment_text(3).await?, @":hourglass: Testing commit pr-3-sha with merge merge-0-pr-3-d7d45f1f-reauthored-to-bors..."); insta::assert_snapshot!(ctx.get_next_comment_text(4).await?, @" - PR #2, which is a member of this rollup, changed its commit SHA. + PR #2, which is a member of this rollup, changed its commit SHA to foobar. This rollup was closed. "); From cbf2076beef7d43d3eb6531ede8bef636f95332c Mon Sep 17 00:00:00 2001 From: David Manca <1900179+medzernik@users.noreply.github.com> Date: Sat, 19 Sep 2026 11:01:58 +0200 Subject: [PATCH 10/11] WIP: so i can continue on laptop xd Signed-off-by: David Manca <1900179+medzernik@users.noreply.github.com> Signed-off-by: medzernik --- src/bors/command/parser.rs | 101 +++++++++++++++++-------------------- 1 file changed, 47 insertions(+), 54 deletions(-) diff --git a/src/bors/command/parser.rs b/src/bors/command/parser.rs index 933effc5..0539b33f 100644 --- a/src/bors/command/parser.rs +++ b/src/bors/command/parser.rs @@ -267,12 +267,11 @@ fn parse_parts(input: &str) -> Result>, CommandParseError> { /// - "@bors r+ [p=] [rollup=] [note=]" /// - "@bors r= [p=] [rollup=] [note=]" fn parser_approval(command: &CommandPart<'_>, parts: &[CommandPart<'_>]) -> ParseResult { - if let CommandPart::Bare("r+") = command - && !parts.is_empty() - && parts.contains(&CommandPart::Bare("squash")) - { - return parser_squash_approve(command, parts); - } + let also_squash = parts + .iter() + .position(|x| *x == CommandPart::Bare("squash")) + .and_then(|x| Some(&parts[x..])); + let approver = match command { CommandPart::Bare("r+") => Approver::Myself, CommandPart::KeyValue { key: "r", value } => { @@ -303,6 +302,47 @@ fn parser_approval(command: &CommandPart<'_>, parts: &[CommandPart<'_>]) -> Pars _ => None, }) .next(); + + // parse the squash part of the approve_squash command + if let Some(also_squash) = also_squash { + match parser_squash(&also_squash[0], &also_squash[1..]) { + None => { + return Some(Err(CommandParseError::UnknownArg { + arg: also_squash[1..][0].as_key().to_owned(), + did_you_mean: "r+ squash [msg|message=\"\"|description]" + .to_string(), + })); + } + Some(val) => match val { + Ok(val) => match val { + BorsCommand::Squash { commit_message, .. } => { + return Some(Ok(BorsCommand::SquashApprove { + commit_message, + approver, + priority, + rollup, + note, + })); + } + _ => { + return Some(Err(CommandParseError::UnknownArg { + arg: also_squash[1..][0].as_key().to_owned(), + did_you_mean: "r+ squash [msg|message=\"\"|description]" + .to_string(), + })); + } + }, + Err(_) => { + return Some(Err(CommandParseError::UnknownArg { + arg: also_squash[1..][0].as_key().to_owned(), + did_you_mean: "r+ squash [msg|message=\"\"|description]" + .to_string(), + })); + } + }, + } + } + Some(Ok(BorsCommand::Approve { approver, priority, @@ -676,53 +716,6 @@ fn parser_squash(command: &CommandPart<'_>, parts: &[CommandPart<'_>]) -> ParseR } } -/// Parses `@bors r+ squash` command. -/// Supports specifying a commit message via `@bors r+ squash [msg|message]="message"`. -fn parser_squash_approve(_: &CommandPart<'_>, parts: &[CommandPart<'_>]) -> ParseResult { - match parts { - &[CommandPart::Bare("squash")] => Some(Ok(BorsCommand::SquashApprove { - commit_message: SquashCommitMessage::AutoGenerate, - approver: Approver::Myself, - priority: None, - rollup: None, - note: None, - })), - &[ - CommandPart::Bare("squash"), - CommandPart::KeyValue { - key: "msg" | "message", - value, - }, - .., - ] => { - if value == "description" { - Some(Ok(BorsCommand::SquashApprove { - commit_message: SquashCommitMessage::PullRequestDescription, - approver: Approver::Myself, - priority: None, - rollup: None, - note: None, - })) - } else { - Some(Ok(BorsCommand::SquashApprove { - commit_message: SquashCommitMessage::Explicit(value.to_owned()), - approver: Approver::Myself, - priority: None, - rollup: None, - note: None, - })) - } - } - [part, ..] => Some(Err(CommandParseError::UnknownArg { - arg: part.as_key().to_owned(), - did_you_mean: "r+ squash [msg|message=\"\"|description]".to_string(), - })), - part => Some(Err(CommandParseError::MissingArgValue { - arg: format!("{:#?}", part), - })), - } -} - #[cfg(test)] mod tests { use crate::bors::command::BorsCommand; @@ -2255,7 +2248,7 @@ for the crater", [ Err( UnknownArg { - arg: "squash", + arg: "commit", did_you_mean: "r+ squash [msg|message=\"\"|description]", }, ), From d4d4926c007b808a292ca716407b708d304d506f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 19 Sep 2026 13:50:13 +0200 Subject: [PATCH 11/11] Finish the `squash_approve_push_webhook` test --- src/bors/handlers/help.rs | 4 +--- src/bors/handlers/mod.rs | 2 +- src/bors/handlers/squash.rs | 20 +++++++++++++------- src/bors/mod.rs | 1 + src/github/rollup.rs | 4 ++-- src/tests/mod.rs | 17 +++++++++++++++++ 6 files changed, 35 insertions(+), 13 deletions(-) diff --git a/src/bors/handlers/help.rs b/src/bors/handlers/help.rs index 04452832..82ae5c66 100644 --- a/src/bors/handlers/help.rs +++ b/src/bors/handlers/help.rs @@ -37,6 +37,7 @@ mod tests { - Optionally, you can specify the `` of the PR and if it is eligible for rollups (`)`. - You can pass a comma-separated list of GitHub usernames. - Optionally, you can attach a `` to the PR that will be displayed on the queue page. + - `r+ squash`: Squash the commits of a PR into a single commit, then approve it. - `r-`: Unapprove this PR - `p= [note=[]]` | `priority= [note=[]]`: Set the priority of this PR - Optionally, you can attach a `` to the PR that will be displayed on the queue page. @@ -65,9 +66,6 @@ mod tests { - `squash [msg|message=|description]`: Squash the commits of a PR into a single commit. - Optionally, you can specify a `` for the created commit. If not specified, the commit messages of all squashed commits will be combined. - If you specify `msg=description`, then the PR body will be used as the squashed commit message. - - `r+ squash [msg|message=|description]`: Squash the commits of a PR into a single commit, then approve on your behalf. - - Optionally, you can specify a `` for the created commit. If not specified, the commit messages of all squashed commits will be combined. - - If you specify `msg=description`, then the PR body will be used as the squashed commit message. - `info`: Get information about the current PR ## Repository management diff --git a/src/bors/handlers/mod.rs b/src/bors/handlers/mod.rs index 92a73cc6..394c8d9d 100644 --- a/src/bors/handlers/mod.rs +++ b/src/bors/handlers/mod.rs @@ -1209,7 +1209,7 @@ pub fn invalidation_comment( let action = match &**reason { InvalidationReason::CommitShaChanged { sha } => { - format!("{} its commit SHA to {sha}", wrap("changed")) + format!("{} its commit SHA to `{sha}`", wrap("changed")) } InvalidationReason::BaseBranchChanged => { format!("{} its base branch", wrap("changed")) diff --git a/src/bors/handlers/squash.rs b/src/bors/handlers/squash.rs index 7d1a71f5..896da832 100644 --- a/src/bors/handlers/squash.rs +++ b/src/bors/handlers/squash.rs @@ -911,14 +911,20 @@ also include this pls ); insta::assert_snapshot!( ctx.get_next_comment_text(()).await?, - @":hammer: 2 commits were squashed into sha2-reauthored-to-git-user." - ); - let branch = ctx.pr(()).await.get_gh_pr().head_branch_copy(); + @" + :pushpin: Commit sha2-reauthored-to-git-user has been approved by `default-user` - // Check that this won't unapprove the PR - ctx.push_to_pr((), branch.get_commit().clone()).await?; - - ctx.pr(()).await.expect_approved_by("default-user"); + It is now in the [queue](https://bors-test.com/queue/borstest) for this repository. + " + ); + // Check that generating a push webhook for the PR's HEAD SHA, because the pushed + // commit was already pre-approved by `@bors r+ squash`. + ctx.send_push_webhook(()).await?; + + ctx.pr(()) + .await + .expect_approved_by("default-user") + .expect_approved_sha("sha2-reauthored-to-git-user"); Ok(()) }) .await; diff --git a/src/bors/mod.rs b/src/bors/mod.rs index 4f0139ab..a29e3e12 100644 --- a/src/bors/mod.rs +++ b/src/bors/mod.rs @@ -120,6 +120,7 @@ You can use the following commands: - Optionally, you can specify the `` of the PR and if it is eligible for rollups (`)`. - You can pass a comma-separated list of GitHub usernames. - Optionally, you can attach a `` to the PR that will be displayed on the queue page. +- `r+ squash`: Squash the commits of a PR into a single commit, then approve it. - `r-`: Unapprove this PR - `p= [note=[]]` | `priority= [note=[]]`: Set the priority of this PR - Optionally, you can attach a `` to the PR that will be displayed on the queue page. diff --git a/src/github/rollup.rs b/src/github/rollup.rs index d5f8a001..90342c14 100644 --- a/src/github/rollup.rs +++ b/src/github/rollup.rs @@ -1353,7 +1353,7 @@ also include this pls" This PR was contained in a rollup (#4), which was closed. "); insta::assert_snapshot!(ctx.get_next_comment_text(4).await?, @" - PR #3, which is a member of this rollup, changed its commit SHA to foo. + PR #3, which is a member of this rollup, changed its commit SHA to `foo`. This rollup was thus unapproved due to being closed. "); @@ -1434,7 +1434,7 @@ also include this pls" "); insta::assert_snapshot!(ctx.get_next_comment_text(3).await?, @":hourglass: Testing commit pr-3-sha with merge merge-0-pr-3-d7d45f1f-reauthored-to-bors..."); insta::assert_snapshot!(ctx.get_next_comment_text(4).await?, @" - PR #2, which is a member of this rollup, changed its commit SHA to foobar. + PR #2, which is a member of this rollup, changed its commit SHA to `foobar`. This rollup was closed. "); diff --git a/src/tests/mod.rs b/src/tests/mod.rs index bfa24fa0..91dd60a5 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -969,6 +969,23 @@ impl BorsTester { self.send_webhook("pull_request", payload).await } + /// Sends commit push webhook for the HEAD commit of the PR identified by `id`. + pub async fn send_push_webhook>( + &mut self, + id: Id, + ) -> anyhow::Result<()> { + let id = id.into(); + let payload = { + let gh = self.github.lock(); + let repo = gh.get_repo(&id.repo); + let repo = repo.lock(); + let pr = repo.pulls().get(&id.number).expect("PR not found"); + GitHubPullRequestEventPayload::new(&repo, &gh, pr.clone(), "synchronize", None) + }; + + self.send_webhook("pull_request", payload).await + } + pub async fn assign_pr>( &mut self, id: Id,