threads / patch / 28372

patch, 2 partsformat-patch: produce non colorized patches when color.ui=always

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

## tl;dr

6 messages between Sep 12, 2011 and Sep 12, 2011. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Pang Yan Han· Sep 12, 2011, 17:46 UTC · lore
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
Pang Yan Han· Sep 12, 2011, 17:46 UTC · re: Pang Yan Han · lore

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

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
Show changes to t/t4051-format-patch-config.sh +23 −0
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
Junio C Hamano· Sep 12, 2011, 18:07 UTC · re: Pang Yan Han · lore

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

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"?

Show 25 quoted lines
> 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
'
Pang Yan Han· Sep 12, 2011, 17:46 UTC · re: Pang Yan Han · lore

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

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(-)
Show changes to 2 files +3 −2

builtin/log.c, t/t4051-format-patch-config.sh

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
Junio C Hamano· Sep 12, 2011, 18:08 UTC · re: Pang Yan Han · lore

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

Pang Yan Han <pangyanhan@gmail.com> writes:
Show 29 quoted lines
> 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.
Junio C Hamano· Sep 12, 2011, 18:43 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 2 files +10 −1

builtin/log.c, t/t4014-format-patch.sh

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

← back to recent threads