On Thu, Aug 21, 2014 at 12:40:59PM -0400, Adam Young wrote: > On 08/21/2014 12:21 PM, Daniel P. Berrange wrote: > >On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote: > >>"I would prefer that you didn't merge this." > >> > >>i.e. The project is better off without it. > >A bit off topic, but I've never liked this message that gets added > >as it think it sounds overly negative. It would better written > >as > > > > "This patch needs further work before it can be merged" > Excellent. > > It also bothers me that a -1 is dropped upon a new submission of the patch. > It seems to me that the review should instead indicate on a given line level > comment whether it is grounds for -1. If it is then either that same > reviewer or another can decide whether a given fix address the reviewers > request.
I guess the idea of dropping the -1 is based on the understanding that most contributors are working in good faith. ie that in the common case they will actually address the review feedback before re-submitting a new version. Sure some people violate this expectation, but in general our contributor base does the right thing in this respect, which is good. As a core reviewer I'd aim to look at the previous version to see if there was an serious -1 there that was not address before approving something anyway. The problem with always preserving the -1 across re-posts, is that it would discourage people from looking at new postings of the patch. A gerrit query will show the -1's sitting there against the patch with no indication that those -1s are probably "stale" and now fixed by the new posting. So I really wouldn't want to see the -1's preserved Regards, 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 [email protected] http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
