# [PATCH] Use --no-color option on git log commands. 10 messages from 2007-11-26 to 2007-12-01. Participants: Pascal Obry, Junio C Hamano, Johannes Schindelin, Jeff King. Thread: https://gitlist.dev/t/11026 ## Pascal Obry, 2007-11-26 22:04 Subject: [PATCH] Use --no-color option on git log commands. Message-ID: <474B42EC.1000408@wanadoo.fr> URL: https://gitlist.dev/e/474B42EC.1000408%40wanadoo.fr ``` 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 ``` ## Junio C Hamano, 2007-11-26 22:30 Subject: Re: [PATCH] Use --no-color option on git log commands. Message-ID: <7vr6icej23.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr6icej23.fsf%40gitster.siamese.dyndns.org In-Reply-To: <474B42EC.1000408@wanadoo.fr> ``` Pascal Obry 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, ``` ## Pascal Obry, 2007-11-27 18:24 Subject: Re: [PATCH] Use --no-color option on git log commands. Message-ID: <474C60FA.4040302@wanadoo.fr> URL: https://gitlist.dev/e/474C60FA.4040302%40wanadoo.fr In-Reply-To: <7vr6icej23.fsf@gitster.siamese.dyndns.org> ``` 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, 2007-11-28 04:45 Subject: Re: [PATCH] Use --no-color option on git log commands. Message-ID: <7vr6ib9dwx.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr6ib9dwx.fsf%40gitster.siamese.dyndns.org In-Reply-To: <474C60FA.4040302@wanadoo.fr> ``` Pascal Obry 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. ``` ## Junio C Hamano, 2007-11-28 07:26 Subject: [PATCH/RFC] "color.diff = true" is not "always" anymore. Message-ID: <7vd4tuakzj.fsf_-_@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vd4tuakzj.fsf_-_%40gitster.siamese.dyndns.org In-Reply-To: <7vr6icej23.fsf@gitster.siamese.dyndns.org> ``` 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 --- * 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 ``` ## Johannes Schindelin, 2007-11-28 13:13 Subject: Re: [PATCH/RFC] "color.diff = true" is not "always" anymore. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0711281313070.27959%40racer.site In-Reply-To: <7vd4tuakzj.fsf_-_@gitster.siamese.dyndns.org> ``` 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, 2007-11-28 19:04 Subject: Re: [PATCH/RFC] "color.diff = true" is not "always" anymore. Message-ID: <20071128190439.GA11396@coredump.intra.peff.net> URL: https://gitlist.dev/e/20071128190439.GA11396%40coredump.intra.peff.net In-Reply-To: <7vd4tuakzj.fsf_-_@gitster.siamese.dyndns.org> ``` 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 ``` ## Junio C Hamano, 2007-12-01 02:36 Subject: Re: [PATCH/RFC] "color.diff = true" is not "always" anymore. Message-ID: <7v4pf39m4j.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v4pf39m4j.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20071128190439.GA11396@coredump.intra.peff.net> ``` Jeff King 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. ``` ## Jeff King, 2007-12-01 04:15 Subject: Re: [PATCH/RFC] "color.diff = true" is not "always" anymore. Message-ID: <20071201041549.GB30725@coredump.intra.peff.net> URL: https://gitlist.dev/e/20071201041549.GB30725%40coredump.intra.peff.net In-Reply-To: <7v4pf39m4j.fsf@gitster.siamese.dyndns.org> ``` 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 ``` ## Junio C Hamano, 2007-12-01 06:10 Subject: Re: [PATCH/RFC] "color.diff = true" is not "always" anymore. Message-ID: <7vd4tr6j38.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vd4tr6j38.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20071201041549.GB30725@coredump.intra.peff.net> ``` Jeff King 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 ;-) ```