On Fri, Mar 25, 2011 at 8:56 PM, Mark A. Hershberger <
mhershberger(a)wikimedia.org> wrote:
Platonides <Platonides(a)gmail.com> writes:
And no, nobody wants our review paradigm to be
"let's spend several
months on the backlog every time we want to release". It was just the
best we managed to afford.
We've been doing a little better for the past month, but Robla's
chart[1] is still looking ugly.
So, other than switching to the mythical GIT, where all is rainbows and
roses, what can we do to improve code review now?
tl;dr summary: The biggest single improvement that can be made is to *ship
code faster*.
When new code comes in, there's basically a few things it can do:
* break in an obvious and visible way
* break in some circumstances, which might not be obvious on first look
* mostly work, but be inefficient or have other negative side effects that
need fixing
* mostly work, but cause HORRIBLE DATA CORRUPTION that's not noticed for
some time
* work pretty well
Because we're afraid of letting hard-to-find bugs go through, we're holding
*everything* back for too long in the hopes that we'll somehow develop the
ability to find hard-to-find bugs easily. As a result, the code paths don't
get exercised until a giant last-minute review-and-push comes through months
later, and finding the actual source of the bugs becomes even *more*
difficult because you have 6 months of changes to search all at once instead
of a few days.
Ship sooner -> fail faster -> fix quickly. Update the live site no less
frequently than weekly; update test sites more frequently than that. Make
sure those test sites include things that developers are actually
dogfooding. Encourage other testers to run on trunk and report issues.
A smaller, but still relevant issue is to see if we can change how we think
about review priorities: something that changes the core of the parser or
how pages get saves might well cause HORRIBLE DATA CORRUPTION, but changes
in UI code probably won't. Changes in UI code might cause an XSS
vulnerability, however... so when thinking about how much attention code
needs, we should be considering the module rather than laying down blanket
policies.
Some more explicit reviewer module 'ownership' could indeed be helpful --
especially if we have a more explicit review process for 'big changes', but
even for less formal review.
As far as I can see, the main reason that people think code review
works better under GIT is because the committer is
responsible for
getting xyr[2] code reviewed *before* it is merged. The committer is
motivated to find get xyr code reviewed because if xe doesn't, the code
won't be used, and others will not experience its beauty.
That isn't specific to git; the same methodology works in SVN or CVS or
whatever where you're reviewing patches submitted through email, bug tracker
systems, etc. The advantage git has here is that your intermediate work is
easier to keep and share within the revision control system, as opposed to
having to keep your work *outside* the version control system until it's
been approved by someone else.
IMO that's a big advantage, but you can still do review-first with SVN, and
we always have for patches submitted through bugzilla or the mailing list by
non-committers.
If review and application of submitted patches can be made consistent and
reasonably speedy, that would again be a big improvement without requiring a
toolset change: getting more good stuff through, with no danger of it
breaking things _before_ approval & merging.
So, while Subversion doesn't support review-first, it can incorporate
revert-later. We can even use our current CodeReview
tool. We just
need to be more aggressive reverting unreviewed code.
And just to be clear, there would be a not-too-distant “later”. I
propose a week.
If code is to survive past a week in the repository, it has to be
reviewed.
If you want to make a commit that depends on
un-reviewed code, you have
to find someone to review it. Otherwise, your commit will break trunk
when that code is reverted.
This is actually a lot harder than it might sound; even in only a week,
trimming out dependency on dependency on dependency can be extremely
difficult, especially if some change involved lots of giant whitespace
cleanup or variable renames or other things that play hell with patch
resolution.
Reverting generically questionable code should probably happen a lot faster
than after a week.
-- brion