|From:||Ingo Molnar <mingo-AT-elte.hu>|
|To:||Andrew Morton <akpm-AT-linux-foundation.org>|
|Subject:||Re: Fix quilt merge error in acpi-cpufreq.c|
|Date:||Wed, 15 Apr 2009 23:03:53 +0200|
|Cc:||Linus Torvalds <torvalds-AT-linux-foundation.org>, hpa-AT-zytor.com, tglx-AT-linutronix.de, rusty-AT-rustcorp.com.au, linux-kernel-AT-vger.kernel.org, davej-AT-redhat.com|
* Andrew Morton <firstname.lastname@example.org> wrote: > On Wed, 15 Apr 2009 12:43:02 -0700 (PDT) > Linus Torvalds <email@example.com> 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 <firstname.lastname@example.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 <email@example.com> | 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 <firstname.lastname@example.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 <email@example.com> | 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 <firstname.lastname@example.org> | 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
Copyright © 2009, Eklektix, Inc.
Comments and public postings are copyrighted by their creators.
Linux is a registered trademark of Linus Torvalds