diff --git a/src/config.rs b/src/config.rs index 99a7d1187..523092a58 100644 --- a/src/config.rs +++ b/src/config.rs @@ -49,6 +49,7 @@ pub(crate) struct Config { #[serde(alias = "canonicalize-issue-links")] pub(crate) issue_links: Option, pub(crate) no_mentions: Option, + pub(crate) behind_upstream: Option, } #[derive(PartialEq, Eq, Debug, serde::Deserialize)] @@ -427,6 +428,16 @@ pub(crate) struct IssueLinksConfig {} #[serde(deny_unknown_fields)] pub(crate) struct NoMentionsConfig {} +/// Configuration for PR behind commits checks +#[derive(PartialEq, Eq, Debug, serde::Deserialize)] +#[serde(rename_all = "kebab-case")] +#[serde(deny_unknown_fields)] +pub(crate) struct BehindUpstreamConfig { + /// The threshold of days for parent commit age to trigger a warning. + /// Default is 7 days if not specified. + pub(crate) days_threshold: Option, +} + fn get_cached_config(repo: &str) -> Option, ConfigurationError>> { let cache = CONFIG_CACHE.read().unwrap(); cache.get(repo).and_then(|(config, fetch_time)| { @@ -554,6 +565,9 @@ mod tests { trigger-files = ["posts/"] [no-mentions] + + [behind-upstream] + days-threshold = 14 "#; let config = toml::from_str::(&config).unwrap(); let mut ping_teams = HashMap::new(); @@ -618,6 +632,9 @@ mod tests { }), issue_links: Some(IssueLinksConfig {}), no_mentions: Some(NoMentionsConfig {}), + behind_upstream: Some(BehindUpstreamConfig { + days_threshold: Some(14), + }), } ); } @@ -637,6 +654,9 @@ mod tests { branch = "stable" [canonicalize-issue-links] + + [behind-upstream] + days-threshold = 7 "#; let config = toml::from_str::(&config).unwrap(); assert_eq!( @@ -685,6 +705,9 @@ mod tests { rendered_link: None, issue_links: Some(IssueLinksConfig {}), no_mentions: None, + behind_upstream: Some(BehindUpstreamConfig { + days_threshold: Some(7), + }), } ); } diff --git a/src/github.rs b/src/github.rs index 29c1ea7e1..c6c8c0e6d 100644 --- a/src/github.rs +++ b/src/github.rs @@ -1458,7 +1458,7 @@ impl Repository { /// Returns a list of commits between the SHA ranges of start (exclusive) /// and end (inclusive). - pub async fn commits_in_range( + pub async fn github_commits_in_range( &self, client: &GithubClient, start: &str, @@ -1499,6 +1499,18 @@ impl Repository { } } + pub async fn github_commit( + &self, + client: &GithubClient, + sha: &str, + ) -> anyhow::Result { + let url = format!("{}/commits/{}", self.url(client), sha); + client + .json(client.get(&url)) + .await + .with_context(|| format!("{} failed to get github commit {sha}", self.full_name)) + } + /// Retrieves a git commit for the given SHA. pub async fn git_commit(&self, client: &GithubClient, sha: &str) -> anyhow::Result { let url = format!("{}/git/commits/{sha}", self.url(client)); diff --git a/src/handlers/check_commits.rs b/src/handlers/check_commits.rs index 128b2ce2e..281378c21 100644 --- a/src/handlers/check_commits.rs +++ b/src/handlers/check_commits.rs @@ -13,6 +13,7 @@ use crate::{ #[cfg(test)] use crate::github::GithubCommit; +mod behind_upstream; mod issue_links; mod modified_submodule; mod no_mentions; @@ -93,6 +94,19 @@ pub(super) async fn handle(ctx: &Context, event: &Event, config: &Config) -> any } } + // Check if PR is behind upstream branch by a significant number of days + if let Some(behind_upstream) = &config.behind_upstream { + let age_threshold = behind_upstream + .days_threshold + .unwrap_or(behind_upstream::DEFAULT_DAYS_THRESHOLD); + + if let Some(warning) = + behind_upstream::behind_upstream(age_threshold, event, &ctx.github, &commits).await + { + warnings.push(warning); + } + } + handle_warnings_and_labels(ctx, event, warnings, labels).await } diff --git a/src/handlers/check_commits/behind_upstream.rs b/src/handlers/check_commits/behind_upstream.rs new file mode 100644 index 000000000..4fcd1edca --- /dev/null +++ b/src/handlers/check_commits/behind_upstream.rs @@ -0,0 +1,102 @@ +use crate::github::{GithubClient, GithubCommit, IssuesEvent, Repository}; +use tracing as log; + +/// Default threshold for parent commit age in days to trigger a warning +pub const DEFAULT_DAYS_THRESHOLD: usize = 7; + +/// Check if the PR is based on an old parent commit +pub async fn behind_upstream( + age_threshold: usize, + event: &IssuesEvent, + client: &GithubClient, + commits: &Vec, +) -> Option { + log::debug!("Checking if PR #{} is behind upstream", event.issue.number); + + let Some(head_commit) = commits.first() else { + return None; + }; + + // First try the parent commit age check as it's more accurate + match is_parent_commit_too_old(head_commit, &event.repository, client, age_threshold).await { + Ok(Some(days_old)) => { + log::info!( + "PR #{} has a parent commit that is {} days old", + event.issue.number, + days_old + ); + + return Some(format!( + r"This PR is based on an upstream commit that is {} days old. + + *It's recommended to update your branch according to the [rustc_dev_guide](https://rustc-dev-guide.rust-lang.org/contributing.html#keeping-your-branch-up-to-date).*", + days_old + )); + } + Ok(None) => { + // Parent commit is not too old, log and do nothing + log::debug!("PR #{} parent commit is not too old", event.issue.number); + } + Err(e) => { + // Error checking parent commit age, log and do nothing + log::error!( + "Error checking parent commit age for PR #{}: {}", + event.issue.number, + e + ); + } + } + + None +} + +/// Checks if the PR's parent commit is too old. +/// +/// This determines if a PR needs updating by examining the first parent of the PR's head commit, +/// which typically represents the base branch commit that the PR is based on. +/// +/// If this parent commit is older than the specified threshold, it suggests the PR +/// should be updated/rebased to a more recent version of the base branch. +/// +/// Returns: +/// - Ok(Some(days_old)) - If parent commit is older than the threshold +/// - Ok(None) +/// - If there is no parent commit +/// - If parent is within threshold +/// - Err(...) - If an error occurred during processing +pub async fn is_parent_commit_too_old( + commit: &GithubCommit, + repo: &Repository, + client: &GithubClient, + max_days_old: usize, +) -> anyhow::Result> { + // Get the first parent (it should be from the base branch) + let Some(parent_sha) = commit.parents.get(0).map(|c| c.sha.clone()) else { + return Ok(None); + }; + + let days_old = commit_days_old(&parent_sha, repo, client).await?; + + if days_old > max_days_old { + Ok(Some(days_old)) + } else { + Ok(None) + } +} + +/// Returns the number of days old the commit is +pub async fn commit_days_old( + sha: &str, + repo: &Repository, + client: &GithubClient, +) -> anyhow::Result { + // Get the commit details to check its date + let commit: GithubCommit = repo.github_commit(client, &sha).await?; + + // compute the number of days old the commit is + let commit_date = commit.commit.author.date; + let now = chrono::Utc::now().with_timezone(&commit_date.timezone()); + let days_old = (now - commit_date).num_days() as usize; + + Ok(days_old) +} diff --git a/src/handlers/milestone_prs.rs b/src/handlers/milestone_prs.rs index c2147633f..c5f90a111 100644 --- a/src/handlers/milestone_prs.rs +++ b/src/handlers/milestone_prs.rs @@ -134,7 +134,7 @@ async fn milestone_cargo( let cargo_repo = gh.repository("rust-lang/cargo").await?; log::info!("loading cargo changes {cargo_start_hash}...{cargo_end_hash}"); let commits = cargo_repo - .commits_in_range(gh, cargo_start_hash, cargo_end_hash) + .github_commits_in_range(gh, cargo_start_hash, cargo_end_hash) .await?; // For each commit, look for a message from bors that indicates which