Re: [PATCH] diff: differentiate error handling in parse_color_moved_ws

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 5, 2018, 06:12 UTC
Message-ID: <xmqqk1lshx26.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <xmqqzhurlzv2.fsf@gitster-ct.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 29 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>>  
>> -static int parse_color_moved_ws(const char *arg)
>> +static unsigned parse_color_moved_ws(const char *arg)
>>  {
>>  	int ret = 0;
>>  	struct string_list l = STRING_LIST_INIT_DUP;
>> @@ -312,15 +312,19 @@ static int parse_color_moved_ws(const char *arg)
>>  			ret |= XDF_IGNORE_WHITESPACE;
>>  		else if (!strcmp(sb.buf, "allow-indentation-change"))
>>  			ret |= COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE;
>> -		else
>> +		else {
>> +			ret |= COLOR_MOVED_WS_ERROR;
>>  			error(_("ignoring unknown color-moved-ws mode '%s'"), sb.buf);
>> +		}
>> ...  
>>  	} else if (skip_prefix(arg, "--color-moved-ws=", &arg)) {
>> -		options->color_moved_ws_handling = parse_color_moved_ws(arg);
>> +		unsigned cm = parse_color_moved_ws(arg);
>> +		if (cm & COLOR_MOVED_WS_ERROR)
>> +			die("bad --color-moved-ws argument: %s", arg);
>> +		options->color_moved_ws_handling = cm;
>
> Excellent.
>
> Will queue.  Perhaps a test or two can follow to ensure a bad value
> from config does not kill while a command line does?

Wait.  This does not fix

	git -c diff.colormovedws=nonsense diff

that dies with an error message---it should ignore the config and at moat issue a warning.

The command line handling of

	git diff --color-moved-ws=nonsense

does correctly die, but it first says "error: ignoring" before saying "fatal: bad argument", which is suboptimal.

So, not so excellent (yet) X-<.