checkpatch: Change format of --color argument to --color[=WHEN]
PATCH 1/1 checkpatch: Change format of --color argument to --color[=WHEN]
The boolean --color argument did not offer the ability to force colourized output even if stdout is not a terminal. Change the format of the argument to the familiar --color[=WHEN] construct as seen in common Linux utilities such as ls and dmesg, which allows the user to specify whether to colourize output always, never, or only when the output is a terminal ("auto").
Because the option is no longer boolean, --nocolor (or --no-color) is no longer available. Users of the old negative option should use --color=never instead.
--- scripts/checkpatch.pl | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)
scripts/checkpatch.pl +17 -6
@@ -56,7 +56,7 @@ my $codespell = 0; my $codespellfile = "/usr/share/codespell/dictionary.txt"; my $conststructsfile = "$D/const_structs.checkpatch"; my $typedefsfile = ""; -my $color = 1; +my $color = "auto"; my $allow_c99_comments = 1; sub help {
@@ -115,7 +115,8 @@ Options: (default:/usr/share/codespell/dictionary.txt) --codespellfile Use this codespell dictionary --typedefsfile Read additional types from this file - --color Use colors when output is STDOUT (default: on) + --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.
@@ -211,7 +212,7 @@ GetOptions( 'codespell!' => \$codespell, 'codespellfile=s' => \$codespellfile, 'typedefsfile=s' => \$typedefsfile, - 'color!' => \$color, + 'color:s' => \$color, 'h|help' => \$help, 'version' => \$help ) or help(1);
@@ -237,6 +238,16 @@ if ($#ARGV < 0) { push(@ARGV, '-'); } +if ($color eq "always") { + $color = 1; +} elsif ($color eq "never") { + $color = 0; +} elsif ($color eq "auto") { + $color = (-t STDOUT); +} else { + die "Invalid color mode: $color\n"; +} + sub hash_save_array_words { my ($hashRef, $arrayRef) = @_;
@@ -1881,7 +1892,7 @@ sub report { return 0; } my $output = ''; - if (-t STDOUT && $color) { + if ($color) { if ($level eq 'ERROR') { $output .= RED; } elsif ($level eq 'WARNING') {
@@ -1892,10 +1903,10 @@ sub report { } $output .= $prefix . $level . ':'; if ($show_types) { - $output .= BLUE if (-t STDOUT && $color); + $output .= BLUE if ($color); $output .= "$type:"; } - $output .= RESET if (-t STDOUT && $color); + $output .= RESET if ($color); $output .= ' ' . $msg . "\n"; if ($showfile) {
Signature · 1 line
2.7.4
Joe Perches
maintainerQuoted 2 lines · John Brooks
The boolean --color argument did not offer the ability to force colourized output even if stdout is not a terminal.
OK, but why is colorizing output not to terminals desired?
Quoted 8 lines
Change the format of the argument
to the familiar --color[=WHEN] construct as seen in common Linux utilities such as ls and dmesg, which allows the user to specify whether to colourize output always, never, or only when the output is a terminal ("auto").Because the option is no longer boolean, --nocolor (or --no-color) is no longer available. Users of the old negative option should use --color=never instead.
In general, I don't mind, but perhaps this option name could/should change.
As is, this also causes a previous command line that worked with --color to fail
$ ./scripts/checkpatch.pl --color foo.patch Invalid color mode: foo.patch
Quoted 5 lines · Joe Perches
Quoted 2 lines · John Brooks
The boolean --color argument did not offer the ability to force colourized output even if stdout is not a terminal.
OK, but why is colorizing output not to terminals desired?
For example, to retain coloured output when using a pager (such as less -R). Which is convenient for viewing/searching lengthy output from larger patch sets, or when one is using something that interferes with the ability to scroll such as screen, tmux, or mosh.
Quoted 10 lines
Quoted 9 lines
Change the format of the argument
to the familiar --color[=WHEN] construct as seen in common Linux utilities such as ls and dmesg, which allows the user to specify whether to colourize output always, never, or only when the output is a terminal ("auto").Because the option is no longer boolean, --nocolor (or --no-color) is no longer available. Users of the old negative option should use --color=never instead.
Quoted 9 lines
In general, I don't mind, but perhaps this option name could/should change.
As is, this also causes a previous command line that worked with --color to fail
$ ./scripts/checkpatch.pl --color foo.patch Invalid color mode: foo.patch
Oh, that's pretty bad. I should have thought of that, sorry. I'll see what I can do to stop it from eating other arguments.
--
John Brooks
Quoted 5 lines · Joe Perches
Quoted 2 lines · John Brooks
The boolean --color argument did not offer the ability to force colourized output even if stdout is not a terminal.
OK, but why is colorizing output not to terminals desired?
* You may post-process the output somehow. grep, sed, some highlighter...
* The output may go to less -R, ansi2html, etc.
I've made a tool that does what you want, "pipetty" (Debian package colorized-logs, in stretch and jessie-backports), but that's a dirty hack. Lying about isatty() works for programs that check STDOUT but it's notorious to instead look at STDIN, which can't be fooled in a reliable way. Thus, it's better to standardize on --color={always,auto,never}.
Meow!
Signature · 4 lines
⢀⣴⠾⠻⢶⣦⠀ A tit a day keeps the vet away. ⣾⠁⢰⠒⠀⣿⡁ ⢿⡄⠘⠷⠚⠋⠀ (Rejoice as my small-animal-murder-machine got unbroken after ⠈⠳⣄⠀⠀⠀⠀ nearly two years of no catch!)
PATCH v2 1/1 checkpatch: Change format of --color argument to --color[=WHEN]
In reply to Joe PerchesThe boolean --color argument did not offer the ability to force colourized output even if stdout is not a terminal. Change the format of the argument to the familiar --color[=WHEN] construct as seen in common Linux utilities such as ls and dmesg, which allows the user to specify whether to colourize output always, never, or only when the output is a terminal ("auto").
Because the option is no longer boolean, --nocolor (or --no-color) is no longer available. Users of the old negative option should use --color=never instead.
v2 (Joe Perches):
- Prevent --color from consuming other args in e.g. ./scripts/checkpatch.pl --color foo.patch
--- scripts/checkpatch.pl | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-)
scripts/checkpatch.pl +25 -6
@@ -56,7 +56,7 @@ my $codespell = 0; my $codespellfile = "/usr/share/codespell/dictionary.txt"; my $conststructsfile = "$D/const_structs.checkpatch"; my $typedefsfile = ""; -my $color = 1; +my $color = "auto"; my $allow_c99_comments = 1; sub help {
@@ -115,7 +115,8 @@ Options: (default:/usr/share/codespell/dictionary.txt) --codespellfile Use this codespell dictionary --typedefsfile Read additional types from this file - --color Use colors when output is STDOUT (default: on) + --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.
@@ -181,6 +182,14 @@ if (-f $conf) { unshift(@ARGV, @conf_args) if @conf_args; } +# 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") { + $_ = "--color=$color"; + } +} + GetOptions( 'q|quiet+' => \$quiet, 'tree!' => \$tree,
@@ -211,7 +220,7 @@ GetOptions( 'codespell!' => \$codespell, 'codespellfile=s' => \$codespellfile, 'typedefsfile=s' => \$typedefsfile, - 'color!' => \$color, + 'color:s' => \$color, 'h|help' => \$help, 'version' => \$help ) or help(1);
@@ -237,6 +246,16 @@ if ($#ARGV < 0) { push(@ARGV, '-'); } +if ($color eq "always") { + $color = 1; +} elsif ($color eq "never") { + $color = 0; +} elsif ($color eq "auto") { + $color = (-t STDOUT); +} else { + die "Invalid color mode: $color\n"; +} + sub hash_save_array_words { my ($hashRef, $arrayRef) = @_;
@@ -1881,7 +1900,7 @@ sub report { return 0; } my $output = ''; - if (-t STDOUT && $color) { + if ($color) { if ($level eq 'ERROR') { $output .= RED; } elsif ($level eq 'WARNING') {
@@ -1892,10 +1911,10 @@ sub report { } $output .= $prefix . $level . ':'; if ($show_types) { - $output .= BLUE if (-t STDOUT && $color); + $output .= BLUE if ($color); $output .= "$type:"; } - $output .= RESET if (-t STDOUT && $color); + $output .= RESET if ($color); $output .= ' ' . $msg . "\n"; if ($showfile) {
Signature · 1 line
2.7.4
Joe Perches
maintainerQuoted 9 lines · John Brooks
The boolean --color argument did not offer the ability to force colourized output even if stdout is not a terminal. Change the format of the argument to the familiar --color[=WHEN] construct as seen in common Linux utilities such as ls and dmesg, which allows the user to specify whether to colourize output always, never, or only when the output is a terminal ("auto").
Because the option is no longer boolean, --nocolor (or --no-color) is no longer available. Users of the old negative option should use --color=never instead.
It is possible to add --nocolor and --no-color to the arguments for GetOptions to keep the old behavior intact.
I think this works:
--- scripts/checkpatch.pl | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-)
scripts/checkpatch.pl +29 -6
@@ -56,7 +56,7 @@ my $codespell = 0; my $codespellfile = "/usr/share/codespell/dictionary.txt"; my $conststructsfile = "$D/const_structs.checkpatch"; my $typedefsfile = ""; -my $color = 1; +my $color = "auto"; my $allow_c99_comments = 1; sub help {
@@ -115,7 +115,8 @@ Options: (default:/usr/share/codespell/dictionary.txt) --codespellfile Use this codespell dictionary --typedefsfile Read additional types from this file - --color Use colors when output is STDOUT (default: on) + --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.
@@ -181,6 +182,14 @@ if (-f $conf) { unshift(@ARGV, @conf_args) if @conf_args; } +# 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,
@@ -211,7 +220,9 @@ GetOptions( 'codespell!' => \$codespell, 'codespellfile=s' => \$codespellfile, 'typedefsfile=s' => \$typedefsfile, - 'color!' => \$color, + 'color=s' => \$color, + '-no-color!' => \$color, #keep old behaviors of -nocolor + '-nocolor!' => \$color, #keep old behaviors of -nocolor 'h|help' => \$help, 'version' => \$help ) or help(1);
@@ -237,6 +248,18 @@ if ($#ARGV < 0) { push(@ARGV, '-'); } +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"; +} + sub hash_save_array_words { my ($hashRef, $arrayRef) = @_;
@@ -1881,7 +1904,7 @@ sub report { return 0; } my $output = ''; - if (-t STDOUT && $color) { + if ($color) { if ($level eq 'ERROR') { $output .= RED; } elsif ($level eq 'WARNING') {
@@ -1892,10 +1915,10 @@ sub report { } $output .= $prefix . $level . ':'; if ($show_types) { - $output .= BLUE if (-t STDOUT && $color); + $output .= BLUE if ($color); $output .= "$type:"; } - $output .= RESET if (-t STDOUT && $color); + $output .= RESET if ($color); $output .= ' ' . $msg . "\n"; if ($showfile) {
Quoted 69 lines · Joe Perches
Quoted 9 lines · John Brooks
The boolean --color argument did not offer the ability to force colourized output even if stdout is not a terminal. Change the format of the argument to the familiar --color[=WHEN] construct as seen in common Linux utilities such as ls and dmesg, which allows the user to specify whether to colourize output always, never, or only when the output is a terminal ("auto").
Because the option is no longer boolean, --nocolor (or --no-color) is no longer available. Users of the old negative option should use --color=never instead.
It is possible to add --nocolor and --no-color to the arguments for GetOptions to keep the old behavior intact.
I think this works:
--- scripts/checkpatch.pl | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-)scripts/checkpatch.pl +14
-3@@ -56,7 +56,7 @@ my $codespell = 0; my $codespellfile = "/usr/share/codespell/dictionary.txt"; my $conststructsfile = "$D/const_structs.checkpatch"; my $typedefsfile = ""; -my $color = 1; +my $color = "auto"; my $allow_c99_comments = 1; sub help {@@ -115,7 +115,8 @@ Options: (default:/usr/share/codespell/dictionary.txt) --codespellfile Use this codespell dictionary --typedefsfile Read additional types from this file - --color Use colors when output is STDOUT (default: on) + --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.@@ -181,6 +182,14 @@ if (-f $conf) { unshift(@ARGV, @conf_args) if @conf_args; } +# 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,@@ -211,7 +220,9 @@ GetOptions( 'codespell!' => \$codespell, 'codespellfile=s' => \$codespellfile, 'typedefsfile=s' => \$typedefsfile, - 'color!' => \$color, + 'color=s' => \$color, + '-no-color!' => \$color, #keep old behaviors of -nocolor + '-nocolor!' => \$color, #keep old behaviors of -nocolor 'h|help' => \$help, 'version' => \$help ) or help(1);
scripts/checkpatch.pl if ($#ARGV < 0) { · line 249
Quoted 2 lines
scripts/checkpatch.pl +0
-0@@ -237,6 +248,18 @@ if ($#ARGV < 0) { push(@ARGV, '-'); }
Good changes overall. Does one want the leading dash and trailing bang here, however? I don't know what the leading dash does (I would guess it makes it store 0 into the variable? I can't find anything in the perldoc), but the bang makes the option negatable, which would allow you to do --nonocolor/ --nono-color, and that may not make sense here.
Quoted 28 lines
scripts/checkpatch.pl +13
-1+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"; +} + sub hash_save_array_words { my ($hashRef, $arrayRef) = @_;@@ -1881,7 +1904,7 @@ sub report { return 0; } my $output = ''; - if (-t STDOUT && $color) { + if ($color) { if ($level eq 'ERROR') { $output .= RED; } elsif ($level eq 'WARNING') {@@ -1892,10 +1915,10 @@ sub report { } $output .= $prefix . $level . ':';
scripts/checkpatch.pl sub report { · line 1924
Quoted 10 lines
scripts/checkpatch.pl +2
-2if ($show_types) { - $output .= BLUE if (-t STDOUT && $color); + $output .= BLUE if ($color); $output .= "$type:"; } - $output .= RESET if (-t STDOUT && $color); + $output .= RESET if ($color); $output .= ' ' . $msg . "\n"; if ($showfile) {
Everything else looks good to me.
Thanks
John
Quoted 58 lines · John Brooks
Quoted 57 lines · Joe Perches
Quoted 9 lines · John Brooks
The boolean --color argument did not offer the ability to force colourized output even if stdout is not a terminal. Change the format of the argument to the familiar --color[=WHEN] construct as seen in common Linux utilities such as ls and dmesg, which allows the user to specify whether to colourize output always, never, or only when the output is a terminal ("auto").
Because the option is no longer boolean, --nocolor (or --no-color) is no longer available. Users of the old negative option should use --color=never instead.
It is possible to add --nocolor and --no-color to the arguments for GetOptions to keep the old behavior intact.
I think this works:
--- scripts/checkpatch.pl | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-)scripts/checkpatch.pl +11
-2@@ -56,7 +56,7 @@ my $codespell = 0; my $codespellfile = "/usr/share/codespell/dictionary.txt"; my $conststructsfile = "$D/const_structs.checkpatch"; my $typedefsfile = ""; -my $color = 1; +my $color = "auto"; my $allow_c99_comments = 1; sub help {@@ -115,7 +115,8 @@ Options: (default:/usr/share/codespell/dictionary.txt) --codespellfile Use this codespell dictionary --typedefsfile Read additional types from this file - --color Use colors when output is STDOUT (default: on) + --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.@@ -181,6 +182,14 @@ if (-f $conf) { unshift(@ARGV, @conf_args) if @conf_args; } +# 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,
Quoted 8 lines
scripts/checkpatch.pl GetOptions( · line 225
Quoted 8 lines
scripts/checkpatch.pl +3
-1@@ -211,7 +220,9 @@ GetOptions( 'codespell!' => \$codespell, 'codespellfile=s' => \$codespellfile, 'typedefsfile=s' => \$typedefsfile, - 'color!' => \$color, + 'color=s' => \$color, + '-no-color!' => \$color, #keep old behaviors of -nocolor + '-nocolor!' => \$color, #keep old behaviors of -nocolor
[]
Quoted 2 lines
Good changes overall. Does one want the leading dash and trailing bang here, however?
You're probably right about the trailing bang.
Quoted 3 lines
I don't know what the leading dash does (I would guess it makes it
store 0 into the variable?
I can't find anything in the perldoc)
No idea. Me neither.
Quoted 3 lines
but the bang
makes the option negatable, which would allow you to do --nonocolor/ --nono-color, and that may not make sense here.
Right. The bang should be removed.
When you submit a V3 with the appropriate changes,
cheers, Joe
Joe Perches
maintainerPATCH v3 1/1 checkpatch: Change format of --color argument to --color[=WHEN]
From: John Brooks <[email protected]>
The boolean --color argument did not offer the ability to force colourized output even if stdout is not a terminal. Change the format of the argument to the familiar --color[=WHEN] construct as seen in common Linux utilities such as git, ls and dmesg, which allows the user to specify whether to colourize output "always", "never", or "auto" when the output is a terminal. The default is "auto".
The old command-line uses of --color and --no-color are unchanged.
--- scripts/checkpatch.pl | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-)
scripts/checkpatch.pl +29 -6
@@ -57,7 +57,7 @@ my $codespell = 0; my $codespellfile = "/usr/share/codespell/dictionary.txt"; my $conststructsfile = "$D/const_structs.checkpatch"; my $typedefsfile = ""; -my $color = 1; +my $color = "auto"; my $allow_c99_comments = 1; sub help {
@@ -116,7 +116,8 @@ Options: (default:/usr/share/codespell/dictionary.txt) --codespellfile Use this codespell dictionary --typedefsfile Read additional types from this file - --color Use colors when output is STDOUT (default: on) + --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.
@@ -182,6 +183,14 @@ if (-f $conf) { unshift(@ARGV, @conf_args) if @conf_args; } +# 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,
@@ -212,7 +221,9 @@ GetOptions( 'codespell!' => \$codespell, 'codespellfile=s' => \$codespellfile, 'typedefsfile=s' => \$typedefsfile, - 'color!' => \$color, + 'color=s' => \$color, + 'no-color' => \$color, #keep old behaviors of -nocolor + 'nocolor' => \$color, #keep old behaviors of -nocolor 'h|help' => \$help, 'version' => \$help ) or help(1);
@@ -238,6 +249,18 @@ if ($#ARGV < 0) { push(@ARGV, '-'); } +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"; +} + sub hash_save_array_words { my ($hashRef, $arrayRef) = @_;
@@ -1882,7 +1905,7 @@ sub report { return 0; } my $output = ''; - if (-t STDOUT && $color) { + if ($color) { if ($level eq 'ERROR') { $output .= RED; } elsif ($level eq 'WARNING') {
@@ -1893,10 +1916,10 @@ sub report { } $output .= $prefix . $level . ':'; if ($show_types) { - $output .= BLUE if (-t STDOUT && $color); + $output .= BLUE if ($color); $output .= "$type:"; } - $output .= RESET if (-t STDOUT && $color); + $output .= RESET if ($color); $output .= ' ' . $msg . "\n"; if ($showfile) {
Signature · 1 line
2.10.0.rc2.1.g053435c
Quoted 9 lines · Joe Perches
From: John Brooks <[email protected]>
The boolean --color argument did not offer the ability to force colourized output even if stdout is not a terminal. Change the format of the argument to the familiar --color[=WHEN] construct as seen in common Linux utilities such as git, ls and dmesg, which allows the user to specify whether to colourize output "always", "never", or "auto" when the output is a terminal. The default is "auto".
Quoted 4 lines
The old command-line uses of --color and --no-color are unchanged.
Signed-off-by John Brooks <[email protected]>Signed-off-by Joe Perches <[email protected]>
Thanks for doing the V3 for me :)
I was going to but had other work to do last night.
John