[libvirt] patch review tool for libvirt patches?
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)?
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 :|
On Thu, Apr 07, 2011 at 07:21:26PM +0100, Daniel P. Berrange wrote:
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'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 :-/),
Definitely, but I'm getting old maybe that's the reason :-)
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).
Why not ...
Here's my list of requirements; feel free to add/shoot down:
My own requirement is that it should not be intrusive, optional, and actually enhance productivity, not decrease it :-) [...]
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.
agreed
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.
Very much agree. One of the key point of Open Source devel is that the design discussions, pros, cons and associated voices including dissenting ones are in the open and logged forever. Basically what would help me is something tracking git and the list and telling me "that has been applied", "that's an old version", "this need review" , "this was ACK'ed but never commited". I'm not sure patchwork really processes automatically, if I look at the qemu-devel example [1] I see only stuff in NEW, if you need manual intervention to set the status, it creates more work, so I hope there is something better. Whether it has a web UI or not is not necessarily important, a tracker which would post status to the list would be IMHO just fine (and possibly slight more efficient since I would be in the same context, and not having to go out of my mail to reach a browser). Daniel [1] http://patchwork.ozlabs.org/project/qemu-devel/list/?order=state&page=5 -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 04/07/2011 02:21 PM, Daniel P. Berrange wrote:
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.
[...]
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).
[...]
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
[...]
Patchwork (http://ozlabs.org/~jk/projects/patchwork/) (used by Linux kernel& kvm maintainers)
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.
Agreed. That's why I made "optional" the first requirement. I wouldn't want anything that screwed up what's already working for somebody else, or made extra work for people who weren't interested, and also agree that having the mailing list archive available is very important (although I wish it was simpler to grab a message from the archive and "git am" it, or create a properly threaded reply to a message (for those times when I've already deleted a message from my mail client, then decide later that I want to do something with it)
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
Well, that narrows down the field rather quickly :-)
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."
That seems a bit narrow-sighted of them. There's nothing in "all review must be on the mailing list" that would prevent a separate application from presenting the diff in some sort of UI (maybe even grabbing in the entire file so that things could be seen in context), allowing comments to be made, then generating email to the list that incorporates the comments. But that's a discussion for patchwork developers, not us...
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.
Plaintext definitely has the most likelyhood of enduring, and of being usable by other applications. So since the range of selection has been narrowed to 1 (well two actually - use patchwork, or don't use patchwork). Does anybody have experience setting it up and using it? Would it be simple for me to set it up locally and try it out.
On 04/07/2011 09:24 AM, Laine Stump wrote:
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 :-/),
No, you're not alone - I feel that my inbox is growing faster than I can respond, and I have in the past committed the wrong version of a patch when I missed that a followup was posted later.
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
Agreed - the tool must be able to interact well with the mailing list, with the list as the primary archive of all patch action. I don't know if periodic summary mails of all pending patch series helps or just adds to the list traffic, but I've seen it work for other lists to have the periodic reminders of all pending patch series work out well as a way to see what still needs reviewing. Unfortunately, I don't have enough experience with any of the tools that you listed to recommend (or reject) any of them, but I'm willing to give a new tool a shot if it can help my productivity. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Laine Stump