From: hsed@unimetic.com
Date: Fri, 17 Nov 2017 18:15:09 GMT
Subject: Re: [PATCH V3] config: add --expiry-date
Message-ID: <aaff8c91c03bbbc797183f26440496b6@unimetic.com>
In-Reply-To: <xmqqlgj7xcuf.fsf@gitster.mtv.corp.google.com>

On 2017-11-16 00:54, Junio C Hamano wrote:
> 
>> -	if (parse_expiry_date(value, expire))
>> -		return error(_("'%s' for '%s' is not a valid timestamp"),
>> -			     value, var);
>> ...
>> +	if (parse_expiry_date(value, timestamp))
>> +		die(_("failed to parse date_string in: '%s'"), value);
> 
> This is an unintended change in behaviour (or at least undocumented
> in the log message) for the "git reflog" command, no?
> 
> Not just the error message is different, but the original gave the
> calling code a chance to react to the failure by returning -1 from
> the function, but this makes the command fail outright here.
> 
> Would it break anything if you did "return error()" just like the
> original used to?  Are your callers of this new function not
> prepared to see an error return?

I did notice the slight change in the error handling but the new one
was copied from one of the other functions in builtin/config.c. I
will revert it to the old method and if the new (and other) tests still
pass, I will provide an updated patch.

Kind Regards,
Haaris