checkpatch.pl is a nice tool to find issues in patches.
Sadly this tool gets more and more abused by various people to create
style cleanups for source files within the kernel.
In order to deal with that bad habit let's remove the --file option
and bring checkpatch.pl back to its original purpose.
Suggested-by: NeilBrown <[email protected]>
Signed-off-by: Richard Weinberger <[email protected]>
---
scripts/checkpatch.pl | 34 ++++++++--------------------------
1 file changed, 8 insertions(+), 26 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 182be0f..41d2092 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -22,7 +22,6 @@ my $chk_patch = 1;
my $tst_only;
my $emacs = 0;
my $terse = 0;
-my $file = 0;
my $check = 0;
my $check_orig = 0;
my $summary = 1;
@@ -58,7 +57,6 @@ Options:
--patch treat FILE as patchfile (default)
--emacs emacs compile window format
--terse one line per report
- -f, --file treat FILE as regular source file
--subjective, --strict enable more subjective tests
--types TYPE(,TYPE2...) show only these comma separated message types
--ignore TYPE(,TYPE2...) ignore various comma separated message types
@@ -124,7 +122,6 @@ GetOptions(
'patch!' => \$chk_patch,
'emacs!' => \$emacs,
'terse!' => \$terse,
- 'f|file!' => \$file,
'subjective!' => \$check,
'strict!' => \$check,
'ignore=s' => \@ignore,
@@ -550,18 +547,13 @@ sub seed_camelcase_includes {
}
}
-$chk_signoff = 0 if ($file);
-
my @rawlines = ();
my @lines = ();
my @fixed = ();
my $vname;
for my $filename (@ARGV) {
my $FILE;
- if ($file) {
- open($FILE, '-|', "diff -u /dev/null $filename") ||
- die "$P: $filename: diff failed - $!\n";
- } elsif ($filename eq '-') {
+ if ($filename eq '-') {
open($FILE, '<&STDIN');
} else {
open($FILE, '<', "$filename") ||
@@ -1809,26 +1801,24 @@ sub process {
my $hunk_line = ($realcnt != 0);
#make up the handle for any error we report on this line
- $prefix = "$filename:$realline: " if ($emacs && $file);
- $prefix = "$filename:$linenr: " if ($emacs && !$file);
+ $prefix = "$filename:$linenr: " if ($emacs);
- $here = "#$linenr: " if (!$file);
- $here = "#$realline: " if ($file);
+ $here = "#$linenr: ";
my $found_file = 0;
# extract the filename as it passes
if ($line =~ /^diff --git.*?(\S+)$/) {
$realfile = $1;
- $realfile =~ s@^([^/]*)/@@ if (!$file);
+ $realfile =~ s@^([^/]*)/@@;
$in_commit_log = 0;
$found_file = 1;
} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
$realfile = $1;
- $realfile =~ s@^([^/]*)/@@ if (!$file);
+ $realfile =~ s@^([^/]*)/@@;
$in_commit_log = 0;
$p1_prefix = $1;
- if (!$file && $tree && $p1_prefix ne '' &&
+ if ($tree && $p1_prefix ne '' &&
-e "$root/$p1_prefix") {
WARN("PATCH_PREFIX",
"patch prefix '$p1_prefix' exists, appears to be a -p0 patch\n");
@@ -2040,7 +2030,6 @@ sub process {
$rawline =~ /\b51\s+Franklin\s+St/i) {
my $herevet = "$here\n" . cat_vet($rawline) . "\n";
my $msg_type = \&ERROR;
- $msg_type = \&CHK if ($file);
&{$msg_type}("FSF_MAILING_ADDRESS",
"Do not include the paragraph about writing to the Free Software Foundation's mailing address from the sample GPL notice. The FSF has changed addresses in the past, and may do so again. Linux already includes a copy of the GPL.\n" . $herevet)
}
@@ -3670,7 +3659,7 @@ sub process {
next if ($word !~ /[A-Z][a-z]|[a-z][A-Z]/);
if ($check) {
seed_camelcase_includes();
- if (!$file && !$camelcase_file_seeded) {
+ if (!$camelcase_file_seeded) {
seed_camelcase_file($realfile);
$camelcase_file_seeded = 1;
}
@@ -4760,14 +4749,7 @@ sub process {
or die "$P: Can't open $newfile for write\n";
foreach my $fixed_line (@fixed) {
$linecount++;
- if ($file) {
- if ($linecount > 3) {
- $fixed_line =~ s/^\+//;
- print $f $fixed_line. "\n";
- }
- } else {
- print $f $fixed_line . "\n";
- }
+ print $f $fixed_line . "\n";
}
close($f);
--
2.0.1
On Thu, Jul 17, 2014 at 05:34:28PM +0200, Richard Weinberger wrote:
> checkpatch.pl is a nice tool to find issues in patches.
> Sadly this tool gets more and more abused by various people to create
> style cleanups for source files within the kernel.
> In order to deal with that bad habit let's remove the --file option
> and bring checkpatch.pl back to its original purpose.
>
> Suggested-by: NeilBrown <[email protected]>
> Signed-off-by: Richard Weinberger <[email protected]>
Very good idea:
Acked-by: Borislav Petkov <[email protected]>
You could also make it do:
if ($file) {
print STDERR "Go find real bugs in the kernel instead!\n";
exit(-1);
}
if someone supplies --file on the cmdline.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Thu, 2014-07-17 at 17:34 +0200, Richard Weinberger wrote:
> checkpatch.pl is a nice tool to find issues in patches.
grep is a similar tool, just less automated.
> Sadly this tool gets more and more abused by various people to create
> style cleanups for source files within the kernel.
> In order to deal with that bad habit let's remove the --file option
> and bring checkpatch.pl back to its original purpose.
Any tool can be misused.
diff -urN /dev/null $file | ./scripts/checkpatch.pl -
does the same thing as --file so I don't see any real
fundamental difference.
I think it should really only be used in --file mode
on drivers/staging/.
I wouldn't mind adding some additional "yes, I really
mean to do this" cmd-line flag when it's used on any
file outside of staging with some additional warning
that "--file" is discouraged outside of staging when
it's not there.
But additional documentation only goes so far.
On Thu, Jul 17, 2014 at 08:51:23AM -0700, Joe Perches wrote:
> grep is a similar tool, just less automated.
And still, we don't want to encourage people with useless cleanups.
> I think it should really only be used in --file mode
> on drivers/staging/.
Also dumb idea - having to look at a single patch "correcting" a whole
file in staging without any explanation why it is done is not something
we want to encourage either.
People cleaning up staging should first understand what they're doing
and not listen to some script what to do.
> I wouldn't mind adding some additional "yes, I really
> mean to do this" cmd-line flag when it's used on any
> file outside of staging with some additional warning
> that "--file" is discouraged outside of staging when
> it's not there.
No, checkpatch should only check patches and not whole file. It is
wrong. Fullstop.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Thu, 2014-07-17 at 18:02 +0200, Borislav Petkov wrote:
> On Thu, Jul 17, 2014 at 08:51:23AM -0700, Joe Perches wrote:
> > grep is a similar tool, just less automated.
[]
> checkpatch should only check patches and not whole file.
> It is wrong. Fullstop.
<eh> No worries, we just disagree about the fullstop.
cheers, Joe
On Thu, 17 Jul 2014, Joe Perches wrote:
> On Thu, 2014-07-17 at 17:34 +0200, Richard Weinberger wrote:
> > checkpatch.pl is a nice tool to find issues in patches.
>
> grep is a similar tool, just less automated.
>
> > Sadly this tool gets more and more abused by various people to create
> > style cleanups for source files within the kernel.
> > In order to deal with that bad habit let's remove the --file option
> > and bring checkpatch.pl back to its original purpose.
>
> Any tool can be misused.
>
> diff -urN /dev/null $file | ./scripts/checkpatch.pl -
>
> does the same thing as --file so I don't see any real
> fundamental difference.
There is a fundamental difference:
Coming up with the above requires actual brain involvement.
./scripts/checkpatch.pl -f
not so much.
So, for Richards patch:
Acked-and-heartely-welcomed-by: Thomas Gleixner <[email protected]>
On 07/17/2014 08:34 AM, Richard Weinberger wrote:
> checkpatch.pl is a nice tool to find issues in patches.
> Sadly this tool gets more and more abused by various people to create
> style cleanups for source files within the kernel.
> In order to deal with that bad habit let's remove the --file option
> and bring checkpatch.pl back to its original purpose.
>
I have a number of problems with this patch.
First, 'abuse' is a relative term. It describes a use you
(and possibly many others) may find objectionable, but that
does not mean all uses are objectionable.
Second, just because something is abused doesn't mean it would be
a good idea to take it away. Many of the amendmends to the US
Constitution can be abused. Should freedom of speech be taken away
because it can be abused ? I hope not. Many drugs can be abused.
Should they be taken away ? I hope not. And so on.
Using your argument, you can take away anything that can be abused,
which is pretty much everything. Not a good idea.
Guenter
On Fri, Jul 18, 2014 at 12:29:37AM -0700, Guenter Roeck wrote:
> First, 'abuse' is a relative term. It describes a use you
> (and possibly many others) may find objectionable, but that
> does not mean all uses are objectionable.
Do you actually have a valid use case for keeping the cmdline switch ...
> Second, just because something is abused doesn't mean it would be
> a good idea to take it away. Many of the amendmends to the US
> Constitution can be abused. Should freedom of speech be taken away
> because it can be abused ?
... or do you feel political and want to do some waste-of-time debating
today just because its Friday?
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 07/18/2014 01:23 AM, Borislav Petkov wrote:
> On Fri, Jul 18, 2014 at 12:29:37AM -0700, Guenter Roeck wrote:
>> First, 'abuse' is a relative term. It describes a use you
>> (and possibly many others) may find objectionable, but that
>> does not mean all uses are objectionable.
>
> Do you actually have a valid use case for keeping the cmdline switch ...
>
I find it convenient to be able to check a new file before committing it
and creating a patch. Also, I find it convenient to be able use it to clean
up a file before I do heavy lifting with it. Yes, I understand the latter
is discouraged nowadays, and I would not use it anymore outside my scope
of responsibility unless specifically asked by the maintainer to do so,
but in such cases it helps me a lot to be able to address the cleanup
prior to the heavy lifting.
Guenter
Am 18.07.2014 15:37, schrieb Guenter Roeck:
> On 07/18/2014 01:23 AM, Borislav Petkov wrote:
>> On Fri, Jul 18, 2014 at 12:29:37AM -0700, Guenter Roeck wrote:
>>> First, 'abuse' is a relative term. It describes a use you
>>> (and possibly many others) may find objectionable, but that
>>> does not mean all uses are objectionable.
>>
>> Do you actually have a valid use case for keeping the cmdline switch ...
>>
>
> I find it convenient to be able to check a new file before committing it
> and creating a patch. Also, I find it convenient to be able use it to clean
> up a file before I do heavy lifting with it. Yes, I understand the latter
> is discouraged nowadays, and I would not use it anymore outside my scope
> of responsibility unless specifically asked by the maintainer to do so,
> but in such cases it helps me a lot to be able to address the cleanup
> prior to the heavy lifting.
As capable kernel hacker you can still use a command like:
diff -urN /dev/null $file | ./scripts/checkpatch.pl -
Thanks,
//richard
On Fri, Jul 18, 2014 at 03:46:29PM +0200, Richard Weinberger wrote:
> Am 18.07.2014 15:37, schrieb Guenter Roeck:
> > On 07/18/2014 01:23 AM, Borislav Petkov wrote:
> >> On Fri, Jul 18, 2014 at 12:29:37AM -0700, Guenter Roeck wrote:
> >>> First, 'abuse' is a relative term. It describes a use you
> >>> (and possibly many others) may find objectionable, but that
> >>> does not mean all uses are objectionable.
> >>
> >> Do you actually have a valid use case for keeping the cmdline switch ...
> >>
> >
> > I find it convenient to be able to check a new file before committing it
> > and creating a patch. Also, I find it convenient to be able use it to clean
> > up a file before I do heavy lifting with it. Yes, I understand the latter
> > is discouraged nowadays, and I would not use it anymore outside my scope
> > of responsibility unless specifically asked by the maintainer to do so,
> > but in such cases it helps me a lot to be able to address the cleanup
> > prior to the heavy lifting.
>
> As capable kernel hacker you can still use a command like:
> diff -urN /dev/null $file | ./scripts/checkpatch.pl -
>
Yes, that came up before. Or I can revert the patch locally, or keep an old
version of checkpatch around. You make me suffer because someone else abuses
the system. That someone will find other ways to abuse the system, such as
using the same approach, or annoy you with something else, such as searching
for "fixme" statements in the code or sending unhelpful emails about failing
builds. The one thing you will not accomplish is to solve the problem.
Guenter
On 07/17/2014 05:34 PM, Richard Weinberger wrote:
[...]
> In order to deal with that bad habit let's remove the --file option
> and bring checkpatch.pl back to its original purpose.
I don't think this is a good solution the problem and I'm not sure how
successful it will actually be at fixing the problem. How about instead
properly documenting that single line trivial style cleanup patches are not
really helpful and people should not send them? (Maybe add a check to
checkpatch.pl that emits a warning if the patch is a trivial style cleanup ;))
--file is a in my opinion useful option and at least I use it on a regular
basis.
- Lars
On Fri, Jul 18, 2014 at 06:56:12AM -0700, Guenter Roeck wrote:
> Yes, that came up before. Or I can revert the patch locally, or keep
> an old version of checkpatch around. You make me suffer
Come on, you suffer from a one-liner?! Script it or whatever. Puh-lease!
The thing is inviting lazy wankers from all over the place who refuse to
turn on their brains just so they get a stupid patch into the kernel.
Keeping --file for your convenience is causing much more trouble all
over the place. Kill it already.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Fri, Jul 18, 2014 at 03:46:29PM +0200, Richard Weinberger wrote:
> As capable kernel hacker you can still use a command like:
> diff -urN /dev/null $file | ./scripts/checkpatch.pl -
Or he could just use the existing -f flag. I really don't understand
why you're trying to educate people by taking their toy away. They'll
find another one in no time anyway.
On Fri, Jul 18, 2014 at 04:17:42PM +0200, Lars-Peter Clausen wrote:
> --file is a in my opinion useful option and at least I use it on a
> regular basis.
Use
diff -urN /dev/null $file | ./scripts/checkpatch.pl -
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Fri, Jul 18, 2014 at 07:21:14AM -0700, Christoph Hellwig wrote:
> Or he could just use the existing -f flag.
If you mean the -f flag to checkpatch, he's removing them both.
> I really don't understand why you're trying to educate people by
> taking their toy away. They'll find another one in no time anyway.
Maybe we should stop accepting patches created from running
checkpatch...?
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Fri, 2014-07-18 at 16:24 +0200, Borislav Petkov wrote:
> On Fri, Jul 18, 2014 at 04:17:42PM +0200, Lars-Peter Clausen wrote:
> > --file is a in my opinion useful option and at least I use it on a
> > regular basis.
[]
> diff -urN /dev/null $file | ./scripts/checkpatch.pl -
Using the one-liner above also makes it harder to
automate checkpatch neatening and avoid using Lindent:
Something like: https://lkml.org/lkml/2014/7/11/794
checkpatch --terse and --emacs options, useful for editing
lines where errors occur, no longer point to the line of
the file being scanned and use the stdin filename "-" and
the line # from stdin.
On 07/18/2014 07:27 AM, Borislav Petkov wrote:
> On Fri, Jul 18, 2014 at 07:21:14AM -0700, Christoph Hellwig wrote:
>> Or he could just use the existing -f flag.
>
> If you mean the -f flag to checkpatch, he's removing them both.
>
Two different 'he'.
>> I really don't understand why you're trying to educate people by
>> taking their toy away. They'll find another one in no time anyway.
>
> Maybe we should stop accepting patches created from running
> checkpatch...?
>
Thinking about it, Linux itself can be abused as well. We should
take it away to prevent that abuse from happening. That would
solve your checkpatch problem quite nicely and forever.
Guenter
On Fri, Jul 18, 2014 at 07:35:26AM -0700, Joe Perches wrote:
> Using the one-liner above also makes it harder to
> automate checkpatch neatening and avoid using Lindent:
automated checkpatch?? More idiocy... we want less, in case you've
forgotten.
<snip more pointless justification attempts>
Let me state it again:
The thing is called checkpatch and not lindent or checkfile or whatever.
It is supposed to check *patches* and *not* whole files.
If people still want to check whole files, they can do so. But patches
generated from checking whole files are completely ignored by most
maintainers and don't help anyone - not the person who wants to learn to
contribute properly to the kernel and not the maintainer on the other
end who cringes at the gazillion lines patch staring him in the face.
It is about time people understood that and dropped the senseless frenzy
of creating patches with less time spent on thinking about them than
preparing them.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Fri, Jul 18, 2014 at 07:43:57AM -0700, Guenter Roeck wrote:
> Thinking about it, Linux itself can be abused as well. We should
> take it away to prevent that abuse from happening.
Whatever floats your boat, dude.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Fri, 2014-07-18 at 16:49 +0200, Borislav Petkov wrote:
> On Fri, Jul 18, 2014 at 07:35:26AM -0700, Joe Perches wrote:
> > Using the one-liner above also makes it harder to
> > automate checkpatch neatening and avoid using Lindent:
>
> automated checkpatch?? More idiocy... we want less, in case you've
> forgotten.
"we" is a subset of "all of us".
> <snip more pointless justification attempts>
Those are not at all pointless if you intend to fix
any issue found by the tool.
I think your viewpoint is a bit tainted as you're the
same guy that wrote "fuck readability".
https://lkml.org/lkml/2013/11/19/116
> It is about time people understood that and dropped the senseless frenzy
> of creating patches with less time spent on thinking about them than
> preparing them.
I suggest you try writing some documentation
patches instead of acrid emails.
cheers, Joe
On Fri, Jul 18, 2014 at 07:57:13AM -0700, Joe Perches wrote:
> I think your viewpoint is a bit tainted as you're the
> same guy that wrote "fuck readability".
>
> https://lkml.org/lkml/2013/11/19/116
It seems you still haven't understood what I actually meant.
If you'd tried to understand what I mean as long as you've remembered
that mail, it might've dawned on ya, who knows...
> I suggest you try writing some documentation
> patches instead of acrid emails.
I suggest you finally try fixing some real bugs. Can ya? I bet you will
come up with some excuse again.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
Borislav Petkov <[email protected]> writes:
> No, checkpatch should only check patches and not whole file. It is
> wrong. Fullstop.
Please don't remove --file, I use it all the time when maintaining
ath6kl and ath10k. It's a lot easier in my workflow to test the whole
driver in one go than start testing individual patches.
--
Kalle Valo