Re: The future of commit access policy for core Firefox

On Mon, Mar 13, 2017 at 2:35 PM, Nathan Froyd <[email protected]> wrote:

> On Mon, Mar 13, 2017 at 2:12 PM, Mike Connor <[email protected]> wrote:
> > 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.
>
> I think you're going to want to handle delegation of reviews, e.g.
>
> 1) I own/peer a module and I would like somebody to review my patches
> so I can get a sense of how thoroughly they review code and/or to help
> provide feedback on their review skills.
>
> 2) Similar to the above, maybe I get tagged for review, but I would
> like somebody else who doesn't own/peer the module to review the code
> because I judge them capable enough.  This and the above case are
> mostly about growing new reviewers.
>
> 2a) Perhaps I would just like a second set of eyes on the patch (e.g.
> threading issues) and am comfortable handing off review to somebody
> who is more expert in such matters and whose judgement I trust to
> handle any other issues capably.
>

My assumption is that "must be reviewed by someone already trusted" is a
minimum bar, and would not exclude additional reviews from another party.

In terms of growing new reviewers, the approach I've used many times in the
past was to have new reviewers take the initial pass, and then do a final
review myself.  That way they get to learn, I get to evaluate and coach
them on their review approach, and I remain confident that the review bar
was sufficiently high.  Once I stop catching issues that they didn't,
they're ready to be a full peer.

3) Sometimes I get asked about reviewing particular things and I ask
> the person who I know wrote the original code to review instead
> because they are more familiar than I am.  Maybe this can be addressed
> with ever-finer lists of code owners.
>

Finer-grained lists can work, or the owner/peer who is delegating a
specific review can formally approve (mark r+) based on the work of the
delegate.

I'd frame this as delegating work, but not responsibility.


> 4) Would just like to reiterate that the rewriting code case needs
> some thought, as somebody who has done such rewrites (and appreciated
> getting a single review) and as somebody who has reviewed code for
> such rewrites.
>

My thinking, as you noted further down, is that we can identify the set of
reviewers who can approve a broad change.  I'm reasonably confident that
the list of people who should be reviewing pan-tree changes is much smaller
than the number of people with domain expertise.


> If we require reviewers to appear in a trusted-list in-tree, then
> actually growing new reviewers is going to be a real pain.


See above.  I think the responsibility angle is key when delegating work.
"Trainees" can mark r+, but that r+ isn't enough to land.  The owner/peer
can make their own decision on how much they validate that work, from a
full re-review to a rubber stamp, but they're assuming responsibility
either way.

-- Mike