1142251 - Have MozReview automatically deduce reviewers from the "r=<nick>" string in the commit message

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P1)

Description

[Botond Ballo [:botond]]

It would be really nice if MozReview parsed the commit message of pushed commits for a string of the form "r=<nick>" (or "r=<nick1>,<nick2>" etc.), mapped the nicks to reviewers, and pre-populated the reviewers field with said reviewers.

Comment 3

[Dan Minor [:dminor]]

Attachment: MozReview Request: bz://1142251/dminor

/r/6373 - Deduce reviewers from the "r=<nick>" string in the commit summary (bug 1142251); r=smacleod

Pull down this commit:

hg pull review -r b085f0b187f922cafac81727b0e03ef2590c3144

Comment 4

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/6373/#review5809

Like you mention we really should be doing this in the server code. I believe what we should be doing is making a request to the "User List Resource"(1) providing the ircnick in the `q` parameter(2). This will sync the RB database for bugzilla users matching that query.

(1) https://www.reviewboard.org/docs/manual/dev/webapi/2.0/resources/user-list/
(2) https://www.reviewboard.org/docs/manual/dev/webapi/2.0/resources/user-list/#GET_params

Comment 5

[Dan Minor [:dminor]]

Comment on attachment 8586255 [details]
MozReview Request: bz://1142251/dminor

/r/6373 - hgext: Deduce reviewers from the "r=<nick>" string in the commit summary (bug 1142251); r=smacleod,gps

Pull down this commit:

hg pull -r 8cef0a85046a58064ea1982e45816858aed5ddd0 https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8586255 [details]
MozReview Request: bz://1142251/dminor

https://reviewboard.mozilla.org/r/6371/#review6431

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:158
(Diff revision 2)
> +    def extract_reviewers(commit):
> +        summary = commit['message'].splitlines()[0]
> +        reviewers = []
> +        if summary.find('r=') != -1:
> +            # the first chunk is everything up to the first r=, so we skip it
> +            chunks = summary.split('r=')[1:]
> +            for chunk in chunks:
> +                subchunks = re.split('[ ,]', chunk)
> +                for subchunk in subchunks:
> +
> +                    # some people use parentheses or brackets to specify
> +                    # reviewers so we might end up with junk here
> +                    subchunk = re.sub('[\[\]\(\)]', '', subchunk)
> +
> +                    # skip empty subchunks or subchunks with other specifiers
> +                    # (e.g. a=)
> +                    if not subchunk or subchunk.find('=') != -1:
> +                        continue
> +
> +                    # Check to see if this user exists (and sync things up
> +                    # between reviewboard and bugzilla, if necessary).
> +                    # If we don't get a user (or get more than one) we'll
> +                    # leave it up to the user to add the reviewer manually.
> +                    try:
> +                        r = api_root.get_users(q=subchunk)
> +                        if len(r.rsp['users']) == 1:
> +                            reviewers.append(r.rsp['users'][0]['username'])
> +                    except APIError:
> +                        pass
> +
> +        return reviewers

There is code in pylib/mozautomation/mozautomation/commitparser.py for parsing reviewers out of commit messages. We should embrace and extend that code instead of reinventing parsing here.

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:196
(Diff revision 2)
> +            "target_people": ','.join(extract_reviewers(commit)),

What's the behavior when target_people is already populated and its contents were modified via the Review Board web UI? Do we lose web-defined reviewers whenever a new commit is pushed?

::: hgext/reviewboard/tests/test-specify-reviewers.t:6
(Diff revision 2)
> +Enable obsolescence so we can test code paths which use it.
> +
> +  $ cat > obs.py << EOF
> +  > import mercurial.obsolete
> +  > mercurial.obsolete._enabled = True
> +  > EOF
> +
> +  $ echo "rebase=" >> client/.hg/hgrc
> +  $ echo "obs=$TESTTMP/obs.py" >> client/.hg/hgrc

This is a holdover from a previous Mercurial era. The new way to do this is to set:

[experimental]
evolution = all

Although, this may not work on Mercurial older than 3.3. I'd have to check...

::: hgext/reviewboard/tests/test-specify-reviewers.t:45
(Diff revision 2)
> +  $ hg commit -m 'Bug 1 - More stuff; (r=romulus)'
> +  $ echo blah >> foo
> +  $ hg commit -m 'Bug 1 - More stuff; (r=romulus,remus)'
> +  $ echo blah >> foo
> +  $ hg commit -m 'Bug 1 - More stuff; [r=romulus]'
> +  $ echo blah >> foo
> +  $ hg commit -m 'Bug 1 - More stuff; [r=remus, r=romulus]'

Let's not support the (r=*) syntax.

Comment 7

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/6373/#review6699

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:42
(Diff revision 2)
>              'individual': [
>                  {
>                      'id': <commit-id>,
>                      'precursors': [<previous changeset>],
>                      'message': <commit-message>,
>                      'diff': <diff>,
>                      'parent_diff': <diff-from-base-to-commit>,
>                  },

I think I'd prefer if we parsed out the reviewers outside of `post_reviews` and then passed the parsed names in with the other data.

Comment 8

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/6371/#review6711

> Let's not support the (r=*) syntax.

Any particular reason? People currently use this in their commits and the regexp in pylib/mozautomation/mozautomation/commitparser.py would have to be modified to disallow it.

Comment 9

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/6371/#review6745

> What's the behavior when target_people is already populated and its contents were modified via the Review Board web UI? Do we lose web-defined reviewers whenever a new commit is pushed?

Right now, if the commit is unchanged during a push then the reviewers are preserved. If the commit is amended then the reviewers are reset to the ones specified in the r= string. For the most part I think this makes sense and I've added tests for these cases. If for some reason the automatic identification is consistently failing for a reviewer and the commit is amended frequently then this could be annoying.

Comment 10

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/6373/#review6757

I think we should consider whether people manually adding "r=" to commit messages *before review* is the right thing. I can make an argument that the "r=" annotation shouldn't be added until after a review is granted. I argue this is the ideal state because it avoids ambiguities ("I thought this patch was reviewed because it has 'r=' so I landed it").

I argue that a large part of people adding "r=foo" to commit messages is because they don't want to waste time adding this annotation later. But in an Autoland world where machines are landing the code and can add annotations at land time, this concern goes away.

I argue that a significant portion of "r=foo" annotations are people flagging reviewers on commits at the time they are committed. That's probably a good idea. But I'm not convinced this is significantly better than say flagging reviewers at push/review submission time or having machines select a reviewer automatically based on what changed. There is value in that tools recognize it and you can "fire and forget" when you commit. I just don't know if "r=" annotations are really the best/ideal way to facilitate this.

Anyway, I think we should at least think of better workflows before cargo culting the widespread practice of adding "r=foo" before review is actually granted.

Comment 11

[Botond Ballo [:botond]]

(In reply to Gregory Szorc [:gps] from comment #10)
> I argue that a significant portion of "r=foo" annotations are people
> flagging reviewers on commits at the time they are committed. That's
> probably a good idea. But I'm not convinced this is significantly better
> than say flagging reviewers at push/review submission time 

I like choosing reviewers at commit time because that's when the content of the commit, and what parts of the code it modifies, is freshest in my mind. Push / review submission time can sometimes be a fair bit later, after other patches are written, Try runs are waited for, etc., and going back to evaluate who should review each patch of the submission at that time can be an extra task.

Also, automatically deducing reviewers means we'd be just one '--publish' option away from a review submitter not having to interact with the ReviewBoard UI at all:

  hg ci -m "Bug XXX - Message 1. r=reviewer1"
  hg ci -m "Bug XXX - Message 2. r=reviewer2"
  hg push review --publish
  # walk away

> Anyway, I think we should at least think of better workflows before cargo
> culting the widespread practice of adding "r=foo" before review is actually
> granted.

The feature suggested in this bug doesn't force people to add "r=foo" before review - it just makes it one of several options that are supported (without requiring duplicate entry of information).

Comment 12

[Mark Côté [:mcote]]

We definitely want to support adding reviewers on the command line, or I do at least--I think the question right now is how we should do that, given gps's concern.  Perhaps it could be as simple as putting a question mark after, e.g. r=gps?, that autoland could strip off during landing.  That's one idea anyway.

Comment 13

[Dan Minor [:dminor]]

(In reply to Gregory Szorc [:gps] from comment #10)
> I think we should consider whether people manually adding "r=" to commit
> messages *before review* is the right thing. I can make an argument that the
> "r=" annotation shouldn't be added until after a review is granted. I argue
> this is the ideal state because it avoids ambiguities ("I thought this patch
> was reviewed because it has 'r=' so I landed it").

Any evidence that this is occurring in practice and is a widespread enough problem that we need to convince people to adopt a new syntax? Like botond said, we're not forcing anyone to do this, just making it an option for the (many) people who have requested it.

I think having Autoland add reviewers when landing to inbound if none have been specified is a wonderful idea (I filed bug 1160479), but we don't land to inbound right now so that isn't really an option for people :)

(If we're really concerned about patches landing without proper review we could create a tool that monitors commits to integration branches on pulse, parses the reviewers and queries bugzilla to ensure that the reviewers actually granted r+ on the patch.)

Comment 14

[Dan Minor [:dminor]]

Comment on attachment 8586255 [details]
MozReview Request: bz://1142251/dminor

/r/6373 - hgext: Deduce reviewers from the "r=<nick>" string in the commit summary (bug 1142251); r=gps, r=smacleod

Pull down this commit:

hg pull -r a901ae9c4920bbe2ddea72d1a7443a5ec1c6307f https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 15

[Gregory Szorc [:gps]]

I just wanted to ask the question so there was *some* discussion.

I just wrote in bug 1160479 a proposal to Autoland to assuage some of my concerns with the "trustability" of r=.

I agree defining reviewers at commit time is a must support. Some kind of syntax in the commit message *is* the easiest way. Although, we could potentially also have Mercurial bring up an interactive wizard at commit time too. That would arguably be even easier (at least for non-power users).

Real quick: how about recognizing "r?gps" as the "reviewer wanted" syntax and parsing "r=gps" as "review granted." Autoland can, of course, rewrite things automatically before landing. I don't think the single character change would confuse muscle memory too much. And it addresses my "r= accuracy" concern.

Comment 16

[Botond Ballo [:botond]]

(In reply to Gregory Szorc [:gps] from comment #15)
> Real quick: how about recognizing "r?gps" as the "reviewer wanted" syntax
> and parsing "r=gps" as "review granted." Autoland can, of course, rewrite
> things automatically before landing. I don't think the single character
> change would confuse muscle memory too much. And it addresses my "r=
> accuracy" concern.

That sounds great to me once Autoland is in production.

Until then, it would be great to recognize the "r=gps" syntax as well.

Comment 17

[(not currently active) Ted Mielczarek]

(In reply to Gregory Szorc [:gps] from comment #15)
> Real quick: how about recognizing "r?gps" as the "reviewer wanted" syntax
> and parsing "r=gps" as "review granted." Autoland can, of course, rewrite
> things automatically before landing. I don't think the single character
> change would confuse muscle memory too much. And it addresses my "r=
> accuracy" concern.

+1 from me, I'm kind of uncomfortable with writing "r=foo" in a commit message for a patch that hasn't actually been reviewed.

Comment 18

[Dan Minor [:dminor]]

What about adding a little warning to the client that shows up when there is a 'r=gps' in a non-public commit that says using 'r?gps' would activate filling in the reviewers automatically. That would make this feature more discoverable and maybe encourage people to stop using r= for unreviewed commits.

I'm still not convinced that using 'r=' in an unreviewed commit is a big enough problem that we need to do something special here, but if both ted and gps think it is, I have no problems with doing this.

Comment 19

[Mark Côté [:mcote]]

I think dminor has a great suggestion.  We could even say that support for r=... may be removed after autoland is implemented, which will automatically convert r? to r= before landing.  Until we have autoland, it is definitely more convenient to write r=... when we push for review, so that after an r+ you can just push to inbound without having to edit the commit message.

Comment 20

[Gregory Szorc [:gps]]

What if someone (me?) wrote a Mercurial extension/command that made it easy to automatically rewrite r? to r=?

Comment 21

[Mark Côté [:mcote]]

That would be cool, but it's still one more step, so I would say support r= but with a warning and a remark that there's a command to transform r? to r=.

Comment 22

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/6373/#review7031

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:35
(Diff revisions 2 - 3)
>      The ``commits`` argument takes the following form::
>  
>          {
>              'squashed': {
>                  'diff': <squashed-diff-string>,
>              },
>              'individual': [
>                  {
>                      'id': <commit-id>,
>                      'precursors': [<previous changeset>],
>                      'message': <commit-message>,
>                      'diff': <diff>,
>                      'parent_diff': <diff-from-base-to-commit>,
>                  },
>                  {
>                      ...
>                  },
>                  ...
>              ]
>          }

Please update this docstring when making any changes to the format of the commits data structure.

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:161
(Diff revisions 2 - 3)
> -        if summary.find('r=') != -1:
> +        for reviewer in commit['reviewers']:

Lets make 'reviewers' optional:
    for reviewer in commit.get('reviewers', []):

Comment 23

[Dan Minor [:dminor]]

Comment on attachment 8586255 [details]
MozReview Request: bz://1142251/dminor

/r/6373 - hgext: Deduce reviewers from the "r?<nick>" string in the commit summary (bug 1142251); r=gps, r=smacleod

Pull down this commit:

hg pull -r d3fd3036e95e54d66630598dcc316b61a9732606 https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 24

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/6373/#review7171

Awesome work!

Thanks for accepting the feedback to use r?.

The core of this patch looks great. Just have a number of questions around the periphery.

::: hgext/reviewboard/client.py:328
(Diff revision 4)
> +    # than r=. This regular expression is based upon the one in

The comment is incomplete.

::: hgext/reviewboard/hgrb/proto.py:256
(Diff revision 4)
> +            'reviewers': [r for r in commitparser.parse_reviewers(summary)]

Can be written with `list(...)` instead of a comprehension.

::: hgext/reviewboard/tests/test-specify-reviewers.t:62
(Diff revision 4)
> +  review:     http://*:$HGPORT1/r/2 (draft) (glob)

We may want to supplement the output with the list of reviewers. This will be a very invasive change to output. Best to do as a follow-up commit, methinks.

Also, please coordinate with me, as my work to add commit identifiers changes hashes everywhere and there will be bit rot. The change to add reviewers to the output is easy: you can mv the .t.err files on top of .t and it should "just work." Whereas my hash changes involve merge conflicts. So I'd prefer to land first :)

::: hgext/reviewboard/tests/test-specify-reviewers.t:111
(Diff revision 4)
> +  romulus+6

This seems like a bug since the input contained "r?romulus,r?remus"

::: hgext/reviewboard/tests/test-specify-reviewers.t:114
(Diff revision 4)
> +  romulus+6

This also seems wrong.

::: hgext/reviewboard/tests/test-specify-reviewers.t:257
(Diff revision 4)
> +Amending a commit will reset the reviewers back to the default.

This is somewhat interesting behavior. I'd think that if someone were added as a reviewer via the web interface that they would stick.

I'm opening an issue for you to consider this. But I reckon this can easily be changed in a follow-up, so I wouldn't worry too much about it.

::: hgext/reviewboard/tests/test-specify-reviewers.t:318
(Diff revision 4)
> +Unrecognized reviewers should be ignored

It would be *really* nice if we got a warning about this at push time.

There is a mechanism in the response protocol to print messages to the client. We should consider using that.

::: hgext/reviewboard/tests/test-specify-reviewers.t:359
(Diff revision 4)
> +  (It appears you are using r= to specify reviewers for a patch under review. Please use r? to avoid ambiguity as to whether or not review has been granted.)

I'd consider:

1) Including the changeset fragment in the message
2) Printing instructions on how to resolve this (`hg histedit <commit>`)
3) Printing this message below each changeset in the review output that is impacted.

(I think I like #3 the best, since once reviewers are printed in output, we can consistently have 1 line for reviewers.)

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:172
(Diff revision 4)
> +                r = api_root.get_users(q=reviewer)

Is this going to add considerable latency to pushes or will RBClient cache results for redundant lookups?

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:172
(Diff revision 4)
> +                r = api_root.get_users(q=reviewer)
> +                if len(r.rsp['users']) == 1:
> +                    username = r.rsp['users'][0]['username']

q= will match substrings, right? Should we be verifying that the username exactly matches the requested reviewer?

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:176
(Diff revision 4)
> +                    squashed_reviewers.add(username)

This is going to conflict with 1 flag per commit, isn't it? But, I reckon this is easy enough to change.

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:188
(Diff revision 4)
> +            "target_people": ','.join(extract_reviewers(commit)),

I /think/ throwing a sorted() in here is warranted. We do that other places where we want output to be deterministic. And, I don't think reviewer order matters too much.

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:324
(Diff revision 4)
> +            target_people=','.join(extract_reviewers(commit)))

sorted()

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:384
(Diff revision 4)
> +        'target_people': ','.join(squashed_reviewers),

sorted()

::: testing/vcttesting/reviewboard/mach_commands.py:208
(Diff revision 4)
>          for p in rr.target_people:
> -            people.add(p.get().username)
> +            people.append(p.get().username)

Now that you added modification to the draft, should we unconditionally modify the published review request? Or should we only do it if there isn't a draft?

::: testing/vcttesting/reviewboard/mach_commands.py:234
(Diff revision 4)
> +    def list_reviewers(self, rrid):
> +        from rbtools.api.errors import APIError
> +        root = self._get_root()
> +        rr = root.get_review_request(review_request_id=rrid)
> +
> +        people = []
> +        for p in rr.target_people:
> +            people.append(p.get().username)
> +
> +        try:
> +            draft = rr.get_draft()
> +            for p in draft.target_people:
> +                people.append(p.get().username)
> +        except APIError:
> +            pass
> +
> +        print(','.join(people))

We should differentiate between target_people on the draft and published review request.

Comment 25

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/6373/#review7187

> Is this going to add considerable latency to pushes or will RBClient cache results for redundant lookups?

RBClient does have caching, but the version of RBTools we're stuck on has bugs in the caching, and I have a patch to disable the cache that will be going up for review as part of upgrading us to Review Board 2.0.15 (with RB 2.0.15 our tests hit the RBClient bug). Though, the problems with RBTools stopping us from upgrading are being fixed and should be in the next RBTools release.

Comment 26

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/6373/#review7189

::: testing/vcttesting/reviewboard/mach_commands.py:208
(Diff revision 4)
>          for p in rr.target_people:
> -            people.add(p.get().username)
> +            people.append(p.get().username)

For each target people entry the `title` attribute is the username, you can just access that rather than fetch the actually resource.

::: testing/vcttesting/reviewboard/mach_commands.py:213
(Diff revision 4)
> +            for p in draft.target_people:
> +                people.append(p.get().username)

Again, we can just access the `title` of each person.

Also, the target_people in the draft start as a copy of the target_people in the review request itself - what we should really be doing here is not ever fetching the people from the review request itself, only the draft, and updating that list.

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:167
(Diff revision 4)
> +            # Check to see if this user exists (and sync things up
> +            # between reviewboard and bugzilla, if necessary).
> +            # If we don't get a user (or get more than one) we'll
> +            # leave it up to the user to add the reviewer manually.
> +            try:
> +                r = api_root.get_users(q=reviewer)

Something that would ensure usernames were an exact match would be using `get_item` from the list rather than querying each time.

Example:
```
users = api_root.get_users()
...
try:
    users.get_item(reviewer) # requests /api/users/<reviewer>/
    ...
except APIError:
    pass
```

The only issue is I'm unsure if requesting a user directly will actually cause our bugzilla wackiness to populate the users.

Comment 27

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/6373/#review7237

> This is going to conflict with 1 flag per commit, isn't it? But, I reckon this is easy enough to change.

I followed up with mcote, he said that this will stop being synchronized to bugzilla in the future but won't cause problems. We need it for now to keep js validation happy when the user publishes from the root review request.

Comment 28

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/6373/#review7253

> It would be *really* nice if we got a warning about this at push time.
> 
> There is a mechanism in the response protocol to print messages to the client. We should consider using that.

I've filed bug 1164127 as a follow up for this simply because the plumbing required to get these messages back from where we're validating users to where we can add them to the response protocol isn't straightforward and is probably better handled in a separate bug.

> This is somewhat interesting behavior. I'd think that if someone were added as a reviewer via the web interface that they would stick.
> 
> I'm opening an issue for you to consider this. But I reckon this can easily be changed in a follow-up, so I wouldn't worry too much about it.

I've filed bug 1164132 as a follow up for this.

Comment 29

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/6373/#review7259

> I /think/ throwing a sorted() in here is warranted. We do that other places where we want output to be deterministic. And, I don't think reviewer order matters too much.

It appears reviewboard was sorting behind the scenes anyway but no harm in being safe.

Comment 30

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/6373/#review7289

> Something that would ensure usernames were an exact match would be using `get_item` from the list rather than querying each time.
> 
> Example:
> ```
> users = api_root.get_users()
> ...
> try:
>     users.get_item(reviewer) # requests /api/users/<reviewer>/
>     ...
> except APIError:
>     pass
> ```
> 
> The only issue is I'm unsure if requesting a user directly will actually cause our bugzilla wackiness to populate the users.

I gave this a shot but it doesn't seem to make the bugzilla magic work.

Comment 31

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/6373/#review7317

> RBClient does have caching, but the version of RBTools we're stuck on has bugs in the caching, and I have a patch to disable the cache that will be going up for review as part of upgrading us to Review Board 2.0.15 (with RB 2.0.15 our tests hit the RBClient bug). Though, the problems with RBTools stopping us from upgrading are being fixed and should be in the next RBTools release.

I'll add a check to see if the reviewer is already present in squashed_reviewers and avoid the API call in this case.

Comment 32

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/6373/#review7325

> q= will match substrings, right? Should we be verifying that the username exactly matches the requested reviewer?

It does match substrings but we need to keep this for placeholder usernames to work (as far as I can tell.) For instance q="romulus+6" in the tests will never match, but q="romulus" does.

Comment 33

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/6373/#review7329

> It does match substrings but we need to keep this for placeholder usernames to work (as far as I can tell.) For instance q="romulus+6" in the tests will never match, but q="romulus" does.

I can avoid this problem in the tests by properly specifying an ircnick in the bugzilla user's name so that a placeholder name isn't used, which will let me use exact matches.

> Now that you added modification to the draft, should we unconditionally modify the published review request? Or should we only do it if there isn't a draft?

I think smacleod's comments below take care of this.

Comment 34

[Dan Minor [:dminor]]

Comment on attachment 8586255 [details]
MozReview Request: bz://1142251/dminor

/r/6373 - hgext: Deduce reviewers from the "r?<nick>" string in the commit summary (bug 1142251); r=gps, r=smacleod

Pull down this commit:

hg pull -r 20cc96521b75e4d5fadbb3a2c2e86213a83807ce https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 35

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/6373/#review7795

I'd really like to see unit tests on the newly recognized reviewer syntax, especially since I plan to leverage this code on hg.mozilla.org for hooks and for extracting reviewers for easier display.

::: hgext/reviewboard/client.py:415
(Diff revision 5)
> +        if len(list(parse_requal_reviewers(ctx.description()))):

No need to do len() on a list since an empty list isn't True.

::: hgext/reviewboard/hgrb/proto.py:256
(Diff revision 5)
> +            'reviewers': list(commitparser.parse_reviewers(summary))

IMO this should should be done inside pushhooks.py. But we can leave it here, it's no big problem.

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:187
(Diff revision 5)
> +        return sorted(reviewers)

I'm not sure why you are using a list only to return sorted results. Use a set?

::: pylib/mozautomation/mozautomation/commitparser.py:40
(Diff revision 5)
> -    for r in REVIEW_RE.findall(s):
> +    for r in SPECIFIER_RE.split(s)[1:]:
> +        for part in LIST_RE.split(r):
> +            part = part.strip('[](){} ')
> +            if part:
> +                yield SPECIFIER_RE.split(part)[-1]
> +
> +def parse_requal_reviewers(s):
> +    for r in REVIEW_REQUAL_RE.findall(s):
>          for part in LIST_RE.split(r):
> -            yield part.strip('[](){}')
> +            part = part.strip('[](){} ')
> +            if part:
> +                yield SPECIFIER_RE.split(part)[-1]

Thanks for doing this!

But where are the unit tests for the new syntax?

Comment 36

[Gregory Szorc [:gps]]

Comment on attachment 8586255 [details]
MozReview Request: bz://1142251/dminor

https://reviewboard.mozilla.org/r/6371/#review7797

Ship It!

Comment 37

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/6373/#review7941

> IMO this should should be done inside pushhooks.py. But we can leave it here, it's no big problem.

I disagree and actually asked Dan to put it here rather than in pushhooks. I believe it keeps pushhooks more focused and allows us to splat any data at it (For example, if we start taking reviewers from the in-tree meta data rather than the comit messages that should really live in outside of pushhooks).

Comment 38

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/6373/#review7939

::: hgext/reviewboard/hgrb/proto.py:14
(Diff revision 5)
> +from mozautomation import commitparser
>  
>  class AuthorizationError(Exception):

2 blanks after import before definitions.

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:168
(Diff revision 5)
> +            # review for this commit.

"review" -> "review request" or "add them as a reviewer for this commit"

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:179
(Diff revision 5)
> +                for user in r.rsp['users']:
> +                    username = user['username']
> +                    if reviewer == username:
> +                        reviewers.append(username)
> +                        squashed_reviewers.add(username)

If the query returns more than 25 users (which TBH, with our 300k users isn't *that* unlikely) the resource will paginate and this code will only loop through the first 25.

We could loop over the pages fetching them as we go or maybe what we should do is combine this, and my other suggestion - If the list returns more than one user for our query then call `r.get_item(reviewer)`. We know that the first query refreshed the bugzilla user data, so `get_item` should have fresh data to find the exact user.

::: testing/vcttesting/reviewboard/mach_commands.py:234
(Diff revision 5)
> +        for p in rr.target_people:
> +            people.append(p.title)
> +
> +        try:
> +            draft = rr.get_draft()
> +            for p in draft.target_people:
> +                people.append(p.title + ' (draft)')
> +        except APIError:
> +            pass

I don't think we should be combining the reviewers from the request and the draft. This function should probably take an option to specify if we want to list the drafts reviewers, or the request itself.

Combining the two lists together could hide subtle bugs if someone isn't carefully thinking about which reviewers they want to check, and what operations they're performing.

Comment 39

[Steven MacLeod [:smacleod]]

Comment on attachment 8586255 [details]
MozReview Request: bz://1142251/dminor

Just setting r- because I want to take another quick look after the issues are fixed.

Comment 40

[Dan Minor [:dminor]]

Comment on attachment 8586255 [details]
MozReview Request: bz://1142251/dminor

/r/6373 - hgext: Deduce reviewers from the "r?<nick>" string in the commit summary (bug 1142251); r=gps, r=smacleod

Pull down this commit:

hg pull -r 78f531e6ec3b0cdfde07fa0a1b163bb4bd5d08b1 https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 41

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/6373/#review8077

LGTM!

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:142
(Diff revision 6)
> +    users = api_root.get_users()

Since you never actually use the users returned from this, a micro-optimization would be to add a `max_results=1` so that it limits the size pulled from the DB / sent over the wire here.

Comment 42

[Steven MacLeod [:smacleod]]

Comment on attachment 8586255 [details]
MozReview Request: bz://1142251/dminor

https://reviewboard.mozilla.org/r/6371/#review8079

Ship It!

Comment 43

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/6373/#review8161

Ship It!

Comment 44

[Gregory Szorc [:gps]]

Comment on attachment 8586255 [details]
MozReview Request: bz://1142251/dminor

https://reviewboard.mozilla.org/r/6371/#review8163

Ship It!

Comment 45

[Dan Minor [:dminor]]

Thanks, pushed to: https://hg.mozilla.org/hgcustom/version-control-tools/rev/e2aa9e4182ac

smacleod, I tried your max_results=1 trick but it seems to once again bust our bugzilla synchronization. That might be good fodder for a follow up bug.

Comment 47

[Dan Minor [:dminor]]

Attachment: MozReview Request: hgext: Deduce reviewers from the "r?<nick>" string in the commit summary (bug 1142251); r=gps, r=smacleod