[libvirt] patch review tool for libvirt patches?

[Daniel P. Berrange]

On Thu, Apr 07, 2011 at 11:24:04AM -0400, Laine Stump wrote:
> Now that 0.9.0 is out, I'd like to ask everyone's opinions about
> patch review tools.
> 
> I've been noticing lately that the volume of libvirt patches has
> increased significantly, and it's getting more and more difficult
> for me to keep up with reading the traffic, much less doing my part
> to review the patches. It would be very helpful if I could just see
> a view of pending patches, each properly colorcoded (or, even
> better, hidden) based on a patch being ACKed, deprecated, someone
> else in process of reviewing, etc. I know there are a few tools
> available, but I've never used any of them and wonder if anyone else
> has, and if they might be able to make a recommendation on something
> the libvirt project could use (or maybe something I could just use
> myself by sending a list feed into an application).
> 
> I'm sending this message 1) to see if others are feeling the
> pressure of the extra traffic too (or is my brain just processing
> more slowly :-/), and 2) to learn what your opinions are of setting
> up such a system for libvirt (for *optional* use only), and any
> opinions you have on what's available (or maybe you could provide a
> recipe for how you already manage all the patches without going
> crazy).
> 
> Here's my list of requirements; feel free to add/shoot down:
> 
> 1) usage must be optional, so it must be able to update itself via
> monitoring messages on the list (a minimal amount of change/addition
> to the current message flow might be acceptable, but nothing major,
> and encountering PATCH/review/ACK messages as currently sent
> shouldn't make it blow up).
> 
> 2) keep track of patches that are posted, NACKed, ACKed, re-posted
> (deprecating the original), and pushed (this should be done by
> monitoring git, not by looking at emails, as I've noticed patches
> are often pushed without a corresponding message to the list).
> 
> 3) would also be nice if there was a place in the UI that those
> wanting to could "take" a patch for review so that two people didn't
> spend a lot of time on something not requiring that much attention,
> at the expense of other ignored patches.
> 
> 4) maintain patchsets according to the "n/n" notation in the header
> (I mention this because one comment I saw about Gerrit said that it
> didn't do this).
> 
> 5) provide some sort of interface for viewing the patch, annotating
> it with comments, and sending that back to the list as an ACK/NACK
> or simply a comment.
> 
> 6) provide a simple way to save a patchset to files that can be "git
> am"ed (or maybe even do it for you, automatically creating a branch
> first, etc. This could possibly even be extended into a command to
> push a given patchset)
> 
> 7) (of course it must be open source. Do I even need to say that? :-))
> 
> Ideally, a person wanting to use this system should be able to setup
> their email to filter all libvir-list traffic containing "PATCH" in
> the subject line, then create an account on the patch review system
> and handle all patch review via the tool's interface
> 
> Here's a list of tool that Rich Jones came up with in another
> discussion on the same topic, along with a few others that were
> mentioned in the ensuing discussion. To the right, I've included
> comments from others that seemed interesting to me. Please point out
> any that you disagree with!
> 
> Gerrit (https://code.google.com/p/gerrit) "The assumption that one issue == one patch is too deeply embedded"
> 
> Kiln (http://www.fogcreek.com/kiln/) (proprietary, so probably shouldn't be considered)
> 
> Patchwork (http://ozlabs.org/~jk/projects/patchwork/) (used by Linux kernel&  kvm maintainers)
> 
> 
> http://www.reviewboard.org/  "didn't work well with git"
> 
> 
> Also someone pointed out that gitorious has code review aids:
> 
> http://blog.gitorious.org/2009/11/06/awesome-code-review/
> 
> 
> If I were going to investigate one of these and try setting it up,
> which do you think would have the greatest likelyhood of success
> (if any)?

For me, any tool which requires visiting a web UI to submit or view
patch code review comments/feedback is a non-starter. All code review
feedback must be on the mailing list, and correctly threaded.
In other words, it would be a tool which serves a 'reporting' or
'tracking' patch series, not a code review management system.
AFAICT, patchwork is the only one expressly designed in this
manner. Their website sums it up nicely:

  "patchwork should supplement mailing lists, not replace them

   Patchwork isn't intended to replace a community mailing list;
   that's why you can't comment on a patch in patchwork. If this
   were the case, then there would be two forums of discussion
   on patches, which fragments the patch review process. Developers
   who don't use patchwork would get left out of the discussion."

To also add to that, public mailing lists are a very good archival
system for code review / discussions. Once in a mailing list, you
can be pretty sure it'll never disappear from the web & is always
searchable from google. The same can't be said of most web apps.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|