Re: [PATCH] diff: Support diff.color-words config option

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 20, 2009, 17:09 UTC
Message-ID: <7vskndkip9.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <alpine.DEB.1.00.0901201058520.3586@pacific.mpi-cbg.de>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 7 quoted lines

>> @@ -92,6 +93,8 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
>>  	}
>>  	if (!strcmp(var, "diff.external"))
>>  		return git_config_string(&external_diff_cmd_cfg, var, value);
>> +	if (!strcmp(var, "diff.color-words"))
>
> I'd call it diff.wordregex, because that's what it is.

If we want to add a new word-oriented option to diff that is not about coloring the word differences, is it safe and sane to reuse the same definition? That is, "git diff --color-words" would be affected when diff.wordregex is set to some value, so does any new word-oriented operation we will add, and the single regex configured would be used as the default value to define how a word would look like.

I think it makes sense; I do not think of a case offhand where you would want to define what a word is for the purpose of coloring diffs in one way, and would want to use a different definition for another word-oriented operation.

Show 9 quoted lines

>> @@ -1550,6 +1553,8 @@ static void builtin_diff(const char *name_a,
>>  				o->word_regex = userdiff_word_regex(one);
>>  			if (!o->word_regex)
>>  				o->word_regex = userdiff_word_regex(two);
>> +			if (!o->word_regex)
>> +				o->word_regex = diff_color_words_cfg;
>
> IMHO this is the wrong order.  config should not override attributes, 
> which are by definition more specific.

Isn't it merely giving a fallback value when attributes does not give one?

By the way, wouldn't it make sense to optimize the precontext of that hunk by doing _something_ like:

	if (!o->word_regex && strcmp(one->path, two->path))
        	o->word_regex = userdiff_word_regex(two);

"Something like" comes from special cases like /dev/null for new/deleted files, etc.