Re: [PATCH] checkpatch: Warn on empty commit log bodies

[Stefan Richter <stefanr@s5r6.in-berlin.de>]

Mark Brown wrote:
> On Sat, Feb 28, 2009 at 08:25:07PM +0100, Stefan Richter wrote:
>> It isn't just a non-obvious issue with git, it is *no issue* with git in
>> the first place.  It also is no issue with other patch importing tools
> 
> Well, exactly.  There is no issue within git so it's not obvious that
> people are going to have a problem when you're working in git.

Forget git.  It's also no problem if one is never ever working "in git".
 (No problems with tools anyway.)

I for one don't use git in patch creation, review, testing, submission,
and publication of patches (except for feeds to Linus and -next).

>> Since "Subject = title = beginning of changelog" is the long established
>> norm and since the other patch handling tools (and people who handle
>> patches) support this norm, checkpatch should follow this convention as
>> well and count a non-empty RFC 2822 Subject header as one non-empty
>> changelog line.
> 
> As I have previously said, that is not the case in reality.  There
> appears to be substantial sentiment among people handling patches that
> not having any text in the body of the e-mail makes it harder to handle
> patches.

It is indeed a problem
  - if the patch title alone insufficiently describes the patch
or
  - if a patch reviewer believes that it is OK to ignore patch titles.

Checkpatch cannot test for the former and should not test for the latter.

> I don't have that issue myself but I understand it and it
> seems easier to write longer changelogs than to try to change everyone's
> workflows.

So for these people your new checkpatch warning is valid.
For everybody else this warning is a false positive.

> Quite a few of the one line changelogs could probably
> benefit from being expanded a little anyway.

Granted.  But only people can detect this, a script can't.

BTW, a data point:

I just looked at firewire patches post 2.6.25 and found 32 patches out
of 272 patches which only had one line as changelog line (including the
title, excluding Signed-off-by).  I am still very satisfied with their
changelogs.  So that would be an unnecessary checkpatch warning in 12%
of patches which I dealt with in the past few months.

However, 9 of the patches with oneliner log were for a submitted
out-of-tree driver, i.e. cleanup related.  Since that cleanup is over
now, the percentage of easy to explain patches in my practice will go
down again.  Therefore I will shut up now even though I still disagree
with your way of counting changelog lines. :-)
-- 
Stefan Richter
-=====-==--= --== ----=
http://arcgraph.de/sr/