On Thu, Mar 24, 2011 at 9:27 PM, Happy-melon happy-melon@live.com wrote:
I think Roan hits it on the nose. Most of the problems Ashar and Neil raise are flaws in our code review process, not flaws in the tools we use *to do* code review. I actually think that CodeReview works quite well, **for the system we currently use**.
I think it works very poorly, independent of our code review process:
* The discussion system is buggy and unpredictably mangles markup (it probably shouldn't have attempted to support wikitext in the first place . . .) * It doesn't allow line-by-line review of patches * It doesn't allow sensible configuration of e-mail notification * It doesn't integrate with version control beyond just reading it (e.g., it could support merging to a branch from within the web UI) * It doesn't integrate with bug tracking at all
These are only the things I can think of off the top of my head without having even *used* decent code review software. I'm pretty sure that if I had used something like Gerrit a lot, I'd recognize a lot more drawbacks. The bottom line is, trying to write our own code review software is just a bad idea in the long run.
Ultimately, though, it's a mistake to think of any of these issues as technical questions: they are **social** problems. We have to choose the *mindset* which works for us as individuals, as a group and as a charitable Foundation.
There are technical problems here as well. The technical advantages of moving to a DVCS can be separated completely from the social advantages of the new code review paradigms we could adopt after doing so. Moving from SVN to git would be a step forward in a lot of ways even if we kept the code review and deployment process basically unchanged. But it's also a prerequisite for adopting certain types of code review, or at least it would make adopting those types of code review much easier, so we should really talk about switching to git before we consider a review-then-commit system.
We know the regime which is at the other end of the scale: the Linux kernel's universal pre-commit review, which I'm going to suggest we call the 'Burnt Offering' approach to coding as patches are worked, reworked, and inevitably reduced in number before being presented for divine approval. That has clear advantages, in ensuring very high code quality and probably improving *everyone's* coding skills, but also the disadvantages Roan mentions.
The real distinguishing feature of Linux development isn't pre-commit review, it's that it's pull-only and thus completely individual-oriented. Linus Torvalds personally decides what gets into the official Linux kernel, and no one else has any actual say (beyond trying to persuade him). He mostly delegates to maintainers, who in turn mostly run their parts of the kernel as fiefdoms as well. This approach is idiosyncratic, and closely related to the fact that Linux is produced by dozens of independent organizations with no central organizational oversight.
I don't think we should be seriously contemplating the Linux model for MediaWiki. The overwhelming majority of MediaWiki development is done by either Wikimedia employees, or volunteers who are closely connected to Wikimedia employees. MediaWiki isn't an individual's project, it's Wikimedia's project, so a totally decentralized version control process wouldn't match the reality of how development works.
I continue to suggest that we look at a process more like Mozilla's. Like MediaWiki, Mozilla's projects are developed under the central control of a not-for-profit organization (modulo wholly-owned for-profit subsidiaries that exist for tax reasons) committed to openness and community participation. It's much more accessible to new contributors than either MediaWiki or Linux development, and I can speak to that personally as someone who's submitted code to both MediaWiki and Mozilla. (Not to Linux, but the reputation of Linux development is consistently scary enough that I don't think I need personal experience . . .)
The smoketest-trunk-every-week development model, which defies being given a crass analogy, is somewhere in the middle, and I think that's closer to where we need to be. If we made an absolute policy of scapping to the WMF cluster once a week, every week, it would force a shift in our mindset (arguably a shift *back*), but not one that's seen as an artificial limitation. No one will begrudge a release manager reverting changes on Tuesday afternoon which people agree will not be fixed in time for a Wednesday scap, while the same release manager spending Tuesday *not* merging changes for the exact same reason is seen in a much more negative light. We retain people's ability to make rapid and immediate changes to a bleeding-edge trunk, but still ensure that we do not get carried away, as we did for 1.17 and are still merrily doing for 1.18, on a tide of editing which is not particularly focussed or managed (witness the fact that out of the 15,000 revisions in 1.17, we can point out only about three 'headline' features).
The key problem with this approach is that you retain the notion of people who have "commit access" -- who are in the privileged position of being able to submit their code without prior review. This creates an artificial barrier to new users' contributions. They have to use an entirely separate process to submit code, and it's very possible that even if we get review of in-trunk code running smoothly, patches on Bugzilla will remain neglected. After all, established contributors aren't submitting patches on Bugzilla for review, so the complaints about Bugzilla review problems will be few and quiet. Only a unified review system, where new contributors and core developers submit their code through the same process, will maximize accessibility to new contributors.
Mozilla uses Bugzilla for this, which is frankly a pain in the neck, but it works for them. Gerrit looks much better, although I have yet to see it really in action.
Of course, under this system we'd still have people with something resembling commit access. Mozilla does, unlike Linux. But commit access would only be for if you've verified something has gone through the proper review procedure, or for emergencies. So it would be a bureaucratic right that makes your life slightly easier, not something that lets you get code accepted more easily.
My main point is that *any* technical discussion, about SVN/Git, about CodeReview or its alternatives, even about Bugzilla/Redmine, is premature unless we have reached an adequate conclusion about the social aspects of this combined issue. Because Git does not write code, nor does CodeReview or Bugzilla.
No, but git is a much better tool than SVN overall. We should switch to it even if we have no intent of changing our development model. Since a lot of us do want to try out changes to our development model, that only strengthens the case for switching, since git is much more flexible and can be more easily adapted to different development models than SVN.
All this should only be after we get back to regular code deployment, though. That's certainly the most important thing. First get code review working with regular deployment, then switch to git, then switch to review-then-commit, is what I'd like to see. (Although my recent involvement with MediaWiki development has been so minimal that what I'd like to see probably doesn't matter much, to me or anyone else. I do hope to become more active again, but probably not for a couple of years at least . . .)