Re: [PATCH] checkpatch: Minimize checkpatch induced patches...
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...
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.
Re: [PATCH] checkpatch: Minimize checkpatch induced patches...
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...
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 > > cc: Greg KH > > cc: Jonathan Corbet > > cc: Josh Triplett > > cc: Christian Borntraeger > > cc: Theodore Ts'o > > 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...
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...
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...
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...
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...
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...
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 Feel free to improve my sentence to something proper.
Re: [PATCH] checkpatch: Minimize checkpatch induced patches...
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...
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
> cc: Greg KH
> cc: Jonathan Corbet
> cc: Josh Triplett
> cc: Christian Borntraeger
> cc: Theodore Ts'o
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";
>
