On Fri, Oct 3, 2008 at 9:48 AM, Roan Kattouw roan.kattouw@home.nl wrote:
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.
In that case I would suggest we change trunk to be an unstable testing ground, and not be so rigorous about requiring that it be guaranteed to work at all times (although any brokenness should still be reverted as soon as it's discovered). Commit access to unstable could be given out more liberally, and committers could be encouraged to be more generous in committing patches to it if they look okay but maybe haven't been thoroughly reviewed.
Since stable shouldn't lag too far behind unstable, people should still be able to submit patches relative to stable, which would be reviewed and enabled on Wikipedia and so probably not horribly broken. People running trunk production wikis would switch to stable, while people running development-only wikis would use unstable.
This way we'd seem to get the best of all worlds. Of course, we'd probably need to use a DVCS for sanity.
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.
Yes, but merges in SVN are *terrible*. svn merge just applies a diff, with no authorship info preserved, all as one giant commit. DVCSes actually copy all the commits on merge, so all the authorship info (author name, date) and the full list of individual changes and commit messages is preserved. You can easily cherry-pick individual commits into your stable branch, or pull a whole bunch, without any merge commits, without svn log making it look like you wrote the commit (commit info still exists, but it's separate from authorship info), etc.