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

[Manik Surtani]

On 25 May 2012, at 12:47, Sanne Grinovero wrote:

> guys, please don't take me as the one who is again complaining about
> failing tests; I'm having doubts about the development process and the
> amount of time this is wasting on all of us.
> 
> We're all humans and do mistakes, still it happens so extremely often
> that this is getting systemic, and discipline could definitely be
> improved: people regularly send pull requests with failing tests or
> broken code, and very regularly this is just merged in master.

Yes, this must not happen.  I have been culprit of this a few times, but we really shouldn't do this.  Galder integrating BuildHive into the process should help to some degree.

> 
> I did it myself a couple of days ago: didn't notice a failure, all
> looked good, sent a pull, it was merged with no complaints. Three days
> later, I resume my work and am appalled to see that it was broken. Now
> fixing it, but I'll have to send another pull and wait for it - which
> feels very pointless, as I'm pretty sure nobody is checking anyway.
> 
> It looks like as the pull request procedure is having this effect:
> 
> # patch writer is not as carefull as he used to be: "someone else
> will check if it's fine or not. I have no time to run the tests
> again..".

Yes, the patch author should still be just as vigilant.  Having a review process is no excuse for not writing good code and testing it properly.

> 
> # reviewer has as quick look. "Looks good - in fact I don't care
> much, it's not my code and need to return to my own issues.. worst
> case someone else will fix it blaming the original author"
> 
> And then again some incomplete test makes it to master, or a patch
> which doesn't even compile is integrated.
> 
> This pull request process is being a big failure. Shall we stop
> wasting time on it and just push on master?

-1.  We should fix the process.

> 
> Which doesn't mean I'm suggesting "let's make it worse" | "unleash
> hell": we should all take responsibility on any change very seriously.
> 
> Again, I'm not enjoying the role of "whom who complains on the
> testsuite again". Just stating a fact, and trying to propose something
> to make it work better. We have great individuals on this team, but we
> need to admit that team work isn't working and we should deal with it
> at it's best; denying it won't help.

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.
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 at jboss.org
twitter.com/maniksurtani

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