1277849 - Pushing an already r+'d patch lead to set myself as a reviewer

Status: RESOLVED FIXED

(MozReview Graveyard :: Integration: Bugzilla, defect)

Description

[Nicolas Chevobbe [:nchevobbe]]

I had a review with 2 patches, both r+ (https://reviewboard.mozilla.org/r/56816/).
The reviewer had made some comments though so I did fixed them and pushed again to review, without changing the commit message.


Expected result:
Nothing, the patches were already r+'d, it should have stay this way

Actual result:
I was set as reviewer on one of my patch (https://bugzilla.mozilla.org/show_bug.cgi?id=1248274#c26).

Might be related : I think Bug 1244263 landed in between my 2 last pushes, and it did remove the r= part in one of my patch. Maybe it triggered some kind of weird state and lead to this bug.

Comment 1

[Robert Helmer [:rhelmer]]

I just saw this happen in bug 1231172.

This bug has been open for a while, but my recollection of events leading up to this:

1) pushed two commits with f?aswan
2) pushed various work-in-progress for discussion.
3) reordered commits, changed commit messages to from f? to r? and pushed

Comment 2

[Mark Côté [:mcote]]

I couldn't reproduce this locally; however, I was trying with a fresh setup.  I wouldn't be surprised if this was a backwards-compat-related bug.

Comment 3

[Mark Côté [:mcote]]

99.9% sure this is related to bug 1197879.  Can you take a look?

Comment 5

[:glob ✱]

i can't reproduce this issue either, even when i perform the initial review prior to bug 1197879, and push a follow-up after.  but something's clearly going on here, continuing my investigations.

Comment 6

[:glob ✱]

a ha!  i can reproduce this locally.

looks like what's happening is when someone performs a 'drive-by' review, they are re-requested for review when a new revision is uploaded.

commenting on your own code after it has been uploaded is technically a review, resulting in the erroneous review flags.

Comment 7

[:glob ✱]

Attachment: mozreview: clear impromptu review flags in bugzilla (bug 1277849);

When a new revision is pushed for review, clear any attachment flags in
Bugzilla that were set by people not targetted by the current review.  This
ensures that "drive-by" reviewers are not requested to review followup commits.

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

Comment 8

[Mark Côté [:mcote]]

Comment on attachment 8760764 [details]
mozreview: clear impromptu review flags in bugzilla (bug 1277849);

https://reviewboard.mozilla.org/r/58224/#review55098

::: pylib/mozreview/mozreview/bugzilla/attachments.py:79
(Diff revision 1)
>              if review_request_draft.diffset:
> +                # The code has changed, we need to determine what needs to
> +                # happen to the flags.
> +
> +                # Don't set carry_forward values for reviewers that are not in
> +                # the target_people list (eg. drive-by reviews).  This will

Can we call these something other than "drive-by"?

Comment 9

[:glob ✱]

https://reviewboard.mozilla.org/r/58224/#review55098

> Can we call these something other than "drive-by"?

i've never heard them called anything else at mozilla, so removing it from the comment could be counter-productive.

how about drive-by/impromptu?

Comment 10

[:glob ✱]

Comment on attachment 8760764 [details]
mozreview: clear impromptu review flags in bugzilla (bug 1277849);

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/2e7c42880550
mozreview: clear impromptu review flags in bugzilla ; r=mcote