|From:||Ingo Molnar <mingo-AT-elte.hu>|
|To:||Dave Jones <davej-AT-redhat.com>, Greg KH <gregkh-AT-suse.de>, Geert Uytterhoeven <geert-AT-linux-m68k.org>, Andrew Morton <akpm-AT-linux-foundation.org>, =?iso-8859-1?Q?J=F6rn?= Engel <joern-AT-log|
|Subject:||Re: [GIT PULL] Squashfs pull request for 2.6.29|
|Date:||Thu, 22 Jan 2009 23:16:41 +0100|
* Dave Jones <email@example.com> wrote: > We've already demonstrated "look how much stuff we can merge" time and > time again, but no-one ever seems to have a proposal for how we increase > the amount of review code gets before it's merged. I think i agree with you in some ways (more review is always good), but there's a very important aspect where i think you are quite wrong: The first step towards increasing review activity is to increase the readability of the code - so that is _can_ be reviewed quickly and efficiently. But talking down such efforts with: > (and I mean real review here, not checkpatch & codingstyle crap) is a shoot-own-foot excercise. You are attacking the very foundation of proper high-level review. Yes, there can be bad types of syntactic cleanups but most of the efforts add real value to the code and should not be dismissed just because they stay on the local level. "real review" only becomes easy if the code _is reviewable to begin with_: if it is written in standard coding patterns where we almost sub-consciously recognize bad constructs and bad practices. I've seen it time and again that if the code is cleaned up visually, real review and real improvements follow eventually. It's a gradual process and you just cannot do "real review" efficiently without what you call the "checkpatch crap". In fact, i claim that doing "real review" on butt-ugly code is a waste of time and resources, and that it is also _harmful_. By doing real review on something that is not even right stylistically, you insert value into it. That way you _encourage_ that author of that ugly piece of code to contribute more code in the same fashion. You indirectly harm Linux that way because you encourage bad taste. I strongly support the notion that high-level review is only warranted on code that is reviewable and looks tasteful, and that code which doesnt meet basic style should not be merged at all. Code that 'looks good' can of course still be utter crap - but that crap is usually easily noticed, and often the crap portion does not permeate the whole code base. But if we have code that _does not even look good_ then crap can hide for a long time. (obscured by filth, so to speak.) I know plenty of in-tree examples of 5-10 years old code that has remained butt-ugly and unhackable for 5-10 years and which has an above-average proportion of bugs and regressions. Ingo
Copyright © 2009, Eklektix, Inc.
Comments and public postings are copyrighted by their creators.
Linux is a registered trademark of Linus Torvalds