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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Tue, 20 Jan 2009, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> @@ -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.

Why not cross that bridge when we're there?  Should we ever feel the need 
for different word regexes, we would just introduce color.wordregex.

> >> @@ -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?

Yep.  Boyd (or Stephen, as he wants to be called, making it hard to guess 
from his email address, but that's all part of the fun, in't it?) already 
realized that I was up too late and got the order wrong myself.

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

You mean to avoid the cost of initializing the regex in case one and the 
same file is diffed against itself?  But that would be better handled 
before calling builtin_diff(), don't you think?

I do not know off-hand if diffcore_std() handles that already, so that the 
diff_flush() ... builtin_diff() cascade is not even called.

But you raise a valid concern: the regular expression is initialized every 
time we look at a file.  We probably should have a member 
word_regex_compiled in diff_options, then, and only initialize it the 
first time.

Ciao,
Dscho "who does not have the time to work on Git right now"