On Thu, Jan 21, 2016 at 10:37 PM, David Rajchenbach-Teller < dtel...@mozilla.com> wrote:
> That sounds like it's going to break stuff. > > Reviewers frequently ask me to change stuff during reviews. I don't > re-run all the tests on all the platforms after every single round of > review. Once in a while, the changes end up breaking stuff which I need > to fix – generally trivial stuff that I can fix without requesting an > additional review. > > Also, I frequently ask for review without having run all the tests on > all the platforms. Every so often, I get r+ and only then realize that > it doesn't build under Windows, or it breaks an entirely unrelated test. > > In either case, if the reviewer takes the habit to land my patches > without asking me, we'll end up with much more breakage. > Ultimate goal is to have a 100% linear history with no backouts due to automation failures. This means no merge commits and no integration repositories. The way we do this is effectively push everything to Try before it lands. This will be automatic. It shouldn't be possible for your busted patch to land. We need to make automation complete faster before we can do this, as I'm sure a lot of people would not appreciate an extra few hours of delay between initiating landing and it making it through Try to the final landing repository. I don't know if we'll get there in 2016. > > On 22/01/16 03:35, Gregory Szorc wrote: > > If you have level 3 source code access (can push to central, inbound, > > fx-team) and have pushed to MozReview via SSH, as of a few weeks ago you > > can now land commits from the "Automation" drop down menu on MozReview. > > (Before only the review request author could trigger autoland.) > > > > This means that anyone [with permissions] can land commits with a few > > mouse clicks! It will even rewrite commit messages with "r=" annotations > > with the review state in MozReview. So if someone does a drive-by > > review, you don't have to update the commit message to reflect that > > reviewer. Neato! > > > > I've gotten into the habit of just landing things if I r+ them and I > > think they are ready to land. This has startled a few people because it > > is a major role reversal of how we've done things for years. (Typically > > we require the patch submitter to do the landing.) But I think > > reviewer-initiated landing is a better approach: code review is a gate > > keeping function so code reviewers should control what goes through the > > gate (as opposed to patch authors [with push access] letting themselves > > through or sheriffs providing a support role for people without push > > access). If nothing else, having the reviewer land things saves time: > > the ready-to-land commit isn't exposed to bit rot and automation results > > are available sooner. > > > > One downside to autoland is that the rebase will happen remotely and > > your local commits may linger forever. But both Mercurial and Git are > > smart enough to delete the commits when they turn into no-ops on rebase. > > We also have bug 1237778 open for autoland to produce obsolscence > > markers so Mercurial will hide the original changesets when you pull > > down the rebased versions. There is also potential for some Mercurial or > > Git command magic to reconcile the state of MozReview with your local > > repo and delete local commits that have been landed. This is a bit > > annoying. But after having it happen to me a few times, I think this is > > a minor annoyance compared to the overhead of pulling, rebasing, > > rewriting commit messages, and pushing locally, possibly hours or days > > after review was granted. > > > > I encourage my fellow reviewers to join me and "just autoland it" when > > granting review on MozReview. > > > > gps > > > > > > _______________________________________________ > > firefox-dev mailing list > > firefox-...@mozilla.org > > https://mail.mozilla.org/listinfo/firefox-dev > > > _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
