On 03/24/2011 12:44 AM, Ashar Voultoiz wrote:
On 24/03/11 06:47, MZMcBride wrote:
It's only impolite if you criticize the code review tool without being constructive. What specifically do you not like about the current code review tool?
I agree with most of what Ashar said. Lack of branching, merging, blame, only semi-integrated with bug tracking.
And have you filed bugs about getting these issues
addressed?
My guess, which could be wrong, is that it would be cleaner to move to a new tool, or new combination of tools. I'm not sure which yet.
As for Special:Code et al, I disagree with the whole paradigm around which it is based -- effectively it just annotates revisions in SVN. This means that you need this "outer circle" of checkins that don't really count. And then there's the "inner circle" where some esteemed developer who's done a lot of cool things for MediaWiki, gets, as their reward, to constantly deal with other people's patches that they may not care about.
I believe that paradigm is broken because the incentives are backwards. Average developers are frustrated because no matter HOW much energy and commitment they have, they can't make code review go faster. The inner circle of production-branch committers are also frustrated because it's up to them to deal with all the pain of merging. Suddenly getting a feature production ready is THEIR problem, and sometimes super-productive people like Roan have found it easier to just rewrite it from scratch. Otherwise it's much easier to just ignore the backlog for long periods.
My ideal is a code review tool that:
- Allows us to deploy trunk. At any time. Eliminate the production branch. Any developer in the world should be able to work on the code we actually have in production without having to decide between trunk and a production branch.
- Allows the "outer circle" developer to take things into their own hands. They can check out the code that is develop a changelist, or set of changes, that is carefully crafted to be applied to trunk. If they are screwing up, they should get instant feedback, not six months later.
- Does not unduly penalize the "inner circle" developer. Give them a constant stream of light duties, not a soul-crushing marathon of heavy duties once a year.
I admire the code review paradigm at Google, which does all that, but which is regrettably based on tools that are not all available freely. So I don't have a 100% solution for you yet. I've talked informally with RobLa about this but I didn't have anything really solid to bring to the community. (In early 2010 I started looking at ReviewBoard but then I realized that the MediaWiki community had their own tool and I figured I should understand that first.)
There's been some confusion, so perhaps I have not been clear that I'm referring to a totally different paradigm of code review, where the code to be reviewed isn't even in subversion. Essentially the developers would pass around something like patches. Some systems make this work over email, but it's easier if it's tracked in a web based system. As developers work on the patch, the change log message or other metadata about the patch is annotated with developer comments. Sometimes you bring in more developers -- maybe there's some aspect that someone else is better situated to understand.
This process goes on until the patch is deemed worthy. Then, and only then, does it get committed to an authoritative repository, authored by some developer and annotated as reviewed by one or more other developers. (Emergency patches get a "review this later" tag).
Now, when code review happens before committing, you get the benefit that all non-emergency checkins have been at least looked at by somebody. Personally I believe this should be happening for everybody, even experienced developers. Even they make mistakes sometime, and if not, then other developers learn how to code like them by reading their changes more intently.
But then your code review system has to reinvent several wheels and you annoy the developer by making them do fresh checkouts all the time.
Git can do a lot of that in a much cleaner way, so I expect Gerrit might be an even better solution.
Anyway, this is all vague and clearly I'm talking about radical changes to the entire MediaWiki community. But I believe it would help quite a bit.
Maybe I should work on it a bit more and present it on a wiki page somewhere, as well as in person in Berlin in May?
Neilk is realist. Either we bring more developers in the system or we drop it and reuse another system already having some developers. For example, we are not developing our own bug tracker or webmail interfaces. We reuse code from others just like other reuse our Wiki code.
I would name a few issues with our CR system:
- does not known about branches
- lacks a manual merging system
- lacks an automatic merging system (something like: if rev and follow
up got 4 sign up, merge them all in release branch).
- a rev + its follow up could be grouped. We will then review the group
as a whole instead of individual revisions.
- I still have not figured out how to filter by author AND path
- comment system should be liquid thread based
- the diff is useless (I use a local tool)
- still have to rely on local tools for merging, reverting, blaming
- not integrated with bugzilla
There are lot of good points though!