diff --git a/src/github/repos.rs b/src/github/repos.rs index 120586191..01892544a 100644 --- a/src/github/repos.rs +++ b/src/github/repos.rs @@ -930,6 +930,21 @@ pub struct GitObject { pub url: String, } +impl GithubClient { + /// Retrieves a git reference for the given refname. + pub async fn get_reference( + &self, + org: &str, + repo: &str, + refname: &str, + ) -> anyhow::Result { + let url = format!("{}/repos/{org}/{repo}/git/ref/{refname}", self.api_url); + self.json(self.get(&url)) + .await + .with_context(|| format!("{org}/{repo} failed to get git reference {refname}")) + } +} + impl Repository { /// Retrieves a git reference for the given refname. pub async fn get_reference( @@ -937,11 +952,9 @@ impl Repository { client: &GithubClient, refname: &str, ) -> anyhow::Result { - let url = format!("{}/git/ref/{refname}", self.url(client)); client - .json(client.get(&url)) + .get_reference(self.owner(), self.name(), refname) .await - .with_context(|| format!("{} failed to get git reference {refname}", self.full_name)) } /// Updates an existing git reference to a new SHA. diff --git a/src/handlers/check_commits.rs b/src/handlers/check_commits.rs index 88f041494..e930c662f 100644 --- a/src/handlers/check_commits.rs +++ b/src/handlers/check_commits.rs @@ -16,6 +16,7 @@ use crate::{ use crate::github::GithubCommit; mod behind_upstream; +mod branch_links; mod force_push_range_diff; mod issue_links; mod modified_submodule; @@ -23,6 +24,9 @@ mod no_merges; mod non_default_branch; mod validate_config; +/// Starting message for merge commits +const MERGE_IGNORE_LIST: [&str; 3] = ["Rollup merge of ", "Auto merge of ", "Merge pull request "]; + /// Key for the state in the database const CHECK_COMMITS_KEY: &str = "check-commits-warnings"; @@ -113,6 +117,7 @@ pub(super) async fn handle( if let Some(issue_links) = &config.issue_links { warnings.extend(issue_links::issue_links_in_commits(issue_links, &commits)); + warnings.extend(branch_links::branch_links_in_commits(ctx, issue_links, &commits).await); } #[expect( diff --git a/src/handlers/check_commits/branch_links.rs b/src/handlers/check_commits/branch_links.rs new file mode 100644 index 000000000..5e142fe83 --- /dev/null +++ b/src/handlers/check_commits/branch_links.rs @@ -0,0 +1,51 @@ +use crate::{ + config::{IssueLinksCheckCommitsConfig, IssueLinksConfig}, + github::GithubCommit, + handlers::{Context, check_commits::MERGE_IGNORE_LIST}, +}; + +pub(super) async fn branch_links_in_commits( + ctx: &Context, + conf: &IssueLinksConfig, + commits: &[GithubCommit], +) -> Option { + match conf.check_commits { + IssueLinksCheckCommitsConfig::Off => return None, + IssueLinksCheckCommitsConfig::All | IssueLinksCheckCommitsConfig::Uncanonicalized => {} + } + + let branch_links_commits = futures::future::join_all( + commits + .iter() + .filter(|c| { + !MERGE_IGNORE_LIST + .iter() + .any(|i| c.commit.message.starts_with(i)) + }) + .map(|c| async { + let mapping = super::super::issue_links::collect_branch_sha_links_mapping( + &ctx, + &c.commit.message, + ) + .await; + if mapping.is_empty() { + None + } else { + Some(format!("- {}\n", c.sha)) + } + }), + ) + .await + .into_iter() + .flatten() + .collect::(); + + if branch_links_commits.is_empty() { + None + } else { + Some(format!( + r"There are links that are not permanent in the commit message of the following commits. Please use a permalink instead. +{branch_links_commits}", + )) + } +} diff --git a/src/handlers/check_commits/issue_links.rs b/src/handlers/check_commits/issue_links.rs index e0c9e85a4..236795234 100644 --- a/src/handlers/check_commits/issue_links.rs +++ b/src/handlers/check_commits/issue_links.rs @@ -5,13 +5,12 @@ use regex::Regex; use crate::{ config::{IssueLinksCheckCommitsConfig, IssueLinksConfig}, github::GithubCommit, + handlers::check_commits::MERGE_IGNORE_LIST, }; static LINKED_RE: LazyLock = LazyLock::new(|| Regex::new(r"\B(?P[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], diff --git a/src/handlers/issue_links.rs b/src/handlers/issue_links.rs index a705fedb0..4c4cc9f73 100644 --- a/src/handlers/issue_links.rs +++ b/src/handlers/issue_links.rs @@ -4,10 +4,11 @@ //! //! Example: `Fixes #123` (in rust-lang/clippy) would now become `Fixes rust-lang/clippy#123` -use std::borrow::Cow; +use std::collections::HashSet; use std::sync::LazyLock; +use std::{borrow::Cow, collections::HashMap}; -use regex::Regex; +use regex::{Captures, Regex}; use crate::{ config::IssueLinksConfig, @@ -18,6 +19,22 @@ use crate::{ static LINKED_RE: LazyLock = LazyLock::new(|| Regex::new(r"\B(?P#[0-9]+)\b").unwrap()); +static LINKED_FILE_RE: LazyLock = LazyLock::new(|| { + Regex::new( + r"https://github\.com/(?P[^/\s]+)/(?P[^/\s]+)/blob/(?P[^/\s]+)/(?P[^\s]+)\b", + ) + .unwrap() +}); + +type BranchShaMappings<'a> = HashMap< + ( + /* org */ &'a str, + /* repo */ &'a str, + /* branch name */ &'a str, + ), + /* sha */ String, +>; + pub(super) struct IssueLinksInput {} pub(super) async fn parse_input( @@ -53,7 +70,10 @@ pub(super) async fn handle_input( ) -> anyhow::Result<()> { let full_repo_name = e.issue.repository().full_repo_name(); + let shas = collect_branch_sha_links_mapping(ctx, &e.issue.body).await; + let new_body = fix_linked_issues(&e.issue.body, full_repo_name.as_str()); + let new_body = fix_linked_files(&new_body, shas); if e.issue.body != new_body { e.issue.edit_body(&ctx.github, &new_body).await?; @@ -67,8 +87,71 @@ fn fix_linked_issues<'a>(body: &'a str, full_repo_name: &str) -> Cow<'a, str> { parser::replace_all_outside_ignore_blocks(&LINKED_RE, body, replace_by) } +fn fix_linked_files<'a>(body: &'a str, shas: BranchShaMappings<'a>) -> Cow<'a, str> { + parser::replace_all_outside_ignore_blocks(&LINKED_FILE_RE, body, |caps: &Captures| { + let org = &caps["org"]; + let repo = &caps["repo"]; + let ref_ = &caps["ref"]; + let filepath = &caps["filepath"]; + + if let Some(sha) = shas.get(&(org, repo, ref_)) { + format!("https://github.com/{org}/{repo}/blob/{sha}/{filepath}") + } else { + caps.get(0).unwrap().as_str().to_string() + } + }) +} + +pub(crate) async fn collect_branch_sha_links_mapping<'a>( + ctx: &Context, + body: &'a str, +) -> BranchShaMappings<'a> { + let mut seen = HashSet::new(); + + let shas: Vec<_> = LINKED_FILE_RE + .captures_iter(body) + .filter_map(|caps: Captures| { + let org = caps.name("org").unwrap().as_str(); + let repo = caps.name("repo").unwrap().as_str(); + let ref_ = caps.name("ref").unwrap().as_str(); + + // Let's assume that an alpha-numeric string of 40 and 64 characters + // is most probably a SHA1 or SHA256 hash. + let looks_like_sha = (ref_.len() == 40 || ref_.len() == 64) + && ref_.chars().all(|c| c.is_ascii_alphanumeric()); + + if looks_like_sha || !seen.insert((org, repo, ref_)) { + return None; + } + + Some(async move { + match ctx + .github + .get_reference(org, repo, &format!("heads/{ref_}")) + .await + { + Ok(git_ref) => Some(((org, repo, ref_), git_ref.object.sha)), + Err(err) => { + // maybe network error, or simply user error, either way don't fail + tracing::warn!("{err}"); + None + } + } + }) + }) + // limit to maximum 10 different branches + .take(10) + .collect(); + + futures::future::join_all(shas) + .await + .into_iter() + .flatten() + .collect() +} + #[test] -fn fixed_body() { +fn fixed_body_issues() { let full_repo_name = "rust-lang/rust"; let body = r#" @@ -99,6 +182,23 @@ Resolves rust-lang/rust#00000 Closes rust-lang/rust#888 assert_eq!(new_body, fixed_body); } +#[test] +fn fixed_body_files() { + let mut shas = HashMap::new(); + shas.insert(("torvalds", "linux", "master"), "123456789".to_string()); + + let body = r#" +This is a PR, which links to https://github.com/torvalds/linux/blob/master/include/uapi/linux/audit.h#L389-L451. + "#; + + let fixed_body = r#" +This is a PR, which links to https://github.com/torvalds/linux/blob/123456789/include/uapi/linux/audit.h#L389-L451. + "#; + + let new_body = fix_linked_files(body, shas); + assert_eq!(new_body, fixed_body); +} + #[test] fn edge_case_body() { let full_repo_name = "rust-lang/rust"; @@ -124,6 +224,8 @@ fn edge_case_body() { #[test] fn untouched_body() { let full_repo_name = "rust-lang/rust"; + let mut shas = HashMap::new(); + shas.insert(("torvalds", "linux", "master"), "123456789".to_string()); let body = r#" This is a PR. @@ -137,11 +239,14 @@ Fixes#123 ``` Example: Fixes #123 +Example2: https://github.com/torvalds/linux/blob/master/include/uapi/linux/audit.h#L389-L451 ``` + "#; let new_body = fix_linked_issues(body, full_repo_name); + let new_body = fix_linked_files(&new_body, shas); assert_eq!(new_body, body); }