On 24/03/11 19:11, Ashar Voultoiz wrote: Please read: CR fits our actual needs ^^^^ // Having English lessons is actually on my TODO list. -- Ashar Voultoiz

2011/3/24 Neil Kandalgaonkar <neilk(a)wikimedia.org>rg>: You're basically arguing for Linux-style pre-commit code review where people e-mail patches back and forth. However, as long as we still have SVN, that means that these pre-committed patches ARE NOT VERSIONED, let alone necessarily public. I believe this is bad because: 1) Keeping track of patches, collaborating on a larger feature, etc. become harder (no benefits of a VCS) 2) Resolving conflicts between patches is done by reviewers when they apply them instead of being conveniently outsourced to the author-committers 3) If review capacity is low, patches don't get committed, their authors bug reviewers a few times, give up, get demotivated and leave the project I think this workflow could work with a DVCS with Git, but I strongly oppose implementing it while we're still using a centralized VCS like Subversion. Instead, let me outline my recollection of how code review and deployment worked back when I joined this project, and explain how I think this process can be resurrected. This was all a long time ago and I was fairly new to MW, so please correct me where I'm wrong. * Someone commits something * A notification is sent to the mediawiki-cvs list. This is still the case, except back then more than a few people were subscribed to it, and traffic wasn't as high * Optionally, a mediawiki-cvs reader (the usual suspects being Brion, Tim and Rob Church) reads the diff and notices something is wrong with it. They reply to the commit notification, citing parts of the diff inline and raising their objections. This reply is automatically sent to wikitech-l (we didn't have the CodeReview extension yet), which committers are expected to be subscribed to. A discussion about the commit takes place, possibly leading to followup commits * The next Monday, Brion smoketests HEAD. If he finds breakage, he tracks down the offending revision(s) and reverts things until everything seems to work. ("Keep trunk runnable" was taken really seriously, and we mostly had a revert->reapply cycle instead of a fixme->followup cycle: it was perfectly acceptable to revert broken things if they couldn't be fixed in 5 minutes, especially if you were as busy as Brion.) * In addition to smoketesting, Brion also reviews all revisions to phase3 and WMF-run extensions (with the level of commit activity we had back then, this wasn't an unreasonable job for one person to do on one day) and reverts things as appropriate. * trunk is now in a state where it seems to run fine on Brion's laptop. Brion deploys trunk to testwiki, tests a bit more, then deploys to the cluster As you know, our workflow has become a bit different over the years. At some point, CodeReview was written to make revision discussions nicer and to provide status fields so Brion could outsource some review work. Later, the WMF branch was introduced to not only track live hacks and WMF-specific changes, but also to remove the dependency on a runnable trunk. The reason this workflow resulted in frequent deployments of trunk was that review was that review was always close to HEAD (never behind more than about 2 weeks). The reason it broke down in the end was that Brion kept having less time to review more things, but that doesn't mean we can't make it work again by having more than one reviewer. I think the following conditions are necessary for this to happen: * We need to have multiple reviewers (duh) * Every reviewer needs to budget time for code review, and they need to not get tangled up in other obligations to a degree where they can't spend enough time on review. This is largely a management thing * It needs to be clear who is responsible for reviewing what. This doesn't need to be set in stone, but we have to avoid a situation where revisions aren't reviewed because no one feels responsible. This can be accomplished by agreeing on review assignments based on e.g. path/subsystem, and having some manager-like person (possibly an EPM) monitor the process and make sure nothing gets left by the wayside. If conventional review assignments leave too much ambiguity, additional criteria can be introduced, e.g. the day of the week something was committed. More on this in a minute * There needs to be a clear expectation that commits are generally reviewed within a certain time (say, a week) after having been committed. The same manager-like person should also be keeping an eye on this and making sure overdue revs are reviewed pronto * We need to set a clear policy for reverting problematic revisions (fixme's) if they aren't addressed quickly enough (again, let's say within a week). Currently we largely leave them be, but I think we should go back to something more decisive and closer to the "keep trunk runnable, or else Brion kicks your ass" paradigm and make it a bit more formal this time As for the who-reviews-what criteria, I think they should be designed to ensure most things will get reviewed without EPM intervention being necessary, while not getting in the way of getting things done. I think the best way to do this would be to first assign revisions based on path or subsystem (e.g. Tim for parser revs, Sam or me for API revs, etc.) then for everything that doesn't neatly fall into one subsystem someone's an expert on or can't easily be found by a path search, we'd divide the week in a number of chunks and make one reviewer responsible for reviewing the 'misc' revs in each chunk (or throwing them to someone else if it's over their head). If this is timed in a way that corresponds nicely with our reviewers' availability (e.g. schedule around school for people like me, schedule Tim for Sunday cause that's his Monday, etc.) we could have a system where the median time to review is like 24-48 hours. These are my thoughts, and I'm definitely interested in talking about this in Berlin, as Sumana suggested. Roan Kattouw (Catrope)

I think Roan hits it on the nose. Most of the problems Ashar and Neil raise are flaws in our code review process, not flaws in the tools we use *to do* code review. I actually think that CodeReview works quite well, **for the system we currently use**. I think many of us agree that, one way or another, *that system* has major flaws. The fact that one discussion has quickly fragmented into fresh threads on *all* of the 'big three' (code review workflow, VCS, and release cycle) illustrates how intimately connected all these things are. It makes no sense to choose a VCS which doesn't support our code review workflow; if our code review is worthless if it does not support a coherent release cycle; and the release workflow (and the to-freeze-or-not-to-freeze question) has a dependency on the VCS infrastructure. Ultimately, though, it's a mistake to think of any of these issues as technical questions: they are **social** problems. We have to choose the *mindset* which works for us as individuals, as a group and as a charitable Foundation. Currently our development mindset is of the Wild West: pretty much everyone works alone, on things which either interest them or which they are being paid to be interested in, and while everyone is responsible enough to fix their own bugs, our focus is on whatever we, individually, are doing rather than the finished product, because the product only *becomes* finished once every 6 months or so. The only reasons now that we keep trunk broadly runnable are a) it makes it easier for us to continue our own development, and b) the TWN people shout at us whenever we break it. I'm not, let me be clear, saying that said 'Wild West' mindset is at all a bad thing, it is very open and inclusive and it keeps us from the endless trivial discussions which lead to cynicism and then flames in more close-knit communities. But as Roan says, it is *not* the only mindset, and the alternative is one which is more focussed at every stage on how changes affect a continuously-finished product. We know the regime which is at the other end of the scale: the Linux kernel's universal pre-commit review, which I'm going to suggest we call the 'Burnt Offering' approach to coding as patches are worked, reworked, and inevitably reduced in number before being presented for divine approval. That has clear advantages, in ensuring very high code quality and probably improving *everyone's* coding skills, but also the disadvantages Roan mentions. The smoketest-trunk-every-week development model, which defies being given a crass analogy, is somewhere in the middle, and I think that's closer to where we need to be. If we made an absolute policy of scapping to the WMF cluster once a week, every week, it would force a shift in our mindset (arguably a shift *back*), but not one that's seen as an artificial limitation. No one will begrudge a release manager reverting changes on Tuesday afternoon which people agree will not be fixed in time for a Wednesday scap, while the same release manager spending Tuesday *not* merging changes for the exact same reason is seen in a much more negative light. We retain people's ability to make rapid and immediate changes to a bleeding-edge trunk, but still ensure that we do not get carried away, as we did for 1.17 and are still merrily doing for 1.18, on a tide of editing which is not particularly focussed or managed (witness the fact that out of the 15,000 revisions in 1.17, we can point out only about three 'headline' features). There are implementation questions to follow on from whichever workflow regime we move towards: for the weekly-scap process we need to find a replacement for Brion and his cluebat which is as reliable and efficient as he was; for a Linux-style system we need to sort out how to ensure that patches get the review that they need and that it doesn't just kill our development stone dead; and even to continue in the Wild West we need to sort out how to stop traceing out the Himlayas with the graph of unreviewed commits and actually get our damn releases out to prove that the system can work. My main point is that *any* technical discussion, about SVN/Git, about CodeReview or its alternatives, even about Bugzilla/Redmine, is premature unless we have reached an adequate conclusion about the social aspects of this combined issue. Because Git does not write code, nor does CodeReview or Bugzilla. *We* write MediaWiki, and we could in principle do it in notepad or pico if we wanted (some of us probably do :-D). The most important question is what will make us, as a group, more effective at writing cool software. Answers on a postcard. --HM

On Fri, Mar 25, 2011 at 2:27 AM, Happy-melon <happy-melon(a)live.com> wrote: I don't think anybody writes in notepad, as notepad inserts UTF byte order marks at the beginning of a text file ;) For the rest of your mail, +1 Bryan

On Fri, Mar 25, 2011 at 10:30 AM, Ilmari Karonen <nospam(a)vyznev.net> wrote: +1 to everything Roan said and +1 to everything above. -Chad

Ilmari Karonen wrote: +1 We should also add another state for fixmes that are not about problems in the revision itself, but request for improving more code (eg. you should fix the same thing -added in MW 1.4- in other 10 locations of the code, too).

Happy-melon wrote: Right. We should use an improve tag and stop using fixmes for that.