Rietveld - code review Google style
code.google.com
code.google.com
Of all the things they showed me there, Mondrian (of which Rietveld is a fork of) impressed me the most. More so than dinosaur skeletons, space ships, 3D Google Earth terminals, or free amazing meals.
I was working in a startup at the time and we did not do code review at all. After seeing the Google workflow and having their build processes explained to me in detail, I was trying to set up code review before commits at the startup too. I hoped to improve code quality with this. Unfortunately the tools we tried were terrible and so it never took off.
To this day, not getting the necessary processes in place remains one of the biggest regrets of working there. Now I work at another place where we don't do that either. Too bad.
Even leaving aside the advantages of a DVCS such as Mercurial, I think that Review Board has a cleaner, more responsive UI. The "interdiff" feature (i.e., show diffs between changesets in this review request, not against the trunk) is pretty great, too.
Also, as a fairly mainstream Django app, it's not dependent on the App Engine runtime, which lets you run it on any standard Python/WSGI-capable web server on any recent version of Python. (App Engine only supports 2.5.)
If you're using Mercurial or Git, I'd highly recommend you check out Review Board; the initial setup is easy if you've deployed any WSGI apps before, and the web API makes it pretty easy to tie into your ticketing/CI/IRC bot/etc.
I haven't used Review Board, but I have however used Rietveld and I don't have any complaints about the UI. It also provides the "interdiff" feature, just choose the patchsets you want compared for a specific file, when you go to the diff page.
[1]: http://code.google.com/p/gerrit/ [2]: http://git-scm.com/about
As much as I like Gerrit I don't think I can use it for our team because Gerrit wants you to review commits, and our team works in terms of whole branches. Reviewing your individual commits made early in the branch history probably isn't useful as bugs may be fixed--or the code entirely changed--by later commits. Anyone else have this problem with Gerrit, reviewing commits vs. reviewing branches?
If you wait until a branch is done, then reviewers are being asked to look at big chunks of code. That is a lot more effort, and it is harder to get reviewers to do it. And when they do do it, it is harder to get them to do a thorough job of it. Furthermore when they notice design issues, it is harder to get developers to go back and redo all of their working code to make it better.
Therefore if you want effective code review, you need to review early and review often. The smaller a change is, the easier it is to review, the easier it is to be thorough, and the easier it is to accept suggested changes.
My conclusion is therefore that choosing to do code review after the fact means that you are guaranteeing no proper code review. But when you review early - literally on every commit - it is then much easier to maintain a truly effective code review process. It initially "feels" much heavier. But after you get in the rhythm, your code will be much, much improved.
- dev works on a feature branch, making multiple commits and pushes
- once ready, squash all the commits and submit it to gerrit
[- perhaps have hudson/jenkins run the unit tests at this point automatically]
- have the code review in gerrit
- once the review is done, gerrit would merge it into the develop or master branch (depending on your git workflow)
That approach blends in nicely with git-flow (1). If you want to be sure that no single dev is pushing to the develop or master branches you'd need to setup per-branch permissions, which can be done with gitolite (2). Too bad github (even the self-hosted version) doesn't support per-branch permissions, which forces organizations that use it and only want gerrit to be able to push into the main branch to do excessive repo forking instead of using feature branches. Also I'd love to be able to do ad-hoc code reviews in github, as the interface is the most beautiful of all imo.
[1] http://nvie.com/posts/a-successful-git-branching-model/, https://github.com/nvie/gitflow, http://jeffkreeftmeijer.com/2010/why-arent-you-using-git-flo...
It get worse. If you have a nice branch with several commits, they must all be reviewed separately with little help to review the branch as a whole. This feels like a throwback to the old days with CVS and patch-sets. Branching and merging suddenly becomes expensive.
If you combine this with a policy of auto-merging upon successful review, you are basically screwed if you have multiple depending commits. You must use rebase and squash to make it livable.
Sorry for the bitterness :-) I just had to review a single commit with 4000+ lines in gerrit.
The things you are complaining about are all things that I enjoy about gerrit. I tend to rebase on pull and use cherry-pick as the submit mechanism so it fits naturally into the flow of my groups.
Why are they still using it? Inertia, I assume.
The essential difference that annoys me to no end: - In Mercurial / Git, you must be synced to tip in order to push. (to avoid creating new heads) - In Perforce (and other old style VCS), you can commit as long as the files that you are committing are based on the newest version.
It is impossible to have any sort of sizeable team commit to the same DVCS repo. With ~12 software engineers, I already have to sync 25 times a day, even if I'm the only one touching my area of the codebase.
Most Google engineers commit to the same repository, and everyone gets the benefits of building from head (instant fix propagation from other systems, rather than waiting for releases). There is no way for everyone to work on the same repo with a DVCS afaik. It would require some sort of complicated hierarchy of repos.
Feature branches are the way to work (for me). You're pretty much on your own in there, and once the thing's done you push to main.
This is the old optimistic vs. pessimistic locking debate. I remember that at my first job, we used SourceSafe, and it would lock the files when you check them out so that nobody else could edit it. When we switched to CVS, I asked "But what happens if people make incompatible changes to different regions of a file and conflict detection doesn't catch it?" The answer was "Then the next person who checks out the code gets a compile error and we deal with it. It just doesn't happen that often in practice."
I've found that the problem you've had just doesn't happen that often. In the two years and hundreds of changes I've made at Google, I can think of a handful (< 5) of times that a CL has broken the build or caused a bug because of a bad sync and yet not caused any conflicts. And even then, the continuous build catches it and it either gets rolled back and tested properly before resubmit, or somebody patches it and we move on.
I've found that any feature branch that lives longer than a week becomes essentially impossible to integrate - in the time necessary to bring it up to head, head has changed enough that you then need to integrate it again, and so on. This obviously depends on team size though - if you've got 10 developers working on a piece of code, it's going to have a lower change rate than if you have 500 developers working on it. Of course, if you have 10 developers working on the code, the chances are miniscule that one will submit something that conflicts with your change in the window after you sync & test, and you can just yell out across the room "Hey, anybody submitting anything that conflicts with my change?"
DCVSs don't stop you from doing your commit, doing a naive merge, and then optimistically pushing. That option is still there, and is partly why so many people rebase before pushing their changes. Of course, pessimistic integration is equally available as an option.
If you find the overhead so high, it suggests that it's process holding you back and forcing you to pessimistically integrate, rather than the DCVS doing so.
Not disagreeing. My current working theory is that integration difficulty increases exponentially with merge distance.
And yes, team size is a factor - but I'd argue that if 500 developers all work in the same area of the code, something else is wrong :)
To get back to the original point: You sync to head, and you compile. I'm not suggesting a full QA cycle, but especially if you changed APIs, that's the polite thing to do.
This would suggest that the developer who checked in changes to A, which is used by B, did not also make the changes to B so that it works with the changes to A. That seems to be the actual careless action to me.
So unless you catch up to reality (i.e. sync), you won't even know you should make those changes to B. Hence my post ;)
See Android, which already has a wrapper around git ( http://source.android.com/source/git-repo.html ) to sync multiple repositories as a single unit.
Well, that and that migrating to anything else would be a huge nightmare. They have a ton of infrastructure written around Perforce, just for starters. Then there's the fact that moving to, say, git, would require significant refactoring of their heavily incestuous code base. Moving to Subversion would be easier, but svn's problems with branching and merging would probably make that a non-starter.