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

Fri May 25 09:34:31 EDT 2012

On 25 May 2012 13:44, Manik Surtani <manik at jboss.org> wrote:
>
> 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.

I don't disagree on having proper reviews, actually the opposite: my
topic was meant to be provocative and I'm very glad of your reaction.
But I don' t understand your "no" in this context after my comment
which is specifically commenting on the game proposal: what I'm saying
is that if there is a chance to be penalized when doing a review,
people will not feel inclined to jump on the task eagerly, which is
what we often need for more expedited merges.

Looking at it in terms of game theory, It might work fine if the
prospect of benefiting out of [doing a good review] outweighs the
penalty of doing it wrong.

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