[infinispan-dev] Let's stop with pull requests

[Manik Surtani]

On 25 May 2012, at 14:44, Mircea Markus wrote:

>> 
>> I'll take over that role.  One thing I'd like to propose:
>> 
>> 1.  The reviewer is ultimately responsible for the patch.  If a broken patch makes it to upstream it is the reviewer's fault.
> 
> I think we should differentiate from  community patches (i.e. external contributors) and internal patches (core team).
> Your suggestion is a must for community patches, but I think for internal patches it should be the patch provider that has the responsibility of running the test and the review process should only be there for a second opinion. The tests should rather be run by the person that *can fix them* - and that's the patch provider - otherwise we introduce an unnecessary loop in communication. Also review takes much longer, which puts people off when it comes to doing it.

-1.  The reviewer is ultimately the person that applies a change to master.  It is his responsibility to make sure bad patches don't go in.  Regardless of whether the patch author is a core developer or a contributor.

And any patch author who wastes reviewers' time by submitting broken pull requests should also be penalised by the reviewer.  This step probably only applies to core developers and not external contributors.  :)  Or more likely, each patch author is allowed a fixed number of "mistakes" which are not penalised.  After this, broken requests should be penalised.  Then this rule can apply to all patch authors.  Of course, it is the reviewer's discretion to penalise or not - since in some cases as Martin pointed out another concurrent patch could break a different patch, and the patch author really isn't to blame in this case.

Essentially, the reviewer's job is to keep the code base pristine.

HTH
Manik
--
Manik Surtani
manik at jboss.org
twitter.com/maniksurtani

Lead, Infinispan
http://www.infinispan.org