Add no GitHub mentions in commits handler

[Urgau]

This PR implements a no-mentions in commits handler. This is so that people are not spammed by GitHub when someone pushes a commit with their GitHub handle in it.

This is coming from rust-lang/rust#137990. I should note that I also have been annoyed by that in the past.

The implementation is heavily inspired by the [no-merges] one.

The warning is gated behind the [no-mentions] config and produces such warning:

There are mentions (@mention) in your commits. We have a no mention policy so these commits will need to be removed for this pull request to be merged.

The following commits have mentions is them:

• commit1
• commit2

r? @ehuss

[ehuss]

If this is intended to go in rust-lang/rust, then I think this would need approval from the primary teams that would be affected there (mainly compiler and libs I'm thinking), but I imagine would transitively also be required for all subtree teams, or otherwise it seems like it would be quite noisy. It's also not clear to me that "need to be removed" is not true, as I don't see anything that would prevent it.

I'd also say that this doesn't seem to cover the PR description which I think is the more common culprit. However, that will conflict with r? commands or general comments people leave to copy other people.

I have a slight worry that this will be too overbearing.

[Urgau]

Sure, this would need buying from subtree first.

I'd also say that this doesn't seem to cover the PR description which I think is the more common culprit. However, that will conflict with r? commands or general comments people leave to copy other people.

I think this is in part already handled by mentions being escaped in homu: rust-lang/homu#100

Non-homu handled repositories would still have this issue, but I think PR description are not as bad as commit messages. I have rarely see people link merge commit (where the PR description end up), while it's much more common for individual commits IMO to be copied.

It's also not clear to me that "need to be removed" is not true, as I don't see anything that would prevent it.

It's the same generic message as the one regarding merge commits not being allowed, which is also not enforced, particularly for subtrees.

I have a slight worry that this will be too overbearing.

As proposed it's limited it to commit message, which hopefully will make it near invisible to most people.

[Kobzol]

The implementation looks good to me. It omits some functionality of no_merges though, notably ignoring drafts and exclude_titles. I'm a bit worried about duplicating the commit loading between these two handlers. After a push to a PR, two handlers will not attempt to load all commits of a PR, which seems a bit wasteful. But since they don't run concurrently, it's probably fine? I wonder if we should perhaps merge these two handlers, into something like "check_commits" handler. They share a lot of code anyway, and in this way we could scan the commits just once and then check merge commits and mentions together.

Same as with no_merges, I think that it's fine that this warns, but doesn't otherwise prevent the PR from being merged.
I'm not sure if the subtrees are such a problem? On sync PRs, this might send a notice if the subtree uses a mention in its commits, but that doesn't seem like such an issue.

[Urgau]

We should probably ignore the draft state. That's a good observation. Excluding some titles seems a bit much, we can add it later if necessary.

Regarding the multiple load of commits, see my response in #1901 (comment).

I'm not sure if the subtrees are such a problem?

Agree with you, it's only a warning, it's not blocking in anyway. I also want to deploy the handler in subtrees first, so it shouldn't be a problem in practice.

[Urgau]

Contrary to what I laid out in #1901 (comment), I rebased in this PR first since it's much much simpler than moving the merge commits handler.

Should be ready for review.

[Kobzol]

Looks good, but left a few comments.

When multiple warnings are included together, it looks a bit confusing, because the submodules handler doesn't actually list the actual commits with submodules. Maybe we could reword it from "These commits modify submodules" to "Some commits modify submodules".

[Kobzol]

Looks good! However, while testing, I found out that the detection of "same warnings" in check_commits is broken 😅
Here we overwrite the warnings variable by adding an asterisk to them, so the bot always thinks that the warnings are different, even though they are in fact the same.

Could you fix that in this PR please? Since it touches these files anyway.

[Urgau]

That's embarrassing. Fixed as a separate commit.

[Kobzol]

Variable shadowing sometimes is a bit dangerous! 😆 Thank you!