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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 16, 2017, 00:54 UTC
Message-ID: <xmqqlgj7xcuf.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171116000547.3246-1-hsed@unimetic.com>

hsed@unimetic.com writes:

Show 18 quoted lines

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

Show 14 quoted lines

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

Show 6 quoted lines

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