[Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

Konstantin Tokarev annulen at yandex.ru
Fri Oct 13 13:17:07 CEST 2017



13.10.2017, 14:05, "Viktor Engelmann" <viktor.engelmann at qt.io>:
> 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
>
> * Allow approving your own commits under certain circumstances. examples:
>
> * when you only changed minor things like a variable name (except in public API), a string, a comment or qdoc entry
> * 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 )

FWIW, in WebKit project it's a usual workflow to set "r+" and at the same time request minor changes to the patch, fixed version of patch is submitted without new review.

> * 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.
> * or more general: when you only made an autotest harder to pass - like adding a Q_VERIFY, Q_COMPARE, etc.
> * when the change is something auto-generated - like you just updated plugins.qmltypes using qmlplugindump
> * when you only changed something in accordance to the guidelines - like Q_DECL_OVERRIDE -> override
> * when you have a certain count of +1's from people who have approver rights

Note that at least some of this rules can be implemented in gerrit itself, so you don't need self-approve

https://codereview.qt-project.org/Documentation/prolog-cookbook.html
https://codereview.qt-project.org/Documentation/prolog-change-facts.html

> * 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!
> * 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.
> * I don't think we need to be as paranoid towards contributions from our own employees as we need to be towards external contributions.

Hey, it's a discrimination!

> * 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.
>
> -- Viktor Engelmann Software Engineer The Qt Company GmbH Rudower Chaussee 13 D-12489 Berlin Viktor.Engelmann at qt.io +49 151 26784521 http://qt.io Geschäftsführer: Mika Pälsi, Juha Varelius, Mika Harjuaho Sitz der Gesellschaft: Berlin, Registergericht: Amtsgericht Charlottenburg, HRB 144331 B The Future is Written with Qt www.qtworldsummit.com
> ,
>
> _______________________________________________
> Development mailing list
> Development at qt-project.org
> http://lists.qt-project.org/mailman/listinfo/development


-- 
Regards,
Konstantin



More information about the Development mailing list