Re: [GIT PULL] Squashfs pull request for 2.6.29

Ingo Molnar
2009-01-22T22:17:55Z

* Dave Jones <davej@redhat.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