[Development] Speeding up the review process

[Frederik Gladhorn]

Hi all,

I think this thread is valuable, let's all spin it into something positive 
indeed. Let's discuss ways to make reviews nicer and faster indeed.
Sorry for the top posting, I have random thoughts that I'll simply write down 
not directly responding to any of the points.

Please just forget about point 4 of the original email and discuss the rest. 
As long as I have any influence in how we handle Qt and contributions, we'll 
follow open governance. I'm very sure that Lars and many others will uphold 
the standards we have as well. So to calm everyone down: there will be no 
preferred treatment of patches depending on employer. We've been there with 
Gitorious, I've seen the pain first on the outside, then inside Nokia, let's 
not repeat that, it was pure horror on both ends, open governance has made 
everyone's work much easier.

There were some interesting comments in the thread though about personal bias 
and meritocracy. We often know each other. I have colleagues that I know will 
mostly produce patches that are good and will be around to deal with flaws in 
them, they do get my personal bias towards an easier +2. So do maintainers of 
a module and other people that earned a standing with me and that I expect to 
be around. Patches from people that I've never heard of before get more 
scrutiny. I think we all (more or less consciously) act that way and that's 
fine. There are people in the community that I know better than in The Qt 
Company, they also get a bias from me.
So if Viktor works intensely with people in the same office, it should be easy 
to get approvals by them. In fact that's the way I see people progressing on 
their work in Qt quickest: they team up for an area and push each others' 
patches forward with mutual review. If you want to make fast progress, get to 
know others in the Qt community and team up with them, help each other and 
review each others' patches, knowing where to look more careful and when to be 
relaxed.

I've been pondering this for a while and it's not fair. And that's actually 
fine. Bias and merit is OK. We're not a democracy.

That does not mean we should ignore anyone or be disrespectful. I expect 
everyone independent of employer to be positive and friendly in reviews.
Reviews are sadly negative by nature, but please assume good intentions and 
try to phrase thing positively, ask questions and make the process work for 
everyone.

I have personally changed to sometimes giving a +2 and still criticizing minor 
things (a typo in the commit message or so). Usually people ended up fixing 
those even though they could have clicked the stage button, so I think we can 
sometimes be more relaxed when it comes to minor details.
I've become convinced that perfection is the enemy of the good - if a patch 
makes things only better, but it could be even better, then consider writing 
the follow up patch yourself. Of course it's still OK to point out optional 
improvements, but they don't need to all happen in the same patch.

Collaboration:
I wish gerrit made it easier. I've experimented with handing patches over to 
other people, I would encourage that as well. If you know you won't really 
finish the thing, but someone is interested in it, instead of letting them 
review it, if they care more than yourself, make it clear that the patch is up 
for grabs, to be adopted, improved upon. That's not super easy in gerrit from 
a social perspective, there seems to be clear ownership and randomly pushing 
over other's patches is not a sensible thing to do of course, so I think the 
authors need to make it clear to others that they're fine with changes to their 
patches.

Cheers,
Frederik

On fredag 13. oktober 2017 13.04.46 CEST Viktor Engelmann wrote:
> On the [Interest] mailing list there was a discussion about the
> review-process taking to long and we also had multiple discussions about
> that at the world summit. I have complained about this myself, so I
> would like to start a new thread and collect your thoughts and ideas on
> how to improve the situation.
> 
> My suggestions would be
> 
>  1. Allow approving your own commits under certain circumstances. examples:
>      1. when you only changed minor things like a variable name (except
>         in public API), a string, a comment or qdoc entry
>      2. when you already had a +2 and only changed minor things like a
>         variable name, a string, a comment or qdoc entry (or more
>         generally: when you already had a +2 and only did something that
>         is also on this list :-D )
>      3. when you only increased a timeout in an autotest. Increasing
>         timeouts is a safe change - it can only solve false negatives
>         and false positives, but not create false positives or false
>         negatives.
>      4. or more general: when you only made an autotest harder to pass -
>         like adding a Q_VERIFY, Q_COMPARE, etc.
>      5. when the change is something auto-generated - like you just
>         updated plugins.qmltypes using qmlplugindump
>      6. when you only changed something in accordance to the guidelines
>         - like Q_DECL_OVERRIDE -> override
>      7. when you have a certain count of +1's from people who have
>         approver rights
>  2. Towards that last point: I think many of us are afraid to get blamed
>     for +2'ing something that causes problems later (introduces a new
>     bug or so), but as far as I have seen, nobody gets blamed for such
>     problems, so we should not be THAT afraid of approving something.
>     Also, don't forget that there is still the CI to get past!
>  3. Remember that brain-cycles are far more expensive than CPU cycles -
>     so when in doubt, rather test-run something on the CI than make a
>     human think about whether the CI "would" accept it. If that causes
>     CI outages, we need to buy more CI machines. It is just a naive
>     fallacy to "save" CI expenses by assigning the CI's work to
>     employees who are much more expensive.
>  4. I don't think we need to be as paranoid towards contributions from
>     our own employees as we need to be towards external contributions.
>  5. Set a deadline for criticism on the general approach to a change.
>     Too often I have had the situation that I uploaded a patch, then we
>     debated the qdoc entries, variable names, method names, etc FOR
>     MONTHS - and when I thought we were finally wrapping up and I could
>     soon submit it, someone else chimes in and says "this should be done
>     completely differently". Even if the person is right - they should
>     have said that months earlier - before I wasted all that valuable
>     time on variable names, compliance with qdoc guidelines, etc.
>     In earlier discussions I have been told that such a deadline would
>     have to be long, because someone who might have an opinion might be
>     on vacation. IMHO, this doesn't count, because a) you can still make
>     an exception to the rule in such a situation and b) by that logic
>     you should never approve anything, because we also might hire a new
>     employee in 10 years who might have an opinion.