Skip to content

Suggest update the branch when behind too many commits #1942

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 24, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ pub(crate) struct Config {
#[serde(alias = "canonicalize-issue-links")]
pub(crate) issue_links: Option<IssueLinksConfig>,
pub(crate) no_mentions: Option<NoMentionsConfig>,
pub(crate) behind_upstream: Option<BehindUpstreamConfig>,
}

#[derive(PartialEq, Eq, Debug, serde::Deserialize)]
Expand Down Expand Up @@ -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<usize>,
}

fn get_cached_config(repo: &str) -> Option<Result<Arc<Config>, ConfigurationError>> {
let cache = CONFIG_CACHE.read().unwrap();
cache.get(repo).and_then(|(config, fetch_time)| {
Expand Down Expand Up @@ -554,6 +565,9 @@ mod tests {
trigger-files = ["posts/"]

[no-mentions]

[behind-upstream]
days-threshold = 14
"#;
let config = toml::from_str::<Config>(&config).unwrap();
let mut ping_teams = HashMap::new();
Expand Down Expand Up @@ -618,6 +632,9 @@ mod tests {
}),
issue_links: Some(IssueLinksConfig {}),
no_mentions: Some(NoMentionsConfig {}),
behind_upstream: Some(BehindUpstreamConfig {
days_threshold: Some(14),
}),
}
);
}
Expand All @@ -637,6 +654,9 @@ mod tests {
branch = "stable"

[canonicalize-issue-links]

[behind-upstream]
days-threshold = 7
"#;
let config = toml::from_str::<Config>(&config).unwrap();
assert_eq!(
Expand Down Expand Up @@ -685,6 +705,9 @@ mod tests {
rendered_link: None,
issue_links: Some(IssueLinksConfig {}),
no_mentions: None,
behind_upstream: Some(BehindUpstreamConfig {
days_threshold: Some(7),
}),
}
);
}
Expand Down
14 changes: 13 additions & 1 deletion src/github.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -1499,6 +1499,18 @@ impl Repository {
}
}

pub async fn github_commit(
&self,
client: &GithubClient,
sha: &str,
) -> anyhow::Result<GithubCommit> {
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<GitCommit> {
let url = format!("{}/git/commits/{sha}", self.url(client));
Expand Down
14 changes: 14 additions & 0 deletions src/handlers/check_commits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use crate::{
#[cfg(test)]
use crate::github::GithubCommit;

mod behind_upstream;
mod issue_links;
mod modified_submodule;
mod no_mentions;
Expand Down Expand Up @@ -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
}

Expand Down
102 changes: 102 additions & 0 deletions src/handlers/check_commits/behind_upstream.rs
Original file line number Diff line number Diff line change
@@ -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<GithubCommit>,
) -> Option<String> {
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<Option<usize>> {
// 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<usize> {
// 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)
}
2 changes: 1 addition & 1 deletion src/handlers/milestone_prs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down