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 ma...@jboss.org twitter.com/maniksurtani Lead, Infinispan http://www.infinispan.org _______________________________________________ infinispan-dev mailing list infinispan-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev
