[PATCH] checkpatch: Minimize checkpatch induced patches...

[PATCH] checkpatch: Minimize checkpatch induced patches...

[Joe Perches]

checkpatch can be a useful tool for patches.

It can be a much more controversial tool when used on files with the
-f option for style and whitespace changes for code that is relatively
stable, obsolete, or for maintained by specific individuals.

o By default, allow checkpatch to be used with the -f|--file option
  for files in drivers/staging/
o Add an undocumented --force command line option to be used together
  with the -f|--file option to scan any file

Signed-off-by: Joe Perches <joe@perches.com>
cc: Greg KH <greg@kroah.com>
cc: Jonathan Corbet <corbet@lwn.net>
cc: Josh Triplett <josh@joshtriplett.org>
cc: Christian Borntraeger <borntraeger@de.ibm.com>
cc: Theodore Ts'o <tytso@mit.edu>
---
 scripts/checkpatch.pl | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 0ef3d83..d998a61 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -27,6 +27,7 @@ my $emacs = 0;
 my $terse = 0;
 my $showfile = 0;
 my $file = 0;
+my $force = 0;
 my $git = 0;
 my %git_commits = ();
 my $check = 0;
@@ -188,6 +189,7 @@ GetOptions(
 	'terse!'	=> \$terse,
 	'showfile!'	=> \$showfile,
 	'f|file!'	=> \$file,
+	'force!'	=> \$force,
 	'g|git!'	=> \$git,
 	'subjective!'	=> \$check,
 	'strict!'	=> \$check,
@@ -893,6 +895,10 @@ if ($git) {
 my $vname;
 for my $filename (@ARGV) {
 	my $FILE;
+	if (!$force && $file && $filename !~ m@^drivers/staging/@) {
+		warn "$P: checking '$filename' is not supported\n";
+		next;
+	}
 	if ($git) {
 		open($FILE, '-|', "git format-patch -M --stdout -1 $filename") ||
 			die "$P: $filename: git format-patch failed - $!\n";
-- 
2.10.0.rc2.1.g053435c

Re: [PATCH] checkpatch: Minimize checkpatch induced patches...

[Christian Borntraeger]

On 09/14/2016 07:51 PM, Joe Perches wrote:
> checkpatch can be a useful tool for patches.
> 
> It can be a much more controversial tool when used on files with the
> -f option for style and whitespace changes for code that is relatively
> stable, obsolete, or for maintained by specific individuals.
> 
> o By default, allow checkpatch to be used with the -f|--file option
>   for files in drivers/staging/
> o Add an undocumented --force command line option to be used together
>   with the -f|--file option to scan any file
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> cc: Greg KH <greg@kroah.com>
> cc: Jonathan Corbet <corbet@lwn.net>
> cc: Josh Triplett <josh@joshtriplett.org>
> cc: Christian Borntraeger <borntraeger@de.ibm.com>
> cc: Theodore Ts'o <tytso@mit.edu>

This will certainly help to reduce the noise. On the other hand I remember Linus
saying something along the line that he does not like the -f parameter (and he
prefers to set this automatically). So while I like the approach I am not happy
enough to ack right now - still looking for a better alternative :-/
> ---
>  scripts/checkpatch.pl | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 0ef3d83..d998a61 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -27,6 +27,7 @@ my $emacs = 0;
>  my $terse = 0;
>  my $showfile = 0;
>  my $file = 0;
> +my $force = 0;
>  my $git = 0;
>  my %git_commits = ();
>  my $check = 0;
> @@ -188,6 +189,7 @@ GetOptions(
>  	'terse!'	=> \$terse,
>  	'showfile!'	=> \$showfile,
>  	'f|file!'	=> \$file,
> +	'force!'	=> \$force,
>  	'g|git!'	=> \$git,
>  	'subjective!'	=> \$check,
>  	'strict!'	=> \$check,
> @@ -893,6 +895,10 @@ if ($git) {
>  my $vname;
>  for my $filename (@ARGV) {
>  	my $FILE;
> +	if (!$force && $file && $filename !~ m@^drivers/staging/@) {
> +		warn "$P: checking '$filename' is not supported\n";
> +		next;
> +	}
>  	if ($git) {
>  		open($FILE, '-|', "git format-patch -M --stdout -1 $filename") ||
>  			die "$P: $filename: git format-patch failed - $!\n";
> 

Re: [PATCH] checkpatch: Minimize checkpatch induced patches...

[Joe Perches]

On Wed, 2016-09-14 at 19:56 +0200, Christian Borntraeger wrote:

> This will certainly help to reduce the noise. On the other hand I remember Linus
> saying something along the line that he does not like the -f parameter (and he
> prefers to set this automatically). So while I like the approach I am not happy
> enough to ack right now - still looking for a better alternative :-/

Linus likely hasn't used checkpatch in a decade or so.

Taste and judgment can't be scripted anyway.

Let me know if you find an alternative.

Re: [PATCH] checkpatch: Minimize checkpatch induced patches...

[Christian Borntraeger]

On 09/14/2016 08:06 PM, Joe Perches wrote:
> On Wed, 2016-09-14 at 19:56 +0200, Christian Borntraeger wrote:
> 
>> This will certainly help to reduce the noise. On the other hand I remember Linus
>> saying something along the line that he does not like the -f parameter (and he
>> prefers to set this automatically). So while I like the approach I am not happy
>> enough to ack right now - still looking for a better alternative :-/
> 
> Linus likely hasn't used checkpatch in a decade or so.
> 
> Taste and judgment can't be scripted anyway.
> 
> Let me know if you find an alternative.

You know what. 
with some additional writing like 
"Existing code outside staging is not supposed to be "fixed" to match checkpatch.
Please do not send checkpatch initiated patches for those files"
near the newly created warn

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

Feel free to improve my sentence to something proper.

Re: [PATCH] checkpatch: Minimize checkpatch induced patches...

[Joe Perches]

On Wed, 2016-09-14 at 20:16 +0200, Christian Borntraeger wrote:
> You know what. 
> with some additional writing like 
> "Existing code outside staging is not supposed to be "fixed" to match checkpatch.
> Please do not send checkpatch initiated patches for those files"
> near the newly created warn

That's not to my taste.
That should be in the Documentation tree somewhere.

Re: [PATCH] checkpatch: Minimize checkpatch induced patches...

[Christian Borntraeger]

On 09/14/2016 08:21 PM, Joe Perches wrote:
> On Wed, 2016-09-14 at 20:16 +0200, Christian Borntraeger wrote:
>> You know what. 
>> with some additional writing like 
>> "Existing code outside staging is not supposed to be "fixed" to match checkpatch.
>> Please do not send checkpatch initiated patches for those files"
>> near the newly created warn
> 
> That's not to my taste.
> That should be in the Documentation tree somewhere.

Fine with me. maybe in SubmitingPatches? 

Re: [PATCH] checkpatch: Minimize checkpatch induced patches...

[Greg KH]

On Wed, Sep 14, 2016 at 08:16:55PM +0200, Christian Borntraeger wrote:
> On 09/14/2016 08:06 PM, Joe Perches wrote:
> > On Wed, 2016-09-14 at 19:56 +0200, Christian Borntraeger wrote:
> > 
> >> This will certainly help to reduce the noise. On the other hand I remember Linus
> >> saying something along the line that he does not like the -f parameter (and he
> >> prefers to set this automatically). So while I like the approach I am not happy
> >> enough to ack right now - still looking for a better alternative :-/
> > 
> > Linus likely hasn't used checkpatch in a decade or so.
> > 
> > Taste and judgment can't be scripted anyway.
> > 
> > Let me know if you find an alternative.
> 
> You know what. 
> with some additional writing like 
> "Existing code outside staging is not supposed to be "fixed" to match checkpatch.
> Please do not send checkpatch initiated patches for those files"
> near the newly created warn

That's not true, I _WANT_ checkpatch cleanups for the portion of the
kernel I maintain.  It keeps the code correct, up to date, easier to
maintain, and in doing so, we have found real bugs over time.

So don't make a blanket statement like that please.  And I'd strongly
suggest you revisit your feelings about this for code you maintain,
unless you want it to bitrot and not get any new contributions or
contributors :)

thanks,

greg k-h

Re: [PATCH] checkpatch: Minimize checkpatch induced patches...

[Christian Borntraeger]

On 09/14/2016 08:33 PM, Greg KH wrote:
> On Wed, Sep 14, 2016 at 08:16:55PM +0200, Christian Borntraeger wrote:
>> On 09/14/2016 08:06 PM, Joe Perches wrote:
>>> On Wed, 2016-09-14 at 19:56 +0200, Christian Borntraeger wrote:
>>>
>>>> This will certainly help to reduce the noise. On the other hand I remember Linus
>>>> saying something along the line that he does not like the -f parameter (and he
>>>> prefers to set this automatically). So while I like the approach I am not happy
>>>> enough to ack right now - still looking for a better alternative :-/
>>>
>>> Linus likely hasn't used checkpatch in a decade or so.
>>>
>>> Taste and judgment can't be scripted anyway.
>>>
>>> Let me know if you find an alternative.
>>
>> You know what. 
>> with some additional writing like 
>> "Existing code outside staging is not supposed to be "fixed" to match checkpatch.
>> Please do not send checkpatch initiated patches for those files"
>> near the newly created warn
> 
> That's not true, I _WANT_ checkpatch cleanups for the portion of the
> kernel I maintain.  It keeps the code correct, up to date, easier to
> maintain, and in doing so, we have found real bugs over time.

Assuming that there are others with the same opinions that means that 
Joes patch is not the right solution?

> So don't make a blanket statement like that please.  And I'd strongly
> suggest you revisit your feelings about this for code you maintain,
> unless you want it to bitrot and not get any new contributions or
> contributors :)

Actually I am totally fine with valid checkpatch patches. (It is embarassing,
but I even applied a correct bugfix from Nick Krause).  On the other hand
I think that there are too many checkpatch or Codingstyle induced patches that
actually break code or make things worse.

Any better idea is certainly welcome.

Re: [PATCH] checkpatch: Minimize checkpatch induced patches...

[Joe Perches]

On Wed, 2016-09-14 at 20:54 +0200, Christian Borntraeger wrote:
> On 09/14/2016 08:33 PM, Greg KH wrote:
> > That's not true, I _WANT_ checkpatch cleanups for the portion of the
> > kernel I maintain.  It keeps the code correct, up to date, easier to
> > maintain, and in doing so, we have found real bugs over time.
> Assuming that there are others with the same opinions that means that 
> Joes patch is not the right solution?

The patch doesn't forbid that.

It just means any checkpatch induced change outside of
drivers/staging would generally be intentional and would not
commonly be submitted by inexperienced developers.

Re: [PATCH] checkpatch: Minimize checkpatch induced patches...

[Christian Borntraeger]

On 09/14/2016 08:33 PM, Greg KH wrote:
> On Wed, Sep 14, 2016 at 08:16:55PM +0200, Christian Borntraeger wrote:
>> On 09/14/2016 08:06 PM, Joe Perches wrote:
>>> On Wed, 2016-09-14 at 19:56 +0200, Christian Borntraeger wrote:
>>>
>>>> This will certainly help to reduce the noise. On the other hand I remember Linus
>>>> saying something along the line that he does not like the -f parameter (and he
>>>> prefers to set this automatically). So while I like the approach I am not happy
>>>> enough to ack right now - still looking for a better alternative :-/
>>>
>>> Linus likely hasn't used checkpatch in a decade or so.
>>>
>>> Taste and judgment can't be scripted anyway.
>>>
>>> Let me know if you find an alternative.
>>
>> You know what. 
>> with some additional writing like 
>> "Existing code outside staging is not supposed to be "fixed" to match checkpatch.
>> Please do not send checkpatch initiated patches for those files"
>> near the newly created warn
> 
> That's not true, I _WANT_ checkpatch cleanups for the portion of the
> kernel I maintain.  It keeps the code correct, up to date, easier to
> maintain, and in doing so, we have found real bugs over time.
> 
> So don't make a blanket statement like that please.  And I'd strongly
> suggest you revisit your feelings about this for code you maintain,
> unless you want it to bitrot and not get any new contributions or
> contributors :)
> 
> thanks,
> 
> greg k-h
> 

After some days of time going by I have to agree with you. It has never 
been a checkpatch patch that bothered me, instead only some (and rare) fruitless
discussions and ignorance of feedback of very few people made me "unhappy".
Certainly nothing where a "stiff-armed" checkpatch would help.

The other question might be still an interesting topic: The process of how to 
update CodingStyle and how detailed Codingstyle.

Christian

Re: [PATCH] checkpatch: Minimize checkpatch induced patches...

[Josh Triplett]

On Wed, Sep 14, 2016 at 07:56:55PM +0200, Christian Borntraeger wrote:
> On 09/14/2016 07:51 PM, Joe Perches wrote:
> > checkpatch can be a useful tool for patches.
> > 
> > It can be a much more controversial tool when used on files with the
> > -f option for style and whitespace changes for code that is relatively
> > stable, obsolete, or for maintained by specific individuals.
> > 
> > o By default, allow checkpatch to be used with the -f|--file option
> >   for files in drivers/staging/
> > o Add an undocumented --force command line option to be used together
> >   with the -f|--file option to scan any file
> > 
> > Signed-off-by: Joe Perches <joe@perches.com>
> > cc: Greg KH <greg@kroah.com>
> > cc: Jonathan Corbet <corbet@lwn.net>
> > cc: Josh Triplett <josh@joshtriplett.org>
> > cc: Christian Borntraeger <borntraeger@de.ibm.com>
> > cc: Theodore Ts'o <tytso@mit.edu>
> 
> This will certainly help to reduce the noise. On the other hand I remember Linus
> saying something along the line that he does not like the -f parameter (and he
> prefers to set this automatically). So while I like the approach I am not happy
> enough to ack right now - still looking for a better alternative :-/

This seems entirely compatible with autodetection.  If checkpatch
detects that it runs on a file rather than a patch, it can assume -f.
It can then apply this same logic to reject that if 1) in a kernel tree
and 2) running on a non-staging file and 3) not passed --force.

Re: [PATCH] checkpatch: Minimize checkpatch induced patches...

[Joe Perches]

On Wed, 2016-09-14 at 16:54 -0700, Josh Triplett wrote:
> On Wed, Sep 14, 2016 at 07:56:55PM +0200, Christian Borntraeger wrote:
> > On 09/14/2016 07:51 PM, Joe Perches wrote:
> > > checkpatch can be a useful tool for patches.
> > >
> > > It can be a much more controversial tool when used on files with the
> > > -f option for style and whitespace changes for code that is relatively
> > > stable, obsolete, or for maintained by specific individuals.
[]
> > This will certainly help to reduce the noise. On the other hand I remember Linus
> > saying something along the line that he does not like the -f parameter (and he
> > prefers to set this automatically). So while I like the approach I am not happy
> > enough to ack right now - still looking for a better alternative :-/

> This seems entirely compatible with autodetection.  If checkpatch
> detects that it runs on a file rather than a patch, it can assume -f.
> It can then apply this same logic to reject that if 1) in a kernel tree
> and 2) running on a non-staging file and 3) not passed --force.

checkpatch doesn't do autodetection and there's no real
need for it to do it either.  The reason is in the name.

get_maintainer does.

Re: [PATCH] checkpatch: Minimize checkpatch induced patches...

[Josh Triplett]

On Wed, Sep 14, 2016 at 05:05:09PM -0700, Joe Perches wrote:
> On Wed, 2016-09-14 at 16:54 -0700, Josh Triplett wrote:
> > On Wed, Sep 14, 2016 at 07:56:55PM +0200, Christian Borntraeger wrote:
> > > On 09/14/2016 07:51 PM, Joe Perches wrote:
> > > > checkpatch can be a useful tool for patches.
> > > >
> > > > It can be a much more controversial tool when used on files with the
> > > > -f option for style and whitespace changes for code that is relatively
> > > > stable, obsolete, or for maintained by specific individuals.
> []
> > > This will certainly help to reduce the noise. On the other hand I remember Linus
> > > saying something along the line that he does not like the -f parameter (and he
> > > prefers to set this automatically). So while I like the approach I am not happy
> > > enough to ack right now - still looking for a better alternative :-/
> 
> > This seems entirely compatible with autodetection.  If checkpatch
> > detects that it runs on a file rather than a patch, it can assume -f.
> > It can then apply this same logic to reject that if 1) in a kernel tree
> > and 2) running on a non-staging file and 3) not passed --force.
> 
> checkpatch doesn't do autodetection and there's no real
> need for it to do it either.  The reason is in the name.

I'm not suggesting that checkpatch *needs* to do autodetection,
just pointing out this this proposed change doesn't preclude any future
autodetection.