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

[Jigal van Hemert]

Hi,

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

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 
the repository.

> 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 
proof either.

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.