Closed Bug 1195661 Opened 5 years ago Closed 2 years ago

Cannot re-request review after getting an r+

Categories

(MozReview Graveyard :: General, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: Gijs, Unassigned)

References

Details

Attachments

(1 obsolete file)

For bug 1192720 I pushed an updated version of the commit (not identical) and published from the commandline. The bugzilla attachment kept r+ instead of going back to r?billm (which is what I wanted, because I'd changed the patch because of test failures).

This seems like a logical consequence of bug 1175166 being fixed. It seems there are a couple of improvements we could make here:

0) Nothing. It's pretty rare that I need to re-request review on a different version of an already r+'d patch because tests failed and I had to make a non-trivial change to address this.
1) if people repush a 1-commit review push that has r+, with the commit listing the same reviewers, assume they do so to re-request review on a changed patch. Don't assume this for multiple commits where some have r+ and some don't.
2) prompt the user?
3) magical detection of whether repushed things are "just" rebases or substantial changes from the previously submitted commit (this seems error-prone and not a great idea, but it is technically one of the possible approaches).


I can live with (0) aka WONTFIX, but I figured I should flag this up.

(CC'ing commenters on bug 1175166 to get feedback if they feel strongly about any of these options)
My plan was to follow up after bug 1175166 and add some way to MozReview to re-request review (whether that by from commit message parsing, web UI, or something else is undecided) because I thought it was important to deal with the carry forward r+ case ASAP. Feel free to provide ideas for how best to allow the author to re-request review.

As of today, your best option is to just go manually re-request the r? flag in bugzilla yourself (which sucks, but I imagined would be a rare case as you pointed out).
(In reply to :Gijs Kruitbosch from comment #0)
> 0) Nothing. It's pretty rare that I need to re-request review on a different
> version of an already r+'d patch because tests failed and I had to make a
> non-trivial change to address this.
> 1) if people repush a 1-commit review push that has r+, with the commit
> listing the same reviewers, assume they do so to re-request review on a
> changed patch. Don't assume this for multiple commits where some have r+ and
> some don't.

I'm not entirely sure I've understood this correctly but I think I agree with (0). I can see that (1) makes sense in your particular example, but in the areas I've been working repushing a previously r+ed 1-commit review push in order to address review feedback / fix bitrot without requesting re-review is much more common. Folks who prefer to let someone else land for them do this a lot.
Assignee: nobody → dminor
Priority: -- → P1
Blocks: 1212358
Duplicate of this bug: 1219646
Blocks: 1220214
No longer blocks: 1220214
The intention is to make r? always flag someone for review, even if they've previously granted an r+. This will be easier to do once Bug 1229468 is fixed, as that will move all of our reviewer parsing inside of MozReview instead of the current situation where some is done through reviewboard API calls.
Depends on: 1229468
This changes the r+ carry forward behaviour to not carry forward r+ if the
reviewer was specified with r? in the commit summary. Doing this from the
UI will be fixed in another bug.

Review commit: https://reviewboard.mozilla.org/r/31965/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31965/
Attachment #8711066 - Flags: review?(smacleod)
Comment on attachment 8711066 [details]
MozReview Request: mozreview: allow r? to cancel carrying forward r+ (bug 1195661) r?smacleod

https://reviewboard.mozilla.org/r/31965/#review28947

::: hgext/reviewboard/tests/test-bugzilla-review-flag-cleared.t:548
(Diff revision 1)
> +Using r? syntax should stop review carry forward

We also need to prevent the carryforward for things like review request landing approval. (It could also be nice to show a little indicator on a review in the UI if it is being carried forward or not, but that might be out of scope for this)
Attachment #8711066 - Flags: review?(smacleod)
Product: Developer Services → MozReview
Duplicate of this bug: 1252757
Comment on attachment 8711066 [details]
MozReview Request: mozreview: allow r? to cancel carrying forward r+ (bug 1195661) r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31965/diff/1-2/
Attachment #8711066 - Flags: review?(smacleod)
Attachment #8711066 - Flags: review?(smacleod)
Assignee: dminor → smacleod
Status: NEW → ASSIGNED
Attachment #8711066 - Attachment is obsolete: true
Depends on: 1274371
Duplicate of this bug: 1247069
Clarifying the summary.
Summary: When pushing an update to an existing review with 1 commit, r+ does not get changed to r? → Cannot re-request review after getting an r+
Duplicate of this bug: 1251449
Blocks: 1227923
Freeing this bug up.
Assignee: smacleod → nobody
Status: ASSIGNED → NEW
Assignee: nobody → glob
zalun's working on this.
Assignee: glob → pzalewa
Duplicate of this bug: 1321699
Summary: Cannot re-request review after getting an r+ → Cannot re-request review after getting an r+ or r- or canceled
You can re-request review after getting an r- or canceled, whenever you push new commits. But you cannot re-request review after getting r+ even if you push new commits. I think this bug is more about the latter issue.
Summary: Cannot re-request review after getting an r+ or r- or canceled → Cannot re-request review after getting an r+
We're planning to implement following feature. Please comment on it.

* There will be a new HG setting to switch to "never rerequest". It's gonna be off by default.
* "r=" prefix might be used in commit message to not rerequest on push.
* "r?" prefix will rerequest review (reset all flags including r+).

As you can see the planned workflow will change current one. Rerequesting will come as a default action.
It's not clear to me from the plan that, in which case the default action would take effect? When there is neither "r=" nor "r?" in the comment message?

What would happen if there are both "r?" and "r="? I would expect that it only re-requests from reviewers after "r?", but not those after "r=" (partially reset). But I guess that's not a big issue.
Also what happens when you use r= for the first review request? It should still request a review. Further in case of a large patch with a lot of commits are we forced to do a histedit each time to change between r? -> r= -> r?. For me it would be annoying.
For me, a button on the reviewboard web UI with the label "Re-request review" would be much more useful. I wouldn't want to go through my whole patch stack and update commit messages manually.
(In reply to Markus Stange [:mstange] from comment #19)
> For me, a button on the reviewboard web UI with the label "Re-request
> review" would be much more useful. I wouldn't want to go through my whole
> patch stack and update commit messages manually.

I agree with Markus - a "re-request review" button seems a more sensible plan, and also simpler to implement than the logic outlined in comment #16. As I noted in comment #0, it is relatively rare that after an r+ I explicitly want to go back and re-request review. So the default should continue to be carrying forward review.
Note also that some of the other suggestions in comment #0 were before autoland - now I might repush in order to address final nits and autoland, as reviewboard is the only way to autoland right now. I know we're planning to change that, but it would be unfortunate to 'regress' the current workflow before that is a reality.

If we really want to stick with the plan in comment #16, I would want the inverse button: "Carry forward earlier r+" to override the re-requested review and mark r+ instead (though that will cause bugspam, so it's not ideal). I would want that because it seems very likely I will occasionally accidentally re-push with "r?" still in the patch commit message, which will re-request review and prevent me from autolanding.
How often do you push changes to r+'d review request?
The idea is that by default pushing a new commit to an r+'d rr would rerequest it. If you know you don't want it - change the commit message and use r= instead.

A button to rerequest is a good idea from UX point of view. It should be implemented as a first stage at least.
It also fits in proposed scenario if one always uses r= and would need to push specifically to rerequest.
(In reply to Piotr Zalewa [:zalun] from comment #21)
> How often do you push changes to r+'d review request?

All the time. Sometimes because there's a small mistake that a try push caught after review was granted, sometimes because I just want to rebase to a more recent mozilla-central base so that autoland is more likely to succeed, but usually just to address review comments. Many people grant review with a comment like "r+ with the comments addressed" and trust the patch author to address the review comments correctly so that the reviewer does not have to re-review the patch.
(In reply to Piotr Zalewa [:zalun] from comment #21)
> How often do you push changes to r+'d review request?

My experience matches Markus'. Also sometimes if I land something on autoland and it has issues - sheriffs back it out, and I fix whatever silly oversight caused bustage, repush to mozreview, reuse the r+ and re-autoland. This happened yesterday, for instance, in https://reviewboard.mozilla.org/r/98862/ . Also, in some cases I have 5 csets and 3 have r+ and 2 get r- and need updates. If any of the r+'d ones follow the r-'d ones, they are technically going to be new csets (and in some cases need rebase-style fixes due to changes to the earlier csets). I wouldn't want mozreview to re-request review for those, either.

Out of curiosity, doesn't the reviewboard database have data on this? I would expect data on requests after autoland-to-inbound/autoland to match the experiences Markus and I are describing - but I could be wrong, of course. Even just the ratio of "commits with r+ without any issues" to "commits with r+ if and only if these issues are fixed" might be interesting data to help inform a decision here.
In the case where the new patch I push has a different MozReview-Commit-ID, it seems like this should definitely re-request review.  Conceptually, this is a different patch entirely.
I could re-request a review in bug 1325299 with the following step:
1. Run "hg histedit" to drop the latest changeset locally.
2. Make changes to the local tree.
3. Run "hg commit" to commit changes.
4. Run "hg push review".

Maybe because a new MozReview-Commit-ID is generated? Is this workaround known?
(In reply to Masatoshi Kimura [:emk] from comment #25)
> I could re-request a review in bug 1325299 with the following step:
> 1. Run "hg histedit" to drop the latest changeset locally.
> 2. Make changes to the local tree.
> 3. Run "hg commit" to commit changes.
> 4. Run "hg push review".
> 
> Maybe because a new MozReview-Commit-ID is generated? Is this workaround
> known?

The ReviewRequest is not re-requested. The old ReviewRequest is discarded and a new one is created.
What is the difference from a reviewboard user perspective?
[:emk] you loose all reviews created for discarded requests.
not working on this at the moment
Assignee: pzalewa → nobody
I noticed this came up again recently in bug 1345222. 
glob, are you working on mozreview? Do you know who might take this on? Thanks.
Flags: needinfo?(glob)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #30)
> I noticed this came up again recently in bug 1345222. 
> glob, are you working on mozreview? Do you know who might take this on?

right now i'm not working on mozreview i'm sorry.
i'll chat with zalun to see what we can do about this issue.
Flags: needinfo?(glob)
MozReview is now obsolete. Please use Phabricator instead. Closing this bug.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.