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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 24, 2009, 18:36 UTC
Message-ID: <7vk58ko8k7.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <200901241228.33690.markus.heidelberg@web.de>

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

Show 7 quoted lines

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

Show 5 quoted lines

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