Re: Just Autoland It

On 1/26/16 10:56 PM, Karl Tomlinson wrote: On Fri, 22 Jan 2016 14:24:38 -0500, Ehsan Akhgari wrote: What about the case where the information doesn't exist in the repository because the author, for example, cherry-picked a specific commit on a throw-away branch because the rest of the

Re: Just Autoland It

On 28/01/16 06:31 PM, Eric Rescorla wrote: On Thu, Jan 28, 2016 at 10:58 AM, Gregory Szorc wrote: I'd like to thank everyone for the feedback in this thread. However, the thread has grown quite long and has detoured from its original subject. Speaking on behalf of

Re: Just Autoland It

On Fri, Jan 29, 2016 at 6:22 AM, Andrew Halberstadt < ahalberst...@mozilla.com> wrote: > On 28/01/16 06:31 PM, Eric Rescorla wrote: > >> On Thu, Jan 28, 2016 at 10:58 AM, Gregory Szorc >> wrote: >> >> I'd like to thank everyone for the feedback in this thread. However, the

Re: Just Autoland It

On 2016-01-29 10:27 AM, Eric Rescorla wrote: > On Fri, Jan 29, 2016 at 6:22 AM, Andrew Halberstadt < > ahalberst...@mozilla.com> wrote: > >> On 28/01/16 06:31 PM, Eric Rescorla wrote: >> >>> On Thu, Jan 28, 2016 at 10:58 AM, Gregory Szorc >>> wrote: >>> >>> I'd like to thank

Re: Just Autoland It

On Fri, Jan 29, 2016 at 7:27 AM, Eric Rescorla wrote: > On Fri, Jan 29, 2016 at 6:22 AM, Andrew Halberstadt < > ahalberst...@mozilla.com> wrote: > >> On 28/01/16 06:31 PM, Eric Rescorla wrote: >> >>> On Thu, Jan 28, 2016 at 10:58 AM, Gregory Szorc >>> wrote:

Re: Just Autoland It

On 1/29/16 10:42 AM, Mike Conley wrote: most of the feedback is via negativa[2] That's definitely no good, I agree. Attacking the MozReview team is also not ok, obviously. and nobody is really forcing you to use MozReview. Well, sort of. A review requester who uses Mozreview is forcing

Re: Just Autoland It

I respect everybody talking in this thread a great deal, but I thought I might gently suggest that folks exercise a bit of empathy for what the MozReview team[1] are trying to accomplish, and how difficult that work actually is. Trying to build a tool that satisfies such a wide spectrum of

Re: Just Autoland It

>> Since making the review requester feel crappy is not generally >> considered good, most review requestees don't push back on MozReview >> requests, even if they find it very frustrating to work with. I think >> this dynamic is at the heart of a lot of the angst about MozReview: >> people just

Re: Just Autoland It

On 1/29/16 11:18 AM, Boris Zbarsky wrote: This is reasonable advice for review requesters, but not for review requestees, per above. :( That said, I guess most of this thread has been from the requester point of view, not the requestee. The main dynamic here seems to be that people who

Re: Just Autoland It

On Fri, Jan 29, 2016 at 8:12 AM, Dave Townsend wrote: > On Fri, Jan 29, 2016 at 7:27 AM, Eric Rescorla wrote: > > More generally, I keep seeing comments (especially from GPS) about > > trying to push people towards some workflow that's different from the >

Re: Just Autoland It

On 01/29/2016 08:37 AM, Mike Conley wrote: Since making the review requester feel crappy is not generally considered good, most review requestees don't push back on MozReview requests, even if they find it very frustrating to work with. I think this dynamic is at the heart of a lot of the angst

Re: Just Autoland It

I'd like to thank everyone for the feedback in this thread. However, the thread has grown quite long and has detoured from its original subject. Speaking on behalf of everyone who works on MozReview, we know the interface is lacking in areas and features are confusing or non-existent. We're

Re: Just Autoland It

On Thu, Jan 28, 2016 at 10:58 AM, Gregory Szorc wrote: > I'd like to thank everyone for the feedback in this thread. However, the > thread has grown quite long and has detoured from its original subject. > > Speaking on behalf of everyone who works on MozReview, we know the >

Re: Just Autoland It

On 1/28/2016 6:30, Karl Tomlinson wrote: Honza Bambas writes: On 1/25/2016 20:23, Steve Fink wrote: For navigation, there's a list of changed files at the top (below the fixed summary pane) that jumps to per-file anchors. Not good enough for review process. Are you saying you want tabs or

Re: Just Autoland It

On Thu, Jan 28, 2016 at 8:25 AM, Honza Bambas wrote: > On 1/28/2016 6:30, Karl Tomlinson wrote: > >> Honza Bambas writes: >> >> On 1/25/2016 20:23, Steve Fink wrote: >>> For navigation, there's a list of changed files at the top (below the fixed summary pane) that

MozReview interdiffs, was Re: Just Autoland It

On 01/26/2016 07:49 PM, Karl Tomlinson wrote: Boris Zbarsky writes: On 1/23/16 9:48 PM, Mike Hommey wrote: Note that if /other/ changes from other bugs have happened to the same files between the last reviewed iteration and the rebase before landing, the interdiff will show them without any

Re: Just Autoland It

Honza Bambas writes: > On 1/25/2016 20:23, Steve Fink wrote: >> For navigation, there's a list of changed files at the top >> (below the fixed summary pane) that jumps to per-file anchors. > > Not good enough for review process. > >> Are you saying you want tabs or something for this (like >>

Re: Just Autoland It

Boris Zbarsky writes: > On 1/23/16 9:48 PM, Mike Hommey wrote: >> Note that if /other/ changes from other bugs have happened to the same >> files between the last reviewed iteration and the rebase before landing, >> the interdiff will show them without any kind of visual cues. > > Ah, that's

Re: Just Autoland It

>> On 25/01/16 05:44 PM, Eric Rescorla wrote: >> >On Mon, Jan 25, 2016 at 1:58 PM, Mike Hommey wrote: >> > >> >>It's also painful to use MozReview's comment system. The comments in the >> >>reviews pane don't show much diff context, and while I just realized >> >>it's possible

Re: Just Autoland It

On Fri, 22 Jan 2016 14:24:38 -0500, Ehsan Akhgari wrote: > What about the case where the information doesn't exist in the > repository because the author, for example, cherry-picked a > specific commit on a throw-away branch because the rest of the > dependencies are still being worked on? Or,

Re: Just Autoland It

Mike Hommey writes: > On Mon, Jan 25, 2016 at 11:23:59AM -0800, Steve Fink wrote: >> Heh. Your list of UI complaints is very similar to mine. Some comments: >> >> >> On 01/25/2016 04:26 AM, Honza Bambas wrote: > Also, iirc, when you reply diff comments in MozReview, the resulting > comments

Re: Just Autoland It

Note that we have problems on the tree due to https://bugzilla.mozilla.org/show_bug.cgi?id=1243276 - to avoid more problems from broken autolander landings we will set the trees to approval-only to avoid incomplete landings from autolander. - Tomcat

Re: Just Autoland It

On 1/26/16 7:38 AM, Axel Hecht wrote: Which is basically what I do whenever I want to do something. I have a clear idea and intention on what I want to show up on bugzilla, but not on what to do on reviewboard to get there. Which might just be a category of documentation that's not written yet.

Re: Just Autoland It

On 1/26/2016 10:26 AM, Boris Zbarsky wrote: On 1/26/16 7:38 AM, Axel Hecht wrote: Which is basically what I do whenever I want to do something. I have a clear idea and intention on what I want to show up on bugzilla, but not on what to do on reviewboard to get there. Which might just be a

Re: Just Autoland It

FWIW, adding r- abilities is bug 1197879[1]. There's a prototype patch that adds the UI, but I believe the MozReview team was still trying to sort out the best terminology to use. [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1197879 On 26/01/2016 10:46 AM, Benjamin Smedberg wrote: > > >

Re: Just Autoland It

On 1/26/2016 16:46, Benjamin Smedberg wrote: On 1/26/2016 10:26 AM, Boris Zbarsky wrote: On 1/26/16 7:38 AM, Axel Hecht wrote: Which is basically what I do whenever I want to do something. I have a clear idea and intention on what I want to show up on bugzilla, but not on what to do on

Re: Just Autoland It

th 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 re

Re: Just Autoland It

On 01/24/2016 04:48 AM, Mike Hommey wrote: On Sat, Jan 23, 2016 at 09:33:15PM -0500, Boris Zbarsky wrote: Sure. And the "r+ with these changes, and feel free to land, but I want to see the interdiff" mode is supported with Autoland because the interdiff would be available in mozreview

Re: Just Autoland It

On 01/23/2016 09:41 PM, Ehsan Akhgari wrote: Related to this, I always found it a bit surprising we perform so much activity on the patch author side before submission. Part of me thinks reviewers should take one quick glance at the interdiff before the final version lands and should be

Re: Just Autoland It

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

Re: Just Autoland It

On Monday 2016-01-25 11:35 -0500, Mike Conley wrote: > Just be sure to file them. Having talked to the MozReview team about these > types of bugs, I do know that trust-worthiness of diffs and interdiffs is > very much a thing that we should be able to take for granted. Any bugs in > diff and

Re: Just Autoland It

I should point out that these interdiff issues are being actively worked on. At least one of them, bug 1238000[1], already has a patch that's under review to land in core[2]. Just be sure to file them. Having talked to the MozReview team about these types of bugs, I do know that trust-worthiness

Re: Just Autoland It

Heh. Your list of UI complaints is very similar to mine. Some comments: On 01/25/2016 04:26 AM, Honza Bambas wrote: Writing both as a patch author and a reviewer as well. - as a patch author I want a full control on when the patch actually lands (dependencies, any other timing reasons, that

Re: Just Autoland It

On Sat, Jan 23, 2016 at 11:11 AM, Eric Rescorla wrote: > Following up in this. We're not the first people to have autoland, so is > there some reason > not to simply copy what others do here. Specifically, here's the Chromium > commitbot > behavior:

Re: Just Autoland It

On 1/22/2016 2:00 PM, Mark Hammond wrote: (Hopefully) related - what exactly is the "checkin?" flag for? As far as I understand, it's used together with the "checkin-needed" keyword when there is ambiguity about which of the attachments in the bug should be landed by the sheriffs or the

Re: Just Autoland It

On 1/23/16 9:48 PM, Mike Hommey wrote: Note that if /other/ changes from other bugs have happened to the same files between the last reviewed iteration and the rebase before landing, the interdiff will show them without any kind of visual cues. Ah, that's unfortunate. I agree that this is a

Re: Just Autoland It

On Mon, Jan 25, 2016 at 11:23:59AM -0800, Steve Fink wrote: > Heh. Your list of UI complaints is very similar to mine. Some comments: > > > On 01/25/2016 04:26 AM, Honza Bambas wrote: > >Writing both as a patch author and a reviewer as well. > > > >- as a patch author I want a full control on

Re: Just Autoland It

On 25/01/16 05:44 PM, Eric Rescorla wrote: On Mon, Jan 25, 2016 at 1:58 PM, Mike Hommey wrote: On Mon, Jan 25, 2016 at 11:23:59AM -0800, Steve Fink wrote: Heh. Your list of UI complaints is very similar to mine. Some comments: On 01/25/2016 04:26 AM, Honza Bambas wrote:

Re: Just Autoland It

On 01/25/2016 01:58 PM, Mike Hommey wrote: On Mon, Jan 25, 2016 at 11:23:59AM -0800, Steve Fink wrote: - will it be possible to still be using hg patch queues? A valid question, though I'd not that the mq-less workflow is actually pretty good these days. mq is still easier/nicer for some

Re: Just Autoland It

On Mon, Jan 25, 2016 at 1:58 PM, Mike Hommey wrote: > On Mon, Jan 25, 2016 at 11:23:59AM -0800, Steve Fink wrote: > > Heh. Your list of UI complaints is very similar to mine. Some comments: > > > > > > On 01/25/2016 04:26 AM, Honza Bambas wrote: > > >Writing both as a patch

Re: Just Autoland It

On Mon, Jan 25, 2016 at 06:15:10PM -0500, Andrew Halberstadt wrote: > On 25/01/16 05:44 PM, Eric Rescorla wrote: > >On Mon, Jan 25, 2016 at 1:58 PM, Mike Hommey wrote: > > > >>On Mon, Jan 25, 2016 at 11:23:59AM -0800, Steve Fink wrote: > >>>Heh. Your list of UI complaints is

Re: Just Autoland It

On 2016-01-23 9:37 PM, Boris Zbarsky wrote: On 1/23/16 2:41 PM, Ehsan Akhgari wrote: FWIW, option 3 is basically my usual workflow Option 3, or option 2? Just to recap, option 3 is that I write patches for bug A and bug B in that order in my tree (A, then B) and ask for review on both. They

Re: Just Autoland It

On Fri, Jan 22, 2016 at 1:45 PM, Boris Zbarsky wrote: > On 1/22/16 3:52 PM, Gregory Szorc wrote: > >> I would say that pushing cherry-picked commits for review that depend on >> other commits not in the commit's ancestry is just wrong. If you pushed >> this to Try, it would

Re: Just Autoland It

ide 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 land

Re: Just Autoland It

. 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

Re: Just Autoland It

On Sat, Jan 23, 2016 at 6:39 AM, Gijs Kruitbosch wrote: > On 22/01/2016 20:52, Gregory Szorc wrote: > >> I would say that pushing cherry-picked commits for review that depend on >> other commits not in the commit's ancestry is just wrong. If you pushed >> this to Try,

Re: Just Autoland It

On 22/01/2016 20:52, Gregory Szorc wrote: I would say that pushing cherry-picked commits for review that depend on other commits not in the commit's ancestry is just wrong. If you pushed this to Try, it would fail. So why are you pushing a "bad" commit/tree for review? If your commits depend on

Re: Just Autoland It

On 2016-01-23 1:20 PM, Gregory Szorc wrote: On Fri, Jan 22, 2016 at 1:45 PM, Boris Zbarsky wrote: On 1/22/16 3:52 PM, Gregory Szorc wrote: I would say that pushing cherry-picked commits for review that depend on other commits not in the commit's ancestry is just wrong. If

Re: Just Autoland It

On Sat, Jan 23, 2016 at 4:01 PM, Eric Rescorla wrote: > > > On Sat, Jan 23, 2016 at 10:20 AM, Gregory Szorc wrote: > >> On Fri, Jan 22, 2016 at 1:45 PM, Boris Zbarsky wrote: >> >> > On 1/22/16 3:52 PM, Gregory Szorc wrote: >> > >> >> I would

Re: Just Autoland It

On 1/23/16 1:20 PM, Gregory Szorc wrote: While MozReview defaults to submitting all pushed commits for review, you can override these defaults to pick a) any single commit b) a range of commits at the bottom c) middle or d) top of the series. OK, but you said people shouldn't be pushing

Re: Just Autoland It

On Sat, Jan 23, 2016 at 10:20 AM, Gregory Szorc wrote: > On Fri, Jan 22, 2016 at 1:45 PM, Boris Zbarsky wrote: > > > On 1/22/16 3:52 PM, Gregory Szorc wrote: > > > >> I would say that pushing cherry-picked commits for review that depend on > >> other commits

Re: Just Autoland It

On Sat, Jan 23, 2016 at 09:33:15PM -0500, Boris Zbarsky wrote: > Sure. And the "r+ with these changes, and feel free to land, but I want to > see the interdiff" mode is supported with Autoland because the interdiff > would be available in mozreview post-facto, as you note. Note that if /other/

Re: Just Autoland It

On 1/23/16 2:41 PM, Ehsan Akhgari wrote: FWIW, option 3 is basically my usual workflow Option 3, or option 2? Just to recap, option 3 is that I write patches for bug A and bug B in that order in my tree (A, then B) and ask for review on both. They are independent. I get review on B first,

Re: Just Autoland It

On Sat, Jan 23, 2016 at 4:42 PM, Gregory Szorc wrote: > On Sat, Jan 23, 2016 at 4:01 PM, Eric Rescorla wrote: > >> >> >> On Sat, Jan 23, 2016 at 10:20 AM, Gregory Szorc wrote: >> >>> On Fri, Jan 22, 2016 at 1:45 PM, Boris Zbarsky

Re: Just Autoland It

On 22/01/16 02:12 AM, Gregory Szorc wrote: On Thu, Jan 21, 2016 at 10:37 PM, Mike Connor wrote: Like Greg, I'm a big fan of reviewer-lands-if-ready. It's a huge simplification of workflow, saves developers time, and lets machines do work instead of humans. That said, I

Re: Just Autoland It

On Fri, Jan 22, 2016 at 11:24 AM, Ehsan Akhgari wrote: > On 2016-01-22 2:10 PM, Gregory Szorc wrote: > >> On Fri, Jan 22, 2016 at 7:30 AM, Boris Zbarsky wrote: >> >> On 1/22/16 10:08 AM, Andrew Halberstadt wrote: >>> >>> In the end, it's on the

Re: Just Autoland It

On 1/22/16 3:52 PM, Gregory Szorc wrote: I would say that pushing cherry-picked commits for review that depend on other commits not in the commit's ancestry is just wrong. If you pushed this to Try, it would fail. So why are you pushing a "bad" commit/tree for review? If your commits depend on

Re: Just Autoland It

This was startling to me at first, but on further reflection, I'd like to caution against reacting for the wrong reasons. One driver for our current workflows is that the patch attached to bugzilla is rarely what actually landed. With things that go through mozreview, that need no longer be

Re: Just Autoland It

I think if we want to experiment in MozReview, we still need to reflect something in Bugzilla that kills the nuance and avoids unexpected landings from folks trying to be helpful. It's not hard or risky to add one additional flag (making review/feedback/land all available) and let MozReview do

Re: Just Autoland It

On 2016-01-22 2:10 PM, Gregory Szorc wrote: On Fri, Jan 22, 2016 at 7:30 AM, Boris Zbarsky wrote: On 1/22/16 10:08 AM, Andrew Halberstadt wrote: In the end, it's on the reviewer. If the patch is complicated, has open dependencies or looks like it might cause problems, as a

Re: Just Autoland It

On Fri, Jan 22, 2016 at 7:30 AM, Boris Zbarsky wrote: > On 1/22/16 10:08 AM, Andrew Halberstadt wrote: > >> In the end, it's on the reviewer. If the patch is complicated, has open >> dependencies or looks like it might cause problems, as a reviewer I'm >> not going to land it

Re: Just Autoland It

On 16-01-22 08:16 AM, Jared Wein wrote: > If a patch only touches JS and CSS, could tryserver use the archive build > implementation to generate faster try builds? > We could do things like that (We can tell test jobs to grab the installer and test packages from different locations). regards,

Re: Just Autoland It

gt; > 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 m

Re: Just Autoland It

On 22/01/2016 6:12 PM, Gregory Szorc wrote: Code review will continue to shift from Bugzilla centric to MozReview centric. And this means that Bugzilla flags mean less and less over time. Perhaps we can solve intent in MozReview without having to change anything in Bugzilla... (Hopefully)

Re: Just Autoland It

checkin? is also used for large multi-patch bugs that land parts at different times. On Fri, Jan 22, 2016 at 3:38 PM, Paolo Amadini wrote: > On 1/22/2016 2:00 PM, Mark Hammond wrote: > >> (Hopefully) related - what exactly is the "checkin?" flag for? >> > > As far as

Re: Just Autoland It

On 22/01/16 12:20 AM, Nicholas Nethercote wrote: On Thu, Jan 21, 2016 at 6:35 PM, Gregory Szorc wrote: 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

Re: Just Autoland It

On 1/22/16 10:08 AM, Andrew Halberstadt wrote: In the end, it's on the reviewer. If the patch is complicated, has open dependencies or looks like it might cause problems, as a reviewer I'm not going to land it no matter what. If it's simple and self-contained, why not? There is no obvious

Re: Just Autoland It

On 22/01/16 10:30 AM, Boris Zbarsky wrote: On 1/22/16 10:08 AM, Andrew Halberstadt wrote: In the end, it's on the reviewer. If the patch is complicated, has open dependencies or looks like it might cause problems, as a reviewer I'm not going to land it no matter what. If it's simple and

Re: Just Autoland It

t; 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,

Re: Just Autoland It

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 enco

Just Autoland It

view was granted. I encourage my fellow reviewers to join me and "just autoland it" when granting review on MozReview. gps ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

gt; > 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 locall

Re: Just Autoland It

> > Both of these behaviours are incompatible with reviewer-initiated landing. > I've fallen on both sides of this particular fence; sometimes I want to fire-and-forget a patch, and sometimes I still want to digest further after getting review (or I know a piece of work is incomplete and further

Re: Just Autoland It

On Jan 21, 2016, at 21:25, Richard Newman wrote: >> Both of these behaviours are incompatible with reviewer-initiated landing. > > I've fallen on both sides of this particular fence; sometimes I want to > fire-and-forget a patch, and sometimes I still want to digest

Re: Just Autoland It

> On Jan 21, 2016, at 21:46, Mike Hommey wrote: > >> On Thu, Jan 21, 2016 at 06:35:08PM -0800, 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

Re: Just Autoland It

On Thu, Jan 21, 2016 at 06:35:08PM -0800, 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

Re: Just Autoland It

h 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

Re: Just Autoland It

Like Greg, I'm a big fan of reviewer-lands-if-ready. It's a huge simplification of workflow, saves developers time, and lets machines do work instead of humans. That said, I don't think we should be surprising people or unilaterally imposing changes to their workflow. The best way to do this is

Re: Just Autoland It

On Thu, Jan 21, 2016 at 10:37 PM, Mike Connor wrote: > Like Greg, I'm a big fan of reviewer-lands-if-ready. It's a huge > simplification of workflow, saves developers time, and lets machines do > work instead of humans. That said, I don't think we should be surprising >