

Rietveld - code review Google style - btilly
http://code.google.com/appengine/articles/rietveld.html

======
sp_
Back in November 2009 I was on vacation in San Francisco and decided to drop
by the Google campus in Mountain View to visit some friends.

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.

------
rcoder
My last team switched from Subversion/Rietveld to Mercurial/Review Board about
a year ago, and I have to say that I much prefer the latter.

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.

~~~
vinutheraj
I think the Go project uses Mercurial but does code reviews using Rietveld.
They have some python code which helps in sending patches for codereview to
Rietveld.

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.

------
coffeejunk
have a look at gerrit[1] if you are a git user. it's a complete rewrite of
rietveld (in java) and tied closely to git which is because the main
contributor is Shawn Pearce (who is also main contributor to git[2]).

[1]: <http://code.google.com/p/gerrit/> [2]: <http://git-scm.com/about>

~~~
draebek
I think Gerrit is a really impressive piece of software. Please correct me if
I'm wrong but if you're looking for a major user of Gerrit I believe the
Android OSP uses it.

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?

~~~
btilly
I'm sorry, but I think that your team has this backwards.

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.

~~~
dkl
We use gerrit at my company, and what you say is true. Developers are told to
do one feature or bug fix per commit/gerrit change. It really does help with
the review process and things move much faster.

------
angryjim
Why not just use Crucible if you are running a small startup? There license
cost is $10 right now, and it has a lot of niceties.
<http://www.atlassian.com/software/crucible/>

~~~
Confusion
Seconding this. We are using it, alongside Jira and Fisheye and they are
really great, well-done tools. Atlassian will gain a customer when we grow
large enough.

------
jojopotato
Doesn't bugzilla (although ugly) handle the same workflow that he is
describing? We have svndiff linked in for the side by side diffs, but that is
the only thing missing.

~~~
btilly
I don't see how bugzilla handles the same problem. The key feature of the
workflow is that a commit cannot succeed until code review is complete. This
is very important for reducing how often you break the build. With bugzilla
integration your bugzilla tickets contain the history of commits that are tied
to that bug, but that is a different problem.

------
ghempton
Anyone know why google is so committed to perforce?

~~~
bdb
We take for granted today that there are several fast, stable DVCS generally
understood by an average engineer (in other words, the bar for using a DVCS
has been lowered in the last few years and it's not a purely academic
endeavor.) When Google was ramping up, Perforce was dramatically faster and
more reliable than the almost all of its competitors. It was one of the most
common source control systems to find at Valley companies.

Why are they _still_ using it? Inertia, I assume.

~~~
robfig
I was way happier at Google using Perforce than I am today using Mercurial.

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.

~~~
groby_b
If you do not sync and test your changes against the latest, I'd argue that
you're being careless - how do you know things still interact properly?

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.

~~~
nostrademons
If you don't sync and _code_ against the latest, how do you know things still
interact properly?

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?"

~~~
lars512
"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."

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.

------
andrewljohnson
If this would have come around 6 months ago, then I would have used it. Now,
I'm satisfied with GitHub and its code-review facilities. We already ditched
SVN.

~~~
mattyb
Rietveld has been open source for years.

~~~
andrewljohnson
Now that you say it, I think I remember my co-founder evaluating it at the
time and deciding not to bother. I forget why.

------
darwinGod
the date on that link is May 2008. havent started seeing the video yet, but
curious to know why this is news now

~~~
btilly
It is something that came to my attention recently which I thought would be a
best practice of interest to people on HN. I hadn't seen it discussed here.
Therefore I posted the link, and apparently some other people agree with me
about its value.

