Last time I played with Review Board and bugzilla request queues things were great for me, but no one else.  I had to create an account for you on the instance, add you to my script that synchronizes request queues, run the script, and then keep running the script periodically.  Not to mention there wasn’t really a way to get your review out and into bugzilla.  No one actually tried to use it, so they probably also didn’t notice there were caching issues related to the ever-changing definition of “HEAD” (“tip”).  It sucked, and when I upgraded and everything broke, no one cared, not even me.

But now it’s back and better than ever:

• People don’t need to login at all to see review requests and reviews!  Just point them at the URL and away they go.  (Actually, this was the case before, but it was not obvious.)
• You can/must sign in with Open ID!  If you have Weave and are reading this in the future, Weave can be your Open ID friend!  If you are like me and live in the present (Weave 0.3.2), something is wrong and it doesn’t work, not to mention that Weave takes over the Open ID box so you can’t use credentials that work.
• There’s a button that updates your request queue for the 16 most recent requests made of you.  Just make sure that you have entered your bugzilla e-mail address on the “my account” page.  This may have happened automatically depending on what your Open ID provider provided/you told them to provide.
• There’s export functionality so you can take your review and cram it in bugzilla.
• The definition of “tip” gets nailed down when the review request gets created, so no more ugly caching issues.  Patches can still fail to apply, though, if “tip” has drifted from when the patch was first created.
• It has friendlier assumptions about what repo you are dealing with.  Thunderbird/MailNews Core/Calendar/SeaMonkey are all assumed to be in comm-central, everyone else is assumed to be in mozilla-central.  Patches against other repos (including mozilla 1.9.1) clearly will not work without additional logic (and some kind of extra info, like people putting “1.9.1” in their attachment descriptions.)

Here is a (fake) example of the “pretty” review output that is possible if you tell people about your reviewboard review (see it live here).  Although it says Bienvenu, it’s just me pretending to be him because his review queue is more interesting than mine.  The comments are accordingly mine.

Now, what does the export look like (see it live here)?

on file: mailnews/base/src/nsMessenger.cpp line 635
>     // if we have a mailbox:// url that points to an .eml file, we have to read
>     // the file size as well

what a pretty comment

on file: mailnews/base/src/nsMessenger.cpp line 642
>     NS_ENSURE_SUCCESS(rv, rv);

please rename rv ARRRRRR-VEEEEEE

Yeah, it looks like that.

A quick feature summary that explains why this is better than just looking at diffs in bugzilla:

• Syntax highlighting!
• It actually has the full-context of the rest of the file!  No more being limited to the 3 or 8 lines of context in the diff you are provided.  I know I have done a lazy review and let a bug through that would not have happened if I had more context at my fingertips (or was not sometimes lazy).
• People just trimming down your patch to what they are commenting on leaves you with no context of what changed at all!

Useful links:

• A more interesting live diff to check out.
• The root of the review board that will prompt you to login via an Open ID account.  When syncing your review queue, please keep in mind that it can take a bit to do all the legwork and you won’t see any feedback until it is actually done doing everything else.  You should get some form of feedback no matter what happens, so don’t keep hitting refresh.
• The hg repo for my modified version of review-board.  It’s based on an extremely shallow hgsvn checkout.  My questionable development strategy was to make changes with emacs locally, commit, then push to my VM, so the changesets are sometimes a bit excessive.

Caveat usor:

• This is running on a linode VM right now.  This is better than my local box or what not, but it’s not Mo[MC]o IT or anything.
• My changes, at least the export functionality, may be buggy.  You may need to rely on me to fix the export functionality to get your stuff out that way.  (If the hg diff doesn’t apply cleanly, you can’t enter data to lose it, so I’m less concerned about that.)
• I have no plans to blow away the database, but at the same time, please be prepared for the possibility that space ninjas destroy your data.  Use the export functionality and save it to a text file or something periodically if you’re doing a major review.  (In case it’s not obvious, the export functionality is the text labeled “bugzilla-style export” to the right of the reviewer’s name at the top-left of each review.)  You can do your review in multiple passes, exporting each review pass individually.
• I am confident something will go wrong.  Feel free to post comments here or ping me on IRC (:asuth).
• If people actually try using this, I’ll stop developing on the live server, but do be aware that apache restarts (lasting a few seconds) may periodically happen, but this should not really impact anything.

Props:

• Review Board

Review board is a delightful django-based review tool.  Whereas bugzilla’s diff display is limited to taking the provided diff and coloring hunks without additional context, review board actually retrieves the underlying files from mercurial, applies the patch, colorizes the source files (using pygments), and thus can give you additional context.  Also, it allows you to click on line(s) and attach a note at that location.

Anywho, I made a new tool that ‘synchronizes’ bugzilla request queues with review board.  It finds the things in your request queue, and then creates a review in review board, transferring the patch.  There is some limited hard-coded biasing towards comm-central, but it’s arguably easily adaptable.  It uses a slightly hacked up version of the post-review script found in review board’s contrib directory to serve as the API to review board.

The intent is to make it easier for people, or at least me, to do reviews.  I am definitely NOT proposing that the review “of record” live outside bugzilla.  But I think it might make it easier to prepare reviews in the first place.  For example, see the picture below?  It’s the result of my clicking on lines of code and making notes.  And those hyperlink-looking things that look like they might take you to the actual context, rather than manually having to try and determine context from people’s comments in the bugzilla notes?  They are actual hyperlinks that do what I just said!

If you are a mailnews hacker and would like to see your review queue in my reviewboard instance, ping me on IRC.  I may have already processed your queue and made you an account, or maybe not.  The review board API does not yet automate user creation and I got tired of doing things manually and gave up.

Warnings:

• Things go awry on old-school patches.  For example, a patch that thinks it is against (the CVS layout of) “mozilla/mailnews” appears to not work.  The right answer is a patch against “a/mailnews” (I’m not sure if the git-style is essential).  Things worked fine for my queue, but some people with old stuff in their queues experienced glitches.  The script now inserts “problem!” into the summary when this happens, but in my reviewboard instance there may be some reviews that pre-date this logic.  (There should be a note in the description though.  Also, there will obviously be no diff attached.)
• I initially forgot about the whole review=one person, super-review=another person, and maybe review=yet another person.  Although this is now fixed and reviews target all requested revieweres, there may be some reviews missing from your queue because they were created for one of the dudes who was not you and my manual nuking didn’t get to those.
• Uh, it creates the reviews, but it doesn’t destroy them.
• Also uh, there will be scaling issues if the reviews don’t eventually get closed out.