On Thu, Oct 2, 2008 at 3:00 PM, Roan Kattouw roan.kattouw@home.nl wrote:
I'd like to suggest a system in which different people are responsible for reviewing commits to different parts of the code, and mark revisions as OK when they've reviewed them. The tag history I suggested on the TODO list would be useful there.
I think it would be a must to keep track of who's marking stuff as reviewed. As things stand, we probably want every commit to be reviewed by either Tim or Brion before a scap. We might also want certain classes of commits to be reviewed by other people, like you reviewing API changes, but in that case either Brion or Tim will still want to at least glance at them.
This is where pull-only DVCSes are pretty neat. Intrinsic to the process is that something's review status can be inferred from where it is: patches in anyone's tree will have been reviewed by them, or someone they trust. So we could all maintain our own trees and just post patches for Brion/Tim to review, and accept or not. You could also do something similar with a centralized VCS, the way Mozilla did while they still used SVN -- only allow reviewers to commit, and require everyone else (even established patch authors) to submit patches. But it's more of a headache with those, for a variety of reasons.
So actually, I think it would be good to think about whether we want to continue our current system of "commit then review" altogether, spiffy new tools or not, or whether we want to move to a "review then commit" system like some other projects have had for a long time (e.g., Mozilla or the Linux kernel). Major advantages of "review then commit":
1) No big barrier of commit access. Everyone has to submit patches just the same, with no direct commit access to a central repo, so there *has* to be a functional patch-review system. We've always been terrible about reviewing third-party patches, but that couldn't happen if practically all regular developers had to use the same patch-submission mechanism.
2) With "commit then review", a reviewer who spots a minor problem and who wants to fix it has to go to the trouble of committing a fix, or reverting the commit. With "review then commit", a reviewer who spots a problem just has to say so, and gets to *avoid* bothering to make a commit. This should encourage more meticulous coding standards, which is a good thing.
There are also more minor advantages, like trunk being more likely to actually work; fewer reverted commits cluttering up the logs and mailing lists; the ability to easily put off review of big features without having to deal with the mess of an SVN branch; and the ability to put off review of unimportant changes when there's an important one that needs to be cherry-picked and made live, on a per-commit basis rather than a per-file basis (which is what caused the file loss lately, wasn't it?).
Of course, if it were decided that there were some people who were trusted to review certain parts of the code, say Roan with the API code, in a DVCS, you could have that person maintain their own tree, and have people submit patches for that tree to them. The core maintainers would then regularly pull from the subsystem tree to their private box, briefly review all the commits, possibly undo any bad ones, and then push the good ones to the main tree. This could be applied to more people too, but preferably not too many, to avoid undoing the positive effects of (1).
On the other hand, I like git a lot more than Mercurial, so I'm all for holding off on the DVCS craze until git works acceptably on Windows. :P Apparently it sort of works now, with a GUI and everything, just not quite as mature: http://kylecordes.com/2008/04/30/git-windows-go/
And yeah, so I realize that I'm raining on weeks of hard work by saying we should chuck it out and go with a totally different system. So sue me. :) Moving to a DVCS would be a much bigger change anyway.
Since only you and Tim are coders right now, could you mark a few other devs (like me) as coders so this virus actually starts spreading?
Bureaucrats can also add people. I'm assuming that people who have had commit access for a reasonably long time and have shown they know how to use it should be marked coders, but I'm still really uncertain given that nobody's actually said what the various statuses are supposed to actually mean. Are people going to actually scap on the basis of nothing other than the fact that every commit is marked ok/resolved? If so, it's probably a bad idea for people other than Tim or Brion to add those markings, at least on a regular basis, unless we really want that.