linux-kernel - Re: [PATCH v2] checkpatch: Change format of --color argument to --color[=WHEN]

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 6 Jun 2017 19:56:52 +0000
From:   John Brooks <john@...tquake.com>
To:     Joe Perches <joe@...ches.com>
Cc:     Andy Whitcroft <apw@...onical.com>, linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: 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)
>    --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, '-');
>  }

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