On Thu, May 26, 2016 at 5:39 AM, Jeff King <peff@xxxxxxxx> wrote: > On Tue, May 24, 2016 at 08:56:49PM -0500, Edward Thomson wrote: > >> Check that we are configured to display colors in the given context when >> the user specifies a format string of `%C(auto)`. This brings that >> behavior in line with the behavior of `%C(auto,<colorname>)`, which will >> display the given color only when the configuration specifies to do so. >> >> This allows the user the ability to specify that color should be >> displayed only when the output is a tty, and to use the default color >> for the given context (instead of a hardcoded color value). >> >> Signed-off-by: Edward Thomson <ethomson@xxxxxxxxxxxxxxxxx> > > I somehow had trouble figuring out the problem from this description and > the patch. It seems to be about much more than just color=auto or a > given context, and more like: > > When %C(auto) is used, we unconditionally turn on color for any > subsequent placeholders, even if the user said "--no-color", or color > config is turned off, or it is set to "auto" and we are not going to a > tty. I think the (old) "auto" here means "automatically select the color" and what you do would be equivalent to %(auto,auto) where the first (and new) "auto" is about on/off switch, and the second is about selecting the actual color. > It's possible somebody is relying on the ability to unconditionally turn > on color for "auto-colored" placeholders like "%H" or "%d", but I'm > inclined to call this a strict bug-fix, for two reasons: > > 1. It says "%C(auto)", not "%C(on)". > > 2. This is documented as behaving like "%C(auto,...)", which as you > note works in a more sane way. > > I think it's worth mentioning this explicitly in the commit message. We > could also add "%C(on)", I guess, but it's unclear to me whether anybody > would want it (they would probably just use "--color" in that case, > unless they really want unconditional coloring for just _some_ > elements). If I could redo, I would go with %C(default) instead of %C(auto) then we could have %C(auto,default). Perhaps we can make %C(auto) an equivalent of %C(auto,default) now (i.e. exactly what this patch does) and at some point in future add %C(default) which is what %C(auto) is now if people really need to force it on? -- Duy -- 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