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

[Joe Perches]

On Tue, 2017-06-06 at 19:56 +0000, 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)
> >    --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

[]

> 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 <joe@xxxxxxxxxxx>

cheers, Joe