All of lore.kernel.org
 help / color / mirror / Atom feed
* [BUG] False positive in scripts/checkpatch.pl
@ 2016-06-11  1:26 Heinrich Schuchardt
  2016-10-23  7:34 ` [PATCH 1/1] checkpatch: remove false warning for commit reference Heinrich Schuchardt
  0 siblings, 1 reply; 9+ messages in thread
From: Heinrich Schuchardt @ 2016-06-11  1:26 UTC (permalink / raw)
  To: Andy Whitcroft, Joe Perches; +Cc: linux-kernel, Heinrich Schuchardt

scripts/checkpatch.pl reports

ERROR: Please use git commit description style
'commit <12+ chars of sha1> ("<title line>")'-
ie: 'commit 0123456789ab ("commit description")'

for this patch.

On 64bit systems an undetected overflow may occur in
do_proc_dointvec_minmax_conv_param as can be demonstrated
with the following example:

	# echo 9223372036854776806 > /proc/sys/kernel/threads-max
	# cat /proc/sys/kernel/threads-max
	998

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 kernel/sysctl.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 35f0dcb..a9e7be3 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2313,7 +2313,17 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
 {
 	struct do_proc_dointvec_minmax_conv_param *param = data;
 	if (write) {
-		int val = *negp ? -*lvalp : *lvalp;
+		int val;
+
+		if (*negp) {
+			if (*lvalp > (unsigned long) INT_MAX + 1)
+				return -EINVAL;
+			val = -*lvalp;
+		} else {
+			if (*lvalp > (unsigned long) INT_MAX)
+				return -EINVAL;
+			val = *lvalp;
+		}
 		if ((param->min && *param->min > val) ||
 		    (param->max && *param->max < val))
 			return -EINVAL;
-- 
2.1.4

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/1] checkpatch: remove false warning for commit reference
  2016-06-11  1:26 [BUG] False positive in scripts/checkpatch.pl Heinrich Schuchardt
@ 2016-10-23  7:34 ` Heinrich Schuchardt
  2016-10-23 20:37   ` Joe Perches
  0 siblings, 1 reply; 9+ messages in thread
From: Heinrich Schuchardt @ 2016-10-23  7:34 UTC (permalink / raw)
  To: Andy Whitcroft, Joe Perches; +Cc: linux-kernel, Heinrich Schuchardt

Checkpatch warns of an incorrect commit reference style
for any hexadecimal number of 12 digits and more.

Numbers of 12 digits are not necessarily commit ids.

For an example provoking the problem see
https://patchwork.kernel.org/patch/9170897/

Checkpatch should only warn if the number refers to an
existing commit.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 scripts/checkpatch.pl | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a8368d1..12e6a1f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -848,6 +848,7 @@ sub git_commit_info {
 #		    echo "commit $(cut -c 1-12,41-)"
 #		done
 	} elsif ($lines[0] =~ /^fatal: ambiguous argument '$commit': unknown revision or path not in the working tree\./) {
+		$id = undef;
 	} else {
 		$id = substr($lines[0], 0, 12);
 		$desc = substr($lines[0], 41);
@@ -2577,7 +2578,9 @@ sub process {
 			($id, $description) = git_commit_info($orig_commit,
 							      $id, $orig_desc);
 
-			if ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens) {
+			if ($id && ($short || $long || $space || $case
+				|| ($orig_desc ne $description)
+				|| !$hasparens)) {
 				ERROR("GIT_COMMIT_ID",
 				      "Please use git commit description style 'commit <12+ chars of sha1> (\"<title line>\")' - ie: '${init_char}ommit $id (\"$description\")'\n" . $herecurr);
 			}
-- 
2.1.4

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] checkpatch: remove false warning for commit reference
  2016-10-23  7:34 ` [PATCH 1/1] checkpatch: remove false warning for commit reference Heinrich Schuchardt
@ 2016-10-23 20:37   ` Joe Perches
  2016-10-24 17:22     ` Heinrich Schuchardt
  2017-06-07 18:40     ` [PATCH 1/1 v2] checkpatch: remove false warning for commit reference Heinrich Schuchardt
  0 siblings, 2 replies; 9+ messages in thread
From: Joe Perches @ 2016-10-23 20:37 UTC (permalink / raw)
  To: Heinrich Schuchardt, Andy Whitcroft, Andrew Morton; +Cc: linux-kernel

On Sun, 2016-10-23 at 09:34 +0200, Heinrich Schuchardt wrote:
> Checkpatch warns of an incorrect commit reference style
> for any hexadecimal number of 12 digits and more.
> 
> Numbers of 12 digits are not necessarily commit ids.
> 
> For an example provoking the problem see
> https://patchwork.kernel.org/patch/9170897/
> 
> Checkpatch should only warn if the number refers to an
> existing commit.

That seems sensible.

> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  scripts/checkpatch.pl | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -848,6 +848,7 @@ sub git_commit_info {
>  #		    echo "commit $(cut -c 1-12,41-)"
>  #		done
>  	} elsif ($lines[0] =~ /^fatal: ambiguous argument '$commit': unknown revision or path not in the working tree\./) {
> +		$id = undef;

OK

>  	} else {
>  		$id = substr($lines[0], 0, 12);
>  		$desc = substr($lines[0], 41);
> @@ -2577,7 +2578,9 @@ sub process {
>  			($id, $description) = git_commit_info($orig_commit,
>  							      $id, $orig_desc);
>  
> -			if ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens) {
> +			if ($id && ($short || $long || $space || $case
> +				|| ($orig_desc ne $description)
> +				|| !$hasparens)) {

I'd prefer
			if (defined($id) &&
			   ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens)) {

and not wrap the tests.

Maybe ignore all the cases there git is not installed too.
Something like:
---
 scripts/checkpatch.pl | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a8368d1c4348..a46b6ebe067b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -829,13 +829,16 @@ sub seed_camelcase_includes {
 sub git_commit_info {
 	my ($commit, $id, $desc) = @_;
 
-	return ($id, $desc) if ((which("git") eq "") || !(-e ".git"));
+	my $git_id = undef;
+	my $git_desc = undef;
+
+	return ($git_id, $git_desc) if ((which("git") eq "") || !(-e ".git"));
 
 	my $output = `git log --no-color --format='%H %s' -1 $commit 2>&1`;
 	$output =~ s/^\s*//gm;
 	my @lines = split("\n", $output);
 
-	return ($id, $desc) if ($#lines < 0);
+	return ($git_id, $git_desc) if ($#lines < 0);
 
 	if ($lines[0] =~ /^error: short SHA1 $commit is ambiguous\./) {
 # Maybe one day convert this block of bash into something that returns
@@ -849,11 +852,11 @@ sub git_commit_info {
 #		done
 	} elsif ($lines[0] =~ /^fatal: ambiguous argument '$commit': unknown revision or path not in the working tree\./) {
 	} else {
-		$id = substr($lines[0], 0, 12);
-		$desc = substr($lines[0], 41);
+		$git_id = substr($lines[0], 0, 12);
+		$git_desc = substr($lines[0], 41);
 	}
 
-	return ($id, $desc);
+	return ($git_id, $git_desc);
 }
 
 $chk_signoff = 0 if ($file);
@@ -2577,7 +2580,8 @@ sub process {
 			($id, $description) = git_commit_info($orig_commit,
 							      $id, $orig_desc);
 
-			if ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens) {
+			if (defined($id) &&
+			    ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens)) {
 				ERROR("GIT_COMMIT_ID",
 				      "Please use git commit description style 'commit <12+ chars of sha1> (\"\")' - ie: '${init_char}ommit $id (\"$description\")'\n" . $herecurr);
 			}

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] checkpatch: remove false warning for commit reference
  2016-10-23 20:37   ` Joe Perches
@ 2016-10-24 17:22     ` Heinrich Schuchardt
  2016-10-24 18:39       ` Joe Perches
  2017-06-07 18:40     ` [PATCH 1/1 v2] checkpatch: remove false warning for commit reference Heinrich Schuchardt
  1 sibling, 1 reply; 9+ messages in thread
From: Heinrich Schuchardt @ 2016-10-24 17:22 UTC (permalink / raw)
  To: Joe Perches, Andy Whitcroft, Andrew Morton; +Cc: linux-kernel

On 10/23/2016 10:37 PM, Joe Perches wrote:
> On Sun, 2016-10-23 at 09:34 +0200, Heinrich Schuchardt wrote:
>> Checkpatch warns of an incorrect commit reference style
>> for any hexadecimal number of 12 digits and more.
>>
>> Numbers of 12 digits are not necessarily commit ids.
>>
>> For an example provoking the problem see
>> https://patchwork.kernel.org/patch/9170897/
>>
>> Checkpatch should only warn if the number refers to an
>> existing commit.
> 
> That seems sensible.
> 
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  scripts/checkpatch.pl | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
>> @@ -848,6 +848,7 @@ sub git_commit_info {
>>  #		    echo "commit $(cut -c 1-12,41-)"
>>  #		done
>>  	} elsif ($lines[0] =~ /^fatal: ambiguous argument '$commit': unknown revision or path not in the working tree\./) {
>> +		$id = undef;
> 
> OK
> 
>>  	} else {
>>  		$id = substr($lines[0], 0, 12);
>>  		$desc = substr($lines[0], 41);
>> @@ -2577,7 +2578,9 @@ sub process {
>>  			($id, $description) = git_commit_info($orig_commit,
>>  							      $id, $orig_desc);
>>  
>> -			if ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens) {
>> +			if ($id && ($short || $long || $space || $case
>> +				|| ($orig_desc ne $description)
>> +				|| !$hasparens)) {
> 
> I'd prefer
> 			if (defined($id) &&
For $id = 0 or $id = "" this would make a difference.
Surely I can update the patch like this to make the test more readable.

> 			   ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens)) {
> 
> and not wrap the tests.

Putting defined($id) on a line of its own is fine.

Not wrapping the tests will produce a line of over 80 characters and
give a warning in scripts/checkpatch.pl.

The script should respect the standards it imposes on others.

> 
> Maybe ignore all the cases there git is not installed too.
> Something like:
> ---
>  scripts/checkpatch.pl | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index a8368d1c4348..a46b6ebe067b 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -829,13 +829,16 @@ sub seed_camelcase_includes {
>  sub git_commit_info {
>  	my ($commit, $id, $desc) = @_;
>  
> -	return ($id, $desc) if ((which("git") eq "") || !(-e ".git"));
> +	my $git_id = undef;
> +	my $git_desc = undef;
> +
> +	return ($git_id, $git_desc) if ((which("git") eq "") || !(-e ".git"));
Why not simply
return (undef, undef) if ((which("git") eq "") || !(-e ".git"));

Should we provide a warning: "git not found"?
>  
>  	my $output = `git log --no-color --format='%H %s' -1 $commit 2>&1`;
>  	$output =~ s/^\s*//gm;
>  	my @lines = split("\n", $output);
>  
> -	return ($id, $desc) if ($#lines < 0);
> +	return ($git_id, $git_desc) if ($#lines < 0);
>  
>  	if ($lines[0] =~ /^error: short SHA1 $commit is ambiguous\./) {
>  # Maybe one day convert this block of bash into something that returns
> @@ -849,11 +852,11 @@ sub git_commit_info {
>  #		done
>  	} elsif ($lines[0] =~ /^fatal: ambiguous argument '$commit': unknown revision or path not in the working tree\./) {
>  	} else {
> -		$id = substr($lines[0], 0, 12);
> -		$desc = substr($lines[0], 41);
> +		$git_id = substr($lines[0], 0, 12);
> +		$git_desc = substr($lines[0], 41);
>  	}
>  
> -	return ($id, $desc);
> +	return ($git_id, $git_desc);

This will change the program logic for
if ($lines[0] =~ /^error: short SHA1 $commit is ambiguous\./)

>  }
>  
>  $chk_signoff = 0 if ($file);
> @@ -2577,7 +2580,8 @@ sub process {
>  			($id, $description) = git_commit_info($orig_commit,
>  							      $id, $orig_desc);
>  
> -			if ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens) {
> +			if (defined($id) &&
> +			    ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens)) {
This line is over 80 characters.
Which is not accepatble according to checkpatch.pl.

>  				ERROR("GIT_COMMIT_ID",
>  				      "Please use git commit description style 'commit <12+ chars of sha1> (\"\")' - ie: '${init_char}ommit $id (\"$description\")'\n" . $herecurr);
>  			}
> 
> 
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] checkpatch: remove false warning for commit reference
  2016-10-24 17:22     ` Heinrich Schuchardt
@ 2016-10-24 18:39       ` Joe Perches
  2016-10-24 18:46         ` Andy Whitcroft
  0 siblings, 1 reply; 9+ messages in thread
From: Joe Perches @ 2016-10-24 18:39 UTC (permalink / raw)
  To: Heinrich Schuchardt, Andy Whitcroft, Andrew Morton; +Cc: linux-kernel

On Mon, 2016-10-24 at 19:22 +0200, Heinrich Schuchardt wrote:
> On 10/23/2016 10:37 PM, Joe Perches wrote:
> > On Sun, 2016-10-23 at 09:34 +0200, Heinrich Schuchardt wrote:
> > > Checkpatch warns of an incorrect commit reference style
> > > for any hexadecimal number of 12 digits and more.
> > > 
> > > Numbers of 12 digits are not necessarily commit ids.
> > > 
> > > For an example provoking the problem see
> > > https://patchwork.kernel.org/patch/9170897/
> > > 
> > > Checkpatch should only warn if the number refers to an
> > > existing commit.
> > 
> > That seems sensible.
> > 
> > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > ---
> > >  scripts/checkpatch.pl | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > 
> > []
> > > @@ -848,6 +848,7 @@ sub git_commit_info {
> > >  #		    echo "commit $(cut -c 1-12,41-)"
> > >  #		done
> > >  	} elsif ($lines[0] =~ /^fatal: ambiguous argument '$commit': unknown revision or path not in the working tree\./) {
> > > +		$id = undef;
> > 
> > OK
> > 
> > >  	} else {
> > >  		$id = substr($lines[0], 0, 12);
> > >  		$desc = substr($lines[0], 41);
> > > @@ -2577,7 +2578,9 @@ sub process {
> > >  			($id, $description) = git_commit_info($orig_commit,
> > >  							      $id, $orig_desc);
> > >  
> > > -			if ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens) {
> > > +			if ($id && ($short || $long || $space || $case
> > > +				|| ($orig_desc ne $description)
> > > +				|| !$hasparens)) {
> > 
> > I'd prefer
> > 			if (defined($id) &&
> 
> For $id = 0 or $id = "" this would make a difference.
> Surely I can update the patch like this to make the test more readable.
> 
> > 			   ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens)) {
> > 
> > and not wrap the tests.
> 
> Putting defined($id) on a line of its own is fine.
> 
> Not wrapping the tests will produce a line of over 80 characters and
> give a warning in scripts/checkpatch.pl.
> 
> The script should respect the standards it imposes on others.

Nope.

I never bother using checkpatch on checkpatch.
perl is unreadable enough with having to wrap long and
complex regexes on 80 columns.

> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> > @@ -829,13 +829,16 @@ sub seed_camelcase_includes {
> >  sub git_commit_info {
> >  	my ($commit, $id, $desc) = @_;
> >  
> > -	return ($id, $desc) if ((which("git") eq "") || !(-e ".git"));
> > +	my $git_id = undef;
> > +	my $git_desc = undef;
> > +
> > +	return ($git_id, $git_desc) if ((which("git") eq "") || !(-e ".git"));
> 
> Why not simply
> return (undef, undef) if ((which("git") eq "") || !(-e ".git"));

shrug.  coding taste or lack thereof.
Maybe mine.  No specific reason one is
better than the other.

> Should we provide a warning: "git not found"?

No. git isn't required.

> > @@ -2577,7 +2580,8 @@ sub process {
> >  			($id, $description) = git_commit_info($orig_commit,
> >  							      $id, $orig_desc);
> >  
> > -			if ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens) {
> > +			if (defined($id) &&  
> > +			    ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens)) {
> 
> This line is over 80 characters.

And I specifically don't care.

> Which is not accepatble according to checkpatch.pl.

see above

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] checkpatch: remove false warning for commit reference
  2016-10-24 18:39       ` Joe Perches
@ 2016-10-24 18:46         ` Andy Whitcroft
  2016-10-24 20:17           ` [PATCH] checkpatch: Don't check .pl files, improve absolute path commit log test Joe Perches
  0 siblings, 1 reply; 9+ messages in thread
From: Andy Whitcroft @ 2016-10-24 18:46 UTC (permalink / raw)
  To: Joe Perches; +Cc: Heinrich Schuchardt, Andrew Morton, linux-kernel

On Mon, Oct 24, 2016 at 11:39:45AM -0700, Joe Perches wrote:
> On Mon, 2016-10-24 at 19:22 +0200, Heinrich Schuchardt wrote:
> > On 10/23/2016 10:37 PM, Joe Perches wrote:
> > > On Sun, 2016-10-23 at 09:34 +0200, Heinrich Schuchardt wrote:
> > > > Checkpatch warns of an incorrect commit reference style
> > > > for any hexadecimal number of 12 digits and more.
> > > > 
> > > > Numbers of 12 digits are not necessarily commit ids.
> > > > 
> > > > For an example provoking the problem see
> > > > https://patchwork.kernel.org/patch/9170897/
> > > > 
> > > > Checkpatch should only warn if the number refers to an
> > > > existing commit.
> > > 
> > > That seems sensible.
> > > 
> > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > ---
> > > >  scripts/checkpatch.pl | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > 
> > > []
> > > > @@ -848,6 +848,7 @@ sub git_commit_info {
> > > >  #		    echo "commit $(cut -c 1-12,41-)"
> > > >  #		done
> > > >  	} elsif ($lines[0] =~ /^fatal: ambiguous argument '$commit': unknown revision or path not in the working tree\./) {
> > > > +		$id = undef;
> > > 
> > > OK
> > > 
> > > >  	} else {
> > > >  		$id = substr($lines[0], 0, 12);
> > > >  		$desc = substr($lines[0], 41);
> > > > @@ -2577,7 +2578,9 @@ sub process {
> > > >  			($id, $description) = git_commit_info($orig_commit,
> > > >  							      $id, $orig_desc);
> > > >  
> > > > -			if ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens) {
> > > > +			if ($id && ($short || $long || $space || $case
> > > > +				|| ($orig_desc ne $description)
> > > > +				|| !$hasparens)) {
> > > 
> > > I'd prefer
> > > 			if (defined($id) &&
> > 
> > For $id = 0 or $id = "" this would make a difference.
> > Surely I can update the patch like this to make the test more readable.
> > 
> > > 			   ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens)) {
> > > 
> > > and not wrap the tests.
> > 
> > Putting defined($id) on a line of its own is fine.
> > 
> > Not wrapping the tests will produce a line of over 80 characters and
> > give a warning in scripts/checkpatch.pl.
> > 
> > The script should respect the standards it imposes on others.
> 
> Nope.
> 
> I never bother using checkpatch on checkpatch.
> perl is unreadable enough with having to wrap long and
> complex regexes on 80 columns.

More specifically checkpatch is primarily checking C language construction
to keep the style of the Linux kernel consistent.  The rules were never
really intended to apply to perl or other languages.  We have long leant
towards longer lines in checkpatch for some kind of readability, as much
as you can have in "systactic sugar overload" language.

-apw

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] checkpatch: Don't check .pl files, improve absolute path commit log test
  2016-10-24 18:46         ` Andy Whitcroft
@ 2016-10-24 20:17           ` Joe Perches
  0 siblings, 0 replies; 9+ messages in thread
From: Joe Perches @ 2016-10-24 20:17 UTC (permalink / raw)
  To: Andrew Morton, Andy Whitcroft; +Cc: Heinrich Schuchardt, linux-kernel

perl files (*.pl) are mostly inappropriate to check coding styles so
exempt them from long line checks and various .[ch] file type tests.

And as well, only scan absolute paths in the commit log, not in the patch.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a8368d1c4348..2fc154bd81c0 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2601,20 +2601,6 @@ sub process {
 				$herecurr) if (!$emitted_corrupt++);
 		}
 
-# Check for absolute kernel paths.
-		if ($tree) {
-			while ($line =~ m{(?:^|\s)(/\S*)}g) {
-				my $file = $1;
-
-				if ($file =~ m{^(.*?)(?::\d+)+:?$} &&
-				    check_absolute_file($1, $herecurr)) {
-					#
-				} else {
-					check_absolute_file($file, $herecurr);
-				}
-			}
-		}
-
 # UTF-8 regex found at http://www.w3.org/International/questions/qa-forms-utf-8.en.php
 		if (($realfile =~ /^$/ || $line =~ /^\+/) &&
 		    $rawline !~ m/^$UTF8*$/) {
@@ -2652,6 +2638,20 @@ sub process {
 			    "8-bit UTF-8 used in possible commit log\n" . $herecurr);
 		}
 
+# Check for absolute kernel paths in commit message
+		if ($tree && $in_commit_log) {
+			while ($line =~ m{(?:^|\s)(/\S*)}g) {
+				my $file = $1;
+
+				if ($file =~ m{^(.*?)(?::\d+)+:?$} &&
+				    check_absolute_file($1, $herecurr)) {
+					#
+				} else {
+					check_absolute_file($file, $herecurr);
+				}
+			}
+		}
+
 # Check for various typo / spelling mistakes
 		if (defined($misspellings) &&
 		    ($in_commit_log || $line =~ /^(?:\+|Subject:)/i)) {
@@ -2805,7 +2805,7 @@ sub process {
 		}
 
 # check we are in a valid source file if not then ignore this hunk
-		next if ($realfile !~ /\.(h|c|s|S|pl|sh|dtsi|dts)$/);
+		next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);
 
 # line length limit (with some exclusions)
 #
-- 
2.10.0.rc2.1.g053435c

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/1 v2] checkpatch: remove false warning for commit reference
  2016-10-23 20:37   ` Joe Perches
  2016-10-24 17:22     ` Heinrich Schuchardt
@ 2017-06-07 18:40     ` Heinrich Schuchardt
  2017-06-07 18:56       ` Joe Perches
  1 sibling, 1 reply; 9+ messages in thread
From: Heinrich Schuchardt @ 2017-06-07 18:40 UTC (permalink / raw)
  To: Andy Whitcroft, Joe Perches; +Cc: linux-kernel, Heinrich Schuchardt

Checkpatch warns of an incorrect commit reference style
for any hexadecimal number of 12 digits and more.

Numbers of 12 digits are not necessarily commit ids.

For an example provoking the problem see
https://patchwork.kernel.org/patch/9170897/

Checkpatch should only warn if the number refers to an
existing commit.

Cc: Joe Perches <joe@perches.com>
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2:
	changed formatting according to suggestions by Joe Perches
---
 scripts/checkpatch.pl | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4b9569fa931b..3895978c5bbd 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -866,6 +866,7 @@ sub git_commit_info {
 #		    echo "commit $(cut -c 1-12,41-)"
 #		done
 	} elsif ($lines[0] =~ /^fatal: ambiguous argument '$commit': unknown revision or path not in the working tree\./) {
+		$id = undef;
 	} else {
 		$id = substr($lines[0], 0, 12);
 		$desc = substr($lines[0], 41);
@@ -2605,7 +2606,8 @@ sub process {
 			($id, $description) = git_commit_info($orig_commit,
 							      $id, $orig_desc);
 
-			if ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens) {
+			if (defined($id) &&
+			   ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens)) {
 				ERROR("GIT_COMMIT_ID",
 				      "Please use git commit description style 'commit <12+ chars of sha1> (\"<title line>\")' - ie: '${init_char}ommit $id (\"$description\")'\n" . $herecurr);
 			}
-- 
2.11.0

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1 v2] checkpatch: remove false warning for commit reference
  2017-06-07 18:40     ` [PATCH 1/1 v2] checkpatch: remove false warning for commit reference Heinrich Schuchardt
@ 2017-06-07 18:56       ` Joe Perches
  0 siblings, 0 replies; 9+ messages in thread
From: Joe Perches @ 2017-06-07 18:56 UTC (permalink / raw)
  To: Heinrich Schuchardt, Andy Whitcroft, Andrew Morton; +Cc: linux-kernel

On Wed, 2017-06-07 at 20:40 +0200, Heinrich Schuchardt wrote:
> Checkpatch warns of an incorrect commit reference style
> for any hexadecimal number of 12 digits and more.
> 
> Numbers of 12 digits are not necessarily commit ids.
> 
> For an example provoking the problem see
> https://patchwork.kernel.org/patch/9170897/
> 
> Checkpatch should only warn if the number refers to an
> existing commit.
> 
> Cc: Joe Perches <joe@perches.com>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2:
> 	changed formatting according to suggestions by Joe Perches

Wow, the original patch was 8+ months ago.  Anyway:

Acked-by: Joe Perches <joe@perches.com>

> ---
>  scripts/checkpatch.pl | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 4b9569fa931b..3895978c5bbd 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -866,6 +866,7 @@ sub git_commit_info {
>  #		    echo "commit $(cut -c 1-12,41-)"
>  #		done
>  	} elsif ($lines[0] =~ /^fatal: ambiguous argument '$commit': unknown revision or path not in the working tree\./) {
> +		$id = undef;
>  	} else {
>  		$id = substr($lines[0], 0, 12);
>  		$desc = substr($lines[0], 41);
> @@ -2605,7 +2606,8 @@ sub process {
>  			($id, $description) = git_commit_info($orig_commit,
>  							      $id, $orig_desc);
>  
> -			if ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens) {
> +			if (defined($id) &&
> +			   ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens)) {
>  				ERROR("GIT_COMMIT_ID",
>  				      "Please use git commit description style 'commit <12+ chars of sha1> (\"<title line>\")' - ie: '${init_char}ommit $id (\"$description\")'\n" . $herecurr);
>  			}

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-06-07 18:56 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-11  1:26 [BUG] False positive in scripts/checkpatch.pl Heinrich Schuchardt
2016-10-23  7:34 ` [PATCH 1/1] checkpatch: remove false warning for commit reference Heinrich Schuchardt
2016-10-23 20:37   ` Joe Perches
2016-10-24 17:22     ` Heinrich Schuchardt
2016-10-24 18:39       ` Joe Perches
2016-10-24 18:46         ` Andy Whitcroft
2016-10-24 20:17           ` [PATCH] checkpatch: Don't check .pl files, improve absolute path commit log test Joe Perches
2017-06-07 18:40     ` [PATCH 1/1 v2] checkpatch: remove false warning for commit reference Heinrich Schuchardt
2017-06-07 18:56       ` Joe Perches

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.