[patch] checkpatch.pl: revert wrong --file message

[patch] checkpatch.pl: revert wrong --file message

[Ingo Molnar]

Revert the incorrect, 6-line "WARNING" message that "checkpatch.pl 
--file" started emitting since commit 13214adf738ab, which was merged 
yesterday:

    Andi Kleen (1):
              Introduce a warning when --file mode is used

The message warns against sending "pure code style patches":

   $ scripts/checkpatch.pl --file arch/x86/mm/init_64.c
   total: 0 errors, 0 warnings, 789 lines checked

   arch/x86/mm/init_64.c has no obvious style problems and is ready for submission.

   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.

this new "WARNING" is wrong, what it suggests could not be farther from 
the truth.

In the past few months we frequently mentioned checkpatch.pl --file to 
arch/x86 newbies and it's been a great source of cleanup patches and it 
has become an integral part of our workflow. Newbies should start with 
small baby steps, with trivial patches, they should learn to write clean 
code, they should learn how to interact with other Linux developers and 
then they'll evolve over time towards larger changes.

So this new checkpatch.pl --file message is not just incorrect, the 
change is outright _damaging_ to Linux and to our arch/x86 workflow in 
particular.

Sometimes cleanliness problems in files are so distracting that not even 
very apparent bugs are visible "at a glance". People change such files 
only if they really are forced to, and they bitrot all the time.

arch/x86 was particularly affected by this problem so we have decided to 
put an end to that and have started doing wide-scale cleanups, backed by 
checkpatch --file. We have learned how to deal with even large-scope 
cleanup patches, we've learned how to check their correctness via size 
comparisons and .o file md5 sums. We have learned how to port fixes back 
and forth across cleanups without much effort, how to avoid rejects, 
etc. We dont apply it rigidly, but checkpatch.pl is a constant and 
reliable background force that helps us constantly improve the quality 
of arch/x86.

And this method is working out really well for us.

While nothing beats the value of old-fashioned code review, i have also 
found that reviewing patches that are against clean files is _easier_, 
because the cleanliness problems and inconsistencies in a file do not 
act as a constant "information noise" that distract from the real 
purpose of source code: to map intent to functionality.

So i can only recommend checkpatch.pl to all Linux maintainers, and a 
scripts/checkpatch.pl --file output is also a particularly funny sight 
when one applies it to a file one has written a long time ago and which 
one was proud about how clean it was back then ;-)

Lastly, even if someone were to disagree about how relevant 
checkpatch.pl --file errors are, dealing with the resulting cleanups is 
a policy matter up to the current maintainers of the files in question. 
Emitting a thick "WARNING" message by default easily kills cleanups at 
their source, before maintainers even had a chance to express their 
wishes.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 scripts/checkpatch.pl |    9 ---------
 1 file changed, 9 deletions(-)

Index: linux/scripts/checkpatch.pl
===================================================================
--- linux.orig/scripts/checkpatch.pl
+++ linux/scripts/checkpatch.pl
@@ -1828,15 +1828,6 @@ sub process {
 		print "are false positives report them to the maintainer, see\n";
 		print "CHECKPATCH in MAINTAINERS.\n";
 	}
-	print <<EOL if ($file == 1 && $quiet == 0);
-
-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.
-EOL
 
 	return $clean;
 }

Re: [patch] checkpatch.pl: revert wrong --file message

[Andi Kleen]

> 
> In the past few months we frequently mentioned checkpatch.pl --file to 
> arch/x86 newbies and it's been a great source of cleanup patches and it 
> has become an integral part of our workflow. Newbies should start with 
> small baby steps, with trivial patches, they should learn to write clean 
> code, they should learn how to interact with other Linux developers and 
> then they'll evolve over time towards larger changes.

I found this doesn't work unfortunately. 

I actively worked with a few people who sent continuous streams of formatting
only checkpatch.pl patches in the last months trying to get them to graduate to 
more complex patches and found they always had to little C knowledge to actively 
contribute something actually useful to the kernel.

At the end I usually had to give them the honest advice "You need to learn
more C first, but I'm afraid the kernel is not the best place to learn C
because it is too unforgiving".

I'm all for actively recruiting new developers (and I think I did my fair 
share on that front), but trying to turn absolute C newbies into
kernel hackers short term just doesn't work. 

On the other hand I found that people who already know enough C and start 
hacking code directly do not really need the "white space only" stage.
They just start hacking code directly. They usually need some education
on how to properly send patches, but that can be always done with
real bug fixes or changes they did.

Out of that experience came the checkpatch.pl message.

-Andi

Re: [patch] checkpatch.pl: revert wrong --file message

[Thomas Gleixner]

On Fri, 15 Feb 2008, Andi Kleen wrote:
> > In the past few months we frequently mentioned checkpatch.pl --file to 
> > arch/x86 newbies and it's been a great source of cleanup patches and it 
> > has become an integral part of our workflow. Newbies should start with 
> > small baby steps, with trivial patches, they should learn to write clean 
> > code, they should learn how to interact with other Linux developers and 
> > then they'll evolve over time towards larger changes.
>
> On the other hand I found that people who already know enough C and start 
> hacking code directly do not really need the "white space only" stage.
> They just start hacking code directly. They usually need some education
> on how to properly send patches, but that can be always done with
> real bug fixes or changes they did.

People, who do cleanups - I'm not talking about running lindent here -
read through the code while they fix it up.

Actually they find bugs that way or at least come up with useful
questions about code which is not obvious in the first place.

Discouraging such cleanups with a pretty offensive warning is
counterproductive.

Thanks,

	tglx

Re: [patch] checkpatch.pl: revert wrong --file message

[Pekka Enberg]

Hi,

On Feb 16, 2008 12:18 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> People, who do cleanups - I'm not talking about running lindent here -
> read through the code while they fix it up.
>
> Actually they find bugs that way or at least come up with useful
> questions about code which is not obvious in the first place.
>
> Discouraging such cleanups with a pretty offensive warning is
> counterproductive.

Well, it's not just about cleanup patches submitted by "newbies". I
use checkpatch for development too and the warning is real PITA for
that.

Re: [patch] checkpatch.pl: revert wrong --file message

[Cyrill Gorcunov]

[Pekka Enberg - Sat, Feb 16, 2008 at 12:27:33PM +0200]
| Hi,
| 
| On Feb 16, 2008 12:18 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
| > People, who do cleanups - I'm not talking about running lindent here -
| > read through the code while they fix it up.
| >
| > Actually they find bugs that way or at least come up with useful
| > questions about code which is not obvious in the first place.
| >
| > Discouraging such cleanups with a pretty offensive warning is
| > counterproductive.
| 
| Well, it's not just about cleanup patches submitted by "newbies". I
| use checkpatch for development too and the warning is real PITA for
| that.
| 

As the one who sent such a cleanup sometime ago I think I've rights to
say my POV ;)

Benefits
--------

I think all developers would agree with me that to have a clean code is
quite good. Even removing absolutely useless 'spaces' is good. Ingo
wrote a lot about such a benefit month ago - and I think most (or even
all) of developers agree with him. Actually I've
(show-trailing-whitespace t) in my emacs settings - so every useless
space char makes me nerve - of course I could turn this setting off
and be happy but...

Dark side of space-removing-patches
-----------------------------------

The only problem could be - such a cleanup would break patches
*already* in maintainer queue. And from that side I really understand
Andi's complains about such patches.

		- Cyrill -

Re: [patch] checkpatch.pl: revert wrong --file message

[Andy Whitcroft]

On Sat, Feb 16, 2008 at 12:27:33PM +0200, Pekka Enberg wrote:
> Hi,
> 
> On Feb 16, 2008 12:18 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > People, who do cleanups - I'm not talking about running lindent here -
> > read through the code while they fix it up.
> >
> > Actually they find bugs that way or at least come up with useful
> > questions about code which is not obvious in the first place.
> >
> > Discouraging such cleanups with a pretty offensive warning is
> > counterproductive.
> 
> Well, it's not just about cleanup patches submitted by "newbies". I
> use checkpatch for development too and the warning is real PITA for
> that.

The warning is suppressed on -q as its a pain indeed if one is using it
to check files and you are not intending a single file cleanup.

Is the concensus the warning is useful, or unhelpful.

-apw

Re: [patch] checkpatch.pl: revert wrong --file message

[Thomas Gleixner]

On Sat, 16 Feb 2008, Andy Whitcroft wrote:
> On Sat, Feb 16, 2008 at 12:27:33PM +0200, Pekka Enberg wrote:
> > Hi,
> > 
> > On Feb 16, 2008 12:18 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > > People, who do cleanups - I'm not talking about running lindent here -
> > > read through the code while they fix it up.
> > >
> > > Actually they find bugs that way or at least come up with useful
> > > questions about code which is not obvious in the first place.
> > >
> > > Discouraging such cleanups with a pretty offensive warning is
> > > counterproductive.
> > 
> > Well, it's not just about cleanup patches submitted by "newbies". I
> > use checkpatch for development too and the warning is real PITA for
> > that.
> 
> The warning is suppressed on -q as its a pain indeed if one is using it
> to check files and you are not intending a single file cleanup.
> 
> Is the concensus the warning is useful, or unhelpful.

The warning is wrong and annoying. It reflects the personal opinion of
Andi and imposes it on everybody else. 

There was and is no consenus about the usefulness of such patches and
probably never will be. It's up to the maintainer of a particular
subsystem to accept or reject such patches.

It's definitely not the decision of a single kernel developer who has
his own definition of checkpatch.pl correctness:

<http://lkml.org/lkml/2008/1/4/98>

> Please run your patches through checkpatch.pl.
> 
> ERROR: use tabs not spaces
> #48: FILE: arch/x86/kernel/alternative.c:360:

I saw a lot of these warnings, but disregarded them as obviously
silly. I don't have plans to redo all the patches for that.

</http://lkml.org/lkml/2008/1/4/98>


Thanks,

	tglx

Re: [patch] checkpatch.pl: revert wrong --file message

[Andi Kleen]

On Saturday 16 February 2008 11:27:33 Pekka Enberg wrote:
> Hi,
> 
> On Feb 16, 2008 12:18 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > People, who do cleanups - I'm not talking about running lindent here -
> > read through the code while they fix it up.
> >
> > Actually they find bugs that way or at least come up with useful
> > questions about code which is not obvious in the first place.
> >
> > Discouraging such cleanups with a pretty offensive warning is
> > counterproductive.
> 
> Well, it's not just about cleanup patches submitted by "newbies". I
> use checkpatch for development too and the warning is real PITA for
> that.

Just use --file-force then. That will shut it up.

-Andi
 

Re: [patch] checkpatch.pl: revert wrong --file message

[Andi Kleen]

> People, who do cleanups - I'm not talking about running lindent here -
> read through the code while they fix it up.

Please feel free to repeat my little experiment: give someone who sends
you a lot of checkpatch.pl only patches a simple task that actually
requires a little actual code change and at least a little localized 
understanding of code. See how well they do.

In my case it was the "turn ->ioctl into ->unlocked_ioctl" task,
which was simple enough.

I found that there were a lot of useful unlocked_ioctl patches
in the end, but they all only came from people who hadn't submitted
any checkpatch.pl only patches before.

-Andi