1096768 - Improve workflow for partially landing a series during review

Status: RESOLVED INVALID

(MozReview Graveyard :: General, defect, P2)

Description

[Gregory Szorc [:gps]]

I argue it is a best practice to land code as soon as it gets reviewed. You avoid bit rot and you feel like you are moving in a forward direction.

MozReview encourages writing more, smaller patches. It is natural for some patches to get granted review before others. We shouldn't have to wait for the entire series to receive r+ before landing some of it.

Currently, if you have a series that gets partially reviewed, you land the patches that have been reviewed, and push an updated series to MozReview, the old, landed patches disappear from the web interface. If you aren't using obsolescence / changeset evolution, the patches at the end of the series get mapping to the landed commits! That's very bad.

I'd like to see better support for handling commit series that include already-landed patches. We have enough context in the client and server: we should be able to detect when a series has commits that landed already. We should be able to intelligently mark them as such and prevent state from getting out of whack.

This is kinda/sorta related to the "how do we tell Review Board that commits landed" problem.

Comment 1

[Mark Côté [:mcote]]

We have too many P1s, so I'm spreading out the priorities.  P3 -> P4, P2 -> P3, and some portion of P1s will become P2.

Comment 2

[Dan Minor [:dminor]]

We've had a number of people in IRC with problems with this lately, so I'm bumping the priority accordingly.

Comment 3

[Patrick Brosset <:pbro>]

Just got bitten by this in bug 1181837. I pushed the 5 first commits to mozreview, got them reviewed and landed. The week after I worked on 3 more commits. When I pushed them to mozreview I realized it thought these were new versions of the some of the 5 first commits (they were already marked as R+ and the diffs had the revision sliders in them).
It'd be really nice if mozreview could either detect this on its own, or if we had a way to tell it that these were extra commits, not new versions of the old ones.

Comment 4

[Gregory Szorc [:gps]]

We're about to have a bunch of Git users start using MozReview and I have a feeling this will be one of their top complaints.

Comment 5

[Brian Birtles (:birtles)]

Just got bitten by this in bug 1245748 using exactly the same steps as comment 3.

Comment 6

[Mark Côté [:mcote]]

This is super useful, but I'm going to whittle the P1s down to bugs that we might actually be able to fix in the short term.

Comment 9

[BMO Automation]

MozReview is now obsolete. Please use Phabricator instead. Closing this bug.