Skip to content

Review Board and Bugzilla reviews, take 2.

review-board-diff

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:

review-board-bugzilla-queue-sync

  • 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.

review-board-review-pretty

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:

{ 2 } Comments

  1. Mossop | June 26, 2009 at 2:03 am | Permalink

    Looks nice, shame it doesn’t seem to handle Mercurial patches

  2. Andrew Sutherland | June 26, 2009 at 2:13 am | Permalink

    It should definitely handle git-style mercurial patches (git=true under the [diff] section). That’s what I’ve tested on, and I actually had to hack that support in. In theory the default code path (that existed before the hack and should still exist) should still work and handle mercurial diffs.

    Can you point me at a specific bug or patch so that I can check it out?

    I should note that git-style diffs with mq are turning out to be insufficient for the needs of Thunderbird development currently. I had completely forgotten that I had set a setting to get git-style diffs; maybe without that the mq patches have more information in them, but I fear that might have other limitations. In any event, I’m considering moving to using pbranch instead of mq, and that might improve the review situation too, as you’d have specific revisions under review and an easy coherent check-out point.

{ 1 } Trackback

  1. […] updated my review board setup once more (part 2, part 1).  The low barrier to entry is now even lower.  “How low?”, you might ask.  […]