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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 01 Mar 2009 00:01:38 +0100
From:	Stefan Richter <stefanr@...6.in-berlin.de>
To:	Mark Brown <broonie@...ena.org.uk>
CC:	Andy Whitcroft <apw@...onical.com>, linux-kernel@...r.kernel.org
Subject: Re: [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/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/