[PATCH/RFC 0/2] format-patch: produce non colorized patches when color.ui=always

[PATCH/RFC 0/2] format-patch: produce non colorized patches when color.ui=always

[Pang Yan Han]

Hi list,

commit c9bfb953 (want_color: automatically fallback to color.ui) introduced
a regression which causes format-patch to produce colorized patches when
color.ui is set to "always".

Since patches are ultimately intended for machine consumption, having color
codes present in them is undesirable.


My understanding of the codebase is very limited. I've looked into builtin/log.c
and the call chain which causes format-patch to produce colorized output is:

git_format_config
  |_ git_log_config
       |_ git_diff_ui_config
	        |_ git_color_config
			     |_ git_config_colorbool

which causes git_use_color_default to be set to 1 when color.ui is set to
"always".

I believe that I can assume that the parsing done in git_diff_ui_config is
related to the [<common diff options>] based on git format-patch manpage?

I've introduced a color_disable function in color.c which changes
git_use_color_default to 0. This is the simplest solution I can see without
heavily touching the stuff in the call chain above since they might be
needed for format-patch.

I understand that this is very hacky but well, I'm really looking for ways
to contribute to Git and this seems like one.

Any advice on how this can be better solved is deeply appreciated.

Thanks.


Pang Yan Han (2):
  format-patch: demonstrate that color.ui=always produces colorized
    patches
  format-patch: produce non colorized patches when ui.color=always

 builtin/log.c                 |    1 +
 color.c                       |    5 +++++
 color.h                       |    1 +
 t/t4051-format-patch-color.sh |   23 +++++++++++++++++++++++
 4 files changed, 30 insertions(+), 0 deletions(-)
 create mode 100755 t/t4051-format-patch-color.sh

-- 
1.7.7.rc0.190.g816e

[PATCH/RFC 1/2] format-patch: demonstrate that color.ui=always produces colorized patches

[Pang Yan Han]

commit c9bfb953 (want_color: automatically fallback to color.ui) introduced
a regression where format-patch produces colorized patches when color.ui is
set to "always". Demonstrate this through a new test.

Signed-off-by: Pang Yan Han <pangyanhan@gmail.com>
---
Hi, I don't know if I actually understand the naming convention for tests
correctly here, so I used the next available number for the last 2 digits.

 t/t4051-format-patch-color.sh |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)
 create mode 100755 t/t4051-format-patch-color.sh

diff --git a/t/t4051-format-patch-color.sh b/t/t4051-format-patch-color.sh
new file mode 100755
index 0000000..db30840
--- /dev/null
+++ b/t/t4051-format-patch-color.sh
@@ -0,0 +1,23 @@
+#!/bin/sh
+
+test_description='format-patch - check for non colorized output'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	echo foo >foo &&
+	git add foo &&
+	git commit -m "commit1" &&
+	echo bar >foo &&
+	git add foo &&
+	git commit -m "commit2"
+'
+
+test_expect_failure 'format patch with ui.color=always generates non colorized patch' '
+	git config color.ui always &&
+	git format-patch -1 &&
+	mv 0001-commit2.patch actual &&
+	test_must_fail grep "\[31m-" actual
+'
+
+test_done
-- 
1.7.7.rc0.190.g816e

Re: [PATCH/RFC 1/2] format-patch: demonstrate that color.ui=always produces colorized patches

[Jeff King]

On Mon, Sep 12, 2011 at 11:56:55PM +0800, Pang Yan Han wrote:

> Hi, I don't know if I actually understand the naming convention for tests
> correctly here, so I used the next available number for the last 2 digits.
> 
>  t/t4051-format-patch-color.sh |   23 +++++++++++++++++++++++

Usually we would try to keep format-patch tests clustered numerically,
but it seems the t40* space has gotten quite filled up and fragmented.
So I think where you added it is fine (and if somebody cares to
reorganize tests, they can do so later).

Often if there is only one or two tests to add, it is more logical to
add to an existing script. However, I think in this case, starting a new
script to check how format-patch handles various config features
(including color) makes sense. Maybe it makes sense to call it
"format-patch-config" instead, and set the description to something like
"check that format-patch does not respect porcelain config".

-Peff

[PATCH/RFC 2/2] format-patch: produce non colorized patches when ui.color=always

[Pang Yan Han]

commit c9bfb953 (want_color: automatically fallback to color.ui) introduced
a regression where format-patch produces colorized patches when color.ui is
set to "always".

Teach format-patch to disable colorized output by introducing the color_disable
function in color.c

Signed-off-by: Pang Yan Han <pangyanhan@gmail.com>
---
 builtin/log.c                 |    1 +
 color.c                       |    5 +++++
 color.h                       |    1 +
 t/t4051-format-patch-color.sh |    2 +-
 4 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index d760ee0..f62520d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1090,6 +1090,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	extra_to.strdup_strings = 1;
 	extra_cc.strdup_strings = 1;
 	git_config(git_format_config, NULL);
+	color_disable();
 	init_revisions(&rev, prefix);
 	rev.commit_format = CMIT_FMT_EMAIL;
 	rev.verbose_header = 1;
diff --git a/color.c b/color.c
index e8e2681..48e7208 100644
--- a/color.c
+++ b/color.c
@@ -207,6 +207,11 @@ int want_color(int var)
 	return var;
 }
 
+void color_disable(void)
+{
+	git_use_color_default = 0;
+}
+
 int git_color_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "color.ui")) {
diff --git a/color.h b/color.h
index 9a8495b..ff4e6e5 100644
--- a/color.h
+++ b/color.h
@@ -77,6 +77,7 @@ int git_color_default_config(const char *var, const char *value, void *cb);
 
 int git_config_colorbool(const char *var, const char *value);
 int want_color(int var);
+void color_disable(void);
 void color_parse(const char *value, const char *var, char *dst);
 void color_parse_mem(const char *value, int len, const char *var, char *dst);
 __attribute__((format (printf, 3, 4)))
diff --git a/t/t4051-format-patch-color.sh b/t/t4051-format-patch-color.sh
index db30840..44dba16 100755
--- a/t/t4051-format-patch-color.sh
+++ b/t/t4051-format-patch-color.sh
@@ -13,7 +13,7 @@ test_expect_success setup '
 	git commit -m "commit2"
 '
 
-test_expect_failure 'format patch with ui.color=always generates non colorized patch' '
+test_expect_success 'format patch with ui.color=always generates non colorized patch' '
 	git config color.ui always &&
 	git format-patch -1 &&
 	mv 0001-commit2.patch actual &&
-- 
1.7.7.rc0.190.g816e

Re: [PATCH/RFC 0/2] format-patch: produce non colorized patches when color.ui=always

[Jeff King]

On Mon, Sep 12, 2011 at 11:56:54PM +0800, Pang Yan Han wrote:

> commit c9bfb953 (want_color: automatically fallback to color.ui) introduced
> a regression which causes format-patch to produce colorized patches when
> color.ui is set to "always".
> 
> Since patches are ultimately intended for machine consumption, having color
> codes present in them is undesirable.

Thanks for looking at this. I meant to do so last week, but it looks
like my procrastination paid off. :)

> My understanding of the codebase is very limited. I've looked into builtin/log.c
> and the call chain which causes format-patch to produce colorized output is:
> 
> git_format_config
>   |_ git_log_config
>        |_ git_diff_ui_config
> 	        |_ git_color_config
> 			     |_ git_config_colorbool
> 
> which causes git_use_color_default to be set to 1 when color.ui is set to
> "always".
> 
> I believe that I can assume that the parsing done in git_diff_ui_config is
> related to the [<common diff options>] based on git format-patch manpage?

Yes. However, there is also git_diff_basic_config, which is used by the
diff plumbing tools. So my plan had been to provide a callpath like
this:

  git_format_config
   |_ git_log_basic_config
     |_ git_diff_basic_config
       |_git_default_config

However, git_diff_ui_config does some other things that affect patch
output, including:

  diff.renames
  diff.mnemonicprefix
  diff.noprefix
  diff.external
  diff.wordregex
  diff.ignoresubmodules

Which of these are OK for format-patch to respect, and which not? Even
though it is plumbing, I think people depend on it respecting
diff.renames. So probably switching to just using git_diff_basic_config
is not right.

Another option is to just clear ALLOW_COLOR from the diff_options before
producing any output. But that would disable something like:

  git format-patch --color ...

which, while it is crazy for somebody actually generating a patch to
send, may be useful for somebody who wants to review what format-patch
will produce. So that's not right either.

At this point in my thinking, I scratched my head a bit. Shouldn't
setting "diff.color = always" have the exact same problem? But it
doesn't. And indeed, we have code to handle that in f3aafa4 (Disable
color detection during format-patch, 2006-07-09). It intercepts
diff.color in git_format_config and doesn't pass it along.

I can't decide if that is a hack (because format-patch needs to know
about diff config variables) or elegant (because it can intercept and
ignore whichever variables it likes). But it probably makes sense to use
the same strategy for color.ui, so at least we don't have two different
hacks. :)

Can you tweak your 2/2 to use that approach?

> I understand that this is very hacky but well, I'm really looking for ways
> to contribute to Git and this seems like one.
> 
> Any advice on how this can be better solved is deeply appreciated.

Welcome to git development. :) Despite my comments above, thank you for
a well-written submission. We don't always get the patches perfect the
first time, but communicating the problem and the proposed solution well
helps reviewers move it in the right direction.

-Peff