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

[Jigal van Hemert]

Hi,

On 29-3-2011 11:38, Steffen Kamper wrote:
> Am 29.03.2011 11:10, schrieb Jigal van Hemert:
>> Unfortunately core devs can also give +1s and do this frequently. This
>> is really not very helpful, because with 10 +1s the patch can still not
>> be submitted.
>
> 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).

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

>> Also, the rule was that the first core dev involved (either the patch
>> owner if that was a core dev or the first core dev to give a +1) would
>> take care of committing.
>> Now you have the option to give your vote (Publish) and not commit
>> (Submit) the patch.
>
> anyone now can submit, no problem - credits are all in the changelog.
> But be careful, some patches has to be applied to other branches as well.

This was covered by the rule that the patches for all branches involved 
should be pushed and we should not wait for the master branch to be 
submitted before actually port a change to other branches?

>>> Of course a new patchset resets the voting level, which is indeed fine.
>> Even for non-functional changes (CGL, commit message, etc.) the voting
>> is reset. Gerrit cannot decide, but this is not fine in all cases IMO.
> np from my side - i view again, see the non-functional changes and give
> my vote again.

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.

>> Even in the cases where it's useless to review it again the
>> votes are needed in Gerrit.
> just wait a bit or ask the reviewers direct.

Again, it takes longer to get a patch in the repository and doesn't 
bring anything positive.

> i think that gerrit can't have a complete KI. (...) it's
> much better than only the news list ;)

I don't agree. The only advantage with Git/Gerrit to me is that we don't 
have to take care of the changelog.
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.

> Yesterday i learned a new thin and it should be a rule:
>
> Whenever you start modifying files, create a branch!!!

And when you haven't finished modifying and don't want to commit you 
can't switch branches; then you can "stash" your changes and the branch 
is clean for switching (kinda like pushing all stuff in your room under 
the bed :-) )

-- 
Kind regards / met vriendelijke groet,

Jigal van Hemert.