Merge pull request #1381 from rylev/better-commit-message

Mark-Simulacrum · web-flow · commit b7b11a621bfc · 2022-08-02T07:46:07.000-04:00
Make unrolled commit message more descriptive
diff --git a/site/src/github.rs b/site/src/github.rs
@@ -10,17 +10,58 @@ type BoxedError = Box<dyn std::error::Error + Send + Sync>;
 
 pub use comparison_summary::post_finished;
 
+/// Enqueues try build artifacts and posts a message about them on the original rollup PR
+pub async fn unroll_rollup(
+    ci_client: client::Client,
+    main_repo_client: client::Client,
+    rollup_merges: impl Iterator<Item = &Commit>,
+    previous_master: &str,
+    rollup_pr_number: u32,
+) -> Result<(), String> {
+    let mapping = enqueue_unrolled_try_builds(ci_client, rollup_merges, previous_master)
+        .await?
+        .into_iter()
+        .fold(String::new(), |mut string, c| {
+            use std::fmt::Write;
+            write!(
+                &mut string,
+                "|#{pr}|[{commit}](https://github.com/rust-lang-ci/rust/commit/{commit})|\n",
+                pr = c.original_pr_number,
+                commit = c.sha
+            )
+            .unwrap();
+            string
+        });
+    let msg =
+        format!("📌 Perf builds for each rolled up PR:\n\n\
+        |PR# | Perf Build Sha|\n|----|-----|\n\
+        {mapping}\nIn the case of a perf regression, \
+        run the following command for each PR you suspect might be the cause: `@rust-timer build $SHA`");
+    main_repo_client.post_comment(rollup_pr_number, msg).await;
+    Ok(())
+}
+
 /// Enqueues try builds on the try-perf branch for every rollup merge in `rollup_merges`.
 /// Returns a mapping between the rollup merge commit and the try build sha.
-///
-/// `rollup_merges` must only include actual rollup merge commits.
-pub async fn enqueue_unrolled_try_builds<'a>(
+async fn enqueue_unrolled_try_builds<'a>(
     client: client::Client,
     rollup_merges: impl Iterator<Item = &'a Commit>,
     previous_master: &str,
-) -> Result<Vec<(&'a Commit, String)>, String> {
+) -> Result<Vec<UnrolledCommit<'a>>, String> {
     let mut mapping = Vec::new();
     for rollup_merge in rollup_merges {
+        // Grab the number of the rolled up PR from its commit message
+        let original_pr_number = ROLLEDUP_PR_NUMBER
+            .captures(&rollup_merge.message)
+            .and_then(|c| c.get(1))
+            .map(|m| m.as_str())
+            .ok_or_else(|| {
+                format!(
+                    "Could not get PR number from message: '{}'",
+                    rollup_merge.message
+                )
+            })?;
+
         // Fetch the rollup merge commit which should have two parents.
         // The first parent is in the chain of rollup merge commits all the way back to `previous_master`.
         // The second parent is the head of the PR that was rolled up. We want the second parent.
@@ -45,7 +86,11 @@ pub async fn enqueue_unrolled_try_builds<'a>(
 
         // Merge in the rolled up PR's head commit into the previous master
         let sha = client
-            .merge_branch("perf-tmp", rolled_up_head, "merge")
+            .merge_branch(
+                "perf-tmp",
+                rolled_up_head,
+                &format!("Unrolled build for #{}", original_pr_number),
+            )
             .await
             .map_err(|e| format!("Error merging commit into perf-tmp: {e:?}"))?;
 
@@ -55,17 +100,33 @@ pub async fn enqueue_unrolled_try_builds<'a>(
             .await
             .map_err(|e| format!("Error updating the try-perf branch: {e:?}"))?;
 
-        mapping.push((rollup_merge, sha));
+        mapping.push(UnrolledCommit {
+            original_pr_number,
+            rollup_merge,
+            sha,
+        });
         // Wait to ensure there's enough time for GitHub to checkout these changes before they are overwritten
         tokio::time::sleep(std::time::Duration::from_secs(15)).await
     }
 
     Ok(mapping)
 }
 
+/// A commit representing a rolled up PR as if it had been merged into master directly
+pub struct UnrolledCommit<'a> {
+    /// The PR number that was rolled up
+    pub original_pr_number: &'a str,
+    /// The original rollup merge commit
+    pub rollup_merge: &'a Commit,
+    /// The sha of the new unrolled merge commit
+    pub sha: String,
+}
+
 lazy_static::lazy_static! {
     static ref ROLLUP_PR_NUMBER: regex::Regex =
         regex::Regex::new(r#"^Auto merge of #(\d+)"#).unwrap();
+    static ref ROLLEDUP_PR_NUMBER: regex::Regex =
+        regex::Regex::new(r#"^Rollup merge of #(\d+)"#).unwrap();
 }
 
 // Gets the pr number for the associated rollup PR message. Returns None if this is not a rollup PR
@@ -101,43 +162,49 @@ pub async fn rollup_pr_number(
         .then(|| issue.number))
 }
 
-pub async fn enqueue_sha(
+pub async fn enqueue_shas(
     ctxt: &SiteCtxt,
     main_client: &client::Client,
     ci_client: &client::Client,
     pr_number: u32,
-    commit: String,
+    commits: impl Iterator<Item = &str>,
 ) -> Result<(), String> {
-    let commit_response = ci_client
-        .get_commit(&commit)
-        .await
-        .map_err(|e| e.to_string())?;
-    if commit_response.parents.len() != 2 {
-        log::error!(
-            "Bors try commit {} unexpectedly has {} parents.",
-            commit_response.sha,
-            commit_response.parents.len()
-        );
-        return Ok(());
-    }
-    let try_commit = TryCommit {
-        sha: commit_response.sha.clone(),
-        parent_sha: commit_response.parents[0].sha.clone(),
-    };
-    let queued = {
-        let conn = ctxt.conn().await;
-        conn.pr_attach_commit(pr_number, &try_commit.sha, &try_commit.parent_sha)
+    let mut msg = String::new();
+    for commit in commits {
+        let mut commit_response = ci_client
+            .get_commit(&commit)
             .await
-    };
-    if queued {
-        let msg = format!(
-            "Queued {} with parent {}, future [comparison URL]({}).",
-            try_commit.sha,
-            try_commit.parent_sha,
-            try_commit.comparison_url(),
-        );
-        main_client.post_comment(pr_number, msg).await;
+            .map_err(|e| e.to_string())?;
+        if commit_response.parents.len() != 2 {
+            log::error!(
+                "Bors try commit {} unexpectedly has {} parents.",
+                commit_response.sha,
+                commit_response.parents.len()
+            );
+            return Ok(());
+        }
+        let try_commit = TryCommit {
+            sha: commit_response.sha,
+            parent_sha: commit_response.parents.remove(0).sha,
+        };
+        let queued = {
+            let conn = ctxt.conn().await;
+            conn.pr_attach_commit(pr_number, &try_commit.sha, &try_commit.parent_sha)
+                .await
+        };
+        if queued {
+            if !msg.is_empty() {
+                msg.push('\n');
+            }
+            msg.push_str(&format!(
+                "Queued {} with parent {}, future [comparison URL]({}).",
+                try_commit.sha,
+                try_commit.parent_sha,
+                try_commit.comparison_url(),
+            ));
+        }
     }
+    main_client.post_comment(pr_number, msg).await;
     Ok(())
 }
 
diff --git a/site/src/request_handlers/github.rs b/site/src/request_handlers/github.rs
@@ -1,21 +1,15 @@
 use crate::api::{github, ServerResult};
-use crate::github::{
-    client, enqueue_sha, enqueue_unrolled_try_builds, parse_homu_comment, rollup_pr_number,
-};
+use crate::github::{client, enqueue_shas, parse_homu_comment, rollup_pr_number, unroll_rollup};
 use crate::load::SiteCtxt;
 
 use std::sync::Arc;
 
 use regex::Regex;
 
 lazy_static::lazy_static! {
-    static ref ROLLUP_PR_NUMBER: Regex =
-        Regex::new(r#"^Auto merge of #(\d+)"#).unwrap();
-    static ref ROLLEDUP_PR_NUMBER: Regex =
-        Regex::new(r#"^Rollup merge of #(\d+)"#).unwrap();
-    static ref BODY_TRY_COMMIT: Regex =
+    static ref BODY_TIMER_BUILD: Regex =
         Regex::new(r#"(?:\W|^)@rust-timer\s+build\s+(\w+)(?:\W|$)(?:include=(\S+))?\s*(?:exclude=(\S+))?\s*(?:runs=(\d+))?"#).unwrap();
-    static ref BODY_QUEUE: Regex =
+    static ref BODY_TIMER_QUEUE: Regex =
         Regex::new(r#"(?:\W|^)@rust-timer\s+queue(?:\W|$)(?:include=(\S+))?\s*(?:exclude=(\S+))?\s*(?:runs=(\d+))?"#).unwrap();
 }
 
@@ -54,10 +48,15 @@ async fn handle_push(ctxt: Arc<SiteCtxt>, push: github::Push) -> ServerResult<gi
     // GitHub webhooks have a timeout of 10 seconds, so we process this
     // in the background.
     tokio::spawn(async move {
-        let result = handle_rollup_merge(
+        let rollup_merges = commits
+            .iter()
+            .rev()
+            .skip(1) // skip the head commit
+            .take_while(|c| c.message.starts_with("Rollup merge of "));
+        let result = unroll_rollup(
             ci_client,
             main_repo_client,
-            commits,
+            rollup_merges,
             &previous_master,
             rollup_pr_number,
         )
@@ -67,54 +66,6 @@ async fn handle_push(ctxt: Arc<SiteCtxt>, push: github::Push) -> ServerResult<gi
     Ok(github::Response)
 }
 
-/// Handler for when a rollup has been merged
-async fn handle_rollup_merge(
-    ci_client: client::Client,
-    main_repo_client: client::Client,
-    commits: Vec<github::Commit>,
-    previous_master: &str,
-    rollup_pr_number: u32,
-) -> Result<(), String> {
-    let rollup_merges = commits
-        .iter()
-        .rev()
-        .skip(1) // skip the head commit
-        .take_while(|c| c.message.starts_with("Rollup merge of "));
-    let mapping = enqueue_unrolled_try_builds(ci_client, rollup_merges, previous_master).await?;
-    let mapping = mapping
-        .into_iter()
-        .map(|(rollup_merge, sha)| {
-            ROLLEDUP_PR_NUMBER
-                .captures(&rollup_merge.message)
-                .and_then(|c| c.get(1))
-                .map(|m| (m.as_str(), sha))
-                .ok_or_else(|| {
-                    format!(
-                        "Could not get PR number from message: '{}'",
-                        rollup_merge.message
-                    )
-                })
-        })
-        .fold(ServerResult::Ok(String::new()), |string, n| {
-            use std::fmt::Write;
-            let (pr, commit) = n?;
-            let mut string = string?;
-            write!(
-                &mut string,
-                "|#{pr}|[{commit}](https://github.com/rust-lang-ci/rust/commit/{commit})|\n"
-            )
-            .unwrap();
-            Ok(string)
-        })?;
-    let msg =
-        format!("📌 Perf builds for each rolled up PR:\n\n\
-        |PR# | Perf Build Sha|\n|----|-----|\n\
-        {mapping}\nIn the case of a perf regression, \
-        run the following command for each PR you suspect might be the cause: `@rust-timer build $SHA`");
-    main_repo_client.post_comment(rollup_pr_number, msg).await;
-    Ok(())
-}
-
 async fn handle_issue(
     ctxt: Arc<SiteCtxt>,
     issue: github::Issue,
@@ -130,7 +81,14 @@ async fn handle_issue(
     );
     if comment.body.contains(" homu: ") {
         if let Some(sha) = parse_homu_comment(&comment.body).await {
-            enqueue_sha(&ctxt, &main_client, &ci_client, issue.number, sha).await?;
+            enqueue_shas(
+                &ctxt,
+                &main_client,
+                &ci_client,
+                issue.number,
+                std::iter::once(sha.as_str()),
+            )
+            .await?;
             return Ok(github::Response);
         }
     }
@@ -161,7 +119,7 @@ async fn handle_rust_timer(
         return Ok(github::Response);
     }
 
-    if let Some(captures) = BODY_QUEUE.captures(&comment.body) {
+    if let Some(captures) = BODY_TIMER_QUEUE.captures(&comment.body) {
         let include = captures.get(1).map(|v| v.as_str());
         let exclude = captures.get(2).map(|v| v.as_str());
         let runs = captures.get(3).and_then(|v| v.as_str().parse::<i32>().ok());
@@ -179,30 +137,43 @@ async fn handle_rust_timer(
             .await;
         return Ok(github::Response);
     }
-    if let Some(captures) = BODY_TRY_COMMIT.captures(&comment.body) {
-        if let Some(commit) = captures.get(1).map(|c| c.as_str().to_owned()) {
-            let include = captures.get(2).map(|v| v.as_str());
-            let exclude = captures.get(3).map(|v| v.as_str());
-            let runs = captures.get(4).and_then(|v| v.as_str().parse::<i32>().ok());
-            let commit = commit.trim_start_matches("https://github.com/rust-lang/rust/commit/");
-            {
-                let conn = ctxt.conn().await;
-                conn.queue_pr(issue.number, include, exclude, runs).await;
-            }
-            enqueue_sha(
-                &ctxt,
-                &main_client,
-                &ci_client,
-                issue.number,
-                commit.to_owned(),
-            )
-            .await?;
-            return Ok(github::Response);
+
+    for captures in build_captures(&comment).map(|(_, captures)| captures) {
+        let include = captures.get(2).map(|v| v.as_str());
+        let exclude = captures.get(3).map(|v| v.as_str());
+        let runs = captures.get(4).and_then(|v| v.as_str().parse::<i32>().ok());
+        {
+            let conn = ctxt.conn().await;
+            conn.queue_pr(issue.number, include, exclude, runs).await;
         }
     }
+
+    enqueue_shas(
+        &ctxt,
+        &main_client,
+        &ci_client,
+        issue.number,
+        build_captures(&comment).map(|(commit, _)| commit),
+    )
+    .await?;
+
     Ok(github::Response)
 }
 
+/// Run the `@rust-timer build` regex over the comment message extracting the commit and the other captures
+fn build_captures(comment: &github::Comment) -> impl Iterator<Item = (&str, regex::Captures)> {
+    BODY_TIMER_BUILD
+        .captures_iter(&comment.body)
+        .filter_map(|captures| {
+            captures.get(1).map(|m| {
+                let commit = m
+                    .as_str()
+                    .trim_start_matches("https://github.com/rust-lang/rust/commit/");
+                (commit, captures)
+            })
+        })
+}
+
 pub async fn get_authorized_users() -> Result<Vec<usize>, String> {
     let url = format!("{}/permissions/perf.json", ::rust_team_data::v1::BASE_URL);
     let client = reqwest::Client::new();
