[TYPO3-core] Review system woes

[Philipp Gampe]

Hi Alexander ,

Alexander Opitz wrote:

> Hi Philipp,
> 
>> You should not write a test scenario in the commit message. You can find
>> hinds on the internet on how to write a good commit message.
> 
>  >>>
> commit a467d46876894f5d798a62a453ec384dbfbd3e4b
> Author: Christian Kuhn <lolli at schwarzbu.ch>
> Date:   Thu Feb 7 21:13:15 2013 +0100
> 
>      [BUGFIX][Cache] Method parameter CGL fixes
> 
>      Change-Id: Ie237c62fcd25d0f4ac2430983183756c7aebc633
>      Resolves: #45257
>      Releases: 6.1, 6.0
> <<<
> Good hint.

As I read the commit message, then the patch is a cleanup that does not 
change any logic. It just moves/introduces some whitespace.

It is perfectly OK to relay on the subject line for trivial changes.
It would be pointless if the description just repeats the subject line. If 
there are no more information, then there is not need for a detailed 
description.
 
>  >>>
> commit d7b5d829e7d9a3a6699803e5c7ea308e6b2f55ca
> Author: Christian Kuhn <lolli at schwarzbu.ch>
> Date:   Sun Feb 3 12:13:25 2013 +0100
> 
>      [!!!][TASK] Get rid of loadTCA and simplify FE cache behavior
> 
>      The frontend rendering aims to not load the full TCA including
>      columns settings to reduce rendering time for full cache pages.
> ...
> <<<
> Oh the issue tracker description, nothing realy informational beside the
> bug tracker.

This is a big, breaking change as you can see by the [!!!] tag.
It has a rather long and very detailed commit message, as the change is not 
trivial and any reviewer (in the sense of checking the git log) should have 
an idea what is done and why.
Although most changes will not require a that long commit message, this is 
the right way to go.
Also note that this is a feature and the issue as just opened to have a 
reference in the review system (as required by policy). This reference is 
needed if you want to connect multiple issues.

> So a short message is realy enough: See:
> http://hg.mozilla.org/mozilla-central

This depends on the message. Subject lines are limited to 72 chars.
Sometimes it is not enough to include all needed information, especially not 
for bigger changes.

> Oposite linux kernel:
> http://git.kernel.org/?p=linux/kernel/git/stable/linux-stable.git;a=log
> But they have no central bug/issue tracker, most of them are described
> merge requests of many small commits in different git repos.

Making small changes is a good way to keep commit messages short.

>> Basically, describe the problem in one or two sentences, then describe
>> how you fix it.
> 
> Why? If that is needed, then it is part of the bug report. And how I
> fixed it? Mostly by change the code, sometimes by removing cache files
> that don't get regenerated if they should.

The description is not about the bugtracker or any information you find in 
there, but of what changes you do to the code.
The bug information are secondary and domain of the bugtracker, but changes 
are domain of the source code control system.

>> The purpose of the commit message is that you can are able to figure out
>> why a specific chunk of code has change.
> 
> Yeah, look in the linked bug report, if the short message don't tell you
> all. In the bug tracker you can add or edit extra descriptions. Also
> later on, after commiting a patch. You don't need to double that
> information.

You can not look into the bugtracker if you are offline, but you can always 
read all commit messages.
Also most IDE support code history inspection (sometimes called annotation). 
That way you can figure out why a specific line was change without having to 
cross check the bugtracker all the time.

>> All information not relevant for this should go to the issue tracker or
>> change review system.
> 
> What information isn't relevant? And when put it into review system and
> when to put it in the issue tracker?

The review system is about the source code. As stated above, you need all 
information to understand why a specific code is changed in that commit.
Later you use this to track down bugs and unexpected behaviors.

E.g. an how to test is not relevant for the source code, thus leave it out 
of the commit message. How to reproduce the issue might sometimes be 
relevant if you can not easily guess that by reading the patch and the 
description. In this case it should be part of the commit message.
After all, you only have two things in the source code management system:
1) The patch
2) The description with the subject line as summery
 
>> Once a change is in the review system, make sure to always add a comment
>> there if you add information to the issue system.
> 
> There the hell begins, so we get folloowing:
> 
> issue tracker               gerrit
> A: Yeah info                A: Some more info added in issue tracker
> B: See new info in gerrit   B: My new info
> ...

That is why I said that you should always add a note to Gerrit of you post 
something in the issue tracker. I usually do not recheck the issue tracker 
if I review something.

> You need both to check the information, and the problem raises if you
> have multiple patches for different TYPO3 versions in gerrit.

That is why you should not push a patch to multiple branches at once. It is 
not so much about information, but you easily loose the synchronization of 
the patches.
Push a patch to the highest branch first (which one depends on the patch).

>> Otherwise it might not get noticed. Of course, it would be best if a
>  > change in the issue system would trigger a comment automatically,
>> but that needs someone to get his hands dirty.
> 
> So it needs more/better integration.
> Look at https://bugzilla.mozilla.org/show_bug.cgi?id=454880 there you
> have the box "Attachments" choose "Show Obsolete" on the bottom of the
> box. Now you see all attachements (like the commited patches to gerrit)
> with their + and - reviews.

Bugzilla ... super ugly, besides that, we used this in the past.

> If you have two systems, you will split the people in two groups and
> you'll get half the power.

That is bullshit.

We have two systems that solve two different problems:
1) The issue tracker to keep a todo list with comments and longer examples*
2) The source code review system to review and discuss source code changes

*Note that forge is more then just an issue tracker.

Best regards
-- 
Philipp Gampe – PGP-Key 0AD96065 – TYPO3 UG Bonn/Köln
Documentation – linkvalidator
TYPO3 .... inspiring people to share!