1358608 - dom peers hook doesn't properly parse nicknames containing "+"

Status: NEW

(Developer Services :: Mercurial: hg.mozilla.org, defect)

Description

[Marco Bonardo [:mak]]

In bug 1356440 I cannot use autoland to land the commit because I have a review from "enndeakin+6102"
The DOM peers hook at prevent_webidl_changes.py does this:

              matches = re.findall('\Ws?r\s*=\s*(\w+(?:,\w+)*)', message)

but it doesn't work with fancy chars (due to \w), so it only finds one reviewer (the wrong one), and refuses to land the patch.

Note that I also had other issues with mozreview and Neil's nickname, when I push to mozreview it also can only see one reviewer and I have to edit the reviewers list manually.

Comment 1

[Gregory Szorc [:gps]]

First, tell Neil to update his Bugzilla name field to include the "[:ircnick]" convention.

Second, we should modify the DOM peers hook to use the reviewer parsing logic from the mozautomation.commitparser module so it is consistent with everything else.

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: hooks: use commitparser.py to find reviewers in prevent_webidl_changes.py (bug 1358608)

Review commit: https://reviewboard.mozilla.org/r/183438/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183438/

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

The existing tests still pass with this change, do I need to add one for the "enndeakin+6102" case?

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8912058 [details]
hooks: use commitparser.py to find reviewers in prevent_webidl_changes.py (bug 1358608)

https://reviewboard.mozilla.org/r/183438/#review188690

Just needs a minor change.

I'm not too worried about the test case for users with `+` in their usernames.

::: hghooks/mozhghooks/prevent_webidl_changes.py:138
(Diff revision 1)
>              # Ignore backouts
>              if isBackout(message):
>                  continue
>  
>              def search(authors, peers):
> -              matches = re.findall('\Ws?r\s*=\s*(\w+(?:,\w+)*)', message)
> +              matches = list(commitparser.parse_reviewers(message))

This should be `parse_requal_reviewers()` so we only extract reviewers after `r=`.

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8912058 [details]
hooks: use commitparser.py to find reviewers in prevent_webidl_changes.py (bug 1358608)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183438/diff/1-2/

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8912058 [details]
hooks: use commitparser.py to find reviewers in prevent_webidl_changes.py (bug 1358608)

https://reviewboard.mozilla.org/r/183438/#review189030

::: hghooks/mozhghooks/prevent_webidl_changes.py:138
(Diff revision 2)
>              # Ignore backouts
>              if isBackout(message):
>                  continue
>  
>              def search(authors, peers):
> -              matches = re.findall('\Ws?r\s*=\s*(\w+(?:,\w+)*)', message)
> +              matches = list(commitparser.parse_requal_reviewers(message))

Weird, this is failing some of the tests now that it's using parse_requal_reviewers()...

Previously, the `Multiple reviewers, one of which is a DOM peer, should be allowed` test parsed out these reviewers for the various commits:
['foobar', 'lumpy,jst']
['foobar', 'jst']
['foobar,jst']
['foobar', 'lumpy,jst']
['foobar', 'jst']
['foobar,jst']

Using the requal parser, I get:
[]
[]
[]
['foobar', 'lumpy', 'jst']
['foobar', 'jst']
['foobar', 'jst']

And the test doesn't clean up after itself, so the tests that follow still include these six commits, causing them to fail

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8912058 [details]
hooks: use commitparser.py to find reviewers in prevent_webidl_changes.py (bug 1358608)

https://reviewboard.mozilla.org/r/183438/#review189030

> Weird, this is failing some of the tests now that it's using parse_requal_reviewers()...
> 
> Previously, the `Multiple reviewers, one of which is a DOM peer, should be allowed` test parsed out these reviewers for the various commits:
> ['foobar', 'lumpy,jst']
> ['foobar', 'jst']
> ['foobar,jst']
> ['foobar', 'lumpy,jst']
> ['foobar', 'jst']
> ['foobar,jst']
> 
> Using the requal parser, I get:
> []
> []
> []
> ['foobar', 'lumpy', 'jst']
> ['foobar', 'jst']
> ['foobar', 'jst']
> 
> And the test doesn't clean up after itself, so the tests that follow still include these six commits, causing them to fail

The reviewers with commas in them were wrong and the new behavior is correct.

But there /may/ be an underlying bug in the requal parser where it doesn't like multiple `r=` in commit messages?

If there is, we should fix it. But if that's too much work, we can switch to the regular parse_reviewers() and have a follow-up to use parse_requal_reviewers().

Also, I'll be away for a few weeks starting essentially in a few hours. It is best to send subsequent reviews to glob.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8912058 [details]
hooks: use commitparser.py to find reviewers in prevent_webidl_changes.py (bug 1358608)

https://reviewboard.mozilla.org/r/183438/#review189182