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

[Markus Armbruster]

Philippe Mathieu-Daudé <address@hidden> 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 =~ address@hidden/@@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é <address@hidden>

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++;
>>      }
>>