Re: The future of commit access policy for core Firefox

On 2017-03-10 8:02 PM, Nicholas Nethercote wrote:
> On Sat, Mar 11, 2017 at 4:49 AM, Mike Connor <[email protected]> wrote:
> 
>>
>> To expand a bit on that last point: if we can trust core/known developers
>> to competently make necessary changes prior to checkin, we should also be
>> able to expect them to fix most/all of those issues *before* they submit
>> those patches for review.  My experience as both author and reviewer has
>> generally been that doing code review on something 90% finished takes far
>> more time than something that's been refined thoroughly prior to
>> submission.  I'd love to see us tighten up the expectations on patch
>> authors to only request review when they believe something is fully ready
>> to land.  (If you want early/high-level feedback, there's a flag for that.)
>>
> 
> I frequently makes minor changes to patches after review that haven't been
> suggested by reviewers. Often after sleeping on it. These are probably more
> often in comments than code. Maybe I'm unusual in this respect.

This is part of my usual workflow as well.

> I'm surprised that Servo and Rust haven't come up yet, because they do
> already have a model much like the one suggested -- landing approval must
> come from an authorized reviewer.
> 
> In my experience it's not a disaster but it is a hassle at times and
> motivates against suggesting (as a reviewer) or making (as an author) minor
> changes -- it's easy to be lazy and say "close enough". And in the case
> where minor changes are requested, the final approval often is a complete
> rubber stamp -- I've had Servo patches authorized by people uninvolved with
> the PR simply because they were in my timezone and so online at the time.

That is quite sad to hear...  As a reviewer, oftentimes the changes that
I ask for in my r+ with nits are changes I actually care about quite a
lot for various reasons depending on the context...  To me, r+ with nits
means I'm trusting the author to be able to make the changes without
oversight, not that the changes are unimportant.  Even if all such
changes are trivial nits, this is a good way to build up technical debt.