Re: Fix quilt merge error in acpi-cpufreq.c

Ingo Molnar
2009-04-15T21:04:40Z

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 15 Apr 2009 12:43:02 -0700 (PDT)
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> >
> >
> > On Wed, 15 Apr 2009, H. Peter Anvin wrote:
> > >
> > > "cleanup" is indeed the most common, as it is intended to signify a
> > > trivial but nonzero code change. Whether or not it's *correct* is
> > > another matter. "build fix" is valid and proper use: it tells that it
> > > fixes a compilation error, which succinctly communicates both the
> > > priority of the fix and how it needs to be validated.
> >
> > Why would that be "proper use"?
> >
> > Dammit, if the "build fix" is not obvious from the rest of the commit
> > message, there's something wrong.
> >
> > And if it _is_ obvious, then the mechanical "Impact:" thing is pointless.
> >
> > In other words - in neither case does it actually help anything at all.
> > It's only distracting noise.
> >
>
> I'm getting quite a few Impact:s now and I must say that the
> Impact: line is always duplicative of the Subject:. Except in a
> few cases, and that's because the Subject: sucked.
>
> But I leave the Impact: lines in there because I'm nice.

I looked at the current .30 cycle up to latest -git and i found only
five commits out of 584 that had your signoff (most went upstream
via you) which also had Impact lines:

| commit 3d26dcf7679c5cc6c9f3b95ffdb2152fba2b7fae
| Author: Andi Kleen <andi@firstfloor.org>
| Date: Mon Apr 13 14:40:08 2009 -0700
|
| kernel/sys.c: clean up sys_shutdown exit path
|
| Impact: cleanup, fix
|
| Clean up sys_shutdown() exit path. Factor out common code. Return
| correct error code instead of always 0 on failure.

Impact line exposes wrong patch structure: cleanup should never be
mixed with fix.

Impact line is not duplicative of subject line - it correctly
mentions that there are side-effects. (sys_shutdown() changes its
return code)

The subject line is thus wrong.

| commit 0769c2981495c3d05429840d6fc7a1b5e26accaa
| Author: Andi Kleen <andi@firstfloor.org>
| Date: Mon Apr 13 14:39:56 2009 -0700
|
| asm-generic/siginfo.h: update NSIGTRAP definition
|
| Impact: (nearly) trivial

impact line somewhat atypical but correct - the patch is a cleanup
but might affect user-space.

Impact line is not duplicative of subject line.

| commit 6b44003e5ca66a3fffeb5bc90f40ada2c4340896
| Author: Andrew Morton <akpm@linux-foundation.org>
| Date: Thu Apr 9 09:50:37 2009 -0600
|
| work_on_cpu(): rewrite it to create a kernel thread on demand
|
| Impact: circular locking bugfix

Impact line is correct.

You wrote a fix - Rusty was nice in keeping your subject line and
enhanced the commit with a non-trivial impact line.

[ Btw., this commit also had an unintended impact: it caused a 10%
mysqld performance drop on certain systems. When this commit was
bisected to later on it was immediately obvious from the impact
line that this side-effect was not intended when the patch was
written. ]

Impact line is not duplicative of subject line.

| commit acdd052a285a7b4cc7da4fa7d34ef9fd0a67b2f8
| Author: Hannes Eder <hannes@hanneseder.net>
| Date: Tue Mar 31 15:23:50 2009 -0700
|
| init/main.c: fix sparse warnings: context imbalance
|
| Impact: Attribute function 'init_post' with __releases(...).

Impact line is incorrect (describes action not effect).

Impact line is not duplicative of subject line.

| commit ee99c71c59f897436ec65debb99372b3146f9985
| Author: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
| Date: Tue Mar 31 15:19:31 2009 -0700
|
| mm: introduce for_each_populated_zone() macro
|
| Impact: cleanup

Impact line is correct and appropriate.

Impact line is not duplicative of subject line.

So, here are my conclusions:

- only 0.85% of the commits you were involved with in this cycle had
an impact line.

- out of 5 cases, 4 had correct impact lines, despite _you_
admittedly not liking them and not caring about them. That's a
pretty good ratio IMO. Impact lines should be handled by the
maintainer. You should not pass them through - just erase them
please - just like you erase or change other parts of changelogs
you dont like.

- i one out of the 5 cases, you could have detected a slightly
suboptimal patch structure just based on the (truthful) impact
line. But you dont use the impact lines so you missed this detail.

- in one out of the 5 cases, the impact line was used later on
during bug analysis. _I_ remember having looked at that impact
line and saw that he performance regression was clearly not
anticipated. Could i have recovered that information in some other
way? Yes, i could have mailed you, or i could have made a guess
out of other context data. But the impact line told me for sure.

- i'm not sure on what you base your observation that the impact
lines you get are "always duplicative of the Subject:.". It wasnt
duplicative in any of the above cases.

So i'm puzzled really. This stuff is clearly useful to us every day,
it shows up in only 0.85% of your commits (because you let them
through - you could erase them) but clearly if both Linus and you
are against it what can we do? :-(

Ingo