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

Groups > linux.kernel > #1283476 > unrolled thread

Re: [BUG] checkpatch: false positive for commits with quote characters

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: [BUG] checkpatch: false positive for commits with quote  characters Brian Norris <computersforpeace@gmail.com> - 2015-12-04 01:20 +0100
    Re: [BUG] checkpatch: false positive for commits with quote  characters Joe Perches <joe@perches.com> - 2015-12-04 01:30 +0100
      Re: [BUG] checkpatch: false positive for commits with quote  characters Joe Perches <joe@perches.com> - 2015-12-04 01:40 +0100
        Re: [BUG] checkpatch: false positive for commits with quote  characters Brian Norris <computersforpeace@gmail.com> - 2015-12-04 02:50 +0100

#1283476 — Re: [BUG] checkpatch: false positive for commits with quote characters

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.
> 
--
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/

[toc] | [next] | [standalone]

#1283477

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,
--
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/

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

#1283485

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;
 			}
--
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/

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

#1283534

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web