Reviewing code can suck
The open source world has given developers great tools to make their lives easier. We have editors, bug trackers, source code management tools, repository viewers. Bugzilla, for example, is a very popular bug tracker used by many open source projects and companies alike.
While the life of a developer has in many ways been improved by these tools, there’s one key area of software development that people still do the hard way: Code reviews.
You’re probably familiar with this. A contributor puts a patch up on Bugzilla, Trac, or your project’s listserv. It’s large and spans several files. Eventually you get around to it (if you haven’t lost it in your inbox yet). You open an e-mail or the Bugzilla page to respond, put the diff where you can see it, and start going through the changes, line by line, making comments as you go. It’s tedious. You have to make it clear what function you’re talking about, make references to the general area in the diff. It’s a pain for the contributor too, because they have to figure out what you’re referring to. This leaves room for error.
We’ve been working this way for years in open source projects and at VMware. Code reviews are important to us, and help to keep our code clean and our products stable. It keeps us honest prevents us from cutting corners unnecessarily. However, the process is a bit time-consuming and not at all optimal.
- Generate an htmldiff showing the old code and the new code side-by-side, highlighting the changes.
- Write an e-mail to the target reviewers/listservs explaining what changes we made, the testing that was done, links to screenshots we put up on a file server, a link to the diff, and whatever else.
- Wait for someone to go through the htmldiff and comment on the changes in a new e-mail.
- Go through the comments, try to find the lines they referred to in the diff, and fix them up.
- Make another diff if necessary, repeat.
David Trowbridge and I finally got tired of it. We spent too long preparing review requests and we lost too many of them in our e-mail. As our personal projects and our team grew in size, it became harder to keep track of all the open review request e-mails on the listservs. So we fixed that.
Review Board
We built a code review system called Review Board. Like most projects, it started out simple, but grew to be pretty powerful and useful quickly. It was designed to automate and simplify the process of creating review requests and actually reviewing code.
Management at VMware was excited about this from the beginning. We weren’t being asked to write it, and it was a personal project and all, but when they found out we were doing this, they fully supported us. It didn’t take long for the excitement to spread across many teams, and we had 40 people signed up and playing before we were even ready to announce our phase 1 beta.
One of the things we decided from the beginning is that this must be open source and it must work with other version control systems. For example, we use Perforce internally, but there’s no reason not to include Subversion or anything else.
So we worked and worked in our spare time. After weeks of trial runs and lots of dogfooding, we decided it was time to announce what we had so far.
Enough yapping, details already!
Details? Alright.
Review Board is an open source program licensed under the MIT license. It was designed using Python and Django. It’s compatible with Subversion and Perforce, and can be extended to support other version control systems.
Review Board has a lot of useful features…
Multiple Repositories and Projects
Review Board can generate and display diffs against multiple repositories on multiple servers, each with their own version control system. At VMware, we have a repository for each Perforce server we use, and will soon be adding a Subversion repository for our libview project.
This would be particularly useful for umbrella projects where some parts are available in one repository and others in other repositories.
Writing version control system backends is easy. They’re simple Python modules that can be referenced in the Review Board database entry for the particular backend. We currently ship Subversion and Perforce backends, as well as a subclass of the Perforce backend making use of special extensions we use at VMware.
Review Requests
Posting review requests is fairly easy. If you use Perforce, it’s especially easy. A post-review tool is provided that allows you to post a review request with nothing other than a change number as a parameter.
If you use Subversion, a little more work is required, though we’re working on a tool to automate this as well. You’ll need to generate a unified diff. Click “New Review Request,” select the repository, enter the base path (the path relative to the root of your Subversion repository where you generated the diff), and then select the diff. Click “Create Review Request.”
You’ll be taken to your review request page where you can fill in description and other information. Click “Publish” when you’re done.
Powerful Diff Viewer
- Inline commenting: Instead of jumping to your bug tracker or e-mail client and describing where you’re commenting on, just comment directly on the line! Click the line number or click and drag to select multiple line numbers and a comment dialog will pop up allowing you to type. When you finally publish your review, the lines in the diff you commented on will be shown inline with the rest of your review.
- Inter-line diffs: Sometimes a small change is made to a line and it’s not readily apparent what the change was. In these cases, we highlight the regions between two lines that have changed.
- Revisioned Diffs: Each revision of the diff is saved and can be accessed. In the future, we will make it possible to show differences between two revisions, to ease review of incremental patches.
- Whitespace highlighting: Trailing whitespace is automatically highlighted. Trailing whitespace is a pet peeve of many developers and this helps to catch it early.
- Collapsing/expanding of files: By default, only the changed chunks and some lines of context around them are displayed. This can be quickly changed to show the entire file, in case there’s something you want to look at or comment on elsewhere.
- Keyboard shortcuts: Convenient shortcut keys allow quick navigation around the diff. For example, pressing “n” will jump to the next changed chunk, and pressing “p” will jump to the previous chunk. This will be further improved and documented soon.
Interactive Screenshots
- Screenshots can be easily added to the review request page, and will show up as thumbnails.
- Screenshots can be commented on by clicking and dragging an area on the image. A comment box will pop up, like in the diff viewer. While the clipped part of the screenshot does not yet appear on the review, it will soon.
Contextual Discussions
- Commented lines in the diff are displayed with their comments on the review page.
- Commented areas of a screenshot will be displayed with their comments on the review page.
- Discussion of reviews take place on the reviews themselves, making it possible to read the whole discussion of a change from top to bottom.
E-mail Support
All discussions and updates get sent automatically to the individual reviewers and listservs. This makes it easy for people to see any and all changes. This is configurable and can be disabled if it doesn’t fit in with your setup.
NIS Support
Review Board can use an NIS server as an authentication backend, making it easy to integrate into companies that use NIS. Everyone’s accounts will Just Work in Review Board without any registration required.
User Dashboard
All incoming and outgoing reviews can be seen on the Dashboard page. You can look at incoming reviews sent directly to you, sent to a group, or to either (all incoming reviews).
In the future, this will show more information on what’s going on with the review requests in your list. Recent discussions and updates will be shown, making it easier to keep track of things.
JSON API
Review Board has a JSON-based API for accessing nearly every aspect of the system. This makes it easy to build tools around Review Board. Our post-review tool uses this, as does much of the web UI.
Hopefully in time, developers will make use of this to better integrate with existing systems such as Eclipse, vim, emacs, or other IDEs.
Looking Forward
There’s a lot we have planned. Review Board is still just an early beta and a lot will happen.
- Improved tools: We’ll soon be working on a single tool that understands how to post review requests to both Perforce and Subversion. It will also be able to integrate with multiple projects. For example, if three projects you’re contributing to all use Review Board, post-review will ask once for the Review Board servers and use them from then on when run in the project directories.
- Interdiffs: We plan to include the ability to show changes between revisions of diffs, aiding in reviewing incremental changes to patches.
- Statistics: Information will be available showing how many review requests, reviews, and comments have been made between specified time periods, complete with fancy graphs. We’re working on ideas for what can be displayed here.
- Status Reports: This is perhaps more useful inside companies. At VMware, we’re supposed to submit weekly status reports discussing what we’ve done. Often this consists mostly of review requests made and bugs fixed. Review Board will be offering a page showing this information in several forms so that it can be easily copied and pasted into a weekly status report.
- Integrated Help: To simplify usage for first-time users, we’d like to add help information to all pages explaining simply how to use the interface.
- “Effective Lines” Display Many diffs are easier to review than they seem, as there could be hundreds of lines with nothing more than a function or variable name change. We would like to make this clear before even opening the diff by showing the number of effective lines changed, factoring out the simple redundant changes.
Wrapping Up, and Contributing
This was a bit long, but I hope it gave a good overview of what we’re putting together here. You can see it in action or visit the project page. If you decide to use it in your project, please let us know! Check the Wiki for install instructions.
Hopefully this will become useful to others as well. We’ll post periodic updates as this project progresses.
Can you give us a username/password to “see it in action”? Thanks!
This looks pretty badass. Agreed with Jeff though for more advanced systems like Mercurial and bzr your review can be of a branch instead of successive diffs.
But anyways though, clearly awesome, and looks great too!
Looks nice!
It would really kick ass to have this integrated into public bugzillas, some day…
You’ve seen this, right?
http://video.google.com/videoplay?docid=-8502904076440714866
You can create an account with the “register” link up at the top; it doesn’t require an email address or activation. This is a real live reviewboard server that we’re using to review code, so please don’t fill it with garbage 🙂
You can create a username/password by clicking the “Register” link in the upper right of the page.
I suppose we can always make a guest account with limited permissions. I’ll try to set that up.
Yo: Yep, we have. We started development on this shortly before Mondrian was announced, and thought “Maybe we can just use Mondrian!” Unfortunately, it doesn’t appear to be open source, and wouldn’t integrate well into our setup anyway. So development of Review Board was still justified.
Still, neat project. I’d love to see it live someday.
Okay, a guest account won’t happen right away. We would need to write some code for it. For now, just create accounts and please behave 😉 We’ll be watching to make sure things don’t break.
This is great! Thanks for releasing this, I’m anxious to see how hard it is to extend it to support Bazaar. Do you plan to make the bugtracker interface extensible also, so that it can work with things other than Bugzilla, like Trac and Launchpad?
Currently, the bug tracker integration is just a bug path with a “%s”, like “http://bugzilla/show_bug.cgi?id=%s”. There’s no integration beyond that. Works with Trac fine. We have our Review Board’s Review Board against Google Code’s Issue Tracker.
wow, awesome. This could be really great.
Awesome. I installed it with only a few hiccups, mostly dependency-related (and even those mostly because I didn’t read closely enough, but you need to list patchutils in the dependencies). It’s great.
I’m going to write an IDEA plugin (uhh, I’ll be busy the next two weeks, but probably shortly afterwards) and then switch our company to using it. We’re using CodeStriker, which works well enough, I suppose, but your solution is faster, prettier, and has more features.
I added patchutils to the GettingStarted wiki page. Thanks for the heads up!
Following the install steps in the wiki I get the following when I run manage.py for the first time after changing the settings_local.py.tmpl file
./manage.py syncdb
Error: Can’t find the file ‘settings.py’ in the directory containing ‘./manage.py’. It appears you’ve customized things.
You’ll have to run django-admin.py, passing it your settings module.
(If the file settings.py does indeed exist, it’s causing an ImportError somehow.)
On the wiki it should tell you to
mv settings_local.py.tmpl settings_local.py
When I start it with
./manage.py runserver
I can get to it via localhost, but I can’t get to it via my machine’s hostname. How can I get it to work with both?
Just an FYI, but the registration system doesn’t seem to work in Konqueror
Dude, I’m so glad this is finally out. Maybe now that there is a nice code review system out there, I can convince people to actually *do* code reviews.
Benjamin: Thanks, we’ll update the wiki to talk about settings_local.py some more. I would expect that the import error was due to settings_local.py being non-existant, or perhaps your version of Django is incompatible. It must be 0.96 or higher.
Also, if you run ./devserver.sh, it’ll give you a server that is accessible via other machines. Basically, it calls runserver with an IP of 0.0.0.0. Otherwise it defaults to localhost only.
At what point is registration failing in Konqueror? What errors are you seeing?
Review Board requires Firefox right now. We don’t have any support for Safari, IE, or Konqueror. Before our 1.0, we will likely support at least Safari and IE, but for the time being, there’s a huge chance Konqueror is going to fail in areas other than registration.
Were you aware of Codestriker, another Open Source code review tool? Too little research on your part, or is Codestriker lacking something that your tool has?
Zon: I haven’t seen Codestriker before. However, it appears to be lacking some of the metadata we put into our review requests. There’s also a number of features that we at VMware have in our htmldiffs that we would want in the diff viewer in the code review tool, which Codestriker lacks (namely, keybindings and optional full file listings. As a UI developer, I’m also not too wild about the look and feel of Codestriker personally. These things probably could have been fixed in Codestriker, but I’m not sure I’d want to maintain large Perl code 😉 Especially not a fork of an existing project.
There are also a lot of things we’ve added and are planning to add that Codestriker appears to lack. A public API for integration with our existing tools, for instance.. stats.. status report generation..
Thanks for pointing it out though. It’s nice to see what else is out there. I hope the various projects can get ideas from each other.
nice work. fyi, i work on opensolaris and i’m a big fan of code reviews. for opensolaris we have a home grown tool called webrev. you can see an example of it’s output:
http://cr.opensolaris.org/~dp/i2o-del/
the cool thing about webrev is that it presents the diffs in lots of different formats. for each changed file you get the following views: Cdiffs Udiffs Wdiffs Sdiffs Frames Old New Patch Raw. this is handy because, aside from different developers having different preferences, you can
choose the view that best suits the type of change your reviewing. you also get things like a pdf file (if you prefer to print out reviews to do them offline), a patch file, etc. unfortunatly, webrev doesn’t (and probably never will) have some of the nice features you’ve got here since it:
– doesn’t really support multiple repositories
– doesn’t allow inline commenting. (normally commenting is done out-of-band via email.)
– has a real web 1.0 feel to it.
– is an ungodly hack of a shell script
your tool seems to have a solid design and a great UI, so it’ll be interesting to see how it evolves.
Do you have to commit the changes to get a review request to work? We are required to review before submitting where I work.
AC: Nope, we review before submitting at VMware. All you need is to generate a diff and provide the review request details.
Yay review board! I just wanted to say: good luck, we’re all counting on you.
Thanks 🙂
Worth noting that while Django will work fine on a windows platform, this app uses some bits of Python that aren’t Windows compatible (Popen3 specifically).
Please don’t take that as a criticism, just a comment for anyone else coming along.
I think what you’ve done it fantastic, and now I just need to pitch together a linux VM to host one of these for myself to play with. Fantastic code!
Popen3 should be fairly easy to replace with subprocess, right (maybe I should look at the source first 😉
(hmm. I seem to have accidentally removed portions of that comment before submitting it. and I typed “human” into the food-captcha at first, and couldn’t quite figure out why I wasn’t allowed to post. time to see a doctor, I think. or at least get some more coffee…)
I couldn’t see how to annotate a source line that had an error in the diff view. Perhaps something not working in my browser? Also, on trying to put a comment on a review it said to Publish it but then came back with an error at the top of the page saying it couldn’t do it due to server problems. The error message was displayed off the top of the scrolled web page. I was down the bottom where my comment was, it was added (repeatedly on each server error) at the top.
Ralph: It may have been your browser. You’re just supposed to click a line. Which browser are you using?
The server error part may have been my fault. Shortly before you wrote this comment, I was clearing the database of several test posts and working to make it so only the developers could comment. Basically, this is our actual review board we use for the project, and it’s getting way too many junk comments. We need to set up a test server that people can go wild on, but for now I’m just going to be heavily moderating and deleting things. Sorry 🙁
I posted a blog entry about this, but this post appears to have a lot of Google Juice right now, so I’m posting the relevant news here as well 🙂
We’ve moved the demo server to http://demo.review-board.org/, and we now have an info site up at http://www.review-board.org/.
Feel free to play around all you like here without worrying about messing with our production reviews 🙂
Thanks for making this & making it OS. It really is an important tool for serious developers.
Do you know the Google Mondrian project? It’s an online code review tool and uses Django too.
Robert: Yep, look here: http://www.chipx86.com/blog/?p=222#comment-41710
Your tool seems to be really great. Though i am using bugzilla but i have just registered at review board. Lets see how it goes.
FYI: “See it in action” link above doesn’t work. And how come there isn’t a download link on Google project page?
The link has moved. It’s now http://demo.review-board.org/. This blog post is pretty old now 🙂
We don’t have an official release yet. Everyone’s been using SVN. However, we’re very close to our alphas and will have releases then. For the most part, though, people will be installing through Python’s easy_install.
Thanks for the clarification and prompt reply.
Wow, this article is pleasant, my sister is analyzing these kinds of things, therefore I am going
to inform her.
I don’t like the pre-commit model unless I have a dev and stage repo available and the pre-commit refers to the stage. It’s too easy to lose changes, keep in sync, etc. when you have to keep the modified code on your hard drive.