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

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

[Pascal Obry]

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

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

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

[Junio C Hamano]

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

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,

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

[Pascal Obry]

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

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

[Junio C Hamano]

Pascal Obry <pascal.obry@wanadoo.fr> writes:

> 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.

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

[Junio C Hamano]

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

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

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

[Johannes Schindelin]

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

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

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

[Jeff King]

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. 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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