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

[Ingo Molnar]

* Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:

> On Wed, 15 Apr 2009 12:43:02 -0700 (PDT)
> Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
| 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@xxxxxxxxxxxxxx>
| 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@xxxxxxxxxxxxxxxxxxxx>
| 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@xxxxxxxxxxxxxx>
| 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@xxxxxxxxxxxxxx>
| 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/