Re: The future of commit access policy for core Firefox

On Thu, Mar 9, 2017 at 5:14 PM, L. David Baron <[email protected]> wrote:

> On Thursday 2017-03-09 16:53 -0500, Mike Connor wrote:
> > I've identified the following goals as critical for a responsible commit
> > access policy:
> ...
> >    - The change that gets pushed must be the same change that was
> approved.
>
> I'm curious what this goal means.  In particular, does it mean that
> you're trying to end "review+ if you make the following changes",
> and require that reviewers re-review the revisions no matter what?
>
> (If it does mean that, then that's a substantial increase on
> reviewer load; if it doesn't, then I'm curious what definition of
> "the same" you're using.)


Replying to this specific part, since there's a lot of expected and
understandable concerns here.

That's a potential end state, and one end of the spectrum of strictness of
code review/approval.  That spectrum ranges from "trust approved developers
to make good changes, don't do code review" to "every line of code that
lands should be explicitly reviewed first."  Leaving aside productivity
concerns for the moment, I think we would all agree that code review is a
critical part of our process, because developers are human and humans make
mistakes.  As a result, the closer we get to doing formal review on final
versions, instead of mostly-done versions, the better off we should be.
That will take time, and I think it's the right goal to work toward.

Big picture, I agree that doing this today presents a lot of challenges,
especially for reviewers currently carrying a huge load.  We don't *yet*
have great tooling in place to easily review the differences between
submissions (and ideally recognize when it's just
whitespace/syntax/comments typos getting fixed).  We have a lot of
reviewers barely keeping up with reviews (or worse, always falling
behind).  We don't have an easy enough process for running patches through
automated testing in advance of reviews.  And most difficult of all,
patches are frequently not in a "ready to land" state when submitted for
review, meaning a clean review is relatively rare.  I believe we can and
should work to address all of these issues, especially the last one.

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

Assuming we can make significant progress on all of those challenges, and
the net reviewer overhead is about the same, are there other reasons we
wouldn't want to move to a much stricter review process?

-- Mike