Open Stack

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

On 17/06/14 12:36, Sean Dague wrote:
> On 06/17/2014 07:23 AM, Daniel P. Berrange wrote:
>> On Tue, Jun 17, 2014 at 11:04:17AM +0100, Matthew Booth wrote:
>>> We all know that review can be a bottleneck for Nova
>>> patches.Not only that, but a patch lingering in review, no
>>> matter how trivial, will eventually accrue rebases which sap
>>> gate resources, developer time, and will to live.
>>> 
>>> It occurs to me that there are a significant class of patches
>>> which simply don't require the attention of a core reviewer.
>>> Some examples:
>>> 
>>> * Indentation cleanup/comment fixes * Simple code motion * File
>>> permission changes * Trivial fixes which are obviously correct
>>> 
>>> The advantage of a core reviewer is that they have experience
>>> of the whole code base, and have proven their ability to make
>>> and judge core changes. However, some fixes don't require this
>>> level of attention, as they are self-contained and obvious to
>>> any reasonable programmer.
>>> 
>>> Without knowing anything of the architecture of gerrit, I
>>> propose something along the lines of a '+1 (trivial)' review
>>> flag. If a review gained some small number of these, I suggest
>>> 2 would be reasonable, it would be equivalent to a +2 from a
>>> core reviewer. The ability to set this flag would be a
>>> privilege. However, the bar to gaining this privilege would be
>>> low, and preferably automatically set, e.g. 5 accepted patches.
>>> It would be removed for abuse.
>>> 
>>> Is this practical? Would it help?
>> 
>> You are right that some types of fix are so straightforward that 
>> most reasonable programmers can validate them. At the same time 
>> though, this means that they also don't really consume
>> significant review time from core reviewers.  So having
>> non-cores' approve trivial fixes wouldn't really reduce the
>> burden on core devs.
>> 
>> The main positive impact would probably be a faster turn around 
>> time on getting the patches approved because it is easy for the 
>> trivial fixes to drown in the noise.
>> 
>> IME any non-trivial change to gerrit is just not going to happen 
>> in any reasonably useful timeframe though. Perhaps an
>> alternative strategy would be to focus on identifying which the
>> trivial fixes are. If there was an good way to get a list of all
>> pending trivial fixes, then it would make it straightforward for
>> cores to jump in and approve those simple patches as a priority,
>> to avoid them languishing too long.
>> 
>> If would be nice if gerrit had simple keyword tagging so any
>> reviewer can tag an existing commit as "trivial", but that
>> doesn't seem to exist as a concept yet.
>> 
>> So an alternative perhaps submit trivial stuff using a well
>> known topic eg
>> 
>> # git review --topic trivial
>> 
>> Then you can just query all changes in that topic to find easy
>> stuff to approve.
> 
> It could go in the commit message:
> 
> TrivialFix
> 
> Then could be queried with - 
> https://review.openstack.org/#/q/message:TrivialFix,n,z
> 
> If a reviewer felt it wasn't a trivial fix, they could just edit
> the commit message inline to drop it out.

+1. If possible I'd update the query to filter out anything with a -1.

Where do we document these things? I'd be happy to propose a docs update.

Matt
- -- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlOgML0ACgkQNEHqGdM8NJCmpgCfbSy9bljyEqksjrJ7oRjE8LNH
8nUAoJBE5L+uAcQbew5ff/98eeqoRvW2
=+SOM
-----END PGP SIGNATURE-----