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).
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/
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.
On Mon, 02 Mar 2009 16:15:49 +0100 Stefan Richter <[email protected]> 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.
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.
On Mon, 2 Mar 2009 18:24:57 +0000 Mark Brown <[email protected]> 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.
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.
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.
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Samuel Ortiz <[email protected]>
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" <[email protected]>
... 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
On Mon, Mar 02, 2009 at 02:57:25PM -0500, Theodore Tso wrote:
> If you mean stuff like this:
> commit a39a021fd73ce06aad8d1081ac711a36930e6cb8
> Author: Mark Brown <[email protected]>
> 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.