lkml.org 
[lkml]   [2009]   [Feb]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] checkpatch: Warn on empty commit log bodies
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/


\
 
 \ /
  Last update: 2009-03-01 00:05    [W:0.076 / U:0.156 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site