Re: [PATCH] Add an optional argument for --color options

From: Jeff King <peff@peff.net>
Date: Feb 14, 2010, 06:44 UTC
Message-ID: <20100214064408.GB20630@coredump.intra.peff.net>
In-Reply-To: <1266098475-21929-1-git-send-email-lodatom@gmail.com>

On Sat, Feb 13, 2010 at 05:01:15PM -0500, Mark Lodato wrote:

Show 7 quoted lines

> Make git-branch, git-show-branch, git-grep, and all the diff-based
> programs accept an optional argument <when> for --color.  The argument
> is a colorbool: "always", "never", or "auto".  If no argument is given,
> "always" is used;  --no-color is an alias for --color=never.  This makes
> the command-line interface consistent with other GNU tools, such as `ls'
> and `grep', and with the git-config color options.  Note that, without
> an argument, --color and --no-color work exactly as before.

I think this is a sensible change, and reading over the patch it looks fine to me.

> If the argument is not valid for a diff-family program, a completely
> unhelpful usage message is shown.  It seems that all the other diff
> options silently ignore invalid inputs, so this is consistent.  Perhaps
> this aspect should be tweaked.

Hmm...the only one I see that silently ignores is "--submodule=bogus". But it seems that "git log -Bfoobar" fails but does not print a useful message. Probably both should be fixed, and your option should follow the same convention as those.

>  Documentation/technical/api-parse-options.txt |   12 ++++++++++++

Ooh, api documentation. It is nice to review a patch that is thorough. :)

My only complaint in that respect is that there are no tests. However, I'm not sure we can get a very satisfying test, since the test scripts may or may not have stdout going to a tty.

-Peff