Suggest update the branch when behind too many commits

[xizheyin]

As discussion in #t-compiler/rustc-dev-guide > A reminder in the doc to try to work with `rust-lang/rust`, I add this feature. But I haven't test it (That may takes some time). Do I need to test it locally?

cc @Kobzol

[rustbot]

Error: Invalid triagebot.toml at position 14:2:

TOML parse error at line 14, column 2
   |
14 | [pr-behind-commits]
   |  ^^^^^^^^^^^^^^^^^
unknown field `pr-behind-commits`, expected one of `relabel`, `assign`, `ping`, `nominate`, `prioritize`, `major-change`, `glacier`, `close`, `autolabel`, `notify-zulip`, `github-releases`, `review-submitted`, `review-requested`, `shortcut`, `note`, `mentions`, `no-merges`, `validate-config`, `pr-tracking`, `transfer`, `merge-conflicts`, `bot-pull-requests`, `rendered-link`, `canonicalize-issue-links`, `issue-links`, `no-mentions`

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

[rustbot]

Error: Invalid triagebot.toml at position 14:2:

TOML parse error at line 14, column 2
   |
14 | [pr-behind-commits]
   |  ^^^^^^^^^^^^^^^^^
unknown field `pr-behind-commits`, expected one of `relabel`, `assign`, `ping`, `nominate`, `prioritize`, `major-change`, `glacier`, `close`, `autolabel`, `notify-zulip`, `github-releases`, `review-submitted`, `review-requested`, `shortcut`, `note`, `mentions`, `no-merges`, `validate-config`, `pr-tracking`, `transfer`, `merge-conflicts`, `bot-pull-requests`, `rendered-link`, `canonicalize-issue-links`, `issue-links`, `no-mentions`

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

[Kobzol]

Hi, thanks for the PR! We should definitely test this, but I can do it on my test repo.

Since this check essentially checks commits, and should be ideally done after each push (unless we find the API call to be too expensive), could you please move it to the check_commits handler?

It would be nice to introduce some [check_commits] config option that would contain the individual commit checks as attributes, and migrate the existing commit checks to it (CC @Urgau, WDYT?), but that doesn't need to happen in this PR.

I wonder how this interacts with merge commits, as found e.g. in rust-lang/rust. We should check if behind_by only includes merge commits or all commits.

[xizheyin]

Yeah, thanks for guide, I'll move it to check_commits.

I'm calling github's compare api here, which should take into account all of the commits, is there anything special about merge commits? Does it need to be considered separately?

[Kobzol]

Well, the history isn't linear, so "number of commits between A and B" is not something that can be answered unambigously.

[xizheyin]

Oh, you mean for example Auto merge commit will contain multiple PR commits? Considering only github diffs might be a tradeoff. We can control the approximate range by setting the threshold. I'm concerned that a more granular analysis might take up too much compute resources.

[xizheyin]

In my latest change, I eliminated Auto-merge vs Rollup-merge commits, which should be more fair.

For the part of getting the comparing commits, I didn't take paging because I think a page can query up to 250 leading commits, which should be much higher than our threshold, and to save on queries, I only request it once.
@Kobzol

[Urgau]

It would be nice to introduce some [check_commits] config option that would contain the individual commit checks as attributes, and migrate the existing commit checks to it

Some of them have configs for them-selves, like [no_merges], won't it be a bit awkward to configure them if they were also under a single [check_commits] config? We could prefix the individual config (like no_merges_labels and so on), but that since weird and unnecessary.

[Kobzol]

Ideally I would like to merge all of these checks under a single config option, to have it be more consistent, but it may not be worth the churn. We could also support both configs and incrementally migrate the repos to the new form, but again, it's probably not worth it.

[xizheyin]

I tried it yesterday and it seems like there was a slight problem with the macro expansion, I think this change may require a separate PR. I would like to do it if possible.

[Kobzol]

Just to clarify on my earlier comments, we should only take "root" merge commits into account, so essentially merged PRs. A single PR can contain hundreds of commits (for example sync PRs from subtrees), so if we had a limit of 100 commits, it could trigger after a single PR, which is no good.

So really we want to have a notion of "you are N PRs behind". That being said, each merged PR is a master commit with two parents, the first one is the previous merge and the right one is the tip of the merged PR. I would kind of expect that maybe behind_by only "traverses" the first parents? That's what I wanted to check.

[xizheyin]

Thank you, I got it. We need to count merge PRs and not just commits, I'll revise it.

[Kobzol]

Brainstorming:

I did some experiments and it seems that the behind_by attribute is mostly useless to us, as it seems to count all commits. What we could do instead is to run gh api "repos/rust-lang/rust/[email protected]" to go through all recent merge commits, and try to see if the PR's base SHA is in them and on which position. But the base SHA is actually not the right thing that we need (see below).

I checked if we can get something from the PR details (which we can either fetch, but they should be actually sent proactively in the PR status change webhook). It contains an attribute base.sha, which tells us the SHA of the master commit on which the PR is based. But this SHA is actually reset (to latest master) on each push to the PR, so that doesn't really help us.

So, another idea: we can take the SHA of the PR, and ask for the details of that commit (gh api repos/kobzol/rust/commits/340272e54ed731ed827f991d75afc9db8aa21c50), which has a parents field. So we could look up the first parent of the PR's SHA, and ask GH again, and see what is the date of that parent commit, and if it's too old, then we can warn.

But when I thought of this solution, I realized that we might not even need the parent commit at all, if we only take a look at the date? When PR HEAD commit date is older than N days, we know that it hasn't been rebased onto the latest master at the user's PC. If they do rebase, the author date will remain the same, but the commit date will be updated. Hmm, no, that wouldn't necessarily work, because even if you rebase, you might still rebase onto an old local master, so even though the commit date will be new, it doesn't mean that you have rebased onto a fresh master commit.

So perhaps the approach with checking the HEAD commit and its parent, and then warn if the parent is older than N days might be the way to go.

[Urgau]

So perhaps the approach with checking the HEAD commit and its parent, and then warn if the parent is older than N days might be the way to go.

Seems reasonable to me.

[xizheyin]

I updated the code. The current implementation uses a two-step approach to determine if a PR needs updating:

1. Time-based Check (Preferred Method): Calls is_parent_commit_too_old to check if the PR's parent commit is too old (default: older than 14 days). If the parent commit is too old, immediately generates a warning message suggesting to update the branch. This is considered a more accurate method and is used first

2. Important Commits Check (Fallback Method): If the time check passes or errors, proceeds with the commit-based check. It gets the difference information between the PR and target branch(by github Compare API). We only count Auto-merge and Rollup-merge commits (important integration commits). If the number of missing important commits exceeds the threshold (default 100), generates a warning message. This method focuses on whether the PR is missing important integration changes, ignoring regular commits.

Since there may be a collection of PRs in the rollup merge, the number we count will only be a little less than the actual PRs, so as to avoid disturbing the users. This is not a bad alternative.

This approach considers both the timeliness of the PR (time perspective) and whether the PR includes the latest important merge commits (functional perspective), providing a comprehensive assessment of whether the PR needs updating.

This is the implementation as I understand it, I don't know if I'm wrong.

[Kobzol]

Checking of the commit date looks good. I would just use that, the fallback would be problematic, because some subtree synces can contain fake/empty bors merge commits, and a single PR can contain tens of these. Detecting this based on number of commits simply won't work well in rust-lang/rust, IMO.

[xizheyin]

Ok, I'll remove the logic related to checking for commits.

[Kobzol]

Thank you! I think that the implementation looks good now, modulo a few refactorings that I proposed in comments.

[xizheyin]

I did some refactor, mainly as followings:

• rename behind_master to behind_upstream
• move aging checker to behind_upstream.rs
• add github_commit in Repository
• rename commits_in_range to github_commits_in_range(Because it return a GithubCommit instead of GitCommit)
• some other nits

[Kobzol]

Great! Please remove the stray backtick and squash the commits, and we can merge it :)

[xizheyin]

Ok, I finished it. But you may need to test it, I don't have a test environment. :)

[Urgau]

Please wait before merging, I like to take a look. Will look at it tonight.

[xizheyin]

Thanks both of you!

I realized that the previous code was wrong.

We shouldn't be checking the parent of issue.head, because the head seems to be the latest commit, and its parent is probably the last commit we submitted (if any) instead of the base commit.

The array returned by pulls/xxx/commits is accessed in order from oldest to newest, so commits.first() gets the earliest commit in the current PR, and its parent is what we need.

I've changed it now though. By the way, I think it's possible to do comments on some structures, e.g. some repository field can be annotated whether it's an upstream repository or a forked repository, since the url returned by the github api is quite diverse.

[Kobzol]

Good point, I kind of forgot that PRs can have more than one commit, lol. Thank you!

[xizheyin]

This functionility seems to work well. 😁Do we need to extend it to other code repositories?
rust-lang/rustc-dev-guide#2384 (comment)

[Urgau]

The original motivation was to do it for rust-lang/rust, so I would fine if we enable it there.

Feel free to open a PR and assign it to me.