Refactor no_merges handler into the check_commits super-handler

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,

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<String>,
     /// ID of the most recent warning comment.
     last_warned_comment: Option<String>,
+    /// List of the last labels added.
+    last_labels: Vec<String>,
 }
 
 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<String>,
+    labels: Vec<String>,
 ) -> 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<String>,
+    current: &Vec<String>,
+) -> (Vec<String>, Vec<String>) {
+    let previous_set: HashSet<String> = previous.into_iter().cloned().collect();
+    let current_set: HashSet<String> = current.into_iter().cloned().collect();
+
+    let removals = previous_set.difference(&current_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};

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<GithubCommit>,
+) -> Option<(String, Vec<String>)> {
+    // 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<Item = &'a str>,
+) -> 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
+        );
+    }
+}