* [RFC] checkpatch: add option to change warning return value. @ 2011-03-22 23:13 Sarah Sharp 2011-03-22 23:22 ` Sarah Sharp 2011-03-23 10:07 ` Johannes Weiner 0 siblings, 2 replies; 9+ messages in thread From: Sarah Sharp @ 2011-03-22 23:13 UTC (permalink / raw) To: Andy Whitcroft; +Cc: linux-kernel, James Bottomley There are a lot of warnings in checkpatch.pl that are rather subjective. For instance, when a line is 81 characters long, and the patch submitter is just cleaning up existing code, a maintainer may not care that checkpatch.pl warns about the over 80-character line. It should be up to the maintainer what warnings they want to pay attention to. Unfortunately, if you try to run checkpatch.pl as part of a git pre-commit hook and a patch in a series being applied by git-am fails checkpatch.pl, the whole process stops. At that point I usually end up just disabling that git hook, which means I don't see real errors in the patches. Add an option to checkpatch.pl to print the warnings, but not return an error code when the --lazy flag is present. (Anyone with a better name can pipe up, lazy just seemed to be the opposite to strict.) This allows the maintainer to view the warnings, but still allow the patch application process to continue. Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com> --- scripts/checkpatch.pl | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 4c0383d..4519b5b 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -15,6 +15,7 @@ my $V = '0.31'; use Getopt::Long qw(:config no_auto_abbrev); my $quiet = 0; +my $lazy = 0; my $tree = 1; my $chk_signoff = 1; my $chk_patch = 1; @@ -46,6 +47,7 @@ Options: --terse one line per report -f, --file treat FILE as regular source file --subjective, --strict enable more subjective tests + --lazy print warnings, but don't return an error condition --root=PATH PATH to the kernel tree root --no-summary suppress the per-file summary --mailback only produce a report in case of warnings/errors @@ -80,6 +82,7 @@ GetOptions( 'debug=s' => \%debug, 'test-only=s' => \$tst_only, + 'lazy+' => \$lazy, 'h|help' => \$help, 'version' => \$help ) or help(1); @@ -1096,6 +1099,9 @@ sub report { push(our @report, $line); + if ($lazy == 1) { + return 0; + } return 1; } sub report_dump { -- 1.7.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC] checkpatch: add option to change warning return value. 2011-03-22 23:13 [RFC] checkpatch: add option to change warning return value Sarah Sharp @ 2011-03-22 23:22 ` Sarah Sharp 2011-03-22 23:40 ` [RFC v2] Checkpatch: " Sarah Sharp 2011-03-23 1:41 ` [RFC] checkpatch: " Steven Rostedt 2011-03-23 10:07 ` Johannes Weiner 1 sibling, 2 replies; 9+ messages in thread From: Sarah Sharp @ 2011-03-22 23:22 UTC (permalink / raw) To: Andy Whitcroft; +Cc: linux-kernel, James Bottomley On Tue, Mar 22, 2011 at 04:13:10PM -0700, Sarah Sharp wrote: > There are a lot of warnings in checkpatch.pl that are rather subjective. > For instance, when a line is 81 characters long, and the patch submitter > is just cleaning up existing code, a maintainer may not care that > checkpatch.pl warns about the over 80-character line. It should be up to > the maintainer what warnings they want to pay attention to. > > Unfortunately, if you try to run checkpatch.pl as part of a git pre-commit > hook and a patch in a series being applied by git-am fails checkpatch.pl, > the whole process stops. At that point I usually end up just disabling > that git hook, which means I don't see real errors in the patches. > > Add an option to checkpatch.pl to print the warnings, but not return an > error code when the --lazy flag is present. (Anyone with a better name > can pipe up, lazy just seemed to be the opposite to strict.) This allows > the maintainer to view the warnings, but still allow the patch application > process to continue. BTW, I'm not a perl hacker, and I don't really understand the reporting code, so someone who knows it (Andy?) should make the correct patch. I didn't notice before sending, but right now the warning count reporting is bogus, although the warnings don't make the script return an error code: sarah@xanatos:~/git/kernels/xhci$ git am ~/Maildir.fetchmail/.to-apply/ Applying: USB: xhci - fix unsafe macro definitions total: 0 errors, 0 warnings, 21 lines checked Your patch has no obvious style problems and is ready for submission. Applying: USB: xhci: unsigned char never equals -1 total: 0 errors, 0 warnings, 55 lines checked Your patch has no obvious style problems and is ready for submission. Applying: USB: xhci: simplify logic of skipping missed isoc TDs WARNING: line over 80 characters #12: FILE: drivers/usb/host/xhci-ring.c:1669: + struct xhci_ring *ep_ring = xhci_dma_to_transfer_ring(ep, event->buffer); WARNING: line over 80 characters #136: FILE: drivers/usb/host/xhci-ring.c:1743: + struct xhci_ring *ep_ring = xhci_dma_to_transfer_ring(ep, event->buffer); WARNING: line over 80 characters #164: FILE: drivers/usb/host/xhci-ring.c:1770: + struct xhci_ring *ep_ring = xhci_dma_to_transfer_ring(ep, event->buffer); total: 0 errors, 0 warnings, 231 lines checked Your patch has no obvious style problems and is ready for submission. Applying: USB: fix formatting of SuperSpeed endpoints in /proc/bus/usb/devices total: 0 errors, 0 warnings, 28 lines checked Your patch has no obvious style problems and is ready for submission. Note that second-to-last patch should have said 3 warnings. So my patch needs to be changed, but I really do want this functionality. Sarah Sharp > Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com> > --- > scripts/checkpatch.pl | 6 ++++++ > 1 files changed, 6 insertions(+), 0 deletions(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 4c0383d..4519b5b 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -15,6 +15,7 @@ my $V = '0.31'; > use Getopt::Long qw(:config no_auto_abbrev); > > my $quiet = 0; > +my $lazy = 0; > my $tree = 1; > my $chk_signoff = 1; > my $chk_patch = 1; > @@ -46,6 +47,7 @@ Options: > --terse one line per report > -f, --file treat FILE as regular source file > --subjective, --strict enable more subjective tests > + --lazy print warnings, but don't return an error condition > --root=PATH PATH to the kernel tree root > --no-summary suppress the per-file summary > --mailback only produce a report in case of warnings/errors > @@ -80,6 +82,7 @@ GetOptions( > > 'debug=s' => \%debug, > 'test-only=s' => \$tst_only, > + 'lazy+' => \$lazy, > 'h|help' => \$help, > 'version' => \$help > ) or help(1); > @@ -1096,6 +1099,9 @@ sub report { > > push(our @report, $line); > > + if ($lazy == 1) { > + return 0; > + } > return 1; > } > sub report_dump { > -- > 1.7.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFC v2] Checkpatch: add option to change warning return value. 2011-03-22 23:22 ` Sarah Sharp @ 2011-03-22 23:40 ` Sarah Sharp 2011-03-23 1:41 ` [RFC] checkpatch: " Steven Rostedt 1 sibling, 0 replies; 9+ messages in thread From: Sarah Sharp @ 2011-03-22 23:40 UTC (permalink / raw) To: Andy Whitcroft; +Cc: linux-kernel, James Bottomley There are a lot of warnings in checkpatch.pl that are rather subjective. For instance, when a line is 81 characters long, and the patch submitter is just cleaning up existing code, a maintainer may not care that checkpatch.pl warns about the over 80-character line. It should be up to the maintainer what warnings they want to pay attention to. Unfortunately, if you try to run checkpatch.pl as part of a git pre-commit hook and a patch in a series being applied by git-am fails checkpatch.pl, the whole process stops. At that point I usually end up just disabling that git hook, which means I don't see real errors in the patches. Add an option to checkpatch.pl to print the warnings, but not return an error code when the --lazy flag is present. (Anyone with a better name can pipe up, lazy just seemed to be the opposite to strict.) This allows the maintainer to view the warnings, but still allow the patch application process to continue. Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com> --- Ok, I think this version works correctly. My git hook returns an error when the code to be submitted causes a checkpatch error, and doesn't return an error code (but still prints the warnings) when the code to be committed triggers checkpatch warnings, when --lazy is used. This version also changes the summary message to be unhappy when warnings are found. scripts/checkpatch.pl | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 4c0383d..ad2d423 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -15,6 +15,7 @@ my $V = '0.31'; use Getopt::Long qw(:config no_auto_abbrev); my $quiet = 0; +my $lazy = 0; my $tree = 1; my $chk_signoff = 1; my $chk_patch = 1; @@ -46,6 +47,7 @@ Options: --terse one line per report -f, --file treat FILE as regular source file --subjective, --strict enable more subjective tests + --lazy print warnings, but don't return an error condition --root=PATH PATH to the kernel tree root --no-summary suppress the per-file summary --mailback only produce a report in case of warnings/errors @@ -80,6 +82,7 @@ GetOptions( 'debug=s' => \%debug, 'test-only=s' => \$tst_only, + 'lazy+' => \$lazy, 'h|help' => \$help, 'version' => \$help ) or help(1); @@ -1109,7 +1112,9 @@ sub ERROR { } sub WARN { if (report("WARNING: $_[0]\n")) { - our $clean = 0; + if ($lazy == 0) { + our $clean = 0; + } our $cnt_warn++; } } @@ -2952,10 +2957,10 @@ sub process { } } - if ($clean == 1 && $quiet == 0) { + if ($clean == 1 && $quiet == 0 && ($lazy == 0 || $cnt_warn == 0)) { print "$vname has no obvious style problems and is ready for submission.\n" } - if ($clean == 0 && $quiet == 0) { + if (($clean == 0 && $quiet == 0) || ($lazy == 1 && $cnt_warn != 0)) { print "$vname has style problems, please review. If any of these errors\n"; print "are false positives report them to the maintainer, see\n"; print "CHECKPATCH in MAINTAINERS.\n"; -- 1.7.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC] checkpatch: add option to change warning return value. 2011-03-22 23:22 ` Sarah Sharp 2011-03-22 23:40 ` [RFC v2] Checkpatch: " Sarah Sharp @ 2011-03-23 1:41 ` Steven Rostedt 2011-03-24 15:40 ` Sarah Sharp 1 sibling, 1 reply; 9+ messages in thread From: Steven Rostedt @ 2011-03-23 1:41 UTC (permalink / raw) To: Sarah Sharp; +Cc: Andy Whitcroft, linux-kernel, James Bottomley On Tue, Mar 22, 2011 at 04:22:46PM -0700, Sarah Sharp wrote: > > BTW, I'm not a perl hacker, and I don't really understand the reporting code, > so someone who knows it (Andy?) should make the correct patch. I didn't notice > before sending, but right now the warning count reporting is bogus, although > the warnings don't make the script return an error code: > > > > Note that second-to-last patch should have said 3 warnings. So my patch needs > to be changed, but I really do want this functionality. > > > @@ -1096,6 +1099,9 @@ sub report { > > > > push(our @report, $line); > > > > + if ($lazy == 1) { > > + return 0; > > + } > > return 1; > > } > > sub report_dump { Instead of this hunk, have: diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 58848e3..54ec3d9 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -316,7 +316,7 @@ for my $filename (@ARGV) { } close($FILE); if (!process($filename)) { - $exit = 1; + $exit = 1 if (!$lazy); } @rawlines = (); @lines = (); -- Steve ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC] checkpatch: add option to change warning return value. 2011-03-23 1:41 ` [RFC] checkpatch: " Steven Rostedt @ 2011-03-24 15:40 ` Sarah Sharp 2011-03-24 18:23 ` Steven Rostedt 0 siblings, 1 reply; 9+ messages in thread From: Sarah Sharp @ 2011-03-24 15:40 UTC (permalink / raw) To: Steven Rostedt; +Cc: Andy Whitcroft, linux-kernel, James Bottomley On Tue, Mar 22, 2011 at 09:41:17PM -0400, Steven Rostedt wrote: > On Tue, Mar 22, 2011 at 04:22:46PM -0700, Sarah Sharp wrote: > > > @@ -1096,6 +1099,9 @@ sub report { > > > > > > push(our @report, $line); > > > > > > + if ($lazy == 1) { > > > + return 0; > > > + } > > > return 1; > > > } > > > sub report_dump { > > Instead of this hunk, have: > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 58848e3..54ec3d9 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -316,7 +316,7 @@ for my $filename (@ARGV) { > } > close($FILE); > if (!process($filename)) { > - $exit = 1; > + $exit = 1 if (!$lazy); > } > @rawlines = (); > @lines = (); Hmm, but isn't that going to skip returning an exit value if there's an error as well? I really do want it to fail if there's an error, just not if there's a warning. Sarah Sharp ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] checkpatch: add option to change warning return value. 2011-03-24 15:40 ` Sarah Sharp @ 2011-03-24 18:23 ` Steven Rostedt 2011-03-24 22:02 ` Sarah Sharp 0 siblings, 1 reply; 9+ messages in thread From: Steven Rostedt @ 2011-03-24 18:23 UTC (permalink / raw) To: Sarah Sharp; +Cc: Andy Whitcroft, linux-kernel, James Bottomley On Thu, 2011-03-24 at 08:40 -0700, Sarah Sharp wrote: > On Tue, Mar 22, 2011 at 09:41:17PM -0400, Steven Rostedt wrote: > > On Tue, Mar 22, 2011 at 04:22:46PM -0700, Sarah Sharp wrote: > > > > @@ -1096,6 +1099,9 @@ sub report { > > > > > > > > push(our @report, $line); > > > > > > > > + if ($lazy == 1) { > > > > + return 0; > > > > + } > > > > return 1; > > > > } > > > > sub report_dump { > > > > Instead of this hunk, have: > > > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > index 58848e3..54ec3d9 100755 > > --- a/scripts/checkpatch.pl > > +++ b/scripts/checkpatch.pl > > @@ -316,7 +316,7 @@ for my $filename (@ARGV) { > > } > > close($FILE); > > if (!process($filename)) { > > - $exit = 1; > > + $exit = 1 if (!$lazy); > > } > > @rawlines = (); > > @lines = (); > > Hmm, but isn't that going to skip returning an exit value if there's an > error as well? I really do want it to fail if there's an error, just > not if there's a warning. Ah, I was a bit confused as you had report return 0, which is used by both errors and warnings. Would you want something like this: diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 58848e3..40cbc2f 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -1109,7 +1109,7 @@ sub ERROR { } sub WARN { if (report("WARNING: $_[0]\n")) { - our $clean = 0; + our $clean = 0 if (!$lazy); our $cnt_warn++; } } -- Steve ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC] checkpatch: add option to change warning return value. 2011-03-24 18:23 ` Steven Rostedt @ 2011-03-24 22:02 ` Sarah Sharp 0 siblings, 0 replies; 9+ messages in thread From: Sarah Sharp @ 2011-03-24 22:02 UTC (permalink / raw) To: Steven Rostedt; +Cc: Andy Whitcroft, linux-kernel, James Bottomley On Thu, Mar 24, 2011 at 02:23:49PM -0400, Steven Rostedt wrote: > On Thu, 2011-03-24 at 08:40 -0700, Sarah Sharp wrote: > > On Tue, Mar 22, 2011 at 09:41:17PM -0400, Steven Rostedt wrote: > > > On Tue, Mar 22, 2011 at 04:22:46PM -0700, Sarah Sharp wrote: > > > > > @@ -1096,6 +1099,9 @@ sub report { > > > > > > > > > > push(our @report, $line); > > > > > > > > > > + if ($lazy == 1) { > > > > > + return 0; > > > > > + } > > > > > return 1; > > > > > } > > > > > sub report_dump { > > > > > > Instead of this hunk, have: > > > > > > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > > index 58848e3..54ec3d9 100755 > > > --- a/scripts/checkpatch.pl > > > +++ b/scripts/checkpatch.pl > > > @@ -316,7 +316,7 @@ for my $filename (@ARGV) { > > > } > > > close($FILE); > > > if (!process($filename)) { > > > - $exit = 1; > > > + $exit = 1 if (!$lazy); > > > } > > > @rawlines = (); > > > @lines = (); > > > > Hmm, but isn't that going to skip returning an exit value if there's an > > error as well? I really do want it to fail if there's an error, just > > not if there's a warning. > > Ah, I was a bit confused as you had report return 0, which is used by > both errors and warnings. > > Would you want something like this: > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 58848e3..40cbc2f 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -1109,7 +1109,7 @@ sub ERROR { > } > sub WARN { > if (report("WARNING: $_[0]\n")) { > - our $clean = 0; > + our $clean = 0 if (!$lazy); > our $cnt_warn++; > } > } Yes, that's basically what I did in the second revision of the patch, although I didn't have enough perl skillz to know about the if (!$lazy) syntax. Sarah Sharp ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] checkpatch: add option to change warning return value. 2011-03-22 23:13 [RFC] checkpatch: add option to change warning return value Sarah Sharp 2011-03-22 23:22 ` Sarah Sharp @ 2011-03-23 10:07 ` Johannes Weiner 2011-03-23 18:43 ` Sarah Sharp 1 sibling, 1 reply; 9+ messages in thread From: Johannes Weiner @ 2011-03-23 10:07 UTC (permalink / raw) To: Sarah Sharp; +Cc: Andy Whitcroft, linux-kernel, James Bottomley On Tue, Mar 22, 2011 at 04:13:10PM -0700, Sarah Sharp wrote: > There are a lot of warnings in checkpatch.pl that are rather subjective. > For instance, when a line is 81 characters long, and the patch submitter > is just cleaning up existing code, a maintainer may not care that > checkpatch.pl warns about the over 80-character line. It should be up to > the maintainer what warnings they want to pay attention to. > > Unfortunately, if you try to run checkpatch.pl as part of a git pre-commit > hook and a patch in a series being applied by git-am fails checkpatch.pl, > the whole process stops. At that point I usually end up just disabling > that git hook, which means I don't see real errors in the patches. > > Add an option to checkpatch.pl to print the warnings, but not return an > error code when the --lazy flag is present. (Anyone with a better name > can pipe up, lazy just seemed to be the opposite to strict.) This allows > the maintainer to view the warnings, but still allow the patch application > process to continue. I first thought you would add code that allowed turning only select errors into warnings. If you just want to have git continue in any case, why not append '|| true' to the line that invokes checkpatch in your hook script? Hannes ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] checkpatch: add option to change warning return value. 2011-03-23 10:07 ` Johannes Weiner @ 2011-03-23 18:43 ` Sarah Sharp 0 siblings, 0 replies; 9+ messages in thread From: Sarah Sharp @ 2011-03-23 18:43 UTC (permalink / raw) To: Johannes Weiner; +Cc: Andy Whitcroft, linux-kernel, James Bottomley On Wed, Mar 23, 2011 at 11:07:07AM +0100, Johannes Weiner wrote: > On Tue, Mar 22, 2011 at 04:13:10PM -0700, Sarah Sharp wrote: > > There are a lot of warnings in checkpatch.pl that are rather subjective. > > For instance, when a line is 81 characters long, and the patch submitter > > is just cleaning up existing code, a maintainer may not care that > > checkpatch.pl warns about the over 80-character line. It should be up to > > the maintainer what warnings they want to pay attention to. > > > > Unfortunately, if you try to run checkpatch.pl as part of a git pre-commit > > hook and a patch in a series being applied by git-am fails checkpatch.pl, > > the whole process stops. At that point I usually end up just disabling > > that git hook, which means I don't see real errors in the patches. > > > > Add an option to checkpatch.pl to print the warnings, but not return an > > error code when the --lazy flag is present. (Anyone with a better name > > can pipe up, lazy just seemed to be the opposite to strict.) This allows > > the maintainer to view the warnings, but still allow the patch application > > process to continue. > > I first thought you would add code that allowed turning only select > errors into warnings. > > If you just want to have git continue in any case, why not append > '|| true' to the line that invokes checkpatch in your hook script? Because I still want to stop the patch application process if a true error was found. I just don't want to stop the patch application process if just a warning was found. Sarah Sharp ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-03-24 22:02 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-03-22 23:13 [RFC] checkpatch: add option to change warning return value Sarah Sharp 2011-03-22 23:22 ` Sarah Sharp 2011-03-22 23:40 ` [RFC v2] Checkpatch: " Sarah Sharp 2011-03-23 1:41 ` [RFC] checkpatch: " Steven Rostedt 2011-03-24 15:40 ` Sarah Sharp 2011-03-24 18:23 ` Steven Rostedt 2011-03-24 22:02 ` Sarah Sharp 2011-03-23 10:07 ` Johannes Weiner 2011-03-23 18:43 ` Sarah Sharp
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox