Messages in this thread | | | Date | Sun, 01 Mar 2009 00:01:38 +0100 | From | Stefan Richter <> | 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/
| |