Re: [PATCH 1/2] handle color.ui at a central place

[Junio C Hamano <gitster@pobox.com>]

Markus Heidelberg <markus.heidelberg@web.de> writes:

> I looked at the code and found this in git_format_config():
>
> 	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
> 		return 0;
>
> Which of course didn't handle color.ui, but that wasn't necessary before
> the central color.ui handling from my patch.

Centralized handling is never a goal in itself.  The goal should be to
make it easier for various codepaths to use color settings correctly,
without having to have many special case workarounds.  Centralized
handling, if designed right, could be a good way to achieve that goal.

> So with the following diff it works:
>
> -	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
> +	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")
> +				       || !strcmp(var, "color.ui")) {

Why should format-patch need to even worry about protecting itself from
"color.ui" to begin with?

If your patch is making color handling saner, I would expect that
format-patch can *lose* the existing "ignore diff.color or color.diff"
workaround as a result of that.  If you need to add even *more* workaround
code like that, there's something wrong, don't you think?

> format-patch is perhaps the only place where the commit has broken
> things, because I didn't find other places,...

You did not find the breakage in format-patch either to begin with; so
your not finding does not give us much confidence that there is no other
breakage, does it?

Grumble...