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

Oren Held· Dec 17, 2012, 22:57 UTC · lore

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

Show changes to pretty.c +13 −5

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

Jeff King· Dec 17, 2012, 23:10 UTC · re: Oren Held · lore

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

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:

Show 6 quoted lines

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

Show 8 quoted lines

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

patchPretty formats: skip color codes if !want_color()