[TYPO3-core] Gerrit and new review policy for TYPO3 v4 Core
Jigal van Hemert
jigal at xs4all.nl
Wed Mar 30 09:44:08 CEST 2011
On 30-3-2011 8:25, Ernesto Baschny [cron IT] wrote:
> Jigal van Hemert schrieb am 29.03.2011 21:06:
>> 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).
Okay, if I understand correctly in the two cases we have the procedure
1. patch by core dev:
- +1 by anyone
- +1 by core dev
- patch owner gives +2 and submits
2. patch by other:
- +1 by anyone
- +1 by core dev
- same core dev gives +2 and submits
Anyway if a patch has a +1 by anyone the next core dev who gives a
positive vote (of course both by reviewing the code and testing; if a
vote is only by reviewing anyone can extrapolate the procedure) should
also submit it.
In the first case, if the core dev gives a vote when there is already a
vote by anyone (review+test) should (s)he also submit it in one go?
Anyway, I don't expect to see more than one +1 by a core dev; the second
vote could (and IMO should) already submit the patch.
This is part of the point I was trying to make: I see too many +1 "fine
by me, someone else should submit" votes in email notifications.
> 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.
That would be an extra value over the procedure in the mailing lists...
> 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.
Another missing feature... if a patch enough votes it can be included in
> 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.
It has already been explained that rebasing often makes it impossible to
compare two patches in Gerrit. We simply can't trust that feature.
>> 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? :)
I'm serious. I pushed the Submit button on a change and got (again) the
error that Gerrit couldn't put it in the repository because of a path
problem. I was given the explanation that the cause was that Xavier
removed the $Id lines in a lot of files. Apparently the lines for the
merge/cherry-pick were off by at least one line and Gerrit gives up.
It seems that a three-way-merge can solve this, but this wasn't full
My experience with applying a patch with TortoiseSVN was that there
could be quite a bit added and removed in a file and it was still
capable of applying the patch. In case of conflicts (the patch tried to
modify lines which were changed in a revision later than the one the
patch was based on) there was a nice three-way-merge editor where you
could decide what to do with each line.
Kind regards / met vriendelijke groet,
Jigal van Hemert.
More information about the TYPO3-team-core