Re: Just Autoland It

[Jan-Ivar Bruaroey]

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
dependencies are still being worked on?  Or, as another example,
what if the patch needs to be checked in only after another patch
(perhaps done by a different author) that is not connected to the
review at hand?


I assume that, if the patches have a dependency on other work,
then that would be explained to the reviewer, so that they can
know how the patch will work.


That's funny.

.: Jan-Ivar :.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Andrew Halberstadt]

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 everyone who works on MozReview, we know the
interface is lacking in areas and features are confusing or non-existent.
We're working on it. We're trying to morph workflows that have been
practiced at Mozilla for over a decade. We're playing a delicate balancing
game between giving familiarity with existing workflows (e.g. Bugzilla
integration) while trying to slowly nudge us towards more "modern" and more
powerful workflows.



Frankly, I'm a little dismayed to hear that you think that one of the goals
of Mozreview
is to modify people's workflows. The primary problem with our existing
review system
isn't that it doesn't involve some more "modern" review idiom (leaving
aside the question
of whether it is in fact more modern), but rather that the UI is bad and
that it's a lot
less powerful than existing review tools, including those that enact
basically the
same design (cf. Rietveld).

Speaking purely for myself, I'd be a lot happier if mozreview involved less
nudging
and morphing and more developing of basic functionality.

-Ekr


Not speaking to review per se, but engineering productivity in general.
The problem is there are so many unique and one-off workflows at Mozilla
that it gets harder and harder to improve "basic functionality" across
all of them. At some point, we hit vastly diminishing returns and get
stretched too thin. We'd love to improve every existing workflow, but
simply don't have the resources to do that.

Instead, we try to make one really nice workflow such that people want
to switch. That being said it's a valid opinion to think that the carrot
isn't sweet enough yet. If that's the case, filing bug reports like gps
mentioned is very helpful to us.

-Andrew



We're constantly surprised by all the one-off workflows and needs people
have and the reactions to a seemingly benign change. It's been a humbling
experience to say the least.

The best venue for reporting bugs, UX paper cuts, and suggest improvements
is Bugzilla. Developer Services :: MozReview. Or hop in #mozreview and chat
with us.

We get a lot of requests for changes that initially seem odd to us. So, if
your bug report could articulate why you want something and how many people
would benefit (e.g. "the layout team all does this"), it would help us
better empathize with your position and would increase the chances of your
request getting prioritized.


On Jan 28, 2016, at 10:14, Eric Rescorla  wrote:


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 jumps to per-file anchors.

Not good enough for review process.

Are you saying you want tabs or something for this (like

splinter uses)? I'd certainly like something less sluggish, but
maybe that's just my browser again.

Yes please.  Having one file on the screen at a time is very
useful.

The next/previous file/comment keyboard shortcuts may be useful in
the meantime.


Unfortunately not.  The intention is that when I scroll down the screen
I'm at the end of *a single file*, and of course up the screen means to

be

up at that same file.  Shortcuts are definitely unhelpful for me.  With

how

revboard works now it's just a mess of all  put together.


I agree with this. As I've mentioned before, NSS uses Rietveld, which
file-by-file, and I find
this much more convenient.

-Ekr



Thanks anyway!

-hb-







https://www.reviewboard.org/docs/manual/2.5/users/reviews/reviewing-diffs/#keyboard-shortcuts

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform




___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Eric Rescorla]

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
>>> 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 working on it. We're trying to morph workflows that have been
>>> practiced at Mozilla for over a decade. We're playing a delicate
>>> balancing
>>> game between giving familiarity with existing workflows (e.g. Bugzilla
>>> integration) while trying to slowly nudge us towards more "modern" and
>>> more
>>> powerful workflows.
>>>
>>
>>
>> Frankly, I'm a little dismayed to hear that you think that one of the
>> goals
>> of Mozreview
>> is to modify people's workflows. The primary problem with our existing
>> review system
>> isn't that it doesn't involve some more "modern" review idiom (leaving
>> aside the question
>> of whether it is in fact more modern), but rather that the UI is bad and
>> that it's a lot
>> less powerful than existing review tools, including those that enact
>> basically the
>> same design (cf. Rietveld).
>>
>> Speaking purely for myself, I'd be a lot happier if mozreview involved
>> less
>> nudging
>> and morphing and more developing of basic functionality.
>>
>> -Ekr
>>
>
> Not speaking to review per se, but engineering productivity in general.
> The problem is there are so many unique and one-off workflows at Mozilla
> that it gets harder and harder to improve "basic functionality" across
> all of them.


Well, the functionality that I hear people discussing and complaining about
with MozReview in this thread seems pretty common to most workflows:

- The ability to review individual files
- The ability to r- not just remove r+
- Concerns about how much context is included in the review.

All of these things are mostly just issues in the Mozreview UI.



> At some point, we hit vastly diminishing returns and get
> stretched too thin. We'd love to improve every existing workflow, but
> simply don't have the resources to do that.
>
> Instead, we try to make one really nice workflow such that people want
> to switch.


I think it's clear from this thread that that has not succeeded.

More generally, I keep seeing comments (especially from GPS) about
trying to push people towards some workflow that's different from the
patch-based workflow that's the modal bugzilla workflow that I suspect
most people now use. That doesn't seem like an especially valuable
goal for this work.

-Ekr

That being said it's a valid opinion to think that the carrot
> isn't sweet enough yet. If that's the case, filing bug reports like gps
> mentioned is very helpful to us.
>
> -Andrew
>
>
>
> We're constantly surprised by all the one-off workflows and needs people
>>> have and the reactions to a seemingly benign change. It's been a humbling
>>> experience to say the least.
>>>
>>> The best venue for reporting bugs, UX paper cuts, and suggest
>>> improvements
>>> is Bugzilla. Developer Services :: MozReview. Or hop in #mozreview and
>>> chat
>>> with us.
>>>
>>> We get a lot of requests for changes that initially seem odd to us. So,
>>> if
>>> your bug report could articulate why you want something and how many
>>> people
>>> would benefit (e.g. "the layout team all does this"), it would help us
>>> better empathize with your position and would increase the chances of
>>> your
>>> request getting prioritized.
>>>
>>> On Jan 28, 2016, at 10:14, Eric Rescorla  wrote:

 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 jumps to per-file anchors.

>>> Not good enough for review process.
>>>
>>> Are you saying you want tabs or something for this (like
>>>
 splinter uses)? I'd certainly like something less sluggish, but
 maybe that's just my browser again.

>>> Yes please.  Having one file on the screen at a time is very
>>> useful.
>>>
>> The next/previous file/comment keyboard shortcuts may be useful in
>> the meantime.
>>
>
> Unfortunately not.  The intention is that when I scroll down the screen
> I'm at the end of *a single file*, and of course up the screen means to
>
 be
>>>
 up at that same file.  Shortcuts are definitely unhelpful for me.  With
>
 how
>>>
 revboard works now it's just a mess of all  put together.
>

 I agree with this. As I've 

Re: Just Autoland It

[Mark Côté]

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 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 working on it. We're trying to morph workflows that have been
 practiced at Mozilla for over a decade. We're playing a delicate
 balancing
 game between giving familiarity with existing workflows (e.g. Bugzilla
 integration) while trying to slowly nudge us towards more "modern" and
 more
 powerful workflows.

>>>
>>>
>>> Frankly, I'm a little dismayed to hear that you think that one of the
>>> goals
>>> of Mozreview
>>> is to modify people's workflows. The primary problem with our existing
>>> review system
>>> isn't that it doesn't involve some more "modern" review idiom (leaving
>>> aside the question
>>> of whether it is in fact more modern), but rather that the UI is bad and
>>> that it's a lot
>>> less powerful than existing review tools, including those that enact
>>> basically the
>>> same design (cf. Rietveld).
>>>
>>> Speaking purely for myself, I'd be a lot happier if mozreview involved
>>> less
>>> nudging
>>> and morphing and more developing of basic functionality.
>>>
>>> -Ekr
>>>
>>
>> Not speaking to review per se, but engineering productivity in general.
>> The problem is there are so many unique and one-off workflows at Mozilla
>> that it gets harder and harder to improve "basic functionality" across
>> all of them.
> 
> 
> Well, the functionality that I hear people discussing and complaining about
> with MozReview in this thread seems pretty common to most workflows:
> 
> - The ability to review individual files
> - The ability to r- not just remove r+
> - Concerns about how much context is included in the review.
> 
> All of these things are mostly just issues in the Mozreview UI.

Yes absolutely, many of the UI problems are not really about workflow.

>> At some point, we hit vastly diminishing returns and get
>> stretched too thin. We'd love to improve every existing workflow, but
>> simply don't have the resources to do that.
>>
>> Instead, we try to make one really nice workflow such that people want
>> to switch.
> 
> 
> I think it's clear from this thread that that has not succeeded.
> 
> More generally, I keep seeing comments (especially from GPS) about
> trying to push people towards some workflow that's different from the
> patch-based workflow that's the modal bugzilla workflow that I suspect
> most people now use. That doesn't seem like an especially valuable
> goal for this work.

So, the only workflow that's really baked into MozReview, which I've
talked about before, is the idea of microcommits: that authors should be
splitting their work up into small, atomic commits, and updating them as
they get reviews.  I actually argue that this isn't even a workflow per
se.  It's more of a design principle of how software should be built.
It's my understanding that the senior engineers at Mozilla agree with
this principle, which is why supporting microcommits was a goal from the
beginning, back when Taras and Ehsan and a couple others agreed that we
needed a new tool and starting collecting requirements.

The downside is that no other code-review tool out there really does
this approach well, so we had to pick one (that could support both hg
and git) and build on it.  It's been (clearly) tough to get this right,
with all the confusion over squashed diffs and review summaries and
such.  A lot of the feedback here has been really constructive, though,
so one way or another we're going to get to something that will make
people a lot happier.  I'll be sure to ask for feedback on our design
ideas as we proceed.

Mark

> 
> -Ekr
> 
> That being said it's a valid opinion to think that the carrot
>> isn't sweet enough yet. If that's the case, filing bug reports like gps
>> mentioned is very helpful to us.
>>
>> -Andrew
>>
>>
>>
>> We're constantly surprised by all the one-off workflows and needs people
 have and the reactions to a seemingly benign change. It's been a humbling
 experience to say the least.

 The best venue for reporting bugs, UX paper cuts, and suggest
 improvements
 is Bugzilla. Developer Services :: MozReview. Or hop in #mozreview and
 chat
 with us.

 We get a lot of requests for changes that initially seem odd to us. So,
 if
 your bug report could articulate why you want something and how many
 people
 would benefit (e.g. "the layout team all does this"), it would help us
 better empathize with your 

Re: Just Autoland It

[Dave Townsend]

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:
>>>
>>> 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 working on it. We're trying to morph workflows that have been
 practiced at Mozilla for over a decade. We're playing a delicate
 balancing
 game between giving familiarity with existing workflows (e.g. Bugzilla
 integration) while trying to slowly nudge us towards more "modern" and
 more
 powerful workflows.

>>>
>>>
>>> Frankly, I'm a little dismayed to hear that you think that one of the
>>> goals
>>> of Mozreview
>>> is to modify people's workflows. The primary problem with our existing
>>> review system
>>> isn't that it doesn't involve some more "modern" review idiom (leaving
>>> aside the question
>>> of whether it is in fact more modern), but rather that the UI is bad and
>>> that it's a lot
>>> less powerful than existing review tools, including those that enact
>>> basically the
>>> same design (cf. Rietveld).
>>>
>>> Speaking purely for myself, I'd be a lot happier if mozreview involved
>>> less
>>> nudging
>>> and morphing and more developing of basic functionality.
>>>
>>> -Ekr
>>>
>>
>> Not speaking to review per se, but engineering productivity in general.
>> The problem is there are so many unique and one-off workflows at Mozilla
>> that it gets harder and harder to improve "basic functionality" across
>> all of them.
>
>
> Well, the functionality that I hear people discussing and complaining about
> with MozReview in this thread seems pretty common to most workflows:
>
> - The ability to review individual files
> - The ability to r- not just remove r+
> - Concerns about how much context is included in the review.
>
> All of these things are mostly just issues in the Mozreview UI.
>
>
>
>> At some point, we hit vastly diminishing returns and get
>> stretched too thin. We'd love to improve every existing workflow, but
>> simply don't have the resources to do that.
>>
>> Instead, we try to make one really nice workflow such that people want
>> to switch.
>
>
> I think it's clear from this thread that that has not succeeded.

No-one has claimed that the work to make that nice workflow is done yet.

> More generally, I keep seeing comments (especially from GPS) about
> trying to push people towards some workflow that's different from the
> patch-based workflow that's the modal bugzilla workflow that I suspect
> most people now use. That doesn't seem like an especially valuable
> goal for this work.

I disagree. The workflow everyone is used to is the workflow that
bugzilla essentially forces on us. Given the choice is it the workflow
everyone would use? For some maybe but given how many people I see
using mozreview now (even though it still has its problems) when the
old workflow is still perfectly usable suggests that this new workflow
is preferable to many and so this work is valuable. Some issues aside
I find it much more more straightforward to use.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Boris Zbarsky]

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 the 
review requestee to use MozReview to some extent.


That said, I've seen some review requestees blanket unset review flags 
on any MozReview request, in an attempt to get review requests they 
actually want to deal with.


Note that either way there is bad feeling here as a result: either the 
review requestee feels like the review requester is forcing them into a 
bad workflow, or the review requester feels like the review requestee is 
forcing them into a bad workflow.


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 feel put upon.



I see teams using GitHub pull requests, Reviewable, Opera
Critic, Reitveld, and Splinter in parallel with MozReview.


This works well in reasonably isolated team.

As you might have noticed, the people who seem to be complaining about 
MozReview the most are the ones who (1) do a lot of reviews and (2) do 
them across a wide swath of the codebase.  Item (1) means they already 
have a well-worked out review workflow, and MozReview would initially be 
slower and more annoying even if the UI were perfect.  It also means 
that they represent a disproportionate portion of all the reviews 
done.[1]  Item (2) means they can't just ask all the potential review 
requesters to not use MozReview, because asking a review requester to 
remember the preferred review system on a per-requestee basis is nuts.


So there's some combination of "this is different" and "this is worse" 
here, and as usual disentangling them is a bit hard.  As you say, 
concrete bug reports are the way to go.



are - this is where filed bugs come in[4]).


As a personal anecdote, in the last 5 months I've reported what looks 
like 7 bugs that significantly affect my ability to use MozReview 
effectively, and there is one more that I would have reported were it 
not reported already.  Of these, two have been addressed.  I'm not 
blaming the MozReview team here; they have limited resources, other 
commitments, and I can't be the only person reporting bugs!  But, again, 
we end up with a dynamic where people are effectively being told "use 
it; if you run into issues, file bugs and keep using it", but the bugs 
take a long time to address and in the meantime using it is frustrating. 
 Again, not the fault of the MozReview team, but that doesn't make the 
frustration less.


I agree that if people were not feeling forced into using it and could 
just report the bugs and then wait for them to be fixed while using 
something else, they would be a lot less worked up around this.



Try MozReview. If it doesn't work for you, maybe use something else, and
try it again in a few months once the team has had a chance to address
the bugs you hopefully filed.


This is reasonable advice for review requesters, but not for review 
requestees, per above.  :(


The bad news is that I don't know what advice to give requestees, 
including myself, here.  What I've been doing personally is just the 
"stop, take a deep breath, relax" routine whenever I get a mozreview 
request, but the fact that the routine is necessary is a bummer...



Again, I mean no disrespect here. I just want to gently suggest that we
exercise some restraint while hammering on the MozReview team


Fully agreed, and apologies if I've said things that came across that way.

-Boris

[1] To quantify this, if smaug is doing 6 reviews (bugs, not changesets; 
more changesets) per calendar day, then a set of UI annoyances that adds 
5 minutes per review is a big deal: that's 3.5 hours out of his work 
week.  It's obviously a less big deal for someone doing one review every 
three days.  I did not make up the numbers for smaug, and we have plenty 
of people who average one review every few days, for sure...


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Mike Conley]

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
preferences is extremely difficult - especially when (it seems to me)
most of the feedback is via negativa[2]. Having worked on front-end for
Firefox for a number of years, I believe I can speak with a certain
degree of authority about how difficult it is to satisfy users with UI
and workflow. It's a hard problem. Perhaps this can be alleviated by
putting some UX folks on the problem[3].

The good news is that this is Mozilla, which means that there are a
variety of ways of doing things, and nobody is really forcing you to use
MozReview. I see teams using GitHub pull requests, Reviewable, Opera
Critic, Reitveld, and Splinter in parallel with MozReview. Each have
their strengths. The thing with MozReview, however, is that we can more
easily mold it to suit our needs (once we can settle on what those needs
are - this is where filed bugs come in[4]).

Try MozReview. If it doesn't work for you, maybe use something else, and
try it again in a few months once the team has had a chance to address
the bugs you hopefully filed.

Again, I mean no disrespect here. I just want to gently suggest that we
exercise some restraint while hammering on the MozReview team, who are
trying to accomplish something extremely difficult.

-Mike

[1]: I'm not on the MozReview team, but I've been involved peripherally
from the outset, and I'm also a Review Board contributor.
[2]: This has different meanings for different people, but I'm using it
in the stage-theatre sense, where a director will not tell you what they
want, but every time you come out with something, they'll tell you what
you need to stop doing without telling you what you need to do. As an
actor, I always found this directing technique to be extremely frustrating.
[3]: There have been some casual consultations with folks like bwinton,
but I think that's about as far as it goes.
[4]: The MozReview team triages these biweekly (I believe), so they'll
get read and considered.

On 29/01/2016 9:22 AM, Andrew Halberstadt 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
>>> 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 working on it. We're trying to morph workflows that have been
>>> practiced at Mozilla for over a decade. We're playing a delicate
>>> balancing
>>> game between giving familiarity with existing workflows (e.g. Bugzilla
>>> integration) while trying to slowly nudge us towards more "modern"
>>> and more
>>> powerful workflows.
>>
>>
>> Frankly, I'm a little dismayed to hear that you think that one of the
>> goals
>> of Mozreview
>> is to modify people's workflows. The primary problem with our existing
>> review system
>> isn't that it doesn't involve some more "modern" review idiom (leaving
>> aside the question
>> of whether it is in fact more modern), but rather that the UI is bad and
>> that it's a lot
>> less powerful than existing review tools, including those that enact
>> basically the
>> same design (cf. Rietveld).
>>
>> Speaking purely for myself, I'd be a lot happier if mozreview involved
>> less
>> nudging
>> and morphing and more developing of basic functionality.
>>
>> -Ekr
> 
> Not speaking to review per se, but engineering productivity in general.
> The problem is there are so many unique and one-off workflows at Mozilla
> that it gets harder and harder to improve "basic functionality" across
> all of them. At some point, we hit vastly diminishing returns and get
> stretched too thin. We'd love to improve every existing workflow, but
> simply don't have the resources to do that.
> 
> Instead, we try to make one really nice workflow such that people want
> to switch. That being said it's a valid opinion to think that the carrot
> isn't sweet enough yet. If that's the case, filing bug reports like gps
> mentioned is very helpful to us.
> 
> -Andrew
> 
> 
>>> We're constantly surprised by all the one-off workflows and needs people
>>> have and the reactions to a seemingly benign change. It's been a
>>> humbling
>>> experience to say the least.
>>>
>>> The best venue for reporting bugs, UX paper cuts, and suggest
>>> improvements
>>> is Bugzilla. Developer Services :: MozReview. Or hop in #mozreview
>>> and chat
>>> with us.
>>>
>>> We get a lot of requests for changes that initially seem odd to us.
>>> So, if
>>> your bug report could articulate why you want something and how many
>>> people
>>> would benefit 

Re: Just Autoland It

[Mike Conley]

>> 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 feel put upon.

That's a good point. From my observations and interactions with the
MozReview team over the months, I believe a great deal of the initial
effort has been to lower the ramp for the requestors, as opposed to the
requestees. The fact that we're seeing so many review requests go
through MozReview might be a sign of success in terms of that work.

Anecdotal evidence, along with the sentiments in threads like this (and
elsewhere) suggests that it would now probably be worth shifting the
emphasis on lowering the ramp for the requestees.

I do know that this sort of work has started to get attention from the
MozReview team. smaug, as the most prolific reviewer[1] recently filed a
bug about interdiffs being busted in particular cases, and it's getting
serious attention from the team.

So, I guess we need to show some empathy for the requestees as well. I
think[2] it's not always clear what the most critical thing to fix for
our requestees is. If folks have some wireframes, ascii diagrams,
napkins sketches, I'm certain the MozReview team would find that valuable.

[1]: All hail the mighty smaug!
[2]: Beyond obvious things like, "interdiffs are sometimes wrong", which
is clearly something to fix ASAP

On 29/01/2016 11:18 AM, Boris Zbarsky wrote:
> 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 the
> review requestee to use MozReview to some extent.
> 
> That said, I've seen some review requestees blanket unset review flags
> on any MozReview request, in an attempt to get review requests they
> actually want to deal with.
> 
> Note that either way there is bad feeling here as a result: either the
> review requestee feels like the review requester is forcing them into a
> bad workflow, or the review requester feels like the review requestee is
> forcing them into a bad workflow.
> 
> 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 feel put upon.
> 
>> I see teams using GitHub pull requests, Reviewable, Opera
>> Critic, Reitveld, and Splinter in parallel with MozReview.
> 
> This works well in reasonably isolated team.
> 
> As you might have noticed, the people who seem to be complaining about
> MozReview the most are the ones who (1) do a lot of reviews and (2) do
> them across a wide swath of the codebase.  Item (1) means they already
> have a well-worked out review workflow, and MozReview would initially be
> slower and more annoying even if the UI were perfect.  It also means
> that they represent a disproportionate portion of all the reviews
> done.[1]  Item (2) means they can't just ask all the potential review
> requesters to not use MozReview, because asking a review requester to
> remember the preferred review system on a per-requestee basis is nuts.
> 
> So there's some combination of "this is different" and "this is worse"
> here, and as usual disentangling them is a bit hard.  As you say,
> concrete bug reports are the way to go.
> 
>> are - this is where filed bugs come in[4]).
> 
> As a personal anecdote, in the last 5 months I've reported what looks
> like 7 bugs that significantly affect my ability to use MozReview
> effectively, and there is one more that I would have reported were it
> not reported already.  Of these, two have been addressed.  I'm not
> blaming the MozReview team here; they have limited resources, other
> commitments, and I can't be the only person reporting bugs!  But, again,
> we end up with a dynamic where people are effectively being told "use
> it; if you run into issues, file bugs and keep using it", but the bugs
> take a long time to address and in the meantime using it is frustrating.
>  Again, not the fault of the MozReview team, but that doesn't make the
> frustration less.
> 
> I agree that if people were not feeling forced into using it and could
> just report the bugs and then wait for them to be fixed while using
> something else, they would be a lot less worked up around this.
> 
>> Try MozReview. If it doesn't work for you, maybe use something else, and
>> try it again in a few months once the team has had a chance to address
>> the bugs you hopefully filed.
> 
> This is reasonable advice for review requesters, but not for review
> requestees, per above.  :(
> 
> 

Re: Just Autoland It

[Boris Zbarsky]

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 might not be actively using MozReview yet are interested, and 
want to make sure it will be able to do what they want to do.  The fact 
that they're not actively using it or involved in its development means 
they don't quite have a clear grasp on what the end goal is in terms of 
UX.  I know I don't.  So they're not communicating their concerns very 
clearly, because their concerns are unclear to themselves in large part.


At least for some people, I think this is compounded by a general sense 
of frustration with MozReview from the requestee point of view, which is 
bleeding over in an undesirable way...


-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Eric Rescorla]

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
> > patch-based workflow that's the modal bugzilla workflow that I suspect
> > most people now use. That doesn't seem like an especially valuable
> > goal for this work.
>
> I disagree. The workflow everyone is used to is the workflow that
> bugzilla essentially forces on us. Given the choice is it the workflow
> everyone would use? For some maybe but given how many people I see
> using mozreview now (even though it still has its problems) when the
> old workflow is still perfectly usable suggests that this new workflow
> is preferable to many and so this work is valuable. Some issues aside
> I find it much more more straightforward to use.
>

It's important to separate issues of workflow from issues of UI. I still
prefer
Mozreview for significant-sized Gecko patches despite the general clunkiness
of the workflow because ReviewBoard is so much better at showing what
changed than splinter is. But when I have a choice (e.g., for isolated
projects like NSS) I use Rietveld and when it's small I use Splinter,
because
I find them generally easier to work with.

-Ekr
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Steve Fink]

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 about MozReview:
people just feel put upon.

That's a good point. From my observations and interactions with the
MozReview team over the months, I believe a great deal of the initial
effort has been to lower the ramp for the requestors, as opposed to the
requestees. The fact that we're seeing so many review requests go
through MozReview might be a sign of success in terms of that work.

Anecdotal evidence, along with the sentiments in threads like this (and
elsewhere) suggests that it would now probably be worth shifting the
emphasis on lowering the ramp for the requestees.


Personally, I think the main issue is that people can't "get" the data 
model by looking at the interface, at least not until using it for quite 
a while. And that means that everyone is coming in with different 
notions of what's going on, and complaining about the things that are 
broken *in the context of what their misperceptions* of what is 
happening or should be happening. In short, many of the complaints are 
mistargeted, because people don't understand what the tool actually is. 
And saying "just file specific bugs" isn't always right, because many 
times the user wouldn't have run into the problem in the first place had 
they better understood what was going on.


A random example: the last time I used MR, I wanted to change the 
reviewer on one of the commits. But the reviewer wasn't editable, and I 
didn't know why. The whole list of commits and reviewers was right there 
on the page; why couldn't I edit them? What kind of authoritarian 
overopinionated thing is this, anyway, that doesn't let me change my 
reviewers after I initially select them? I made sure I was logged in, even.


The problem was that I was on the display for a particular commit 
(probably the commit for which I wanted to change the reviewer, I'd 
imagine.) The list of commits at the top is just a read-only summary of 
what you're working on, presumably there to help you stay oriented or 
something. It doesn't make a whole lot of sense for all of the *other* 
commits' reviewers to be editable when you're in a single-commit view. I 
mean, they could be. Or there could be a separate place in the UI where 
you could change the reviewer for the current commit. Or just that one 
commit's reviewer could be editable. I could file a bug requesting any 
of those.


But that's wrong. The underlying problem is that the interface does not 
convey the distinction between single-commit vs whole-review views. The 
right fix is to remove, collapse, or reformat the top summary section to 
deemphasize it and make it very very clear when you're looking at one 
commit vs the overall thing. I did not realize that until much later, 
when I was writing my earlier rant in this thread.


So, IMHO, you're going to get a lot of junk bugs with the current status 
quo -- as in, you could implement the requests and things would be 
incrementally better, but if the interface communicated what's going on 
better then the bug filer wouldn't have gotten stuck in the first place. 
Whatever workflow the filer was trying to follow might be slightly more 
streamlined, but the benefit is less than what could be accomplished if 
users were a little more on the same page [no pun intended].


I hope I'm not seen as beating on the MR team. Despite being a light 
user, I have communicated a bunch of feedback and found them to be 
responsive (and friendly). It helps that I find our current set of 
bugzilla-based workflows to be archaic and arcane, and for MR to be the 
right general structure for what would be better. But I'm going to 
repeat my toplevel beefs with MR as it stands:


- tab abuse (people get lost and confused)
- the data model is nonobvious and difficult to discover (people get 
lost and confused)
- needs to do better at facilitating communication between the author 
and the reviewers


I can file bugs for these, but they're not directly actionable. I could 
file actionable suggestions, but I would hope that people closer to MR 
and/or are more UX-y than me would be able to accomplish these goals 
better than I could[1].


Steve

[1] It turns out that I do have some UX background, but I suck at design 
and prefer to practice my whining skills.


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Gregory Szorc]

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 working on 
it. We're trying to morph workflows that have been practiced at Mozilla for 
over a decade. We're playing a delicate balancing game between giving 
familiarity with existing workflows (e.g. Bugzilla integration) while trying to 
slowly nudge us towards more "modern" and more powerful workflows. We're 
constantly surprised by all the one-off workflows and needs people have and the 
reactions to a seemingly benign change. It's been a humbling experience to say 
the least.

The best venue for reporting bugs, UX paper cuts, and suggest improvements is 
Bugzilla. Developer Services :: MozReview. Or hop in #mozreview and chat with 
us.

We get a lot of requests for changes that initially seem odd to us. So, if your 
bug report could articulate why you want something and how many people would 
benefit (e.g. "the layout team all does this"), it would help us better 
empathize with your position and would increase the chances of your request 
getting prioritized.

> On Jan 28, 2016, at 10:14, Eric Rescorla  wrote:
> 
>> 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 jumps to per-file anchors.
 Not good enough for review process.
 
 Are you saying you want tabs or something for this (like
> splinter uses)? I'd certainly like something less sluggish, but
> maybe that's just my browser again.
 Yes please.  Having one file on the screen at a time is very
 useful.
>>> The next/previous file/comment keyboard shortcuts may be useful in
>>> the meantime.
>> 
>> Unfortunately not.  The intention is that when I scroll down the screen
>> I'm at the end of *a single file*, and of course up the screen means to be
>> up at that same file.  Shortcuts are definitely unhelpful for me.  With how
>> revboard works now it's just a mess of all  put together.
> 
> I agree with this. As I've mentioned before, NSS uses Rietveld, which
> file-by-file, and I find
> this much more convenient.
> 
> -Ekr
> 
> 
>> Thanks anyway!
>> 
>> -hb-
>> 
>> 
>> 
>>> 
>>> https://www.reviewboard.org/docs/manual/2.5/users/reviews/reviewing-diffs/#keyboard-shortcuts
>>> ___
>>> dev-platform mailing list
>>> dev-platform@lists.mozilla.org
>>> https://lists.mozilla.org/listinfo/dev-platform
>> ___
>> dev-platform mailing list
>> dev-platform@lists.mozilla.org
>> https://lists.mozilla.org/listinfo/dev-platform
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Eric Rescorla]

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
> interface is lacking in areas and features are confusing or non-existent.
> We're working on it. We're trying to morph workflows that have been
> practiced at Mozilla for over a decade. We're playing a delicate balancing
> game between giving familiarity with existing workflows (e.g. Bugzilla
> integration) while trying to slowly nudge us towards more "modern" and more
> powerful workflows.


Frankly, I'm a little dismayed to hear that you think that one of the goals
of Mozreview
is to modify people's workflows. The primary problem with our existing
review system
isn't that it doesn't involve some more "modern" review idiom (leaving
aside the question
of whether it is in fact more modern), but rather that the UI is bad and
that it's a lot
less powerful than existing review tools, including those that enact
basically the
same design (cf. Rietveld).

Speaking purely for myself, I'd be a lot happier if mozreview involved less
nudging
and morphing and more developing of basic functionality.

-Ekr



> We're constantly surprised by all the one-off workflows and needs people
> have and the reactions to a seemingly benign change. It's been a humbling
> experience to say the least.
>
> The best venue for reporting bugs, UX paper cuts, and suggest improvements
> is Bugzilla. Developer Services :: MozReview. Or hop in #mozreview and chat
> with us.
>
> We get a lot of requests for changes that initially seem odd to us. So, if
> your bug report could articulate why you want something and how many people
> would benefit (e.g. "the layout team all does this"), it would help us
> better empathize with your position and would increase the chances of your
> request getting prioritized.
>
> > On Jan 28, 2016, at 10:14, Eric Rescorla  wrote:
> >
> >> 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 jumps to per-file anchors.
>  Not good enough for review process.
> 
>  Are you saying you want tabs or something for this (like
> > splinter uses)? I'd certainly like something less sluggish, but
> > maybe that's just my browser again.
>  Yes please.  Having one file on the screen at a time is very
>  useful.
> >>> The next/previous file/comment keyboard shortcuts may be useful in
> >>> the meantime.
> >>
> >> Unfortunately not.  The intention is that when I scroll down the screen
> >> I'm at the end of *a single file*, and of course up the screen means to
> be
> >> up at that same file.  Shortcuts are definitely unhelpful for me.  With
> how
> >> revboard works now it's just a mess of all  put together.
> >
> > I agree with this. As I've mentioned before, NSS uses Rietveld, which
> > file-by-file, and I find
> > this much more convenient.
> >
> > -Ekr
> >
> >
> >> Thanks anyway!
> >>
> >> -hb-
> >>
> >>
> >>
> >>>
> >>>
> https://www.reviewboard.org/docs/manual/2.5/users/reviews/reviewing-diffs/#keyboard-shortcuts
> >>> ___
> >>> dev-platform mailing list
> >>> dev-platform@lists.mozilla.org
> >>> https://lists.mozilla.org/listinfo/dev-platform
> >> ___
> >> dev-platform mailing list
> >> dev-platform@lists.mozilla.org
> >> https://lists.mozilla.org/listinfo/dev-platform
> > ___
> > dev-platform mailing list
> > dev-platform@lists.mozilla.org
> > https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Honza Bambas]

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 something for this (like
splinter uses)? I'd certainly like something less sluggish, but
maybe that's just my browser again.

Yes please.  Having one file on the screen at a time is very
useful.

The next/previous file/comment keyboard shortcuts may be useful in
the meantime.


Unfortunately not.  The intention is that when I scroll down the screen 
I'm at the end of *a single file*, and of course up the screen means to 
be up at that same file.  Shortcuts are definitely unhelpful for me.  
With how revboard works now it's just a mess of all  put together.


Thanks anyway!

-hb-



https://www.reviewboard.org/docs/manual/2.5/users/reviews/reviewing-diffs/#keyboard-shortcuts
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform



___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Eric Rescorla]

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 jumps to per-file anchors.

>>> Not good enough for review process.
>>>
>>> Are you saying you want tabs or something for this (like
 splinter uses)? I'd certainly like something less sluggish, but
 maybe that's just my browser again.

>>> Yes please.  Having one file on the screen at a time is very
>>> useful.
>>>
>> The next/previous file/comment keyboard shortcuts may be useful in
>> the meantime.
>>
>
> Unfortunately not.  The intention is that when I scroll down the screen
> I'm at the end of *a single file*, and of course up the screen means to be
> up at that same file.  Shortcuts are definitely unhelpful for me.  With how
> revboard works now it's just a mess of all  put together.
>

I agree with this. As I've mentioned before, NSS uses Rietveld, which
file-by-file, and I find
this much more convenient.

-Ekr


> Thanks anyway!
>
> -hb-
>
>
>
>>
>> https://www.reviewboard.org/docs/manual/2.5/users/reviews/reviewing-diffs/#keyboard-shortcuts
>> ___
>> dev-platform mailing list
>> dev-platform@lists.mozilla.org
>> https://lists.mozilla.org/listinfo/dev-platform
>>
>>
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

MozReview interdiffs, was Re: Just Autoland It

[Steve Fink]

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 kind of visual cues.

Ah, that's unfortunate.

I agree that this is a hard problem, though (interdiff across
rebases). I guess that does mean that you can't really use the
final attached thing for the "I want to see the interdiff" use
case; need to push something to MozReview before rebasing to
address that.

Yes, from the reviewing efficiency perspective, it is best to push
MozReview updates for re-review on the same base revision.
i.e. separate from the rebase/landing process.


That may be difficult to do in some cases, though it does seem useful to 
keep in mind. If you happen to be using obsolescence markers, is this 
fixable? As in, if you have a patch P based on upstream U:


U -> P

that you then amend

U -> P'
U -> [P] => P' # double arrow => shows successor relationship, 
[brackets] show obsolete revs


and before re-uploading, rebase onto a new U'

U -> [P']
U -> [P] => [P']
[P'] => P''
U -> U' -> P''

then the interdiff you want is between P and P', which you can get by 
looking at the re-uploaded patch (P'') and finding a precursor with a 
parent rev matching the parent rev (U) of the previously uploaded patch (P).


Except you might have done it the other way, by rebasing first, in which 
case you would have


U -> [P] => [P']
U -> U' -> [P']
[P'] => P''
U' -> P''

in which case you want the interdiff between P' and P''. I think there 
you need to categorize successors as due to rebases vs due to changes, 
and you can detect a rebase because the parent rev is different. So in 
general, you ought to be able to look through the precursor chain to 
find a non-rebase precursor, and use that as your interdiff? In this 
case, the precursor of P'' is P', and parent(P'')=U' is parent(P') is 
also U', so you use that diff. In the previous example, parent(P'')=U' 
is not parent(P'), so that's a rebase, so then you ignore that change 
and look at P' vs P. Their parents are the same, so you use them. And 
last, if you re-upload the same patch, just rebased, then it would get 
back to your original patch before finding a pair with the same parent, 
and so the interdiff is empty.


I've *almost* managed to convince myself that I'm not completely crazy 
with the above algorithm; have I convinced anyone else? Or maybe it's 
just a well known fact that if you have precursor/successor relations, 
then interdiffs are "solved"? It's never going to be 100% pretty, 
because you may end up doing violence to a patch when resolving merge 
conflicts from rebasing. Perhaps MR should show all precursor diffs, but 
label the rebase ones as such?


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Karl Tomlinson]

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
>> splinter uses)? I'd certainly like something less sluggish, but
>> maybe that's just my browser again.
>
> Yes please.  Having one file on the screen at a time is very
> useful.

The next/previous file/comment keyboard shortcuts may be useful in
the meantime.

https://www.reviewboard.org/docs/manual/2.5/users/reviews/reviewing-diffs/#keyboard-shortcuts
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Karl Tomlinson]

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 unfortunate.
>
> I agree that this is a hard problem, though (interdiff across
> rebases). I guess that does mean that you can't really use the
> final attached thing for the "I want to see the interdiff" use
> case; need to push something to MozReview before rebasing to
> address that.

Yes, from the reviewing efficiency perspective, it is best to push
MozReview updates for re-review on the same base revision.
i.e. separate from the rebase/landing process.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Karl Tomlinson]

>> 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 to make it show more by hovering the diff part for a
>> >>little while, it's not really great, as it doesn't actually show a diff
>> >>view like the diff pane does, and switching to the diff pane a) is slow
>> >>for large diffs and b) has an entirely different comment UX that doesn't
>> >>seem really great either.
>> >>
>> >
>> >Indeed. It would be great if it would just include 5-8 lines of context by
>> >default.

> On Mon, Jan 25, 2016 at 06:15:10PM -0500, Andrew Halberstadt wrote:
>> It's not terribly obvious, but instead of clicking on a line number you can
>> click and drag on the numbers to set the exact amount of context you want.

Mike Hommey writes:

> Which is something for the person writing the comment to do.

Yes, please.

> Also, even when they do that, most of the time, that won't
> contain the surrounding code.

AFAICT it is a diff view, and, as you say, hovering provides
access to the rest of the function/file, but the animation is
quite slow.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Karl Tomlinson]

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, as another example,
> what if the patch needs to be checked in only after another patch
> (perhaps done by a different author) that is not connected to the
> review at hand?

I assume that, if the patches have a dependency on other work,
then that would be explained to the reviewer, so that they can
know how the patch will work.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Karl Tomlinson]

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 sent to bugzilla lack context (but I could be misremembering).

I think you are remembering splinter here.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[cbook]

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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Boris Zbarsky]

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.


Not just.  For example, there is no way to "r-" in mozreview.  You can 
only "r+" or remove the review request, in Bugzilla terms.


-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Benjamin Smedberg]

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
category of documentation that's not written yet.


Not just.  For example, there is no way to "r-" in mozreview.  You can 
only "r+" or remove the review request, in Bugzilla terms.


There is a pattern that some teams have been using where they never mark 
"r-" on a change. I think the rationale for this is that it feels 
negative and discouraging to receive the "review not granted" email, 
especially for new contributors. Instead, they will either clear the 
review or mark an f+ and ask for a new patch.


I don't like this practice: I encourage people to use the r- flag. It's 
important to make clear in bugzilla that something has already been 
reviewed and that the result is that something is not ready. Without r- 
or something very similar, it's difficult to distinguish between various 
important cases:


 * I'm too busy/not the right person to review this (clear the review
   or redirect it)
 * I started the review and have a question (leave the r? flag, add a
   NEEDINFO)
 * This isn't good enough (r-)
 * I've looked this over and it's ok in general, but it still needs a
   detailed code review: mark f+, redirect the r? flag as appropriate

In addition, I've seen several contributors become confused receiving an 
f+ and not realizing that what it really meant is "not good enough, 
please make changes".


Our reviewboard tooling should support explicit r- and not just clearing 
review flags.


--BDS

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Mike Conley]

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:
> 
> 
> 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
>>> category of documentation that's not written yet.
>>
>> Not just.  For example, there is no way to "r-" in mozreview.  You can
>> only "r+" or remove the review request, in Bugzilla terms.
> 
> There is a pattern that some teams have been using where they never mark
> "r-" on a change. I think the rationale for this is that it feels
> negative and discouraging to receive the "review not granted" email,
> especially for new contributors. Instead, they will either clear the
> review or mark an f+ and ask for a new patch.
> 
> I don't like this practice: I encourage people to use the r- flag. It's
> important to make clear in bugzilla that something has already been
> reviewed and that the result is that something is not ready. Without r-
> or something very similar, it's difficult to distinguish between various
> important cases:
> 
>  * I'm too busy/not the right person to review this (clear the review
>or redirect it)
>  * I started the review and have a question (leave the r? flag, add a
>NEEDINFO)
>  * This isn't good enough (r-)
>  * I've looked this over and it's ok in general, but it still needs a
>detailed code review: mark f+, redirect the r? flag as appropriate
> 
> In addition, I've seen several contributors become confused receiving an
> f+ and not realizing that what it really meant is "not good enough,
> please make changes".
> 
> Our reviewboard tooling should support explicit r- and not just clearing
> review flags.
> 
> --BDS
> 
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Honza Bambas]

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 reviewboard to get there. Which might just be a
category of documentation that's not written yet.


Not just.  For example, there is no way to "r-" in mozreview. You can 
only "r+" or remove the review request, in Bugzilla terms.


There is a pattern that some teams have been using where they never 
mark "r-" on a change. I think the rationale for this is that it feels 
negative and discouraging to receive the "review not granted" email, 
especially for new contributors. Instead, they will either clear the 
review or mark an f+ and ask for a new patch.


I don't like this practice: I encourage people to use the r- flag. 
It's important to make clear in bugzilla that something has already 
been reviewed and that the result is that something is not ready. 
Without r- or something very similar, it's difficult to distinguish 
between various important cases:


 * I'm too busy/not the right person to review this (clear the review
   or redirect it)
 * I started the review and have a question (leave the r? flag, add a
   NEEDINFO)
 * This isn't good enough (r-)
 * I've looked this over and it's ok in general, but it still needs a
   detailed code review: mark f+, redirect the r? flag as appropriate

In addition, I've seen several contributors become confused receiving 
an f+ and not realizing that what it really meant is "not good enough, 
please make changes".


Our reviewboard tooling should support explicit r- and not just 
clearing review flags.


+1 and I also like the feature of bugzilla UI to allow clicking the r- 
flag by the patch to jump to the actual feedback comment (that e.g. I 
gave two weeks ago and don't remember).  It's good to set at least some 
flag on a patch when review/feedback is done with whatever result.


-hb-



--BDS

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform



___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Axel Hecht]

Piling on:

I'm using mozreview mostly as an occasional patch author:

Plus, I can schedule a try build. Minus, I need to bother the reviewer 
with a published request in order to do so. Resorted to add yet another 
hg extension to my local .hg/hgrc.


My most frequent concern is that bugzilla and mozreview use jargon and 
UX flows that have nothing in common. I don't think that either are good 
or better in their own right, too. And the mapping of one to the other 
isn't documented. "I want to cancel a review, or r-" doc is non-existent 
to hard-to-find. I just randomly click buttons.


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. Why I consider that to 
be a problem is gonna be in a separate reply to a different post on this 
thread.


Axel

On 25/01/16 13:26, 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 only the author
knows the best), "Don't land yet" comment somewhere will easily be
overlooked
- as a reviewer I don't want to bare the decision to land or not to
land, at all
- I want to preserve the mechanism "r+ with comments", which means to
let the patch be landed by the author after updated (means reviewer
doesn't need to look over it again)
- as an author I want to write comments about the patch (about the
structure, what it does, why) to make the review simpler for the
reviewer ; commit message description may not be the best place, right?
- will it be possible to still be using hg patch queues?
- I (and others too) am not fun of MozReview UI.  As a reviewer I found
it very hard to orient in it:
- what is the difference between [Reviews] and [Diff] tab? what is
exactly it's content
- where exactly to click to start a reivew of a patch I want to
review now?  Is in the "Commits" table?  And is it under "Diff" or
"Reviews"?
- how can I mark hunks/files are already reviewed (something I like
on Splinter)?
- how can I see only a single file diff and easily navigate between
files? (as in Splinter)
- few weeks ago I didn't even know how to give an r+!!  it's hidden
under the [Finish review...] *tab*?
- simply said: the UI is everything but self-explanatory and highly
unfriendly, until that is fixed I'm not much willing to use MozReview
mainly as a reviewer

-hb-


On 1/22/2016 3: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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[smaug]

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 post-facto, as you note.


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. People
might think the interdiff problem is solved because mozreview uses a VCS
under the hood, but that's not true. In fact, in some cases it's worse
now, because while splinter could essentially tell you it can't display
an interdiff, mozreview will happily display you an interdiff that is
not what you'd expect.

Mike




Indeed. MozReview showing bogus interdiffs is rather major issue. It happens 
also between
the changes for the same bug, if there are some file deletions etc.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[smaug]

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 gate keepers. To not do this is somewhat
undermining the review process.


I am not the busiest reviewer out there but I review a decent number of 
patches.  I can't think of _ever_ wanting to look at the interdiff resulted
from a rebase.

Also, I believe we have data showing that most of our reviews are done by a 
relatively small portion of people.  I'd like to humbly suggest that
changing processes to put even more burden on the reviewers may not necessarily 
be the best course of action going forward.

...


Cheers,
Ehsan



Yeah. This is rather subjective view, but I believe that optimizing reviewers' 
workflows and reducing the time they need to spend on a bug
would increase overall productivity more than optimizing patch authors' workflows. Landing code is not too often blocked on patch author but is often 
blocked on reviewer.

Of course the best would be to improve all the workflows.


-Oli
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Honza Bambas]

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 only the author 
knows the best), "Don't land yet" comment somewhere will easily be 
overlooked
- as a reviewer I don't want to bare the decision to land or not to 
land, at all
- I want to preserve the mechanism "r+ with comments", which means to 
let the patch be landed by the author after updated (means reviewer 
doesn't need to look over it again)
- as an author I want to write comments about the patch (about the 
structure, what it does, why) to make the review simpler for the 
reviewer ; commit message description may not be the best place, right?

- will it be possible to still be using hg patch queues?
- I (and others too) am not fun of MozReview UI.  As a reviewer I found 
it very hard to orient in it:
- what is the difference between [Reviews] and [Diff] tab? what is 
exactly it's content
- where exactly to click to start a reivew of a patch I want to 
review now?  Is in the "Commits" table?  And is it under "Diff" or 
"Reviews"?
- how can I mark hunks/files are already reviewed (something I like 
on Splinter)?
- how can I see only a single file diff and easily navigate between 
files? (as in Splinter)
- few weeks ago I didn't even know how to give an r+!!  it's hidden 
under the [Finish review...] *tab*?
- simply said: the UI is everything but self-explanatory and highly 
unfriendly, until that is fixed I'm not much willing to use MozReview 
mainly as a reviewer


-hb-


On 1/22/2016 3: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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform



___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[L. David Baron]

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 interdiff trust-worthiness are high-priority to fix.

It's not clear to me that making trustworthy interdiffs across
rebases is a thing that can be done given the current UI for showing
diffs, since it's not clear to me that it shows enough information.

I regularly do diffs of diffs (producing double-diffs that have two
columns of +/-/ before the code rather than one), and I've
gotten used to reading them.  It's not clear to me how to explain
interdiffs in a reliable and trustworthy way without that much
metadata about the changes.

-David

-- 
턞   L. David Baron http://dbaron.org/   턂
턢   Mozilla  https://www.mozilla.org/   턂
 Before I built a wall I'd ask to know
 What I was walling in or walling out,
 And to whom I was like to give offense.
   - Robert Frost, Mending Wall (1914)


signature.asc
Description: Digital signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Mike Conley]

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 of diffs and interdiffs is
very much a thing that we should be able to take for granted. Any bugs in
diff and interdiff trust-worthiness are high-priority to fix.

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1238000
[2]: https://reviews.reviewboard.org/r/7874/

On 25 January 2016 at 11:28, Boris Zbarsky  wrote:

> 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 hard problem, though (interdiff across rebases).  I
> guess that does mean that you can't really use the final attached thing for
> the "I want to see the interdiff" use case; need to push something to
> MozReview before rebasing to address that.
>
>
> -Boris
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Steve Fink]

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 only the author 
knows the best), "Don't land yet" comment somewhere will easily be 
overlooked
- as a reviewer I don't want to bare the decision to land or not to 
land, at all
- I want to preserve the mechanism "r+ with comments", which means to 
let the patch be landed by the author after updated (means reviewer 
doesn't need to look over it again)
- as an author I want to write comments about the patch (about the 
structure, what it does, why) to make the review simpler for the 
reviewer ; commit message description may not be the best place, right?


Yes, is there a place for this? I feel this is a really important bit of 
functionality that would fit naturally into the mozreview world, but I 
don't see how to do it. Situation: I put up a set of commits for review, 
and I want to give per-commit notes to the reviewer that they'll see 
before reviewing. Previously, I would put them in as comments on the bug 
and either pray that the reviewer happened to see them, or ping the 
reviewer on IRC and tell them to read them. In MozReview, I don't see a 
place to put such things at all?



- 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 things, but 
doing without mq is nicer in other ways, and the future leads away from mq.


On the other hand, there still isn't a great way to figure out the 
mq-less workflow. I remember a pretty good writeup from someone, and I 
intend to write up my own. But there isn't anything that's easily 
discoverable.


- I (and others too) am not fun of MozReview UI.  As a reviewer I 
found it very hard to orient in it:
- what is the difference between [Reviews] and [Diff] tab? what is 
exactly it's content


I just attempted to use MozReview again recently, and from what I can 
tell: there are kind of two main modes, I'll call them "overall" and 
"per-commit". There's is an overall pane that sticks to the top, 
incidentally making it difficult to figure out whether you're in the 
overall or per-commit mode. Clicking on "Review Summary" goes to the 
overall review mode -- as in, the metadata about the whole review. Below 
the fixed pane is the history of metadata changes (so "Review summary" 
means "Overall review metadata view", or something.) Clicking on 
"Squashed diff" is also for the overall review mode, but now it shows 
the squashed diff. In the overall mode, there's some text that claims 
that you can leave comments on the squashed diff, but advises against 
it. There's also no obvious way to leave such comments, but I just 
figured out that it must mean that you can select regions of code and 
it'll pop up a comment box. AFAICT, there's no way to leave general 
comments.


Also, on a large (overall) change, the UI goes off and spends so much 
time loading in all of the changed chunks that it doesn't have time to 
be responsive to button clicks and things, so it's really hard to tell 
what is working and what isn't. But my browser is suffering for other 
reasons these days (yeah, Nightly really sucks for me right now), so 
that may be local.


You get into "per-commit" mode by clicking on the description of a 
review. That will try to trick you by leaving the overall summary in 
place at the top, but if you are careful you'll notice that the portion 
below it is now specific to one commit.


Oh! In writing this, I finally figured out the data model. You have two 
orthogonal dimensions: "review vs diff" and "overall vs per-commit". 
"Review Summary" means "overall x review". "Squashed diff" means 
"overall x diff". The summary table at the top lists links that will get 
you to per-commit views for each commit. The ones in the "Diff" column 
are "per-commit x diff". The textual descriptions in the "Reviews" 
column are "per-commit x review". The two 
tabs-that-still-aren't-true-tabs in the top right control just the 
"review vs diff" dimension for either the overall or the per-commit views.


\o/ It's starting to finally make sense to me! (I think?)

Then there are the tabs-that-aren't-even-close-to-being-tabs, which have 
various context-specific actions on them.


> - where exactly to click to start a reivew of a patch I want to 
review now?  Is in the "Commits" table?  And is it under "Diff" or 
"Reviews"?


To leave a comment, it looks like you have to go to the Commits table, 
under the "Diff" column, and then select line regions.


It took me a while to figure out the latter part, btw. I selected a 
region of code to comment on it. No luck. I double-clicked a line of 
code. No luck. I eventually figured out that it wants you to only 

Re: Just Autoland It

[Mike Connor]

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: https://www.chromium.org/developers/testing/commit-queue
>

I don't think that policy really solves for putting the patch author in
clear control, or enabling a lands-when-reviewed workflow.  That's why I
suggested a different flag to allow authors and reviewers to explicitly
incorporate autoland into the workflow on an opt-in basis.

tl;dr

* We should make it easy for authors to explicitly opt-in to reviewer-lands
as a workflow.
* If the patch author doesn't opt in, we should assume business as usual.

-- Mike
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Paolo Amadini]

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 reviewer.

Paolo
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Boris Zbarsky]

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 hard problem, though (interdiff across rebases). 
 I guess that does mean that you can't really use the final attached 
thing for the "I want to see the interdiff" use case; need to push 
something to MozReview before rebasing to address that.


-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Mike Hommey]

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 when the patch actually lands
> >(dependencies, any other timing reasons, that only the author knows the
> >best), "Don't land yet" comment somewhere will easily be overlooked
> >- as a reviewer I don't want to bare the decision to land or not to land,
> >at all
> >- I want to preserve the mechanism "r+ with comments", which means to let
> >the patch be landed by the author after updated (means reviewer doesn't
> >need to look over it again)
> >- as an author I want to write comments about the patch (about the
> >structure, what it does, why) to make the review simpler for the reviewer
> >; commit message description may not be the best place, right?
> 
> Yes, is there a place for this? I feel this is a really important bit of
> functionality that would fit naturally into the mozreview world, but I don't
> see how to do it. Situation: I put up a set of commits for review, and I
> want to give per-commit notes to the reviewer that they'll see before
> reviewing. Previously, I would put them in as comments on the bug and either
> pray that the reviewer happened to see them, or ping the reviewer on IRC and
> tell them to read them. In MozReview, I don't see a place to put such things
> at all?

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 to make it show more by hovering the diff part for a
little while, it's not really great, as it doesn't actually show a diff
view like the diff pane does, and switching to the diff pane a) is slow
for large diffs and b) has an entirely different comment UX that doesn't
seem really great either.

Also, iirc, when you reply diff comments in MozReview, the resulting
comments sent to bugzilla lack context (but I could be misremembering).

> >- 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 things, but doing
> without mq is nicer in other ways, and the future leads away from mq.
> 
> On the other hand, there still isn't a great way to figure out the mq-less
> workflow. I remember a pretty good writeup from someone, and I intend to
> write up my own. But there isn't anything that's easily discoverable.

Whether or not mq is deprecated, it still uses changesets that can be
pushed. And with a non publishing remote repository, it should still be
able to qpop without having to fool around with the draft state.

Mike
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Andrew Halberstadt]

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:

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 only the author knows the
best), "Don't land yet" comment somewhere will easily be overlooked
- as a reviewer I don't want to bare the decision to land or not to

land,

at all
- I want to preserve the mechanism "r+ with comments", which means to

let

the patch be landed by the author after updated (means reviewer doesn't
need to look over it again)
- as an author I want to write comments about the patch (about the
structure, what it does, why) to make the review simpler for the

reviewer

; commit message description may not be the best place, right?


Yes, is there a place for this? I feel this is a really important bit of
functionality that would fit naturally into the mozreview world, but I

don't

see how to do it. Situation: I put up a set of commits for review, and I
want to give per-commit notes to the reviewer that they'll see before
reviewing. Previously, I would put them in as comments on the bug and

either

pray that the reviewer happened to see them, or ping the reviewer on IRC

and

tell them to read them. In MozReview, I don't see a place to put such

things

at all?


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 to make it show more by hovering the diff part for a
little while, it's not really great, as it doesn't actually show a diff
view like the diff pane does, and switching to the diff pane a) is slow
for large diffs and b) has an entirely different comment UX that doesn't
seem really great either.



Indeed. It would be great if it would just include 5-8 lines of context by
default.

-Ekr


It's not terribly obvious, but instead of clicking on a line number you 
can click and drag on the numbers to set the exact amount of context you 
want.


-Andrew





Also, iirc, when you reply diff comments in MozReview, the resulting
comments sent to bugzilla lack context (but I could be misremembering).


- 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 things, but

doing

without mq is nicer in other ways, and the future leads away from mq.

On the other hand, there still isn't a great way to figure out the

mq-less

workflow. I remember a pretty good writeup from someone, and I intend to
write up my own. But there isn't anything that's easily discoverable.


Whether or not mq is deprecated, it still uses changesets that can be
pushed. And with a non publishing remote repository, it should still be
able to qpop without having to fool around with the draft state.

Mike
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform



___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Steve Fink]

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 things, but doing
without mq is nicer in other ways, and the future leads away from mq.

On the other hand, there still isn't a great way to figure out the mq-less
workflow. I remember a pretty good writeup from someone, and I intend to
write up my own. But there isn't anything that's easily discoverable.

Whether or not mq is deprecated, it still uses changesets that can be
pushed. And with a non publishing remote repository, it should still be
able to qpop without having to fool around with the draft state.



For the first (force) push, sure. But given mq's love of rewriting 
history, I assume it will not maintain any correspondence between 
versions of a patch (either via obsolescence markers, or gps's magic 
metadata field, or whatever). So if you push an updated patch, I 
wouldn't expect it to be able to figure out that it's an update of an 
earlier commit? Unless it's doing some heuristic matching on comments or 
something.


It just got one of my patches wrong, though, so maybe it *is* doing 
something fancy. :-)


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Eric Rescorla]

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 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 only the author knows the
> > >best), "Don't land yet" comment somewhere will easily be overlooked
> > >- as a reviewer I don't want to bare the decision to land or not to
> land,
> > >at all
> > >- I want to preserve the mechanism "r+ with comments", which means to
> let
> > >the patch be landed by the author after updated (means reviewer doesn't
> > >need to look over it again)
> > >- as an author I want to write comments about the patch (about the
> > >structure, what it does, why) to make the review simpler for the
> reviewer
> > >; commit message description may not be the best place, right?
> >
> > Yes, is there a place for this? I feel this is a really important bit of
> > functionality that would fit naturally into the mozreview world, but I
> don't
> > see how to do it. Situation: I put up a set of commits for review, and I
> > want to give per-commit notes to the reviewer that they'll see before
> > reviewing. Previously, I would put them in as comments on the bug and
> either
> > pray that the reviewer happened to see them, or ping the reviewer on IRC
> and
> > tell them to read them. In MozReview, I don't see a place to put such
> things
> > at all?
>
> 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 to make it show more by hovering the diff part for a
> little while, it's not really great, as it doesn't actually show a diff
> view like the diff pane does, and switching to the diff pane a) is slow
> for large diffs and b) has an entirely different comment UX that doesn't
> seem really great either.
>

Indeed. It would be great if it would just include 5-8 lines of context by
default.

-Ekr


>
> Also, iirc, when you reply diff comments in MozReview, the resulting
> comments sent to bugzilla lack context (but I could be misremembering).
>
> > >- 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 things, but
> doing
> > without mq is nicer in other ways, and the future leads away from mq.
> >
> > On the other hand, there still isn't a great way to figure out the
> mq-less
> > workflow. I remember a pretty good writeup from someone, and I intend to
> > write up my own. But there isn't anything that's easily discoverable.
>
> Whether or not mq is deprecated, it still uses changesets that can be
> pushed. And with a non publishing remote repository, it should still be
> able to qpop without having to fool around with the draft state.
>
> Mike
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Mike Hommey]

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 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 only the author knows the
> best), "Don't land yet" comment somewhere will easily be overlooked
> - as a reviewer I don't want to bare the decision to land or not to
> >>land,
> at all
> - I want to preserve the mechanism "r+ with comments", which means to
> >>let
> the patch be landed by the author after updated (means reviewer doesn't
> need to look over it again)
> - as an author I want to write comments about the patch (about the
> structure, what it does, why) to make the review simpler for the
> >>reviewer
> ; commit message description may not be the best place, right?
> >>>
> >>>Yes, is there a place for this? I feel this is a really important bit of
> >>>functionality that would fit naturally into the mozreview world, but I
> >>don't
> >>>see how to do it. Situation: I put up a set of commits for review, and I
> >>>want to give per-commit notes to the reviewer that they'll see before
> >>>reviewing. Previously, I would put them in as comments on the bug and
> >>either
> >>>pray that the reviewer happened to see them, or ping the reviewer on IRC
> >>and
> >>>tell them to read them. In MozReview, I don't see a place to put such
> >>things
> >>>at all?
> >>
> >>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 to make it show more by hovering the diff part for a
> >>little while, it's not really great, as it doesn't actually show a diff
> >>view like the diff pane does, and switching to the diff pane a) is slow
> >>for large diffs and b) has an entirely different comment UX that doesn't
> >>seem really great either.
> >>
> >
> >Indeed. It would be great if it would just include 5-8 lines of context by
> >default.
> >
> >-Ekr
> 
> It's not terribly obvious, but instead of clicking on a line number you can
> click and drag on the numbers to set the exact amount of context you want.

Which is something for the person writing the comment to do. Also, even
when they do that, most of the time, that won't contain the surrounding
code.

Mike
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Ehsan Akhgari]

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 are
independent.  I get review on B first, but wait for review on A before
landing them both.

Option 2 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, reorder the commits and land B, while still waiting
for review on A.

I suspect you're doing option 2 as your usual workflow, not 3.


Yeah sorry, I meant to say option 2.  Thanks for correcting me!


Many times I also have to do this because it's unclear which reviewer
will review first, and serializing the review requests in the order in
which the commits appear in my repo will make the process of landing
code take too long.


Precisely why option 3 is the worst.  ;)


Agreed.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Gregory Szorc]

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 fail. So why are you pushing a "bad" commit/tree for
>> review? If your commits depend on something else, that something else
>> should be in the ancestry [and available to MozReview to inspect].
>>
>
> I'm going to assume this is a non-rhetorical question.
>
> Here's a concrete example of a reason to do this.  Say I have two patches
> for two separate bugs.  They're totally unrelated to each other in general,
> but they happen to both need to touch the same spot of code, possibly in
> the same way, but possibly in slightly different ways).  So the patches
> conflict with each other, effectively.
>
> Now I have several options:
>
> 1)  Develop the patches on separate branches (or on separate repos),
> request separate reviews on them, whichever one loses the review/land race
> needs to get updated in mozreview before getting pushed.
>
> 2)  Develop the patches on the same branch, request reviews on them, if
> the one that's "second" gets review first, reorder the commits, making the
> minor changes needed to handle that one conflict, land the thing that has
> review, update the other one in mozreview.
>
> 3)  Develop the patches on the same branch, request reviews on them, wait
> until the "first" one has reviews before landing either one.
>
>
In practice, I believe option 3 is the worst one for the situation I
> described, because it gates an unrelated bug on another bug.  And this
> situation happens all the time.  I'd estimate that at least 10% of my
> patches have self-conflicts of the type I described above.
>
> Option 2 is nice in needing a bit less overhead in terms of working trees
> or rebuilds on branch-switches.  It also just happens automatically quite
> often, unless you're using one branch per bug always.  I believe you're
> saying this option is unsupported by MozReview if the actual branch is
> pushed (because it's two separate bugs), so the natural way to use
> MozReview here would be to cherry-pick.
>


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.


>
> Option 1 just happens if you use one branch per bug, or just happen to
> work in a different local repo and don't realize you have the
> self-conflict. Happens all the time, again.  In practice it will lead to
> precisely the "cherrypicked" setup Ehsan described: as soon as one patch
> lands, the other will behave as if it had been cherrypicked.
>

In general, I don't want MozReview to dictate client-side branching
workflows. It does that today courtesy of not handling multi-bug series and
advanced commit tracking that well. Support will improve over time.

It's worth noting that I also want to put less emphasis on bugs as our unit
of work. Modern version control workflows revolve around lines of work, or
topic branches. It is a generally accepted best practice to use multiple
topic branches. But some people just lump everything together because it
can be easier than wrestling with endless history editing and merge
conflicts in your version control tool. Anyway. sometimes a feature branch
is related to a single bug. Sometimes multiple bugs. Sometimes you start
work on something that has no bug yet! I'd like our code development
workflow to flow more around the code than bugs. Bugs may be the driver for
the work and the ultimate mechanism to track that work. But the
code/commits - not the bugs - should be driving the workflow. It feels
wrong to me that we currently have to practice sub-optimal code writing and
review workflows to accommodate the relationship between bugs. This changes
as code review is extracted from Bugzilla. See also
http://gregoryszorc.com/blog/2015/01/16/bugzilla-and-the-future-of-firefox-development/


>
> The ultimate goal is to
>> push things as they need to land
>>
>
> This is a ... pretty vague concept.  In practice, the way it works is you
> write a bunch of code for nominally unrelated bugs, some of it possibly
> with non-obvious dependencies or self-conflicts or both, possibly on one
> branch, possibly on several branches, possibly on several branches with
> multiple bugs per branch.  Then you ask for reviews on all of the unrelated
> things in parallel and wait hours to months depending on the reviewers.
> Then in the order you get the reviews you cherrypick things onto that day's
> tip, fix merge issues as needed, run a try push if you're not confident
> enough that things are still OK, and then push.


We would eventually like most Firefox landings to go through Autoland. This
tentatively means going through MozReview as a means to get into Autoland
because the 

Re: Just Autoland It

[Gregory Szorc]

On Sat, Jan 23, 2016 at 8:07 AM, Eric Rescorla  wrote:

> IMO the right place for this checkbox is in the review request, which puts
> the
> control in the right place: the patch author.
>

I agree we need an author-set flag. I like mconnor's suggestion for a
"land?" flag or similar.

I also like the idea of an extra prompt during submission. The way a lot of
review submission tools work is they open your editor with a `hg histedit`
or `git rebase -i` like summary of what is going to be submitted and you
can tailor the list of commits, enter extra comments to the reviewer, etc.
This is something we eventually want to add and throwing a "ready to land?"
flag in there is totally doable.

Also, missing from my original post and subsequent replies is that "just
land it" is of course not always the correct mechanism. I should have said
"use your best judgement and then land it." I have little qualms about
"just landing" small commits to fix typos without explicitly asking the
author. A 40 part series to refactor layout code, not so much :)



>
> On Thu, Jan 21, 2016 at 7:00 PM, Dave Townsend 
> wrote:
>
>> Should we just add a "and land it" checkbox to the review page, maybe
>> disabled if there are still open issues?
>>
>> On Thu, Jan 21, 2016 at 6:35 PM, 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
>> >
>> ___
>> firefox-dev mailing list
>> firefox-...@mozilla.org
>> https://mail.mozilla.org/listinfo/firefox-dev
>>
>
>
> ___
> 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

Re: Just Autoland It

[Eric Rescorla]

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: https://www.chromium.org/developers/testing/commit-queue

Current process for the user

   1. Upload a review to rietveld where it gets reviewed and LGTM'ed.
   2. One of:
  1. Check (select) the 'Commit' checkbox on a Rietveld issue, on the
  particular patchset that has been approved. Checking the checkbox is the
  only action required and there will be no immediate feedback in
the form of
  messages, etc, to notify you that something has happened.
 - Only the issue owner, someone at @chromium.org or @google.com
 can check the box.
 - Yes, *non-Chromium committers are allowed *to use the commit
 queue but cannot LGTM a change.
  2. At the command line, type git cl set_commit
  3. Have a reviewer use 'Quick LGTM & CQ'.
   3. Wait an hour. The current list of patches to be queued can be found
   at Commit Queue Patches
   , while CQ
   progress can be tracked at Commit Queue Progress
   . The commit queue will wait
   automatically for the tree to reopen.
   4. Wait for an email from commit-...@chromium.org with success or
   failure.

-Ekr


On Sat, Jan 23, 2016 at 8:07 AM, Eric Rescorla  wrote:

> IMO the right place for this checkbox is in the review request, which puts
> the
> control in the right place: the patch author.
>
> -Ekr
>
>
> On Thu, Jan 21, 2016 at 7:00 PM, Dave Townsend 
> wrote:
>
>> Should we just add a "and land it" checkbox to the review page, maybe
>> disabled if there are still open issues?
>>
>> On Thu, Jan 21, 2016 at 6:35 PM, 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
>> >
>> ___
>> 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

Re: Just Autoland It

[Gregory Szorc]

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, it would fail. So why are you pushing a "bad" commit/tree for
>> review? If your commits depend on something else, that something else
>> should be in the ancestry [and available to MozReview to inspect].
>>
>
> There are two reasons I disagree with this, both of which I believe to be
> distinct from Boris' reasons:
>
> * "depends" is fuzzy and does not always lead to try or build failure. For
> an example, I now have review for 80% of my patches on bug 1219215, but I
> need to wait to land that until bug 1241275 gets a patch, lands, etc.,
> otherwise I will regress window dragging on Windows. To the best of my
> knowledege we do not have try coverage for this, in part (I think) because
> doing "native" aero snap dragging within mochitest isn't possible. The code
> I'm touching won't conflict with the patch in bug 1241275, and anyway, a
> finished patch wasn't available when I wrote patches for 1219215.
>
> * Historically, mozreview has been... less than amazing... about pushing
> draft commits with multiple different bug numbers and treating them "as I
> would expect" (ie, separate bug numbers go on separate bugs, not all in one
> parent review. If it doesn't know what to do, it should ask with a warning,
> or do nothing, not blunder ahead and mess things up.). I have developed the
> reflex to always re-'up' to fx-team tip in my local repo before starting a
> new patch - even if it's related to the other patch I'm writing - simply to
> avoid accidentally pushing reviews onto the wrong bugzilla bug, which is
> impossible to undo anyway, and sometimes messes with other bugs' state in
> ways that are painful to manually recover.


These are known problems and there is tons of room to improve. We made the
decision to prioritize Q4 work around UI warts. With Git support for
MozReview liking landing next week, this issue will likely be raised by a
whole new crop of users, further reminding us of how painful it is. I think
there are some low hanging fruits we can deploy in the next few weeks to
make this much better.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Gijs Kruitbosch]

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 something else, that something else
should be in the ancestry [and available to MozReview to inspect].


There are two reasons I disagree with this, both of which I believe to 
be distinct from Boris' reasons:


* "depends" is fuzzy and does not always lead to try or build failure. 
For an example, I now have review for 80% of my patches on bug 1219215, 
but I need to wait to land that until bug 1241275 gets a patch, lands, 
etc., otherwise I will regress window dragging on Windows. To the best 
of my knowledege we do not have try coverage for this, in part (I think) 
because doing "native" aero snap dragging within mochitest isn't 
possible. The code I'm touching won't conflict with the patch in bug 
1241275, and anyway, a finished patch wasn't available when I wrote 
patches for 1219215.


* Historically, mozreview has been... less than amazing... about pushing 
draft commits with multiple different bug numbers and treating them "as 
I would expect" (ie, separate bug numbers go on separate bugs, not all 
in one parent review. If it doesn't know what to do, it should ask with 
a warning, or do nothing, not blunder ahead and mess things up.). I have 
developed the reflex to always re-'up' to fx-team tip in my local repo 
before starting a new patch - even if it's related to the other patch 
I'm writing - simply to avoid accidentally pushing reviews onto the 
wrong bugzilla bug, which is impossible to undo anyway, and sometimes 
messes with other bugs' state in ways that are painful to manually recover.


~ Gijs
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Ehsan Akhgari]

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 you pushed
this to Try, it would fail. So why are you pushing a "bad" commit/tree for
review? If your commits depend on something else, that something else
should be in the ancestry [and available to MozReview to inspect].



I'm going to assume this is a non-rhetorical question.

Here's a concrete example of a reason to do this.  Say I have two patches
for two separate bugs.  They're totally unrelated to each other in general,
but they happen to both need to touch the same spot of code, possibly in
the same way, but possibly in slightly different ways).  So the patches
conflict with each other, effectively.

Now I have several options:

1)  Develop the patches on separate branches (or on separate repos),
request separate reviews on them, whichever one loses the review/land race
needs to get updated in mozreview before getting pushed.

2)  Develop the patches on the same branch, request reviews on them, if
the one that's "second" gets review first, reorder the commits, making the
minor changes needed to handle that one conflict, land the thing that has
review, update the other one in mozreview.

3)  Develop the patches on the same branch, request reviews on them, wait
until the "first" one has reviews before landing either one.



In practice, I believe option 3 is the worst one for the situation I

described, because it gates an unrelated bug on another bug.  And this
situation happens all the time.  I'd estimate that at least 10% of my
patches have self-conflicts of the type I described above.


FWIW, option 3 is basically my usual workflow, and you didn't really 
respond to this.


The reason why I need to do things this way by default is that out build 
system is extremely hostile to topic branches, for example, 
, or the need to 
spend ~20 minutes rebuilding everything if the branch you switched to 
happens to change a configure variable, etc.


Many times I also have to do this because it's unclear which reviewer 
will review first, and serializing the review requests in the order in 
which the commits appear in my repo will make the process of landing 
code take too long.



Option 2 is nice in needing a bit less overhead in terms of working trees
or rebuilds on branch-switches.  It also just happens automatically quite
often, unless you're using one branch per bug always.  I believe you're
saying this option is unsupported by MozReview if the actual branch is
pushed (because it's two separate bugs), so the natural way to use
MozReview here would be to cherry-pick.


FWIW I should also mention that I regularly run into situations where I 
need to maintain a 30-40 commit long local queue of unlanded stuff, and 
in which case option 2 will just be way too painful if something gets 
landed which has 10 commits on top of it waiting for review.



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.


I think that doesn't really address any of the aforementioned problems. 
 The problem is not in submitting specific commits through MozReview.



Option 1 just happens if you use one branch per bug, or just happen to
work in a different local repo and don't realize you have the
self-conflict. Happens all the time, again.  In practice it will lead to
precisely the "cherrypicked" setup Ehsan described: as soon as one patch
lands, the other will behave as if it had been cherrypicked.



In general, I don't want MozReview to dictate client-side branching
workflows. It does that today courtesy of not handling multi-bug series and
advanced commit tracking that well. Support will improve over time.


Note that there are also other things that might dictate people's local 
workflows, as I gave a few examples above.



It's worth noting that I also want to put less emphasis on bugs as our unit
of work.


That's fine, but it's orthogonal to the problems above.  You can just 
re-read everything above and remove all mentions of "bugs" and I believe 
all problems stated so far will remain.


> Modern version control workflows revolve around lines of work, or

topic branches. It is a generally accepted best practice to use multiple
topic branches. But some people just lump everything together because it
can be easier than wrestling with endless history editing and merge
conflicts in your version control tool. Anyway. sometimes a feature branch
is related to a single bug. Sometimes multiple bugs. Sometimes you start
work on something that has no bug yet! I'd like our code development
workflow to flow more around the code than 

Re: Just Autoland It

[Gregory Szorc]

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 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 something else, that something else
>> >> should be in the ancestry [and available to MozReview to inspect].
>> >>
>> >
>> > I'm going to assume this is a non-rhetorical question.
>> >
>> > Here's a concrete example of a reason to do this.  Say I have two
>> patches
>> > for two separate bugs.  They're totally unrelated to each other in
>> general,
>> > but they happen to both need to touch the same spot of code, possibly in
>> > the same way, but possibly in slightly different ways).  So the patches
>> > conflict with each other, effectively.
>> >
>> > Now I have several options:
>> >
>> > 1)  Develop the patches on separate branches (or on separate repos),
>> > request separate reviews on them, whichever one loses the review/land
>> race
>> > needs to get updated in mozreview before getting pushed.
>> >
>> > 2)  Develop the patches on the same branch, request reviews on them, if
>> > the one that's "second" gets review first, reorder the commits, making
>> the
>> > minor changes needed to handle that one conflict, land the thing that
>> has
>> > review, update the other one in mozreview.
>> >
>> > 3)  Develop the patches on the same branch, request reviews on them,
>> wait
>> > until the "first" one has reviews before landing either one.
>> >
>> >
>> In practice, I believe option 3 is the worst one for the situation I
>> > described, because it gates an unrelated bug on another bug.  And this
>> > situation happens all the time.  I'd estimate that at least 10% of my
>> > patches have self-conflicts of the type I described above.
>> >
>> > Option 2 is nice in needing a bit less overhead in terms of working
>> trees
>> > or rebuilds on branch-switches.  It also just happens automatically
>> quite
>> > often, unless you're using one branch per bug always.  I believe you're
>> > saying this option is unsupported by MozReview if the actual branch is
>> > pushed (because it's two separate bugs), so the natural way to use
>> > MozReview here would be to cherry-pick.
>> >
>>
>>
>> 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.
>>
>>
>> >
>> > Option 1 just happens if you use one branch per bug, or just happen to
>> > work in a different local repo and don't realize you have the
>> > self-conflict. Happens all the time, again.  In practice it will lead to
>> > precisely the "cherrypicked" setup Ehsan described: as soon as one patch
>> > lands, the other will behave as if it had been cherrypicked.
>> >
>>
>> In general, I don't want MozReview to dictate client-side branching
>> workflows. It does that today courtesy of not handling multi-bug series
>> and
>> advanced commit tracking that well. Support will improve over time.
>>
>> It's worth noting that I also want to put less emphasis on bugs as our
>> unit
>> of work. Modern version control workflows revolve around lines of work, or
>> topic branches. It is a generally accepted best practice to use multiple
>> topic branches. But some people just lump everything together because it
>> can be easier than wrestling with endless history editing and merge
>> conflicts in your version control tool. Anyway. sometimes a feature branch
>> is related to a single bug. Sometimes multiple bugs. Sometimes you start
>> work on something that has no bug yet! I'd like our code development
>> workflow to flow more around the code than bugs. Bugs may be the driver
>> for
>> the work and the ultimate mechanism to track that work. But the
>> code/commits - not the bugs - should be driving the workflow. It feels
>> wrong to me that we currently have to practice sub-optimal code writing
>> and
>> review workflows to accommodate the relationship between bugs. This
>> changes
>> as code review is extracted from Bugzilla. See also
>>
>> http://gregoryszorc.com/blog/2015/01/16/bugzilla-and-the-future-of-firefox-development/
>
>
> I'm not sure why you're focusing on "Bugs" versus "Commits" as the issue.
> It's not.
> Any situation where you have two parallel streams of work that tend to
> impact
> the same code can run into each other like this.
>
> To take a trivial example, I'm presently working on the implementation of
> TLS 1.3 in
> NSS. Along the way, we discovered the need to change the way that the
> GTests are
> linked. Jed Davis has a patch for this which hasn't 

Re: Just Autoland It

[Boris Zbarsky]

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 cherry-picked commits like 
this to MozReview...



It's worth noting that I also want to put less emphasis on bugs as our unit
of work.


Bugs are not the relevant thing here.  The relevant thing is multiple 
conceptually independent lines of work that happen to touch the same code.



It is a generally accepted best practice to use multiple
topic branches.


If switching topic branches, or even from a topic branch back to tip so 
you can start a new topic branch didn't typically involve long rebuilds, 
I'd consider doing this



It feels
wrong to me that we currently have to practice sub-optimal code writing and
review workflows to accommodate the relationship between bugs. This changes
as code review is extracted from Bugzilla. See also
http://gregoryszorc.com/blog/2015/01/16/bugzilla-and-the-future-of-firefox-development/


I've read this, yes.  I even agree with a lot of it, but it has nothing 
to do with the issue we're discussing here.



We would eventually like most Firefox landings to go through Autoland.


Yes, would be good.


1) Whatever you have done up to this point
2) Rebase / rewrite history / cherry-pick / etc your commits on top of
central
3) Push to MozReview
4) Autoland


OK.  So review effectively happens in step 1 here, yes?


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 gate keepers.


With my reviewer hat on, I often do just that.  As in, I'll explicitly 
say "r+ with these changes, but I want to see the interdiff before you 
land" or "r+ with these changes, and feel free to land, but I want to 
see the interdiff".  Of course sometimes I'll also just say "r+ with 
these changes", or "You need to make these changes and request another 
review".  It all depends on the scope of the changes and my past 
experience (or lack thereof) with the patch author...



I think this is the correct model: if a reviewer absolutely wants to see the
final version before landing, they can withhold r+ until all issues are
addressed.


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.



I hope this addressed your concerns.


If we assume that review happens in step 1 of your 4-item list above, 
then yes.


-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Eric Rescorla]

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 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 something else, that something else
> >> should be in the ancestry [and available to MozReview to inspect].
> >>
> >
> > I'm going to assume this is a non-rhetorical question.
> >
> > Here's a concrete example of a reason to do this.  Say I have two patches
> > for two separate bugs.  They're totally unrelated to each other in
> general,
> > but they happen to both need to touch the same spot of code, possibly in
> > the same way, but possibly in slightly different ways).  So the patches
> > conflict with each other, effectively.
> >
> > Now I have several options:
> >
> > 1)  Develop the patches on separate branches (or on separate repos),
> > request separate reviews on them, whichever one loses the review/land
> race
> > needs to get updated in mozreview before getting pushed.
> >
> > 2)  Develop the patches on the same branch, request reviews on them, if
> > the one that's "second" gets review first, reorder the commits, making
> the
> > minor changes needed to handle that one conflict, land the thing that has
> > review, update the other one in mozreview.
> >
> > 3)  Develop the patches on the same branch, request reviews on them, wait
> > until the "first" one has reviews before landing either one.
> >
> >
> In practice, I believe option 3 is the worst one for the situation I
> > described, because it gates an unrelated bug on another bug.  And this
> > situation happens all the time.  I'd estimate that at least 10% of my
> > patches have self-conflicts of the type I described above.
> >
> > Option 2 is nice in needing a bit less overhead in terms of working trees
> > or rebuilds on branch-switches.  It also just happens automatically quite
> > often, unless you're using one branch per bug always.  I believe you're
> > saying this option is unsupported by MozReview if the actual branch is
> > pushed (because it's two separate bugs), so the natural way to use
> > MozReview here would be to cherry-pick.
> >
>
>
> 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.
>
>
> >
> > Option 1 just happens if you use one branch per bug, or just happen to
> > work in a different local repo and don't realize you have the
> > self-conflict. Happens all the time, again.  In practice it will lead to
> > precisely the "cherrypicked" setup Ehsan described: as soon as one patch
> > lands, the other will behave as if it had been cherrypicked.
> >
>
> In general, I don't want MozReview to dictate client-side branching
> workflows. It does that today courtesy of not handling multi-bug series and
> advanced commit tracking that well. Support will improve over time.
>
> It's worth noting that I also want to put less emphasis on bugs as our unit
> of work. Modern version control workflows revolve around lines of work, or
> topic branches. It is a generally accepted best practice to use multiple
> topic branches. But some people just lump everything together because it
> can be easier than wrestling with endless history editing and merge
> conflicts in your version control tool. Anyway. sometimes a feature branch
> is related to a single bug. Sometimes multiple bugs. Sometimes you start
> work on something that has no bug yet! I'd like our code development
> workflow to flow more around the code than bugs. Bugs may be the driver for
> the work and the ultimate mechanism to track that work. But the
> code/commits - not the bugs - should be driving the workflow. It feels
> wrong to me that we currently have to practice sub-optimal code writing and
> review workflows to accommodate the relationship between bugs. This changes
> as code review is extracted from Bugzilla. See also
>
> http://gregoryszorc.com/blog/2015/01/16/bugzilla-and-the-future-of-firefox-development/


I'm not sure why you're focusing on "Bugs" versus "Commits" as the issue.
It's not.
Any situation where you have two parallel streams of work that tend to
impact
the same code can run into each other like this.

To take a trivial example, I'm presently working on the implementation of
TLS 1.3 in
NSS. Along the way, we discovered the need to change the way that the
GTests are
linked. Jed Davis has a patch for this which hasn't landed yet. So now we
have two:
things in flight:

- Jed's patch to the build system (smallish)
- My enormous TLS 1.3 patch

So, I cherry-picked Jed's patch into my branch, kept developing and am
having
my patch reviewed 

Re: Just Autoland It

[Mike Hommey]

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/ 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. People
might think the interdiff problem is solved because mozreview uses a VCS
under the hood, but that's not true. In fact, in some cases it's worse
now, because while splinter could essentially tell you it can't display
an interdiff, mozreview will happily display you an interdiff that is
not what you'd expect.

Mike
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Boris Zbarsky]

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, but wait for review on A before 
landing them both.


Option 2 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, reorder the commits and land B, while still waiting 
for review on A.


I suspect you're doing option 2 as your usual workflow, not 3.


Many times I also have to do this because it's unclear which reviewer
will review first, and serializing the review requests in the order in
which the commits appear in my repo will make the process of landing
code take too long.


Precisely why option 3 is the worst.  ;)

-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Eric Rescorla]

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  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 fail. So why are you pushing a "bad"
>>> commit/tree for
>>> >> review? If your commits depend on something else, that something else
>>> >> should be in the ancestry [and available to MozReview to inspect].
>>> >>
>>> >
>>> > I'm going to assume this is a non-rhetorical question.
>>> >
>>> > Here's a concrete example of a reason to do this.  Say I have two
>>> patches
>>> > for two separate bugs.  They're totally unrelated to each other in
>>> general,
>>> > but they happen to both need to touch the same spot of code, possibly
>>> in
>>> > the same way, but possibly in slightly different ways).  So the patches
>>> > conflict with each other, effectively.
>>> >
>>> > Now I have several options:
>>> >
>>> > 1)  Develop the patches on separate branches (or on separate repos),
>>> > request separate reviews on them, whichever one loses the review/land
>>> race
>>> > needs to get updated in mozreview before getting pushed.
>>> >
>>> > 2)  Develop the patches on the same branch, request reviews on them, if
>>> > the one that's "second" gets review first, reorder the commits, making
>>> the
>>> > minor changes needed to handle that one conflict, land the thing that
>>> has
>>> > review, update the other one in mozreview.
>>> >
>>> > 3)  Develop the patches on the same branch, request reviews on them,
>>> wait
>>> > until the "first" one has reviews before landing either one.
>>> >
>>> >
>>> In practice, I believe option 3 is the worst one for the situation I
>>> > described, because it gates an unrelated bug on another bug.  And this
>>> > situation happens all the time.  I'd estimate that at least 10% of my
>>> > patches have self-conflicts of the type I described above.
>>> >
>>> > Option 2 is nice in needing a bit less overhead in terms of working
>>> trees
>>> > or rebuilds on branch-switches.  It also just happens automatically
>>> quite
>>> > often, unless you're using one branch per bug always.  I believe you're
>>> > saying this option is unsupported by MozReview if the actual branch is
>>> > pushed (because it's two separate bugs), so the natural way to use
>>> > MozReview here would be to cherry-pick.
>>> >
>>>
>>>
>>> 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.
>>>
>>>
>>> >
>>> > Option 1 just happens if you use one branch per bug, or just happen to
>>> > work in a different local repo and don't realize you have the
>>> > self-conflict. Happens all the time, again.  In practice it will lead
>>> to
>>> > precisely the "cherrypicked" setup Ehsan described: as soon as one
>>> patch
>>> > lands, the other will behave as if it had been cherrypicked.
>>> >
>>>
>>> In general, I don't want MozReview to dictate client-side branching
>>> workflows. It does that today courtesy of not handling multi-bug series
>>> and
>>> advanced commit tracking that well. Support will improve over time.
>>>
>>> It's worth noting that I also want to put less emphasis on bugs as our
>>> unit
>>> of work. Modern version control workflows revolve around lines of work,
>>> or
>>> topic branches. It is a generally accepted best practice to use multiple
>>> topic branches. But some people just lump everything together because it
>>> can be easier than wrestling with endless history editing and merge
>>> conflicts in your version control tool. Anyway. sometimes a feature
>>> branch
>>> is related to a single bug. Sometimes multiple bugs. Sometimes you start
>>> work on something that has no bug yet! I'd like our code development
>>> workflow to flow more around the code than bugs. Bugs may be the driver
>>> for
>>> the work and the ultimate mechanism to track that work. But the
>>> code/commits - not the bugs - should be driving the workflow. It feels
>>> wrong to me that we currently have to practice sub-optimal code writing
>>> and
>>> review workflows to accommodate the relationship between bugs. This
>>> changes
>>> as code review is extracted from Bugzilla. See also
>>>
>>> http://gregoryszorc.com/blog/2015/01/16/bugzilla-and-the-future-of-firefox-development/
>>
>>
>> I'm not sure why you're focusing on "Bugs" versus "Commits" as the issue.
>> It's not.
>> Any situation where you have two parallel streams of work that tend to
>> impact
>> the same code can run into each other like this.
>>
>> To take a trivial 

Re: Just Autoland It

[Andrew Halberstadt]

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 don't think we should be surprising
people or unilaterally imposing changes to their workflow.  The best way to
do this is to make it simple to adopt, and demonstrably better than the old
way.  Developers will migrate over time as they see the light.

I think this is an easy fix if we're willing to modify our patch
submission workflow to reflect a wider set of asks and responses.  It's
more or less the same mental model as the multi-state flags we use for
tracking, there's more than one type of request, and more than one possible
response.

Simple proposal: replace review/feedback flags with a new,
multiple-requestable flag.  Possible values could be:

feedback?
review?
land?
feedback+
withcomments+
review+
land+





Patch authors would be able to opt into the easier flow by setting "land?"
instead of
"review?"  "review?" and "feedback?" would retain their current
definitions.  Patch reviewers would be able to respond explicitly with
feedback, a conditional r+, full r+, or land+.

* Anything where land+ is set would trigger autoland.
* land+ should be set only if requested, or if the reviewer believes
that's expected
* patch authors would be allowed to autoland with review+
* withcomments+ or feedback+ would not allow autoland.

In the short term, we could experiment with this as an additional patch
flag (land?/+/-).  I think the multi-state flag reflects current workflow
reality and eliminates nuance, so should be explored.



We've already talked about changing the Bugzilla flags to better express
intent. The BMO gurus say it isn't something we should undertake lightly.
It's much easier to experiment on the commit message / MozReview side of
things while leaving the existing Bugzilla flags in place. e.g. we can have
MozReview convert "land?" in commit messages to "review? flags in Bugzilla
while having autoland convert "land?" into "r=" upon landing.

It's worth noting that part of the reason we haven't enabled feedback flag
support and done more advanced things with commit messages is because
seemingly everyone practices slight variations of interactions with
Bugzilla flags and it's difficult codifying "One True Workflow" because
there are excellent justifications for nearly every variation. 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...


Personally I'm a little wary of adding too many flags. I think in this
case it should just be left as is. Will there be the occasional
miscommunication and backout as a result? Sure. That's not the end of
the world though.

People who work together often will quickly establish their personal set
of rules and expectations and adjust their workflows accordingly. And
for people who don't work together often, a simple "Please do not land
this yet" is only slightly less convenient than a flag, with the benefit
of being way more explicit.

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?

-Andrew


On Fri, Jan 22, 2016 at 12:25 AM, 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 further after
getting review (or I know a piece of work is incomplete and further parts
will be forthcoming).

Perhaps this needs an opt-in flag on the input side?

"r?, and land it if you're happy" versus "r?, but I'll take care of
landing"?


___
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

Re: Just Autoland It

[Gregory Szorc]

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 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 indication in mozreview whether a patch is
>>> self-contained or has dependencies, is there?
>>>
>>> Sometimes one can make a good guess, of course.
>>>
>>
>>
>> Not currently, no.
>>
>> However, we know from the repository DAG what the "dependencies" are and
>> we
>> done want to eventually surface these in MozReview by moving the review
>> request grouping in MozReview to something that is more DAG based instead
>> of bug-based.
>>
>
> 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, as another example, what if the patch needs to be checked
> in only after another patch (perhaps done by a different author) that is
> not connected to the review at hand?
>
> (Both of these examples are from my real workflow in the past couple of
> weeks or so.)
>
>
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 something else, that something else
should be in the ancestry [and available to MozReview to inspect].

MozReview today is very simple about how it handles incoming commits. It
defaults to reviewing all pushed commits (although you can control this).
It assumes one bug per submitted series. It assumes one author per series.
This is done because it was simple to implement. The ultimate goal is to
push things as they need to land and MozReview will sort it out. This
includes pushing commits from multiple bugs and commits belonging to other
people. I don't think it is possible for this to be perfect for all
workflows. But we should be able to handle common workflows or at least
nudge people towards well-supported workflows.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Boris Zbarsky]

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 something else, that something else
should be in the ancestry [and available to MozReview to inspect].


I'm going to assume this is a non-rhetorical question.

Here's a concrete example of a reason to do this.  Say I have two 
patches for two separate bugs.  They're totally unrelated to each other 
in general, but they happen to both need to touch the same spot of code, 
possibly in the same way, but possibly in slightly different ways).  So 
the patches conflict with each other, effectively.


Now I have several options:

1)  Develop the patches on separate branches (or on separate repos), 
request separate reviews on them, whichever one loses the review/land 
race needs to get updated in mozreview before getting pushed.


2)  Develop the patches on the same branch, request reviews on them, if 
the one that's "second" gets review first, reorder the commits, making 
the minor changes needed to handle that one conflict, land the thing 
that has review, update the other one in mozreview.


3)  Develop the patches on the same branch, request reviews on them, 
wait until the "first" one has reviews before landing either one.


In practice, I believe option 3 is the worst one for the situation I 
described, because it gates an unrelated bug on another bug.  And this 
situation happens all the time.  I'd estimate that at least 10% of my 
patches have self-conflicts of the type I described above.


Option 2 is nice in needing a bit less overhead in terms of working 
trees or rebuilds on branch-switches.  It also just happens 
automatically quite often, unless you're using one branch per bug 
always.  I believe you're saying this option is unsupported by MozReview 
if the actual branch is pushed (because it's two separate bugs), so the 
natural way to use MozReview here would be to cherry-pick.


Option 1 just happens if you use one branch per bug, or just happen to 
work in a different local repo and don't realize you have the 
self-conflict. Happens all the time, again.  In practice it will lead to 
precisely the "cherrypicked" setup Ehsan described: as soon as one patch 
lands, the other will behave as if it had been cherrypicked.



The ultimate goal is to
push things as they need to land


This is a ... pretty vague concept.  In practice, the way it works is 
you write a bunch of code for nominally unrelated bugs, some of it 
possibly with non-obvious dependencies or self-conflicts or both, 
possibly on one branch, possibly on several branches, possibly on 
several branches with multiple bugs per branch.  Then you ask for 
reviews on all of the unrelated things in parallel and wait hours to 
months depending on the reviewers.  Then in the order you get the 
reviews you cherrypick things onto that day's tip, fix merge issues as 
needed, run a try push if you're not confident enough that things are 
still OK, and then push.


-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Steve Fink]

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 the case. If you need to update a 
patch with review comments, update it and re-push to mozreview.


Question: how does mozreview handle the case where a commit has r+ and 
then a newer version is pushed? Does it inherit the r+ or not? IMHO, 
neither answer is correct all of the time, and it's problematic that I 
even have to wonder about what happens. This should be surfaced clearly 
in the UX... somehow. (Do we need a new |hg push --update| flag that 
means "inherit previous reviews"? Or should you just push and then go to 
the web interface and click to inherit the reviews?)


Anyway, my thinking about reviewers landing:

Landing on r+ is freaky because it is very common for the reviewer to 
request (or just suggest as a possibility, without requiring) changes 
before landing. That's not a problem because the reviewer would not land 
it in that case.


Landing on r+ is freaky because sometimes review is requested before a 
patch is known to work or not. That's not as bad as it sounds, because 
autoland won't land until tests pass. Question: if the implementation 
and its tests are in separate commits, is there a chance that autoland 
will land just the implementation but not the failing tests? The best 
practice around this should be documented.


So if tests properly cover the change, then the only cost for 
prematurely queuing up a commit for landing is machine time for doomed 
test runs. In other words, it is more important than it used to be to 
have tests for your changes. (But still not critical, since you can 
leave a "please don't land yet" note.)


But that's for testable stuff. The other meaning of "known to work" is 
whether the change achieves its intended purpose, and that's not always 
discoverable from an automated test run. This is quite common in my 
experience. Heck, performance optimizations often fall within this 
category, especially if it's a somewhat speculative change that 
complicates things in exchange for hoped-for but uncertain speed gains. 
Still, "please don't land" seems sufficient for these, if it's even 
necessary given the nature of the patch.


I would also suggest that reviewers should feel freer to land on r+ if 
there are adequate tests accompanying the change. And of course, there 
always "should" be adequate test in the first place, but that's often 
impractical, so I'm just saying to use tests as a signal when deciding 
whether to land on review.


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Mike Connor]

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 most of the experimentation with the workflow.

I've changed a ton of stuff in Bugzilla over the years, which is why I
tacked on the low risk alternative. :)

On Fri, Jan 22, 2016 at 2: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 don't think we should be surprising
>> people or unilaterally imposing changes to their workflow.  The best way to
>> do this is to make it simple to adopt, and demonstrably better than the old
>> way.  Developers will migrate over time as they see the light.
>>
>> I think this is an easy fix if we're willing to modify our patch
>> submission workflow to reflect a wider set of asks and responses.  It's
>> more or less the same mental model as the multi-state flags we use for
>> tracking, there's more than one type of request, and more than one possible
>> response.
>>
>> Simple proposal: replace review/feedback flags with a new,
>> multiple-requestable flag.  Possible values could be:
>>
>> feedback?
>> review?
>> land?
>> feedback+
>> withcomments+
>> review+
>> land+
>>
>
>>
>> Patch authors would be able to opt into the easier flow by setting
>> "land?" instead of
>> "review?"  "review?" and "feedback?" would retain their current
>> definitions.  Patch reviewers would be able to respond explicitly with
>> feedback, a conditional r+, full r+, or land+.
>>
>> * Anything where land+ is set would trigger autoland.
>> * land+ should be set only if requested, or if the reviewer believes
>> that's expected
>> * patch authors would be allowed to autoland with review+
>> * withcomments+ or feedback+ would not allow autoland.
>>
>> In the short term, we could experiment with this as an additional patch
>> flag (land?/+/-).  I think the multi-state flag reflects current workflow
>> reality and eliminates nuance, so should be explored.
>>
>>
> We've already talked about changing the Bugzilla flags to better express
> intent. The BMO gurus say it isn't something we should undertake lightly.
> It's much easier to experiment on the commit message / MozReview side of
> things while leaving the existing Bugzilla flags in place. e.g. we can have
> MozReview convert "land?" in commit messages to "review? flags in Bugzilla
> while having autoland convert "land?" into "r=" upon landing.
>
> It's worth noting that part of the reason we haven't enabled feedback flag
> support and done more advanced things with commit messages is because
> seemingly everyone practices slight variations of interactions with
> Bugzilla flags and it's difficult codifying "One True Workflow" because
> there are excellent justifications for nearly every variation. 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...
>
>
>> On Fri, Jan 22, 2016 at 12:25 AM, 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 further after
>>> getting review (or I know a piece of work is incomplete and further parts
>>> will be forthcoming).
>>>
>>> Perhaps this needs an opt-in flag on the input side?
>>>
>>> "r?, and land it if you're happy" versus "r?, but I'll take care of
>>> landing"?
>>>
>>>
>>> ___
>>> 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

Re: Just Autoland It

[Ehsan Akhgari]

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 reviewer I'm
not going to land it no matter what. If it's simple and self-contained,
why not?



There is no obvious indication in mozreview whether a patch is
self-contained or has dependencies, is there?

Sometimes one can make a good guess, of course.



Not currently, no.

However, we know from the repository DAG what the "dependencies" are and we
done want to eventually surface these in MozReview by moving the review
request grouping in MozReview to something that is more DAG based instead
of bug-based.


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, as another example, what if the patch needs 
to be checked in only after another patch (perhaps done by a different 
author) that is not connected to the review at hand?


(Both of these examples are from my real workflow in the past couple of 
weeks or so.)


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Gregory Szorc]

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 no matter what. If it's simple and self-contained,
>> why not?
>>
>
> There is no obvious indication in mozreview whether a patch is
> self-contained or has dependencies, is there?
>
> Sometimes one can make a good guess, of course.


Not currently, no.

However, we know from the repository DAG what the "dependencies" are and we
done want to eventually surface these in MozReview by moving the review
request grouping in MozReview to something that is more DAG based instead
of bug-based.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Armen Zambrano G.]

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,
Armen
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Jared Wein]

If a patch only touches JS and CSS, could tryserver use the archive build
implementation to generate faster try builds?

On Fri, Jan 22, 2016 at 2:16 AM, Gregory Szorc  wrote:

> 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
>> >
>>
>
>
> ___
> 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

Re: Just Autoland It

[Mark Hammond]

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) related - what exactly is the "checkin?" flag for? No one 
I've asked before seems to know, but it seems relevant.


Mark
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Marco Bonardo]

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 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 reviewer.
>
> Paolo
>
> ___
> 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

Re: Just Autoland It

[Andrew Halberstadt]

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


I often ask for review first and only do a try run after I've gotten
r+ and made any necessary changes. I also often make small changes
after obtaining review (e.g. minor comment fix-ups). Both of these
behaviours are incompatible with reviewer-initiated landing.

Maybe I'm unusual. But I can definitely understand why people are startled.


I do this too, I don't think it's unusual. Another case is when there
are multiple dependent bugs that need to land in a specific order, and
you flag them all for review at the same time for parallelism.

I was concerned at first as well, but after thinking about it some more,
this seems like something that will sort itself out automatically. There
may be a few miscommunications that lead to backouts at first, but in
the end either:

1. Patch authors will amend their workflows with the assumption that r?
== ready to land.

2. Specific teams/individuals will come to an agreement not to land each
others' patches. In this case if you need a review from someone outside
your team and you don't want them to land, a simple "Please do not land
yet" is likely good enough.

-Andrew
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Boris Zbarsky]

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 indication in mozreview whether a patch is 
self-contained or has dependencies, is there?


Sometimes one can make a good guess, of course.

-Boris

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Andrew Halberstadt]

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 self-contained,
why not?


There is no obvious indication in mozreview whether a patch is
self-contained or has dependencies, is there?

Sometimes one can make a good guess, of course.

-Boris


Yeah not currently, you'd have to look at the bug. That might be a good
feature request. On the other hand, I'd feel uncomfortable landing
something for someone without looking at bug context anyway.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Dave Townsend]

Should we just add a "and land it" checkbox to the review page, maybe
disabled if there are still open issues?

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

Re: Just Autoland It

[Gregory Szorc]

> On Jan 21, 2016, at 19:00, Dave Townsend  wrote:
> 
> Should we just add a "and land it" checkbox to the review page, maybe
> disabled if there are still open issues?

We talked about improving the review finalization overlay window at our meeting 
today and this is definitely one of the improvements we want to make. Look for 
significant changes to this UI in the weeks ahead.

> 
>> On Thu, Jan 21, 2016 at 6:35 PM, 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
> ___
> 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

Re: Just Autoland It

[Gregory Szorc]

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

Re: Just Autoland It

[Richard Newman]

>
> 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 parts
will be forthcoming).

Perhaps this needs an opt-in flag on the input side?

"r?, and land it if you're happy" versus "r?, but I'll take care of
landing"?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Gregory Szorc]

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 further after 
> getting review (or I know a piece of work is incomplete and further parts 
> will be forthcoming).
> 
> Perhaps this needs an opt-in flag on the input side?
> 
> "r?, and land it if you're happy" versus "r?, but I'll take care of landing"? 

I agree there can be ambiguity. I'd like to say that in the ideal world a 
reviewer can make a determination if something is suitable for landing. This 
optionally includes autoland looking at try results and only landing if green 
(or even "landing" to try first as part of the landing). But I know that's not 
perfect.

This boils down to communicating readiness from author to reviewer. We could 
put something in the commit message to reflect unsuitability for landing. 
"DONTLAND" or some such. Perhaps we could leverage the feedback flag for 
indicating "don't land" as well. "f?gps." We know we need to integrate feedback 
flag support into MozReview... We could also invent a new commit message syntax 
with implications for autoland. "rl?gps" for "review + land." I'm just throwing 
out ideas here. But I feel somewhat strongly we should default to (or at least 
encourage through UI) "land by default" (at least in the long term) as it is 
the most streamlined workflow.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Gregory Szorc]

> 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 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!
> 
> Just to be sure, this only happens if the drive-by does "ship-it",
> right?

Correct. And there might also be LDAP group permissions checks involved as well 
(for hysterical raisins that we're revisiting).

At some point we may want to hook up a more formal "reviewers database" to 
identify "appropriate" reviewers. Let's try not get into that bag of worms just 
yet...
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[Mike Hommey]

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 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!

Just to be sure, this only happens if the drive-by does "ship-it",
right?

Mike
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Just Autoland It

[David Rajchenbach-Teller]

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.

Cheers,
 David

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

Re: Just Autoland It

[Mike Connor]

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 to make it simple to adopt, and demonstrably better than the old
way.  Developers will migrate over time as they see the light.

I think this is an easy fix if we're willing to modify our patch submission
workflow to reflect a wider set of asks and responses.  It's more or less
the same mental model as the multi-state flags we use for tracking, there's
more than one type of request, and more than one possible response.

Simple proposal: replace review/feedback flags with a new,
multiple-requestable flag.  Possible values could be:

feedback?
review?
land?
feedback+
withcomments+
review+
land+

Patch authors would be able to opt into the easier flow by setting "land?"
instead of
"review?"  "review?" and "feedback?" would retain their current
definitions.  Patch reviewers would be able to respond explicitly with
feedback, a conditional r+, full r+, or land+.

* Anything where land+ is set would trigger autoland.
* land+ should be set only if requested, or if the reviewer believes that's
expected
* patch authors would be allowed to autoland with review+
* withcomments+ or feedback+ would not allow autoland.

In the short term, we could experiment with this as an additional patch
flag (land?/+/-).  I think the multi-state flag reflects current workflow
reality and eliminates nuance, so should be explored.

-- Mike


On Fri, Jan 22, 2016 at 12:25 AM, 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 further after
> getting review (or I know a piece of work is incomplete and further parts
> will be forthcoming).
>
> Perhaps this needs an opt-in flag on the input side?
>
> "r?, and land it if you're happy" versus "r?, but I'll take care of
> landing"?
>
>
> ___
> 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

Re: Just Autoland It

[Gregory Szorc]

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
> people or unilaterally imposing changes to their workflow.  The best way to
> do this is to make it simple to adopt, and demonstrably better than the old
> way.  Developers will migrate over time as they see the light.
>
> I think this is an easy fix if we're willing to modify our patch
> submission workflow to reflect a wider set of asks and responses.  It's
> more or less the same mental model as the multi-state flags we use for
> tracking, there's more than one type of request, and more than one possible
> response.
>
> Simple proposal: replace review/feedback flags with a new,
> multiple-requestable flag.  Possible values could be:
>
> feedback?
> review?
> land?
> feedback+
> withcomments+
> review+
> land+
>

>
> Patch authors would be able to opt into the easier flow by setting "land?"
> instead of
> "review?"  "review?" and "feedback?" would retain their current
> definitions.  Patch reviewers would be able to respond explicitly with
> feedback, a conditional r+, full r+, or land+.
>
> * Anything where land+ is set would trigger autoland.
> * land+ should be set only if requested, or if the reviewer believes
> that's expected
> * patch authors would be allowed to autoland with review+
> * withcomments+ or feedback+ would not allow autoland.
>
> In the short term, we could experiment with this as an additional patch
> flag (land?/+/-).  I think the multi-state flag reflects current workflow
> reality and eliminates nuance, so should be explored.
>
>
We've already talked about changing the Bugzilla flags to better express
intent. The BMO gurus say it isn't something we should undertake lightly.
It's much easier to experiment on the commit message / MozReview side of
things while leaving the existing Bugzilla flags in place. e.g. we can have
MozReview convert "land?" in commit messages to "review? flags in Bugzilla
while having autoland convert "land?" into "r=" upon landing.

It's worth noting that part of the reason we haven't enabled feedback flag
support and done more advanced things with commit messages is because
seemingly everyone practices slight variations of interactions with
Bugzilla flags and it's difficult codifying "One True Workflow" because
there are excellent justifications for nearly every variation. 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...


> On Fri, Jan 22, 2016 at 12:25 AM, 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 further after
>> getting review (or I know a piece of work is incomplete and further parts
>> will be forthcoming).
>>
>> Perhaps this needs an opt-in flag on the input side?
>>
>> "r?, and land it if you're happy" versus "r?, but I'll take care of
>> landing"?
>>
>>
>> ___
>> 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