From 7229b2a8a9b7b4b1dbd5b2c2c442d6535f101ad5 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Thu, 28 Nov 2024 01:29:25 -0500 Subject: [PATCH 1/3] fix: handle cargo milestone update for PR from GitHub merge queue Cargo has migrated to GitHub merge queue: https://github.com/rust-lang/cargo/pull/14718 The regex doesn't work for merge commit made by GitHub merge queue. This PR adds extra regex pattern to capture that. This regex has been proved working locally. See --- src/handlers/milestone_prs.rs | 52 +++++++++++++++++++++++------------ 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/src/handlers/milestone_prs.rs b/src/handlers/milestone_prs.rs index bf363dae4..cd241a868 100644 --- a/src/handlers/milestone_prs.rs +++ b/src/handlers/milestone_prs.rs @@ -145,24 +145,40 @@ async fn milestone_cargo( // , // but it is a little awkward to use, only works on the default branch, // and this is a bit simpler/faster. However, it is sensitive to the - // specific messages generated by bors, and won't catch things merged - // without bors. - let merge_re = Regex::new("(?:Auto merge of|Merge pull request) #([0-9]+)").unwrap(); - - let pr_nums = commits.iter().filter_map(|commit| { - log::info!( - "getting PR number for cargo commit {} (len={})", - commit.sha, - commit.commit.message.len() - ); - merge_re.captures(&commit.commit.message).map(|cap| { - cap.get(1) - .unwrap() - .as_str() - .parse::() - .expect("digits only") - }) - }); + // specific messages generated by bors or GitHub merge queue, and won't + // catch things merged beyond them. + let merge_re = + Regex::new(r"(?:Auto merge of|Merge pull request) #([0-9]+)|\(#([0-9]+)\)$").unwrap(); + + let pr_nums = commits + .iter() + .filter(|commit| + // Assumptions: + // * A merge commit always has two parent commits. + // * Cargo's PR never got merged as fast-forward / rebase / squash merge. + commit.parents.len() == 2) + .filter_map(|commit| { + log::info!( + "getting PR number for cargo commit {} (len={})", + commit.sha, + commit.commit.message.len() + ); + + let first = commit + .commit + .message + .lines() + .next() + .expect("commit message"); + merge_re.captures(first).map(|cap| { + cap.get(1) + .or_else(|| cap.get(2)) + .unwrap() + .as_str() + .parse::() + .expect("digits only") + }) + }); let milestone = cargo_repo .get_or_create_milestone(gh, release_version, "closed") .await?; From 7cf03cd777838f75ce56188d7f451ced818a5184 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Thu, 28 Nov 2024 16:26:05 -0500 Subject: [PATCH 2/3] refactor: remove unnecessary log lines --- src/handlers/milestone_prs.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/handlers/milestone_prs.rs b/src/handlers/milestone_prs.rs index cd241a868..48150ed50 100644 --- a/src/handlers/milestone_prs.rs +++ b/src/handlers/milestone_prs.rs @@ -158,12 +158,6 @@ async fn milestone_cargo( // * Cargo's PR never got merged as fast-forward / rebase / squash merge. commit.parents.len() == 2) .filter_map(|commit| { - log::info!( - "getting PR number for cargo commit {} (len={})", - commit.sha, - commit.commit.message.len() - ); - let first = commit .commit .message From 60158266b8a397d4f8850d325dfcff3d4890bb61 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Fri, 29 Nov 2024 15:57:54 -0800 Subject: [PATCH 3/3] Allow empty commit message Although this is unlikely (or impossible?) to ever happen in practice, I don't feel comfortable with crashing triagebot if it ever happens. --- src/handlers/milestone_prs.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/handlers/milestone_prs.rs b/src/handlers/milestone_prs.rs index 48150ed50..c2147633f 100644 --- a/src/handlers/milestone_prs.rs +++ b/src/handlers/milestone_prs.rs @@ -158,12 +158,7 @@ async fn milestone_cargo( // * Cargo's PR never got merged as fast-forward / rebase / squash merge. commit.parents.len() == 2) .filter_map(|commit| { - let first = commit - .commit - .message - .lines() - .next() - .expect("commit message"); + let first = commit.commit.message.lines().next().unwrap_or_default(); merge_re.captures(first).map(|cap| { cap.get(1) .or_else(|| cap.get(2))