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.

On Thu, Oct 2, 2008 at 8:57 PM, Brion Vibber brion@wikimedia.org wrote:

The current theory is we'd like it to be easy to mark things as needing *more* review, but hard to mark things as needing *less* review.

So that probably means a split-level permissions model, perhaps with distinct pre-review and super-review (to use the Mozilla term -- patch reviews get "super-reviewed" by a core committer, or some such crazy thing).

It is, of course, an evolving system. :)

Yeah, currently I'm only marking things "fixme". How about only you and Tim get put in the coder group, but that group only gives you the right to mark resolved/ok, while everyone can mark fixme? Is there a useful difference between resolved and ok, too?

Maybe the levels should be like:

* unreviewed: default * fixme: anyone can change a commit's status to fixme, from any other status (including reviewed/superreviewed) * reviewed: anyone can change a commit's status from unreviewed or fixme to reviewed . . . or maybe only from unreviewed? So this would just mean "someone other than the committer has reviewed this". May or may not be useful. * superreviewed: only privileged users (you and Tim) can move to superreviewed (of course this is a complete bastardization of the Mozilla terms :) )

And there should be some prominent message saying which commit is the last one that's scappable, i.e., preceded by only a) superreviewed commits and maybe b) commits to extensions (or even files, like DatabasePostgres.php) not used by Wikimedia.

A couple of things to think about:

* Should it be possible for ordinary people to clear "fixme" in some way? Maybe ordinary people should be able to change fixme to unreviewed or reviewed, *but* only if they added the fixme status themselves; and only you/Tim should be allowed to change it to other statuses. Maybe we could try this way out, and if there's too much noise, we could change it so anyone can resolve fixmes. * Should there be some way of indicating who's reviewed the commit? How prominent should this be in the interface? Should multiple people be able to mark a commit reviewed independently? Maybe we should track fixme/fixed status separately from review status altogether?

On Thu, Oct 2, 2008 at 11:56 PM, Alex mrzmanwiki@gmail.com wrote:

I think allowing anyone to change "fixme" to "resolved" shouldn't be a problem in most cases, as the fix would presumably be in the form of another commit, which would need its own review, and should indicate in the commit message that its a fix for the previous revision.

Well, in some cases, yes. But in other cases, "resolved" will mean "it turns out that there wasn't a problem after all" -- I've used "fixme" so far in more than one case to flag something that I thought had some minor issue, that others might disagree with. And in some cases, the commit that fixes the issue might not be clear on the fact that that's what it's doing.

Cool system, Brion!

On Fri, Oct 3, 2008 at 11:57 AM, Brion Vibber brion@wikimedia.org wrote:

Aryeh Gregor wrote:

...

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.

The current theory is we'd like it to be easy to mark things as needing *more* review, but hard to mark things as needing *less* review.

So that probably means a split-level permissions model, perhaps with distinct pre-review and super-review (to use the Mozilla term -- patch reviews get "super-reviewed" by a core committer, or some such crazy thing).

It is, of course, an evolving system. :)

I was thinking somewhat along those lines. It's nifty to be able to have people pitch into the review process - it might take some load off Tim and Brion for people to be able to point out problems with code before it hits the final review - that way, mostly good stuff is left for Tim and Brion to look at.

We could also have a preliminary tag for "looks okay, but needs to be looked at by somebody else before it's put live", so that less thorough reviewers (e.g. myself) would be able to look over code without actually marking it as "definitely OK", which should be reserved for thorough, trusted reviewers like Tim and Brion.

It also might be a good idea to add a 'tested' tag - for code which has been verified to at least *work* (ideally, of course, this will be done BEFORE committing, but perhaps testing by somebody else would aid in the review process).

Just a few thoughts...

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.

On Fri, Oct 3, 2008 at 2:25 PM, Roan Kattouw roan.kattouw@home.nl wrote:

Tell me about it. I spent quite a lot of time being annoyed at merge conflicts in the apiedit branch (and later the ApiEdit_Vodafone branch), but that was mostly because I didn't run the merge often enough (only like once a month).

Nothing will magically prevent merge conflicts, but my understanding is that RVCSes tend to be better at helping you solve them. git rebase, for instance, will start with the updated trunk, then apply each of *your* patches (probably a lot fewer than the patches applied to trunk since your branch!) one by one. When it hits a conflict, it will let you manually apply that specific patch of yours (or drop it from the patch set, if you like) before proceeding with the rest of your patches.

I don't know if git merge is similar, since I haven't used it much, and I haven't maintained any branches in SVN or Mercurial. But everyone seems to say that RVCSes are way better at this.

That would be a good argument for moving to another VCS; branches would also be less cumbersome.

Yes, that's one of their key advantages. There are plenty of others as well, of course.