The future of commit access policy for core Firefox

[Mike Connor]

(please direct followups to dev-planning, cross-posting to governance,
firefox-dev, dev-platform)


Nearly 19 years after the creation of the Mozilla Project, commit access
remains essentially the same as it has always been. We've evolved the
vouching process a number of times, CVS has long since been replaced by
Mercurial & others, and we've taken some positive steps in terms of
securing the commit process. And yet we've never touched the core idea of
granting developers direct commit access to our most important
repositories. After a large number of discussions since taking ownership
over commit policy, I believe it is time for Mozilla to change that
practice.

Before I get into the meat of the current proposal, I would like to outline
a set of key goals for any change we make. These goals have been informed
by a set of stakeholders from across the project including the engineering,
security, release and QA teams. It's inevitable that any significant
change will disrupt longstanding workflows. As a result, it is critical
that we are all aligned on the goals of the change.


I've identified the following goals as critical for a responsible commit
access policy:


- Compromising a single individual's credentials must not be sufficient
to land malicious code into our products.
- Two-factor auth must be a requirement for all users approving or
pushing a change.
- The change that gets pushed must be the same change that was approved.
- Broken commits must be rejected automatically as a part of the commit
process.


In order to achieve these goals, I propose that we commit to making the
following changes to all Firefox product repositories:


- Direct commit access to repositories will be strictly limited to
sheriffs and a subset of release engineering.
- Any direct commits by these individuals will be limited to fixing
bustage that automation misses and handling branch merges.
- All other changes will go through an autoland-based workflow.
- Developers commit to a staging repository, with scripting that
connects the changeset to a Bugzilla attachment, and integrates
with review
flags.
- Reviewers and any other approvers interact with the changeset as
today (including ReviewBoard if preferred), with Bugzilla flags as the
canonical source of truth.
- Upon approval, the changeset will be pushed into autoland.
- If the push is successful, the change is merged to mozilla-central,
and the bug updated.

I know this is a major change in practice from how we currently operate,
and my ask is that we work together to understand the impact and concerns.
If you find yourself disagreeing with the goals, let's have that discussion
instead of arguing about the solution. If you agree with the goals, but
not the solution, I'd love to hear alternative ideas for how we can achieve
the outcomes outlined above.

-- Mike

[Bobby Holley]

At a high level, I think the goals here are good.

However, the tooling here needs to be top-notch for this to work, and the
standard approach of flipping on an MVP and dealing with the rest in
followup bugs isn't going to be acceptable for something so critical to our
productivity as landing code. The only reason more developers aren't
complaining about the MozReview+autoland workflow right now is that it's
optional.

The busiest and most-productive developers (ehsan, bz, dbaron, etc) tend
not to pay attention to new workflows because they have too much else on
their plate. The onus needs to be on the team deploying this to have the
highest-volume committers using the new system happily and voluntarily
before it becomes mandatory. That probably means that the team should not
have any deadline-oriented incentives to ship it before it's ready.

Also, getting rid of "r+ with comments" is a non-starter.

bholley

> _______________________________________________
> dev-planning mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-planning
>

[Eric Rescorla]

First, let me state that I am generally in support of this type of change.

More comments below.

On Thu, Mar 9, 2017 at 1:53 PM, Mike Connor <mco...@mozilla.com> wrote:

> (please direct followups to dev-planning, cross-posting to governance,
> firefox-dev, dev-platform)
>
>
> Nearly 19 years after the creation of the Mozilla Project, commit access
> remains essentially the same as it has always been. We've evolved the
> vouching process a number of times, CVS has long since been replaced by
> Mercurial & others, and we've taken some positive steps in terms of
> securing the commit process. And yet we've never touched the core idea of
> granting developers direct commit access to our most important
> repositories. After a large number of discussions since taking ownership
> over commit policy, I believe it is time for Mozilla to change that
> practice.
>
> Before I get into the meat of the current proposal, I would like to
> outline a set of key goals for any change we make. These goals have been
> informed by a set of stakeholders from across the project including the
> engineering, security, release and QA teams. It's inevitable that any
> significant change will disrupt longstanding workflows. As a result, it is
> critical that we are all aligned on the goals of the change.
>
>
> I've identified the following goals as critical for a responsible commit
> access policy:
>
>
> - Compromising a single individual's credentials must not be
> sufficient to land malicious code into our products.
>

> I think this is a good long-term goal, but I don't think it's one we need
to achieve now.
At the moment, I would settle for "only a few privileged people can
single-handedly
land malicious code into our products". In support of this, I would note
that what you
propose below is insufficient to achieve your stated objective, because
there will
still be single person admin access to machines.

> - Two-factor auth must be a requirement for all users approving or
> pushing a change.
> - The change that gets pushed must be the same change that was
> approved.
> - Broken commits must be rejected automatically as a part of the
> commit process.
>
>
> In order to achieve these goals, I propose that we commit to making the
> following changes to all Firefox product repositories:
>
>
> - Direct commit access to repositories will be strictly limited to
> sheriffs and a subset of release engineering.
> - Any direct commits by these individuals will be limited to fixing
> bustage that automation misses and handling branch merges.
>

> I think this is a good eventual goal, but in the short term, I think it's
probably useful
to keep checkin-needed floating around, especially given the somewhat iffy
state
of the toolchain.

>
> - All other changes will go through an autoland-based workflow.
> - Developers commit to a staging repository, with scripting that
> connects the changeset to a Bugzilla attachment, and integrates with review
> flags.
> - Reviewers and any other approvers interact with the changeset as
> today (including ReviewBoard if preferred), with Bugzilla flags as the
> canonical source of truth.
> - Upon approval, the changeset will be pushed into autoland.
>

> Implicit in this is some sort of hierarchy of reviewers (tied to what was
previous L3 commit?)
that says who can review a patch. Otherwise, I can just create a dummy
account, r+ my own
patch, and Land Ho! IIRC Chromium does this by saying that LGTM implies
autolanding
only if the reviewer could have landed the code themselves.

-Ekr

> - If the push is successful, the change is merged to mozilla-central,
> and the bug updated.
>
> I know this is a major change in practice from how we currently operate,
> and my ask is that we work together to understand the impact and concerns.
> If you find yourself disagreeing with the goals, let's have that discussion
> instead of arguing about the solution. If you agree with the goals, but
> not the solution, I'd love to hear alternative ideas for how we can achieve
> the outcomes outlined above.
>
> -- Mike
>
> _______________________________________________

> firefox-dev mailing list
> firef...@mozilla.org
> https://mail.mozilla.org/listinfo/firefox-dev
>
>

[chris.ry...@gmail.com]

How will uplifts work? Can only sheriffs land them?

This would do-away with "r+ with comments addressed". Reviewers typically only say this for patches submitted by people they trust. So removing "r+ with comments" would mean reviewers would need to re-review code, causing an extra delay and extra reviewing load. Is there some way we can keep the "r+ with comments addressed" behaviour for trusted contributors?

[Eric Rescorla]

One potential approach would be to keep it for contributors who themselves
were L3
committers in the current framework (and met a similar bar in the future).

The way to rationalize this is that those people could always set up a sock
puppet
account, submit a patch, and then review it themselves and it would be
landed,
so the policy is ultimately "sign off by one L3 committer"

-Ekr


>
> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

[Justin Dolske]

Similar to "r+ with fixes" are cases where a patch (or patch series)
requires reviews from a multitude of people. Which means variable delays in
getting reviews, during which things may slightly bitrot or be trivially
modified based on feedback from other reviewers. Code refactorings are one
example of this. Having to re-request review from everyone, perhaps
multiple times, would seem pretty painful. :)

>From previous discussions, it sounded like there was a middle ground in
still requiring all landings to go through automation (i.e., a strong link
between what actually landed and what was posted in the bug), but allow
some slop in what was formally reviewed (i.e., a weak link between last
review and what's was asked to land.) The automation might at least require
that you can't land a "r=sparky" unless "sparky" did at some point grant
r+. At the very least, this makes it somewhat easier to compare what landed
with what was last reviewed (with current tools, thanks to ReviewBoard's
revision diffing).

I suppose this policy also implies no more "Typo fix, no bug" landings. But
I never really liked those anyway, so I'm fine with that. Bugs are cheap! :)

Justin

On Thu, Mar 9, 2017 at 3:11 PM, <chris.ry...@gmail.com> wrote:

[Mike Hommey]

On Thu, Mar 09, 2017 at 03:51:07PM -0800, Justin Dolske wrote:
> I suppose this policy also implies no more "Typo fix, no bug" landings. But
> I never really liked those anyway, so I'm fine with that. Bugs are cheap! :)

Considering the overhead of creating a bug, attaching a patch,
requesting a review, landing, vs. just landing typo fixes, it's hard to
say bugs are cheap. (plus, you now have 2 persons involved instead of
one)

Mike

[Ehsan Akhgari]

On 2017-03-09 6:51 PM, Justin Dolske wrote:
> Similar to "r+ with fixes" are cases where a patch (or patch series)
> requires reviews from a multitude of people. Which means variable delays in
> getting reviews, during which things may slightly bitrot or be trivially
> modified based on feedback from other reviewers. Code refactorings are one
> example of this. Having to re-request review from everyone, perhaps
> multiple times, would seem pretty painful. :)

Even with a single reviewer, I often times end up making some trivial
changes to my patches to fix stupid mistakes and issues that I know the
reviewer doesn't care enough to want to look at before landing. In
general our code review process has a lot of flexibility built into it,
and reviewers generally understand that the goal ultimately is to ensure
the quality of the produced code, so depending on the circumstances as a
reviewer I can treat a patch on different levels of scrutiny, from
anywhere between checking the actual landed patch and complaining if
something wasn't done in the way I asked to r+ing asking for a lot of
changes and trusting the author will do the right thing without needing
me look over their work more.

> From previous discussions, it sounded like there was a middle ground in
> still requiring all landings to go through automation (i.e., a strong link
> between what actually landed and what was posted in the bug),

Which, to some extent, goes against the flexibility of an author when
they're trusted like in the above scenario. :( This is pretty
disrupting to my personal productivity, for whatever that's worth (but
I'll of course deal with it if I have to). I just don't understand why
this is desirable as a goal.

> but allow
> some slop in what was formally reviewed (i.e., a weak link between last
> review and what's was asked to land.) The automation might at least require
> that you can't land a "r=sparky" unless "sparky" did at some point grant
> r+. At the very least, this makes it somewhat easier to compare what landed
> with what was last reviewed (with current tools, thanks to ReviewBoard's
> revision diffing).

FTR we have real actual experience with this: for years I have had to
append a=me etc to work around various hooks. I don't understand how
something in the commit message can help here, if for example the
requirement is for the reviewer to have to r+ the thing that gets
autolanded every time.

> I suppose this policy also implies no more "Typo fix, no bug" landings.

That is also unacceptable.

> But
> I never really liked those anyway, so I'm fine with that. Bugs are cheap! :)
>

> Justin
>
> On Thu, Mar 9, 2017 at 3:11 PM, <chris.ry...@gmail.com> wrote:
>

[dand...@mozilla.com]

On Thursday, March 9, 2017 at 1:53:50 PM UTC-8, Mike Connor wrote:
>
> - Direct commit access to repositories will be strictly limited to
> sheriffs and a subset of release engineering.
> - Any direct commits by these individuals will be limited to fixing
> bustage that automation misses and handling branch merges.
> - All other changes will go through an autoland-based workflow.
> - Developers commit to a staging repository, with scripting that
> connects the changeset to a Bugzilla attachment, and integrates
> with review
> flags.
> - Reviewers and any other approvers interact with the changeset as
> today (including ReviewBoard if preferred), with Bugzilla flags as the
> canonical source of truth.
> - Upon approval, the changeset will be pushed into autoland.
> - If the push is successful, the change is merged to mozilla-central,
> and the bug updated.

Requiring secure accounts, and not being able to land bustage, sound like good goals. But these proposals sound like a lot of serious productivity-hampering restrictions. It's not explained what actual problems are being solved, or why our current system fails to meet these goals and needs drastic changes.

[Edmund Wong]

Aside for the first one, the other items seem to be mere
'implementation/application details', rather than actual goals.

- Protect Firefox repositories to the best of our abilities and
resources availability by implementing a set of rules and factors
to prevent unauthorized access/modifications to the core content.

> In order to achieve these goals, I propose that we commit to making the
> following changes to all Firefox product repositories:

By all Firefox product repositories, I'm assuming mainly m-*,
and integration/m-i?

And with this new proposed process, this means the doing away
of integration/m-i as it would be superfluous if an autoland-esque
repo is used.

What about other repositories? Tier 2, etc.. i.e. comm-*?

>
>
> - Direct commit access to repositories will be strictly limited to
> sheriffs and a subset of release engineering.

> - Any direct commits by these individuals will be limited to fixing
> bustage that automation misses and handling branch merges.
> - All other changes will go through an autoland-based workflow.
> - Developers commit to a staging repository, with scripting that
> connects the changeset to a Bugzilla attachment, and integrates
> with review
> flags.

So m-i is obsoleted. Autoland is the defacto push repo?

1) Developer submits a patch to bugzilla or reviewboard
2) it gets reviewed and/or approved
3) it then gets pushed to autoland [tbh, I don't know how autoland
works, so does someone push to autoland, or does bugzilla do it
for us? -rhetorical question.. can find out off-list]
4) sheriffs watch the autoland tree and backout whatever bustages
happen and every so often, they merge autoland to m-c. If
patches need to be uplifted, they are done by sheriffs.

So we're pretty much back to the similar vein of m-* and
mozilla-inbound (except now, it's autoland).

Is this the gist of it?

Edmund

[zbran...@mozilla.com]

As others stated, the idea that patch cannot be altered after r+ has a massive effect on productivity. I can't overstate how much it would impact day-to-day work for engineers, and I don't really see an easy way out.

Even if we added "approval to land with minor changes" there's a) no way to distinguish minor fro major, and b) reviewers will either start using it as a default, or keep forgetting about it.

I like the direction, but I honestly believe that this single idea would make working with Gecko a massive PITA.
With autoland my path to central from when I get all the required reviews is already ~24h because I push the "land" button around 2pm PST and it gets merged into central around 3am, so I can only follow-up the next day.

I recently introduced a regression not caught by me, my reviewer or tests. It wasn't major enough to warrant panic mode, but I'm sure it irritated people with spawned warnings and of course it has some impact on our nightly users.
I landed the follow up within 20 minutes of discovering the bug, but since it wen't through autoland, it took two nightly builds and a full day before users stopped reporting dups of the bug.

Now, if you add to that, that every minor change I make after my reviewer approved my patch I need to get a re-review (and most of my reviewers are in a different timezone), it'll basically at the very best add just another 24h to the cycle.
If it's Friday, or my reviewer is busy with other stuff or on PTO, it'll add a couple days.

zb.

[Mike Hommey]

While we're talking about drag on productivity, there's already one that
comes from autoland already, which is that you can't easily land fixups
for stupid mistakes (like, a build failure on one platform, or other
lame things that happen when things land).

You either have to get a sheriff to land the fixup for you on autoland,
or to back you out, in which case you're back to square one and need to
reland the whole thing.

If on top of that, you also need another round of reviews...

Mike

[Masatoshi Kimura]

On 2017/03/10 6:53, Mike Connor wrote:
> - Two-factor auth must be a requirement for all users approving or
> pushing a change.

I have no mobile devices. How can I use 2FA?

Previously I was suggested to buy a new PC and SSD only to shorten the
build time. Now do I have to buy a smartphone only to contribute
Mozilla? What's the next?

--
VYV0...@nifty.ne.jp

[Masatoshi Kimura]

On 2017/03/10 14:00, Mike Hommey wrote:
> While we're talking about drag on productivity, there's already one that
> comes from autoland already, which is that you can't easily land fixups
> for stupid mistakes (like, a build failure on one platform, or other
> lame things that happen when things land).
>
> You either have to get a sheriff to land the fixup for you on autoland,
> or to back you out, in which case you're back to square one and need to
> reland the whole thing.
>
> If on top of that, you also need another round of reviews...

Yes, it is really frustrating and wasting bandwidth of both developers
and shriffs. It is one of reasons I dislike mozreview/autoland.

[jw...@mozilla.com]

Fwiw, Yubikey's are generally suitable 2FA devices, and are significantly cheaper than smartphones. I'd not be surprised if key contributors who need commit rights were able to apply to mozilla in order to acquire a yubikey should they be unable to procure on themselves. (I'm not a policy maker, so do NOT take my statement as any sort of promise)

~Justin Wood (Callek)

[Frank-Rainer Grahl]

Me too. Have a phone which does what I need (calling people and be called:) )
No need for a mobile device which has its own security and usability problems.

FRG

[Frank-Rainer Grahl]

Ehsan Akhgari wrote:
> Even with a single reviewer, I often times end up making some trivial
> changes to my patches to fix stupid mistakes and issues that I know the
> reviewer doesn't care enough to want to look at before landing. In
> general our code review process has a lot of flexibility built into it,
> and reviewers generally understand that the goal ultimately is to ensure
> the quality of the produced code, so depending on the circumstances as a
> reviewer I can treat a patch on different levels of scrutiny, from
> anywhere between checking the actual landed patch and complaining if
> something wasn't done in the way I asked to r+ing asking for a lot of
> changes and trusting the author will do the right thing without needing
> me look over their work more.

...

Same here. Automation is fine if everything goes according to plan but pushing
manually is much less time consuming if something goes wrong e.g. a patch
needs trivial changes to un-bitrot it. So there should still be a way to just
push manually if needed or desired.

FRG

[Eric Rescorla]

On Fri, Mar 10, 2017 at 7:23 PM, smaug via governance <
gover...@lists.mozilla.org> wrote:

> On 03/10/2017 12:59 AM, Bobby Holley wrote:
>
>> At a high level, I think the goals here are good.
>>
>> However, the tooling here needs to be top-notch for this to work, and the
>> standard approach of flipping on an MVP and dealing with the rest in
>> followup bugs isn't going to be acceptable for something so critical to
>> our
>> productivity as landing code. The only reason more developers aren't
>> complaining about the MozReview+autoland workflow right now is that it's
>> optional.
>>
>> The busiest and most-productive developers (ehsan, bz, dbaron, etc) tend
>> not to pay attention to new workflows because they have too much else on
>> their plate. The onus needs to be on the team deploying this to have the
>> highest-volume committers using the new system happily and voluntarily
>> before it becomes mandatory. That probably means that the team should not
>> have any deadline-oriented incentives to ship it before it's ready.
>>
>> Also, getting rid of "r+ with comments" is a non-starter.
>>
>

> FWIW, with my reviewer hat on, I think that is not true, _assuming_ there
> is a reliable interdiff for patches.
> And not only MozReview patches but for all the patches. (and MozReview
> interdiff isn't reliable, it has dataloss issues so it
> doesn't really count there.).
> I'd be ok to do a quick r+ if interdiff was working well.
>

Without taking a position on the broader proposal, I agree with this.

We have been using Phabricator for our reviews in NSS and its interdiffs
work pretty well
(modulo rebases, which are not so great), and it's very easy to handle LGTM
with
nits and verify the nits.

-Ekr


>
>
>
> -Olli
>
>
>> bholley

>>> - Two-factor auth must be a requirement for all users approving or
>>> pushing a change.

>>> - The change that gets pushed must be the same change that was
>>> approved.
>>> - Broken commits must be rejected automatically as a part of the
>>> commit
>>> process.
>>>
>>>

>>> In order to achieve these goals, I propose that we commit to making the
>>> following changes to all Firefox product repositories:
>>>
>>>

>>> - Direct commit access to repositories will be strictly limited to
>>> sheriffs and a subset of release engineering.
>>> - Any direct commits by these individuals will be limited to
>>> fixing
>>> bustage that automation misses and handling branch merges.
>>> - All other changes will go through an autoland-based workflow.
>>> - Developers commit to a staging repository, with scripting that
>>> connects the changeset to a Bugzilla attachment, and integrates
>>> with review
>>> flags.

>>> - Reviewers and any other approvers interact with the changeset as
>>> today (including ReviewBoard if preferred), with Bugzilla flags as
>>> the
>>> canonical source of truth.
>>> - Upon approval, the changeset will be pushed into autoland.
>>> - If the push is successful, the change is merged to
>>> mozilla-central,
>>> and the bug updated.
>>>

>>> I know this is a major change in practice from how we currently operate,
>>> and my ask is that we work together to understand the impact and
>>> concerns.
>>> If you find yourself disagreeing with the goals, let's have that
>>> discussion
>>> instead of arguing about the solution. If you agree with the goals, but
>>> not the solution, I'd love to hear alternative ideas for how we can
>>> achieve
>>> the outcomes outlined above.
>>>
>>> -- Mike

>>> _______________________________________________
>>> dev-planning mailing list
>>> dev-pl...@lists.mozilla.org
>>> https://lists.mozilla.org/listinfo/dev-planning
>>>
>>>

> _______________________________________________
> governance mailing list
> gover...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/governance
>

[L. David Baron]

On Friday 2017-03-10 19:33 -0800, Eric Rescorla wrote:
> We have been using Phabricator for our reviews in NSS and its interdiffs
> work pretty well
> (modulo rebases, which are not so great), and it's very easy to handle LGTM
> with
> nits and verify the nits.

For what it's worth, I think proper interdiffs have two columns of
[ -+] at the beginning of each line, not one column like diffs do.
I've gotten used to reading interdiffs as diff -u of a diff -u, and
while it takes a little getting used to, but once you're used to it,
it actually represents what an interdiff is and is quite usable. I
think anything that pretends that something like this:

// Frame has a LayerActivityProperty property
FRAME_STATE_BIT(Generic, 54, NS_FRAME_HAS_LAYER_ACTIVITY_PROPERTY)

+ // Frame owns anonymous boxes whose style contexts it will need to update during
+ // a stylo tree traversal.
+ FRAME_STATE_BIT(Generic, 55, NS_FRAME_OWNS_ANON_BOXES)
+
+// If this bit is set, then reflow may be dispatched from the current
+// frame instead of the root frame.
-+FRAME_STATE_BIT(Generic, 55, NS_FRAME_DYNAMIC_REFLOW_ROOT)
++FRAME_STATE_BIT(Generic, 56, NS_FRAME_DYNAMIC_REFLOW_ROOT)
+
// Set for all descendants of MathML sub/supscript elements (other than the
// base frame) to indicate that the SSTY font feature should be used.
FRAME_STATE_BIT(Generic, 58, NS_FRAME_MATHML_SCRIPT_DESCENDANT)

can be represented with only one column of [+- ] at the beginning
is going to fail for some substantial set of important cases (such
as rebases, as you mention).

(That's a piece of interdiff from rebasing my own patch queue
earlier this week.)

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

[Nicholas Nethercote]

On Sat, Mar 11, 2017 at 2:23 PM, smaug via governance <
gover...@lists.mozilla.org> wrote:
>
> I'd be ok to do a quick r+ if interdiff was working well.

Depending on the relative timezones of the reviewer and reviewee, that
could delay landing by 24 hours or even a whole weekend.

In general there seems to be a large amount of support in this thread for
continuing to allow the r+-with-minor-fixes option.

Nick

[Gabor Krizsanits]

On Sat, Mar 11, 2017 at 7:23 AM, Nicholas Nethercote <n.neth...@gmail.com
> wrote:

>
> Depending on the relative timezones of the reviewer and reviewee, that
> could delay landing by 24 hours or even a whole weekend.
>
>

As someone working from Europe and 90% of the time with people from the
West Coast, thank
you very much for bringing up the timezone argument. r+-with-minor-fixes is
an absolute must for
some of us.

Gabor

[smaug]

On 03/11/2017 08:23 AM, Nicholas Nethercote wrote:
> On Sat, Mar 11, 2017 at 2:23 PM, smaug via governance <
> gover...@lists.mozilla.org> wrote:
>>
>> I'd be ok to do a quick r+ if interdiff was working well.
>
> Depending on the relative timezones of the reviewer and reviewee, that
> could delay landing by 24 hours or even a whole weekend.
>

The final r+, if it is just cosmetic changes wouldn't need to be done by the same reviewer.

Perhaps we shouldn't even call the last step a review. It would be "ok-to-land".
r+ without asking any changes would implicitly contain that "ok-to-land".
(if rebasing causes some changes, that would then need explicit "ok-to-land")

> In general there seems to be a large amount of support in this thread for
> continuing to allow the r+-with-minor-fixes option.

Yeah. I don't object that, but I also think that having final approval to land the patch might not really be that bad
(assuming the tools are working well enough).

>
> Nick
>

[gsqu...@mozilla.com]

If we really want to enforce a final review to prevent unwanted code to land, Mozilla (as a whole) will incur some costs: Reviewers spending time re-reviewing; delays to land (worse across tz, and which could result in the need to rebase again, and therefore another round of ok-to-land); and mounting anger at all these papercuts.

So if Mozilla is really committed to this solution, and is ready to bear the associated financial costs, I would suggest we recruit people who would do just these tasks! Could be existing sheriffs, and/or volunteering peers, and/or new staff with distinct job descriptions.

Hopefully they'd rubber-stamp 99% of last-minute changes, and only the more complex changes would be referred back to the author and reviewer.
As a bonus they could also handle trivial autoland merge issues.

In the end it should cost a similar amount of money, but would lessen the other costs (delays and frustration).


And while I've got the mic, a small suggestion for mozreview to help with some of this: We need a way for the author to add a comment explaining what was done between pushes (rebase, nit-fixing, other notes to reviewer, etc.); this comment would only appear in Bugzilla and mozreview, not in the actual patch commit description.
(Could be a paragraph starting with "notes-to-reviewer:", to be removed before autolanding.)

- Gerald

[Cameron Kaiser]

I actually use a Perl script to do HOTP. And there are things like
Firekey. No mobile device necessary.

Cameron Kaiser
tier-3-2-1-contact!

[Eric Rescorla]

The systems I am most familiar with (Phabricator, Rietveld), present
interdiffs
by allowing you to look at the diff between any revision of the CL
(including
the base revision that's the code as-is). This works pretty well for
anything other
than rebases (and is actually rather equivalent to the UI you get with
Github when people update PRs by pushing new commits onto the branch).

What sort of UI you want for rebases seems like a bit more of an open
question. i can imagine several things:

1. For simple rebases (where there's not much interaction with the
surrounding
code, you probably just want to see the patch in context as if the rebase
hadn't
happened.
2. For more complicated rebases (where there is real interaction with the
code that was rebased onto), you want to see the difference between
base1 + CL1 and base2 + CL2, but with the diffs that are due to the
rebase distinguished from the ones that are due to the CL1-CL2
difference (this what Phabricator does by marking the rebase artifacts
as lighter).
3. The design you suggest (which, at least for me, seems like it's
harder to read than designs where the tools provide more support).

It seems like designs here are evolving and it's probably going to be a
matter of personal taste, at least for a while

-Ekr

[Ehsan Akhgari]

On 2017-03-11 9:23 AM, smaug via governance wrote:
> On 03/11/2017 08:23 AM, Nicholas Nethercote wrote:
>> On Sat, Mar 11, 2017 at 2:23 PM, smaug via governance <
>> gover...@lists.mozilla.org> wrote:
>>>
>>> I'd be ok to do a quick r+ if interdiff was working well.
>>
>> Depending on the relative timezones of the reviewer and reviewee, that
>> could delay landing by 24 hours or even a whole weekend.
>>
> The final r+, if it is just cosmetic changes wouldn't need to be done by
> the same reviewer.

OK, but what is the exact time zone efficient way to ensure that no huge
amount of delay is added for the turn around of this final review round?

Let's be realistic. In practice, adding one extra person in the process
of landing of the patches that currently land with r+-with-nits *will*
slow them down. And we should expect that it will slow them down by
factors such as time zones, people missing bugmail, etc, all of the
reasons why currently one review can end up taking a week, it may end up
being that final round of review after this proposed change.

And I still don't understand what the proposal means with rebases in
practice. What if, after automation tries to land your change after you
got your final r+ the final rebase fails and you need to do a manual
rebase? Do you need to get another r+? What happens if you're touching
one of our giant popular headers and someone else manages to land a
conflict while your reviewer gets back to you?

> Perhaps we shouldn't even call the last step a review. It would be
> "ok-to-land".
> r+ without asking any changes would implicitly contain that "ok-to-land".
> (if rebasing causes some changes, that would then need explicit
> "ok-to-land")

Are you suggesting a change to the nature of the review process for the
last step with the rename?

[smaug]

On 03/12/2017 10:40 PM, Ehsan Akhgari wrote:
> On 2017-03-11 9:23 AM, smaug via governance wrote:
>> On 03/11/2017 08:23 AM, Nicholas Nethercote wrote:
>>> On Sat, Mar 11, 2017 at 2:23 PM, smaug via governance <
>>> gover...@lists.mozilla.org> wrote:
>>>>
>>>> I'd be ok to do a quick r+ if interdiff was working well.
>>>
>>> Depending on the relative timezones of the reviewer and reviewee, that
>>> could delay landing by 24 hours or even a whole weekend.
>>>
>> The final r+, if it is just cosmetic changes wouldn't need to be done by
>> the same reviewer.
>
> OK, but what is the exact time zone efficient way to ensure that no huge
> amount of delay is added for the turn around of this final review round?
>
> Let's be realistic. In practice, adding one extra person in the process
> of landing of the patches that currently land with r+-with-nits *will*
> slow them down. And we should expect that it will slow them down by
> factors such as time zones, people missing bugmail, etc,

Why we need to expect that? In my mind the process would be closer to
"I want to land this patch, asking ok-to-land on irc." And then someone who has the rights to say ok to that would
mark the patch after seeing that the interdiff doesn't add anything inherently bad.
(well working interdiff and tooling in general is rather critical)

> all of the
> reasons why currently one review can end up taking a week,

I see this being _very_ different to normal reviews.

I do understand that people don't want extra process. Overhead is always overhead.
But the overhead here might not be as bad as feared.

> it may end up
> being that final round of review after this proposed change.
>
> And I still don't understand what the proposal means with rebases in
> practice. What if, after automation tries to land your change after you
> got your final r+ the final rebase fails and you need to do a manual
> rebase? Do you need to get another r+? What happens if you're touching
> one of our giant popular headers and someone else manages to land a
> conflict while your reviewer gets back to you?
>
>> Perhaps we shouldn't even call the last step a review. It would be
>> "ok-to-land".
>> r+ without asking any changes would implicitly contain that "ok-to-land".
>> (if rebasing causes some changes, that would then need explicit
>> "ok-to-land")
>
> Are you suggesting a change to the nature of the review process for the
> last step with the rename?

The last step here would be quite different to normal reviews. It is just a rather quick glance-over that no
obviously evil code is about to land. (as I said, well working interdiff is absolutely critical to make this working)

[Frederik Braun]

Doesn't that defeat the point of a second factor?

>
> Cameron Kaiser
> tier-3-2-1-contact!

[Eric Rescorla]

On Mon, Mar 13, 2017 at 12:22 AM, Frederik Braun <fbr...@mozilla.com> wrote:

> On 12.03.2017 04:08, Cameron Kaiser wrote:

> Doesn't that defeat the point of a second factor?
>

Not entirely, no, because it is not replayable.

-Ekr

[smaug]

On 03/12/2017 10:40 PM, Ehsan Akhgari wrote:

> On 2017-03-11 9:23 AM, smaug via governance wrote:
>> On 03/11/2017 08:23 AM, Nicholas Nethercote wrote:
>>> On Sat, Mar 11, 2017 at 2:23 PM, smaug via governance <
>>> gover...@lists.mozilla.org> wrote:
>>>>
>>>> I'd be ok to do a quick r+ if interdiff was working well.
>>>
>>> Depending on the relative timezones of the reviewer and reviewee, that
>>> could delay landing by 24 hours or even a whole weekend.
>>>
>> The final r+, if it is just cosmetic changes wouldn't need to be done by
>> the same reviewer.
>
> OK, but what is the exact time zone efficient way to ensure that no huge
> amount of delay is added for the turn around of this final review round?
>
> Let's be realistic. In practice, adding one extra person in the process
> of landing of the patches that currently land with r+-with-nits *will*
> slow them down. And we should expect that it will slow them down by
> factors such as time zones, people missing bugmail, etc,

Why we need to expect that? In my mind the process would be closer to
"I want to land this patch, asking ok-to-land on irc." And then someone who has the rights to say ok to that would
mark the patch after seeing that the interdiff doesn't add anything inherently bad.
(well working interdiff and tooling in general is rather critical)

> all of the
> reasons why currently one review can end up taking a week,

I see this being _very_ different to normal reviews.

I do understand that people don't want extra process. Overhead is always overhead.
But the overhead here might not be as bad as feared.

> it may end up
> being that final round of review after this proposed change.
>
> And I still don't understand what the proposal means with rebases in
> practice. What if, after automation tries to land your change after you
> got your final r+ the final rebase fails and you need to do a manual
> rebase? Do you need to get another r+? What happens if you're touching
> one of our giant popular headers and someone else manages to land a
> conflict while your reviewer gets back to you?
>
>> Perhaps we shouldn't even call the last step a review. It would be
>> "ok-to-land".
>> r+ without asking any changes would implicitly contain that "ok-to-land".
>> (if rebasing causes some changes, that would then need explicit
>> "ok-to-land")
>
> Are you suggesting a change to the nature of the review process for the
> last step with the rename?

[smaug]

On 03/11/2017 08:23 AM, Nicholas Nethercote wrote:

> On Sat, Mar 11, 2017 at 2:23 PM, smaug via governance <
> gover...@lists.mozilla.org> wrote:
>>
>> I'd be ok to do a quick r+ if interdiff was working well.
>
> Depending on the relative timezones of the reviewer and reviewee, that
> could delay landing by 24 hours or even a whole weekend.
>

The final r+, if it is just cosmetic changes wouldn't need to be done by the same reviewer.

Perhaps we shouldn't even call the last step a review. It would be "ok-to-land".
r+ without asking any changes would implicitly contain that "ok-to-land".
(if rebasing causes some changes, that would then need explicit "ok-to-land")

> In general there seems to be a large amount of support in this thread for
> continuing to allow the r+-with-minor-fixes option.

[David Burns]

As the manager of the sheriffs, I am in favour of this proposal.

The reasons why are as follow (and to note there are only 3 paid sheriffs
to try cover the world):

* A number of r+ with nits land up in the sheriffs queue for
checkin-needed. This then puts the onus on the sheriffs, not the reviewer
that the right thing has been done. The sheriffs do no have the context
knowledge of the patch, never mind the knowledge of the system being
changed.

* The throughput of patches into the trees is only going to increase. If
there are failures, and the sheriffs need to back out, this can be a long
process depending on the failure leading to pile ups of broken patches.

A number of people have complained that using autoland doesn't allow us to
fail forward on patches. While that is true, there is the ability to do
T-shaped try pushes to make sure that you at least compile on all
platforms. This can easily done from Mozreview (Note: I am not suggesting
we move to mozreview).

Regarding burden on reviewers, the comments in this thread just highlight
how broken our current process is by having to flag individual people for
reviews. This leads to the a handful of people doing 50%+ of reviews on the
code. We, at least visibly, don't do enough to encourage new committers
that would lighten the load to allow the re-review if there are nits. Also,
we need to do work to automate the removal of nits to limit the amount of
re-reviews that reviewer can do.

We should try mitigate the security problem and fix our nit problem instead
of bashing that we can't handle re-reviews because of nits.

David

On 9 March 2017 at 21:53, Mike Connor <mco...@mozilla.com> wrote:

> (please direct followups to dev-planning, cross-posting to governance,
> firefox-dev, dev-platform)
>
>
> Nearly 19 years after the creation of the Mozilla Project, commit access
> remains essentially the same as it has always been. We've evolved the
> vouching process a number of times, CVS has long since been replaced by
> Mercurial & others, and we've taken some positive steps in terms of
> securing the commit process. And yet we've never touched the core idea of
> granting developers direct commit access to our most important
> repositories. After a large number of discussions since taking ownership
> over commit policy, I believe it is time for Mozilla to change that
> practice.
>
> Before I get into the meat of the current proposal, I would like to
> outline a set of key goals for any change we make. These goals have been
> informed by a set of stakeholders from across the project including the
> engineering, security, release and QA teams. It's inevitable that any
> significant change will disrupt longstanding workflows. As a result, it is
> critical that we are all aligned on the goals of the change.
>
>
> I've identified the following goals as critical for a responsible commit
> access policy:
>
>
> - Compromising a single individual's credentials must not be
> sufficient to land malicious code into our products.

> - Two-factor auth must be a requirement for all users approving or
> pushing a change.

> firefox-dev mailing list
> firef...@mozilla.org
> https://mail.mozilla.org/listinfo/firefox-dev
>
>

[Byron Jones]

David Burns wrote:
> We should try mitigate the security problem and fix our nit problem
> instead of bashing that we can't handle re-reviews because of nits.

one way tooling could help here is to allow the reviewer to make minor
changes to the patch before it lands.
ie. "r+, fix typo in comment before landing" would become "r+, i fixed
the comment typo"

--
glob — engineering productivity — moz://a

[James Graham]

On 13/03/17 14:45, Byron Jones wrote:
> David Burns wrote:
>> We should try mitigate the security problem and fix our nit problem
>> instead of bashing that we can't handle re-reviews because of nits.
> one way tooling could help here is to allow the reviewer to make minor
> changes to the patch before it lands.
> ie. "r+, fix typo in comment before landing" would become "r+, i fixed
> the comment typo"
>

Assuming you mean "and land without further review", I don't see how
this has different security properties from r+-with-nits in the —
reasonably common — case that the patch author is at least as trusted as
the reviewer (e.g. both L3 today).

I do think that tooling to support multiple authors collaborating on a
single branch is a good thing independent of the changes discussed in
this thread.

[Ehsan Akhgari]

On 2017-03-12 4:53 PM, smaug wrote:
> On 03/12/2017 10:40 PM, Ehsan Akhgari wrote:
>> On 2017-03-11 9:23 AM, smaug via governance wrote:
>>> On 03/11/2017 08:23 AM, Nicholas Nethercote wrote:
>>>> On Sat, Mar 11, 2017 at 2:23 PM, smaug via governance <
>>>> gover...@lists.mozilla.org> wrote:
>>>>>
>>>>> I'd be ok to do a quick r+ if interdiff was working well.
>>>>
>>>> Depending on the relative timezones of the reviewer and reviewee, that
>>>> could delay landing by 24 hours or even a whole weekend.
>>>>
>>> The final r+, if it is just cosmetic changes wouldn't need to be done by
>>> the same reviewer.
>>
>> OK, but what is the exact time zone efficient way to ensure that no huge
>> amount of delay is added for the turn around of this final review round?
>>
>> Let's be realistic. In practice, adding one extra person in the process
>> of landing of the patches that currently land with r+-with-nits *will*
>> slow them down. And we should expect that it will slow them down by
>> factors such as time zones, people missing bugmail, etc,
>
> Why we need to expect that? In my mind the process would be closer to
> "I want to land this patch, asking ok-to-land on irc." And then someone
> who has the rights to say ok to that would
> mark the patch after seeing that the interdiff doesn't add anything
> inherently bad.
> (well working interdiff and tooling in general is rather critical)

Depending on the timezone, finding people on IRC may be challenging.
For example, many of our colleagues in Taipei may find this kind of IRC
reviews difficult if the people who need to look at the change are on
the other side of the planet.

OK, this distinction is key. To me this seems adding process for the
sake of adding process. Is this really going to be all that much better
at preventing vulnerabilities from sneaking in than what we have today?

[zbran...@mozilla.com]

On Monday, March 13, 2017 at 7:45:44 AM UTC-7, Byron Jones wrote:
> David Burns wrote:
> > We should try mitigate the security problem and fix our nit problem
> > instead of bashing that we can't handle re-reviews because of nits.
> one way tooling could help here is to allow the reviewer to make minor
> changes to the patch before it lands.
> ie. "r+, fix typo in comment before landing" would become "r+, i fixed
> the comment typo"

I don't think it's realistic to expect already-overloaded reviewers to do even more.

In my experience, reviewer's time is worth ~4 times more than patch author's time.
That's a completely arbitrary number, but represents how in my experience the load balance work.

So, I'd actually say we should do everything possible to *minimize* the amount of time required from the reviewer, rather than increasing it.

And I also don't think that increasing the number of reviewers would fix it. Reviewer by nature is often a senior engineer trying to balance out writing patches that very few people can, and review patches that less experienced engineers wrote.

Their time is insanely valuable because neither of those tasks can be easily done by the person requesting the review.

Of course there are exceptions like peer-reviews and rubber-stamping of a patch, but in general, I'd like us to think about shifting the burden onto automation / patch author to do as much work as possible before the reviewer commits their time.
And once their done, once again we should imho limit the time we expect from the reviewer for any follow-up reviews.

For that reason, the lack of interdiff in rebase scenario in MR is a major hassle in my experience. And the idea that the reviewer has to re-review multiple times or edit the patch themselves, as a step in the wrong direction.

Also, the idea that "anyone can re-review the patch" is very shaky. It would not work in the most crucial and delicate areas where the number of people familiar with the area is just low. Say, accessibility, graphics, internationalization, security etc.

In those lines, there's often a single person in the organization who can comfortably review the patch, and if they're in a different timezone, then asking a random reviewer on IRC for a review on nits is an illusion if the nits are anything beyond "update the comment".

On top of that, the idea also taps into the concern I raised above. Cognitive load required for a reviewer to step into a bug, skim through all comments, patch history and latest review with request for nits to understand if the nits represent the original reviewer request is also non trivial.

The way it's presented in this thread feels like a utopian vision where anyone can just take a quick glance and stamp an r+, but in reality it'll either add significantly to the load of already overloaded group in our project, or become an illusion of security with people just accepting everything from people they know.

I'm actually concerned that in the era where most projects go in the direction of streamlining the development and reducing the bureaucracy as much as possible (post-landing reviews, peer-reviews etc.), we're talking about adding another hoop to jump through.

I'm all for increased security (2FA etc.), but unless there's an unspoken set of cases where security of our project has been compromised by a change in the patch that was added after r+, I'd like to question if we're really at the point where we need such tradeoff.

zb.

[smaug]

A bit off topic

On 03/13/2017 04:37 PM, David Burns wrote:
>
> Regarding burden on reviewers, the comments in this thread just highlight
> how broken our current process is by having to flag individual people for
> reviews. This leads to the a handful of people doing 50%+ of reviews on the
> code.

Unfortunately I haven't heard other reviewing processes working much better.
Atm my thinking is that if everyone prioritized reviewing over pretty much everything else,
we'd be in better situation. There would be less incentive to ask reviews from current "fast reviewers", since
everyone would process their queue soon enough.
(other, less nice way would be something like bug 1189500)

> We, at least visibly, don't do enough to encourage new committers
> that would lighten the load to allow the re-review if there are nits.

true. I have tried to forward some reviews to relatively new team members.

[smaug]

A bit off topic

On 03/13/2017 04:37 PM, David Burns wrote:
>

> Regarding burden on reviewers, the comments in this thread just highlight
> how broken our current process is by having to flag individual people for
> reviews. This leads to the a handful of people doing 50%+ of reviews on the
> code.

Unfortunately I haven't heard other reviewing processes working much better.
Atm my thinking is that if everyone prioritized reviewing over pretty much everything else,
we'd be in better situation. There would be less incentive to ask reviews from current "fast reviewers", since
everyone would process their queue soon enough.
(other, less nice way would be something like bug 1189500)

> We, at least visibly, don't do enough to encourage new committers
> that would lighten the load to allow the re-review if there are nits.

[Jean-Yves Avenard]

> On 12 Mar 2017, at 9:40 pm, Ehsan Akhgari <ehsan....@gmail.com> wrote:
> And I still don't understand what the proposal means with rebases in
> practice. What if, after automation tries to land your change after you
> got your final r+ the final rebase fails and you need to do a manual
> rebase? Do you need to get another r+? What happens if you're touching
> one of our giant popular headers and someone else manages to land a
> conflict while your reviewer gets back to you?

+1, this is a very common scenario for me…

/me just loves when a new set of “rules” are put in place to prevent a problem that has never existed so far and will be a hindrance to everyone in the future.

[Mike Hoye]

On 2017-03-14 7:10 PM, Jean-Yves Avenard wrote:
> /me just loves when a new set of “rules” are put in place to prevent a problem that has never existed so far and will be a hindrance to everyone in the future.

Two dozen or so of our most veteran engineers are deeply involved in
this discussion. Their time and attention are extraordinarily valuable,
and there is no question about their commitment to Mozilla's success.
And yet: here they are, working through the details.

On top of everything else that's been said here, maybe take a moment to
reflect on that.

- mhoye

[Jean-Yves Avenard]

And yet, despite many people’s concerns, it appears that policy of removing r+ whenever a new push has been made effective.

And so, here I am with a r+ requesting to fix a comment, I have to ask for r+ again from someone not in my timezone and already on week-end.

Turn around time, from 30 minutes to 3.5 days…. How is that making our tree safer?

JY

[Bobby Holley]

On Fri, Mar 17, 2017 at 12:41 AM, Jean-Yves Avenard <jyav...@mozilla.com>
wrote:

> And yet, despite many people’s concerns, it appears that policy of
> removing r+ whenever a new push has been made effective.
>

To my knowledge, mconnor's proposal has yet to get anywhere close to an
actual policy. Was there a behavior change in the MozReview/autoland
workflow to do this? That sounds like something we should fix, but in the
mean time it seems fine to me to push your patch manually to inbound.

[Steven MacLeod]

On Fri, Mar 17, 2017 at 3:55 AM, Bobby Holley <bobby...@gmail.com> wrote:

> On Fri, Mar 17, 2017 at 12:41 AM, Jean-Yves Avenard <jyav...@mozilla.com
> >
> wrote:
>
> > And yet, despite many people’s concerns, it appears that policy of
> > removing r+ whenever a new push has been made effective.
> >
>
> To my knowledge, mconnor's proposal has yet to get anywhere close to an
> actual policy. Was there a behavior change in the MozReview/autoland
> workflow to do this? That sounds like something we should fix, but in the
> mean time it seems fine to me to push your patch manually to inbound.
>

There has been no behavior change (MozReview landing approval and
carryforwards have not been changed in quite a while)

[Gregory Szorc]

(Responding to several topics from the top post because I don't want to
spam with multiple replies.)

I think a lot of people are conflating "push access" with "ability to land
something." The distinction is the former grants you privileges to `hg
push` directly to repos on hg.mozilla.org and the latter allows delegation
of this ability. It is easy to conflate them because today "push access"
(level 3) gives you permission to trigger the landing (via the autoland
service via MozReview). But this is only because it was convenient to
implement this way. I want to explicitly state that we're moving towards
N=~10 people having raw push access to the Firefox repos with the majority
of landings occurring via autoland (via Conduit via MozReview/Bugzilla).
This reduces our attack surface area (less exposure to compromised SSH
keys), establishes better auditing (history of change will be on
Bugzilla/MozReview and not on a random local machine), eliminates potential
for bugs or variance with incorrect rebasing done on local machines, and
allows us to do extra things as part of the landing/rebase, such as
automatically reformat code to match unified code styles, do binding
generation, etc. They say all problems in computer science can be solved by
another level of indirection. Deferred landing (autoland) is such an
indirection and it solves a lot of problems. It will be happening. Most of
us will lose access to push directly to the Firefox repos. We won't be
losing ability to initiate a push, however. For the record, I'm not in
favor of making this change until the tooling is such that it won't be a
significant workflow/productivity regression. This is a reason why it
hasn't been made yet.

A handful have commented on whether a rebase invalidates a previous r+.
This is a difficult topic. Strictly speaking, a rebase invalidates
everything because a change in a commit being rebased over could invalidate
assumptions. Same goes for a merge commit. In reality, most rebases are
harmless and we shouldn't be concerned with their existence. In the cases
where it does matter, we can help prevent things from falling through the
cracks by having the review tool detect when in-flight reviews touch the
same files / topics and route them to the same reviewer(s) and/or notify
the different reviewer(s) so people are aware of potential for conflict.
This feature was conceived before MozReview launched but (sadly) hasn't
been implemented.

There have been a few comments on interdiffs when rebases are in play. I
want to explicitly state that there is no single "correct" way to display a
diff much less an interdiff much less an interdiff when a rebase is
involved. This is why tools like Git have multiple diffing algorithms
(minimal, patience, histogram, Myers). This is why there are different ways
of rendering a diff (unified, side-by-side, 3-way). Rendering a simple diff
is hard. Rendering an interdiff is harder. Unfortunately, central review
tools force a specific rendering on users. ReviewBoard does allow some
tweaking of diff behavior (such as ignore whitespace). But no matter what
options you use, someone will be unhappy with it. An example is MozReview's
handling of interdiffs. It used to hide lines changed by a rebase that
weren't changed in the commit up for review. But that was slightly buggy in
some situations and some people wanted to actually see those changes, so
the behavior was changed to show all file changes in the interdiff. This
made a different set of people unhappy because the changes were spurious
and contributed noise. In summary, you can't please all the people all the
time. So you have to pick a reasonable default then debate about adding
complexity to placate the long tail or handle corner cases. Standard
product design challenges. On top of that, technical users are the 1% and
are a very difficult crowd to please.

I'm sensitive to concerns that removal of "r+ with nits" will provide a net
productivity regression. We should be thinking of ways to make landing code
easier, not harder. This is a larger discussion spanning several topics, of
course. But the point I want to make is we have major, systemic problems
with code review latency today. Doing away with "r+ with nits" exacerbates
them, which is part of the reason I think people feel so passionately about
it. I feel like there needs to be a major cultural shift that emphasizes
low review latency, especially for new and high volume contributors. Tools
can help some. But tools only encourage behavior, not enforce it. I feel
like some kind of forcing function (either social/cultural or formal in
policy) is needed. What exactly, I'm not sure. At this point (where I see
very senior engineers with massive review queues), I fear the problem may
have to be addressed by management to adequately correct. (Yes, I hated
having to type the past few sentences.)

I think Steven MacLeod's response was highly insightful and is worth
re-reading multiple times. Pursuant to his lines of thought, I think there
is room to have more flexible policies for different components. For
example, high-value code like anything related to cryptography or security
could be barred from having "r+ with nits" but low-impact, non-shipping
files like in-tree documentation could have a very low barrier to change.
Of course, this implies flexible tools to accommodate flexible workflows
which may imply Mozilla's continued investment in those tools (which is in
contrast to a mindset held by some that we should use tools in an
off-the-shelf configuration instead of highly tailoring them to our
workflows). I like the simplicity of one policy for everything but am not
convinced it is best. I'd like more details on what other large companies
do here.

I hope this post doesn't revive this thread and apologize in advance if it
does. Please consider replying privately instead of poking the bear.

[Randell Jesup]

Responding to an old thread... (had saved as draft but didn't realize I
hadn't sent). Only replying to .platform here since last time I
cross-post-replied it didn't show up.

>I want to explicitly state that we're moving towards
>N=~10 people having raw push access to the Firefox repos with the majority
>of landings occurring via autoland (via Conduit via MozReview/Bugzilla).
>This reduces our attack surface area (less exposure to compromised SSH
>keys),

Reducing our attack surface is (on the surface) good... but...

>establishes better auditing (history of change will be on
>Bugzilla/MozReview and not on a random local machine),

I presume you mean rebases and nit-handling. I see this as unimportant.

>eliminates potential
>for bugs or variance with incorrect rebasing done on local machines,

Ok, but I don't see this as critical, and rarely if ever see issues with this.

>and allows us to do extra things as part of the landing/rebase, such as
>automatically reformat code to match unified code styles,

Oh, that is something that would be a *massive* annoyance and likely
involve destroying a bunch of history info (or rather hiding it from
easy access).

>do binding generation, etc. They say all problems in computer science
>can be solved by another level of indirection.

"Can be" and "Is a good idea" are two very separate things... :-) :-)

>Deferred landing (autoland) is such an
>indirection and it solves a lot of problems.

"A lot of problems" - please, this needs very concrete enumeration and
discussion. I do see one (SSH key compromise), though this isn't the
only way to deal with that. The rest listed above I don't see as
compelling when weighed against the costs (direct and indirect) of this
proposal. I also agree with a lot of when Ehsan said.

Also, the SSH issue - compromised SSH keys are certainly an avenue for
compromise of our binaries, but there are a whole bunch more ways that
could still happen, ways that don't leave the forensic trails checkins
would (as ekr mentioned). How many times (that we know of) has SSH
compromise led to a rogue checkin that got to release? (feel free to
answer that privately, for anyone who knows.)

>It will be happening. Most of
>us will lose access to push directly to the Firefox repos. We won't be
>losing ability to initiate a push, however. For the record, I'm not in
>favor of making this change until the tooling is such that it won't be a
>significant workflow/productivity regression. This is a reason why it
>hasn't been made yet.

I don't see how this proposal will achieve the goal you state as a
blocker. (E.g. ehsan's and other's comments.)

>A handful have commented on whether a rebase invalidates a previous r+.
>This is a difficult topic. Strictly speaking, a rebase invalidates
>everything because a change in a commit being rebased over could invalidate
>assumptions. Same goes for a merge commit. In reality, most rebases are
>harmless and we shouldn't be concerned with their existence.

This speaks against some of the benefits mentioned above.

>In the cases where it does matter, we can help prevent things from
>falling through the cracks by having the review tool detect when
>in-flight reviews touch the same files / topics and route them to the
>same reviewer(s) and/or notify the different reviewer(s) so people are
>aware of potential for conflict. This feature was conceived before
>MozReview launched but (sadly) hasn't been implemented.

I can't imagine how this will work (and be effective) in practice.

>There have been a few comments on interdiffs when rebases are in play. I
>want to explicitly state that there is no single "correct" way to display a
>diff much less an interdiff much less an interdiff when a rebase is
>involved. This is why tools like Git have multiple diffing algorithms
>(minimal, patience, histogram, Myers). This is why there are different ways
>of rendering a diff (unified, side-by-side, 3-way). Rendering a simple diff
>is hard. Rendering an interdiff is harder. Unfortunately, central review
>tools force a specific rendering on users. ReviewBoard does allow some
>tweaking of diff behavior (such as ignore whitespace). But no matter what
>options you use, someone will be unhappy with it. An example is MozReview's
>handling of interdiffs. It used to hide lines changed by a rebase that
>weren't changed in the commit up for review. But that was slightly buggy in
>some situations and some people wanted to actually see those changes, so
>the behavior was changed to show all file changes in the interdiff. This
>made a different set of people unhappy because the changes were spurious
>and contributed noise. In summary, you can't please all the people all the
>time. So you have to pick a reasonable default then debate about adding
>complexity to placate the long tail or handle corner cases. Standard
>product design challenges. On top of that, technical users are the 1% and
>are a very difficult crowd to please.

I've dealt with these by handling rebases myself (i.e. reviewers don't
get involved), with the rare exception that a rebase requires
non-trivial rewrite, in which case I just ask for re-review or re-review
on that part, or write the rebase as a new patch on top of the reject.
This is rarely needed, however.

Interdiffs for responding to review comments - sometimes (trivial
patches) I just replace and r? the entire patch. More complex ones I'll
set up as a separate patch (interdiff) and ask them to review that, then
fold/merge before checkin (yes, I use mq and splinter still - but the
general idea would apply regardless). And yes, mozreview does a MUCH
better (and more automatic) job of dealing with interdiffs
automatically. But that exposes you to the rebase problem. As you say,
there isn't an easy answer. Or just "never rebase until the patch is
"done", then deal with rebasing all at the end. (And, of course, for some
long-in-review patch queues, this won't be possible.)

>I'm sensitive to concerns that removal of "r+ with nits" will provide a net
>productivity regression. We should be thinking of ways to make landing code
>easier, not harder. This is a larger discussion spanning several topics, of
>course. But the point I want to make is we have major, systemic problems
>with code review latency today. Doing away with "r+ with nits" exacerbates
>them, which is part of the reason I think people feel so passionately about
>it. I feel like there needs to be a major cultural shift that emphasizes
>low review latency, especially for new and high volume contributors. Tools
>can help some. But tools only encourage behavior, not enforce it. I feel
>like some kind of forcing function (either social/cultural or formal in
>policy) is needed. What exactly, I'm not sure. At this point (where I see
>very senior engineers with massive review queues), I fear the problem may
>have to be addressed by management to adequately correct. (Yes, I hated
>having to type the past few sentences.)

We have greatly improved review latency in the last year or two (an
extreme case was certain people left reviews active for 9 months or a
year, because they disagreed with the patch direction). It's much
better now, but it isn't perfect, and trying to drive it a lot lower can
have negative impacts that may be hard to measure. C.f. Peopleware, and
the impact of interruptions on productivity (and quality). Many people
don't read email for stretches because they're busy working on something
(or meetings, etc). Some even disconnect from the internet to focus
(roc did). People have PTO, they have doctor's visits, they have p1
quantum bugs that are blocking other people - not all reviews are equal.

>I think Steven MacLeod's response was highly insightful and is worth
>re-reading multiple times. Pursuant to his lines of thought, I think there
>is room to have more flexible policies for different components. For
>example, high-value code like anything related to cryptography or security
>could be barred from having "r+ with nits" but low-impact, non-shipping
>files like in-tree documentation could have a very low barrier to
>change.

Having tighter requirements for select portions of the tree would be ok.
We effectively have that for things like NSS since it's imported
periodically. Such a limited tightening would be very different than
what's proposed.

>Of course, this implies flexible tools to accommodate flexible workflows
>which may imply Mozilla's continued investment in those tools (which is in
>contrast to a mindset held by some that we should use tools in an
>off-the-shelf configuration instead of highly tailoring them to our
>workflows). I like the simplicity of one policy for everything but am not
>convinced it is best. I'd like more details on what other large companies
>do here.
>
>I hope this post doesn't revive this thread and apologize in advance if it
>does. Please consider replying privately instead of poking the bear.

I did consider it. But I can't be quiet publicly with a posting stating
"this *will* happen" (emphasis added) on the table.

I certainly know of the vulnerabilities here.... but I also see how much
friction for our development cycle might be caused, with little
real-world benefit (IMHO). The last thing we need is to move a lot slower.

--
Randell Jesup, Mozilla Corp
remove "news" for personal email