On 2017-03-13 1:40 PM, Bobby Holley wrote:
> On Mon, Mar 13, 2017 at 10:33 AM, Eric Rescorla wrote:
>> On Mon, Mar 13, 2017 at 6:36 AM, Boris Zbarsky 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=.
>>> 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
>> fix) that is marked r=.
>> 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
>>>> 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
>> - If a patch is r+ with nits, then the final patch must be posted by
>> This would ensure that every CL that lands was signed off on in its final
>> by a someone trusted.
>> Does this seem crazy?
> This seems like a very reasonable compromise to me. It gets the overhead
> out of the way in the hot paths but still gives us some organization-wide
I still think this has mostly the same issues as has been raised in the
thread so far, even though at first glance it may seem a bit nicer than
the original proposal: for example, there is still the issue of whether
reviewers actually treat code coming from someone named Ehsan Akhgari as
potentially malicious code, what this means for rebases, who will be
responsible for the final sign off on the patch in the r+-with-nits
Note that it seems now we have modified our goal slightly. Originally
it seems we were trying to ensure we don't get incoming security bugs by
ensuring that any commit that ultimately gets checked in gets an r+.
Now we are talking about every commit needs to be r+ed or authored by
someone with L3 access. Are we still protecting against incoming