On Fri, Oct 3, 2008 at 9:48 AM, Roan Kattouw <roan.kattouw(a)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.