Hi, On Tue, 20 Jan 2009, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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" -- 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
