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

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

[Pang Yan Han]

Hi list,

This 2-patch series has been rerolled with Jeff's suggestions.

For patch 1/2, I've renamed the new test file to t4051-format-patch-config.sh
and changed the description to "check that format-patch does not respect
porcelain config". In addition, I've slightly reworded the commit message to
include the date of the commit (c9bfb953) which introduces the regression.

For patch 2/2, as per Jeff's recommendation, teach git_format_config to
intercept color.ui in the same way as commit f3aafa4 (Disable color detection
during format-patch, 2006-07-09). This is inline with the way we avoid passing
down diff.color and color.diff to git_log_config and config functions further
down the callchain.

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                  |    3 ++-
 t/t4051-format-patch-config.sh |   23 +++++++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletions(-)
 create mode 100755 t/t4051-format-patch-config.sh

-- 
1.7.7.rc0.190.gb47b6

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

[Pang Yan Han]

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

Demonstrate this through a new test.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Pang Yan Han <pangyanhan@gmail.com>
---
 t/t4051-format-patch-config.sh |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)
 create mode 100755 t/t4051-format-patch-config.sh

diff --git a/t/t4051-format-patch-config.sh b/t/t4051-format-patch-config.sh
new file mode 100755
index 0000000..cea9c7d
--- /dev/null
+++ b/t/t4051-format-patch-config.sh
@@ -0,0 +1,23 @@
+#!/bin/sh
+
+test_description='check that format-patch does not respect porcelain config'
+
+. ./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.gb47b6

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

[Junio C Hamano]

Pang Yan Han <pangyanhan@gmail.com> writes:

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

Thanks.

I'd very much prefer this "one setup, one test" folded into an existing
test script, instead of wasting a new test number. Perhaps in t4014 whose
title reads "various format-patch tests"?

> diff --git a/t/t4051-format-patch-config.sh b/t/t4051-format-patch-config.sh
> new file mode 100755
> index 0000000..cea9c7d
> --- /dev/null
> +++ b/t/t4051-format-patch-config.sh
> @@ -0,0 +1,23 @@
> +#!/bin/sh
> +
> +test_description='check that format-patch does not respect porcelain config'
> +
> +. ./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 &&

If you have a concrete output file, there is no point moving it to
"actual", especially when you are not comparing it with "expect".

> +	test_must_fail grep "\[31m-" actual
> +'

Hmm, is this grep safe?

Perhaps adding something like this near the end of an existing test script
might be simpler and sufficient, no?

test_expect_failure 'format patch ignores color.ui' '
	test_unconfig color.ui &&
        git format-patch --stdout -1 >expect &&
	test_config color.ui always &&
	git format-patch --stdout -1 >actual &&
        test_cmp expect actual
'

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

[Pang Yan Han]

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

In f3aafa4 (Disable color detection during format-patch, 2006-07-09),
git_format_config was taught to intercept diff.color to avoid passing it
down to git_log_config and later, git_diff_ui_config.

Teach git_format_config to intercept color.ui in the same way.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Pang Yan Han <pangyanhan@gmail.com>
---
 builtin/log.c                  |    3 ++-
 t/t4051-format-patch-config.sh |    2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index d760ee0..99f8c4c 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -608,7 +608,8 @@ static int git_format_config(const char *var, const char *value, void *cb)
 		string_list_append(&extra_cc, value);
 		return 0;
 	}
-	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
+	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff") ||
+		!strcmp(var, "color.ui")) {
 		return 0;
 	}
 	if (!strcmp(var, "format.numbered")) {
diff --git a/t/t4051-format-patch-config.sh b/t/t4051-format-patch-config.sh
index cea9c7d..35b349e 100755
--- a/t/t4051-format-patch-config.sh
+++ b/t/t4051-format-patch-config.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.gb47b6

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

[Junio C Hamano]

Pang Yan Han <pangyanhan@gmail.com> writes:

> commit c9bfb953 (want_color: automatically fallback to color.ui,
> 2011-08-17) introduced a regression where format-patch produces colorized
> patches when color.ui is set to "always".
>
> In f3aafa4 (Disable color detection during format-patch, 2006-07-09),
> git_format_config was taught to intercept diff.color to avoid passing it
> down to git_log_config and later, git_diff_ui_config.
>
> Teach git_format_config to intercept color.ui in the same way.
>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Pang Yan Han <pangyanhan@gmail.com>
> ---
>  builtin/log.c                  |    3 ++-
>  t/t4051-format-patch-config.sh |    2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index d760ee0..99f8c4c 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -608,7 +608,8 @@ static int git_format_config(const char *var, const char *value, void *cb)
>  		string_list_append(&extra_cc, value);
>  		return 0;
>  	}
> -	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
> +	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff") ||
> +		!strcmp(var, "color.ui")) {
>  		return 0;

Looks sensible. Thanks.

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

[Junio C Hamano]

I've queued this. Thanks.

-- >8 --
From: Pang Yan Han <pangyanhan@gmail.com>
Date: Tue, 13 Sep 2011 01:46:41 +0800
Subject: [PATCH] format-patch: ignore ui.color

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

In f3aafa4 (Disable color detection during format-patch, 2006-07-09),
git_format_config was taught to intercept diff.color to avoid passing it
down to git_log_config and later, git_diff_ui_config.

Teach git_format_config to intercept color.ui in the same way.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Pang Yan Han <pangyanhan@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/log.c           |    3 ++-
 t/t4014-format-patch.sh |    8 ++++++++
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index d760ee0..f5d4930 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -608,7 +608,8 @@ static int git_format_config(const char *var, const char *value, void *cb)
 		string_list_append(&extra_cc, value);
 		return 0;
 	}
-	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
+	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff") ||
+	    !strcmp(var, "color.ui")) {
 		return 0;
 	}
 	if (!strcmp(var, "format.numbered")) {
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 5cbc066..7e405d7 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -886,4 +886,12 @@ test_expect_success 'empty subject prefix does not have extra space' '
 	test_cmp expect actual
 '
 
+test_expect_success 'format patch ignores color.ui' '
+	test_unconfig color.ui &&
+	git format-patch --stdout -1 >expect &&
+	test_config color.ui always &&
+	git format-patch --stdout -1 >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.7.rc1