[TYPO3-core] Review system woes

[Alexander Opitz]

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

The rules are broken, from the commit message view I don't see that it 
only adds whitespaces. So if the rules are there you also should write 
that down.

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

But the information is useless, as it is the same as the issue 
description. All what it do is to blow up the output of a "git log" command.

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

As this information should go into the issue and not into the commit 
message.

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

You don't understand my point?

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

So code changes are independent of an issue? So coding without naming 
the problem before?

> You can not look into the bugtracker if you are offline, but you can always
> read all commit messages.

Yeah, the all in one offline hint. Yes, but you can't fetch all new 
commit messages if you are offline. I think thats a bit outdated.

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

I also use annotation in my IDE, works nice, but I had only 2 situations:
- The short line tells enough
- The linked issue tells more than the "long" description gives me
So, I need the issue tracker anyway.

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

Thats why you have the issue tracker, you can see notes from the users 
and so on.

> 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

But you should have three parts, the patch, a short description/subject 
and a link to the issue.

>> 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 tried to show you what happens, if that will be done. That will 
produce silly cross postings.

> I usually do not recheck the issue tracker if I review something.

Thats bad practice. If someone adds a note to a bug you never get this. 
Maybe this person is new, you can't depend on that he is writing a note 
to gerrit too.

> 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).

So at the moment we wait that a patch get reviewed, very long time. So I 
should change this to wait very long, then make the patches for older 
releases and wait more time? Instead of hoping that the reviewer can 
review all patches, cause he has this situation in his brain at the moment?
That only costs extra time and I think thats stupid handling.

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

Nope, mantis was used in the past: http://bugs.typo3.org and the yet 
used issue tracker isn't better then mantis.
For example you can't search in descriptions by bugs that are authored 
by me.

> 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

That is bullshit, the issue tracker isn't used as todo list (for Core), 
nor does realy someone look into that issues. With discussing code 
changes inside the review system you only move issue discussion from one 
to another place. For "default" users a hard way to get into it. And it 
doesn't help to qualify your code.
The code review system should be a helper and not a new place, but that 
isn't practical yet with the issue tracker and gerrit but thats a 
technical problem.

The issue tracker hasn't the possibility of review votes and source code 
comments, thats something gerrit has but this function needs more 
transparent handling between the systems.

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

But we speak about issue tracking and gerrit, we don't speak about the 
forge system.


-- 
Viele Grüße
Alexander Opitz
Developer

T: +49 341 47842 15

++++++++++++++
Netresearch - Passion for eCommerce
++++++++++++++

Netresearch GmbH & Co. KG
Nonnenstraße 11d - 04229 Leipzig
http://www.netresearch.de

Registergericht: AG Leipzig HRA 15614
Komplementär: Netresearch Beteiligungs GmbH, AG Leipzig HRB 17018
Geschäftsführer: Michael Ablass