Re: The future of commit access policy for core Firefox

On Fri, Mar 10, 2017 at 12:49:50PM -0500, Mike Connor wrote:
> 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.

If we pretend the goal has no downside then of course it seems like a
good idea.  However its really hard for me to imagine a world where its
worth the time to review patches that amount to

+  MOZ_ASSERT(x);

after I asked you to please assert x there.  The worst downsides would
seem to be forgeting to add the assert which often isn't that big a
deal, or you adding a wrong assert which will show up soon enough.

Even more extreme consider a patch like

-#include <IUnknown.h>
+#include <iunknown.h>

For that to have fixed things on a case sensitive fs it basically has to
have been the correct change.

Finally for
-class X : public Y
+class X final : public Y

which I don't have a way to screw up that a compiler will accept on the
top of my head.

So until we have a world where getting that reviewed takes less time than
dealing with the bug mail for it today I'm inclined to think r+ with
nits for the first and the standing rubber stamps for the latter two
make a lot of sense.

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

I share the experience of seeing plenty of patches that aren't perfectly
ready to land, but I think most of the reasons are pretty unavoidable.
I'd break them up into a couple groups.

First we have patches that are just baddly designed.    As people learn
the area they will get better at avoiding writing those patches, but it
seems likely there will always be some.

Then we have the patches that missed some corner case or race condition.
Idealy we might be able to catch all those with tests, but I'm kind of
skeptical especially when we look at the number of possibly busted tests
we find ourselves disabling.

The third group is patches missing things like asserts, maybe missing a
comment for something tricky, or maybe just a comment typo.  Any of
those might easily slip by the author, so I'd expect we'll keep seeing
them.

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

Given what I observed above I tend to come up with some different
conclusions.

First I don't think we have a particularly big problem in the people I
work with of people asking for review on particularly half done sloppy
patches.

Second while they could fix the issues before asking for review they very
often reasonably don't know of them.  So the choice is between making
them aware and then rereviewing, or trusting them to get it right /
being ok with them messing it up once in a while.

So to the degreee I review patches that aren't ready to land the
problems in them are not the major time sinks in reviewing the patch.

> 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?

I think there are some really big assumptions in there, but its hard to
disagree with wanting to print free money.

Trev

> 
> -- Mike
> _______________________________________________
> dev-planning mailing list
> [email protected]
> https://lists.mozilla.org/listinfo/dev-planning