[Qemu-devel] [PATCH for-4.0 0/4] Small checkpatch fixes and improvements

[Qemu-devel] [PATCH for-4.0 0/4] Small checkpatch fixes and improvements

[Paolo Bonzini]

Right now, patchew has a small script around checkpatch to show
some feedback about which patches have problems.  The idea behind
this series is that patchew should be able to do just
"scripts/checkpatch.pl --mailback base..", and everything
should just work.

Paolo

Paolo Bonzini (4):
  checkpatch: fix premature exit when no input or --mailback
  checkpatch: check Signed-off-by in --mailback mode
  checkpatch: improve handling of multiple patches or files
  checkpatch: colorize output to terminal

 scripts/checkpatch.pl | 92 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 73 insertions(+), 19 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH for-4.0 1/4] checkpatch: fix premature exit when no input or --mailback

[Paolo Bonzini]

In some cases, checkpatch's process subroutine is exiting the
whole process.  This is wrong, just return from the subroutine
instead.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/checkpatch.pl | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index eccd656c41..d58fcb1efd 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2869,19 +2869,19 @@ sub process {
 	# If we have no input at all, then there is nothing to report on
 	# so just keep quiet.
 	if ($#rawlines == -1) {
-		exit(0);
+		return 1;
 	}
 
 	# In mailback mode only produce a report in the negative, for
 	# things that appear to be patches.
 	if ($mailback && ($clean == 1 || !$is_patch)) {
-		exit(0);
+		return 1;
 	}
 
 	# This is not a patch, and we are are in 'no-patch' mode so
 	# just keep quiet.
 	if (!$chk_patch && !$is_patch) {
-		exit(0);
+		return 1;
 	}
 
 	if (!$is_patch) {
-- 
2.19.1

Re: [Qemu-devel] [PATCH for-4.0 1/4] checkpatch: fix premature exit when no input or --mailback

[Philippe Mathieu-Daudé]

On 29/11/18 10:01, Paolo Bonzini wrote:
> In some cases, checkpatch's process subroutine is exiting the
> whole process.  This is wrong, just return from the subroutine
> instead.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  scripts/checkpatch.pl | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index eccd656c41..d58fcb1efd 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2869,19 +2869,19 @@ sub process {
>  	# If we have no input at all, then there is nothing to report on
>  	# so just keep quiet.
>  	if ($#rawlines == -1) {
> -		exit(0);
> +		return 1;
>  	}
>  
>  	# In mailback mode only produce a report in the negative, for
>  	# things that appear to be patches.
>  	if ($mailback && ($clean == 1 || !$is_patch)) {
> -		exit(0);
> +		return 1;
>  	}
>  
>  	# This is not a patch, and we are are in 'no-patch' mode so
>  	# just keep quiet.
>  	if (!$chk_patch && !$is_patch) {
> -		exit(0);
> +		return 1;
>  	}
>  
>  	if (!$is_patch) {
> 

Re: [Qemu-devel] [PATCH for-4.0 1/4] checkpatch: fix premature exit when no input or --mailback

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> In some cases, checkpatch's process subroutine is exiting the
> whole process.  This is wrong, just return from the subroutine
> instead.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  scripts/checkpatch.pl | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index eccd656c41..d58fcb1efd 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2869,19 +2869,19 @@ sub process {
>  	# If we have no input at all, then there is nothing to report on
>  	# so just keep quiet.
>  	if ($#rawlines == -1) {
> -		exit(0);
> +		return 1;
>  	}
>  
>  	# In mailback mode only produce a report in the negative, for
>  	# things that appear to be patches.
>  	if ($mailback && ($clean == 1 || !$is_patch)) {
> -		exit(0);
> +		return 1;
>  	}
>  
>  	# This is not a patch, and we are are in 'no-patch' mode so
>  	# just keep quiet.
>  	if (!$chk_patch && !$is_patch) {
> -		exit(0);
> +		return 1;
>  	}
>  
>  	if (!$is_patch) {

Would this make sense for Linux's checkpatch.pl, too?

[Qemu-devel] [PATCH for-4.0 2/4] checkpatch: check Signed-off-by in --mailback mode

[Paolo Bonzini]

Pull the test before the anticipated exits from the process sub.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/checkpatch.pl | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d58fcb1efd..c216c55e01 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2866,6 +2866,10 @@ sub process {
 		}
 	}
 
+	if ($is_patch && $chk_signoff && $signoff == 0) {
+		ERROR("Missing Signed-off-by: line(s)\n");
+	}
+
 	# If we have no input at all, then there is nothing to report on
 	# so just keep quiet.
 	if ($#rawlines == -1) {
@@ -2887,9 +2891,6 @@ sub process {
 	if (!$is_patch) {
 		ERROR("Does not appear to be a unified-diff format patch\n");
 	}
-	if ($is_patch && $chk_signoff && $signoff == 0) {
-		ERROR("Missing Signed-off-by: line(s)\n");
-	}
 
 	print report_dump();
 	if ($summary && !($clean == 1 && $quiet == 1)) {
-- 
2.19.1

Re: [Qemu-devel] [PATCH for-4.0 2/4] checkpatch: check Signed-off-by in --mailback mode

[Philippe Mathieu-Daudé]

On 29/11/18 10:01, Paolo Bonzini wrote:
> Pull the test before the anticipated exits from the process sub.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  scripts/checkpatch.pl | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d58fcb1efd..c216c55e01 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2866,6 +2866,10 @@ sub process {
>  		}
>  	}
>  
> +	if ($is_patch && $chk_signoff && $signoff == 0) {
> +		ERROR("Missing Signed-off-by: line(s)\n");
> +	}
> +
>  	# If we have no input at all, then there is nothing to report on
>  	# so just keep quiet.
>  	if ($#rawlines == -1) {
> @@ -2887,9 +2891,6 @@ sub process {
>  	if (!$is_patch) {
>  		ERROR("Does not appear to be a unified-diff format patch\n");
>  	}
> -	if ($is_patch && $chk_signoff && $signoff == 0) {
> -		ERROR("Missing Signed-off-by: line(s)\n");
> -	}
>  
>  	print report_dump();
>  	if ($summary && !($clean == 1 && $quiet == 1)) {
> 

Re: [Qemu-devel] [PATCH for-4.0 2/4] checkpatch: check Signed-off-by in --mailback mode

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Pull the test before the anticipated exits from the process sub.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  scripts/checkpatch.pl | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d58fcb1efd..c216c55e01 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2866,6 +2866,10 @@ sub process {
>  		}
>  	}
>  
> +	if ($is_patch && $chk_signoff && $signoff == 0) {
> +		ERROR("Missing Signed-off-by: line(s)\n");
> +	}
> +
>  	# If we have no input at all, then there is nothing to report on
>  	# so just keep quiet.
>  	if ($#rawlines == -1) {
> @@ -2887,9 +2891,6 @@ sub process {
>  	if (!$is_patch) {
>  		ERROR("Does not appear to be a unified-diff format patch\n");
>  	}
> -	if ($is_patch && $chk_signoff && $signoff == 0) {
> -		ERROR("Missing Signed-off-by: line(s)\n");
> -	}
>  
>  	print report_dump();
>  	if ($summary && !($clean == 1 && $quiet == 1)) {

Would this make sense for Linux's checkpatch.pl, too?

Re: [Qemu-devel] [PATCH for-4.0 2/4] checkpatch: check Signed-off-by in --mailback mode

[Paolo Bonzini]

On 29/11/18 17:21, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Pull the test before the anticipated exits from the process sub.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  scripts/checkpatch.pl | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index d58fcb1efd..c216c55e01 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -2866,6 +2866,10 @@ sub process {
>>  		}
>>  	}
>>  
>> +	if ($is_patch && $chk_signoff && $signoff == 0) {
>> +		ERROR("Missing Signed-off-by: line(s)\n");
>> +	}
>> +
>>  	# If we have no input at all, then there is nothing to report on
>>  	# so just keep quiet.
>>  	if ($#rawlines == -1) {
>> @@ -2887,9 +2891,6 @@ sub process {
>>  	if (!$is_patch) {
>>  		ERROR("Does not appear to be a unified-diff format patch\n");
>>  	}
>> -	if ($is_patch && $chk_signoff && $signoff == 0) {
>> -		ERROR("Missing Signed-off-by: line(s)\n");
>> -	}
>>  
>>  	print report_dump();
>>  	if ($summary && !($clean == 1 && $quiet == 1)) {
> 
> Would this make sense for Linux's checkpatch.pl, too?
> 

Yes, but I have never had any luck upstreaming our changes. :(  For
example, e20122ff0faf07cb701d35e39e106d1783c07725 is a genuine bugfix
but it was ignored.

I am willing to give it a try, but I'd rather not hold this change to
QEMU's checkpatch.

(Same answer for 1/4).

Paolo

[Qemu-devel] [PATCH for-4.0 3/4] checkpatch: improve handling of multiple patches or files

[Paolo Bonzini]

Similar to how patchew output looks like for multiple patches,
say what file or patch is being tested _before_ emitting errors.
This is clearer to a human that scans the output from top to
bottom.

In addition, provide a truncated commit hash and subject instead of
the full hash, and process the commits first-to-last rather than
last-to-first.

Inspired by Linux commit 0dea9f1eef86bedacad91b6f652ca1ab0d08854c
("checkpatch: reduce number of `git log` calls with --git", 2016-03-20).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/checkpatch.pl | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c216c55e01..f5284d910c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -340,13 +340,18 @@ my @lines = ();
 my $vname;
 if ($chk_branch) {
 	my @patches;
+	my %git_commits = ();
 	my $HASH;
-	open($HASH, "-|", "git", "log", "--format=%H", $ARGV[0]) ||
-		die "$P: git log --format=%H $ARGV[0] failed - $!\n";
-
-	while (<$HASH>) {
-		chomp;
-		push @patches, $_;
+	open($HASH, "-|", "git", "log", "--reverse", "--no-merges", "--format=%H %s", $ARGV[0]) ||
+		die "$P: git log --reverse --no-merges --format='%H %s' $ARGV[0] failed - $!\n";
+
+	for my $line (<$HASH>) {
+		$line =~ /^([0-9a-fA-F]{40,40}) (.*)$/;
+		next if (!defined($1) || !defined($2));
+		my $sha1 = $1;
+		my $subject = $2;
+		push(@patches, $sha1);
+		$git_commits{$sha1} = $subject;
 	}
 
 	close $HASH;
@@ -354,21 +359,31 @@ if ($chk_branch) {
 	die "$P: no revisions returned for revlist '$chk_branch'\n"
 	    unless @patches;
 
+	my $i = 1;
+	my $num_patches = @patches;
 	for my $hash (@patches) {
 		my $FILE;
 		open($FILE, '-|', "git", "show", $hash) ||
 			die "$P: git show $hash - $!\n";
-		$vname = $hash;
 		while (<$FILE>) {
 			chomp;
 			push(@rawlines, $_);
 		}
 		close($FILE);
+		$vname = substr($hash, 0, 12) . ' (' . $git_commits{$hash} . ')';
+		if ($num_patches > 1 && $quiet == 0) {
+			print "$i/$num_patches Checking commit $vname\n";
+			$vname = "Patch $i/$num_patches";
+		} else {
+			$vname = "Commit " . $vname;
+		}
 		if (!process($hash)) {
 			$exit = 1;
+			print "\n" if ($num_patches > 1 && $quiet == 0);
 		}
 		@rawlines = ();
 		@lines = ();
+		$i++;
 	}
 } else {
 	for my $filename (@ARGV) {
@@ -387,6 +402,7 @@ if ($chk_branch) {
 		} else {
 			$vname = $filename;
 		}
+		print "Checking $filename...\n" if @ARGV > 1 && $quiet == 0;
 		while (<$FILE>) {
 			chomp;
 			push(@rawlines, $_);
-- 
2.19.1

[Qemu-devel] [PATCH for-4.0 4/4] checkpatch: colorize output to terminal

[Paolo Bonzini]

Add optional colors to make seeing message types a bit easier.
The default is to show them on a tty.  The chosen colors should resemble
gcc's.

Inspired by Linux commit 57230297116faf5b0a995916d5dd5fedab54cba3
("checkpatch: colorize output to terminal").
---
 scripts/checkpatch.pl | 51 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 7 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f5284d910c..14be11719c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -7,6 +7,7 @@
 
 use strict;
 use warnings;
+use Term::ANSIColor qw(:constants);
 
 my $P = $0;
 $P =~ s@.*/@@g;
@@ -26,6 +27,7 @@ my $tst_only;
 my $emacs = 0;
 my $terse = 0;
 my $file = undef;
+my $color = "auto";
 my $no_warnings = 0;
 my $summary = 1;
 my $mailback = 0;
@@ -64,6 +66,8 @@ Options:
                              is all off)
   --test-only=WORD           report only warnings/errors containing WORD
                              literally
+  --color[=WHEN]             Use colors 'always', 'never', or only when output
+                             is a terminal ('auto'). Default is 'auto'.
   -h, --help, --version      display this help and exit
 
 When FILE is - read standard input.
@@ -72,6 +76,14 @@ EOM
 	exit($exitcode);
 }
 
+# Perl's Getopt::Long allows options to take optional arguments after a space.
+# Prevent --color by itself from consuming other arguments
+foreach (@ARGV) {
+	if ($_ eq "--color" || $_ eq "-color") {
+		$_ = "--color=$color";
+	}
+}
+
 GetOptions(
 	'q|quiet+'	=> \$quiet,
 	'tree!'		=> \$tree,
@@ -89,6 +101,8 @@ GetOptions(
 
 	'debug=s'	=> \%debug,
 	'test-only=s'	=> \$tst_only,
+	'color=s'       => \$color,
+	'no-color'      => sub { $color = 'never'; },
 	'h|help'	=> \$help,
 	'version'	=> \$help
 ) or help(1);
@@ -144,6 +158,18 @@ if (!$chk_patch && !$chk_branch && !$file) {
 	die "One of --file, --branch, --patch is required\n";
 }
 
+if ($color =~ /^[01]$/) {
+	$color = !$color;
+} elsif ($color =~ /^always$/i) {
+	$color = 1;
+} elsif ($color =~ /^never$/i) {
+	$color = 0;
+} elsif ($color =~ /^auto$/i) {
+	$color = (-t STDOUT);
+} else {
+	die "Invalid color mode: $color\n";
+}
+
 my $dbg_values = 0;
 my $dbg_possible = 0;
 my $dbg_type = 0;
@@ -372,7 +398,9 @@ if ($chk_branch) {
 		close($FILE);
 		$vname = substr($hash, 0, 12) . ' (' . $git_commits{$hash} . ')';
 		if ($num_patches > 1 && $quiet == 0) {
-			print "$i/$num_patches Checking commit $vname\n";
+			my $prefix = "$i/$num_patches";
+			$prefix = BLUE . BOLD . $prefix . RESET if $color;
+			print "$prefix Checking commit $vname\n";
 			$vname = "Patch $i/$num_patches";
 		} else {
 			$vname = "Commit " . $vname;
@@ -1182,14 +1210,23 @@ sub possible {
 my $prefix = '';
 
 sub report {
-	if (defined $tst_only && $_[0] !~ /\Q$tst_only\E/) {
+	my ($level, $msg) = @_;
+	if (defined $tst_only && $msg !~ /\Q$tst_only\E/) {
 		return 0;
 	}
-	my $line = $prefix . $_[0];
 
-	$line = (split('\n', $line))[0] . "\n" if ($terse);
+	my $output = '';
+	$output .= BOLD if $color;
+	$output .= $prefix;
+	$output .= RED if $color && $level eq 'ERROR';
+	$output .= MAGENTA if $color && $level eq 'WARNING';
+	$output .= $level . ':';
+	$output .= RESET if $color;
+	$output .= ' ' . $msg . "\n";
+
+	$output = (split('\n', $output))[0] . "\n" if ($terse);
 
-	push(our @report, $line);
+	push(our @report, $output);
 
 	return 1;
 }
@@ -1197,13 +1234,13 @@ sub report_dump {
 	our @report;
 }
 sub ERROR {
-	if (report("ERROR: $_[0]\n")) {
+	if (report("ERROR", $_[0])) {
 		our $clean = 0;
 		our $cnt_error++;
 	}
 }
 sub WARN {
-	if (report("WARNING: $_[0]\n")) {
+	if (report("WARNING", $_[0])) {
 		our $clean = 0;
 		our $cnt_warn++;
 	}
-- 
2.19.1

Re: [Qemu-devel] [PATCH for-4.0 4/4] checkpatch: colorize output to terminal

[Paolo Bonzini]

On 29/11/18 10:01, Paolo Bonzini wrote:
>  
> +if ($color =~ /^[01]$/) {
> +	$color = !$color;
> +} elsif ($color =~ /^always$/i) {

This first "if" is unnecessary, not sure why Linux has it (probably
because it implements --nocolor differently).

Paolo

Re: [Qemu-devel] [PATCH for-4.0 4/4] checkpatch: colorize output to terminal

[Philippe Mathieu-Daudé]

On 29/11/18 10:01, Paolo Bonzini wrote:
> Add optional colors to make seeing message types a bit easier.
> The default is to show them on a tty.  The chosen colors should resemble
> gcc's.
> 
> Inspired by Linux commit 57230297116faf5b0a995916d5dd5fedab54cba3
> ("checkpatch: colorize output to terminal").
> ---
>  scripts/checkpatch.pl | 51 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 44 insertions(+), 7 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index f5284d910c..14be11719c 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -7,6 +7,7 @@
>  
>  use strict;
>  use warnings;
> +use Term::ANSIColor qw(:constants);
>  
>  my $P = $0;
>  $P =~ s@.*/@@g;
> @@ -26,6 +27,7 @@ my $tst_only;
>  my $emacs = 0;
>  my $terse = 0;
>  my $file = undef;
> +my $color = "auto";
>  my $no_warnings = 0;
>  my $summary = 1;
>  my $mailback = 0;
> @@ -64,6 +66,8 @@ Options:
>                               is all off)
>    --test-only=WORD           report only warnings/errors containing WORD
>                               literally
> +  --color[=WHEN]             Use colors 'always', 'never', or only when output
> +                             is a terminal ('auto'). Default is 'auto'.
>    -h, --help, --version      display this help and exit
>  
>  When FILE is - read standard input.
> @@ -72,6 +76,14 @@ EOM
>  	exit($exitcode);
>  }
>  
> +# Perl's Getopt::Long allows options to take optional arguments after a space.
> +# Prevent --color by itself from consuming other arguments
> +foreach (@ARGV) {
> +	if ($_ eq "--color" || $_ eq "-color") {
> +		$_ = "--color=$color";
> +	}
> +}
> +
>  GetOptions(
>  	'q|quiet+'	=> \$quiet,
>  	'tree!'		=> \$tree,
> @@ -89,6 +101,8 @@ GetOptions(
>  
>  	'debug=s'	=> \%debug,
>  	'test-only=s'	=> \$tst_only,
> +	'color=s'       => \$color,
> +	'no-color'      => sub { $color = 'never'; },
>  	'h|help'	=> \$help,
>  	'version'	=> \$help
>  ) or help(1);
> @@ -144,6 +158,18 @@ if (!$chk_patch && !$chk_branch && !$file) {
>  	die "One of --file, --branch, --patch is required\n";
>  }
>  
> +if ($color =~ /^[01]$/) {
> +	$color = !$color;

Without this if:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +} elsif ($color =~ /^always$/i) {
> +	$color = 1;
> +} elsif ($color =~ /^never$/i) {
> +	$color = 0;
> +} elsif ($color =~ /^auto$/i) {
> +	$color = (-t STDOUT);
> +} else {
> +	die "Invalid color mode: $color\n";
> +}
> +
>  my $dbg_values = 0;
>  my $dbg_possible = 0;
>  my $dbg_type = 0;
> @@ -372,7 +398,9 @@ if ($chk_branch) {
>  		close($FILE);
>  		$vname = substr($hash, 0, 12) . ' (' . $git_commits{$hash} . ')';
>  		if ($num_patches > 1 && $quiet == 0) {
> -			print "$i/$num_patches Checking commit $vname\n";
> +			my $prefix = "$i/$num_patches";
> +			$prefix = BLUE . BOLD . $prefix . RESET if $color;
> +			print "$prefix Checking commit $vname\n";
>  			$vname = "Patch $i/$num_patches";
>  		} else {
>  			$vname = "Commit " . $vname;
> @@ -1182,14 +1210,23 @@ sub possible {
>  my $prefix = '';
>  
>  sub report {
> -	if (defined $tst_only && $_[0] !~ /\Q$tst_only\E/) {
> +	my ($level, $msg) = @_;
> +	if (defined $tst_only && $msg !~ /\Q$tst_only\E/) {
>  		return 0;
>  	}
> -	my $line = $prefix . $_[0];
>  
> -	$line = (split('\n', $line))[0] . "\n" if ($terse);
> +	my $output = '';
> +	$output .= BOLD if $color;
> +	$output .= $prefix;
> +	$output .= RED if $color && $level eq 'ERROR';
> +	$output .= MAGENTA if $color && $level eq 'WARNING';
> +	$output .= $level . ':';
> +	$output .= RESET if $color;
> +	$output .= ' ' . $msg . "\n";
> +
> +	$output = (split('\n', $output))[0] . "\n" if ($terse);
>  
> -	push(our @report, $line);
> +	push(our @report, $output);
>  
>  	return 1;
>  }
> @@ -1197,13 +1234,13 @@ sub report_dump {
>  	our @report;
>  }
>  sub ERROR {
> -	if (report("ERROR: $_[0]\n")) {
> +	if (report("ERROR", $_[0])) {
>  		our $clean = 0;
>  		our $cnt_error++;
>  	}
>  }
>  sub WARN {
> -	if (report("WARNING: $_[0]\n")) {
> +	if (report("WARNING", $_[0])) {
>  		our $clean = 0;
>  		our $cnt_warn++;
>  	}
> 

Re: [Qemu-devel] [PATCH for-4.0 4/4] checkpatch: colorize output to terminal

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 29/11/18 10:01, Paolo Bonzini wrote:
>> Add optional colors to make seeing message types a bit easier.
>> The default is to show them on a tty.  The chosen colors should resemble
>> gcc's.
>> 
>> Inspired by Linux commit 57230297116faf5b0a995916d5dd5fedab54cba3
>> ("checkpatch: colorize output to terminal").

And also commit 737c0767758b "checkpatch: change format of --color
argument to --color[=WHEN]".

Funny:

    ERROR: Missing Signed-off-by: line(s)

    total: 1 errors, 0 warnings, 114 lines checked

>> ---
>>  scripts/checkpatch.pl | 51 +++++++++++++++++++++++++++++++++++++------
>>  1 file changed, 44 insertions(+), 7 deletions(-)
>> 
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index f5284d910c..14be11719c 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -7,6 +7,7 @@
>>  
>>  use strict;
>>  use warnings;
>> +use Term::ANSIColor qw(:constants);
>>  
>>  my $P = $0;
>>  $P =~ s@.*/@@g;
>> @@ -26,6 +27,7 @@ my $tst_only;
>>  my $emacs = 0;
>>  my $terse = 0;
>>  my $file = undef;
>> +my $color = "auto";
>>  my $no_warnings = 0;
>>  my $summary = 1;
>>  my $mailback = 0;
>> @@ -64,6 +66,8 @@ Options:
>>                               is all off)
>>    --test-only=WORD           report only warnings/errors containing WORD
>>                               literally
>> +  --color[=WHEN]             Use colors 'always', 'never', or only when output
>> +                             is a terminal ('auto'). Default is 'auto'.
>>    -h, --help, --version      display this help and exit
>>  
>>  When FILE is - read standard input.
>> @@ -72,6 +76,14 @@ EOM
>>  	exit($exitcode);
>>  }
>>  
>> +# Perl's Getopt::Long allows options to take optional arguments after a space.
>> +# Prevent --color by itself from consuming other arguments
>> +foreach (@ARGV) {
>> +	if ($_ eq "--color" || $_ eq "-color") {
>> +		$_ = "--color=$color";
>> +	}
>> +}
>> +
>>  GetOptions(
>>  	'q|quiet+'	=> \$quiet,
>>  	'tree!'		=> \$tree,
>> @@ -89,6 +101,8 @@ GetOptions(
>>  
>>  	'debug=s'	=> \%debug,
>>  	'test-only=s'	=> \$tst_only,
>> +	'color=s'       => \$color,
>> +	'no-color'      => sub { $color = 'never'; },
>>  	'h|help'	=> \$help,
>>  	'version'	=> \$help
>>  ) or help(1);

Note for next hunk: Linux has

	'color=s'	=> \$color,
	'no-color'	=> \$color,	#keep old behaviors of -nocolor
	'nocolor'	=> \$color,	#keep old behaviors of -nocolor

>> @@ -144,6 +158,18 @@ if (!$chk_patch && !$chk_branch && !$file) {
>>  	die "One of --file, --branch, --patch is required\n";
>>  }
>>  
>> +if ($color =~ /^[01]$/) {
>> +	$color = !$color;
>
> Without this if:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Well, there's value in keeping our checkpatch.pl reasonably close to the
original.  But I agree with Philippe, if we deviate in the argument of
GetOptions, it makes sense to deviate here, too.

>> +} elsif ($color =~ /^always$/i) {
>> +	$color = 1;
>> +} elsif ($color =~ /^never$/i) {
>> +	$color = 0;
>> +} elsif ($color =~ /^auto$/i) {
>> +	$color = (-t STDOUT);
>> +} else {
>> +	die "Invalid color mode: $color\n";
>> +}
>> +
>>  my $dbg_values = 0;
>>  my $dbg_possible = 0;
>>  my $dbg_type = 0;
>> @@ -372,7 +398,9 @@ if ($chk_branch) {
>>  		close($FILE);
>>  		$vname = substr($hash, 0, 12) . ' (' . $git_commits{$hash} . ')';
>>  		if ($num_patches > 1 && $quiet == 0) {
>> -			print "$i/$num_patches Checking commit $vname\n";
>> +			my $prefix = "$i/$num_patches";
>> +			$prefix = BLUE . BOLD . $prefix . RESET if $color;
>> +			print "$prefix Checking commit $vname\n";
>>  			$vname = "Patch $i/$num_patches";
>>  		} else {
>>  			$vname = "Commit " . $vname;

Linux doesn't have this hunk, because it doesn't have all of your
(Linux-inspired) PATCH 3.  Okay, but it makes me wonder whether the
missing parts would make sense for Linux's checkpatch.pl, too.

>> @@ -1182,14 +1210,23 @@ sub possible {
>>  my $prefix = '';
>>  
>>  sub report {
>> -	if (defined $tst_only && $_[0] !~ /\Q$tst_only\E/) {
>> +	my ($level, $msg) = @_;
>> +	if (defined $tst_only && $msg !~ /\Q$tst_only\E/) {
>>  		return 0;
>>  	}
>> -	my $line = $prefix . $_[0];
>>  
>> -	$line = (split('\n', $line))[0] . "\n" if ($terse);
>> +	my $output = '';
>> +	$output .= BOLD if $color;
>> +	$output .= $prefix;
>> +	$output .= RED if $color && $level eq 'ERROR';
>> +	$output .= MAGENTA if $color && $level eq 'WARNING';
>> +	$output .= $level . ':';
>> +	$output .= RESET if $color;
>> +	$output .= ' ' . $msg . "\n";

More idiomatic than Linux's nested if.  Worth the deviation?

Hmm, the colors differ, too:

             Your patch   Linux
    ERROR    RED          RED
    WARNING  MAGENTA      YELLOW
    other                 GREEN

In addition, you use BOLD.  Worth the deviation?

Note that I'm not asking for a debate here, I merely want you to
consider the tradeoff.  "Yes" would be a sufficient answer as far as I'm
concerned :)

>> +
>> +	$output = (split('\n', $output))[0] . "\n" if ($terse);
>>  
>> -	push(our @report, $line);
>> +	push(our @report, $output);
>>  
>>  	return 1;
>>  }
>> @@ -1197,13 +1234,13 @@ sub report_dump {
>>  	our @report;
>>  }
>>  sub ERROR {
>> -	if (report("ERROR: $_[0]\n")) {
>> +	if (report("ERROR", $_[0])) {
>>  		our $clean = 0;
>>  		our $cnt_error++;
>>  	}
>>  }
>>  sub WARN {
>> -	if (report("WARNING: $_[0]\n")) {
>> +	if (report("WARNING", $_[0])) {
>>  		our $clean = 0;
>>  		our $cnt_warn++;
>>  	}
>> 

Re: [Qemu-devel] [PATCH for-4.0 0/4] Small checkpatch fixes and improvements

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:



Type: series
Subject: [Qemu-devel] [PATCH for-4.0 0/4] Small checkpatch fixes and improvements
Message-id: 20181129090120.28828-1-pbonzini@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
82caf95 checkpatch: colorize output to terminal
df8393a checkpatch: improve handling of multiple patches or files
b98039a checkpatch: check Signed-off-by in --mailback mode
f2c33c2 checkpatch: fix premature exit when no input or --mailback

=== OUTPUT BEGIN ===
Checking PATCH 1/4: checkpatch: fix premature exit when no input or --mailback...
Checking PATCH 2/4: checkpatch: check Signed-off-by in --mailback mode...
Checking PATCH 3/4: checkpatch: improve handling of multiple patches or files...
ERROR: line over 90 characters
#33: FILE: scripts/checkpatch.pl:345:
+       open($HASH, "-|", "git", "log", "--reverse", "--no-merges", "--format=%H %s", $ARGV[0]) ||

ERROR: line over 90 characters
#34: FILE: scripts/checkpatch.pl:346:
+               die "$P: git log --reverse --no-merges --format='%H %s' $ARGV[0] failed - $!\n";

WARNING: line over 80 characters
#65: FILE: scripts/checkpatch.pl:373:
+               $vname = substr($hash, 0, 12) . ' (' . $git_commits{$hash} . ')';

total: 2 errors, 1 warnings, 62 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 4/4: checkpatch: colorize output to terminal...
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 114 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH for-4.0 0/4] Small checkpatch fixes and improvements

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:



Type: series
Subject: [Qemu-devel] [PATCH for-4.0 0/4] Small checkpatch fixes and improvements
Message-id: 20181129090120.28828-1-pbonzini@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20181130192216.26987-1-richard.henderson@linaro.org -> patchew/20181130192216.26987-1-richard.henderson@linaro.org
Switched to a new branch 'test'
82caf95 checkpatch: colorize output to terminal
df8393a checkpatch: improve handling of multiple patches or files
b98039a checkpatch: check Signed-off-by in --mailback mode
f2c33c2 checkpatch: fix premature exit when no input or --mailback

=== OUTPUT BEGIN ===
Checking PATCH 1/4: checkpatch: fix premature exit when no input or --mailback...
Checking PATCH 2/4: checkpatch: check Signed-off-by in --mailback mode...
Checking PATCH 3/4: checkpatch: improve handling of multiple patches or files...
ERROR: line over 90 characters
#33: FILE: scripts/checkpatch.pl:345:
+       open($HASH, "-|", "git", "log", "--reverse", "--no-merges", "--format=%H %s", $ARGV[0]) ||

ERROR: line over 90 characters
#34: FILE: scripts/checkpatch.pl:346:
+               die "$P: git log --reverse --no-merges --format='%H %s' $ARGV[0] failed - $!\n";

WARNING: line over 80 characters
#65: FILE: scripts/checkpatch.pl:373:
+               $vname = substr($hash, 0, 12) . ' (' . $git_commits{$hash} . ')';

total: 2 errors, 1 warnings, 62 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 4/4: checkpatch: colorize output to terminal...
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 114 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com