The future of commit access policy for core Firefox

classic Classic list List threaded Threaded
77 messages Options
1234
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Eric Shepherd

> On Mar 13, 2017, at 3:48 PM, Justin Dolske <[hidden email]> wrote:
>
> Specifically, here: if a reviewer has already decided that a patch is "r+ with fixes", it's unlikely that the followup patch is going to get a vigorous, detailed review. Especially if the process is perceived as pointless overhead, causing delays, and 99.9999% of the time the patch author is not trying to sneak in malicious code.
>
> So a simple "every patch must be reviewed" requirement which doesn't address that in some way isn't really going to change much from the status quo -- you'd get "reviews", but only as a paperwork formality.


That brings to mind this question, for me: does it make more sense to either shoot for a slightly improved review process but find and implement practices that have automated security verifications being performed on the code when it’s submitted for review, even before the reviewer sees it? Before you jump, yes, I know these tools aren’t able to catch all the varieties of issues that exist, but it would certainly help. (Personally, this is an area we could apply some resources to, if only by donation of funds).

Anyway, once the automated security scans are complete and validated, only then would the reviewer actually do their review. They’d still have to do security related checks, but they’d have backup, and hopefully the really tiny changes, such as "nit” fixes would have a pretty low risk factor given the automated analysis. If it gets sent back for changes — of any kind — it goes through security scans again.

I may or may not have made sense, but hopefully I did. Just throwing thoughts out there.


Eric Shepherd
Senior Technical Writer
Mozilla Developer Network <https://developer.mozilla.org/>
Blog: https://www.bitstampede.com/
Twitter: https://twitter.com/sheppy

_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Lawrence Mandel-2
In reply to this post by Eric Rescorla
One issue with r+ with nits that we ran into last year is that the
resulting patch/diff is often committed directly to the repo and not
uploaded back to Bugzilla or MozReview. This makes it difficult to audit
the changes to the repo. Keeping the review system in sync with what lands
(regardless of the review requirements) will make it easier to automate a
repo audit and reduce the time that our reviewers need to spend looking at
code changes in the audit scenario. Any concerns with making it a
requirement that the final patch/diff is documented in the bug/review tool
rather than landing directly? (Assuming we adopt a working autoland system
as the way to land code this will be obviously enforced by the tool.)

Lawrence

On Mon, Mar 13, 2017 at 4:01 PM, Eric Rescorla <[hidden email]> wrote:

> On Mon, Mar 13, 2017 at 12:30 PM, Boris Zbarsky <[hidden email]> wrote:
>
> > On 3/13/17 1:33 PM, Eric Rescorla wrote:
> >
> >> Actually, I wish I had written this differently. Say I get an r+ w/o
> nits,
> >> I suspect
> >> that the sheriffs will accept an updated patch (e.g., ostensibly with a
> >> comment fix) that is marked r=<foo>.
> >>
> >
> > This seems entirely too plausible.  :(
> >
> > Me too. And I think "trust" in this case at least arguably should be
> >> defined as
> >> "trusted by Mozilla" (e.g., L3 committer). So, one possibility would be
> >> have a
> >> policy like the following.
> >>
> >
> > This seems reasonable, if that's the goal.  But this is not the goal
> > mconnor had in his original post.  I'd love to get to the point where we
> > agree on the goals.
>
>
> Me too. I have come to the conclusion that mconnor's goal cannot be
> achieved
> without substantial disruption.
>
>
> - Every CL must either be written by someone trusted OR r+ed by someone
> >> trusted.
> >> - If a patch is r+ with nits, then the final patch must be posted by
> >> someone
> >> trusted.
> >>
> >
> > This doesn't quite address your "r+ without nits, then the patch author
> > updates it anyway" scenario; presumably we would need something to
> address
> > that too.
>
>
> Sorry, I should have clarified this. In any case, the final (landed) patch
> must be
> either reviewed or posted by someone trusted. The nits thing is a side
> issue
> (which you would think I would have realized from earlier in my own
> message!)
>
> -Ekr
>
>
>
>
>
> >
> >
> > -Boris
> > _______________________________________________
> > dev-planning mailing list
> > [hidden email]
> > https://lists.mozilla.org/listinfo/dev-planning
> >
> _______________________________________________
> dev-planning mailing list
> [hidden email]
> https://lists.mozilla.org/listinfo/dev-planning
>
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Boris Zbarsky
In reply to this post by Eric Rescorla
On 3/13/17 4:24 PM, Lawrence Mandel wrote:
> Any concerns with making it a
> requirement that the final patch/diff is documented in the bug/review tool
> rather than landing directly?

Just one: can we make this not send bugmail to people?  At least as an
option?

>(Assuming  we adopt a working autoland system
> as the way to land code this will be obviously enforced by the tool.)

Indeed.

-Boris
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Bobby Holley-2
In reply to this post by Lawrence Mandel-2
On Mon, Mar 13, 2017 at 1:24 PM, Lawrence Mandel <[hidden email]>
wrote:

> One issue with r+ with nits that we ran into last year is that the
> resulting patch/diff is often committed directly to the repo and not
> uploaded back to Bugzilla or MozReview. This makes it difficult to audit
> the changes to the repo. Keeping the review system in sync with what lands
> (regardless of the review requirements) will make it easier to automate a
> repo audit and reduce the time that our reviewers need to spend looking at
> code changes in the audit scenario. Any concerns with making it a
> requirement that the final patch/diff is documented in the bug/review tool
> rather than landing directly?
>

Submitting the final patches to two places instead of one seems like
busywork to me, and I don't do it (even though some do). I don't know what
it buys us, given that pulsebot posts the hashes of the pushed changes in
the bug.

So I would object to this.


>
> Lawrence
>
> On Mon, Mar 13, 2017 at 4:01 PM, Eric Rescorla <[hidden email]> wrote:
>
> > On Mon, Mar 13, 2017 at 12:30 PM, Boris Zbarsky <[hidden email]>
> wrote:
> >
> > > On 3/13/17 1:33 PM, Eric Rescorla wrote:
> > >
> > >> Actually, I wish I had written this differently. Say I get an r+ w/o
> > nits,
> > >> I suspect
> > >> that the sheriffs will accept an updated patch (e.g., ostensibly with
> a
> > >> comment fix) that is marked r=<foo>.
> > >>
> > >
> > > This seems entirely too plausible.  :(
> > >
> > > Me too. And I think "trust" in this case at least arguably should be
> > >> defined as
> > >> "trusted by Mozilla" (e.g., L3 committer). So, one possibility would
> be
> > >> have a
> > >> policy like the following.
> > >>
> > >
> > > This seems reasonable, if that's the goal.  But this is not the goal
> > > mconnor had in his original post.  I'd love to get to the point where
> we
> > > agree on the goals.
> >
> >
> > Me too. I have come to the conclusion that mconnor's goal cannot be
> > achieved
> > without substantial disruption.
> >
> >
> > - Every CL must either be written by someone trusted OR r+ed by someone
> > >> trusted.
> > >> - If a patch is r+ with nits, then the final patch must be posted by
> > >> someone
> > >> trusted.
> > >>
> > >
> > > This doesn't quite address your "r+ without nits, then the patch author
> > > updates it anyway" scenario; presumably we would need something to
> > address
> > > that too.
> >
> >
> > Sorry, I should have clarified this. In any case, the final (landed)
> patch
> > must be
> > either reviewed or posted by someone trusted. The nits thing is a side
> > issue
> > (which you would think I would have realized from earlier in my own
> > message!)
> >
> > -Ekr
> >
> >
> >
> >
> >
> > >
> > >
> > > -Boris
> > > _______________________________________________
> > > dev-planning mailing list
> > > [hidden email]
> > > https://lists.mozilla.org/listinfo/dev-planning
> > >
> > _______________________________________________
> > dev-planning mailing list
> > [hidden email]
> > https://lists.mozilla.org/listinfo/dev-planning
> >
> _______________________________________________
> dev-planning mailing list
> [hidden email]
> https://lists.mozilla.org/listinfo/dev-planning
>
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Mike Connor-4
On Mon, Mar 13, 2017 at 4:35 PM, Bobby Holley <[hidden email]> wrote:

> On Mon, Mar 13, 2017 at 1:24 PM, Lawrence Mandel <[hidden email]>
> wrote:
>
> > One issue with r+ with nits that we ran into last year is that the
> > resulting patch/diff is often committed directly to the repo and not
> > uploaded back to Bugzilla or MozReview. This makes it difficult to audit
> > the changes to the repo. Keeping the review system in sync with what
> lands
> > (regardless of the review requirements) will make it easier to automate a
> > repo audit and reduce the time that our reviewers need to spend looking
> at
> > code changes in the audit scenario. Any concerns with making it a
> > requirement that the final patch/diff is documented in the bug/review
> tool
> > rather than landing directly?
> >
>
> Submitting the final patches to two places instead of one seems like
> busywork to me, and I don't do it (even though some do). I don't know what
> it buys us, given that pulsebot posts the hashes of the pushed changes in
> the bug.
>
> So I would object to this.


I'd agree that posting a bug to two places is a non-starter.  That's a
problem we can and should solve with automation. It should be possible to
commit to somewhere, have it show up in Bugzilla, and have it land where it
needs to go.

-- Mike
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Mike Connor-4
In reply to this post by Boris Zbarsky
On Mon, Mar 13, 2017 at 4:35 PM, Boris Zbarsky <[hidden email]> wrote:

> On 3/13/17 4:24 PM, Lawrence Mandel wrote:
>
>> Any concerns with making it a
>> requirement that the final patch/diff is documented in the bug/review tool
>> rather than landing directly?
>>
>
> Just one: can we make this not send bugmail to people?  At least as an
> option?


At the least, it should probably send mail, and maybe even an interdiff, to
the reviewer(s).

Otherwise it's sort of "trust and never verify" as a stance.  Not sure that
quite makes sense.

-- Mike
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

SmauG-2
In reply to this post by Mike Connor-4
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.



_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Steve Fink-4
In reply to this post by Eric Shepherd
On 03/13/2017 01:23 PM, Eric Shepherd (Sheppy) wrote:

>> On Mar 13, 2017, at 3:48 PM, Justin Dolske <[hidden email]> wrote:
>>
>> Specifically, here: if a reviewer has already decided that a patch is "r+ with fixes", it's unlikely that the followup patch is going to get a vigorous, detailed review. Especially if the process is perceived as pointless overhead, causing delays, and 99.9999% of the time the patch author is not trying to sneak in malicious code.
>>
>> So a simple "every patch must be reviewed" requirement which doesn't address that in some way isn't really going to change much from the status quo -- you'd get "reviews", but only as a paperwork formality.
>
> That brings to mind this question, for me: does it make more sense to either shoot for a slightly improved review process but find and implement practices that have automated security verifications being performed on the code when it’s submitted for review, even before the reviewer sees it? Before you jump, yes, I know these tools aren’t able to catch all the varieties of issues that exist, but it would certainly help. (Personally, this is an area we could apply some resources to, if only by donation of funds).
>
> Anyway, once the automated security scans are complete and validated, only then would the reviewer actually do their review. They’d still have to do security related checks, but they’d have backup, and hopefully the really tiny changes, such as "nit” fixes would have a pretty low risk factor given the automated analysis. If it gets sent back for changes — of any kind — it goes through security scans again.
>
> I may or may not have made sense, but hopefully I did. Just throwing thoughts out there.

Lest people find this unrealistic, I would like to point out that within
the JS engine at least, we sort of already have this via fuzzing. It's
not an analysis of a specific change, it's the analysis of the overall
state after a set of changes, with mostly automated bisection when
something is found. I believe it provides the sorts of benefit sheppy is
describing, but in a retroactive way -- reviewers don't have the results
in hand at the time of review, but at least they can depend on one class
of problems being caught before too long.

Obviously, it relies heavily on how fuzzable a component is.

_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Mike Connor-4
In reply to this post by Nathan Froyd-5
On Mon, Mar 13, 2017 at 2:35 PM, Nathan Froyd <[hidden email]> wrote:

> On Mon, Mar 13, 2017 at 2:12 PM, Mike Connor <[hidden email]> wrote:
> > While there's some fuzzy lines between modules (and the pan-tree rewrite
> > case needs a bit more discussion), I think we can quickly to get to a
> point
> > where module owners and peers are defined in-tree, in a machine-readable
> > format that we could use to a) validate that the reviewer(s) of a patch
> are
> > the appropriate people and b) suggest potential reviewers.
>
> I think you're going to want to handle delegation of reviews, e.g.
>
> 1) I own/peer a module and I would like somebody to review my patches
> so I can get a sense of how thoroughly they review code and/or to help
> provide feedback on their review skills.
>
> 2) Similar to the above, maybe I get tagged for review, but I would
> like somebody else who doesn't own/peer the module to review the code
> because I judge them capable enough.  This and the above case are
> mostly about growing new reviewers.
>
> 2a) Perhaps I would just like a second set of eyes on the patch (e.g.
> threading issues) and am comfortable handing off review to somebody
> who is more expert in such matters and whose judgement I trust to
> handle any other issues capably.
>

My assumption is that "must be reviewed by someone already trusted" is a
minimum bar, and would not exclude additional reviews from another party.

In terms of growing new reviewers, the approach I've used many times in the
past was to have new reviewers take the initial pass, and then do a final
review myself.  That way they get to learn, I get to evaluate and coach
them on their review approach, and I remain confident that the review bar
was sufficiently high.  Once I stop catching issues that they didn't,
they're ready to be a full peer.

3) Sometimes I get asked about reviewing particular things and I ask
> the person who I know wrote the original code to review instead
> because they are more familiar than I am.  Maybe this can be addressed
> with ever-finer lists of code owners.
>

Finer-grained lists can work, or the owner/peer who is delegating a
specific review can formally approve (mark r+) based on the work of the
delegate.

I'd frame this as delegating work, but not responsibility.


> 4) Would just like to reiterate that the rewriting code case needs
> some thought, as somebody who has done such rewrites (and appreciated
> getting a single review) and as somebody who has reviewed code for
> such rewrites.
>

My thinking, as you noted further down, is that we can identify the set of
reviewers who can approve a broad change.  I'm reasonably confident that
the list of people who should be reviewing pan-tree changes is much smaller
than the number of people with domain expertise.


> If we require reviewers to appear in a trusted-list in-tree, then
> actually growing new reviewers is going to be a real pain.


See above.  I think the responsibility angle is key when delegating work.
"Trainees" can mark r+, but that r+ isn't enough to land.  The owner/peer
can make their own decision on how much they validate that work, from a
full re-review to a rubber stamp, but they're assuming responsibility
either way.

-- Mike
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Justin Dolske-2
In reply to this post by Justin Dolske-2
On 3/13/17 1:23 PM, Eric Shepherd (Sheppy) wrote:

>
> That brings to mind this question, for me: does it make more sense to
> either shoot for a slightly improved review process but find and
> implement practices that have automated security verifications being
> performed on the code when it’s submitted for review, even before the
> reviewer sees it? Before you jump, yes, I know these tools aren’t
> able to catch all the varieties of issues that exist, but it would
> certainly help.

Yeah, automated security scanning is... hard.

But a related example I had in mind was stuff like eslint -- the OG "r+
with nits" was a review on substance plus corrections for (sometimes
arbitrary) style.

Ongoing improvements for seeing integrated test results is similarly
useful. Making it "it passed tests, now please review" easy is an
improvement over landing a reviewed patch plus 2 unreviewed followups
for test bustage. [Which of course has never ever happened to me. *cough*]

Justin
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Trevor Saunders-2
In reply to this post by Mike Connor-4
On Mon, Mar 13, 2017 at 04:42:35PM -0400, Mike Connor wrote:

> On Mon, Mar 13, 2017 at 4:35 PM, Boris Zbarsky <[hidden email]> wrote:
>
> > On 3/13/17 4:24 PM, Lawrence Mandel wrote:
> >
> >> Any concerns with making it a
> >> requirement that the final patch/diff is documented in the bug/review tool
> >> rather than landing directly?
> >>
> >
> > Just one: can we make this not send bugmail to people?  At least as an
> > option?
>
>
> At the least, it should probably send mail, and maybe even an interdiff, to
> the reviewer(s).

Now, the case of Boris is the reviewer is different from the case boris is
just cc'd to the bug.  However sending the mail is only useful if Boris
chooses to do something with  it other than delete it unread.  Which it
sounds like he may well want to, and even if I wanted to do differently
I can easily see doing the same because there is something more
important at the moment.

> Otherwise it's sort of "trust and never verify" as a stance.  Not sure that
> quite makes sense.

Well, if you want it to be more than that you need to somehow force
people to look at things and think about them, which is rather hard.

Trev

>
> -- Mike
> _______________________________________________
> dev-planning mailing list
> [hidden email]
> https://lists.mozilla.org/listinfo/dev-planning
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Nicholas Nethercote
In reply to this post by Lawrence Mandel-2
On Tue, Mar 14, 2017 at 7:24 AM, Lawrence Mandel <[hidden email]>
wrote:
>
> One issue with r+ with nits that we ran into last year is that the
> resulting patch/diff is often committed directly to the repo and not
> uploaded back to Bugzilla or MozReview. This makes it difficult to audit
> the changes to the repo.

I always look at the hg commit link rather than posted patches, precisely
because I know that's exactly what landed. Even if the patch author doesn't
explicitly change anything after getting review, rebasing can still change
the patch.

Nick
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

jmaher-2
In reply to this post by Lawrence Mandel-2
On Monday, March 13, 2017 at 8:53:11 PM UTC-4, Nicholas Nethercote wrote:

> On Tue, Mar 14, 2017 at 7:24 AM, Lawrence Mandel <[hidden email]>
> wrote:
> >
> > One issue with r+ with nits that we ran into last year is that the
> > resulting patch/diff is often committed directly to the repo and not
> > uploaded back to Bugzilla or MozReview. This makes it difficult to audit
> > the changes to the repo.
>
> I always look at the hg commit link rather than posted patches, precisely
> because I know that's exactly what landed. Even if the patch author doesn't
> explicitly change anything after getting review, rebasing can still change
> the patch.
>
> Nick

I just r-'d a patch last night because a rebase caused it to do unintended things.  While I don't see this everyday, it seems that rebases cause many issues (I see it monthly) and given that data I would say any rebases should be re-reviewed.  How often do patch authors examine their patch in detail after a rebase?  Just looking at the diff can be OK, but seeing the source code in more context can point out the code being added in the wrong place (or in some cases duplicated).  

-Joel
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Axel Hecht
Am 14.03.17 um 13:15 schrieb [hidden email]:

> On Monday, March 13, 2017 at 8:53:11 PM UTC-4, Nicholas Nethercote wrote:
>> On Tue, Mar 14, 2017 at 7:24 AM, Lawrence Mandel <[hidden email]>
>> wrote:
>>>
>>> One issue with r+ with nits that we ran into last year is that the
>>> resulting patch/diff is often committed directly to the repo and not
>>> uploaded back to Bugzilla or MozReview. This makes it difficult to audit
>>> the changes to the repo.
>>
>> I always look at the hg commit link rather than posted patches, precisely
>> because I know that's exactly what landed. Even if the patch author doesn't
>> explicitly change anything after getting review, rebasing can still change
>> the patch.
>>
>> Nick
>
> I just r-'d a patch last night because a rebase caused it to do unintended things.  While I don't see this everyday, it seems that rebases cause many issues (I see it monthly) and given that data I would say any rebases should be re-reviewed.  How often do patch authors examine their patch in detail after a rebase?  Just looking at the diff can be OK, but seeing the source code in more context can point out the code being added in the wrong place (or in some cases duplicated).
>
> -Joel
>

One point on rebases:

autoland, AFAIK, *never* lands the changeset it was asked to land.

It always rebases, and often adjusts the commit message to change the r?
to r+. (Does it also change the reviewer to the actual reviewer? I'd
have to check the code.)

I'm pretty sure that at the pace that we're changing the head of our
repository, we won't get any code landed without rs=foopy on rebases,
and thus hash changes.

Which also raises an interesting question about how much we can prove
about the landed patch at the time it lands.

Axel
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Ehsan Akhgari
In reply to this post by Mike Connor-4
On 2017-03-13 4:38 PM, Mike Connor wrote:

> On Mon, Mar 13, 2017 at 4:35 PM, Bobby Holley <[hidden email]> wrote:
>
>> On Mon, Mar 13, 2017 at 1:24 PM, Lawrence Mandel <[hidden email]>
>> wrote:
>>
>>> One issue with r+ with nits that we ran into last year is that the
>>> resulting patch/diff is often committed directly to the repo and not
>>> uploaded back to Bugzilla or MozReview. This makes it difficult to audit
>>> the changes to the repo. Keeping the review system in sync with what
>> lands
>>> (regardless of the review requirements) will make it easier to automate a
>>> repo audit and reduce the time that our reviewers need to spend looking
>> at
>>> code changes in the audit scenario. Any concerns with making it a
>>> requirement that the final patch/diff is documented in the bug/review
>> tool
>>> rather than landing directly?
>>>
>>
>> Submitting the final patches to two places instead of one seems like
>> busywork to me, and I don't do it (even though some do). I don't know what
>> it buys us, given that pulsebot posts the hashes of the pushed changes in
>> the bug.
>>
>> So I would object to this.
>
>
> I'd agree that posting a bug to two places is a non-starter.  That's a
> problem we can and should solve with automation. It should be possible to
> commit to somewhere, have it show up in Bugzilla, and have it land where it
> needs to go.

I feel like I'm starting to reply to every message in this thread with
"rebases, rebases, rebases".  ;-)

More seriously, what Lawrence is asking for is simply just impossible.
You can't know what the final patch you will push will be before you
push it due to the fact that we prohibit pushing more than one head, so
you may have to do an unlimited number of rebases before you actually
succeed in landing your patch.

Once your patch has landed, then pulsebot already sends a link to the
landed commit to the bug, and that is what has to be counted on as the
actual source of truth as to what needs to landed.  The fact that
MozReview is incapable of reflecting that in its UI is just a deficiency
of the review tool which we could fix.  I use Phabricator for
contributing to LLVM, and there as soon as I manage to push the final
version of my patch to their SVN repo, Phabricator UI links my review
page to the real SVN revision landed (example:
https://reviews.llvm.org/D16761 says
"Was committed in r260265, but reverted in r260536.")
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Lawrence Mandel-2
On Wed, Mar 15, 2017 at 10:53 AM, Ehsan Akhgari <[hidden email]>
wrote:

> On 2017-03-13 4:38 PM, Mike Connor wrote:
> > On Mon, Mar 13, 2017 at 4:35 PM, Bobby Holley <[hidden email]>
> wrote:
> >
> >> On Mon, Mar 13, 2017 at 1:24 PM, Lawrence Mandel <[hidden email]>
> >> wrote:
> >>
> >>> One issue with r+ with nits that we ran into last year is that the
> >>> resulting patch/diff is often committed directly to the repo and not
> >>> uploaded back to Bugzilla or MozReview. This makes it difficult to
> audit
> >>> the changes to the repo. Keeping the review system in sync with what
> >> lands
> >>> (regardless of the review requirements) will make it easier to
> automate a
> >>> repo audit and reduce the time that our reviewers need to spend looking
> >> at
> >>> code changes in the audit scenario. Any concerns with making it a
> >>> requirement that the final patch/diff is documented in the bug/review
> >> tool
> >>> rather than landing directly?
> >>>
> >>
> >> Submitting the final patches to two places instead of one seems like
> >> busywork to me, and I don't do it (even though some do). I don't know
> what
> >> it buys us, given that pulsebot posts the hashes of the pushed changes
> in
> >> the bug.
> >>
> >> So I would object to this.
> >
> >
> > I'd agree that posting a bug to two places is a non-starter.  That's a
> > problem we can and should solve with automation. It should be possible to
> > commit to somewhere, have it show up in Bugzilla, and have it land where
> it
> > needs to go.
>
> I feel like I'm starting to reply to every message in this thread with
> "rebases, rebases, rebases".  ;-)
>
> More seriously, what Lawrence is asking for is simply just impossible.
> You can't know what the final patch you will push will be before you
> push it due to the fact that we prohibit pushing more than one head, so
> you may have to do an unlimited number of rebases before you actually
> succeed in landing your patch.
>
> Once your patch has landed, then pulsebot already sends a link to the
> landed commit to the bug, and that is what has to be counted on as the
> actual source of truth as to what needs to landed.  The fact that
> MozReview is incapable of reflecting that in its UI is just a deficiency
> of the review tool which we could fix.  I use Phabricator for
> contributing to LLVM, and there as soon as I manage to push the final
> version of my patch to their SVN repo, Phabricator UI links my review
> page to the real SVN revision landed (example:
> https://reviews.llvm.org/D16761 says
> "Was committed in r260265, but reverted in r260536.")
>

OK. Clearly I didn't quite think that one through. Point taken.

Lawrence
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Gregory Szorc-3
In reply to this post by Mike Connor-4
On Thu, Mar 9, 2017 at 1:53 PM, Mike Connor <[hidden email]> 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.
>    - 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.
>

(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.
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
1234