linux-kernel - Re: [GIT PULL] Squashfs pull request for 2.6.29

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 22 Jan 2009 23:16:41 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Dave Jones <davej@...hat.com>, Greg KH <gregkh@...e.de>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Jörn Engel <joern@...fs.org>,
	David Brown <lkml@...idb.org>,
	Phil Oester <kernel@...uxace.com>,
	Kay Sievers <kay.sievers@...y.org>,
	Phillip Lougher <phillip@...gher.demon.co.uk>,
	Christoph Hellwig <hch@...radead.org>,
	torvalds@...ux-foundation.org, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org
Subject: Re: [GIT PULL] Squashfs pull request for 2.6.29

* Dave Jones <davej@...hat.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/