On Fri, Mar 25, 2011 at 8:56 PM, Mark A. Hershberger < mhershberger@wikimedia.org> wrote:
Platonides Platonides@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