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

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

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

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

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

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

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

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

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?

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