[PATCH] Pretty formats: skip color codes if !want_color()

[PATCH] Pretty formats: skip color codes if !want_color()

[Oren Held]

Avoid color escape codes if colors are disabled, just like the behavior of other git commands.
This solves the case of color escape codes in stdout when piping or redirecting, e.g.:
$ git log --format=%Cred%h > out

Signed-off-by: Oren Held <oren@held.org.il>
---
Would appreciate your help or comments :)

 pretty.c |   18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/pretty.c b/pretty.c
index 5bdc2e7..9637dfd 100644
--- a/pretty.c
+++ b/pretty.c
@@ -947,6 +947,12 @@ static int format_reflog_person(struct strbuf *sb,
 	return format_person_part(sb, part, ident, strlen(ident), dmode);
 }
 
+static void conditional_strbuf_addstr(int conditional, struct strbuf *sb,
+				      const char *s) {
+	if (conditional)
+		strbuf_addstr(sb, s);
+}
+
 static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 				void *context)
 {
@@ -956,6 +962,8 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 	struct commit_list *p;
 	int h1, h2;
 
+	int colors_enabled = want_color(GIT_COLOR_UNKNOWN);
+
 	/* these are independent of the commit */
 	switch (placeholder[0]) {
 	case 'C':
@@ -967,20 +975,20 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 			color_parse_mem(placeholder + 2,
 					end - (placeholder + 2),
 					"--pretty format", color);
-			strbuf_addstr(sb, color);
+			conditional_strbuf_addstr(colors_enabled, sb, color);
 			return end - placeholder + 1;
 		}
 		if (!prefixcmp(placeholder + 1, "red")) {
-			strbuf_addstr(sb, GIT_COLOR_RED);
+			conditional_strbuf_addstr(colors_enabled, sb, GIT_COLOR_RED);
 			return 4;
 		} else if (!prefixcmp(placeholder + 1, "green")) {
-			strbuf_addstr(sb, GIT_COLOR_GREEN);
+			conditional_strbuf_addstr(colors_enabled, sb, GIT_COLOR_GREEN);
 			return 6;
 		} else if (!prefixcmp(placeholder + 1, "blue")) {
-			strbuf_addstr(sb, GIT_COLOR_BLUE);
+			conditional_strbuf_addstr(colors_enabled, sb, GIT_COLOR_BLUE);
 			return 5;
 		} else if (!prefixcmp(placeholder + 1, "reset")) {
-			strbuf_addstr(sb, GIT_COLOR_RESET);
+			conditional_strbuf_addstr(colors_enabled, sb, GIT_COLOR_RESET);
 			return 6;
 		} else
 			return 0;
-- 
1.7.10.4

Re: [PATCH] Pretty formats: skip color codes if !want_color()

[Jeff King]

On Tue, Dec 18, 2012 at 12:57:03AM +0200, Oren Held wrote:

> Avoid color escape codes if colors are disabled, just like the
> behavior of other git commands.  This solves the case of color escape
> codes in stdout when piping or redirecting, e.g.: $ git log
> --format=%Cred%h > out

You may be interested in this thread from today, which is attacking the
same problem:

  http://thread.gmane.org/gmane.comp.version-control.git/211370/focus=211714

Two issues with your patch:

> @@ -956,6 +962,8 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
>  	struct commit_list *p;
>  	int h1, h2;
>  
> +	int colors_enabled = want_color(GIT_COLOR_UNKNOWN);
> +

I think this would want to use the regular diff color variable to be
consistent with other coloring of "git log".

> @@ -967,20 +975,20 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
>  			color_parse_mem(placeholder + 2,
>  					end - (placeholder + 2),
>  					"--pretty format", color);
> -			strbuf_addstr(sb, color);
> +			conditional_strbuf_addstr(colors_enabled, sb, color);
>  			return end - placeholder + 1;
>  		}

This breaks backwards compatibility for callers who expect the coloring
to be unconditional; adding new syntax would solve that.

The patch I linked to above solves both.

-Peff