Prevent spam from GitHub mentions in merge commits

[xosnrdev]

Resolves #258

[Kobzol]

Hi, thanks for the PR, I'll try to take a look in the upcoming days.

[Kobzol]

Thank you for the PR! The implementation looks like a good start, but there are a few things that should be modified, I left a bunch of comments.

[xosnrdev]

Thanks for the feedback. I didn't put unicode text into consideration 😞.

Will resolve shortly.

[xosnrdev]

Would fuzz test be ideal or overkill ?

[Kobzol]

Fuzz testing would be way overkill I think :) To clarify, I didn't mean that the GitHub username could include Unicode, as far as I know, it cannot contain it. So for matching the usernames, doing what you had before (a-zA-Z0-9) was fine. Just the way you searched for the character before/after the match wasn't correct.

The code is now more complicated than before. What I meant was to solve the preceding/following word in the regex itself.

What do you think about this regex? (^|\s)@([a-zA-Z0-9][a-zA-Z0-9]*)($|\s)

[xosnrdev]

Not sure this (^|\s)@([a-zA-Z0-9][a-zA-Z0-9]*)($|\s) matches auto-linking for @org/team and also trying boundary checks with look-around using the regex crate is not supported:

regex parse error:
    (?<!\S)@[a-zA-Z0-9][a-zA-Z0-9\-]{0,38}(?:/[a-zA-Z0-9][a-zA-Z0-9\-]{0,38})*(?!\S)
    ^^^^
error: look-around, including look-ahead and look-behind, is not supported
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
)

A workaround for look-around can be may be using the fancy-regex crate rust-lang/regex#910 at the cost of performance or writing more complex regex code.

[Kobzol]

I know that regex doesn't support lookaround, but we don't need it, right? We can just match the before/after part of the regex, and then ignore it, it doesn't need to be lookbehind/lookahead. Of course the regex would have to be extended for teams, it was just an example.

[xosnrdev]

Ofc we don't need look-around.

[Kobzol]

Nice, this made the code much shorter and easier to understand! :) I think that we should use \s for the before/after chars though, otherwise e.g. abč@user.com would count as a mention.

[xosnrdev]

I'll add test for this abč@user.com.

using /s will fail this test Hello,@user! How are you? as it matches whitespace characters like spaces, tabs, and newlines

[Kobzol]

using /s will fail this test Hello,@user! How are you? as it matches whitespace characters like spaces, tabs, and newlines

Yes, that was the idea :D I wonder if GitHub does indeed ping in these situations. It doesn't seem like so? I think that whitespace should be enough.

Hello,@xosrndev, are you pinged?

[Kobzol]

Hello, @xosrndev, are you pinged?

Hmm, it doesn't ping even above, unless I manually select the user. I guess that it shouldn't be tested in PR/issue comments, but rather in the commit messages directly. We should first test on some test repo in which exact situations does GitHub indeed ping when the mention is in the commit message.

[xosnrdev]

Well it did ping on a test repo.

[Kobzol]

Interesting, thanks for testing that out! Ok, in that case let's just use what homu does for now, which is \B, so "not a word boundary". It tries to detect a situation where there is a word on the left, but not on the right (and on the right we always have @). I'm not fully sure how rust-lang/homu#230 works yet, so let's start with just copying homu, and we can improve the regex in the future, once we have the baseline functionality merged.

[xosnrdev]

bet.

[Kobzol]

Thanks! Copying homu is usually not a bad default, we can always make it smarter in the future. Left one last nit, otherwise LGTM.

[Kobzol]

Thanks!