Merge pull request #2175 from Urgau/mentions-uncanonicalized-in-commits

Add support for only checking uncanonicalized issue links in commits

Author: Urgau

src/config.rs

@@ -521,8 +521,62 @@ pub(crate) struct RenderedLinkConfig {
 #[serde(rename_all = "kebab-case")]
 #[serde(deny_unknown_fields)]
 pub(crate) struct IssueLinksConfig {
-    #[serde(default = "default_true")]
-    pub(crate) check_commits: bool,
+    #[serde(default)]
+    pub(crate) check_commits: IssueLinksCheckCommitsConfig,
+}
+
+#[derive(PartialEq, Eq, Debug)]
+pub(crate) enum IssueLinksCheckCommitsConfig {
+    /// No checking of commits
+    Off,
+    /// Only check for uncanonicalized issue links in commits
+    Uncanonicalized,
+    /// Check for all issue links in commits
+    All,
+}
+
+impl Default for IssueLinksCheckCommitsConfig {
+    fn default() -> Self {
+        IssueLinksCheckCommitsConfig::All
+    }
+}
+
+impl<'de> serde::Deserialize<'de> for IssueLinksCheckCommitsConfig {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        struct CheckCommitsVisitor;
+
+        impl<'de> serde::de::Visitor<'de> for CheckCommitsVisitor {
+            type Value = IssueLinksCheckCommitsConfig;
+
+            fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+                formatter.write_str("a bool or the string \"uncanonicalized\"")
+            }
+
+            fn visit_bool<E>(self, v: bool) -> Result<Self::Value, E> {
+                Ok(if v {
+                    IssueLinksCheckCommitsConfig::All
+                } else {
+                    IssueLinksCheckCommitsConfig::Off
+                })
+            }
+
+            fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
+            where
+                E: serde::de::Error,
+            {
+                if v == "uncanonicalized" {
+                    Ok(IssueLinksCheckCommitsConfig::Uncanonicalized)
+                } else {
+                    Err(E::custom("expected \"uncanonicalized\""))
+                }
+            }
+        }
+
+        deserializer.deserialize_any(CheckCommitsVisitor)
+    }
 }
 
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
@@ -545,11 +599,6 @@ pub(crate) struct BehindUpstreamConfig {
     pub(crate) days_threshold: Option<usize>,
 }
 
-#[inline]
-fn default_true() -> bool {
-    true
-}
-
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
 pub(crate) struct BackportConfig {
     // Config identifier -> labels
@@ -841,7 +890,7 @@ mod tests {
                     exclude_files: vec![],
                 }),
                 issue_links: Some(IssueLinksConfig {
-                    check_commits: true,
+                    check_commits: IssueLinksCheckCommitsConfig::All,
                 }),
                 no_mentions: Some(NoMentionsConfig {
                     exclude_titles: vec!["subtree update".into()],
@@ -936,7 +985,7 @@ mod tests {
                 bot_pull_requests: None,
                 rendered_link: None,
                 issue_links: Some(IssueLinksConfig {
-                    check_commits: false,
+                    check_commits: IssueLinksCheckCommitsConfig::Off,
                 }),
                 no_mentions: None,
                 behind_upstream: Some(BehindUpstreamConfig {
@@ -976,4 +1025,19 @@ mod tests {
             })
         );
     }
+
+    #[test]
+    fn issue_links_uncanonicalized() {
+        let config = r#"
+            [issue-links]
+            check-commits = "uncanonicalized"
+        "#;
+        let config = toml::from_str::<Config>(&config).unwrap();
+        assert!(matches!(
+            config.issue_links,
+            Some(IssueLinksConfig {
+                check_commits: IssueLinksCheckCommitsConfig::Uncanonicalized
+            })
+        ));
+    }
 }

src/handlers/check_commits/issue_links.rs

@@ -2,20 +2,25 @@ use std::sync::LazyLock;
 
 use regex::Regex;
 
-use crate::{config::IssueLinksConfig, github::GithubCommit};
+use crate::{
+    config::{IssueLinksCheckCommitsConfig, IssueLinksConfig},
+    github::GithubCommit,
+};
 
 static LINKED_RE: LazyLock<Regex> =
-    LazyLock::new(|| Regex::new(r"\B([a-zA-Z-_]+/[a-zA-Z-_]+)?(#[0-9]+)\b").unwrap());
+    LazyLock::new(|| Regex::new(r"\B(?P<org>[a-zA-Z-_]+/[a-zA-Z-_]+)?(#[0-9]+)\b").unwrap());
 
 const MERGE_IGNORE_LIST: [&str; 3] = ["Rollup merge of ", "Auto merge of ", "Merge pull request "];
 
 pub(super) fn issue_links_in_commits(
     conf: &IssueLinksConfig,
     commits: &[GithubCommit],
 ) -> Option<String> {
-    if !conf.check_commits {
-        return None;
-    }
+    let does_match = match conf.check_commits {
+        IssueLinksCheckCommitsConfig::Off => return None,
+        IssueLinksCheckCommitsConfig::All => has_issue_link,
+        IssueLinksCheckCommitsConfig::Uncanonicalized => has_uncanonicalized_issue_link,
+    };
 
     let issue_links_commits = commits
         .into_iter()
@@ -24,12 +29,21 @@ pub(super) fn issue_links_in_commits(
                 .iter()
                 .any(|i| c.commit.message.starts_with(i))
         })
-        .filter(|c| LINKED_RE.is_match(&c.commit.message))
+        .filter(|c| does_match(&c.commit.message))
         .map(|c| format!("- {}\n", c.sha))
         .collect::<String>();
 
     if issue_links_commits.is_empty() {
         None
+    } else if matches!(
+        conf.check_commits,
+        IssueLinksCheckCommitsConfig::Uncanonicalized
+    ) {
+        Some(format!(
+            r"There are uncanonicalized issue links (such as `#123`) in the commit messages of the following commits.
+*Please add the organization and repository before the issue number (like so `rust-lang/rust#123`) to avoid issues with subtree.*
+{issue_links_commits}",
+        ))
     } else {
         Some(format!(
             r"There are issue links (such as `#123`) in the commit messages of the following commits.
@@ -39,12 +53,23 @@ pub(super) fn issue_links_in_commits(
     }
 }
 
+fn has_issue_link(text: &str) -> bool {
+    LINKED_RE.is_match(text)
+}
+
+fn has_uncanonicalized_issue_link(text: &str) -> bool {
+    let Some(caps) = LINKED_RE.captures(text) else {
+        return false;
+    };
+    caps.name("org").is_none()
+}
+
 #[test]
 fn test_mentions_in_commits() {
     use super::dummy_commit_from_body;
 
     let config = IssueLinksConfig {
-        check_commits: true,
+        check_commits: IssueLinksCheckCommitsConfig::All,
     };
 
     let mut commits = vec![dummy_commit_from_body(
@@ -87,7 +112,7 @@ fn test_mentions_in_commits() {
     assert_eq!(
         issue_links_in_commits(
             &IssueLinksConfig {
-                check_commits: false,
+                check_commits: IssueLinksCheckCommitsConfig::Off,
             },
             &commits
         ),
@@ -110,3 +135,41 @@ fn test_mentions_in_commits() {
         )
     );
 }
+
+#[test]
+fn uncanonicalized() {
+    use super::dummy_commit_from_body;
+
+    let config = IssueLinksConfig {
+        check_commits: IssueLinksCheckCommitsConfig::Uncanonicalized,
+    };
+
+    let mut commits = vec![dummy_commit_from_body(
+        "d1992a392617dfb10518c3e56446b6c9efae38b0",
+        "This is simple without issue links!",
+    )];
+
+    assert_eq!(issue_links_in_commits(&config, &commits), None);
+
+    commits.push(dummy_commit_from_body(
+        "86176475acda9c775f844f5ad2470f05aebd4249",
+        "Test for canonicalized rust-lang/rust#123",
+    ));
+
+    assert_eq!(issue_links_in_commits(&config, &commits), None);
+
+    commits.push(dummy_commit_from_body(
+        "fererfe5acda9c775f844f5ad2470f05aebd4249",
+        "Test for uncanonicalized #123",
+    ));
+
+    assert_eq!(
+        issue_links_in_commits(&config, &commits),
+        Some(
+            r"There are uncanonicalized issue links (such as `#123`) in the commit messages of the following commits.
+*Please add the organization and repository before the issue number (like so `rust-lang/rust#123`) to avoid issues with subtree.*
+- fererfe5acda9c775f844f5ad2470f05aebd4249
+".to_string()
+        )
+    );
+}