On Fri, Mar 25, 2011 at 11:56 PM, Mark A. Hershberger mhershberger@wikimedia.org wrote:
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.
I don't think that's the right way to put it. In a properly-functioning review-then-commit system, it should be easy to get code reviewed. The advantage of reviewing the code first is that psychologically, it's much easier to say "Fix these minor things and then I'll approve it" than to say "Fix these minor things or else I'll revert it". The first gives positive incentives, while the second gives negative incentives, and people appreciate positive incentives a lot more. In a review-first system, you're going to routinely have reviewers asking that the patch author write better comments or conform to style guidelines or simplify the logic a bit before they give approval -- or even restructure the changes entirely. In a commit-first system, reviewers are going to be reluctant to revert code that works, even if it has some minor deficiencies, so committers have little incentive to fix minor code issues. Code quality suffers as a result.
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 a terrible idea. Review needs to be something that everyone is guaranteed to get without effort on their part. You cannot design a code review system on the theory that code authors are supposed to somehow get their code reviewed when no one is formally required or expected to review it. I'm all for giving people incentives to do the right thing, but incentives are pointless if the person being incentivized has no way to do what you're trying to get them to do. Incentives have to be placed on the code *reviewers*, because they're the only ones who can decide to review a given patch. Conveniently, almost all the code reviewers happen to be employed by Wikimedia, so the incentive can be the good old conventional "your boss told you to".
If, as Tim says, Wikimedia developers were un-assigned from code review after the 1.17 deployment, *that* is the problem that needs to be fixed. We need a managerial decision that all relatively experienced developers employed by Wikimedia need to set aside their other work to do as much code review as necessary to keep current. If commits are not, as a general rule, consistently reviewed within two or three days, the system is broken. I don't know why this isn't clear to everyone yet.