Merge pull request #364 from Kobzol/strip-try-build-pr-message

Strip PR description from merge message in try builds

Author: Kobzol

src/bors/handlers/trybuild.rs

@@ -19,6 +19,7 @@ use crate::github::{CommitSha, GithubUser, LabelTrigger, MergeError, PullRequest
 use crate::permissions::PermissionType;
 use crate::utils::text::suppress_github_mentions;
 use anyhow::{Context, anyhow};
+use itertools::Itertools;
 use octocrab::params::checks::CheckRunConclusion;
 use octocrab::params::checks::CheckRunOutput;
 use octocrab::params::checks::CheckRunStatus;
@@ -87,7 +88,11 @@ pub(super) async fn command_try_build(
         &repo.client,
         &pr.github.head.sha,
         &base_sha,
-        &auto_merge_commit_message(pr, repo.client.repository(), "<try>", jobs),
+        &create_merge_commit_message(
+            pr,
+            repo.client.repository(),
+            MergeType::Try { try_jobs: jobs },
+        ),
     )
     .await?
     {
@@ -339,28 +344,61 @@ fn get_pending_build(pr: &PullRequestModel) -> Option<&BuildModel> {
         .and_then(|b| (b.status == BuildStatus::Pending).then_some(b))
 }
 
-fn auto_merge_commit_message(
+/// Prefix used to specify custom try jobs in PR descriptions.
+const CUSTOM_TRY_JOB_PREFIX: &str = "try-job:";
+
+enum MergeType {
+    Try { try_jobs: Vec<String> },
+}
+
+fn create_merge_commit_message(
     pr: &PullRequestData,
     name: &GithubRepoName,
-    reviewer: &str,
-    jobs: Vec<String>,
+    merge_type: MergeType,
 ) -> String {
     let pr_number = pr.number();
+
+    let reviewer = match &merge_type {
+        MergeType::Try { .. } => "<try>",
+    };
+
+    let mut pr_description = suppress_github_mentions(&pr.github.message);
+    match &merge_type {
+        // Strip all PR text for try builds, to avoid useless issue pings on the repository.
+        // Only keep any lines starting with `CUSTOM_TRY_JOB_PREFIX`.
+        MergeType::Try { try_jobs } => {
+            // If we do not have any custom try jobs, keep the ones that might be in the PR
+            // description.
+            pr_description = if try_jobs.is_empty() {
+                pr_description
+                    .lines()
+                    .map(|l| l.trim())
+                    .filter(|l| l.starts_with(CUSTOM_TRY_JOB_PREFIX))
+                    .join("\n")
+            } else {
+                // If we do have custom jobs, ignore the original description completely
+                String::new()
+            };
+        }
+    };
+
     let mut message = format!(
         r#"Auto merge of {repo_owner}/{repo_name}#{pr_number} - {pr_label}, r={reviewer}
 {pr_title}
 
-{pr_message}"#,
+{pr_description}"#,
         pr_label = pr.github.head_label,
         pr_title = pr.github.title,
-        pr_message = suppress_github_mentions(&pr.github.message),
         repo_owner = name.owner(),
         repo_name = name.name()
     );
 
-    // if jobs is empty, try-job won't be added to the message
-    for job in jobs {
-        message.push_str(&format!("\ntry-job: {}", job));
+    match merge_type {
+        MergeType::Try { try_jobs } => {
+            for job in try_jobs {
+                message.push_str(&format!("\n{CUSTOM_TRY_JOB_PREFIX} {}", job));
+            }
+        }
     }
     message
 }
@@ -469,6 +507,92 @@ mod tests {
         .await;
     }
 
+    #[sqlx::test]
+    async fn try_commit_message_strip_description(pool: sqlx::PgPool) {
+        run_test(pool, async |tester: &mut BorsTester| {
+            tester
+                .edit_pr(default_repo_name(), default_pr_number(), |pr| {
+                    pr.description = r"This is a very good PR.
+
+It fixes so many issues, sir."
+                        .to_string();
+                })
+                .await?;
+
+            tester.post_comment("@bors try").await?;
+            tester.expect_comments(1).await;
+
+            insta::assert_snapshot!(tester.get_branch_commit_message(&tester.try_branch()), @r"
+            Auto merge of rust-lang/borstest#1 - pr-1, r=<try>
+            PR #1
+            ");
+            Ok(())
+        })
+        .await;
+    }
+
+    #[sqlx::test]
+    async fn try_commit_message_strip_description_keep_try_jobs(pool: sqlx::PgPool) {
+        run_test(pool, async |tester: &mut BorsTester| {
+            tester
+                .edit_pr(default_repo_name(), default_pr_number(), |pr| {
+                    pr.description = r"This is a very good PR.
+
+try-job: Foo
+
+It fixes so many issues, sir.
+
+try-job: Bar
+"
+                    .to_string();
+                })
+                .await?;
+
+            tester.post_comment("@bors try").await?;
+            tester.expect_comments(1).await;
+
+            insta::assert_snapshot!(tester.get_branch_commit_message(&tester.try_branch()), @r"
+            Auto merge of rust-lang/borstest#1 - pr-1, r=<try>
+            PR #1
+
+            try-job: Foo
+            try-job: Bar
+            ");
+            Ok(())
+        })
+        .await;
+    }
+
+    #[sqlx::test]
+    async fn try_commit_message_overwrite_try_jobs(pool: sqlx::PgPool) {
+        run_test(pool, async |tester: &mut BorsTester| {
+            tester
+                .edit_pr(default_repo_name(), default_pr_number(), |pr| {
+                    pr.description = r"This is a very good PR.
+
+try-job: Foo
+try-job: Bar
+"
+                    .to_string();
+                })
+                .await?;
+
+            tester.post_comment("@bors try jobs=Baz,Baz2").await?;
+            tester.expect_comments(1).await;
+
+            insta::assert_snapshot!(tester.get_branch_commit_message(&tester.try_branch()), @r"
+            Auto merge of rust-lang/borstest#1 - pr-1, r=<try>
+            PR #1
+
+
+            try-job: Baz
+            try-job: Baz2
+            ");
+            Ok(())
+        })
+        .await;
+    }
+
     #[sqlx::test]
     async fn try_merge_branch_history(pool: sqlx::PgPool) {
         let gh = run_test(pool, async |tester| {

src/github/mod.rs

@@ -83,7 +83,7 @@ impl From<octocrab::models::Author> for GithubUser {
     }
 }
 
-#[derive(Clone, Debug, PartialEq)]
+#[derive(Clone, Debug, PartialEq, Eq, Hash)]
 pub struct CommitSha(pub String);
 
 impl From<String> for CommitSha {

src/tests/mocks/bors.rs

@@ -47,7 +47,7 @@ use crate::{
 
 /// How long should we wait before we timeout a test.
 /// You can increase this if you want to do interactive debugging.
-const TEST_TIMEOUT: Duration = Duration::from_secs(10);
+const TEST_TIMEOUT: Duration = Duration::from_secs(100);
 
 pub fn default_cmd_prefix() -> CommandPrefix {
     "@bors".to_string().into()
@@ -277,6 +277,13 @@ impl BorsTester {
             .clone()
     }
 
+    pub fn get_branch_commit_message(&self, branch: &Branch) -> String {
+        self.github
+            .default_repo()
+            .lock()
+            .get_commit_message(branch.get_sha())
+    }
+
     pub async fn push_to_branch(&mut self, branch: &str) -> anyhow::Result<()> {
         self.send_webhook("push", GitHubPushEventPayload::new(branch))
             .await

src/tests/mocks/pull_request.rs

@@ -199,28 +199,43 @@ pub struct GitHubPullRequest {
 
 impl From<PullRequest> for GitHubPullRequest {
     fn from(pr: PullRequest) -> Self {
-        let number = pr.number.0;
+        let PullRequest {
+            number,
+            repo: _,
+            added_labels: _,
+            removed_labels: _,
+            comment_counter: _,
+            head_sha,
+            author,
+            base_branch,
+            mergeable_state,
+            status,
+            merged_at,
+            closed_at,
+            assignees,
+            description,
+        } = pr;
         GitHubPullRequest {
-            user: pr.author.clone().into(),
+            user: author.clone().into(),
             url: "https://test.com".to_string(),
-            id: number + 1000,
+            id: number.0 + 1000,
             title: format!("PR #{number}"),
-            body: format!("Description of PR #{number}"),
-            mergeable_state: pr.mergeable_state,
-            draft: pr.status == PullRequestStatus::Draft,
-            number,
+            body: description,
+            mergeable_state,
+            draft: status == PullRequestStatus::Draft,
+            number: number.0,
             head: Box::new(GitHubHead {
                 label: format!("pr-{number}"),
                 ref_field: format!("pr-{number}"),
-                sha: pr.head_sha,
+                sha: head_sha,
             }),
             base: Box::new(GitHubBase {
-                ref_field: pr.base_branch.get_name().to_string(),
-                sha: pr.base_branch.get_sha().to_string(),
+                ref_field: base_branch.get_name().to_string(),
+                sha: base_branch.get_sha().to_string(),
             }),
-            merged_at: pr.merged_at,
-            closed_at: pr.closed_at,
-            assignees: pr.assignees.into_iter().map(Into::into).collect(),
+            merged_at,
+            closed_at,
+            assignees: assignees.into_iter().map(Into::into).collect(),
         }
     }
 }

src/tests/mocks/repository.rs

@@ -54,6 +54,7 @@ pub struct PullRequest {
     pub merged_at: Option<DateTime<Utc>>,
     pub closed_at: Option<DateTime<Utc>>,
     pub assignees: Vec<User>,
+    pub description: String,
 }
 
 impl PullRequest {
@@ -76,6 +77,7 @@ impl PullRequest {
             merged_at: None,
             closed_at: None,
             assignees: Vec::new(),
+            description: format!("Description of PR {number}"),
         }
     }
 }
@@ -153,6 +155,7 @@ pub struct Repo {
     pub permissions: Permissions,
     pub config: String,
     pub branches: Vec<Branch>,
+    pub commit_messages: HashMap<String, String>,
     pub workflows_cancelled_by_bors: Vec<u64>,
     pub workflow_cancel_error: bool,
     /// All workflows that we know about from the side of the test.
@@ -172,6 +175,7 @@ impl Repo {
             config,
             pull_requests: Default::default(),
             branches: vec![Branch::default()],
+            commit_messages: Default::default(),
             workflows_cancelled_by_bors: vec![],
             workflow_cancel_error: false,
             workflow_runs: vec![],
@@ -243,6 +247,18 @@ impl Repo {
         self.pr_push_counter += 1;
         self.pr_push_counter
     }
+
+    pub fn get_commit_message(&self, sha: &str) -> String {
+        self.commit_messages
+            .get(sha)
+            .expect("Commit message not found")
+            .clone()
+    }
+
+    pub fn set_commit_message(&mut self, sha: &str, message: &str) {
+        self.commit_messages
+            .insert(sha.to_string(), message.to_string());
+    }
 }
 
 /// Represents the default repository for tests.
@@ -285,7 +301,6 @@ pub fn default_repo_name() -> GithubRepoName {
 pub struct Branch {
     name: String,
     sha: String,
-    commit_message: String,
     sha_history: Vec<String>,
     merge_counter: u64,
     pub merge_conflict: bool,
@@ -296,7 +311,6 @@ impl Branch {
         Self {
             name: name.to_string(),
             sha: sha.to_string(),
-            commit_message: format!("Commit {sha}"),
             sha_history: vec![],
             merge_counter: 0,
             merge_conflict: false,
@@ -549,7 +563,7 @@ async fn mock_merge_branch(repo: Arc<Mutex<Repo>>, mock_server: &MockServer) {
             );
             base_branch.merge_counter += 1;
             base_branch.set_to_sha(&merge_sha);
-            base_branch.commit_message = data.commit_message;
+            repo.set_commit_message(&merge_sha, &data.commit_message);
 
             #[derive(serde::Serialize)]
             struct MergeResponse {