[Engine-devel] Changing Gerrit -1 message

[Doron Fediuck]

----- Original Message -----
> From: "Moti Asayag" <masayag at redhat.com>
> To: "Eli Mesika" <emesika at redhat.com>
> Cc: engine-devel at ovirt.org
> Sent: Friday, February 22, 2013 9:02:08 AM
> Subject: Re: [Engine-devel] Changing Gerrit -1 message
> 
> On 02/22/2013 12:53 AM, Eli Mesika wrote:
> > 
> > 
> > ----- Original Message -----
> >> From: "Dave Neary" <dneary at redhat.com>
> >> To: engine-devel at ovirt.org
> >> Sent: Thursday, February 21, 2013 11:21:53 PM
> >> Subject: Re: [Engine-devel] Changing Gerrit -1 message
> >>
> >> Hi,
> >>
> >> On 02/20/2013 10:32 AM, Itamar Heim wrote:
> >>> On 19/02/2013 12:06, Laszlo Hornyak wrote:
> >>>> Hi,
> >>>>
> >>>> I agree with that.
> >>>>
> >>>> for the - messages this in my opinion would be both more clear
> >>>> and
> >>>> friendly:
> >>>> -1: In my opinion it needs work.
> >>>
> >>>
> >>> how about
> >>> "-1: Please review my comments"
> > 
> > +1
> 
> +1
> 

Seems that we're in agreement on this one....

> > 
> >>
> >> Sounds great.
> >>
> >>>> -2: I disagree.
> > 
> > I prefer -2 : Do not submit!
> > 
> > IMHO:
> > -1 should be used whenever the code is OK but can be done better or
> > has a missing part (tests for example)
> > -2 should be used when the code does not work, has a serious bug
> > (possible NPE for example) , break the build
> 
> +1 for the -2 which to my opinion should indicate any serious flaw in
> the design, even post the design discussion. I think that this should
> follow with a discussion on engine-devel about the required change in
> the design instead of debating over the gerrit since it is not
> transparent enough.
> 

-2 should be given on rare occasions, as it is so blunt and meaningful.
Remember that -1 is good enough to block a patch.
So if you see an issue which may be fixed, give it -1.

If you see something which is illegal (violate project license) or
contradicts the project foundations, give it -2. This is how rare -2
should be.

In this view I'd rephrase "-2" to: "This patch can not be accepted".
Obviously this should be followed by a detailed explanation by the reviewer.

> > 
> >>>
> >>> "-2: Please reconsider"
> >>
> >> I am trying to think under which circumstances people give a -2.
> >> Maybe
> >> something like "We have discussed this feature, and I disagree
> >> that
> >> it
> >> is good for the project." Basically, I don't think that a
> >> developer
> >> should ever see a -2, unless they specifically disagree with the
> >> maintainer, and insist that they are right, to the point of
> >> repeatedly
> >> submitting patches.
> >>
> >> Cheers,
> >> Dave.
> >>