On Mon, Dec 17, 2012 at 11:34:48AM -0800, Junio C Hamano wrote:
> > 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.
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html