Patch review for OpenLMI

Stephen Gallagher sgallagh at redhat.com
Tue Mar 12 11:41:46 UTC 2013


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/12/2013 05:47 AM, Radek Novacek wrote:
> On Mon 11 of Mar 2013 15:35:51 Jan Safranek wrote:
>> Hi,
>> 
>> we've talked on Friday about starting code review for all
>> OpenLMI patches. Fedora 19 alpha freeze is here and it could be
>> good milestone to start with reviews (or at least as good as any
>> other milestone).
>> 
>> I would prefer as light review process as possible, we're still
>> in initial development where we code pretty quickly and I
>> personally don't want to get slowed down by reviewing each patch
>> character-by-character.
>> 
>> @sgallahg, you mentioned some review tool, what was it and 
>> where/when/how can we use it?
>> 
>> Jan
> 
> I personally prefer some web interface for reviewing patches.
> Advantages for using some tool from my point of view (might not be
> applicable for all review tools): * one click for apply the patch

I don't know of any review tools that meet this requirement. In the
case of git, some tools can allow you to register a (virtual) remote
repo branch that includes the applied patch, but if you want to merge
it to your own in-progress branches, that's going to be a manual step
no matter what.

In the case of Review Board, one of it's current major bugs is that
the downloaded patch comes back to you as a pure diff (without the
extended git header), which means that it's trickier to apply locally
than we would like. In this case, there's an argument to be made for
Gerrit, which creates the virtual branches like I mentioned above.

> * inline comments (discussion is easier to read than in the mail
> thread, especially with more parallel discussions)

Review Board is simply the best at this. Gerrit is pretty poor at it
(capable, but not as clear and easy to follow). Github only allows
general comments on a pull request; they do not allow inline comments,
last I checked.

> * showing more context of the patch hunk is easier (just click vs
> applying to local checkout)

Review Board, Gerrit and Github can do this easily.

> * possibility to send notification about review to mailing list
> too
> 

All three have this capability, with different degrees of information
about what was updated.

> On the other hand, advantages for using mails: * easy to submit for
> review via git-send-email

Gerrit wins here, as you can do a 'git push' to the Gerrit server and
it can be configured to also send the 'git-send-email' messages to a
mailing list.

> * acks are visible in mailing list (can review tools be configured
> to do it?)
> 

All three review tools discussed above can do this (in different ways).

Review Board: There is a "Ship It!" flag that can be set on the
review, which is functionally an Ack.

Gerrit: Reviews are configured to require a certain number of acks
(possibly from specific groups of users). When this is met, the
gatekeepers get a "merge" button enabled. All reviews are sent to the
list, including whether it was reviewed as "Fully acked", "Acked but I
want someone else to look too" or "Nacked".

Github: All reviews are comments on the pull request and all reviews
are mailed out. So just adding a comment "Ack" meets this requirement.


> Feel free to add more pros and cons for both approaches.
> 
> Radek Novacek _______________________________________________ 
> openlmi-devel mailing list openlmi-devel at lists.fedorahosted.org 
> https://lists.fedorahosted.org/mailman/listinfo/openlmi-devel
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlE/FHoACgkQeiVVYja6o6Mp0QCfWp9tZluRXTI5kArrtAEaGrJv
f0cAoId8DwwmpkIg56M6suPHJNO3N8LF
=cyxp
-----END PGP SIGNATURE-----


More information about the openlmi-devel mailing list