On Mon, Sep 12, 2011 at 11:56:54PM +0800, Pang Yan Han wrote:
> 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. :)
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html