Re: The future of commit access policy for core Firefox

How do you plan to handle code in external libraries?

As long as anyone can submit a random patch to libwhatever and get a
free pass into Firefox the next time we pull an update, it's not clear
to me what the proposed restrictions are really buying us.

(And I don't think that requiring an L3 developer to review such merges
would be sufficient. Mozilla may lack the expertise on those libraries,
lack the time to review the details of large merges, or run into the
problems of retro-review that kats mentioned.)

On Tue, Mar 14, 2017, at 07:12 AM, Mike Connor wrote:
> This is something I've been thinking about a bunch over the weekend.  I
> think trusted needs a smaller scope than "one of the hundreds of people
> with L3 access."  As Bobby noted, the primary issue is the hot paths, so
> we
> should look at how to explicitly define those paths.
> 
> 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.  b) is
> especially crucial in terms of better balancing review load, which is
> something David Burns noted earlier.  I'm almost certain that we could do
> a
> much better job of balancing review loads.
> 
> If we had that, we could reframe this idea as:
> 
> * every patch must be reviewed by an appropriate reviewer (verifiable)
> * patch authors who are *also* owners/peers or otherwise marked as
> trusted
> for the code in question (membership in SR group, or explicitly trusted
> by
> the module owner) are permitted to carry over a review.
> * carrying over a review requires the original patch be marked explicitly
> (i.e. review+ and review-with-nits+)
> 
> This does require explicitly marking people as trusted, but I don't think
> that's a huge burden, since that list of people doesn't change often.
> 
> I think this tightens things up, without creating a significant amount of
> pain.
> 
> -- Mike
> 
> On Mon, Mar 13, 2017 at 1:33 PM, Eric Rescorla <[email protected]> wrote:
> 
> > On Mon, Mar 13, 2017 at 6:36 AM, Boris Zbarsky <[email protected]> wrote:
> >
> > > On 3/12/17 9:55 PM, Eric Rescorla wrote:
> > >
> > >> Alternately, you can create a patch which gets r+ with nits, and
> > >> then update with some malicious code and  r=<committer>.
> > >>
> > >
> > > Speaking as a reviewer, for people I don't trust I pretty much never give
> > > "r+ with nits", because I don't trust them to address the nits correctly.
> > >
> >
> > 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>.
> >
> >
> > So, I would ask: do people believe that this is an acceptable state of
> > >> affairs or should the minimum number of 'trusted' people required to
> > land
> > >> a patch be 1
> > >> or more?
> > >>
> > >
> > > Obviously the latter.  ;)
> >
> >
> > 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.
> >
> > - 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 would ensure that every CL that lands was signed off on in its final
> > form
> > by a someone trusted.
> >
> > Does this seem crazy?
> >
> > -Ekr
> >
> >
> > >
> > >
> > > -Boris
> > >
> > >
> > > _______________________________________________
> > > dev-planning mailing list
> > > [email protected]
> > > https://lists.mozilla.org/listinfo/dev-planning
> > >
> > _______________________________________________
> > dev-planning mailing list
> > [email protected]
> > https://lists.mozilla.org/listinfo/dev-planning
> >
> _______________________________________________
> dev-planning mailing list
> [email protected]
> https://lists.mozilla.org/listinfo/dev-planning