[PATCH] Deprecate checkpatch.pl --file

[PATCH] Deprecate checkpatch.pl --file

[Andi Kleen]

Deprecate checkpatch.pl --file mode; add warning; add --file-force

As discussed on linux-kernel checkpatch.pl only patches for whole
files have a significant cost for the maintenance process. 

Better such changes should be only
done together with other changes. Add a explicit warning about
this; deprecate --file and add a --file-force instead.

Signed-off-by: Andi Kleen <ak@suse.de>

Index: linux/scripts/checkpatch.pl
===================================================================
--- linux.orig/scripts/checkpatch.pl
+++ linux/scripts/checkpatch.pl
@@ -21,6 +21,7 @@ my $tst_type = 0;
 my $emacs = 0;
 my $terse = 0;
 my $file = 0;
+my $file_force = 0;
 my $check = 0;
 my $summary = 1;
 my $mailback = 0;
@@ -34,6 +35,7 @@ GetOptions(
 	'emacs!'	=> \$emacs,
 	'terse!'	=> \$terse,
 	'file!'		=> \$file,
+	'file-force!'	=> \$file_force,
 	'subjective!'	=> \$check,
 	'strict!'	=> \$check,
 	'root=s'	=> \$root,
@@ -50,12 +52,28 @@ if ($#ARGV < 0) {
 	print "         --no-tree    => run without a kernel tree\n";
 	print "         --terse      => one line per report\n";
 	print "         --emacs      => emacs compile window format\n";
-	print "         --file       => check a source file\n";
+	print "         --file       => check a source file (deprecated)\n";
+	print "		--file-force => check a source file\n";
 	print "         --strict     => enable more subjective tests\n";
 	print "         --root       => path to the kernel tree root\n";
 	exit(1);
 }
 
+if ($file) {
+	print <<EOL
+WARNING: Using --file mode. Please do not send patches to linux-kernel
+that change whole existing files if you did not significantly change most
+of the the file for other reasons anyways or just wrote the file newly
+from scratch. Pure code style patches have a significant cost in a
+quickly changing code base like Linux because they cause rejects
+with other changes.
+If you're sure you want to use whole file mode please use --file-force
+EOL
+;
+	exit(1);
+};
+$file = $file_force;
+
 if ($terse) {
 	$emacs = 1;
 	$quiet++;

Re: [PATCH] Deprecate checkpatch.pl --file

[Daniel Walker]

On Tue, 2008-01-08 at 17:14 +0100, Andi Kleen wrote:


> +if ($file) {
> +	print <<EOL
> +WARNING: Using --file mode. Please do not send patches to linux-kernel
> +that change whole existing files if you did not significantly change most
> +of the the file for other reasons anyways or just wrote the file newly
> +from scratch. Pure code style patches have a significant cost in a
> +quickly changing code base like Linux because they cause rejects
> +with other changes.
> +If you're sure you want to use whole file mode please use --file-force
> +EOL
> +;
> +	exit(1);

Can't say I like this too much .. It sounds like your telling people to
stop sending cleanup patches, which doesn't make much sense .. We want a
clean kernel .. Or at least I do ..

Linus and Andrew could slow acceptable of these patches, but I don't
think discouraging people from cleaning up code and submitting a patch
for that is the right thing to do..

(Btw, I changed to akpm@linux-foundation.org and not @osdl.org .. )

Daniel

Re: [PATCH] Deprecate checkpatch.pl --file

[Sam Ravnborg]

On Tue, Jan 08, 2008 at 08:50:33AM -0800, Daniel Walker wrote:
> 
> On Tue, 2008-01-08 at 17:14 +0100, Andi Kleen wrote:
> 
> 
> > +if ($file) {
> > +	print <<EOL
> > +WARNING: Using --file mode. Please do not send patches to linux-kernel
> > +that change whole existing files if you did not significantly change most
> > +of the the file for other reasons anyways or just wrote the file newly
> > +from scratch. Pure code style patches have a significant cost in a
> > +quickly changing code base like Linux because they cause rejects
> > +with other changes.
> > +If you're sure you want to use whole file mode please use --file-force
> > +EOL
> > +;
> > +	exit(1);
> 
> Can't say I like this too much .. It sounds like your telling people to
> stop sending cleanup patches, which doesn't make much sense .. We want a
> clean kernel .. Or at least I do ..

It is a simple pain/benefit issue.
Fixing the 25 errors and 13 warnings in kernel/profile.c may look
like an easy task but then we put additional burden on the 10 people
that have patches pending for this file.

It is the same reason why we do not run a 'kill-trailing-whitespace'
on the full kernel tree.

	Sam

Re: [PATCH] Deprecate checkpatch.pl --file

[Daniel Walker]

On Tue, 2008-01-08 at 18:53 +0100, Sam Ravnborg wrote:
> On Tue, Jan 08, 2008 at 08:50:33AM -0800, Daniel Walker wrote:
> > 
> > On Tue, 2008-01-08 at 17:14 +0100, Andi Kleen wrote:
> > 
> > 
> > > +if ($file) {
> > > +	print <<EOL
> > > +WARNING: Using --file mode. Please do not send patches to linux-kernel
> > > +that change whole existing files if you did not significantly change most
> > > +of the the file for other reasons anyways or just wrote the file newly
> > > +from scratch. Pure code style patches have a significant cost in a
> > > +quickly changing code base like Linux because they cause rejects
> > > +with other changes.
> > > +If you're sure you want to use whole file mode please use --file-force
> > > +EOL
> > > +;
> > > +	exit(1);
> > 
> > Can't say I like this too much .. It sounds like your telling people to
> > stop sending cleanup patches, which doesn't make much sense .. We want a
> > clean kernel .. Or at least I do ..
> 
> It is a simple pain/benefit issue.
> Fixing the 25 errors and 13 warnings in kernel/profile.c may look
> like an easy task but then we put additional burden on the 10 people
> that have patches pending for this file.

This goes for all patches on kernel/profile.c tho .. If I make a big mod
to kernel/profile.c, that will screw up anyone else who has patches for
that file..

> It is the same reason why we do not run a 'kill-trailing-whitespace'
> on the full kernel tree.

At least style clean ups at worst are file by file..

Daniel

Re: [PATCH] Deprecate checkpatch.pl --file

[Theodore Tso]

On Tue, Jan 08, 2008 at 10:01:19AM -0800, Daniel Walker wrote:
> > It is a simple pain/benefit issue.
> > Fixing the 25 errors and 13 warnings in kernel/profile.c may look
> > like an easy task but then we put additional burden on the 10 people
> > that have patches pending for this file.
> 
> This goes for all patches on kernel/profile.c tho .. If I make a big mod
> to kernel/profile.c, that will screw up anyone else who has patches for
> that file..

Obviously, but why make it worse?  And what's more important?  A
"clean tree" (especially when some of the things that checkpatch.pl
flag are arbitrary and Not All That Important), or wasting developers'
time invalidating potentially huge number of patches thanks to cleanup
patches?

							- Ted

Re: [PATCH] Deprecate checkpatch.pl --file

[Daniel Walker]

On Tue, 2008-01-08 at 13:20 -0500, Theodore Tso wrote:
> On Tue, Jan 08, 2008 at 10:01:19AM -0800, Daniel Walker wrote:
> > > It is a simple pain/benefit issue.
> > > Fixing the 25 errors and 13 warnings in kernel/profile.c may look
> > > like an easy task but then we put additional burden on the 10 people
> > > that have patches pending for this file.
> > 
> > This goes for all patches on kernel/profile.c tho .. If I make a big mod
> > to kernel/profile.c, that will screw up anyone else who has patches for
> > that file..
> 
> Obviously, but why make it worse?  And what's more important?  A
> "clean tree" (especially when some of the things that checkpatch.pl
> flag are arbitrary and Not All That Important), or wasting developers'
> time invalidating potentially huge number of patches thanks to cleanup
> patches?

What I was saying in my first email was we can throttle "patches"
arbitrarily (I think Andrew/Linus even have a merge period for these
types of patches) ..  So that's not the issue .. I feel style clean ups
are fundamentally good.. So what we don't want to do is discourage the
creation of good patches, which is what I think Andi is doing..

If there's something in checkpatch.pl that you think is "Not All That
Important", than send a patch to remove those cases and we can discuss
that directly.. I'm not saying the tool is perfect..

Daniel

Re: [PATCH] Deprecate checkpatch.pl --file

[Sam Ravnborg]

> 
> What I was saying in my first email was we can throttle "patches"
> arbitrarily (I think Andrew/Linus even have a merge period for these
> types of patches) ..  So that's not the issue .. I feel style clean ups
> are fundamentally good.. So what we don't want to do is discourage the
> creation of good patches, which is what I think Andi is doing..

The way I read this:
+WARNING: Using --file mode. Please do not send patches to linux-kernel
+that change whole existing files if you did not significantly change most
+of the the file for other reasons anyways or just wrote the file newly
+from scratch. Pure code style patches have a significant cost in a
+quickly changing code base like Linux because they cause rejects
+with other changes.
does not discourage the creation of good patches.

But is discourage the creation of pure clean-up patches because it
may have a disturbing effect on several other peoples work.

	Sam

Re: [PATCH] Deprecate checkpatch.pl --file

[Andi Kleen]

> But is discourage the creation of pure clean-up patches because it
> may have a disturbing effect on several other peoples work.

Exactly. Or rather it spells out when exactly pure cleanup patches are ok.

-Andi

Re: [PATCH] Deprecate checkpatch.pl --file

[Daniel Walker]

On Tue, 2008-01-08 at 20:21 +0100, Sam Ravnborg wrote:

> But is discourage the creation of pure clean-up patches because it
> may have a disturbing effect on several other peoples work.

pure clean ups are _good_ patches , are they not?

Daniel

Re: [PATCH] Deprecate checkpatch.pl --file

[Theodore Tso]

On Tue, Jan 08, 2008 at 12:19:44PM -0800, Daniel Walker wrote:
> > But is discourage the creation of pure clean-up patches because it
> > may have a disturbing effect on several other peoples work.
> 
> pure clean ups are _good_ patches , are they not?
> 

Not necessarily.  Whether or not it is requires common sense, and very
often we get enthusiastic new-comers (some of them with very weak C
programming skills :-) who might try to use checkpatch.pl.  So we
can't assume that they will know when a pure clean-up patch is a good
thing, and when it's a waste of everyone's time, including theirs.

That's why I think the warning is a good thing.  It makes it more
likely that this gets communicated to the enthusiastic, well-meaning,
newcomer.  Someone who is more experienced and who knows how to
determine whether some driver is ancient and not being worked on, and
hence a pure clean-up patch won't be screwing up other developers,
will know how to suppress the warning.  (OTOH, how important is it in
the grand scheem of things to create or apply a pure clean-up patch on
a patch that few people if any are looking at?)

					- Ted

Re: [PATCH] Deprecate checkpatch.pl --file

[Daniel Walker]

On Tue, 2008-01-08 at 16:14 -0500, Theodore Tso wrote:
> On Tue, Jan 08, 2008 at 12:19:44PM -0800, Daniel Walker wrote:
> > > But is discourage the creation of pure clean-up patches because it
> > > may have a disturbing effect on several other peoples work.
> > 
> > pure clean ups are _good_ patches , are they not?
> > 
> 
> Not necessarily.  Whether or not it is requires common sense, and very
> often we get enthusiastic new-comers (some of them with very weak C
> programming skills :-) who might try to use checkpatch.pl.  So we
> can't assume that they will know when a pure clean-up patch is a good
> thing, and when it's a waste of everyone's time, including theirs.

You can pretty much generalize this to many patches on many mailing
lists.. Someone submits a patch that they didn't think about, and ..
It's at least a learning experience for the person submitting the patch.
It does however seem a lot less likely someone will screw up a change
when checkpatch.pl actually tells you the line where the style problem
is, and what it is .. Even with "WARNING" and "ERROR" classifications.

> That's why I think the warning is a good thing.  It makes it more
> likely that this gets communicated to the enthusiastic, well-meaning,
> newcomer.  Someone who is more experienced and who knows how to
> determine whether some driver is ancient and not being worked on, and
> hence a pure clean-up patch won't be screwing up other developers,
> will know how to suppress the warning.  (OTOH, how important is it in
> the grand scheem of things to create or apply a pure clean-up patch on
> a patch that few people if any are looking at?)

There is a maintainer involved in all this .. The maintainer needs to
accept the patch prior to it "... screwing up other developers." Given a
sane maintainer, and a sane list review then why wouldn't a pure clean
up be accepted?

The warning message is actually an end run around the maintainer.. The
clean up never gets created, and the maintainer doesn't have the choice
to include it or not.. 

Daniel

Re: [PATCH] Deprecate checkpatch.pl --file

[Bartlomiej Zolnierkiewicz]

On Tuesday 08 January 2008, Sam Ravnborg wrote:
> On Tue, Jan 08, 2008 at 08:50:33AM -0800, Daniel Walker wrote:
> > 
> > On Tue, 2008-01-08 at 17:14 +0100, Andi Kleen wrote:
> > 
> > 
> > > +if ($file) {
> > > +	print <<EOL
> > > +WARNING: Using --file mode. Please do not send patches to linux-kernel
> > > +that change whole existing files if you did not significantly change most
> > > +of the the file for other reasons anyways or just wrote the file newly
> > > +from scratch. Pure code style patches have a significant cost in a
> > > +quickly changing code base like Linux because they cause rejects
> > > +with other changes.
> > > +If you're sure you want to use whole file mode please use --file-force
> > > +EOL
> > > +;
> > > +	exit(1);

I'm not against the patch (although --file to --file-force parameter change
seems unnecessary and the warning should be toned down) but I think that
cleanup patches have been demonized out of proportion recently on LKML.

> > Can't say I like this too much .. It sounds like your telling people to
> > stop sending cleanup patches, which doesn't make much sense .. We want a
> > clean kernel .. Or at least I do ..

Seconded, also if this is what people like to work on we shouldn't be trying
to stop them but point them into the direction where their work could be more
productive in "the big picture" context.

> It is a simple pain/benefit issue.
> Fixing the 25 errors and 13 warnings in kernel/profile.c may look
> like an easy task but then we put additional burden on the 10 people
> that have patches pending for this file.

OTOH there is a lot of old/legacy code that almost nobody is touching where
cleanup would be appreciated (it is quite likely that somebody will notice
and fix a bug or two in the process).

> It is the same reason why we do not run a 'kill-trailing-whitespace'
> on the full kernel tree.

Agreed but we sometimes accept per file whitespace cleanups.

The important thing here is "common sense".  Sending a big patch series
of "checkpatch.pl fixes" for kernel/* files is certainly not a good thing.
Core code has a lot of high-profile people looking over it so you will be
just interfering with their changes.

However if somebody do such fixes for some old/orphaned driver _and_ also
fix other things at the same time then everything is sweet from my POV. :)

Thanks,
Bart