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