Re: [PATCH] log --format: teach %C(auto,black) to paint it black only on terminals

From: Jeff King <peff@peff.net>
Date: Dec 17, 2012, 19:49 UTC
Message-ID: <20121217194926.GA5209@sigill.intra.peff.net>
In-Reply-To: <7v4njkmq07.fsf@alter.siamese.dyndns.org>

On Mon, Dec 17, 2012 at 11:34:48AM -0800, Junio C Hamano wrote:

Show 27 quoted lines

> > Yeah, that should definitely be documented. I wonder if it should
> > actually respect color.diff, which is what "log" usually uses (albeit
> > mostly for the diff itself, we have always used it for the graph and for
> > the "commit" header of each entry).
> 
> I actually do not like this patch very much.  The original motive
> behind this "auto" thing was to relieve the script writers from
> the burden of having to write:
> 
> 	if tty -s
>         then
>         	warn='%C(red)'
>                 reset='%C(reset)'
> 	else
>         	warn= reset=
> 	fi
>         fmt="${warn}WARNING: boo${reset} %s"
> 
> and instead let them write:
> 
> 	fmt="%C(auto,red)WARNING: boo%C(auto,reset) %s"
> 
> but between the two, there is no $cmd.color configuration involved
> in the first place.  I am not sure what $cmd.color configuration
> should take effect---perhaps for a "git frotz" script, we should
> allow the script writer to honor frotz.color=(auto,never,always)
> configuration, not just ui.color variable.

Since this is by definition just talking about the log traversal, I think it makes sense to respect the log traversal option by default (which is confusingly color.diff, of course, but that is a separate issue). That means that scripts which just wrap a regular traversal will do what the user is accustomed to.

If "git frotz" wants to have a separate "color.frotz" option to override that, then they would need to implement that themselves either with or without your patch. I do not think its presence makes things any harder.

Show 15 quoted lines

> Also the patch as posted deliberately omits support to honor command
> line override --color=(auto,never,always), but it may be more
> natural to expect
> 
>     git show --format='%C(auto,red)%s%C(auto,reset)' --color=never
> 
> to defeat the "auto," part the script writer wrote.
> 
> Now, such a script would be run by its end users as
> 
>     $ git frotz --color=never
> 
> It has to do its own option parsing before running the underlying
> "git show" to decide if it passes "--color=never" from the command
> line for that to happen.

Yeah, _if_ it does not just pass its options directly to git-log. Which I think a reasonable number of scripts do, as well as pretty.* aliases (which are not really scripts, but have the same relationship with respect to this feature). For example, "git stash list" does not use color in its output, but could be improved to do so after your patch.

So no, I do not think you can cover every conceivable case. But having git-log respect --color and the usual color.* variables for this feature seems like the only sane default. It makes the easy cases just work, and the hard cases are not any worse off (and they may even be better off, since the script can manipulate --color instead of rewriting their format strings, but that is a minor difference).

-Peff