[PATCH/RFC] "color.diff = true" is not "always" anymore.

Too many people got burned by setting color.diff and color.status to
true when they really should have set it to "auto".

This makes only "always" to do the unconditional colorization, and
change the meaning of "true" to the same as "auto": colorize only when
we are talking to a terminal.

Signed-off-by: Junio C Hamano <redacted>
---

 * This is definitely a backward incompatible change, but I think it is
   only in a good way.  Are there people who have "color.* = true" and
   do mean it?  If we do this, they need to change their configuration
   and use "always", but I suspect there is no sane workflow that wants
   the color escape code in files (e.g. "git log >file") or pipes
   (e.g. "git diff | grep foo") by default, in which case this won't
   hurt anybody and would help countless normal people who were bitten
   by the mistaken meaning originally chosen for "true".

 color.c |   32 +++++++++++++++++++-------------
 1 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/color.c b/color.c
index 124ba33..97cfbda 100644
--- a/color.c
+++ b/color.c

@@ -118,21 +118,27 @@ bad:
 
 int git_config_colorbool(const char *var, const char *value)
 {
-	if (!value)
-		return 1;
-	if (!strcasecmp(value, "auto")) {
-		if (isatty(1) || (pager_in_use && pager_use_color)) {
-			char *term = getenv("TERM");
-			if (term && strcmp(term, "dumb"))
-				return 1;
-		}
-		return 0;
+	if (value) {
+		if (!strcasecmp(value, "never"))
+			return 0;
+		if (!strcasecmp(value, "always"))
+			return 1;
+		if (!strcasecmp(value, "auto"))
+			goto auto_color;
 	}
-	if (!strcasecmp(value, "never"))
+
+	/* Missing or explicit false to turn off colorization */
+	if (!git_config_bool(var, value))
 		return 0;
-	if (!strcasecmp(value, "always"))
-		return 1;
-	return git_config_bool(var, value);
+
+	/* any normal truth value defaults to 'auto' */
+ auto_color:
+	if (isatty(1) || (pager_in_use && pager_use_color)) {
+		char *term = getenv("TERM");
+		if (term && strcmp(term, "dumb"))
+			return 1;
+	}
+	return 0;
 }
 
 static int color_vfprintf(FILE *fp, const char *color, const char *fmt,

-- 
1.5.3.6.2039.g0495

Hi,

On Tue, 27 Nov 2007, Junio C Hamano wrote:

 * This is definitely a backward incompatible change, but I think it is
   only in a good way.

I think so, too.

Thanks,
Dscho

On Tue, Nov 27, 2007 at 11:26:56PM -0800, Junio C Hamano wrote:

Too many people got burned by setting color.diff and color.status to
true when they really should have set it to "auto".

This makes only "always" to do the unconditional colorization, and
change the meaning of "true" to the same as "auto": colorize only when
we are talking to a terminal.

I think this is a good change. However, there needs to be a matching
change for all scripts which read the color.* variables (git-svn is the
only one now, I think, but Dan's git-add--interactive patch does the
same thing).

It would be nice to have a "git config --colorbool" option, but it has
the unfortunate problem that the stdout of "git config" is piped back to
the caller, so the isatty check is meaningless (and the "pager in use"
is similarly tricky). Perhaps it should go in Git.pm, so it at least
only needs to be written once.

-Peff

Jeff King [off-list ref] writes:

It would be nice to have a "git config --colorbool" option, but it has
the unfortunate problem that the stdout of "git config" is piped back to
the caller, so the isatty check is meaningless (and the "pager in use"
is similarly tricky). Perhaps it should go in Git.pm, so it at least
only needs to be written once.

About the isatty(3) check, you do not have to use the stdout to report
the result, though.  IOW, you could use the exit code from the command.

On Fri, Nov 30, 2007 at 06:36:44PM -0800, Junio C Hamano wrote:

quoted

It would be nice to have a "git config --colorbool" option, but it has
the unfortunate problem that the stdout of "git config" is piped back to
the caller, so the isatty check is meaningless (and the "pager in use"
is similarly tricky). Perhaps it should go in Git.pm, so it at least
only needs to be written once.

About the isatty(3) check, you do not have to use the stdout to report
the result, though.  IOW, you could use the exit code from the command.

I thought about that, but it feels a little wrong since it is so unlike
all of the other interfaces to git-config. Still, I would consider doing
it if there weren't other issues (like knowing when a pager is in use).
At some point it becomes more complex than simply having the 5-10 lines
necessary to do the check in perl.

-Peff