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.
2. The patch author is responsible for wasting reviewer's time
with bad patches.
I think we should make a game of this: we have a "bad commits" jar. Each time
a reviewer pushes a bad patch up, he needs to put $1 into the jar. The next time, $(1
<< 2), the next $(1 << 3), etc. :)
To prevent incomplete pull requests wasting reviewer's time, the reviewer is allowed
to impose a similar penalty on the patch author. Naturally, this only covers blatantly
bad patches - i.e., incomplete tests, broken builds - and not genuine feedback on the
patch, etc.
And of course the "bad commits jar" is used to buy beer the next time we all
meet. Inspired by a tweet Sanne fwd'd a while back. ;)
If we're happy with this, I'll maintain the accounting spreadsheet for this bad
commits jar.
- M
--
Manik Surtani
manik(a)jboss.org
twitter.com/maniksurtani
Lead, Infinispan
http://www.infinispan.org
_______________________________________________
infinispan-dev mailing list
infinispan-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev