Aryeh Gregor schreef:
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":
- 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.
- 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?).
There's one big disadvantage of review-then-commit that you're omitting here. Because Brion and Tim are busy people, reviewing happens very irregularly. It's not uncommon for Brion to commit nothing for a week, then go on a reviewing spree (which typically shows up as a reverting spree in the logs). If people submit patches and have to wait for Brion or Tim to review them, those patches will spend a considerable amount of time (anywhere from an hour to a week) in limbo. While in limbo, patches are invisible to other developers, making submitting duplicate or conflicting patches really easy (a mess Brion and Tim will have to clean up). There's also the more general issue that some people want to know about changes as soon as they're made.
While I believe throwing commit-then-review out of the window isn't the way to go, you do make a good point. The best approach IMO would be to create a new branch (I'll call it "stable") that only contains reviewed revisions. Committers would continue committing to trunk the way they do now, and when Brion or Tim has reviewed a commit, it'd be merged with stable. If a commit fails review, it'd be reverted in trunk as usual. We're actually more or less doing this already on the WMF servers, except that syncing happens in batches and less frequently, and there's no easy way to view/download the code currently running on the servers.
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).
We could also do that in SVN with a stableapi branch, into which I would merge reviewed commits. Brion and Tim would then merge the includes/api directory from my branch instead of trunk, after brief review. (Note that the API is a rather special case: while Brion and Tim can review API commits for basic sanity, they don't (currently) know the API well enough to be able to review them thoroughly without reinventing the wheel every time.)
Roan Kattouw (Catrope)