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(a)jboss.org
twitter.com/maniksurtani
Lead, Infinispan
http://www.infinispan.org