|From:||Ingo Molnar <mingo-AT-elte.hu>|
|To:||Linus Torvalds <torvalds-AT-linux-foundation.org>, "H. Peter Anvin" <hpa-AT-zytor.com>, Thomas Gleixner <tglx-AT-linutronix.de>|
|Subject:||Re: Fix quilt merge error in acpi-cpufreq.c|
|Date:||Wed, 15 Apr 2009 18:26:27 +0200|
|Cc:||Rusty Russell <rusty-AT-rustcorp.com.au>, Linux Kernel Mailing List <linux-kernel-AT-vger.kernel.org>, Andrew Morton <akpm-AT-linux-foundation.org>, Dave Jones <davej-AT-redhat.com>|
* Linus Torvalds <email@example.com> wrote: > > Impact: clarify and extend confusing API > > And what the hell is up with these bogus "Impact:" things? Who > started doing that, and why? If your single-line explanation at > the top is not good enough, and your multi-line explanation isn't > clear enough, then you should fix the OTHER parts, not add that > _idiotic_ "Impact" statement. I got Rusty to use it so i'm to blame for this one. A number of developers/maintainers use it now and they find it useful in a number of circumstances, when used judiciously. We are using impact lines to judge "practical impact of a commit". The shorter (while still correct and expressive), the better. We are trying to use it in well-defined cases - but not always. Here it helped expose the bogosity of a patch more clearly: the intended impact of "clarifying a confusing API" was not met, and the commit became easier to flame. There's 6 different classes of uses of impact lines right now: 1) They force smaller patch submissions: it is hard to write a correct impact line for an overly complex, multi-purpose patch. Just try it in practice and you'll see. It is _much_ easier to write a correct impact line for a properly split up patch series. So instead of bitching with developers again and again, we asked frequent sinners to write proper impact lines. Voila, the patches became smaller: one patch, one main line of (intended) impact. It is a very nicely self-regulating process. 2) One other purpose of them is to have a ... managerial risk-at-a-glance view of a larger set of commits, post facto. For example: $ git log v2.6.29..v2.6.30-rc1 arch/x86/kernel/apic/ | \ grep Impact: | sort | uniq -c | sort -n 1 Impact: build fix 1 Impact: build fix, cleanup 1 Impact: cleanup, paranoia 1 Impact: cleanup, reduce memory usage for CONFIG_CPUMASK_OFFSTACK=y 1 Impact: cleanup, remove cpumask from stack 1 Impact: fix bug with irq-descriptor moving when logical flat 1 Impact: fix incorrect error message 1 Impact: fix possible race 1 Impact: fix spurious IRQs 1 Impact: get correct smp_affinity as user requested 1 Impact: interface augmentation (not yet used) 1 Impact: make kexec work with x2apic 1 Impact: optimize APIC IPI related barriers 1 Impact: simplification 10 Impact: cleanup Shows (at a glance) that we had 5-6 runtime problems (mostly misbehavior, not crashes) in the APIC code during the last development window. Or: $ git log v2.6.29..v2.6.30-rc1 kernel/sched.c | \ grep Impact: | sort | uniq -c | sort -n 1 Impact: cleanup, micro-optimization 1 Impact: cleanup, new schedstat ABI 1 Impact: fix boot crash 1 Impact: fix circular locking 1 Impact: fix function graph trace hang / drop pointless softirq on UP 1 Impact: fix to preempt trace triggering lockdep check_flag failure 1 Impact: more precise avg_overlap metric - better load-balancing 1 Impact: struct rq size optimization 2 Impact: micro-optimization 12 Impact: cleanup Shows that we had ~4 runtime problems (crashes or lockdep asserts) in the scheduler during the last development window. 3) "Risk judgement at a glance" cannot be done via other attributes of the commit: The subject lines are too important to be burdened with structured risk information, and they are also too specific and spread too much. But if you think we can add [fix crash] and [pure cleanup] tags to primary subject lines that would be even better ... We thought that such artificial risk attribute structure was an obvious non-starter, because it would depart from existing commit practices so much. The subject line also tends to mimic the patch-submission subject lines (so that it aligns with lkml discussions/submissions) and tends to be more detailed and differently structured - so it's a lot harder to extract only risk/impact information from it, at a glance. 4) We are also using "Impact: cleanup" as a special tag for commits that are supposed to have _zero_ side-effects. "cleanup" otherwise can be ambigious: it often covers restructuring / refactoring of code and it covers changes that have side-effects. We had several cases in the past when an "Impact: cleanup" patch was bisected to - and the bisector / bug reporter already knew it straight away that there was a misunderstanding about the impact of the commit, without having to fully read the patch. This is very useful when someone not versed in that particular code meets such a commit down the line. 5) Impact lines are also very useful during maintenance, when adding it to a patch that got submitted by others: if i mis-interpret the impact of a patch and add the wrong impact line, people point it out. This happened several times in the past, and it can be embarrasing - but it forces maintainer attention and honesty. 6) It forces developer honesty/disclosure as well: sometimes a commit log is too wishy-washy and instead of forcing people to rewrite full commit logs (often made difficult by language barriers - the intersection of people who can write good code an good documentation/commits in English is very small - and we shouldnt exclude good coders who are not able to write good documentation/commits) we ask them (or show them) impact lines. It also forces developers to _think_ about the full impact of patches. We often got bad patches because people clearly did not think through what they were trying to do. With an impact line it's much easier to argue whether a developer was fully aware of a given risk of a commit or not. All in one, the impact line standardizes risk/impact info in a compact form. I do think that 'risk' is an essential attribute of a commit. Is this scheme perfect? Clearly not - and we are still experimenting with exactly how to shape it better (you can see several small variants). But it's already pretty useful in the history too - not just while writing changes and queueing up commits. I guess your flame means we must stop using it. Oh well. The party was nice while it lasted ;-) Ingo
Copyright © 2009, Eklektix, Inc.
Comments and public postings are copyrighted by their creators.
Linux is a registered trademark of Linus Torvalds