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

[Manik Surtani]

On 25 May 2012, at 13:34, Sanne Grinovero wrote:

> On 25 May 2012 13:16, Manik Surtani <manik at jboss.org> wrote:
>> 
>> 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.
> 
> Ok we can work on that too of course. but I also think we should relax
> the pull request rule a bit. I pushed two obviously easy patches today
> directly as I was sure of them and they would fix broken tests.
> Of course, I take responsibility of that, but ultimately I feel doing
> it to not waste time of others on such a trivial and obvious patch.
> 
> Also, I still value pull requests in those areas in which discussion
> is needed, or complex things are changes; but in those cases the
> complexity (i.e. responsibility) should really not be pushed to the
> reviewer: it's the author who should labour his commits in a clear
> sequence with enough comments and tests to make everyone comfortable.
> 
> Another thing which occasionally happens is that a messy pull request
> is fine, but very complex to review. I'm pretty sure it happened to
> everybody at least once to have the feeling they are taking more time
> to review something than it took the original author to write it;
> that's also a smelly sign of quality and the process overall.
> People should make sure their pull requests are straight forward to
> understand, not only by the reviewer but by anyone looking at the
> history, including himself after a year.

Yes, and the reviewer should reject such pull requests.  It isn't just code quality that the reviewer is looking for, but style as well.

> 
> 
>>> 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.
> 
> The problem with such a game is that we lack reviewers, or are often
> slow at it. That's one of my reasons to avoid pull requests,
> especially when related to "fix the current build": it takes too long.

Sure, for such small, targeted patches as per your example, that makes sense.  But maybe a quick discussion with at least one other reviewer on IRC about what's going to happen makes sense.

> 
>> 
>> 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.  :)
> 
> Such a game would discourage people from doing reviews, which is the
> opposite of what we need.

No, reviews are necessary.  The project doesn't progress without it, full stop.

> 
>> 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
> 
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev

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

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