Deprecate checkpatch.pl --file
PATCH 1/1 Deprecate checkpatch.pl --file
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.
scripts/checkpatch.pl +19 -1
@@ -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++;
scripts/checkpatch.pl if ($#ARGV < 0) { · line 73
Quoted 12 lines · Andi Kleen
scripts/checkpatch.pl +12
-0+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 [email protected] and not @osdl.org .. )
Daniel
Quoted 4 lines · Daniel Walker
On Tue, 2008-01-08 at 17:14 +0100, Andi Kleen wrote:
Quoted 16 lines
scripts/checkpatch.pl if ($#ARGV < 0) { · line 73
Quoted 12 lines
scripts/checkpatch.pl +12
-0+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
Quoted 22 lines · Sam Ravnborg
Quoted 4 lines · Daniel Walker
On Tue, 2008-01-08 at 17:14 +0100, Andi Kleen wrote:
Quoted 16 lines
scripts/checkpatch.pl if ($#ARGV < 0) { · line 73
Quoted 12 lines
scripts/checkpatch.pl +12
-0+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 ..
Quoted 4 lines
It is a simple pain/benefit issue.
Fixing the 25 errors and 13 warnings inkernel/profile.cmay 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..
Quoted 2 lines
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
Quoted 8 lines · Daniel Walker
Quoted 4 lines
It is a simple pain/benefit issue.
Fixing the 25 errors and 13 warnings inkernel/profile.cmay 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.ctho .. If I make a big mod tokernel/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
Quoted 10 lines · Theodore Tso
Quoted 8 lines · Daniel Walker
Quoted 4 lines
It is a simple pain/benefit issue.
Fixing the 25 errors and 13 warnings inkernel/profile.cmay 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.ctho .. If I make a big mod tokernel/profile.c, that will screw up anyone else who has patches for that file..
Quoted 5 lines
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
Quoted 6 lines
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
Quoted 2 lines
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
Quoted 2 lines · Sam Ravnborg
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
Quoted 5 lines · Daniel Walker
Quoted 2 lines
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
Quoted 4 lines · Theodore Tso
Quoted 3 lines · Daniel Walker
Quoted 2 lines
But is discourage the creation of pure clean-up patches because it may have a disturbing effect on several other peoples work.
Quoted 8 lines
Quoted 2 lines
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.
Quoted 8 lines
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
Quoted 5 lines · Sam Ravnborg
Quoted 4 lines · Daniel Walker
On Tue, 2008-01-08 at 17:14 +0100, Andi Kleen wrote:
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
scripts/checkpatch.pl +4
-0+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 mostQuoted 8 lines
scripts/checkpatch.pl if ($#ARGV < 0) { · line 73
Quoted 8 lines
scripts/checkpatch.pl +8
-0+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.
Quoted 3 lines
Quoted 3 lines
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.
Quoted 4 lines
It is a simple pain/benefit issue.
Fixing the 25 errors and 13 warnings inkernel/profile.cmay 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).
Quoted 2 lines
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