[PATCH] checkpatch: Warn on empty commit log bodies

[PATCH] checkpatch: Warn on empty commit log bodies

[Mark Brown]

Try to help ensure that e-mailed patches have some commit log text in
the body of the e-mail by warning if we can't find any lines that look
like a header of some kind prior to the Signed-off-by.

Signed-off-by: Mark Brown <broonie@sirena.org.uk>
---
 scripts/checkpatch.pl |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 45eb0ae..d162421 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1072,6 +1072,7 @@ sub process {
 
 	our $clean = 1;
 	my $signoff = 0;
+	my $headers = 0;
 	my $is_patch = 0;
 
 	our @report = ();
@@ -1258,6 +1259,11 @@ sub process {
 
 		$cnt_lines++ if ($realcnt != 0);
 
+		# Check for a RFC822 style header
+		if ($line =~ /^([a-z0-9-]+:|From )/i) {
+			$headers++;
+		}
+
 #check the patch for a signoff:
 		if ($line =~ /^\s*signed-off-by:/i) {
 			# This is a signoff, if ugly, so do not double report.
@@ -1270,6 +1276,14 @@ sub process {
 				WARN("space required after Signed-off-by:\n" .
 					$herecurr);
 			}
+
+			# There should be at least 1 line of ordinary
+			# text for the body of the commit log prior to
+			# the signoff.
+			if ($linenr - $headers < 2) {
+				print $headers . $linenr . "\n";
+				WARN("no body for commit log");
+			}
 		}
 
 # Check for wrappage within a valid hunk of the file
-- 
1.5.6.3

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

[Stefan Richter]

Mark Brown wrote:
> Try to help ensure that e-mailed patches have some commit log text in
> the body of the e-mail by warning if we can't find any lines that look
> like a header of some kind prior to the Signed-off-by.

This gives false positives for patches like this:

From: [...]
Subject: this single line fully explains the whole patch

Signed-off-by: [...]
---
[...]

-- 
Stefan Richter
-=====-==--= --=- ===--
http://arcgraph.de/sr/

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

[Mark Brown]

On Sat, Feb 28, 2009 at 02:58:14PM +0100, Stefan Richter wrote:

> This gives false positives for patches like this:

> From: [...]
> Subject: this single line fully explains the whole patch

> Signed-off-by: [...]

That's not a false postive, that's exactly the case it's intended to
catch.  People like Andrew Morton complain because there's no text in
the actual body of the email (as well as because some of the patches
that do this could probably use a bit more explanation).

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

[Stefan Richter]

Mark Brown wrote:
> On Sat, Feb 28, 2009 at 02:58:14PM +0100, Stefan Richter wrote:
> 
>> This gives false positives for patches like this:
> 
>> From: [...]
>> Subject: this single line fully explains the whole patch
> 
>> Signed-off-by: [...]
> 
> That's not a false postive, that's exactly the case it's intended to
> catch.

It *is* a false positive if a single-line changelog is entirely
sufficient.  (There are many patches whose changelogs can and should be
a single line, plus sign-offs.)

You could as well

	if few_changelog_lines
		warn "You have written a short changelog."
		warn "Don't you want to write some more?"
		warn "(PS: I don't care what you write in particular"
		warn "as long as you just add more lines.)"

	if many_changelog_lines
		warn "You have written a long changelog."
		warn "Can't you express yourselves more concisely?"
		warn "(PS: I don't care what you'll omit in particular"
		warn "as long as you get the line count down.)"

> People like Andrew Morton complain because there's no text in
> the actual body of the email (as well as because some of the patches
> that do this could probably use a bit more explanation).

Well, I haven't closely watched akpm's complaints, but I'm sure he
doesn't complain about too few lines changelog, he complains about
insufficient information in the changelog.

Your checkpatch modification does not check for insufficient information
in the changelog.  It only checks line count.
-- 
Stefan Richter
-=====-==--= --=- ===--
http://arcgraph.de/sr/

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

[Mark Brown]

On Sat, Feb 28, 2009 at 05:14:48PM +0100, Stefan Richter wrote:
> Mark Brown wrote:

> > That's not a false postive, that's exactly the case it's intended to
> > catch.

> It *is* a false positive if a single-line changelog is entirely
> sufficient.  (There are many patches whose changelogs can and should be
> a single line, plus sign-offs.)

No, really.  People are literally saying that no changelog in the body
of the e-mail is a problem for them.  Since if (like me) you use git
there's nothing in the toolchain that suggests that a changelog might
not have been written it seemed best to add something to generate a
warning.

> > People like Andrew Morton complain because there's no text in
> > the actual body of the email (as well as because some of the patches
> > that do this could probably use a bit more explanation).

> Well, I haven't closely watched akpm's complaints, but I'm sure he
> doesn't complain about too few lines changelog, he complains about
> insufficient information in the changelog.

No, people (not just Andrew) really are complaining about a simple lack
of any text in the body of the mail.  The term "unchangedlogged" has
been specifically used here.  It is apparent that they do not consider
the subject line of the e-mail (which is the first line of the changelog
if you're using git) to be part of the changelog.

> Your checkpatch modification does not check for insufficient information
> in the changelog.  It only checks line count.

Well, yes.  The intention is purely to detect if there is a body in the
changelog.

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

[Stefan Richter]

Mark Brown wrote:
> It is apparent that they do not consider
> the subject line of the e-mail (which is the first line of the changelog
> if you're using git) to be part of the changelog.

This really surprises me.  ^Subject: .+$ is the title of the log and
thereby its first part.  Also, I don't see a connection to git.  I
manage patches with quilt and never loose patch titles.  I still think
you should count ^Subject: .+$ too if you want to check for presence of
a changelog.

Sometimes I see patch postings with the title appearing twice; I always
thought these were beginners' mistakes and a nuisance when importing a
patch.

(PS:  Granted, many patches come with poorly written changelogs; I
struggle with it in my own patches all the time too.  But it's something
which checkpatch just cannot check.)
-- 
Stefan Richter
-=====-==--= --=- ===--
http://arcgraph.de/sr/

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

[Arjan van de Ven]

On Fri, 27 Feb 2009 18:05:20 +0000
Mark Brown <broonie@sirena.org.uk> wrote:

> Try to help ensure that e-mailed patches have some commit log text in
> the body of the e-mail by warning if we can't find any lines that look
> like a header of some kind prior to the Signed-off-by.

maybe we should also spell check the changelog (aspell ?)

-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

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

[Mark Brown]

On Sat, Feb 28, 2009 at 09:40:12AM -0800, Arjan van de Ven wrote:
> Mark Brown <broonie@sirena.org.uk> wrote:

> > Try to help ensure that e-mailed patches have some commit log text in
> > the body of the e-mail by warning if we can't find any lines that look
> > like a header of some kind prior to the Signed-off-by.

> maybe we should also spell check the changelog (aspell ?)

I'd expect that'd lead to a lot of false positives on names and
technical terms, wouldn't it?

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

[Mark Brown]

On Sat, Feb 28, 2009 at 06:33:23PM +0100, Stefan Richter wrote:

> This really surprises me.  ^Subject: .+$ is the title of the log and
> thereby its first part.  Also, I don't see a connection to git.  I
> manage patches with quilt and never loose patch titles.  I still think
> you should count ^Subject: .+$ too if you want to check for presence of
> a changelog.

I believe the issue is the UI of a MUA - the subject line of a mail is
normally presented separately to the body and isn't always as
immediately prominent as the body so it's harder work to look at it.
This is sensible for e-mail since the general style is that the subject
line shouldn't be required in order to comprehend what the message is
about but that's exactly what's happening when a one line changelog is
e-mailed.

The connection with git is that it doesn't really draw a similar
distinction so the issue isn't as immediately obvious when you're
working within it. 

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

[Stefan Richter]

Mark Brown wrote:
[some people disregard the patch title in the Subject header]
> I believe the issue is the UI of a MUA - the subject line of a mail is
> normally presented separately to the body and isn't always as
> immediately prominent as the body so it's harder work to look at it.

That's not universally true for all MUA; and in affected MUAs only if
mails are read one-after-another, not if mails are read by looking at
the list of messages in a mailbox first, then going to the interesting
messages.  So if there are people who disregard the patch title for this
reason, then I dare to say it is because their view is limited by their
particular MUA and their individual mail reading preferences.  Their
preferences still don't make the convention go away that the Subject is
the title.  (And that the title should be a quick intro into what the
patch is about.)

> This is sensible for e-mail since the general style is that the subject
> line shouldn't be required in order to comprehend what the message is
> about

No.  The subject is the primary means to establish context for the
message (along with the mailinglist topic).  We ask people to post with
a good subject.

And since the subject already established context, there is no need to
repeat its information in the e-mail body.

> The connection with git is that it doesn't really draw a similar
> distinction so the issue isn't as immediately obvious when you're
> working within it. 

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
like quilt.  All those tools either explicitly or implicitly support the
notion that the RFC 2822 Subject header contains the patch title.  They
support it because the *people* who use those tools support it.

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.
-- 
Stefan Richter
-=====-==--= --=- ===--
http://arcgraph.de/sr/

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

[Mark Brown]

On Sat, Feb 28, 2009 at 08:25:07PM +0100, Stefan Richter wrote:

> messages.  So if there are people who disregard the patch title for this
> reason, then I dare to say it is because their view is limited by their
> particular MUA and their individual mail reading preferences.  Their

Look, I pretty much agree.  Personally I don't find this a problem.

> > The connection with git is that it doesn't really draw a similar
> > distinction so the issue isn't as immediately obvious when you're
> > working within it. 

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

> 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.  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.  Quite a few of the one line changelogs could probably
benefit from being expanded a little anyway.

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

[Stefan Richter]

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/

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

[Theodore Tso]

On Sun, Mar 01, 2009 at 12:01:38AM +0100, Stefan Richter wrote:
> > 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.

Worse yet, if we start getting these sorts of entries being returned
by "git log":

------------
ext4: Fix spelling error: successfull

Fix spelling error: successfull

Signed-off-by: "Trivial Patch Submitter" <spelling@nits.org>
------------

Just to shut up checkpatch, I'm going to feel the urge to shake a
checkpatch maintainer warmly by the throat.

Sometimes, all that is needed is:

------------
ext4: Fix spelling error: successfull

Signed-off-by: "Trivial Patch Submitter" <spelling@nits.org>
------------

							- Ted

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

[Mark Brown]

On Sat, Feb 28, 2009 at 07:18:29PM -0500, Theodore Tso wrote:

> Just to shut up checkpatch, I'm going to feel the urge to shake a
> checkpatch maintainer warmly by the throat.

> Sometimes, all that is needed is:

> ------------
> ext4: Fix spelling error: successfull
> 
> Signed-off-by: "Trivial Patch Submitter" <spelling@nits.org>
> ------------

As I've said already I pretty much agree with this.

The reason I sent the patch was that sending changelogs like that for
trivial changes is getting me negative feedback and I'm seeing other
comments about "unchangeloged patches" on the lists so I'm pretty sure
it's not just something I'm doing.  I'm not saying I'm always blameless
here but when people are using terms like like "unchangeloged" it really
does suggest that one line changelogs are just considered not to have
changelogs.

Some consistency would be good here.

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

[Theodore Tso]

On Sun, Mar 01, 2009 at 12:46:19AM +0000, Mark Brown wrote:
> 
> The reason I sent the patch was that sending changelogs like that for
> trivial changes is getting me negative feedback and I'm seeing other
> comments about "unchangeloged patches" on the lists so I'm pretty sure
> it's not just something I'm doing.  I'm not saying I'm always blameless
> here but when people are using terms like like "unchangeloged" it really
> does suggest that one line changelogs are just considered not to have
> changelogs.

Well, I certainly don't have a problem with this.  I in fact get
*really* *annoyed* when I get patch submissions where the subject line
is replicated in the body, since I then have to manually edit the mail
message before I can run "git am" on the mail message.

Who's been complaining?  I can certainly tell you I'll complain in the
opposite direction, but that's because it actually causes me more work
as a maintainer.  If people are kvetching, maybe they should complain
to git mailing list and ask for a different git commit to e-mail
message convention --- but it's really not hard to look at the subject
line.

						- Ted

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

[Mark Brown]

On Sat, Feb 28, 2009 at 09:53:57PM -0500, Theodore Tso wrote:

> Who's been complaining?  I can certainly tell you I'll complain in the
> opposite direction, but that's because it actually causes me more work

Andrew Morton is one of them but not the only one.  Like I say, I don't
want to claim that my changelogs are always ideal here, it was mostly
the specific language used that made me think of doing this.

> as a maintainer.  If people are kvetching, maybe they should complain
> to git mailing list and ask for a different git commit to e-mail
> message convention --- but it's really not hard to look at the subject
> line.

Any attempt to change the format to handle this would presumably also
end up confusing other tools that work with patches, though (at least to
the point of causing the first line of the commit log to be replicated).

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

[Stefan Richter]

Mark Brown wrote:
> On Sat, Feb 28, 2009 at 09:53:57PM -0500, Theodore Tso wrote:
>> Who's been complaining?  I can certainly tell you I'll complain in the
>> opposite direction, but that's because it actually causes me more work
> 
> Andrew Morton is one of them but not the only one.  Like I say, I don't
> want to claim that my changelogs are always ideal here, it was mostly
> the specific language used that made me think of doing this.

As far as I have observed, akpm's (Cc'd now) complaints are about
patches whose impact or benefit etc. are insufficiently explained ---
which is an issue on a higher level than pure formalism.  I believe I
too have seen the term "unchangelogged" (as you mentioned) in one of
those discussions but I associated lack of information with it rather
than a violation of a formalism.

I still say there are some straightforward changes which /can/ be well
explained in a single line (which would be the title line).  Still, by
far the most changes, including several kinds of janitorial changes,
require more explanation than that.  At which level a changelog should
start and how deep it should go is a rather subjective matter of course.
It is not trivial to give general advice on that, and it is impossible
to encode even simple tests for the quality of a changelog in a script
like checkpatch.

I for one am training how to write changelogs by the following methods:
 0. I occasionally write some of course.
 1. I intensively work with code written by other people long ago and
    wonder why it came to be how it is.  I look up when the code was
    added or changed and try to make sense of the changelogs which were
    provided at that time.
 2. I write release notes for a subsystem (targeted primarily towards
    users, secondarily towards developers) and use changelogs as primary
    input for that.
 3. I issue pull requests for new changes to be merged into the
    mainline.  These pull requests include a shortlog, plus extra
    information if the shortlog is unable to give a good picture of
    what the pull request is about.  The ideal would be that the
    shortlog says it all.
Nr. 1 especially trains to avoid lack of detail.  Nr. 2 and 3 train to
not forget the high-level viewpoint and to aim for clear language.  (I
am not sure about the success of this training though. ;-)
-- 
Stefan Richter
-=====-==--= --== ---=-
http://arcgraph.de/sr/

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

[Mark Brown]

On Mon, Mar 02, 2009 at 04:15:49PM +0100, Stefan Richter wrote:
> Mark Brown wrote:

> > Andrew Morton is one of them but not the only one.  Like I say, I don't

> As far as I have observed, akpm's (Cc'd now) complaints are about
> patches whose impact or benefit etc. are insufficiently explained ---
> which is an issue on a higher level than pure formalism.  I believe I
> too have seen the term "unchangelogged" (as you mentioned) in one of
> those discussions but I associated lack of information with it rather
> than a violation of a formalism.

The terminology and comments about normally skipping these "unchangelogged"
patches create a very different impression.  Obviously, there's going to
be a crossover between the two cases.

> I still say there are some straightforward changes which /can/ be well
> explained in a single line (which would be the title line).  Still, by
> far the most changes, including several kinds of janitorial changes,
> require more explanation than that.  At which level a changelog should

Sure, this is all very standard stuff.

> It is not trivial to give general advice on that, and it is impossible
> to encode even simple tests for the quality of a changelog in a script
> like checkpatch.

As I've said already on a number of occasions the patch was purely
intended to catch the case where there was no body in the patch log,
which appeared to be something that was being specifically objected to.

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

[Andrew Morton]

On Mon, 02 Mar 2009 16:15:49 +0100 Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> Mark Brown wrote:
> > On Sat, Feb 28, 2009 at 09:53:57PM -0500, Theodore Tso wrote:
> >> Who's been complaining?  I can certainly tell you I'll complain in the
> >> opposite direction, but that's because it actually causes me more work
> > 
> > Andrew Morton is one of them but not the only one.  Like I say, I don't
> > want to claim that my changelogs are always ideal here, it was mostly
> > the specific language used that made me think of doing this.
> 
> As far as I have observed, akpm's (Cc'd now) complaints are about
> patches whose impact or benefit etc. are insufficiently explained ---
> which is an issue on a higher level than pure formalism.  I believe I
> too have seen the term "unchangelogged" (as you mentioned) in one of
> those discussions but I associated lack of information with it rather
> than a violation of a formalism.

Oh absolutely.  Quite often the changelog body contains no information
which wasn't in the title, so there's no need for a body.

I think what triggered this was a patch from Mark which had no
changelog and which had me sitting there wondering wtf it does, whether
we need it in 2.6.29, whether we need it in 2.6.28.x and earlier and me
not having the foggiest clue then getting grumpy.

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

[Mark Brown]

On Mon, Mar 02, 2009 at 10:01:58AM -0800, Andrew Morton wrote:

> I think what triggered this was a patch from Mark which had no
> changelog and which had me sitting there wondering wtf it does, whether
> we need it in 2.6.29, whether we need it in 2.6.28.x and earlier and me
> not having the foggiest clue then getting grumpy.

Do you mean no changelog in the body of the e-mail here?  I'm assuming
now that you mean no changelog in the body but when you say "no
changelog" that reads differently.  I'm not saying the changelog was
perfect here but your comments really do read like you felt there was
nothing at all.

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

[Andrew Morton]

On Mon, 2 Mar 2009 18:24:57 +0000 Mark Brown <broonie@sirena.org.uk> wrote:

> On Mon, Mar 02, 2009 at 10:01:58AM -0800, Andrew Morton wrote:
> 
> > I think what triggered this was a patch from Mark which had no
> > changelog and which had me sitting there wondering wtf it does, whether
> > we need it in 2.6.29, whether we need it in 2.6.28.x and earlier and me
> > not having the foggiest clue then getting grumpy.
> 
> Do you mean no changelog in the body of the e-mail here?  I'm assuming
> now that you mean no changelog in the body but when you say "no
> changelog" that reads differently.  I'm not saying the changelog was
> perfect here but your comments really do read like you felt there was
> nothing at all.


The text covering a patch should describe what the patch does, why it
does it, how it does it and it should describe the end-user effects of
not having the patch present.  Any and all of these can be skipped if
they are utterly obvious and unneeded.

Changes should be properly described, that's all.  The means by which
that is done isn't terribly important.  Sometimes most of the
description is in code comments, or in a newly-added Documentation/
file.

The reason I asked you personally to always send a changelog is because
I quite frequently sit there scratching my head at your patches not
having a clue what they do nor how to prioritise them.

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

[Theodore Tso]

On Mon, Mar 02, 2009 at 10:34:37AM -0800, Andrew Morton wrote:
> 
> The text covering a patch should describe what the patch does, why it
> does it, how it does it and it should describe the end-user effects of
> not having the patch present.  Any and all of these can be skipped if
> they are utterly obvious and unneeded.
> 
> Changes should be properly described, that's all.  The means by which
> that is done isn't terribly important.  Sometimes most of the
> description is in code comments, or in a newly-added Documentation/
> file.

My usual advise to folks is that if someone might be scratching their
head about why the code 3 months later, it probably does belong in the
code comments.  On the other hand, an explanation for why the previous
code was buggy probably should be in the commit description --- if it
isn't obvious.

An explanation for what the user might see when the bug gets hit is
also useful if after the fact someone is trying to see if a particular
bug has been fixed in mainline already, as is a pointer to the
bugzilla URL.

But if it's something as simple as "fix spelling mistake", or "handle
OOM condition gracefully", it may be that thing more than a single
one-line patch title is all that is necessary.

						- Ted

> The reason I asked you personally to always send a changelog is because
> I quite frequently sit there scratching my head at your patches not
> having a clue what they do nor how to prioritise them.

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

[Mark Brown]

On Mon, Mar 02, 2009 at 10:34:37AM -0800, Andrew Morton wrote:

> The reason I asked you personally to always send a changelog is because
> I quite frequently sit there scratching my head at your patches not
> having a clue what they do nor how to prioritise them.

Hrm.  Are you by any chance referring to the changelog as being *only*
the bit in the body of the e-mail?  If so that's half the confusion here
- in tools such as git the changelog also includes what ends up in the
subject of the e-mail.  This is what I was reading your comments (to
others as well, I wouldn't have bothered if it had just been me) as
referring to.

I'm not sure that simply supplying a body would help anything here, FWIW
- if I'm writing a body for the sake of it it'll generally just be
repetitive which isn't terribly constructive.  Obviously I do *try* to
write sensible changelogs and will keep making an effort to do so.  As
far as I remember most of the issues in the past have been due to
missing out something along the lines of "...because that's what the
silicon does" but ICBW.

As far as prioritisation goes I'd always expect to have to explicitly
call out anything other than merging via -next with no particular
urgency.

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

[Theodore Tso]

On Mon, Mar 02, 2009 at 07:19:11PM +0000, Mark Brown wrote:
> I'm not sure that simply supplying a body would help anything here, FWIW
> - if I'm writing a body for the sake of it it'll generally just be
> repetitive which isn't terribly constructive.  Obviously I do *try* to
> write sensible changelogs and will keep making an effort to do so.  As
> far as I remember most of the issues in the past have been due to
> missing out something along the lines of "...because that's what the
> silicon does" but ICBW.

If you mean stuff like this:

    commit a39a021fd73ce06aad8d1081ac711a36930e6cb8
    Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
    Date:   Wed Feb 4 21:10:58 2009 +0100

        mfd: Mark WM835x USB_SLV_500MA bit as accessible
        
        The code is out of sync with the silicon.
        
        Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
        Signed-off-by: Samuel Ortiz <sameo@openedhand.com>

What I wouldn't know, looking at the message, and what I suspect
Andrew is complaining about, is after reading the entire commit log,
"So what?"  Is it going to cause the the device to explode?  Will the
system crash?  Will the device go nonfunctional until you power cycle
it?  What will the user see before this patch is applied?

Maybe it's obvious to you, since you know the device, but it's not
obvious to the rest of us.   This in contrast to a commit log such as:

subsys: Add error checking to kmalloc() call

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

... where all good kernel developers should understand the
consequences of not checking the return value to kmalloc().  But do
you really expect most kernel developers to know what the heck a
USB_SLV_500MA bit means, and what it means for it to be "accessible"
versus "not accessible"?

   "Always write with a deep sympathy for the reader"
   	   	      	     Will Strunk, Elements of Style

		     	  	  	   - Ted

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

[Mark Brown]

On Mon, Mar 02, 2009 at 02:57:25PM -0500, Theodore Tso wrote:

> If you mean stuff like this:

>     commit a39a021fd73ce06aad8d1081ac711a36930e6cb8
>     Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
>     Date:   Wed Feb 4 21:10:58 2009 +0100
> 
>         mfd: Mark WM835x USB_SLV_500MA bit as accessible

>         The code is out of sync with the silicon.

> What I wouldn't know, looking at the message, and what I suspect
> Andrew is complaining about, is after reading the entire commit log,

No, and in fact that's a perfect example of what I'm talking about when
I say that simply writing a second paragraph for the commit logs is not
a helpful way to deal with the problem here (since it isn't the issue at
all).

As I've said on a number of occasions already in this thread I am not
trying to claim that my commit logs have always been the best or that
Andrew was complaining about nothing.  I'm perfectly well aware of the
sort of things that go into a good commit log, I read quite a lot of
them when reviewing patches myself, and do try to provide useful logs
even though I don't manage it all the time.

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

[Andy Whitcroft]

On Sat, Feb 28, 2009 at 07:18:29PM -0500, Theodore Tso wrote:
> On Sun, Mar 01, 2009 at 12:01:38AM +0100, Stefan Richter wrote:
> > > 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.
> 
> Worse yet, if we start getting these sorts of entries being returned
> by "git log":
> 
> ------------
> ext4: Fix spelling error: successfull
> 
> Fix spelling error: successfull
> 
> Signed-off-by: "Trivial Patch Submitter" <spelling@nits.org>
> ------------
> 
> Just to shut up checkpatch, I'm going to feel the urge to shake a
> checkpatch maintainer warmly by the throat.
> 
> Sometimes, all that is needed is:
> 
> ------------
> ext4: Fix spelling error: successfull
> 
> Signed-off-by: "Trivial Patch Submitter" <spelling@nits.org>
> ------------

Not wanting my throat felt warmly, and as there does not seem to be
a sensible way to detect poor changelogs I am not propsing to make any
change to checkpatch on this one.

-apw