[PATCH 0/3] checkpatch: output style changes

[PATCH 0/3] checkpatch: output style changes

[Joe Perches]

Some think that checkpatch output is not easily parsed by people.
Try some things to make it easier to visually find messages.

Joe Perches (3):
  checkpatch: Improve output with multiple command-line files
  checkpatch: colorize output to terminal
  checkpatch: Add --showfile to allow input via pipe to show filenames

 scripts/checkpatch.pl | 111 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 77 insertions(+), 34 deletions(-)

-- 
2.1.2

[PATCH 1/3] checkpatch: Improve output with multiple command-line files

[Joe Perches]

If there are multiple patches/files on the command line,
use a prefix before the patch/file message output like:
        --------------
        patch/filename
        --------------
to make the identifying which messages go with which
file/patch a bit easier to parse.

Move the perl version and false positive messages after
all the files have been scanned so that they are emitted
only once.

Standardize the NOTE: <...> form to always emit a blank
line before the NOTE and always use print << "EOM" style.

Suggested-by: Petr Mladek <pmladek@suse.cz>
Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 62 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 39 insertions(+), 23 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c8032a0..eaa76bd 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -197,11 +197,11 @@ sub hash_show_words {
 	my ($hashRef, $prefix) = @_;
 
 	if ($quiet == 0 && keys %$hashRef) {
-		print "NOTE: $prefix message types:";
+		print "\nNOTE: $prefix message types:";
 		foreach my $word (sort keys %$hashRef) {
 			print " $word";
 		}
-		print "\n\n";
+		print "\n";
 	}
 }
 
@@ -741,6 +741,13 @@ for my $filename (@ARGV) {
 		push(@rawlines, $_);
 	}
 	close($FILE);
+
+	if ($#ARGV > 0 && $quiet == 0) {
+		print '-' x length($vname) . "\n";
+		print "$vname\n";
+		print '-' x length($vname) . "\n";
+	}
+
 	if (!process($filename)) {
 		$exit = 1;
 	}
@@ -755,6 +762,23 @@ for my $filename (@ARGV) {
 	build_types();
 }
 
+if (!$quiet) {
+	if ($^V lt 5.10.0) {
+		print << "EOM"
+
+NOTE: perl $^V is not modern enough to detect all possible issues.
+      An upgrade to at least perl v5.10.0 is suggested.
+EOM
+	}
+	if ($exit) {
+		print << "EOM"
+
+NOTE: If any of the errors are false positives, please report
+      them to the maintainer, see CHECKPATCH in MAINTAINERS.
+EOM
+	}
+}
+
 exit($exit);
 
 sub top_of_kernel_tree {
@@ -5578,22 +5602,18 @@ sub process {
 		print "total: $cnt_error errors, $cnt_warn warnings, " .
 			(($check)? "$cnt_chk checks, " : "") .
 			"$cnt_lines lines checked\n";
-		print "\n" if ($quiet == 0);
 	}
 
 	if ($quiet == 0) {
-
-		if ($^V lt 5.10.0) {
-			print("NOTE: perl $^V is not modern enough to detect all possible issues.\n");
-			print("An upgrade to at least perl v5.10.0 is suggested.\n\n");
-		}
-
 		# If there were whitespace errors which cleanpatch can fix
 		# then suggest that.
 		if ($rpt_cleaners) {
-			print "NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or\n";
-			print "      scripts/cleanfile\n\n";
 			$rpt_cleaners = 0;
+			print << "EOM"
+
+NOTE: Whitespace errors detected.
+      You may wish to use scripts/cleanpatch or scripts/cleanfile
+EOM
 		}
 	}
 
@@ -5627,6 +5647,7 @@ sub process {
 
 		if (!$quiet) {
 			print << "EOM";
+
 Wrote EXPERIMENTAL --fix correction(s) to '$newfile'
 
 Do _NOT_ trust the results written to this file.
@@ -5634,22 +5655,17 @@ Do _NOT_ submit these changes without inspecting them for correctness.
 
 This EXPERIMENTAL file is simply a convenience to help rewrite patches.
 No warranties, expressed or implied...
-
 EOM
 		}
 	}
 
-	if ($clean == 1 && $quiet == 0) {
-		print "$vname has no obvious style problems and is ready for submission.\n"
-	}
-	if ($clean == 0 && $quiet == 0) {
-		print << "EOM";
-$vname has style problems, please review.
-
-If any of these errors are false positives, please report
-them to the maintainer, see CHECKPATCH in MAINTAINERS.
-EOM
+	if ($quiet == 0) {
+		print "\n";
+		if ($clean == 1) {
+			print "$vname has no obvious style problems and is ready for submission.\n";
+		} else {
+			print "$vname has style problems, please review.\n";
+		}
 	}
-
 	return $clean;
 }
-- 
2.1.2

[PATCH 2/3] checkpatch: colorize output to terminal

[Joe Perches]

Add optional colors to make seeing message types a bit easier.

Add --color command line switch, default:on

Error is RED, warning is YELLOW, check is GREEN.
The message type, if shown, is BLUE.

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index eaa76bd..cef2cd4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -9,6 +9,7 @@ use strict;
 use POSIX;
 use File::Basename;
 use Cwd 'abs_path';
+use Term::ANSIColor qw(:constants);
 
 my $P = $0;
 my $D = dirname(abs_path($P));
@@ -49,6 +50,7 @@ my $min_conf_desc_length = 4;
 my $spelling_file = "$D/spelling.txt";
 my $codespell = 0;
 my $codespellfile = "/usr/local/share/codespell/dictionary.txt";
+my $color = 1;
 
 sub help {
 	my ($exitcode) = @_;
@@ -93,6 +95,7 @@ Options:
   --codespell                Use the codespell dictionary for spelling/typos
                              (default:/usr/local/share/codespell/dictionary.txt)
   --codespellfile            Use this codespell dictionary
+  --color                    Use colors when output is STDOUT (default: on)
   -h, --help, --version      display this help and exit
 
 When FILE is - read standard input.
@@ -153,6 +156,7 @@ GetOptions(
 	'test-only=s'	=> \$tst_only,
 	'codespell!'	=> \$codespell,
 	'codespellfile=s'	=> \$codespellfile,
+	'color!'	=> \$color,
 	'h|help'	=> \$help,
 	'version'	=> \$help
 ) or help(1);
@@ -1672,15 +1676,26 @@ sub report {
 	    (defined $tst_only && $msg !~ /\Q$tst_only\E/)) {
 		return 0;
 	}
-	my $line;
+	my $output = '';
+	if (-t STDOUT && $color) {
+		if ($level eq 'ERROR') {
+			$output .= RED;
+		} elsif ($level eq 'WARNING') {
+			$output .= YELLOW;
+		} else {
+			$output .= GREEN;
+		}
+	}
+	$output .= $prefix . $level . ':';
 	if ($show_types) {
-		$line = "$prefix$level:$type: $msg\n";
-	} else {
-		$line = "$prefix$level: $msg\n";
+		$output .= BLUE if (-t STDOUT && $color);
+		$output .= "$type:";
 	}
-	$line = (split('\n', $line))[0] . "\n" if ($terse);
+	$output .= RESET if (-t STDOUT && $color);
+	$output .= ' ' . $msg . "\n";
+	$output = (split('\n', $output))[0] . "\n" if ($terse);
 
-	push(our @report, $line);
+	push(our @report, $output);
 
 	return 1;
 }
-- 
2.1.2

[PATCH 3/3] checkpatch: Add --showfile to allow input via pipe to show filenames

[Joe Perches]

Using "git diff | ./scripts/checkpatch -" does not have an
easy mechanism to see the files and lines actually modified.

Add --showfile to see the file and line specified in the diff.

When --showfile is used without --terse, the second line of each
message output is redundant, so it is removed.

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index cef2cd4..1241f99d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -25,6 +25,7 @@ my $chk_patch = 1;
 my $tst_only;
 my $emacs = 0;
 my $terse = 0;
+my $showfile = 0;
 my $file = 0;
 my $check = 0;
 my $check_orig = 0;
@@ -66,6 +67,7 @@ Options:
   --patch                    treat FILE as patchfile (default)
   --emacs                    emacs compile window format
   --terse                    one line per report
+  --showfile                 emit diffed file position, not input file position
   -f, --file                 treat FILE as regular source file
   --subjective, --strict     enable more subjective tests
   --types TYPE(,TYPE2...)    show only these comma separated message types
@@ -137,6 +139,7 @@ GetOptions(
 	'patch!'	=> \$chk_patch,
 	'emacs!'	=> \$emacs,
 	'terse!'	=> \$terse,
+	'showfile!'	=> \$showfile,
 	'f|file!'	=> \$file,
 	'subjective!'	=> \$check,
 	'strict!'	=> \$check,
@@ -1693,6 +1696,12 @@ sub report {
 	}
 	$output .= RESET if (-t STDOUT && $color);
 	$output .= ' ' . $msg . "\n";
+
+	if ($showfile) {
+		my @lines = split("\n", $output, -1);
+		splice(@lines, 1, 1);
+		$output = join("\n", @lines);
+	}
 	$output = (split('\n', $output))[0] . "\n" if ($terse);
 
 	push(our @report, $output);
@@ -2119,10 +2128,6 @@ sub process {
 
 		my $hunk_line = ($realcnt != 0);
 
-#make up the handle for any error we report on this line
-		$prefix = "$filename:$realline: " if ($emacs && $file);
-		$prefix = "$filename:$linenr: " if ($emacs && !$file);
-
 		$here = "#$linenr: " if (!$file);
 		$here = "#$realline: " if ($file);
 
@@ -2152,6 +2157,13 @@ sub process {
 			$found_file = 1;
 		}
 
+#make up the handle for any error we report on this line
+		if ($showfile) {
+			$prefix = "$realfile:$realline: "
+		} elsif ($emacs) {
+			$prefix = "$filename:$linenr: ";
+		}
+
 		if ($found_file) {
 			if ($realfile =~ m@^(drivers/net/|net/)@) {
 				$check = 1;
@@ -5606,7 +5618,7 @@ sub process {
 		ERROR("NOT_UNIFIED_DIFF",
 		      "Does not appear to be a unified-diff format patch\n");
 	}
-	if ($is_patch && $chk_signoff && $signoff == 0) {
+	if ($is_patch && $filename ne '-' && $chk_signoff && $signoff == 0) {
 		ERROR("MISSING_SIGN_OFF",
 		      "Missing Signed-off-by: line(s)\n");
 	}
-- 
2.1.2

Re: [PATCH 1/3] checkpatch: Improve output with multiple command-line files

[Petr Mladek]

On Wed 2015-06-03 08:53:38, Joe Perches wrote:
> If there are multiple patches/files on the command line,
> use a prefix before the patch/file message output like:
>         --------------
>         patch/filename
>         --------------
> to make the identifying which messages go with which
> file/patch a bit easier to parse.
> 
> Move the perl version and false positive messages after
> all the files have been scanned so that they are emitted
> only once.
> 
> Standardize the NOTE: <...> form to always emit a blank
> line before the NOTE and always use print << "EOM" style.
> 
> Suggested-by: Petr Mladek <pmladek@suse.cz>
> Signed-off-by: Joe Perches <joe@perches.com>

Tested-by: Petr Mladek <pmladek@suse.cz>

One has to get used to the output but it is better readable, definitely.

> ---
>  scripts/checkpatch.pl | 62 ++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 39 insertions(+), 23 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index c8032a0..eaa76bd 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -197,11 +197,11 @@ sub hash_show_words {
>  	my ($hashRef, $prefix) = @_;
>  
>  	if ($quiet == 0 && keys %$hashRef) {
> -		print "NOTE: $prefix message types:";
> +		print "\nNOTE: $prefix message types:";
>  		foreach my $word (sort keys %$hashRef) {
>  			print " $word";
>  		}
> -		print "\n\n";
> +		print "\n";
>  	}
>  }
>  
> @@ -741,6 +741,13 @@ for my $filename (@ARGV) {
>  		push(@rawlines, $_);
>  	}
>  	close($FILE);
> +
> +	if ($#ARGV > 0 && $quiet == 0) {
> +		print '-' x length($vname) . "\n";
> +		print "$vname\n";
> +		print '-' x length($vname) . "\n";
> +	}
> +
>  	if (!process($filename)) {
>  		$exit = 1;
>  	}
> @@ -755,6 +762,23 @@ for my $filename (@ARGV) {
>  	build_types();
>  }
>  
> +if (!$quiet) {
> +	if ($^V lt 5.10.0) {
> +		print << "EOM"
> +
> +NOTE: perl $^V is not modern enough to detect all possible issues.
> +      An upgrade to at least perl v5.10.0 is suggested.
> +EOM
> +	}
> +	if ($exit) {
> +		print << "EOM"
> +
> +NOTE: If any of the errors are false positives, please report
> +      them to the maintainer, see CHECKPATCH in MAINTAINERS.
> +EOM
> +	}
> +}
> +
>  exit($exit);
>  
>  sub top_of_kernel_tree {
> @@ -5578,22 +5602,18 @@ sub process {
>  		print "total: $cnt_error errors, $cnt_warn warnings, " .
>  			(($check)? "$cnt_chk checks, " : "") .
>  			"$cnt_lines lines checked\n";
> -		print "\n" if ($quiet == 0);
>  	}
>  
>  	if ($quiet == 0) {
> -
> -		if ($^V lt 5.10.0) {
> -			print("NOTE: perl $^V is not modern enough to detect all possible issues.\n");
> -			print("An upgrade to at least perl v5.10.0 is suggested.\n\n");
> -		}
> -
>  		# If there were whitespace errors which cleanpatch can fix
>  		# then suggest that.
>  		if ($rpt_cleaners) {
> -			print "NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or\n";
> -			print "      scripts/cleanfile\n\n";
>  			$rpt_cleaners = 0;
> +			print << "EOM"
> +
> +NOTE: Whitespace errors detected.
> +      You may wish to use scripts/cleanpatch or scripts/cleanfile
> +EOM

It would make sense to write this message only once as well.

Best Regards,
Petr

>  		}
>  	}
>  
> @@ -5627,6 +5647,7 @@ sub process {
>  
>  		if (!$quiet) {
>  			print << "EOM";
> +
>  Wrote EXPERIMENTAL --fix correction(s) to '$newfile'
>  
>  Do _NOT_ trust the results written to this file.
> @@ -5634,22 +5655,17 @@ Do _NOT_ submit these changes without inspecting them for correctness.
>  
>  This EXPERIMENTAL file is simply a convenience to help rewrite patches.
>  No warranties, expressed or implied...
> -
>  EOM
>  		}
>  	}
>  
> -	if ($clean == 1 && $quiet == 0) {
> -		print "$vname has no obvious style problems and is ready for submission.\n"
> -	}
> -	if ($clean == 0 && $quiet == 0) {
> -		print << "EOM";
> -$vname has style problems, please review.
> -
> -If any of these errors are false positives, please report
> -them to the maintainer, see CHECKPATCH in MAINTAINERS.
> -EOM
> +	if ($quiet == 0) {
> +		print "\n";
> +		if ($clean == 1) {
> +			print "$vname has no obvious style problems and is ready for submission.\n";
> +		} else {
> +			print "$vname has style problems, please review.\n";
> +		}
>  	}
> -
>  	return $clean;
>  }
> -- 
> 2.1.2
> 

Re: [PATCH 2/3] checkpatch: colorize output to terminal

[Petr Mladek]

On Wed 2015-06-03 08:53:39, Joe Perches wrote:
> Add optional colors to make seeing message types a bit easier.
> 
> Add --color command line switch, default:on
> 
> Error is RED, warning is YELLOW, check is GREEN.
> The message type, if shown, is BLUE.
> 
> Signed-off-by: Joe Perches <joe@perches.com>

Tested-by: Petr Mladek <pmladek@suse.cz>

I like it. It really helps noticing the problems.

Best Regards,
Petr

> ---
>  scripts/checkpatch.pl | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index eaa76bd..cef2cd4 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -9,6 +9,7 @@ use strict;
>  use POSIX;
>  use File::Basename;
>  use Cwd 'abs_path';
> +use Term::ANSIColor qw(:constants);
>  
>  my $P = $0;
>  my $D = dirname(abs_path($P));
> @@ -49,6 +50,7 @@ my $min_conf_desc_length = 4;
>  my $spelling_file = "$D/spelling.txt";
>  my $codespell = 0;
>  my $codespellfile = "/usr/local/share/codespell/dictionary.txt";
> +my $color = 1;
>  
>  sub help {
>  	my ($exitcode) = @_;
> @@ -93,6 +95,7 @@ Options:
>    --codespell                Use the codespell dictionary for spelling/typos
>                               (default:/usr/local/share/codespell/dictionary.txt)
>    --codespellfile            Use this codespell dictionary
> +  --color                    Use colors when output is STDOUT (default: on)
>    -h, --help, --version      display this help and exit
>  
>  When FILE is - read standard input.
> @@ -153,6 +156,7 @@ GetOptions(
>  	'test-only=s'	=> \$tst_only,
>  	'codespell!'	=> \$codespell,
>  	'codespellfile=s'	=> \$codespellfile,
> +	'color!'	=> \$color,
>  	'h|help'	=> \$help,
>  	'version'	=> \$help
>  ) or help(1);
> @@ -1672,15 +1676,26 @@ sub report {
>  	    (defined $tst_only && $msg !~ /\Q$tst_only\E/)) {
>  		return 0;
>  	}
> -	my $line;
> +	my $output = '';
> +	if (-t STDOUT && $color) {
> +		if ($level eq 'ERROR') {
> +			$output .= RED;
> +		} elsif ($level eq 'WARNING') {
> +			$output .= YELLOW;
> +		} else {
> +			$output .= GREEN;
> +		}
> +	}
> +	$output .= $prefix . $level . ':';
>  	if ($show_types) {
> -		$line = "$prefix$level:$type: $msg\n";
> -	} else {
> -		$line = "$prefix$level: $msg\n";
> +		$output .= BLUE if (-t STDOUT && $color);
> +		$output .= "$type:";
>  	}
> -	$line = (split('\n', $line))[0] . "\n" if ($terse);
> +	$output .= RESET if (-t STDOUT && $color);
> +	$output .= ' ' . $msg . "\n";
> +	$output = (split('\n', $output))[0] . "\n" if ($terse);
>  
> -	push(our @report, $line);
> +	push(our @report, $output);
>  
>  	return 1;
>  }
> -- 
> 2.1.2
> 

Re: [PATCH 1/3] checkpatch: Improve output with multiple command-line files

[Joe Perches]

On Thu, 2015-06-04 at 14:03 +0200, Petr Mladek wrote:
> On Wed 2015-06-03 08:53:38, Joe Perches wrote:
> > If there are multiple patches/files on the command line,
> > use a prefix before the patch/file message output like:
> >         --------------
> >         patch/filename
> >         --------------
> > to make the identifying which messages go with which
> > file/patch a bit easier to parse.
[]
> Tested-by: Petr Mladek <pmladek@suse.cz>
[]
> > +NOTE: Whitespace errors detected.
> > +      You may wish to use scripts/cleanpatch or scripts/cleanfile
> > +EOM
> 
> It would make sense to write this message only once as well.

I don't think so as it applies to each patch/file separately.

Re: [PATCH 3/3] checkpatch: Add --showfile to allow input via pipe to show filenames

[Petr Mladek]

On Wed 2015-06-03 08:53:40, Joe Perches wrote:
> Using "git diff | ./scripts/checkpatch -" does not have an
> easy mechanism to see the files and lines actually modified.
> 
> Add --showfile to see the file and line specified in the diff.
> 
> When --showfile is used without --terse, the second line of each
> message output is redundant, so it is removed.

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

I like idea but there is a problem, see below.

> ---
>  scripts/checkpatch.pl | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index cef2cd4..1241f99d 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -25,6 +25,7 @@ my $chk_patch = 1;
>  my $tst_only;
>  my $emacs = 0;
>  my $terse = 0;
> +my $showfile = 0;
>  my $file = 0;
>  my $check = 0;
>  my $check_orig = 0;
> @@ -66,6 +67,7 @@ Options:
>    --patch                    treat FILE as patchfile (default)
>    --emacs                    emacs compile window format
>    --terse                    one line per report
> +  --showfile                 emit diffed file position, not input file position
>    -f, --file                 treat FILE as regular source file
>    --subjective, --strict     enable more subjective tests
>    --types TYPE(,TYPE2...)    show only these comma separated message types
> @@ -137,6 +139,7 @@ GetOptions(
>  	'patch!'	=> \$chk_patch,
>  	'emacs!'	=> \$emacs,
>  	'terse!'	=> \$terse,
> +	'showfile!'	=> \$showfile,
>  	'f|file!'	=> \$file,
>  	'subjective!'	=> \$check,
>  	'strict!'	=> \$check,
> @@ -1693,6 +1696,12 @@ sub report {
>  	}
>  	$output .= RESET if (-t STDOUT && $color);
>  	$output .= ' ' . $msg . "\n";
> +
> +	if ($showfile) {
> +		my @lines = split("\n", $output, -1);
> +		splice(@lines, 1, 1);
> +		$output = join("\n", @lines);
> +	}
>  	$output = (split('\n', $output))[0] . "\n" if ($terse);
>  
>  	push(our @report, $output);
> @@ -2119,10 +2128,6 @@ sub process {
>  
>  		my $hunk_line = ($realcnt != 0);
>  
> -#make up the handle for any error we report on this line
> -		$prefix = "$filename:$realline: " if ($emacs && $file);
> -		$prefix = "$filename:$linenr: " if ($emacs && !$file);
> -
>  		$here = "#$linenr: " if (!$file);
>  		$here = "#$realline: " if ($file);
>  
> @@ -2152,6 +2157,13 @@ sub process {
>  			$found_file = 1;
>  		}
>  
> +#make up the handle for any error we report on this line
> +		if ($showfile) {
> +			$prefix = "$realfile:$realline: "
> +		} elsif ($emacs) {
> +			$prefix = "$filename:$linenr: ";
> +		}
> +
>  		if ($found_file) {
>  			if ($realfile =~ m@^(drivers/net/|net/)@) {
>  				$check = 1;
> @@ -5606,7 +5618,7 @@ sub process {
>  		ERROR("NOT_UNIFIED_DIFF",
>  		      "Does not appear to be a unified-diff format patch\n");
>  	}
> -	if ($is_patch && $chk_signoff && $signoff == 0) {
> +	if ($is_patch && $filename ne '-' && $chk_signoff && $signoff == 0) {

You might use also "cat $patch | ./scripts/checkpatch -" and in this
case you would want to print the warning.

It still prints invalid filename:linenum when you call:
./scripts/checkpatch 0001-my-commit.patch

A better solution would be to omit the filename:linenum information
for this patch-specific messages or print the patchname instead of
the filename.

Best Regards,
Petr

>  		ERROR("MISSING_SIGN_OFF",
>  		      "Missing Signed-off-by: line(s)\n");
>  	}
> -- 
> 2.1.2
> 

Re: [PATCH 1/3] checkpatch: Improve output with multiple command-line files

[Petr Mladek]

On Thu 2015-06-04 05:14:39, Joe Perches wrote:
> On Thu, 2015-06-04 at 14:03 +0200, Petr Mladek wrote:
> > On Wed 2015-06-03 08:53:38, Joe Perches wrote:
> > > If there are multiple patches/files on the command line,
> > > use a prefix before the patch/file message output like:
> > >         --------------
> > >         patch/filename
> > >         --------------
> > > to make the identifying which messages go with which
> > > file/patch a bit easier to parse.
> []
> > Tested-by: Petr Mladek <pmladek@suse.cz>
> []
> > > +NOTE: Whitespace errors detected.
> > > +      You may wish to use scripts/cleanpatch or scripts/cleanfile
> > > +EOM
> > 
> > It would make sense to write this message only once as well.
> 
> I don't think so as it applies to each patch/file separately.

IMHO, it advertises scripts/cleanpatch or scripts/cleanfile. I think
that we do not need to repeat it for each affected patch.

In fact, using scripts/cleanfile is questionable. IMHO, it does not
make sense to fix indentation just because it looks better. It makes
problems when backporting fixes. But it might make sense to fix it
when you do some real change on that line.

Best Regards,
Petr

Re: [PATCH 1/3] checkpatch: Improve output with multiple command-line files

[Joe Perches]

On Thu, 2015-06-04 at 14:29 +0200, Petr Mladek wrote:
> On Thu 2015-06-04 05:14:39, Joe Perches wrote:
> > On Thu, 2015-06-04 at 14:03 +0200, Petr Mladek wrote:
> > > On Wed 2015-06-03 08:53:38, Joe Perches wrote:
[]
> > > > +NOTE: Whitespace errors detected.
> > > > +      You may wish to use scripts/cleanpatch or scripts/cleanfile
> > > > +EOM
> > > 
> > > It would make sense to write this message only once as well.
> > 
> > I don't think so as it applies to each patch/file separately.
> 
> IMHO, it advertises scripts/cleanpatch or scripts/cleanfile. I think
> that we do not need to repeat it for each affected patch.

No worries, we just disagree.

> In fact, using scripts/cleanfile is questionable. IMHO, it does not
> make sense to fix indentation just because it looks better. It makes
> problems when backporting fixes. But it might make sense to fix it
> when you do some real change on that line.

Whitespace at EOL backporting is trivially handled by git am.

cheers, Joe

Re: [PATCH 3/3] checkpatch: Add --showfile to allow input via pipe to show filenames

[Joe Perches]

On Thu, 2015-06-04 at 14:18 +0200, Petr Mladek wrote:
> On Wed 2015-06-03 08:53:40, Joe Perches wrote:
> > Using "git diff | ./scripts/checkpatch -" does not have an
> > easy mechanism to see the files and lines actually modified.
> > 
> > Add --showfile to see the file and line specified in the diff.
> > 
> > When --showfile is used without --terse, the second line of each
> > message output is redundant, so it is removed.
> 
> > Signed-off-by: Joe Perches <joe@perches.com>
> 
> I like idea but there is a problem, see below.
[]
> > @@ -5606,7 +5618,7 @@ sub process {
> >  		ERROR("NOT_UNIFIED_DIFF",
> >  		      "Does not appear to be a unified-diff format patch\n");
> >  	}
> > -	if ($is_patch && $chk_signoff && $signoff == 0) {
> > +	if ($is_patch && $filename ne '-' && $chk_signoff && $signoff == 0) {
> 
> You might use also "cat $patch | ./scripts/checkpatch -" and in this
> case you would want to print the warning.

Yeah, but I'm not straining for likely unusual use-cases.

> It still prints invalid filename:linenum when you call:
> ./scripts/checkpatch 0001-my-commit.patch

I don't see this.
It prints the file/line of the patch without -terse
just as before.

> A better solution would be to omit the filename:linenum information
> for this patch-specific messages or print the patchname instead of
> the filename.

Anyway, I think the patch appropriate to apply
for now and I'll ponder and listen to suggestions
for improvement later.

cheers, Joe