[PATCH v2] 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.
v2 (Joe Perches):
- Prevent --color from consuming other args in e.g.
./scripts/checkpatch.pl --color foo.patch
Signed-off-by: John Brooks
---
scripts/checkpatch.pl | 31 +--
1 file changed, 25 insertions(+), 6 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4b9569f..9d03ae0 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -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)
--codespellfileUse this codespell dictionary
--typedefsfile Read additional types from this file
- --colorUse 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) {
--
2.7.4
Re: [PATCH v2] checkpatch: Change format of --color argument to --color[=WHEN]
On Tue, 2017-06-06 at 13:07 -0400, John Brooks wrote:
> 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(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4b9569fa931b..372d541c2c46 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -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)
--codespellfileUse this codespell dictionary
--typedefsfile Read additional types from this file
- --colorUse 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) {
Re: [PATCH v2] checkpatch: Change format of --color argument to --color[=WHEN]
On Tue, Jun 06, 2017 at 12:21:22PM -0700, Joe Perches wrote:
> On Tue, 2017-06-06 at 13:07 -0400, John Brooks wrote:
> > 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(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 4b9569fa931b..372d541c2c46 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -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)
>--codespellfileUse this codespell dictionary
>--typedefsfile Read additional types from this file
> - --colorUse 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, '-');
> }
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.
>
> +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) {
Everything else looks good to me.
Thanks
John
Re: [PATCH v2] checkpatch: Change format of --color argument to --color[=WHEN]
On Tue, 2017-06-06 at 19:56 +, John Brooks wrote:
> On Tue, Jun 06, 2017 at 12:21:22PM -0700, Joe Perches wrote:
> > On Tue, 2017-06-06 at 13:07 -0400, John Brooks wrote:
> > > 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(-)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 4b9569fa931b..372d541c2c46 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -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)
> >--codespellfileUse this codespell dictionary
> >--typedefsfile Read additional types from this file
> > - --colorUse 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
[]
> Good changes overall. Does one want the leading dash and trailing bang here,
> however?
You're probably right about the trailing bang.
> 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.
> 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,
Acked-by: Joe Perches
cheers, Joe
