diff --git a/src/github.rs b/src/github.rs index 8c5438d7f..48e4e235a 100644 --- a/src/github.rs +++ b/src/github.rs @@ -320,6 +320,18 @@ pub struct FileDiff { pub diff: String, } +/// The return from GitHub compare API +#[derive(Debug, serde::Deserialize)] +pub struct GithubCompare { + /// The base commit of the PR + pub base_commit: GithubCommit, + /// The merge base commit + /// + /// See for more details + pub merge_base_commit: GithubCommit, + // FIXME: Also retrieve and use the files list (see our diff function) +} + impl PullRequestDetails { pub fn new() -> PullRequestDetails { PullRequestDetails { @@ -994,6 +1006,23 @@ impl Issue { Ok(Some(diff)) } + /// Returns the comparison of this event. + /// + /// Returns `None` if the issue is not a PR. + pub async fn compare(&self, client: &GithubClient) -> anyhow::Result> { + let (before, after) = if let (Some(base), Some(head)) = (&self.base, &self.head) { + (&base.sha, &head.sha) + } else { + return Ok(None); + }; + + let req = client.get(&format!( + "{}/compare/{before}...{after}", + self.repository().url(client) + )); + Ok(Some(client.json(req).await?)) + } + /// Returns the commits from this pull request (no commits are returned if this `Issue` is not /// a pull request). pub async fn commits(&self, client: &GithubClient) -> anyhow::Result> { diff --git a/src/handlers/check_commits.rs b/src/handlers/check_commits.rs index c5703e9e2..526cbcf0e 100644 --- a/src/handlers/check_commits.rs +++ b/src/handlers/check_commits.rs @@ -58,6 +58,12 @@ pub(super) async fn handle(ctx: &Context, event: &Event, config: &Config) -> any event.issue.number ) }; + let Some(compare) = event.issue.compare(&ctx.github).await? else { + bail!( + "expected issue {} to be a PR, but the compare could not be determined", + event.issue.number + ) + }; let commits = event.issue.commits(&ctx.github).await?; let mut warnings = Vec::new(); @@ -101,7 +107,7 @@ pub(super) async fn handle(ctx: &Context, event: &Event, config: &Config) -> any .unwrap_or(behind_upstream::DEFAULT_DAYS_THRESHOLD); if let Some(warning) = - behind_upstream::behind_upstream(age_threshold, event, &ctx.github, &commits).await + behind_upstream::behind_upstream(age_threshold, event, &compare).await { warnings.push(warning); } diff --git a/src/handlers/check_commits/behind_upstream.rs b/src/handlers/check_commits/behind_upstream.rs index 34dc28dc2..b9dec019d 100644 --- a/src/handlers/check_commits/behind_upstream.rs +++ b/src/handlers/check_commits/behind_upstream.rs @@ -1,4 +1,4 @@ -use crate::github::{GithubClient, GithubCommit, IssuesEvent, Repository}; +use crate::github::{GithubCompare, IssuesEvent}; use tracing as log; /// Default threshold for parent commit age in days to trigger a warning @@ -8,95 +8,32 @@ pub(super) const DEFAULT_DAYS_THRESHOLD: usize = 7; pub(super) async fn behind_upstream( age_threshold: usize, event: &IssuesEvent, - client: &GithubClient, - commits: &Vec, + compare: &GithubCompare, ) -> Option { log::debug!("Checking if PR #{} is behind upstream", event.issue.number); - let Some(head_commit) = commits.first() else { - return None; - }; + // Compute the number of days old the merge base commit is + let commit_date = compare.merge_base_commit.commit.author.date; + let now = chrono::Utc::now().with_timezone(&commit_date.timezone()); + let days_old = (now - commit_date).num_days() as usize; // 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(super) 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)) + if days_old > age_threshold { + log::info!( + "PR #{} has a parent commit that is {} days old", + event.issue.number, + days_old + ); + + 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 + )) } else { - Ok(None) + // Parent commit is not too old, log and do nothing + log::debug!("PR #{} parent commit is not too old", event.issue.number); + None } } - -/// Returns the number of days old the commit is -pub(super) 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) -}