Open Stack

On 11/07/2013 05:35 PM, Jiri Tomasek wrote:
> 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...)

Agreed, the comments are there for a reason. It's also handy to provide
forward looking statements to other reviewers in case you don't get back
to the review queue right away.

For instance when I know that might happen I tend to leave comments like
"I'm +2 after the following .... is addressed." That's also extremely
helpful to provide incentive to the submitter to make those kinds of
changes.

Not all submissions fit into this kind of category, some really need to
iterate through sets of issues to get ready for the code base. But when
they are close, defining the end state for the contributor and other
reviewers that won't feel like they need to wait for your comments on
the updated review (because you clearly explained what you thought
needed to change) is helpful.

	-Sean

-- 
Sean Dague
http://dague.net

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 482 bytes
Desc: OpenPGP digital signature
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20131108/d710947b/attachment.pgp>