* [BUG] checkpatch: false positive for commits with quote characters
@ 2015-11-16 22:43 Brian Norris
2015-11-17 17:48 ` Joe Perches
0 siblings, 1 reply; 7+ messages in thread
From: Brian Norris @ 2015-11-16 22:43 UTC (permalink / raw)
To: Joe Perches, Andy Whitcroft; +Cc: linux-kernel
Hi,
What is the Blessed (TM) style for referencing commits that have quote
characters in their subject line? e.g., this commit:
commit 43163022927b6e7d202a7e6f939c3f392465494d
Author: Brian Norris <computersforpeace@gmail.com>
Date: Tue May 19 14:38:22 2015 -0700
mtd: m25p80: allow arbitrary OF matching for "jedec,spi-nor"
Checkpatch reports false positive errors like this:
ERROR: Please use git commit description style 'commit <12+ chars of
sha1> ("<title line>")'
when I try to reference it on this patch:
https://lkml.org/lkml/2015/11/16/826
I understand the double quoting is a little nasty to parse, but I think
that just means we should relax the regexes in checkpatch.pl. I could
try to patch myself, but I figured I'd just follow checkpatch's advice
instead:
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
Brian
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [BUG] checkpatch: false positive for commits with quote characters 2015-11-16 22:43 [BUG] checkpatch: false positive for commits with quote characters Brian Norris @ 2015-11-17 17:48 ` Joe Perches 2015-11-17 18:03 ` Brian Norris 0 siblings, 1 reply; 7+ messages in thread From: Joe Perches @ 2015-11-17 17:48 UTC (permalink / raw) To: Brian Norris, Andy Whitcroft; +Cc: linux-kernel On Mon, 2015-11-16 at 14:43 -0800, Brian Norris wrote: > Hi, > > What is the Blessed (TM) style for referencing commits that have quote > characters in their subject line? e.g., this commit: > > commit 43163022927b6e7d202a7e6f939c3f392465494d > Author: Brian Norris <computersforpeace@gmail.com> > Date: Tue May 19 14:38:22 2015 -0700 > > mtd: m25p80: allow arbitrary OF matching for "jedec,spi-nor" > > Checkpatch reports false positive errors like this: > > ERROR: Please use git commit description style 'commit <12+ chars of > sha1> ("")' Hi Brian. What version of checkpatch are you using? Using linux-next: $ git log --stat -p -1 --format=email 43163022927b6e7d202a7e6f939c3f392465494d | ./scripts/checkpatch.pl --strict - total: 0 errors, 0 warnings, 0 checks, 53 lines checked Your patch has no obvious style problems and is ready for submission. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG] checkpatch: false positive for commits with quote characters 2015-11-17 17:48 ` Joe Perches @ 2015-11-17 18:03 ` Brian Norris 2015-12-04 0:13 ` Brian Norris 0 siblings, 1 reply; 7+ messages in thread From: Brian Norris @ 2015-11-17 18:03 UTC (permalink / raw) To: Joe Perches; +Cc: Andy Whitcroft, linux-kernel On Tue, Nov 17, 2015 at 09:48:27AM -0800, Joe Perches wrote: > On Mon, 2015-11-16 at 14:43 -0800, Brian Norris wrote: > > Hi, > > > > What is the Blessed (TM) style for referencing commits that have quote > > characters in their subject line? e.g., this commit: > > > > commit 43163022927b6e7d202a7e6f939c3f392465494d > > Author: Brian Norris <computersforpeace@gmail.com> > > Date: Tue May 19 14:38:22 2015 -0700 > > > > mtd: m25p80: allow arbitrary OF matching for "jedec,spi-nor" > > > > Checkpatch reports false positive errors like this: > > > > ERROR: Please use git commit description style 'commit <12+ chars of > > sha1> ("")' > > Hi Brian. > > What version of checkpatch are you using? > > Using linux-next: > > $ git log --stat -p -1 --format=email 43163022927b6e7d202a7e6f939c3f392465494d | ./scripts/checkpatch.pl --strict - I was referring to running checkpatch on this: https://lkml.org/lkml/2015/11/16/826 which *referenced* commit 43163022927b6e7d202a7e6f939c3f392465494d. Sorry if that wasn't clear. See below, Brian $ curl http://patchwork.ozlabs.org/patch/545234/mbox/ | scripts/checkpatch.pl - % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 3741 0 3741 0 0 11525 0 --:--:-- --:--:-- --:--:-- 11510 ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'Commit 43163022927b ("mtd: m25p80: allow arbitrary OF matching for "jedec,spi-nor"")' #17: Commit 43163022927b ("mtd: m25p80: allow arbitrary OF matching for total: 1 errors, 0 warnings, 29 lines checked Your patch has style problems, please review. NOTE: Ignored message types: FILE_PATH_CHANGES NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG] checkpatch: false positive for commits with quote characters 2015-11-17 18:03 ` Brian Norris @ 2015-12-04 0:13 ` Brian Norris 2015-12-04 0:29 ` Joe Perches 0 siblings, 1 reply; 7+ messages in thread From: Brian Norris @ 2015-12-04 0:13 UTC (permalink / raw) To: Joe Perches; +Cc: Andy Whitcroft, linux-kernel Ping? I've hit some different false positives today on the same rule. I'll stop bothering to report them if no one cares. On Tue, Nov 17, 2015 at 10:03:36AM -0800, Brian Norris wrote: > On Tue, Nov 17, 2015 at 09:48:27AM -0800, Joe Perches wrote: > > On Mon, 2015-11-16 at 14:43 -0800, Brian Norris wrote: > > > Hi, > > > > > > What is the Blessed (TM) style for referencing commits that have quote > > > characters in their subject line? e.g., this commit: > > > > > > commit 43163022927b6e7d202a7e6f939c3f392465494d > > > Author: Brian Norris <computersforpeace@gmail.com> > > > Date: Tue May 19 14:38:22 2015 -0700 > > > > > > mtd: m25p80: allow arbitrary OF matching for "jedec,spi-nor" > > > > > > Checkpatch reports false positive errors like this: > > > > > > ERROR: Please use git commit description style 'commit <12+ chars of > > > sha1> ("")' > > > > Hi Brian. > > > > What version of checkpatch are you using? > > > > Using linux-next: > > > > $ git log --stat -p -1 --format=email 43163022927b6e7d202a7e6f939c3f392465494d | ./scripts/checkpatch.pl --strict - > > I was referring to running checkpatch on this: > > https://lkml.org/lkml/2015/11/16/826 > > which *referenced* commit 43163022927b6e7d202a7e6f939c3f392465494d. > Sorry if that wasn't clear. > > See below, > Brian > > $ curl http://patchwork.ozlabs.org/patch/545234/mbox/ | scripts/checkpatch.pl - > % Total % Received % Xferd Average Speed Time Time Time Current > Dload Upload Total Spent Left Speed > 100 3741 0 3741 0 0 11525 0 --:--:-- --:--:-- --:--:-- 11510 > ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'Commit 43163022927b ("mtd: m25p80: allow arbitrary OF matching for "jedec,spi-nor"")' > #17: > Commit 43163022927b ("mtd: m25p80: allow arbitrary OF matching for > > total: 1 errors, 0 warnings, 29 lines checked > > Your patch has style problems, please review. > > NOTE: Ignored message types: FILE_PATH_CHANGES > > NOTE: If any of the errors are false positives, please report > them to the maintainer, see CHECKPATCH in MAINTAINERS. > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG] checkpatch: false positive for commits with quote characters 2015-12-04 0:13 ` Brian Norris @ 2015-12-04 0:29 ` Joe Perches 2015-12-04 0:36 ` Joe Perches 0 siblings, 1 reply; 7+ messages in thread From: Joe Perches @ 2015-12-04 0:29 UTC (permalink / raw) To: Brian Norris; +Cc: Andy Whitcroft, linux-kernel On Thu, 2015-12-03 at 16:13 -0800, Brian Norris wrote: > Ping? I've hit some different false positives today on the same rule. > I'll stop bothering to report them if no one cares. Perhaps this: diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 9f0949b..196b77b 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2397,22 +2397,26 @@ sub process { $long = 1 if ($line =~ /\bcommit\s+[0-9a-f]{41,}/i); $space = 0 if ($line =~ /\bcommit [0-9a-f]/i); $case = 0 if ($line =~ /\b[Cc]ommit\s+[0-9a-f]{5,40}[^A-F]/); - if ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)"\)/i) { + if ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("(.*)"\)/i) { $orig_desc = $1; $hasparens = 1; + print("here1\n"); } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s*$/i && defined $rawlines[$linenr] && - $rawlines[$linenr] =~ /^\s*\("([^"]+)"\)/) { + $rawlines[$linenr] =~ /^\s*\("(.*)"\)/) { $orig_desc = $1; $hasparens = 1; + print("here2\n"); } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("[^"]+$/i && defined $rawlines[$linenr] && - $rawlines[$linenr] =~ /^\s*[^"]+"\)/) { + $rawlines[$linenr] =~ /^\s*.*"\)/) { $line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)$/i; $orig_desc = $1; - $rawlines[$linenr] =~ /^\s*([^"]+)"\)/; + $rawlines[$linenr] =~ /^\s*(.*)"\)/; $orig_desc .= " " . $1; $hasparens = 1; + print("orig_desc: <$orig_desc>\n"); + print("here3\n"); } ($id, $description) = git_commit_info($orig_commit, ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [BUG] checkpatch: false positive for commits with quote characters 2015-12-04 0:29 ` Joe Perches @ 2015-12-04 0:36 ` Joe Perches 2015-12-04 1:39 ` Brian Norris 0 siblings, 1 reply; 7+ messages in thread From: Joe Perches @ 2015-12-04 0:36 UTC (permalink / raw) To: Brian Norris; +Cc: Andy Whitcroft, linux-kernel On Thu, 2015-12-03 at 16:29 -0800, Joe Perches wrote: > On Thu, 2015-12-03 at 16:13 -0800, Brian Norris wrote: > > Ping? I've hit some different false positives today on the same rule. > > I'll stop bothering to report them if no one cares. > > Perhaps this: (minus the debugging this time...) --- scripts/checkpatch.pl | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index d4960f7..b23dff8 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2397,20 +2397,20 @@ sub process { $long = 1 if ($line =~ /\bcommit\s+[0-9a-f]{41,}/i); $space = 0 if ($line =~ /\bcommit [0-9a-f]/i); $case = 0 if ($line =~ /\b[Cc]ommit\s+[0-9a-f]{5,40}[^A-F]/); - if ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)"\)/i) { + if ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("(.*)"\)/i) { $orig_desc = $1; $hasparens = 1; } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s*$/i && defined $rawlines[$linenr] && - $rawlines[$linenr] =~ /^\s*\("([^"]+)"\)/) { + $rawlines[$linenr] =~ /^\s*\("(.*)"\)/) { $orig_desc = $1; $hasparens = 1; } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("[^"]+$/i && defined $rawlines[$linenr] && - $rawlines[$linenr] =~ /^\s*[^"]+"\)/) { + $rawlines[$linenr] =~ /^\s*.*"\)/) { $line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)$/i; $orig_desc = $1; - $rawlines[$linenr] =~ /^\s*([^"]+)"\)/; + $rawlines[$linenr] =~ /^\s*(.*)"\)/; $orig_desc .= " " . $1; $hasparens = 1; } ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [BUG] checkpatch: false positive for commits with quote characters 2015-12-04 0:36 ` Joe Perches @ 2015-12-04 1:39 ` Brian Norris 0 siblings, 0 replies; 7+ messages in thread From: Brian Norris @ 2015-12-04 1:39 UTC (permalink / raw) To: Joe Perches; +Cc: Andy Whitcroft, linux-kernel On Thu, Dec 03, 2015 at 04:36:28PM -0800, Joe Perches wrote: > On Thu, 2015-12-03 at 16:29 -0800, Joe Perches wrote: > > On Thu, 2015-12-03 at 16:13 -0800, Brian Norris wrote: > > > Ping? I've hit some different false positives today on the same rule. > > > I'll stop bothering to report them if no one cares. > > > > Perhaps this: Aside from the non-breaking spaces your copy of Evolution inserted, it's an improvement. (It doesn't give a false positive for the patch I reported.) But it's still got some holes. > (minus the debugging this time...) > --- > scripts/checkpatch.pl | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index d4960f7..b23dff8 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -2397,20 +2397,20 @@ sub process { > $long = 1 if ($line =~ /\bcommit\s+[0-9a-f]{41,}/i); > $space = 0 if ($line =~ /\bcommit [0-9a-f]/i); > $case = 0 if ($line =~ /\b[Cc]ommit\s+[0-9a-f]{5,40}[^A-F]/); > - if ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)"\)/i) { > + if ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("(.*)"\)/i) { You're got the reverse problem now. This will match too broadly. For instance, if there is another instance of terminating quote + parentheses on the same line, then we'll get a false postive. e.g., if I wrote a patch that included something like this: Commit 6dc0dcde406b ("parisc: Use platform_device_register_simple("rtc-generic")") is cool (as in "cool") Or even if it's wrapped like this: Commit 6dc0dcde406b ("parisc: Use platform_device_register_simple("rtc-generic")") is cool (as in "cool") Then the regexes will think the description was: parisc: Use platform_device_register_simple("rtc-generic")") is cool (as in "cool A hacky workaround for that one: only check for (loosely, not proper regex syntax): parsed_description =~ description . ")"; rather than: description == parsed_description Not sure how far you want to go on chasing false positives... > $orig_desc = $1; > $hasparens = 1; > } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s*$/i && > defined $rawlines[$linenr] && > - $rawlines[$linenr] =~ /^\s*\("([^"]+)"\)/) { > + $rawlines[$linenr] =~ /^\s*\("(.*)"\)/) { > $orig_desc = $1; > $hasparens = 1; > } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("[^"]+$/i && > defined $rawlines[$linenr] && > - $rawlines[$linenr] =~ /^\s*[^"]+"\)/) { > + $rawlines[$linenr] =~ /^\s*.*"\)/) { > $line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)$/i; > $orig_desc = $1; > - $rawlines[$linenr] =~ /^\s*([^"]+)"\)/; > + $rawlines[$linenr] =~ /^\s*(.*)"\)/; > $orig_desc .= " " . $1; > $hasparens = 1; > } BTW, another false positive: just including this text in a commit triggers a different one: http://lkml.kernel.org/g/20151113220039.GA74382@google.com A simple hack (appended, in addition to yours) would be to assume that when people are trying to include badly-formatted commit hashes, they will be preceding them with whitespace, the beginning of a line, or encapsulating punctuation. Or use a URL parser. Brian diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index d2993f19df3f..e7110ba3242b 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2349,7 +2349,7 @@ sub process { # Check for git id commit length and improperly formed commit descriptions if ($in_commit_log && !$commit_log_possible_stack_dump && ($line =~ /\bcommit\s+[0-9a-f]{5,}\b/i || - ($line =~ /\b[0-9a-f]{12,40}\b/i && + ($line =~ /\b[\s^\("][0-9a-f]{12,40}\b/i && $line !~ /[\<\[][0-9a-f]{12,40}[\>\]]/i && $line !~ /\bfixes:\s*[0-9a-f]{12,40}/i))) { my $init_char = "c"; ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-12-04 1:39 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-11-16 22:43 [BUG] checkpatch: false positive for commits with quote characters Brian Norris 2015-11-17 17:48 ` Joe Perches 2015-11-17 18:03 ` Brian Norris 2015-12-04 0:13 ` Brian Norris 2015-12-04 0:29 ` Joe Perches 2015-12-04 0:36 ` Joe Perches 2015-12-04 1:39 ` Brian Norris
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox