Closed Bug 1175166 Opened 9 years ago Closed 9 years ago

mozreview sets R? again on R+'d commits

Categories

(MozReview Graveyard :: General, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: pbro, Assigned: smacleod)

References

Details

Attachments

(2 files)

This happened to me recently in bug 1174821, but I've seen this before in other bugs too.

STR:
- work on a bug, create a few commits
- push these commits to mozreview
- in mozreview, select reviewers, publish
- wait for reviews to be done, let's say 3 are R+ and 1 is R-
- address minor review comments in the 3 R+'d commits, and modify the commit message to append r=reviewer
- make changes to the R-'d commit
- use histedit to rewrite the history so that you end up with the same number of commits as before, in the same order, with roughly the same messages (apart from the r=reviewer part)
- push again to mozreview
- in mozreview, the reviewers are already selected in the UI, click publish

Expected:
only the R-'d commit is set to R? again in bugzilla

Actual:
all 4 commits are set to R? again, spamming the reviewers for nothing, and adding confusion to the bug. You then need to add even more confusion and comments by setting them to R+ yourself again.

Am I doing something wrong? It seems to me that there should be a choice for the user in the mozreview UI to ask again for review or not for commits that have been marked "ship it" already (defaulting to no).
mozreview does a good job as far as I can see at dealing with history rewriting, so it knows which commits have been R+'d already.
I am having this exact same problem with the same workflow. If you find out a workaround please let me know. I have to stop using MozReview until this is fixed because of all the spam it's generating.
Yeah I hear you; this problem does clash with our microcommit philosophy.  I'll see about getting this fixed soon.

Note that partial-series landing (bug 1096768) will help this to some degree, but only for r+ed commits that come before r-ed[1] commits, so this is still a valid bug.

[1] "r cancelled" is a more accurate description of MozReview's flow, since r- is never set automatically.
Priority: -- → P1
In the past, I have removed the reviewers in MozReview for already r+ed commits as a workaround before publishing.  But, then you have to mark the attachments in Bugzilla r+ manually.
Assignee: nobody → smacleod
Status: NEW → ASSIGNED
My expectations are that after doing an 'hg push review' on a partially reviewed series something like the following happens:

* 'r+'/'r+ with comments' and patch content has changed
  => no change on Bugzilla
  (or, alternatively, update attachment but preserve review flags)

* 'r+'/'r+ with comments' and patch title has changed
  (e.g. to add '; r=xxx' at the end)
  => no change on Bugzilla

* 'r+'/'r+ with comments' and patch has been rebased
  => no change on Bugzilla

* 'r cancelled' / not yet reviewed and patch content has not changed
  (but might have been rebased)
  => no change on Bugzilla

* 'r cancelled' and patch content has changed
  => re-request review (ideally, but it's ok if this is manual)

* not yet reviewed and patch content has changed
  => not sure, probably should re-request review in case the reviewer
     has downloaded the changeset locally
"'r+'/'r+ with comments' and patch has been rebased" needs to be split into multiple forms: a) file content has changed b) file content hasn't changed.

One can argue that if a rebase results in file level merges then r+ should not automatically be carried over, as other changes in the file may have invalidated the review. Although, *any* change (even in other files) has the potential to invalidate review. So maybe it's not such a big deal. This is why we write tests and use try before landing.
(In reply to Gregory Szorc [:gps] from comment #6)
> "'r+'/'r+ with comments' and patch has been rebased" needs to be split into
> multiple forms: a) file content has changed b) file content hasn't changed.

Yes, I'm referring to just (b). (a) comes under "'r+'/'r+ with comments' and patch content has changed"

> One can argue that if a rebase results in file level merges then r+ should
> not automatically be carried over, as other changes in the file may have
> invalidated the review.

I don't know any teams that work like this. At least in platform we trust people to do the right thing. If someone does a major rebase that involves re-writing significant parts of the patches, we leave it to their discretion to determine if it needs re-review.
We should clarify terminology. When I say "rebase" I mean the act of "transplanting" a commit between parents and nothing more. e.g. here is a rebase of E from B to D:

A -- B -- C  -- D          A -- B --C -- D -- E' -- F'
      \              -->
       E - F

Now, some rebases involve conflicts. We'll ignore those for right now.

I think your term for "rebase" conflates with my term "amending." This is when you rewrite commits in place, preserving the parent.

While both fall under the umbrella of "history rewriting," in my mind "rebase" and "amend" are drastically different because "amend" reflects intent to change the logical composition of a commit whereas "rebase" only (possibly) changes things if other commits have changed them.

Also, something else to consider is that some code may want different policies for carrying r+ forward. e.g. if I were reviewing crypto/security code, I wouldn't want any changes slipping through undetected. This is probably a minority of the use cases, so I reckon we shouldn't give it too much attention at this juncture.
(In reply to Gregory Szorc [:gps] from comment #8)
> We should clarify terminology. When I say "rebase" I mean the act of
> "transplanting" a commit between parents and nothing more. e.g. here is a
> rebase of E from B to D:
> 
> A -- B -- C  -- D          A -- B --C -- D -- E' -- F'
>       \              -->
>        E - F
> 
> Now, some rebases involve conflicts. We'll ignore those for right now.
> 
> I think your term for "rebase" conflates with my term "amending." This is
> when you rewrite commits in place, preserving the parent.

I'm mostly thinking about rebasing here. Personally, if I do amend, I normally rebase at the same time.

In layout code at least, it's not uncommon to have a long patch series that we rebase several times throughout the review process (so we can keep testing the changes against the latest m-c and fix any bitrot that crops up).
(In reply to Brian Birtles (:birtles, away 17~20 July JST) from comment #7)
> (In reply to Gregory Szorc [:gps] from comment #6)
> > One can argue that if a rebase results in file level merges then r+ should
> > not automatically be carried over, as other changes in the file may have
> > invalidated the review.
> 
> I don't know any teams that work like this. At least in platform we trust
> people to do the right thing. If someone does a major rebase that involves
> re-writing significant parts of the patches, we leave it to their discretion
> to determine if it needs re-review.

Additionally, the failure cases from this approach are much less noisy than the failure cases in the current approach.

Right now, mozreview clears r+ and I have to set it back again. This leads to 3 emails for the reviewers (review request, bug comment from mozreview, review canceled) and the review flag is now set to "r+ by patch submitter" rather than "r+ by reviewer", which is something that has bugged me about our previous workflows, and I thought we could avoid here.

If mozreview doesn't automatically clear things and I did want to re-request review, it will lead to 1-2 emails, for my new review request and potentially a comment, and no loss of the flag setter data in bugzilla.

IME, in 99% of cases I have to rebase a patch when I land it (after r+) anyway. We trust people to do the right thing then, I would assume that we can do that while other parts of the patch are still in progress, too.
I've recently switched to using hg bookmarks, but am frequently being asked to rebase patches in response to checkin-needed, even when a subsequent hg rebase ends up succeeding without conflict, which drives me to rebase preemptively to increase the chance of a checkin-needed going through rather than coming back. This means I constantly experience what Gijs explains in comment 11 where Review Board does the wrong thing and makes me spam every reviewer, a source of aggrevation for me, and I'm sure for the reviewers I'm spamming. Isn't everyone experiencing this? If not then please tell me your workflow.

Also, I cannot add a second reviewer it seems, without undoing the r+ from the first reviewer, but maybe that's a separate bug?
mozreview: refactor post_bugzilla_attachment to allow setting r+ (Bug 1175166). r?mcote

To allow ship-its to be carried forward refactor post_bugzilla_attachment
to take a boolean for each reviewer, indicating if that reviewer should
have their associated flag be marked r+. Currently we always pass False
to preserve the current behaviour.
Attachment #8637924 - Flags: review?(mcote)
mozreview: carry forward r+s for L3 request authors (Bug 1175166). r?mcote

If an author of a review request has level 3 commit access we trust that
they will re-request review when it is needed due to changes on an
already reviewed commit. We now formalize this by carrying forward any
r+s that a level 3 submitter had received before updating their review
request.
Attachment #8637925 - Flags: review?(mcote)
(In reply to Steven MacLeod [:smacleod] from comment #14)
> If an author of a review request has level 3 commit access we trust that
> they will re-request review when it is needed due to changes on an
> already reviewed commit. We now formalize this by carrying forward any
> r+s that a level 3 submitter had received before updating their review
> request.

Link to policy please? This seems like a misunderstanding of current practices and who has level 1 access.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #15)
> Link to policy please? This seems like a misunderstanding of current
> practices and who has level 1 access.

I said nothing of level 1 access, I said level 3. Since these level 3 users can directly commit to the repository and "r=<reviewer>" isn't verified as having actually happened we already trust level 3 users.
Is this bug going to be solved only for level 3 users?
Comment on attachment 8637924 [details]
MozReview Request: mozreview: refactor post_bugzilla_attachment to allow setting r+ (Bug 1175166). r?mcote

https://reviewboard.mozilla.org/r/14013/#review12563

::: pylib/rbbz/rbbz/extension.py:242
(Diff revision 1)
> +        # Never carryforward for now to perserve old behaviour.

"preserve"

::: pylib/rbbz/rbbz/extension.py:241
(Diff revision 1)
> +        # TODO: Determine if we should carryforward an r+ for the reviewer.
> +        # Never carryforward for now to perserve old behaviour.

"carry forward"

::: pylib/rbbz/rbbz/bugzilla.py:178
(Diff revision 1)
>  

Hm do we usually double-space elifs?

::: pylib/rbbz/rbbz/bugzilla.py:179
(Diff revision 1)
> -                if (f['name'] == 'review' and 'requestee' in f
> -                    and f['requestee'] in reviewers):
> -                    flags.append({'id': f['id'], 'name': 'review',
> +                elif f['name'] == 'feedback':
> +                    # We always clear feedback flags.
> +                    flags.append({'id': f['id'], 'status': 'X'})

What's the reason you put in a couple of these "if X, clear the flag" instead of just having an else: like the original code did?

::: pylib/rbbz/rbbz/bugzilla.py:204
(Diff revision 1)
> +                    # a reviewer (or there is no requestee?).

FYI 'requestee' is present only if status is '?'.  So these clause would be executed if, for example, someone had manually set an r- on the attachment.

::: pylib/rbbz/rbbz/bugzilla.py:186
(Diff revision 1)
> +                    # clear these flags

Missing period.

::: pylib/rbbz/rbbz/bugzilla.py:208
(Diff revision 1)
> +                    # Ask for re-review

Missing period.

::: pylib/rbbz/rbbz/bugzilla.py:207
(Diff revision 1)
> +                elif f['requestee'] in reviewers:
> +                    # Ask for re-review
> +                    flags.append({
> +                        'id': f['id'],
> +                        'name': 'review',
> +                        'status': '?',
> +                        'requestee': f['requestee']
> +                    })

I just realized that I don't think this is required after all.  Any flag not mentioned in update_attachment() will be left alone, so you should just need to pop the reviewer and that's it.
Attachment #8637924 - Flags: review?(mcote)
https://reviewboard.mozilla.org/r/14013/#review12563

> FYI 'requestee' is present only if status is '?'.  So these clause would be executed if, for example, someone had manually set an r- on the attachment.

*this clause
Comment on attachment 8637925 [details]
MozReview Request: mozreview: carry forward r+s for L3 request authors (Bug 1175166). r=mcote

https://reviewboard.mozilla.org/r/14015/#review12611

::: pylib/rbbz/rbbz/extension.py:245
(Diff revision 1)
> +    # object somewhere

Missing period.

::: pylib/rbbz/rbbz/extension.py:248
(Diff revision 1)
> +    # We will only carryforward ship-it reviews if the submitter

"carry forward"

::: pylib/rbbz/rbbz/extension.py:253
(Diff revision 1)
> +    # publishing, we should cache the returned value somewhere

s/,/;/

::: hgext/reviewboard/tests/test-bugzilla-review-flag-cleared.t:473
(Diff revision 1)
> +We publish on push since we already have a revewer

reviewer
Attachment #8637925 - Flags: review?(mcote) → review+
https://reviewboard.mozilla.org/r/14013/#review12563

> What's the reason you put in a couple of these "if X, clear the flag" instead of just having an else: like the original code did?

I found it simplified the conditionals for some of the cases and made things more explicit for how each flag is handled.
Comment on attachment 8637924 [details]
MozReview Request: mozreview: refactor post_bugzilla_attachment to allow setting r+ (Bug 1175166). r?mcote

mozreview: refactor post_bugzilla_attachment to allow setting r+ (Bug 1175166). r?mcote

To allow ship-its to be carried forward refactor post_bugzilla_attachment
to take a boolean for each reviewer, indicating if that reviewer should
have their associated flag be marked r+. Currently we always pass False
to preserve the current behaviour.
Attachment #8637924 - Flags: review?(mcote)
Comment on attachment 8637925 [details]
MozReview Request: mozreview: carry forward r+s for L3 request authors (Bug 1175166). r=mcote

mozreview: carry forward r+s for L3 request authors (Bug 1175166). r=mcote

If an author of a review request has level 3 commit access we trust that
they will re-request review when it is needed due to changes on an
already reviewed commit. We now formalize this by carrying forward any
r+s that a level 3 submitter had received before updating their review
request.
Attachment #8637925 - Attachment description: MozReview Request: mozreview: carry forward r+s for L3 request authors (Bug 1175166). r?mcote → MozReview Request: mozreview: carry forward r+s for L3 request authors (Bug 1175166). r=mcote
Attachment #8637925 - Flags: review+
Comment on attachment 8637924 [details]
MozReview Request: mozreview: refactor post_bugzilla_attachment to allow setting r+ (Bug 1175166). r?mcote

https://reviewboard.mozilla.org/r/14013/#review12795

Ship It!
Attachment #8637924 - Flags: review?(mcote) → review+
https://hg.mozilla.org/hgcustom/version-control-tools/pushloghtml?changeset=cfef7f25bc53
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
This is deployed to prod.
Depends on: 1195661
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: