Enable triagebot no-merges check

[pitaj]

Follow-up on rust-lang/triagebot#1704

Motivation

Occasionally, a merge commit like cb5c011 makes it past manual review and gets merged into master.

At one point, we tried adding a check to CI to prevent this from happening (#105058), but that ended up problematic and was reverted. This kind of check is simply too fragile for CI, and there must be a way for a human to override the bot's decision.

The capability to detect and warn about merge commits has been present in triagebot for quite some time, but was never enabled at rust-lang/rust, possibly due to concerns about false positives on rollup and subtree sync PRs. This PR intends to alleviate those concerns.

Configuration

This configuration will exclude rollup PRs and subtree sync PRs from merge commit detection, and it will post the default warning message and add the has-merge-commits and S-waiting-on-author labels when merge commits are detected on other PRs.

The eventual vision is to have bors refuse to merge if the has-merge-commits label is present. A reviewer can still force the merge by removing that label if they so wish.

Note for contributors

The rollup tool should add that label automatically, but anyone performing subtree updates should begin including "subtree update" in the titles of those PRs, to avoid false positives.

r? infra

Open Questions

1. This configuration uses the default message that's built into triagebot:

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

Any changes to this are easy, I'll just have to add a message option. Should we mention the excluded titles in the message?

[ehuss]

but anyone performing submodule/subtree updates should begin including the sync label on those PRs, to avoid false positives.

This shouldn't affect submodule updates, should it? I don't think they have multiple parents.

Affecting subtree updates seems like a real concern. I don't know what their workflow is like, but asking people to manually set labels seems like it would be a bit too much of an ask. Have you consulted with the people doing subtree updates? Would it be possible to add a configuration to ignore certain directories?

BTW, I recommend not doing things like r? infra. That randomly assigns someone from the team, many of whom aren't doing reviews. I'm going to reassign, though Mark has an excessive load. If the subtree issue is resolved, I can approve.

r? @Mark-Simulacrum

[pitaj]

This shouldn't affect submodule updates, should it? I don't think they have multiple parents.

Yeah I think you're right.

Affecting subtree updates seems like a real concern. I don't know what their workflow is like, but asking people to manually set labels seems like it would be a bit too much of an ask. Have you consulted with the people doing subtree updates? Would it be possible to add a configuration to ignore certain directories?

It's really too bad that Github doesn't have better support for pull request templates, or I would recommend that. I haven't consulted with them, that's a good idea. Ignoring directories may or may not work - I imagine most sync PRs touch both inside and outside the subtree. I'll have to experiment and see how I can exclude subtree commits.

Edit: Ignoring directories would work, but would of course come with a small amount of false negatives. I haven't found any other way of detecting if a commit comes from a subtree update. I've started a discussion with some contributors who commonly open sync PRs.

[Mark-Simulacrum]

r? @ehuss -- I think the only concern from me is also the subtree updates. FWIW, I have wanted to move those towards automation... but that's probably a longer term thing.

[pitaj]

Alright so I've talked with the most prolific subtree updaters including:

• @calebcartwright
• @workingjubilee
• @RalfJung
• @antoyo
• @bjorn3
• @flip1995

And the consensus was that the label method is better than a triagebot heuristic. Most will simply begin adding the subtree-sync label manually, but Ralf asked me to update the link in his Miri update script. That was done at rust-lang/miri#3057

Those wanting to add the label manually can use a link like the following:

https://github.com/rust-lang/rust/compare/{github_user}:{branch}?quick_pull=1&labels=subtree-sync

Or you can use Github CLI tool:

gh pr create --label subtree-sync

If anyone else is already using scripts or would like a script for this, please let me know and I'll be happy to open a PR to add that to your repo.

[pitaj]

One final concern is backport PRs. I don't know if any of those contain merge commits, or if they are always cherry-picks.

[flip1995]

We also want to add those labels to the no-merges policy in the Clippy repo: https://github.com/rust-lang/rust-clippy/blob/8c48b936ccf5cf62647420e604bb2cae155268a4/triagebot.toml#L12-L13

[RalfJung]

Ah, we should probably do the same in Miri. And that that point the sync PRs for the rustc → Miri direction (which are auto-created via our cronjob) should then also get that label.

[RalfJung]

Hm, the label doesn't seem to actually work? In rust-lang/miri#3059 the bot complained loudly about the merge commits.

[ehuss]

There is probably a race between creating the PR and adding the labels. AFAIK, GitHub doesn't do that atomically.

[RalfJung]

Hm, but that means the planned strategy of using the labels to silence the bot isn't working.

An alternative might be to have a tag inside the PR comment, like "triagebot: subtree-sync". I assume when triagebot runs to do its initial checks, it has definite access to the PR comment text, so there's no chance of a race condition here.

[pitaj]

Yeah I might need to add a delay or something. It has worked fine in my testing but that very well could be because the rust repo has a very large triagebot config and there was enough processing before no-merges to act as suitable delay.

[pitaj]

I think things like @rustbot label +subtree-sync in the body are processed before everything else but I'll need to check.

I don't particularly love using the body but that may be the easiest way.

[RalfJung]

Is it possible to set the body in a PR URL?

[pitaj]

Yes. For reference: https://docs.github.com/en/[email protected]/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/using-query-parameters-to-create-a-pull-request

Also mentioned there is the possibility of using a template

[Mark-Simulacrum]

I think things like @rustbot label +subtree-sync in the body are processed before everything else but I'll need to check.

I don't think the label set by that is guaranteed to be visible to later triagebot commands, even if the processing happens before. In general if we need some metadata around PR creation time you either need to schedule the work to happen in the background after a delay or depend on metadata set in the PR (e.g., PR title, description).

[pitaj]

rust-lang/triagebot#1720

[pitaj]

Updated for rust-lang/triagebot#1720 which has been merged

[ehuss]

@bors r+ rollup

[bors]

📌 Commit 4baa12b has been approved by ehuss

It is now in the queue for this repository.