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

["Boyd Stephen Smith Jr." <bss@iguanasuicide.net>]

[-- Attachment #1: Type: text/plain, Size: 3081 bytes --]

On Tuesday 2009 January 20 04:02:00 you wrote:
>On Mon, 19 Jan 2009, Boyd Stephen Smith Jr. wrote:
>> diff --git a/diff.c b/diff.c
>> index 9fcde96..c53e1d1 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -23,6 +23,7 @@ static int diff_detect_rename_default;
>>  static int diff_rename_limit_default = 200;
>>  static int diff_suppress_blank_empty;
>>  int diff_use_color_default = -1;
>> +static const char *diff_color_words_cfg = NULL;
>>  static const char *external_diff_cmd_cfg;
>
>Guess why external_diff_cmd_cfg is not set to NULL?  All variables
>defined outside a function are set to all-zero anyway.

I suppose I just initialize variables by reflex, having been bitten with too 
many sometimes-crashes due to variables that were usually-zero.  Assuming C 
does guarantee that it is zeroed, I'll drop the " = NULL" line noise in the 
next version.

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

I don't like runtogetherwords because they are hard to read for me; I tend to 
choose the wrong word breaks if it is ambiguous.  There are other 
configuration values that use camelCaseWords so I will convert over to using 
that.

I thought "word regex" made more sense, but I wanted to match the command-line 
option.  Will change.

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

You are up too late Dscho.  This ordering makes the config not override 
attributes.  If one of the files has a diff driver, o->word_regex will be set 
to it (and become non-NULL).  That will prevent execution of the body of the 
added "if (!o->word_regex)" -- preventing the configuration option from being 
used.

>> diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
>> index 6ebce9d..a207d9e 100755
>> --- a/t/t4034-diff-words.sh
>> +++ b/t/t4034-diff-words.sh
>> @@ -105,7 +105,7 @@ a = b + c<RESET>
>>  EOF
>>  cp expect.non-whitespace-is-word expect
>>
>> -test_expect_failure 'use default supplied by config' '
>> +test_expect_success 'use default supplied by config' '
>
>Let's squash the two, okay?

Will do.  I expected the code changes to be larger than the test, and when I 
finished it was completely the other way.  My next patch will be all-in-one.

Thanks for your feedback.
-- 
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
bss@iguanasuicide.net                     ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.net/                      \_/     

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]