threads / patch / 11026

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

Subject: [PATCH] Use --no-color option on git log commands.

## tl;dr

10 messages between Nov 26, 2007 and Dec 1, 2007. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Pascal Obry· Nov 26, 2007, 22:04 UTC · lore
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(-)
Show changes to git-stash.sh +2 −4
diff --git a/git-stash.sh b/git-stash.sh
index 534eb16..cde9767 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -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
Junio C Hamano· Nov 26, 2007, 22:30 UTC · re: Pascal Obry · lore

Re: [PATCH] Use --no-color option on git log commands.

Pascal Obry <pascal.obry@wanadoo.fr> writes:
> When colors are activated on the repository the git log output
> will contain control characters to set/reset the colors.
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(-)
Show changes to color.c +18 −15
diff --git a/color.c b/color.c
index 09d82ee..060d3cf 100644
--- a/color.c
+++ b/color.c
@@ -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,
Pascal Obry· Nov 27, 2007, 18:24 UTC · re: Junio C Hamano · lore

Re: [PATCH] Use --no-color option on git log commands.

Junio C Hamano a écrit :
> The patch is good as belt-and-suspender, thanks.
Ok.
> 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.

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
Junio C Hamano· Nov 28, 2007, 04:45 UTC · re: Pascal Obry · lore

Re: [PATCH] Use --no-color option on git log commands.

Pascal Obry <pascal.obry@wanadoo.fr> writes:
Show 13 quoted lines
> Junio C Hamano a écrit :
>> The patch is good as belt-and-suspender, thanks.
>
> Ok.
>
>> 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.
>
> 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.
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.
Junio C Hamano· Nov 28, 2007, 07:26 UTC · re: Junio C Hamano · lore

[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 <gitster@pobox.com>
---
 * 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(-)
Show changes to color.c +19 −13
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
Johannes Schindelin· Nov 28, 2007, 13:13 UTC · re: Junio C Hamano · lore

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

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

Jeff King· Nov 28, 2007, 19:04 UTC · re: Junio C Hamano · lore

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

On Tue, Nov 27, 2007 at 11:26:56PM -0800, Junio C Hamano wrote:
Show 6 quoted lines
> 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
Junio C Hamano· Dec 1, 2007, 02:36 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> 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.

Jeff King· Dec 1, 2007, 04:15 UTC · re: Junio C Hamano · lore

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

On Fri, Nov 30, 2007 at 06:36:44PM -0800, Junio C Hamano wrote:
Show 8 quoted lines
> > 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
Junio C Hamano· Dec 1, 2007, 06:10 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 13 quoted lines
> On Fri, Nov 30, 2007 at 06:36:44PM -0800, Junio C Hamano wrote:
>
>> > 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.

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 ;-)

← back to recent threads