1192958 - r= warning should not be displayed if r+ would be carried forward

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P3)

Description

[Gregory Szorc [:gps]]

When I get r+ on some commits in a series, I rewrite the commit message from r? to r= to reflect that r+. I then push the updated series for review on the commits that didn't get r+. The client prints a whole bunch of warnings that I'm using r= syntax:

changeset:  14610:90ad9855ed2d
summary:    ansible/hg-reviewboard: remove executability of pash files (bug 1192338); r=fubar
(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.)
review:     https://reviewboard.mozilla.org/r/15411 (draft)

changeset:  14612:2736652a3611
summary:    ansible/hg-reviewboard: more minor formatting changes (bug 1192388); r=fubar
(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.)
review:     https://reviewboard.mozilla.org/r/15413 (draft)

changeset:  14613:879d3776005b
summary:    ansible/hg-reviewboard: grab LDAP settings from a file (bug 1192388); r?fubar
review:     https://reviewboard.mozilla.org/r/15557 (draft)

changeset:  14614:570e46e9e917
summary:    ansible/hg-reviewboard: convert pash from templates to regular files (bug 1192388); r?fubar
review:     https://reviewboard.mozilla.org/r/15559 (draft)

review id:  bz://1192338/gps
review url: https://reviewboard.mozilla.org/r/15387 (draft)

I think we should not show the r= warning message if the review request already has a ship it that would be carried forward.

Comment 1

[Mark Côté [:mcote]]

We discussed this today.  While a good idea, it sounds a little complicated to do.  With both the r+ carry forward and autoland, manually updating your commit messages from r? to r= isn't particularly useful.  I found it useful before r+ carry forward as a note to myself to clear reviewers for those commits so as to not re-request review, but now this is done automatically.  And autoland will rewrite r? to r=.

So, yes, would be nice to support it, but given that it's probably not trivial, I set it to P3.

Comment 2

[Martin Thomson [:mt:]]

As a frequent reviewer, this is one of the most annoying features of mozreview.  I grant a review, but get a long series of emails asking for new requests.

I suggest this:

1. Take someone off the reviewers list if they are marked r= (warn if you like, maybe don't offer to publish)
2. Later, add whatever it takes to detect that the review has been carried forward

Comment 3

[Dan Minor [:dminor]]

(In reply to Martin Thomson [:mt:] from comment #2)
> As a frequent reviewer, this is one of the most annoying features of
> mozreview.  I grant a review, but get a long series of emails asking for new
> requests.
> 
> I suggest this:
> 
> 1. Take someone off the reviewers list if they are marked r= (warn if you
> like, maybe don't offer to publish)
> 2. Later, add whatever it takes to detect that the review has been carried
> forward

I think your suggestion on r= behaviour makes good sense.

Reviews are already being carried forward, but only for individuals with L3 access, so if you're being flagged for review again, either the review requester does not have L3 access, or you've hit a bug.

We realize this is more conservative than the current system, where someone could have a r+ with nits and then be landed with checkin-needed without requiring a new review. We're planning to look at this policy again once we see how things play out with Autoland.

Comment 4

[Dan Minor [:dminor]]

Attachment: MozReview Request: mozreview: remove warning when pushing with r= (bug 1192958) r=gps

mozreview: remove warning when pushing with r= (bug 1192958) r=glob

We're planning to change the behaviour of r?/r= so that r= will not
request a new review. The first step is to remove the warning from the
client if someone pushes with a r=.

Comment 5

[Dan Minor [:dminor]]

Attachment: MozReview Request: mozreview: add function to parse r? reviewers (bug 1192958) r=glob

mozreview: add function to parse r? reviewers (bug 1192958) r=glob

Comment 6

[Dan Minor [:dminor]]

Attachment: MozReview Request: mozreview: r= warning should not be displayed if r+ would be carried forward (bug 1192958) r=gps

mozreview: r= warning should not be displayed if r+ would be carried forward (bug 1192958)

This checks to see if the most recent review by a reviewer flagged as r=
was a ship-it. If so, no warning is displayed. Otherwise, we display a
warning and flag the reviewer for another review.

Comment 7

[Dan Minor [:dminor]]

Comment on attachment 8686077 [details]
MozReview Request: mozreview: remove warning when pushing with r= (bug 1192958) r=gps

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

Comment 8

[Dan Minor [:dminor]]

Comment on attachment 8686078 [details]
MozReview Request: mozreview: add function to parse r? reviewers (bug 1192958) r=glob

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

Comment 9

[Dan Minor [:dminor]]

Comment on attachment 8686079 [details]
MozReview Request: mozreview: r= warning should not be displayed if r+ would be carried forward (bug 1192958) r=gps

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

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8686077 [details]
MozReview Request: mozreview: remove warning when pushing with r= (bug 1192958) r=gps

https://reviewboard.mozilla.org/r/24943/#review22663

Comment 11

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/24945/#review22665

::: pylib/mozautomation/mozautomation/commitparser.py:29
(Diff revision 2)
> +RQUESTION_SPECIFIER_RE = re.compile(r'r?')

r? means "optionally match the literal 'r'."

You want "r\?". And some tests.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8686079 [details]
MozReview Request: mozreview: r= warning should not be displayed if r+ would be carried forward (bug 1192958) r=gps

https://reviewboard.mozilla.org/r/24947/#review22667

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:231
(Diff revision 2)
> +        requal_reviewers = extract_reviewers(commit.get('requal_reviewers', []))

Ugh - another HTTP request :/

Push performance is already suffering a bit. We really need to start thinking about an API on ReviewBoard for doing all this work faster.

My Git push work this quarter /might/ take me down the road in order to avoid code duplication.

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:238
(Diff revision 2)
> +                    hgresp.append('display summary for commit %s has r=%s '

"display summary" feels a bit weird for this context since this will be printed to hg clients. Can we say "commit message" instead?

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:686
(Diff revision 2)
> +        return datetime.datetime.strptime(x[2], '%Y-%m-%dT%H:%M:%SZ')

Dates are probably a worse sorting key than database primary key since clock skew is a thing. And I'm pretty sure the RB API returns things in ascending PK order. Ask smacleod to confirm.

Comment 13

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/24947/#review22667

> Dates are probably a worse sorting key than database primary key since clock skew is a thing. And I'm pretty sure the RB API returns things in ascending PK order. Ask smacleod to confirm.

smacleod's not available to confirm and the behaviour is not obvious to me from a quick look at the reviewboard source. I'm going to add a sort on "id" rather than datetime. My worry is that sqlite will always return them in pk order but MySQL won't and we won't realize until this hits production.

Comment 14

[Dan Minor [:dminor]]

Comment on attachment 8686077 [details]
MozReview Request: mozreview: remove warning when pushing with r= (bug 1192958) r=gps

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

Comment 15

[Dan Minor [:dminor]]

Attachment: MozReview Request: testing: make mozautomation.commitparser importable from test_commitparser (bug 1192958) r=glob

testing: make mozautomation.commitparser importable from test_commitparser (bug 1192958) r=glob

At the moment, this test is not directly runnable from run-tests. This adds
commitparser to the system path to fix this.

Comment 16

[Dan Minor [:dminor]]

Comment on attachment 8686078 [details]
MozReview Request: mozreview: add function to parse r? reviewers (bug 1192958) r=glob

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

Comment 17

[Dan Minor [:dminor]]

Comment on attachment 8686079 [details]
MozReview Request: mozreview: r= warning should not be displayed if r+ would be carried forward (bug 1192958) r=gps

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

Comment 18

[:glob ✱]

Comment on attachment 8686078 [details]
MozReview Request: mozreview: add function to parse r? reviewers (bug 1192958) r=glob

https://reviewboard.mozilla.org/r/24945/#review22915

lgtm

Comment 19

[:glob ✱]

Comment on attachment 8688617 [details]
MozReview Request: testing: make mozautomation.commitparser importable from test_commitparser (bug 1192958) r=glob

https://reviewboard.mozilla.org/r/25355/#review22917

lgtm

Comment 20

[Dan Minor [:dminor]]

Thanks!

https://hg.mozilla.org/hgcustom/version-control-tools/rev/0c9fb9e611cd
https://hg.mozilla.org/hgcustom/version-control-tools/rev/bf0f019f41aa
https://hg.mozilla.org/hgcustom/version-control-tools/rev/bea04a60ab18
https://hg.mozilla.org/hgcustom/version-control-tools/rev/648776fdc476