Super-review: policy draft
Mike Connor
("But I'm doing _much_ better now.")
This will probably replace http://www.mozilla.org/hacking/
reviewers.html, with the existing super-reviewer list bits kept intact
(and the 21 rules moved to MDC in an appropriate list).
-- Mike
=========================
Introduction
Mozilla has required code review from the beginning, and it is a key
factor in our success. Code review helps us produce better code and
better products in many ways, from catching bugs early, teaching and
mentoring new contributors, and keeping progress going in the right
directions. Strong module ownership and strong domain expertise are
essential, and we continue to work to improve these capabilities
within the project.
Super-review has its roots in the early days of Mozilla, when we
needed a core group of hackers to bring even the early module owners
up to speed. This group helped improve quality across the board, and
acted as a safety net against the tendency of a large group of hackers
to move in widely varied directions. Over time, we have learned a lot
about how to work fast and effectively, and in many parts of the code
we've done away with super-review in favour of deferring to strong
module ownership. This has been both a blessing and a curse, and it
has become clear over time that there was room to improve the review
process across our codebase to get the best of both worlds.
At this point, the super-reviewers group continues to be composed of
senior, experienced hackers who can add value across the codebase in
some specific ways separate from domain expertise. We have been
bouncing around the problem for quite some time, and the conclusion
has been to modify the review policy across the board to be a single
policy, using super-review where it will have the biggest impact, and
no longer using it where it is unnecessary. This will apply to all
code maintained in the mozilla-central repository, as well as any
release branch repositories based on mozilla-central.
What needs super-review?
* Significant architectual refactoring
All changes that involve significant changes to how code works and
interacts must be submitted for super-review. Code design is hard,
and getting more input helps us in the areas of maintainability.
* All changes involving security
For security bugs, and changes to security models or mechanisms, we
want extra experienced eyes to be involved with those changes.
* Any change to any API or pseudo-API
APIs are not just XPCOM APIs, but include global JS utility functions
and the like. Designing these better up front makes it easier to
build things on top of these APIs, and helps us avoid compatibility-
killing "API cleanup" down the road.
* All changes that affect how code modules interact
Any other changes that fall outside of the above rules, but affect how
two or more code modules function, must have super-review.
=========================
Mitchell Baker
could you be more specific about what sections of the existing doc you
are proposing this replace?
mitchell
Mike Connor wrote:
> Sorry for the much-delayed aspect, writer's block sucks sometimes.
> ("But I'm doing _much_ better now.")
>
> This will probably replace
> http://www.mozilla.org/hacking/reviewers.html, with the existing
Robert Kaiser
> What needs super-review?
Could we bring in the repos to which this applies here?
Robert Kaiser
Mitchell Baker
I know you're asking about the SR piece, but I'm going to respond on the
commit access piece anyway. My current draft is:
Mozilla has a number of source code repositories. This document
outlines the requirements for earning commit access to the following
repositories:
moz-central Hg repository
comm-central Hg repository
Mozilla's main CVS repository.
Requirements for creating and gaining commit access to an incubator
repository can be found at:
http://www.mozilla.org/hacking/incubator-repository.html#commit
A full list of repositories and their requirements can be found at:
https://wiki.mozilla.org/Commit_Policy:Current_Procedures
mitchell
Mike Connor
On 1-Jun-09, at 10:39 PM, Mitchell Baker wrote:
> Mike
>
> could you be more specific about what sections of the existing doc
> you are proposing this replace?
>
> mitchell
Hi Mitchell, sorry for the long delay here:
What I want to do with the existing doc contents is as follows:
* Separate out the Seeking a Super-review and Rules and Tips sections
into a general-purpose "before you request review" doc, since those
are not specific to super-review. This should probably go on MDC as
part of the evolving getting started guide.
* Keep the existing list of super-reviewers and areas intact.
* Leave the Exceptions section as-is for now, in the interest of
improving this aspect first.
* Use the revised text below in place of the first two sections.
-- Mike
Introduction
Super-review in mozilla-central
Effective immediately, super-review will be required for certain types
of changes for _all_ code residing in mozilla-central (and all release
branches based on mozilla-central) unless explicitly exempted in the
Exceptions section below. This includes areas which previously did
not require super-review under the previous policy. This is required
in addition to appropriate module owner or peer review in the cases
outlined below.
Mitchell Baker
1. On replacing the first two sections, did you intend to leave out the
logisitics part: "There is a revi...@mozilla.org mailing list, which
has a newsgroup mirror, . . . observe and help." ???
2. what about the CVS repository -- what super-review rules apply
there? Your current draft could be read to say there are no longer any
SR rules for CVS. Is that the intent?
Process
1. I would suggest getting this part changed as a step in itself. Then
go back and move the Seeking and Rules section elsewhere. We can
probably get some help from Gerv on fitting that into the MDC work that
bsmedberg has done.
2. I have some concern that a bunch of people who will be affected by
this -- everyone working in mozilla-central for example -- don't follow
governance and may be surprised. One of us should post in . planning I
guess with the very simple message: the rules for *your* check-ins are
changing, or something like that. Do you want to do this? If not I will.
mitchell
Mike Connor
On 19-Jun-09, at 9:09 AM, Mitchell Baker wrote:
> Content:
>
> 1. On replacing the first two sections, did you intend to leave out
> the logisitics part: "There is a revi...@mozilla.org mailing
> list, which has a newsgroup mirror, . . . observe and help." ???
Yes, as I am fairly certain that list is dead at this point. It's a
high-traffic list which no one posts to, and I don't think anyone
still reads it.
> 2. what about the CVS repository -- what super-review rules apply
> there? Your current draft could be read to say there are no longer
> any SR rules for CVS. Is that the intent?
Oops, these should continue to apply to CVS. Wording should be
something like "all code contained in mozilla.org CVS that is part of
the Firefox build is also covered by this policy.
> Process
>
> 1. I would suggest getting this part changed as a step in itself.
> Then go back and move the Seeking and Rules section elsewhere. We
> can probably get some help from Gerv on fitting that into the MDC
> work that bsmedberg has done.
Awesome.
> 2. I have some concern that a bunch of people who will be affected
> by this -- everyone working in mozilla-central for example -- don't
> follow governance and may be surprised. One of us should post in .
> planning I guess with the very simple message: the rules for *your*
> check-ins are changing, or something like that. Do you want to do
> this? If not I will.
I can do this, the initial thread/discussion was in dev.planning in
January, FWIW.
-- Mike
Mitchell Baker
mitchell
Mitchell Baker
mitchell