[Development] Proposal: Allow contributors to +1 sanity review.

[André Pönitz]

On Tue, Aug 13, 2013 at 10:51:17AM -0700, Alan Alpert wrote:
> On Tue, Aug 13, 2013 at 10:45 AM, Thiago Macieira
> <thiago.macieira at intel.com> wrote:
> > On terça-feira, 13 de agosto de 2013 10:26:42, Alan Alpert wrote:
> >> > (or because they enjoy outsmarting the system just for the sake
> >> > of it, like you apparently do)
> >>
> >> We *define* the system. The fact that I have to work around it in
> >> order to do my job properly shows that the system is defective and
> >> needs to be rectified. Lars mentioned at the contributor summit that
> >> we need to make the review process easier for people, and fixing the
> >> system to have fewer unnecessary hurdles is an obvious way to do that.
> >>
> >> A more effective change which I recommend would be to have the bot not
> >> give -1 on the heuristics which are known to give false positives more
> >> frequently. It's enough that it provides a warning comment, allowing
> >> the contributors to address the issue if legitimate. But the -1 can
> >> also be viewed as a hint that there's relevant output, since it's so
> >> prominent in the gerrit interface, so the -1 isn't a problem so long
> >> as it doesn't slow us down.
> >
> > I don't see what's wrong with the current system. Let's keep it.
> >
> > The bot usually doesn't make a mistake.
> 
> I run into it fairly often.
> https://codereview.qt-project.org/#q,reviewer:aalpert%2540blackberry.com+status:merged,n,z
> shows five times this month that it needed to be overridden in my
> reviews.

I am sure we are not on the same page here. The first patch on the
list there shows that you approved new(!) code containing things like

   QQuickWindow* window = ...

and

   for (int i = 99; i < 102; ++i)
   {
       ...

whereas the Qt Coding Style (http://qt-project.org/wiki/Qt_Coding_Style)
clearly states 

   For pointers or references, always use a single space between the type
   and ‘*’ or ‘&’, but no space between the ‘*’ or ‘&’ and the variable name

and

   Use attached braces: The opening brace goes on the same line as the
   start of the statement.


Sure, coding style does not affect functionality, "only" maintenance,
but we do have a rough consensus in the project that at least new code
should follow the rules unless there's a _good_ reason why it shouldn't.

Given that the bot did not complain about the (unjustified) style
violations, but "only" about the missing copyright headers, this rather
calls for the bot becoming more strict, not less.

What you call a "defective" system is something that tries to establish a
minimum level of sanity in the code base, and to ease maintenance. 

I personally find it also very helpful to prevent stupid accidental
commits (it certainly caught a couple of mine), but of course, an advisory
system does not work well if people overrule it willfully.

Andre'