Re: [PATCH V3] config: add --expiry-date

[Junio C Hamano <gitster@pobox.com>]

hsed@unimetic.com writes:

> From: Haaris Mehmood <hsed@unimetic.com>
>
> Add --expiry-date as a data-type for config files when
> 'git config --get' is used. This will return any relative
> or fixed dates from config files  as a timestamp value.
>
> This is useful for scripts (e.g. gc.reflogexpire) that work
> with timestamps so that '2.weeks' can be converted to a format
> acceptable by those scripts/functions.
>
> Following the convention of git_config_pathname(), move
> the helper function required for this feature from
> builtin/reflog.c to builtin/config.c where other similar
> functions exist (e.g. for --bool or --path), and match
> the order of parameters with other functions (i.e. output
> pointer as first parameter).
>
> Signed-off-by: Haaris Mehmood <hsed@unimetic.com>

Very nicely explained.  I often feel irritated when people further
rewrite what I wrote for them as an example and make it much worse,
but this one definitely is a lot more readable than the "something
like this perhaps?" in my response to the previous round.

> @@ -273,12 +280,13 @@ static char *normalize_value(const char *key, const char *value)
>  	if (!value)
>  		return NULL;
>  
> -	if (types == 0 || types == TYPE_PATH)
> +	if (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)
>  		/*
>  		 * We don't do normalization for TYPE_PATH here: If
>  		 * the path is like ~/foobar/, we prefer to store
>  		 * "~/foobar/" in the config file, and to expand the ~
>  		 * when retrieving the value.
> +		 * Also don't do normalization for expiry dates.
>  		 */
>  		return xstrdup(value);

Sensible.  Just like we want to save "~u/path" as-is without
expanding the "~u"/ part, we want to keep "2 weeks ago" as-is.

> -	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?