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

From: Jeff King <peff@peff.net>
Date: Sep 12, 2011, 16:53 UTC
Message-ID: <20110912165317.GA8054@sigill.intra.peff.net>
In-Reply-To: <1315843016-10959-1-git-send-email-pangyanhan@gmail.com>

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

Show 6 quoted lines

> 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. :)

Show 14 quoted lines

> 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