Open Bug 1358608 Opened 5 years ago Updated 4 years ago

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

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

People

(Reporter: mak, Unassigned)

References

Details

Attachments

(1 file)

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.
Flags: needinfo?(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.
Flags: needinfo?(gps)
See Also: → 1350767
The existing tests still pass with this change, do I need to add one for the "enndeakin+6102" case?
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=`.
Attachment #8912058 - Flags: review?(gps) → review-
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 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 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
Attachment #8912058 - Flags: review?(gps) → review-
Assignee: kwierso → nobody
You need to log in before you can comment on or make changes to this bug.