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

[André Pönitz]

On Thu, Aug 15, 2013 at 11:44:31AM -0700, Thiago Macieira wrote:
> 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.

It should be rather quick to get another approver's +1 on a _really_
_accidentally_ missing sanity-bot override by simply asking on #qt-labs.

I don't dispute that it might _theoretically_ happen that #qt-labs is not
manned by half a dozen approvers at 5 am on Christmas Day, but to incur a
real delay this has to coincide with a false positve from the bot _and_
with the original approver forgetting to override it at the same time.

> 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).

The argument is valid, but it has to be weighted with the likelihood that
the situation occurs. That likelihood is very small. I've yet to see
someone waiting for a (justified) sanity-bot override for 24 hours.

> 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.

"Sanity" covers a lot of aspects, lots of rules, written and unwritten. It
is not to be expected that a first-time contributor can take all of them
into account, it is even less expected that he can clearly recognize a 
wrong -1 from the bot as inappropriate.

> 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.

A -1 from the bot is not hard to miss in a review, and should trigger
double-checking. I understand one might forget to tick it off after an
hour-long review, but I really doubt this happens regularly.

> Therefore, it's very common to forget to give the +1 sanity.

How often did that happen to you personally, and how much was the delay
caused by that?

> 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.

> [Big snip]
> 
> 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.

There is way more work _that passed sanity review_ that misses merges like
that. This is optimizing for the wrong case.

> 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.

The contribution _includes_ sanity. If the approver does not approve
sanity, he does not approve the contribution.

Approvers can and do err, but that could as well happen in the "regular"
part of the review. If you go the full mile here, we would need to allow
contributors tick +2 on the "real" review themselves, after all, the
reviewer might just have forgotten that...

Andre'