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
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++;
> }
>
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++;
>> }
>>
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
© 2016 - 2026 Red Hat, Inc.