Open Stack

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 :|