Use --no-color option on git log commands.

When colors are activated on the repository the git log output will contain control characters to set/reset the colors. This makes list_stash() fails as the sed regular expression does not match the color control characters. Also use --no-color when computing the head on create_stash() procedure.

---
 git-stash.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index 534eb16..cde9767 100755
@@ -37,7 +37,7 @@ create_stash () {
        # state of the base commit
        if b_commit=$(git rev-parse --verify HEAD)
        then
-               head=$(git log --abbrev-commit --pretty=oneline -n 1 HEAD)
+               head=$(git log --no-color --abbrev-commit
--pretty=oneline -n 1 HEAD)
        else
                die "You do not have the initial commit yet"

fi
@@ -108,7 +108,7 @@ have_stash () {

 list_stash () {
        have_stash || return 0
-       git log --pretty=oneline -g "$@" $ref_stash |
+       git log --no-color --pretty=oneline -g "$@" $ref_stash |
        sed -n -e 's/^[.0-9a-f]* refs\///p'

}

--
1.5.3.6.959.g1ab5

--|------------------------------------------------------
--| Pascal Obry                           Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--|              http://www.obry.net
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595

The patch is good as belt-and-suspender, thanks.

But I suspect that we should make 'true' to mean 'auto' someday in git_config_colorbool(). Crazy people can set 'always' if they really wanted to, but most normal people would not want color unless the output goes to the terminal, I would think.

Something like this, perhaps...

---
 color.c |   25 ++++++++++++-------------
 1 files changed, 12 insertions(+), 13 deletions(-)

index 09d82ee..060d3cf 100644
@@ -118,21 +118,24 @@ 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 (!strcasecmp(value, "never"))
- 		return 0;
-	if (!strcasecmp(value, "always"))
-		return 1;
-	return git_config_bool(var, value);
+	if (value) {
+		if (!strcasecmp(value, "never"))
+			return 0;
+		if (!strcasecmp(value, "always"))
+			return 1;
+		if (!strcasecmp(value, "auto"))
+			goto auto;
+ 	}
+	if (!git_config_bool(var, value))
+ 		return 0;
+auto:
+	/* any normal truth value defaults to 'auto' */
+	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_vprintf(const char *color, const char *fmt,

Ok.

I definitely agree. I add it set to true, using auto instead I do not have the problem. Anyway I still think that it is good to apply my patch to completely avoid such issues.

Pascal.

--|------------------------------------------------------
--| Pascal Obry                           Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--|              http://www.obry.net
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595

Yes, that is what I said.

Except that the patch is severely whitespace damaged, and the message lack a sign-off.

I fixed them up by hand, so no need to resend.

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.

---

* 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(-)

index 124ba33..97cfbda 100644
@@ -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

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

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

Yeah, that is why I did not seriously suggest it. The message you were responding to was sitting in my "I do not know if this should go out" box for a few days and was sent out purely by accident ;-)

Hi,

I think so, too.

Thanks,
Dscho