diff --git a/src/handlers.rs b/src/handlers.rs index ee39a0a0b..377794cf1 100644 --- a/src/handlers.rs +++ b/src/handlers.rs @@ -37,7 +37,6 @@ mod major_change; mod mentions; mod merge_conflicts; mod milestone_prs; -mod no_merges; mod nominate; mod note; mod notification; @@ -228,7 +227,6 @@ issue_handlers! { issue_links, major_change, mentions, - no_merges, notify_zulip, review_requested, pr_tracking, diff --git a/src/handlers/check_commits.rs b/src/handlers/check_commits.rs index 71bcb2e69..128b2ce2e 100644 --- a/src/handlers/check_commits.rs +++ b/src/handlers/check_commits.rs @@ -1,10 +1,13 @@ +use std::collections::HashSet; + use anyhow::bail; +use anyhow::Context as _; use super::Context; use crate::{ config::Config, db::issue_data::IssueData, - github::{Event, IssuesAction, IssuesEvent, ReportedContentClassifiers}, + github::{Event, IssuesAction, IssuesEvent, Label, ReportedContentClassifiers}, }; #[cfg(test)] @@ -13,6 +16,7 @@ use crate::github::GithubCommit; mod issue_links; mod modified_submodule; mod no_mentions; +mod no_merges; mod non_default_branch; /// Key for the state in the database @@ -25,6 +29,8 @@ struct CheckCommitsWarningsState { last_warnings: Vec, /// ID of the most recent warning comment. last_warned_comment: Option, + /// List of the last labels added. + last_labels: Vec, } pub(super) async fn handle(ctx: &Context, event: &Event, config: &Config) -> anyhow::Result<()> { @@ -34,12 +40,17 @@ pub(super) async fn handle(ctx: &Context, event: &Event, config: &Config) -> any if !matches!( event.action, - IssuesAction::Opened | IssuesAction::Synchronize + IssuesAction::Opened | IssuesAction::Synchronize | IssuesAction::ReadyForReview ) || !event.issue.is_pr() { return Ok(()); } + // Don't ping on rollups or draft PRs. + if event.issue.title.starts_with("Rollup of") || event.issue.draft { + return Ok(()); + } + let Some(diff) = event.issue.diff(&ctx.github).await? else { bail!( "expected issue {} to be a PR, but the diff could not be determined", @@ -49,6 +60,7 @@ pub(super) async fn handle(ctx: &Context, event: &Event, config: &Config) -> any let commits = event.issue.commits(&ctx.github).await?; let mut warnings = Vec::new(); + let mut labels = Vec::new(); // Compute the warnings if let Some(assign_config) = &config.assign { @@ -72,14 +84,24 @@ pub(super) async fn handle(ctx: &Context, event: &Event, config: &Config) -> any warnings.extend(issue_links::issue_links_in_commits(issue_links, &commits)); } - handle_warnings(ctx, event, warnings).await + if let Some(no_merges) = &config.no_merges { + if let Some(warn) = + no_merges::merges_in_commits(&event.issue.title, &event.repository, no_merges, &commits) + { + warnings.push(warn.0); + labels.extend(warn.1); + } + } + + handle_warnings_and_labels(ctx, event, warnings, labels).await } // Add, hide or hide&add a comment with the warnings. -async fn handle_warnings( +async fn handle_warnings_and_labels( ctx: &Context, event: &IssuesEvent, warnings: Vec, + labels: Vec, ) -> anyhow::Result<()> { // Get the state of the warnings for this PR in the database. let mut db = ctx.db.get().await; @@ -105,10 +127,8 @@ async fn handle_warnings( let warning = warning_from_warnings(&warnings); let comment = event.issue.post_comment(&ctx.github, &warning).await?; - // Save new state in the database state.data.last_warnings = warnings; state.data.last_warned_comment = Some(comment.node_id); - state.save().await?; } else if warnings.is_empty() { // No warnings to be shown, let's resolve a previous warnings comment, if there was one. if let Some(last_warned_comment_id) = state.data.last_warned_comment { @@ -123,10 +143,46 @@ async fn handle_warnings( state.data.last_warnings = Vec::new(); state.data.last_warned_comment = None; - state.save().await?; } } + // Handle the labels, add the new ones, remove the one no longer required, or don't do anything + if !state.data.last_labels.is_empty() || !labels.is_empty() { + let (labels_to_remove, labels_to_add) = + calculate_label_changes(&state.data.last_labels, &labels); + + // Remove the labels no longer required + if !labels_to_remove.is_empty() { + for label in labels_to_remove { + event + .issue + .remove_label(&ctx.github, &label) + .await + .context("failed to remove a label in check_commits")?; + } + } + + // Add the labels that are now required + if !labels_to_add.is_empty() { + event + .issue + .add_labels( + &ctx.github, + labels_to_add + .into_iter() + .map(|name| Label { name }) + .collect(), + ) + .await + .context("failed to add labels in check_commits")?; + } + + state.data.last_labels = labels; + } + + // Save new state in the database + state.save().await?; + Ok(()) } @@ -139,6 +195,20 @@ fn warning_from_warnings(warnings: &[String]) -> String { format!(":warning: **Warning** :warning:\n\n{}", warnings.join("\n")) } +// Calculate the label changes +fn calculate_label_changes( + previous: &Vec, + current: &Vec, +) -> (Vec, Vec) { + let previous_set: HashSet = previous.into_iter().cloned().collect(); + let current_set: HashSet = current.into_iter().cloned().collect(); + + let removals = previous_set.difference(¤t_set).cloned().collect(); + let additions = current_set.difference(&previous_set).cloned().collect(); + + (removals, additions) +} + #[cfg(test)] fn dummy_commit_from_body(sha: &str, body: &str) -> GithubCommit { use chrono::{DateTime, FixedOffset}; diff --git a/src/handlers/check_commits/no_merges.rs b/src/handlers/check_commits/no_merges.rs new file mode 100644 index 000000000..4a1258f66 --- /dev/null +++ b/src/handlers/check_commits/no_merges.rs @@ -0,0 +1,165 @@ +//! Purpose: When opening a PR, or pushing new changes, check for merge commits +//! and notify the user of our no-merge policy. + +use crate::{ + config::NoMergesConfig, + github::{GithubCommit, Repository}, +}; +use std::collections::BTreeSet; +use std::fmt::Write; + +pub(super) fn merges_in_commits( + issue_title: &str, + repository: &Repository, + config: &NoMergesConfig, + commits: &Vec, +) -> Option<(String, Vec)> { + // Don't trigger if the PR has any of the excluded title segments. + if config + .exclude_titles + .iter() + .any(|s| issue_title.contains(s)) + { + return None; + } + + let mut merge_commits = BTreeSet::new(); + for commit in commits { + if commit.parents.len() > 1 { + merge_commits.insert(&*commit.sha); + } + } + + // No merge commits. + if merge_commits.is_empty() { + return None; + } + + let message = config + .message + .as_deref() + .unwrap_or(&get_default_message( + &repository.full_name, + &repository.default_branch, + merge_commits.into_iter(), + )) + .to_string(); + + Some((message, config.labels.clone())) +} + +fn get_default_message<'a>( + repository_name: &str, + default_branch: &str, + commits: impl IntoIterator, +) -> String { + let mut message = format!( + " +The following commits have merge commits (commits with multiple parents) in your changes. \ +We have a [no merge policy](https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy) \ +so these commits will need to be removed for this pull request to be merged. +" + ); + + for commit in commits { + writeln!(message, " - {commit}").unwrap(); + } + + writeln!( + message, + " + You can start a rebase with the following commands: + ```shell-session + $ # rebase + $ git pull --rebase https://github.com/{repository_name}.git {default_branch} + $ git push --force-with-lease + ```" + ) + .unwrap(); + + message +} + +#[test] +fn end_to_end() { + use super::dummy_commit_from_body; + use crate::github::Parent; + + let config = NoMergesConfig { + exclude_titles: vec!["Subtree update".to_string()], + labels: vec!["merge-commits".to_string()], + message: None, + }; + + let title = "This a PR!"; + let repository = Repository { + full_name: "rust-lang/rust".to_string(), + default_branch: "master".to_string(), + fork: false, + parent: None, + }; + + let commit_with_merge = || { + let mut commit_with_merge = + dummy_commit_from_body("9cc6dce67c917fe5937e984f58f5003ccbb5c37e", "+ main.rs"); + commit_with_merge.parents = vec![ + Parent { + sha: "4fb1228e72cf76549e275c38c226c1c2326ca991".to_string(), + }, + Parent { + sha: "febd545030008f13541064895ae36e19d929a043".to_string(), + }, + ]; + commit_with_merge + }; + + // contains merges + { + let Some((warning, labels)) = merges_in_commits( + &title, + &repository, + &config, + &vec![ + commit_with_merge(), + dummy_commit_from_body("499bdd2d766f98420c66a80a02b7d3ceba4d06ba", "+ nothing.rs"), + ], + ) else { + unreachable!() + }; + assert_eq!(warning, " +The following commits have merge commits (commits with multiple parents) in your changes. We have a [no merge policy](https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy) so these commits will need to be removed for this pull request to be merged. + - 9cc6dce67c917fe5937e984f58f5003ccbb5c37e + + You can start a rebase with the following commands: + ```shell-session + $ # rebase + $ git pull --rebase https://github.com/rust-lang/rust.git master + $ git push --force-with-lease + ``` +"); + assert_eq!(labels, vec!["merge-commits"]); + } + + // doesn't contains merges + { + let commit = dummy_commit_from_body("67c917fe5937e984f58f5003ccbb5c37e", "+ main.rs"); + + assert_eq!( + merges_in_commits(&title, &repository, &config, &vec![commit]), + None + ); + } + + // contains merges, but excluded by title + { + assert_eq!( + merges_in_commits( + "Subtree update of rustc_custom_codegen", + &repository, + &config, + &vec![commit_with_merge()] + ), + None + ); + } +} diff --git a/src/handlers/no_merges.rs b/src/handlers/no_merges.rs deleted file mode 100644 index e94b26f0b..000000000 --- a/src/handlers/no_merges.rs +++ /dev/null @@ -1,266 +0,0 @@ -//! Purpose: When opening a PR, or pushing new changes, check for merge commits -//! and notify the user of our no-merge policy. - -use crate::{ - config::NoMergesConfig, - db::issue_data::IssueData, - github::{IssuesAction, IssuesEvent, Label, ReportedContentClassifiers}, - handlers::Context, -}; -use anyhow::Context as _; -use serde::{Deserialize, Serialize}; -use std::collections::HashSet; -use std::fmt::Write; -use tracing as log; - -const NO_MERGES_KEY: &str = "no_merges"; - -pub(super) struct NoMergesInput { - /// Hashes of merge commits in the pull request. - merge_commits: HashSet, -} - -#[derive(Debug, Default, Deserialize, Serialize)] -struct NoMergesState { - /// Hashes of merge commits that have already been mentioned by triagebot in a comment. - mentioned_merge_commits: HashSet, - /// List of all the no_merge comments as GitHub GraphQL NodeId. - #[serde(default)] - no_merge_comments: Vec, - /// Labels that the bot added as part of the no-merges check. - #[serde(default)] - added_labels: Vec, -} - -pub(super) async fn parse_input( - ctx: &Context, - event: &IssuesEvent, - config: Option<&NoMergesConfig>, -) -> Result, String> { - if !matches!( - event.action, - IssuesAction::Opened | IssuesAction::Synchronize | IssuesAction::ReadyForReview - ) { - return Ok(None); - } - - // Require a `[no_merges]` configuration block to enable no-merges notifications. - let Some(config) = config else { - return Ok(None); - }; - - // Don't ping on rollups or draft PRs. - if event.issue.title.starts_with("Rollup of") || event.issue.draft { - return Ok(None); - } - - // Don't trigger if the PR has any of the excluded title segments. - if config - .exclude_titles - .iter() - .any(|s| event.issue.title.contains(s)) - { - return Ok(None); - } - - let mut merge_commits = HashSet::new(); - let commits = event - .issue - .commits(&ctx.github) - .await - .map_err(|e| { - log::error!("failed to fetch commits: {:?}", e); - }) - .unwrap_or_default(); - for commit in commits { - if commit.parents.len() > 1 { - merge_commits.insert(commit.sha.clone()); - } - } - - // Run the handler even if we have no merge commits, - // so we can take an action if some were removed. - Ok(Some(NoMergesInput { merge_commits })) -} - -fn get_default_message(repository_name: &str, default_branch: &str) -> String { - format!( - " -There are merge commits (commits with multiple parents) in your changes. We have a \ -[no merge policy](https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy) \ -so these commits will need to be removed for this pull request to be merged. - -You can start a rebase with the following commands: -```shell-session -$ # rebase -$ git pull --rebase https://github.com/{repository_name}.git {default_branch} -$ git push --force-with-lease -``` - -" - ) -} - -pub(super) async fn handle_input( - ctx: &Context, - config: &NoMergesConfig, - event: &IssuesEvent, - input: NoMergesInput, -) -> anyhow::Result<()> { - let mut client = ctx.db.get().await; - let mut state: IssueData<'_, NoMergesState> = - IssueData::load(&mut client, &event.issue, NO_MERGES_KEY).await?; - - // No merge commits. - if input.merge_commits.is_empty() { - if state.data.mentioned_merge_commits.is_empty() { - // No merge commits from before, so do nothing. - return Ok(()); - } - - // Merge commits were removed, so remove the labels we added. - for name in state.data.added_labels.iter() { - event - .issue - .remove_label(&ctx.github, name) - .await - .context("failed to remove label")?; - } - - // Minimize prior no_merges comments. - for node_id in state.data.no_merge_comments.iter() { - event - .issue - .hide_comment( - &ctx.github, - node_id.as_str(), - ReportedContentClassifiers::Resolved, - ) - .await - .context("failed to hide previous merge commit comment")?; - } - - // Clear from state. - state.data.mentioned_merge_commits.clear(); - state.data.no_merge_comments.clear(); - state.data.added_labels.clear(); - state.save().await?; - return Ok(()); - } - - let first_time = state.data.mentioned_merge_commits.is_empty(); - - let mut message = config - .message - .as_deref() - .unwrap_or(&get_default_message( - &event.repository.full_name, - &event.repository.default_branch, - )) - .to_string(); - - let since_last_posted = if first_time { - "" - } else { - " (since this message was last posted)" - }; - writeln!( - message, - "The following commits are merge commits{since_last_posted}:" - ) - .unwrap(); - - let mut should_send = false; - for commit in &input.merge_commits { - if state.data.mentioned_merge_commits.contains(commit) { - continue; - } - - should_send = true; - state.data.mentioned_merge_commits.insert((*commit).clone()); - writeln!(message, "- {commit}").unwrap(); - } - - if should_send { - if !first_time { - // Check if the labels are still set. - // Otherwise, they were probably removed manually. - let any_removed = state.data.added_labels.iter().any(|label| { - // No label on the issue matches. - event.issue.labels().iter().all(|l| &l.name != label) - }); - - if any_removed { - // Assume it was a false positive, so don't - // re-add the labels or send a message this time. - state.save().await?; - return Ok(()); - } - } - - let existing_labels = event.issue.labels(); - - let mut labels = Vec::new(); - for name in config.labels.iter() { - // Only add labels not already on the issue. - if existing_labels.iter().all(|l| &l.name != name) { - state.data.added_labels.push(name.clone()); - labels.push(Label { name: name.clone() }); - } - } - - // Set labels - event - .issue - .add_labels(&ctx.github, labels) - .await - .context("failed to set no_merges labels")?; - - // Post comment - let comment = event - .issue - .post_comment(&ctx.github, &message) - .await - .context("failed to post no_merges comment")?; - - state.data.no_merge_comments.push(comment.node_id); - state.save().await?; - } - Ok(()) -} - -#[cfg(test)] -mod test { - use super::*; - - #[test] - fn message() { - let mut message = get_default_message("foo/bar", "baz").to_string(); - - writeln!(message, "The following commits are merge commits:").unwrap(); - - for n in 1..5 { - writeln!(message, "- commit{n}").unwrap(); - } - - assert_eq!( - message, - " -There are merge commits (commits with multiple parents) in your changes. We have a [no merge policy](https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy) so these commits will need to be removed for this pull request to be merged. - -You can start a rebase with the following commands: -```shell-session -$ # rebase -$ git pull --rebase https://github.com/foo/bar.git baz -$ git push --force-with-lease -``` - -The following commits are merge commits: -- commit1 -- commit2 -- commit3 -- commit4 -" - ); - } -}