Open Stack

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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|