Use a manual parser for @rust-timer commands

site/src/request_handlers/github.rs

@@ -6,14 +6,8 @@ use crate::github::{
 use crate::load::SiteCtxt;
 
 use hashbrown::HashMap;
-use regex::Regex;
 use std::sync::Arc;
 
-lazy_static::lazy_static! {
-    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();
-}
-
 pub async fn handle_github(
     request: github::Request,
     ctxt: Arc<SiteCtxt>,
@@ -120,8 +114,13 @@ async fn handle_rust_timer(
         let msg = match queue {
             Ok(cmd) => {
                 let conn = ctxt.conn().await;
-                conn.queue_pr(issue.number, cmd.include, cmd.exclude, cmd.runs)
-                    .await;
+                conn.queue_pr(
+                    issue.number,
+                    cmd.params.include,
+                    cmd.params.exclude,
+                    cmd.params.runs,
+                )
+                .await;
                 format!(
                     "Awaiting bors try build completion.
 
@@ -138,13 +137,30 @@ async fn handle_rust_timer(
         return Ok(github::Response);
     }
 
-    for captures in build_captures(&comment.body).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;
+    let build_cmds: Vec<_> = parse_build_commands(&comment.body).collect();
+    let mut valid_build_cmds = vec![];
+    let mut errors = String::new();
+    for cmd in build_cmds {
+        match cmd {
+            Ok(cmd) => valid_build_cmds.push(cmd),
+            Err(error) => errors.push_str(&format!("Cannot parse build command: {error}\n")),
+        }
+    }
+    if !errors.is_empty() {
+        main_client.post_comment(issue.number, errors).await;
+        return Ok(github::Response);
+    }
+
+    {
+        let conn = ctxt.conn().await;
+        for command in &valid_build_cmds {
+            conn.queue_pr(
+                issue.number,
+                command.params.include,
+                command.params.exclude,
+                command.params.runs,
+            )
+            .await;
         }
     }
 
@@ -153,47 +169,82 @@ async fn handle_rust_timer(
         main_client,
         ci_client,
         issue.number,
-        build_captures(&comment.body).map(|(commit, _)| commit),
+        valid_build_cmds.iter().map(|c| c.sha),
     )
     .await?;
 
     Ok(github::Response)
 }
 
-/// Parses the first occurrence of a `@rust-timer queue <...>` command
+/// Parses the first occurrence of a `@rust-timer queue <shared-args>` command
 /// in the input string.
 fn parse_queue_command(body: &str) -> Option<Result<QueueCommand, String>> {
-    let prefix = "@rust-timer";
-    let bot_line = body.lines().find_map(|line| {
-        line.find(prefix)
-            .map(|index| line[index + prefix.len()..].trim())
-    })?;
-
-    let args = bot_line.strip_prefix("queue").map(|l| l.trim())?;
-    let mut args = match parse_command_arguments(args) {
+    let args = get_command_lines(body, "queue").next()?;
+    let args = match parse_command_arguments(args) {
         Ok(args) => args,
         Err(error) => return Some(Err(error)),
     };
-    let mut cmd = QueueCommand {
+    let params = match parse_benchmark_parameters(args) {
+        Ok(params) => params,
+        Err(error) => return Some(Err(error)),
+    };
+
+    Some(Ok(QueueCommand { params }))
+}
+
+/// Parses all occurrences of a `@rust-timer build <shared-args>` command in the input string.
+fn parse_build_commands(body: &str) -> impl Iterator<Item = Result<BuildCommand, String>> {
+    get_command_lines(body, "build").map(|line| {
+        let mut iter = line.splitn(2, ' ');
+        let Some(sha) = iter.next().filter(|s| !s.is_empty() && !s.contains('=')) else {
+            return Err("Missing SHA in build command".to_string());
+        };
+
+        let sha = sha.trim_start_matches("https://github.com/rust-lang/rust/commit/");
+        let args = iter.next().unwrap_or("");
+        let args = parse_command_arguments(args)?;
+        let params = parse_benchmark_parameters(args)?;
+        Ok(BuildCommand { sha, params })
+    })
+}
+
+fn get_command_lines<'a: 'b, 'b>(
+    body: &'a str,
+    command: &'b str,
+) -> impl Iterator<Item = &'a str> + 'b {
+    let prefix = "@rust-timer";
+    body.lines()
+        .filter_map(move |line| {
+            line.find(prefix)
+                .map(|index| line[index + prefix.len()..].trim())
+        })
+        .filter_map(move |line| line.strip_prefix(command))
+        .map(move |l| l.trim_start())
+}
+
+fn parse_benchmark_parameters<'a>(
+    mut args: HashMap<&'a str, &'a str>,
+) -> Result<BenchmarkParameters<'a>, String> {
+    let mut params = BenchmarkParameters {
         include: args.remove("include"),
         exclude: args.remove("exclude"),
         runs: None,
     };
     if let Some(runs) = args.remove("runs") {
         let Ok(runs) = runs.parse::<u32>() else {
-            return Some(Err(format!("Cannot parse runs {runs} as a number")));
+            return Err(format!("Cannot parse runs {runs} as a number"));
         };
-        cmd.runs = Some(runs as i32);
+        params.runs = Some(runs as i32);
     }
 
     if !args.is_empty() {
-        return Some(Err(format!(
+        Err(format!(
             "Unknown command argument(s) `{}`",
             args.into_keys().collect::<Vec<_>>().join(",")
-        )));
+        ))
+    } else {
+        Ok(params)
     }
-
-    Some(Ok(cmd))
 }
 
 /// Parses command arguments from a single line of text.
@@ -219,25 +270,22 @@ fn parse_command_arguments(args: &str) -> Result<HashMap<&str, &str>, String> {
 
 #[derive(Debug)]
 struct QueueCommand<'a> {
+    params: BenchmarkParameters<'a>,
+}
+
+#[derive(Debug)]
+struct BuildCommand<'a> {
+    sha: &'a str,
+    params: BenchmarkParameters<'a>,
+}
+
+#[derive(Debug)]
+struct BenchmarkParameters<'a> {
     include: Option<&'a str>,
     exclude: Option<&'a str>,
     runs: Option<i32>,
 }
 
-/// Run the `@rust-timer build` regex over the comment message extracting the commit and the other captures
-fn build_captures(comment_body: &str) -> 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<u64>, String> {
     let url = format!("{}/permissions/perf.json", ::rust_team_data::v1::BASE_URL);
     let client = reqwest::Client::new();
@@ -259,39 +307,69 @@ mod tests {
     use super::*;
 
     #[test]
-    fn captures_all_shas() {
-        let comment_body = r#"
-Going to do perf runs for a few of these:
-
-@rust-timer build 5832462aa1d9373b24ace96ad2c50b7a18af9952 (https://github.com/rust-lang/rust/pull/100307)
-@rust-timer build 23936af287657fa4148aeab40cc2a780810fae52 (https://github.com/rust-lang/rust/pull/100392)
-        "#;
-        let shas = build_captures(comment_body)
-            .map(|(c, _)| c)
-            .collect::<Vec<_>>();
-        assert_eq!(
-            shas,
-            &[
-                "5832462aa1d9373b24ace96ad2c50b7a18af9952",
-                "23936af287657fa4148aeab40cc2a780810fae52"
-            ]
-        );
+    fn build_command_missing() {
+        assert!(get_build_commands("").is_empty());
+    }
+
+    #[test]
+    fn build_unknown_command() {
+        assert!(get_build_commands("@rust-timer foo").is_empty());
+    }
+
+    #[test]
+    fn build_command_missing_sha() {
+        insta::assert_compact_debug_snapshot!(get_build_commands("@rust-timer build"),
+            @r###"[Err("Missing SHA in build command")]"###);
+    }
+
+    #[test]
+    fn build_command() {
+        insta::assert_compact_debug_snapshot!(get_build_commands("@rust-timer build 5832462aa1d9373b24ace96ad2c50b7a18af9952"),
+            @r###"[Ok(BuildCommand { sha: "5832462aa1d9373b24ace96ad2c50b7a18af9952", params: BenchmarkParameters { include: None, exclude: None, runs: None } })]"###);
+    }
+
+    #[test]
+    fn build_command_multiple() {
+        insta::assert_compact_debug_snapshot!(get_build_commands(r#"
+@rust-timer build 5832462aa1d9373b24ace96ad2c50b7a18af9952
+@rust-timer build 23936af287657fa4148aeab40cc2a780810fae52
+"#),
+            @r###"[Ok(BuildCommand { sha: "5832462aa1d9373b24ace96ad2c50b7a18af9952", params: BenchmarkParameters { include: None, exclude: None, runs: None } }), Ok(BuildCommand { sha: "23936af287657fa4148aeab40cc2a780810fae52", params: BenchmarkParameters { include: None, exclude: None, runs: None } })]"###);
     }
 
     #[test]
-    fn command_missing() {
+    fn build_command_unknown_arg() {
+        insta::assert_compact_debug_snapshot!(get_build_commands("@rust-timer build foo=bar"),
+            @r###"[Err("Missing SHA in build command")]"###);
+    }
+
+    #[test]
+    fn build_command_complex() {
+        insta::assert_compact_debug_snapshot!(get_build_commands("  @rust-timer  build    sha123456  exclude=baz    include=foo,bar runs=4"),
+            @r###"[Ok(BuildCommand { sha: "sha123456", params: BenchmarkParameters { include: Some("foo,bar"), exclude: Some("baz"), runs: Some(4) } })]"###);
+    }
+
+    #[test]
+    fn build_command_link() {
+        insta::assert_compact_debug_snapshot!(get_build_commands(r#"
+@rust-timer build https://github.com/rust-lang/rust/commit/323f521bc6d8f2b966ba7838a3f3ee364e760b7e"#),
+            @r###"[Ok(BuildCommand { sha: "323f521bc6d8f2b966ba7838a3f3ee364e760b7e", params: BenchmarkParameters { include: None, exclude: None, runs: None } })]"###);
+    }
+
+    #[test]
+    fn queue_command_missing() {
         assert!(parse_queue_command("").is_none());
     }
 
     #[test]
-    fn unknown_command() {
+    fn queue_unknown_command() {
         assert!(parse_queue_command("@rust-timer foo").is_none());
     }
 
     #[test]
     fn queue_command() {
         insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue"),
-            @"Some(Ok(QueueCommand { include: None, exclude: None, runs: None }))");
+            @"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None } }))");
     }
 
     #[test]
@@ -309,19 +387,19 @@ Going to do perf runs for a few of these:
     #[test]
     fn queue_command_include() {
         insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue include=abcd,feih"),
-            @r###"Some(Ok(QueueCommand { include: Some("abcd,feih"), exclude: None, runs: None }))"###);
+            @r###"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("abcd,feih"), exclude: None, runs: None } }))"###);
     }
 
     #[test]
     fn queue_command_exclude() {
         insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue exclude=foo134,barzbaz41baf"),
-            @r###"Some(Ok(QueueCommand { include: None, exclude: Some("foo134,barzbaz41baf"), runs: None }))"###);
+            @r###"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: Some("foo134,barzbaz41baf"), runs: None } }))"###);
     }
 
     #[test]
     fn queue_command_runs() {
         insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue runs=5"),
-            @"Some(Ok(QueueCommand { include: None, exclude: None, runs: Some(5) }))");
+            @"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: Some(5) } }))");
     }
 
     #[test]
@@ -333,7 +411,7 @@ Going to do perf runs for a few of these:
     #[test]
     fn queue_command_combination() {
         insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue include=acda,13asd exclude=c13,DA runs=5"),
-            @r###"Some(Ok(QueueCommand { include: Some("acda,13asd"), exclude: Some("c13,DA"), runs: Some(5) }))"###);
+            @r###"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("acda,13asd"), exclude: Some("c13,DA"), runs: Some(5) } }))"###);
     }
 
     #[test]
@@ -345,18 +423,33 @@ Going to do perf runs for a few of these:
     #[test]
     fn queue_command_spaces() {
         insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer     queue     include=abcd,das   "),
-            @r###"Some(Ok(QueueCommand { include: Some("abcd,das"), exclude: None, runs: None }))"###);
+            @r###"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("abcd,das"), exclude: None, runs: None } }))"###);
     }
 
     #[test]
     fn queue_command_with_bors() {
         insta::assert_compact_debug_snapshot!(parse_queue_command("@bors try @rust-timer queue include=foo,bar"),
-            @r###"Some(Ok(QueueCommand { include: Some("foo,bar"), exclude: None, runs: None }))"###);
+            @r###"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("foo,bar"), exclude: None, runs: None } }))"###);
     }
 
     #[test]
     fn queue_command_parameter_order() {
         insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue runs=3 exclude=c,a include=b"),
-        @r###"Some(Ok(QueueCommand { include: Some("b"), exclude: Some("c,a"), runs: Some(3) }))"###);
+        @r###"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("b"), exclude: Some("c,a"), runs: Some(3) } }))"###);
+    }
+
+    #[test]
+    fn queue_command_multiline() {
+        insta::assert_compact_debug_snapshot!(parse_queue_command(r#"Ok, this looks good now.
+Let's do a perf run quickly and then we can merge it.
+
+@bors try @rust-timer queue include=foo,bar
+
+Otherwise LGTM."#),
+            @r###"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("foo,bar"), exclude: None, runs: None } }))"###);
+    }
+
+    fn get_build_commands(body: &str) -> Vec<Result<BuildCommand, String>> {
+        parse_build_commands(body).collect()
     }
 }