On 29/03/11 11:26, MZMcBride wrote:
Long ago I lost track of who's in charge of what, but I'm told there is some sort of hierarchy in place in the tech department. Who's empowered to start assigning people to review code in a reasonable timeframe? Like Aryeh, I find this entire thread a bit baffling.
The hierarchy is CTO -> EPMs -> regular plebs like me. The EPMs are Rob Lanphier, CT Woo, Mark Bergsma and Alolita Sharma. General MediaWiki work is mostly Rob Lanphier's responsibility, which is why he's been so active in this thread.
Rob doesn't know as much about MediaWiki as me, but he knows the people who work on it and how to manage them. I think his response with subject "The priority of code review" was entirely reasonable.
I'm not saying that I think MediaWiki code review should be the highest priority task for the Foundation, or that it's important to review all commits within a few days, as Aryeh contends:
Aryeh wrote:
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.
I'm saying that the Git/Subversion debate is peripheral, and that human factors like assignment of labour and level of motivation are almost entirely responsible for the rate of code review.
In the last week, I've been reviewing extensions that were written years ago, and were never properly looked at. I don't think it's appropriate to measure success in code review solely by the number of "new" revisions after the last branch point.
Code review of self-contained projects becomes easier the longer you leave it. This is because you can avoid reading code which was superseded, and because it becomes possible to read whole files instead of diffs. So maintaining some amount of review backlog means that you can make more efficient use of reviewer time.
Our current system links version control with review. After a developer has done a substantial amount of work, they commit it. That doesn't necessarily mean they want their code looked at at that point, they may just want to make a backup.
It's useful to look at such intermediate code for the purposes of mentoring, but that's not the same sort of task as a review prior to a tarball release or deployment, and it shouldn't have the same priority.
-- Tim Starling