Open Stack

On 11/07/2013 08:25 AM, Daniel P. Berrange wrote:
> On Thu, Nov 07, 2013 at 12:21:38AM +0000, Day, Phil wrote:
>>> Leaving a mark.
>>> ===============
>>>
>>> You review a change and see that it is mostly fine, but you feel that since you
>>> did so much work reviewing it, you should at least find
>>> *something* wrong. So you find some nitpick and -1 the change just so that
>>> they know you reviewed it.
>>>
>>> This is quite obvious. Just don't do it. It's OK to spend an hour reviewing
>>> something, and then leaving no comments on it, because it's simply fine, or
>>> because we had to means to test someting (see the first pattern).
>>>
>>>
>> Another one that comes into this category is adding a -1 which just says "I agree with
>> the other -1's in here".   If you have some additional perspective and can expand on
>> it then that's fine - otherwise it adds very little and is just review count chasing.
> I don't think that it is valueless as you describe. If I multiple people
> add '-1' with a "same comments as <name>", then as a core reviewer I will
> consider that initial -1 to be a much stronger nack, than if only one person
> had added the -1. So I welcome people adding "I agree with <blah>" to any
> review.
>
>> It's an unfortunate consequence of counting and publishing review stats that having
>> such a measure will inevitable also drive behavour.
> IMHO what this shows is not people trying to game the stats, but rather the
> inadequacy of gerrit. We don't have any way to distinguish a "-1 minor nice
> to have nitpick" from a "-1 serious code issue that is a must-fix". Adding
> a -2 is really too heavyweight because it is sticky, and implies "do not
> ever merge this".
>
> It would be nice to be able to use '-2' for "serious must-fix issue" without
> it being sticky, and have a separate way for core reviewers to put an review
> into a "block from being merged indefinitely" state - perhaps a new state
> would be more useful eg a "Blocked" state, to add to New, Open, Merged,
> Abadoned.
>
> Daniel
The comment describing the  -1 should be enough to distinquish between 
minor nitpick and serious code issue IMHO. If it is a serious issue, 
other reviewers also giving -1 confirming the issue is probably a good 
thing. (Not with the minor nit though...)

Jirka