Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1483511 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] checkpatch: Minimize checkpatch induced patches... Christian Borntraeger <borntraeger@de.ibm.com> - 2016-09-14 20:00 +0200
    Re: [PATCH] checkpatch: Minimize checkpatch induced patches... Joe Perches <joe@perches.com> - 2016-09-14 20:10 +0200
      Re: [PATCH] checkpatch: Minimize checkpatch induced patches... Christian Borntraeger <borntraeger@de.ibm.com> - 2016-09-14 20:20 +0200
        Re: [PATCH] checkpatch: Minimize checkpatch induced patches... Christian Borntraeger <borntraeger@de.ibm.com> - 2016-09-14 20:30 +0200
        Re: [PATCH] checkpatch: Minimize checkpatch induced patches... Joe Perches <joe@perches.com> - 2016-09-14 20:30 +0200
        Re: [PATCH] checkpatch: Minimize checkpatch induced patches... Greg KH <greg@kroah.com> - 2016-09-14 20:40 +0200
          Re: [PATCH] checkpatch: Minimize checkpatch induced patches... Christian Borntraeger <borntraeger@de.ibm.com> - 2016-09-14 21:00 +0200
            Re: [PATCH] checkpatch: Minimize checkpatch induced patches... Joe Perches <joe@perches.com> - 2016-09-14 21:20 +0200
          Re: [PATCH] checkpatch: Minimize checkpatch induced patches... Christian Borntraeger <borntraeger@de.ibm.com> - 2016-09-18 21:40 +0200
    Re: [PATCH] checkpatch: Minimize checkpatch induced patches... Josh Triplett <josh@joshtriplett.org> - 2016-09-15 02:00 +0200
      Re: [PATCH] checkpatch: Minimize checkpatch induced patches... Josh Triplett <josh@joshtriplett.org> - 2016-09-15 02:10 +0200
      Re: [PATCH] checkpatch: Minimize checkpatch induced patches... Joe Perches <joe@perches.com> - 2016-09-15 02:10 +0200

#1483511 — Re: [PATCH] checkpatch: Minimize checkpatch induced patches...

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";
> 

[toc] | [next] | [standalone]

#1483518

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.

[toc] | [prev] | [next] | [standalone]

#1483525

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.

[toc] | [prev] | [next] | [standalone]

#1483526

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? 

[toc] | [prev] | [next] | [standalone]

#1483528

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.

[toc] | [prev] | [next] | [standalone]

#1483534

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

[toc] | [prev] | [next] | [standalone]

#1483543

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.

[toc] | [prev] | [next] | [standalone]

#1483568

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.

[toc] | [prev] | [next] | [standalone]

#1485929

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

[toc] | [prev] | [next] | [standalone]

#1483773

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.

[toc] | [prev] | [next] | [standalone]

#1483779

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.

[toc] | [prev] | [next] | [standalone]

#1483780

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web