{"thread":{"id":"28371","subject":"[PATCH/RFC 0/2] format-patch: produce non colorized patches when color.ui=always","startedAt":"2011-09-12T15:56:54Z","lastAt":"2011-09-12T16:58:11Z","messageCount":5,"participants":["Pang Yan Han","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"175337","messageId":"1315843016-10959-1-git-send-email-pangyanhan@gmail.com","threadId":"28371","inReplyTo":null,"subject":"[PATCH/RFC 0/2] format-patch: produce non colorized patches when color.ui=always","fromName":"Pang Yan Han","fromEmail":"pangyanhan@gmail.com","sentAt":"2011-09-12T15:56:54Z","receivedAt":"2011-09-12T15:56:54Z","isPatch":true,"sender":{"key":"pangyanhan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/539739?v=4"},"body":"Hi list,\n\ncommit c9bfb953 (want_color: automatically fallback to color.ui) introduced\na regression which causes format-patch to produce colorized patches when\ncolor.ui is set to \"always\".\n\nSince patches are ultimately intended for machine consumption, having color\ncodes present in them is undesirable.\n\n\nMy understanding of the codebase is very limited. I've looked into builtin/log.c\nand the call chain which causes format-patch to produce colorized output is:\n\ngit_format_config\n  |_ git_log_config\n       |_ git_diff_ui_config\n\t        |_ git_color_config\n\t\t\t     |_ git_config_colorbool\n\nwhich causes git_use_color_default to be set to 1 when color.ui is set to\n\"always\".\n\nI believe that I can assume that the parsing done in git_diff_ui_config is\nrelated to the [<common diff options>] based on git format-patch manpage?\n\nI've introduced a color_disable function in color.c which changes\ngit_use_color_default to 0. This is the simplest solution I can see without\nheavily touching the stuff in the call chain above since they might be\nneeded for format-patch.\n\nI understand that this is very hacky but well, I'm really looking for ways\nto contribute to Git and this seems like one.\n\nAny advice on how this can be better solved is deeply appreciated.\n\nThanks.\n\n\nPang Yan Han (2):\n  format-patch: demonstrate that color.ui=always produces colorized\n    patches\n  format-patch: produce non colorized patches when ui.color=always\n\n builtin/log.c                 |    1 +\n color.c                       |    5 +++++\n color.h                       |    1 +\n t/t4051-format-patch-color.sh |   23 +++++++++++++++++++++++\n 4 files changed, 30 insertions(+), 0 deletions(-)\n create mode 100755 t/t4051-format-patch-color.sh\n\n-- \n1.7.7.rc0.190.g816e\n"},{"id":"175338","messageId":"1315843016-10959-2-git-send-email-pangyanhan@gmail.com","threadId":"28371","inReplyTo":"1315843016-10959-1-git-send-email-pangyanhan@gmail.com","subject":"[PATCH/RFC 1/2] format-patch: demonstrate that color.ui=always produces colorized patches","fromName":"Pang Yan Han","fromEmail":"pangyanhan@gmail.com","sentAt":"2011-09-12T15:56:55Z","receivedAt":"2011-09-12T15:56:55Z","isPatch":true,"sender":{"key":"pangyanhan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/539739?v=4"},"body":"commit c9bfb953 (want_color: automatically fallback to color.ui) introduced\na regression where format-patch produces colorized patches when color.ui is\nset to \"always\". Demonstrate this through a new test.\n\nSigned-off-by: Pang Yan Han <pangyanhan@gmail.com>\n---\nHi, I don't know if I actually understand the naming convention for tests\ncorrectly here, so I used the next available number for the last 2 digits.\n\n t/t4051-format-patch-color.sh |   23 +++++++++++++++++++++++\n 1 files changed, 23 insertions(+), 0 deletions(-)\n create mode 100755 t/t4051-format-patch-color.sh\n\ndiff --git a/t/t4051-format-patch-color.sh b/t/t4051-format-patch-color.sh\nnew file mode 100755\nindex 0000000..db30840\n--- /dev/null\n+++ b/t/t4051-format-patch-color.sh\n@@ -0,0 +1,23 @@\n+#!/bin/sh\n+\n+test_description='format-patch - check for non colorized output'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\techo foo >foo &&\n+\tgit add foo &&\n+\tgit commit -m \"commit1\" &&\n+\techo bar >foo &&\n+\tgit add foo &&\n+\tgit commit -m \"commit2\"\n+'\n+\n+test_expect_failure 'format patch with ui.color=always generates non colorized patch' '\n+\tgit config color.ui always &&\n+\tgit format-patch -1 &&\n+\tmv 0001-commit2.patch actual &&\n+\ttest_must_fail grep \"\\[31m-\" actual\n+'\n+\n+test_done\n-- \n1.7.7.rc0.190.g816e\n"},{"id":"175339","messageId":"1315843016-10959-3-git-send-email-pangyanhan@gmail.com","threadId":"28371","inReplyTo":"1315843016-10959-1-git-send-email-pangyanhan@gmail.com","subject":"[PATCH/RFC 2/2] format-patch: produce non colorized patches when ui.color=always","fromName":"Pang Yan Han","fromEmail":"pangyanhan@gmail.com","sentAt":"2011-09-12T15:56:56Z","receivedAt":"2011-09-12T15:56:56Z","isPatch":true,"sender":{"key":"pangyanhan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/539739?v=4"},"body":"commit c9bfb953 (want_color: automatically fallback to color.ui) introduced\na regression where format-patch produces colorized patches when color.ui is\nset to \"always\".\n\nTeach format-patch to disable colorized output by introducing the color_disable\nfunction in color.c\n\nSigned-off-by: Pang Yan Han <pangyanhan@gmail.com>\n---\n builtin/log.c                 |    1 +\n color.c                       |    5 +++++\n color.h                       |    1 +\n t/t4051-format-patch-color.sh |    2 +-\n 4 files changed, 8 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex d760ee0..f62520d 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1090,6 +1090,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \textra_to.strdup_strings = 1;\n \textra_cc.strdup_strings = 1;\n \tgit_config(git_format_config, NULL);\n+\tcolor_disable();\n \tinit_revisions(&rev, prefix);\n \trev.commit_format = CMIT_FMT_EMAIL;\n \trev.verbose_header = 1;\ndiff --git a/color.c b/color.c\nindex e8e2681..48e7208 100644\n--- a/color.c\n+++ b/color.c\n@@ -207,6 +207,11 @@ int want_color(int var)\n \treturn var;\n }\n \n+void color_disable(void)\n+{\n+\tgit_use_color_default = 0;\n+}\n+\n int git_color_config(const char *var, const char *value, void *cb)\n {\n \tif (!strcmp(var, \"color.ui\")) {\ndiff --git a/color.h b/color.h\nindex 9a8495b..ff4e6e5 100644\n--- a/color.h\n+++ b/color.h\n@@ -77,6 +77,7 @@ int git_color_default_config(const char *var, const char *value, void *cb);\n \n int git_config_colorbool(const char *var, const char *value);\n int want_color(int var);\n+void color_disable(void);\n void color_parse(const char *value, const char *var, char *dst);\n void color_parse_mem(const char *value, int len, const char *var, char *dst);\n __attribute__((format (printf, 3, 4)))\ndiff --git a/t/t4051-format-patch-color.sh b/t/t4051-format-patch-color.sh\nindex db30840..44dba16 100755\n--- a/t/t4051-format-patch-color.sh\n+++ b/t/t4051-format-patch-color.sh\n@@ -13,7 +13,7 @@ test_expect_success setup '\n \tgit commit -m \"commit2\"\n '\n \n-test_expect_failure 'format patch with ui.color=always generates non colorized patch' '\n+test_expect_success 'format patch with ui.color=always generates non colorized patch' '\n \tgit config color.ui always &&\n \tgit format-patch -1 &&\n \tmv 0001-commit2.patch actual &&\n-- \n1.7.7.rc0.190.g816e\n"},{"id":"175341","messageId":"20110912165317.GA8054@sigill.intra.peff.net","threadId":"28371","inReplyTo":"1315843016-10959-1-git-send-email-pangyanhan@gmail.com","subject":"Re: [PATCH/RFC 0/2] format-patch: produce non colorized patches when color.ui=always","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-12T16:53:17Z","receivedAt":"2011-09-12T16:53:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 12, 2011 at 11:56:54PM +0800, Pang Yan Han wrote:\n\n> commit c9bfb953 (want_color: automatically fallback to color.ui) introduced\n> a regression which causes format-patch to produce colorized patches when\n> color.ui is set to \"always\".\n> \n> Since patches are ultimately intended for machine consumption, having color\n> codes present in them is undesirable.\n\nThanks for looking at this. I meant to do so last week, but it looks\nlike my procrastination paid off. :)\n\n> My understanding of the codebase is very limited. I've looked into builtin/log.c\n> and the call chain which causes format-patch to produce colorized output is:\n> \n> git_format_config\n>   |_ git_log_config\n>        |_ git_diff_ui_config\n> \t        |_ git_color_config\n> \t\t\t     |_ git_config_colorbool\n> \n> which causes git_use_color_default to be set to 1 when color.ui is set to\n> \"always\".\n> \n> I believe that I can assume that the parsing done in git_diff_ui_config is\n> related to the [<common diff options>] based on git format-patch manpage?\n\nYes. However, there is also git_diff_basic_config, which is used by the\ndiff plumbing tools. So my plan had been to provide a callpath like\nthis:\n\n  git_format_config\n   |_ git_log_basic_config\n     |_ git_diff_basic_config\n       |_git_default_config\n\nHowever, git_diff_ui_config does some other things that affect patch\noutput, including:\n\n  diff.renames\n  diff.mnemonicprefix\n  diff.noprefix\n  diff.external\n  diff.wordregex\n  diff.ignoresubmodules\n\nWhich of these are OK for format-patch to respect, and which not? Even\nthough it is plumbing, I think people depend on it respecting\ndiff.renames. So probably switching to just using git_diff_basic_config\nis not right.\n\nAnother option is to just clear ALLOW_COLOR from the diff_options before\nproducing any output. But that would disable something like:\n\n  git format-patch --color ...\n\nwhich, while it is crazy for somebody actually generating a patch to\nsend, may be useful for somebody who wants to review what format-patch\nwill produce. So that's not right either.\n\nAt this point in my thinking, I scratched my head a bit. Shouldn't\nsetting \"diff.color = always\" have the exact same problem? But it\ndoesn't. And indeed, we have code to handle that in f3aafa4 (Disable\ncolor detection during format-patch, 2006-07-09). It intercepts\ndiff.color in git_format_config and doesn't pass it along.\n\nI can't decide if that is a hack (because format-patch needs to know\nabout diff config variables) or elegant (because it can intercept and\nignore whichever variables it likes). But it probably makes sense to use\nthe same strategy for color.ui, so at least we don't have two different\nhacks. :)\n\nCan you tweak your 2/2 to use that approach?\n\n> I understand that this is very hacky but well, I'm really looking for ways\n> to contribute to Git and this seems like one.\n> \n> Any advice on how this can be better solved is deeply appreciated.\n\nWelcome to git development. :) Despite my comments above, thank you for\na well-written submission. We don't always get the patches perfect the\nfirst time, but communicating the problem and the proposed solution well\nhelps reviewers move it in the right direction.\n\n-Peff\n"},{"id":"175342","messageId":"20110912165811.GB8054@sigill.intra.peff.net","threadId":"28371","inReplyTo":"1315843016-10959-2-git-send-email-pangyanhan@gmail.com","subject":"Re: [PATCH/RFC 1/2] format-patch: demonstrate that color.ui=always produces colorized patches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-12T16:58:11Z","receivedAt":"2011-09-12T16:58:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 12, 2011 at 11:56:55PM +0800, Pang Yan Han wrote:\n\n> Hi, I don't know if I actually understand the naming convention for tests\n> correctly here, so I used the next available number for the last 2 digits.\n> \n>  t/t4051-format-patch-color.sh |   23 +++++++++++++++++++++++\n\nUsually we would try to keep format-patch tests clustered numerically,\nbut it seems the t40* space has gotten quite filled up and fragmented.\nSo I think where you added it is fine (and if somebody cares to\nreorganize tests, they can do so later).\n\nOften if there is only one or two tests to add, it is more logical to\nadd to an existing script. However, I think in this case, starting a new\nscript to check how format-patch handles various config features\n(including color) makes sense. Maybe it makes sense to call it\n\"format-patch-config\" instead, and set the description to something like\n\"check that format-patch does not respect porcelain config\".\n\n-Peff\n"}]}