Message-ID: <20080409131420.GL17915@shadowen.org>
Date:	Wed, 9 Apr 2008 14:14:20 +0100
From:	Andy Whitcroft <apw@...dowen.org>
To:	Andi Kleen <andi@...stfloor.org>
Cc:	Jan Engelhardt <jengelh@...putergmbh.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [patch] checkpatch: relax spacing and line length

On Wed, Apr 09, 2008 at 10:46:06AM +0200, Andi Kleen wrote:
> > Oh, and if people felt that the concensus was for something to be
> > implemented and that you are waiting for me to implement the change in
> > checkpatch; please say so.
> 
> Well at least I think the printk change is a good one to implement and there 
> wasn't much protest to it at least.

Ok.  will put this on my todo list.

> If you're looking for another change request I think dropping the trailing
> white space check would be good a idea because a lot of maintainers already
> handle that automatically and it is not really worth bothering the "leaf
> developers" with because that can be trivially automated.

I assume you are talking about git squashing bad line ends as it loads
the patches up.  I thought that this still triggered issues when loading
subsequent patches which relied on the context including those spaces.
Surely, this would lead to increased load on the maintainers as they
loaded those patches up?  Where the leaf maintainers use quilt would they
either not get seen or trigger failurs for him (dependant on whether he
has the option enabled), both bad outcomes.  Better to waste the time of
leaf contributers (such as myself) than any maintainers time, IMO.

> Also I still think --file needs to go, or at least be replaced with
> --file-force and warning. See the recent unpleasant incident it caused
> again, e.g.
> 
> http://thread.gmane.org/gmane.linux.kernel/656847/focus=657003
> and 
> http://thread.gmane.org/gmane.linux.kernel/656847/focus=657003

Ok, these are both the same link, so either you miss pasted one, or you
repeat to emphasise.

Reading over that thread there seems to be a few themes:

1) "don't send out 150 patches at once",
2) accepting checkpatch only changes is stupid, and
3) some maintainers do not like the style checkpatch recommends.

The first is really a general complaint against the originator, not really
a complaint about checkpatch.  Checkpatch may have prompted the changes but
that is not ultimatly its fault.  We may not like a huge pile of patches
dropped at once, but failure to recognise that is a failing in education,
not a tooling issue.  Also with the huge volumes on linux-kernel these
are hardly a major component of overall volumes?

The second is actually a matter for maintainers, either they will accept
checkpatch only changes or they won't.  Clearly Dave Miller and Andi
will not, clearly Ingo will.  Surly that is a matter for the Maintainer,
and not for the tool.

The third is about some maintainers wishing to use different style for
their parts of their tree.  That is their right, assuming those upstream
of them will accept their style.


There is a lot of polorised view on checkpatch.  But clearly there are
maintainers on both sides of the argument, those who find the tool
helps them and those who do not.  Maintainers who believe that they
should police the coding style, and find that the tool helps them with
that part of the process reducing their burden when reviewing submission.
Others who think its crap and would rather visually check style themselves,
or indeed ignore style.

On the --force-file, we tried making the warning occur in the tool itself
once before and that produced extremely vocal opposition, flame fests,
and general unhappyness, so I don't really see that flying here.  What I
do not really understand is why a simple maintainer rejection of such
patches would not work just as well, "I do not accept checkpatch style
only cleanups".  As things stand a chunk of those changes were picked up,
others implicitly rejected; lesson learned for the submitter surely.

I do feel that it may be helpful to update SubmittingPatches to mention
the problems introduced by style only churn, and (as we have a difference
of opinion) to recommend contacting the area Maintainer before embarking
on such things.  Something like the patch below perhaps?

-apw

=== 8< ===
add a note on the risks associated with wide ranging style cleanups

Add a NOTE to the SubmittingPatches guide pointing out the issues which
may occur when submitting wide ranging style cleanups, and pointing the
reader to the area maintainers.

Signed-off-by: Andy Whitcroft <apw@...dowen.org>
---
 Documentation/SubmittingPatches |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 08a1ed1..633b9cf 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -472,6 +472,12 @@ The checker reports at three levels:
 You should be able to justify all violations that remain in your
 patch.
 
+NOTE: updating entire files purely for style violations can cause
+significant issues for other contributers and maintainers by increasing
+collisions between patches.  It is therefore recommended you contact the
+area Maintainer to ensure these patches are likely to be accepted _before_
+starting work.
+
 
 
 2) #ifdefs are ugly

--
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/

Powered by blists - more mailing lists