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

[Thiago Macieira]

On quinta-feira, 15 de agosto de 2013 18:32:09, Oswald Buddenhagen wrote:
> ironically, you failed to respond to exactly the part of the message
> that you quoted. yes, sometimes one forgets the sanity override. *what
> is the actual, observable harm?*

A full 24-hour cycle because the approver forgot to give the necessary +1 
sanity check. I didn't think I needed to reply because Alan had already said 
that.

You dismissed the argument. But that's still my argument and I don't dismiss 
it (in other words, I think you're wrong to dismiss it).

Approvers forget to give the +1 to sanity because a reviewer is not supposed 
to be looking at the sanity -- that's the contributor's job. That's why we 
have a bot in the first place. The reviewer will review the code and give a 
code check rating and will usually not look at the sanity. Therefore, it's 
very common to forget to give the +1 sanity.

You said that Git permits highly parallel work. Well, yes, but there's still a 
lot of serial work. In the worst case scenario, the missing sanity means a 
commit misses the merge-to-dev window and the contributor will need to wait a 
week for the next one so they can rebase some dev work.

> > I do not think there will be bad side-effects.
> > 
> > The option is not easily discoverable
> 
> correct - that was part of the plan.
> still, it sounds a lot like security through obscurity.

Yes, but it also sounds a lot like user experience. Since this is not about 
security at all, I'll take my UX argument.

> > and even if it is abused, it cannot override the normal review
> > process.
> 
> override, no. but people are easy to lead to wrong conclusions,
> and tend to trust previous comments without checking themselves. having
> to the check the box themselves is at least a minor wake-up call.

So you're saying that a non-approver who did a +1 will keep doing +1 because 
he had done +1 before? In other words, "a reviewer who has bad behaviour will 
have bad behaviour"?

Or are you saying that someone will override the bot because someone else has 
overridden the bot in a past occasion? If this is the one, I don't buy it. 
It's like saying "Thiago has approved contributions in the past, so I will 
approve too without thinking". People are supposed to use the grey matter 
between their ears, not follow blindly other people's behaviours.

And if there are people with bad behaviour (whose behaviour may be copied or 
not), we need to nip that in the bud. That's not an argument for the sanity 
check.

> > An approver will more easily notice an override that shouldn't be
> > there, as opposed to a missing override.
> 
> what makes you think so?

Because it generates an email when someone does that. The approver should be 
thinking "why are you overriding the bot?". Bot warning emails are common and 
most reviewers will dismiss them as "the contributor will take care of that". 
That's why two different emails get two different levels of attention.

> > Whenever I do notice a correct message from the bot, I refrain from
> > giving +2 and post "please take care of the bot warnings".
> 
> that's you. and me. we are among the most attentive reviewers out there.
> and still, we have failed before.
> 
> sure, the risk is low. but why change? we are talking about a corner
> case here, one in which learning is a key component. i wouldn't want to
> send the wrong message ...

Because the drawbacks of one far outweigh (IMO) the drawbacks of the other.

Allowing approvers only to override means inattentive non-approvers can't 
override the bot. That says nothing about the inattentive approvers, who 
submit 95% of the code contributions anyway. But it means a portion of those 
5% of the contributions may end up delayed.

Allowing non-approvers to override means we could have inattentive or 
inexperienced contributors bypassing the sanity rules. Since the contribution 
*still* needs to be approved by an approver, there's an opportunity to catch 
the mistake. If the mistake isn't caught, it's just that: a mistake. We're all 
human.

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.qt-project.org/pipermail/development/attachments/20130815/c3484655/attachment.sig>