I’ve recently been spending some time working on a light-weight web-based patch review tool, called Splinter, to go along with Bugzilla. I’m not quite ready to reveal it to the world, but I wanted to deal in advance with a question that is certainly going to come up – why not use Review Board? This is a very reasonable question to ask, especially since Splinter was directly inspired by seeing Review Board and saying to myself “I want one of those!”
In thinking about this, I realized that the comparison wasn’t really between Review Board and Splinter, but between Review Board and attaching patches in Bugzilla. You really have to look at the overall process. How do you go from a enhancement request or reported crash, to a first candidate code change, to reviews on that change, to a final version of your change, to pushing those changes to the official repository, to closing the original report? git-bz automates and makes convenient large sections of this. Splinter is just a solution for a gap in the process – how do you review a patch once it’s attached to Bugzilla? But Review Board replaces the entire thing: you don’t attach patches in Bugzilla, you just link to a review request that you’ve filed (with your patch) in Review Board.
First I should list some of the advantages that Review Board would bring to the overall process: first, it has a very slick and well developed web interface for reviewing and commenting on patches. The tool I’ve been working on is pretty nice for a couple of weekends of work, but there’s only so far you can go in a couple of weekends. Second, Review Broad has a very well-defined workflow. Each review request is submitted to a person or group for review. You are presented with your outbox: your patches waiting for review, and your inbox: patches that are waiting on review. With our Bugzilla, the process is buried in a large and complex user interface and is much less explicit. (Attachment flags can be used to do explicit review requests in Bugzilla, but we don’t use them on bugzilla.gnome.org.) Finally if we used Review Board for patch reviews in GNOME, we’d reduce the amount of noise for bug reporters. While I imagine it’s exciting to see two developers discussing obscure details of a patch to fix your reported crash, I also can imagine that it can be confusing and intimidating, and maybe people would prefer to just get a high-level summary when a patch was comimtted rather than all the details in their mailbox.
So, Review Board stacks up pretty well for web user interface and developer workflow. Where things become more problematical are the integration with Git and the integration with Bugzilla.
The normal way to interact with Git is from the command line, so any integration of a review tool with Git will start there. While the web portions of Review Board are highly advanced, the Review Board post-review command-line leaves a lot to be desired when compared to git-bz. It doesn’t follow the git command line conventions. It can’t take patches from Review Board and apply them locally. It can’t insert references to the review into the commit messages. And so forth. That’s partly a question of attention, and probably even more a question of being cross-version-control-system. There’s only so much you can do while handling Subversion, and CVS, and Git, and Mercurial, and …. Certainly a specific “git-rb” tool is possible that does many of the things that git-bz does is possible – the lack of such a tool isn’t an inherent defect of Review Board, just a current lack.
There is a more fundamental problem here – a review in Review Board is of a single diff (potentially with multiple revisions), but that’s not very natural to how people work with Git. More complex enhancements are almost always done as patchsets, with each patch in the set being kept as standalone as possible. In the Bugzilla/git-bz workflow, the entire set of patches attached to a bug logically makes up a patch set. git-bz makes it easy to attach a series of commits to a bug as patches or apply all the patches from a bug to a local branch. Individual patches can be commented upon, marked as accepted-commit_now, replaced with newer versions, but it’s only when everything has been agreed upon and pushed that the bug can is closed. Trying to handle this explicitly in the Review Board user interface would require some substantial changes: you’d want a Review Request to cover an entire patch set, but have Reviews apply to individual patches within it.
The issues with using Bugzilla and Review Board together lie both on the user side and on the infrastructure-development side. On the user side, there’s a simple but non-trivial problem that we’d suddenly have two different identifiers and URLs for the same issue – there would be the Bugzilla bug URL but also the review board review request URL. Which one should you reference in in your commit message? Which one do you search for? There would also be two separate accounts to deal with.
On the infrastructure side, there’s the standard work of installing, maintaining, upgrading, etc. Beyond that, we might want to try and figure out some sort of single-sign-on across Bugzilla and Review Board to help with the multiple accounts problem. We’d have to have some automated process to keep the repository list in Review Board in sync with the 600+ GNOME repositories and auto-create maintainers groups for the repositories. We’d need a bugzilla extension to link to reviews. We’d need a Review Board extension to update bugs in Bugzilla when a review was created.
My summary here is that moving GNOME to Review Board for patch review would be quite a bit of work and a substantial change in our current work habits. There would be significant advantages, but also some definite downsides in having to juggle two systems. If it’s possible to, with a small addition on top of Bugzilla, get a reasonable approximation to the web-based patch review aspects of Review Board without adding an entire second system, I think that’s pretty interesting. Once I put a demo of Splinter out there (days, not weeks), you can judge that “if” for yourself.
fishsoup 
7 Comments
Hi Owen,
It’s good to see articles like this so we know what Review Board is lacking in the eyes of some projects. However, I think there’s a few things I should clarify, and also a few upcoming changes that you may want to know about.
First off, this article seems to make the assumption that there’s a 1:1 mapping of review requests to bugs. This is often not the case. Plenty of changes posted for review are new features that don’t have an accompanying bug. Others may reference multiple bugs. Some bugs may have multiple review requests. It’s very likely that a bug would appear to be fixed by a change that was up for review but would then later be re-opened, and a new review request posted.
Because of this, I don’t see multiple URLs as being any sort of problem. If you’re discussing a bug, by all means reference the bug URL. If you’re talking about a particular change, which may or may not reference a bug itself, then the review request’s URL is more appropriate.
You had mentioned the interaction (or rather, lack thereof) between Bugzilla and Review Board. This is certainly something many people have noted and there’s been a lot of demand for tighter interaction. We had a student working with us this year in our Summer of Code that has implemented support for Web Hooks in Review Board. This will be part of our 1.1 release, which we hope to be out in a number of months. The idea is that people can write hooks for doing all sorts of things when a review request is published, such as spinning off a sandbox build or updating a bug tracker. We will most likely ship a sample hook for updating Bugzilla with the review request URL whenever a change is put up for review. Each bug referenced will be updated, meaning it’s easy to get to the appropriate Review Board change from Bugzilla.
Today, it’s already easy to get from Review Board to Bugzilla. As long as you have your bug tracker specified in Review Board, the bug numbers on the review request will link to the correct bugs.
Multiple accounts are an issue that can be solved today. Review Board supports several authentication backends. There’s built-in auth, LDAP, NIS, ActiveDirectory, and soon OpenID. If these aren’t sufficient, then it’s pretty easy to write a custom backend and configure Review Board to use it. You wouldn’t even have to modify the Review Board codebase. It could just be a custom Python package that you drop on the server. This support has been tested pretty extensively. Lots of companies make use of our backends in many different configurations.
Some of these things you mentioned don’t exist today but are on the roadmap for upcoming releases. post-review may not be a perfect, but it’s improving and is scheduled to be split apart into a Python API and a wrapper that uses it (post-review). The idea is that extra tools will be available that can interact with a Review Board server. One of the first ones we’re going to provide is one that takes the patch from Review Board and applies it to the local tree, optionally (such as in the case of Git) preserving the submitter, description, etc. for the commit. It should be easy for people to write more custom tools once this work is done. I believe it will start as soon as Review Board 1.1 goes into beta. That’s not far off.
Most of Review Board can actually be scripted remotely. There’s a fairly extensive API that can be used for updating Review Board and pulling information from it. Such things as creating repositories can be done with HTTP POSTs to administrator-only URLs. Drafting a script for automatically creating repositories based on a large list shouldn’t be hard at all. The API can also be used for updating Review Board posts based on, say, changes in Bugzilla.
After 1.1, we’re going to continue with work we’ve been doing on an extension API. The current code, while incomplete, is very cool. Basically, anybody will be able to write custom extensions (just Python packages that implement some classes and register into the reviewboard.extension setuptools entry point) that can do, well, practically anything. They have full access to the rest of the Review Board API. They can create new URLs, replace existing ones, register new API calls, interact with the database, augment existing pages with new functionality, etc. This is just what they can do *today*. When we’re done with it, we should have a very powerful extension mechanism for shaping Review Board into whatever an organization wants.
We’ve spent a great deal of time working with organizations on their needs for Review Board. If you’re not completely set against Review Board for this work, we’d certainly be up for discussing any missing pieces and figuring out solutions.
Thanks,
Christian
Thanks for the detailed reply, Christian.
I should emphasize that Splinter isn’t an official GNOME Sysadmin Team project, or even a sysadmin project at all. It’s something I’m hacking on personally because it’s fun and because I needed it. (I’m already using a private instance to do my gnome-shell code reviews.) If I continue liking it, if other people want it, I’ll push for getting it installed onto bugzilla.gnome.org, but it’s not at all exclusive with someone working on a ReviewBoard instance for gnome.org.
It sounds like there is some good development going on in the area of extensibility that should allow for nice Review Board Bugzilla integration. I’d also imagine that some of the areas where ReviewBoard and post-review seem to me like not a perfect fit for Git will be smoothed out as Git is used for Review Board development itself. The attractiveness of ReviewBoard over time should only increase.
Single-sign-on: yes, both Review Board and Bugzilla support LDAP and the GNOME accounts database (what Mango frontends) is in LDAP. But a) it’s not self-service b) it’s not open for sign-up without applying for some sort of GNOME account. So there’s significant infrastructure development work there.
Multiple IDs: My concern really wasn’t that we’d always have two identifiers for the *same* thing, but that we’d have multiple types of identifiers that would sometimes overlap and sometimes not and it would be unclear what to use where.
I’m really interested to see what people have to say about this. I’ve been using Review Board every day for a few months now (via git-svn, not straight git), and it’s way better than traditional patch review via bugzilla.
http://reviews.mono-a11y.org
However, git-bz is also a great tool and I use it all the time for my GNOME work.
Ideally, I would have the best of both worlds. Fortunately, Review Board has recently moved itself from SVN to git, and the developers have stated a strong interest in improving the git workflow. So I think it’s not overly optimistic to think that with so many people eager to move in the same general direction, we can end up with something that gives most people what they want.
A git-pr that somehow merged the UI of git-bz with the features of post-review would be quite sweet! 🙂
For the Chromium (Google Chrome) project we use the open-source Rietveld (http://code.google.com/p/rietveld/) system for reviewing patches.
We have integrated Git support directly into Rietveld and our tool git-cl (http://neugierig.org/software/git/?r=git-cl) does feature many of the pieces you seek (such as patching changes in off the review site, and including review URLs in commit messages).
In general, I think convincing people to use systems they didn’t build themselves is hard; WebKit, an important upstream project of ours has their own suite of tools for meshing bugzilla and git (including a send-to-bugzilla tools as well as tools to go to the otherway, automating patches) and, despite our attempts to bridge the gap, have been building their own bugzilla-integrated system for code reviews.
I don’t know if I can apply a value judgement to all this in the presence of Review Board beyond “it’s sure a bummer so much work is being done over and over again”. In some situations, having a variety of tools in a space leads to healthy competition and crosspolination; in others, the sort of disconnected island feeling I get whenever I look at launchpad.
I’ve thought a lot about git and code review over the years and I’ve written up some thoughts on why I think patch series are the wrong unit of a review here: http://neugierig.org/software/git/?url=git-cl/tree/README.codereview . I’d love to hear your thoughts on it.
Ah, yeah, I had forgotten about Rietveld (and Gerrit, for that matter.) More things I can mine for UI ideas, anyways 🙂
One’s view of the relationship of Git to code review probably depends on exactly how you use Git. And there are as many ways to use Git as there are people using Git. I’m personally very much in the camp of “clean history”. I want my history to have logically separated commits with nice commit messages that look like they were written perfectly at the first go. Even when they weren’t. I don’t want a lot of “fix typo in a3f5sd” type commits. So, if I’m doing the work with ‘git commit –amend’ ‘git rebase -i’ and so forth to produce nice history, I might as well do that work up front *before* I ask someone to review my work. The commit messages and the logical separation will be a great help in reviewing the changes. Squashing everything on the feature branch together and presenting that for review is not as good.
If, as the result of review, I need to change one of the patches that is not at the top of the “stack” of commits, that can, of course, be a little challenging. My general approach would be to write an add-on patch and commit it at top, then use ‘git rebase -i’ to move it down the stack and squash it with the original commit. This usually works great, and any conflicts are easy to resolve during the rebase process. If one of the older patches has to be substantially rewritten, then other techniques like creating new a new branch and cherry-picking onto it may be more useful.
Once I have the new revisions of my patches I can attach them individually or en masse to the bug with git-bz and obsolete the old revisions. The default idea of patch identity with git-bz is simple – if two patches have the same Subject, they are probably revisions of the same commit rather than independent commit, but you can easily override that and mark other patches as obsolete instead.
What isn’t always so easy to deal with here is viewing the incremental changes – if someone submits A->B->C then obsoletes those three patches with four patches A’->B’->d->C’ – the squashed approach is certainly conceptually simpler. But that doesn’t that mean that the chunking isn’t still useful; I want to be able to look at B’ with the parts that changed highlighted and say “OK, that’s good now” and mark that patch as ready to commit, even if A is still under discussion. I haven’t started on interdiff support at all in Splinter and I’m not sure how well I’ll be able to do. As well as being defined as simple and llghtweight, it’s all written in Javascript on the client side, and while Javascript implementations of sha1, diff, etc, do exist, the performance probably won’t be managable if it the algorithms get too complex…
Sure, since ‘s’ isn’t a hex character. *grin*
“While I imagine it’s exciting to see two developers discussing obscure details of a patch to fix your reported crash, I also can imagine that it can be confusing and intimidating, and maybe people would prefer to just get a high-level summary when a patch was comimtted rather than all the details in their mailbox.”
Please don’t take the obscure conversations away. While they may be a bit intimidating they also hold a weird attractive power. They need to be Googleable.
Trawling Bugzilla logs is one of my main places to pick up those gold nuggest that are not documented elsewhere. Speaking for myself at least 🙂