[TYPO3-core] Gerrit and new review policy for TYPO3 v4 Core

[Ernesto Baschny [cron IT]]

Jigal van Hemert schrieb am 29.03.2011 21:06:

>> this isn't hte case, core votes are required anyway to submit a patch,
>> same for testing (verify).
>> +1 from core dev only signals that another vote is needed.
>> And as Karsten already pointed out: if a core dev votes at first place
>> with +2 and another voter votes +1, the +2 is overwritten.
>> I'm not sure about this as i'm puzzled too, i had an issue where i
>> posted +2 at first place, another voted +1, and overview stated +1 only.
>> At the end i look more to the review overview on upper left which shows
>> clearly: was it tested? wwas it voted by core-dev? a.s.o.
> 
> It gets really confusing. Let's take a look at https://review.typo3.org/971
> 
> You (sorry, I picked the first one in the list :-) nothing personal)
> voted +1 in the code review. Why? +2 would be logical as a core dev.
> I've seen quite a few +1's in my mailbox notifications from core devs in
> issues I watch.
> 
> Any core dev positive vote should count as a core dev positive vote (+2
> in our configuration).

Hm no, Jigal. As Steffen (and others) already tried to explain: Nobody
(not even a core dev) should give a "+2" just because he reviewed the
code. The "+2" is just to be given as soon as the our review policy
matches (one +1 by anyone, one +1 by a core dev).

So the core dev himself has to check if the policy is passed, meaning:
one +1 by a non-core dev, and one +1 by a core dev. So if the core dev
is the first reviewer, he obviously cannot give a "+2" right away (one
review still missing). If the issue already has a +1 review, the core
dev can as well give a "+2" right away (policy match = 2 reviews, one by
non-core dev and one by himself - a core dev).

I guess you were expecting Gerrit to simply "inform" us when the patch
is ready (counting votes), but it has been well known and already
explained a lot that it's *not* the case. We only adapted the "+1" and
"+2" scheme from Gerrit to our Policy that we had before and which
worked out quite well (in the mailing lists).

> If a patch set has enough votes Gerrit should go ahead and put it into
> his repository. That would really help the voting process.

Gerrit only pushes it into git.typo3.org once a "Submit" button is hit,
which is only *unlocked* by the "+2" in both reviewing and verifying.

> Why? You already voted. Sorry, but I'd rather spend time coding than
> going over patches I already reviewed voting for spaces, typos, etc.
> IMO this reduces speed of development and doesn't add much (if anything)
> to code quality.

It reduces changes of not seeing changes in a future version of a patch.
It has been a nightmare to follow the "v1 through v10" patches in the
core mailing list, because there was no way to see right away if the
author was indeed following the changes requested by the reviewers. In
Gerrit it's very easy and quick to compare two patches and you can
re-review the patch you have reviewed already very fast.

> KI as in Artificial Intelligence? Come on, even removing a line in the
> beginning of a file cause Gerrit to fail; even good old TortoiseSVN
> could apply patches when stuff was added or removed.

That's ironic, or are you serious? :)

Cheers,
Ernesto