Re: [PATCH] diff: Support diff.color-words config option
- From
Boyd Stephen Smith Jr. <bss@iguanasuicide.net>
- Date
- Jan 20, 2009, 16:52 UTC
- Message-ID
- <200901201053.03256.bss@iguanasuicide.net>
- In-Reply-To
- <alpine.DEB.1.00.0901201058520.3586@pacific.mpi-cbg.de>
On Tuesday 2009 January 20 04:02:00 you wrote:
Show 14 quoted lines
>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.
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.
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.
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.
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.
Show 12 quoted lines
>> 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/ \_/