Philippe Mathieu-Daudé <phi...@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é <phi...@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++; >> } >>