[TYPO3-core] Patch Sets in Gerrit

[Markus Klein]

Hi Ernesto,

I've a few remarks below.

Kind regards
Markus

------------------------------------------------------------
Markus Klein
TYPO3 CMS Active Contributors Team Member
> 
> Hi,
> 
> it is very convenient to upload new patchsets to existing reviews on Gerrit.
> One disadvantage is that the voting score is "cleared". But that hasn't been a
> problem yet.
> 
> One problem I have with reviews that contain >10 Patchset is when new
> patchsets are not described. I would encourage people to write a few lines
> *why* they are sending a new patchset in the comments after uploading it.
> 
> For example:
> 
> https://review.typo3.org/#/c/11804/
> 
> Jigal made some note on Patchset 19. Then came patchset 20 and 21 (Jo and
> Georg), without any comment. Was Jigal's point considered?
> 
> Other prominent example is the Responsive Image rendering:
> 
> https://review.typo3.org/#/c/22052/
> 
> We are already at patchset 29. Of course I can often do a "diff" between
> patchsets to see "what has changed", but it helps focus on the important
> aspects if you have a sentence describing what you did.

Indeed it might help to focus on the major change, but unfortunately in order vote properly, one has to review the whole code diff anyway.
We had it too often that a simple change also contained some unnoticed changes.

> 
> Here's an example where I commented new patchesets after upload:
> 
> https://review.typo3.org/#/c/24187/
> 
> (see #4, #5).
> 
> Often a new patchset is uploaded just to "rebase" to the current state of
> master (and resolving conflicts etc). This is something worth mentioning in
> the notes and help people understand why there is a new patchset. And if
> you are rebasing, it probably means you have some interest in this particular
> patch, maybe explaining it a bit more at the same time helps other occasional
> reviewers undestand why they should review it.

No, I not always have a particular interest when I'm rebasing patches.
Rebasing long standing patches triggers emails to all involved persons, getting the issue back to their mind (hopefully).
If something was only rebased by using the Gerrit button this can be seen because of the comment automatically posted by Gerrit.
(And for those who disabled these emails, I can only suggest to use the tools that email clients provide, to automatically flag emails where one's been involved.
This really helps dealing with the vast amount of mails sent by Gerrit, but IMHO this is vital to stay on track, what's going on.)

> 
> Also please remember that rebasing a patchset will most probably make it
> difficult (if not impossible) to compare it to a previous patchset, if there has
> been other changes in between in the same file. So reviewers that are using
> the "Diff Patchsets" tool to incrementally review newer patchsets will have
> trouble with it. Consider uploading a rebased patchset *only* in the situation
> when there is a conflict while rebasing or the patchset is already really old.

Indeed this is not so easy, but could be a motivation for everyone to first strive to clean up one's dashboard to zero before spending work on even more patches.
(which will be rebased again and cause even more hassle on reviewing)

> 
> What do you think? Other opinions? Nobody stumbled over these problems
> yet?
> 
> Kind regards,
> Ernesto