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
_______________________________________________
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
