Re: [PATCH] config: added --expiry-date type support

> 
> It's great that there are new tests. We'll probably need some
> documentation, too (especially users will need to know what the output
> format means).
> 

>> @@ -80,6 +81,7 @@ static struct option builtin_config_options[] = {
>>  	OPT_BIT(0, "int", &types, N_("value is decimal number"), TYPE_INT),
>>  	OPT_BIT(0, "bool-or-int", &types, N_("value is --bool or --int"), 
>> TYPE_BOOL_OR_INT),
>>  	OPT_BIT(0, "path", &types, N_("value is a path (file or directory 
>> name)"), TYPE_PATH),
>> +	OPT_BIT(0, "expiry-date", &types, N_("value is an expiry date"), 
>> TYPE_EXPIRY_DATE),
>>  	OPT_GROUP(N_("Other")),
>>  	OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL 
>> byte")),
>>  	OPT_BOOL(0, "name-only", &omit_values, N_("show variable names 
>> only")),
> 
> We seem to use both "expire" and "expiry" throughout the code and in
> user-facing bits (e.g., "gc.reflogExpire" and "gc.logExpiry"). I don't
> have a real preference for one versus the other. I just mention it 
> since
> whatever we choose here will be locked in to the interface forever.
> 

>> @@ -159,6 +161,12 @@ static int format_config(struct strbuf *buf, 
>> const char *key_, const char *value
>>  				return -1;
>>  			strbuf_addstr(buf, v);
>>  			free((char *)v);
>> +		} else if (types == TYPE_EXPIRY_DATE) {
>> +			timestamp_t *t = malloc(sizeof(*t));
>> +			if(git_config_expiry_date(&t, key_, value_) < 0)
>> +				return -1;
>> +			strbuf_addf(buf, "%"PRItime, *t);
>> +			free((timestamp_t *)t);
>>  		} else if (value_) {
> 
> Since we only need the timestamp variable within this block, we don't
> need to use a pointer. We can just do something like:
> 
>   } else if (types == TYPE_EXPIRY_DATE) {
> 	timestamp_t t;
> 	if (git_config_expiry_date(&t, key_, value_) < 0)
> 		return -1;
> 	strbuf_addf(buf, "%"PRItime", t);
>   }
> 
> Note that your new git_config_expiry_date would want to take just a
> regular pointer, rather than a pointer-to-pointer. I suspect you picked
> that up from git_config_pathname(). It needs the double pointer because
> it's storing a string (which is itself a pointer), but we don't need
> that here.

> 
>> diff --git a/config.c b/config.c
>> index 903abf9533b18..caa2fd5fb6915 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -990,6 +990,15 @@ int git_config_pathname(const char **dest, const 
>> char *var, const char *value)
>>  	return 0;
>>  }
>> 
>> +int git_config_expiry_date(timestamp_t **timestamp, const char *var, 
>> const char *value)
>> +{
>> +	if (!value)
>> +		return config_error_nonbool(var);
>> +	if (!!parse_expiry_date(value, *timestamp))
>> +		die(_("failed to parse date_string in: '%s'"), value);
>> +	return 0;
>> +}
> 
> I was surprised that we don't already have a function that does this,
> since we parse expiry config elsewhere. We do, but it's just local to
> builtin/reflog.c. So perhaps as a preparatory step we should add this
> function and convert reflog.c to use it, dropping its custom
> parse_expire_cfg_value().

> 
> What's the purpose of the "!!" before parse_expiry_date()? The usual
> idiom for that to normalize a non-zero value into "1", but we don't 
> care
> here. I think just:
> 
>   if (parse_expiry_date(value, timestamp))
> 	die(...);
> 
> would be sufficient.

>> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
>> index 364a537000bbb..59a35be89e511 100755
>> --- a/t/t1300-repo-config.sh
>> +++ b/t/t1300-repo-config.sh
>> @@ -901,6 +901,31 @@ test_expect_success 'get --path barfs on boolean 
>> variable' '
>>  	test_must_fail git config --get --path path.bool
>>  '
>> 
>> +test_expect_success 'get --expiry-date' '
>> +	cat >.git/config <<-\EOF &&
>> +	[date]
>> +	valid1 = "Fri Jun 4 15:46:55 2010"
>> +	valid2 = "2017/11/11 11:11:11PM"
>> +	valid3 = "2017/11/10 09:08:07 PM"
>> +	valid4 = "never"
>> +	invalid1 = "abc"
>> +	EOF
>> +	cat >expect <<-\EOF &&
>> +	1275666415
>> +	1510441871
>> +	1510348087
>> +	0
>> +	EOF
>> +	{
>> +		git config --expiry-date date.valid1 &&
>> +		git config --expiry-date date.valid2 &&
>> +		git config --expiry-date date.valid3 &&
>> +		git config --expiry-date date.valid4
>> +	} >actual &&
>> +	test_cmp expect actual &&
>> +	test_must_fail git config --expiry-date date.invalid1
>> +'
> 
> This looks good to me. It would be nice if we could test a relative
> value (which after all is what we'd expect to see in such a variable).
> But there's no way to do it in a robust way, since it will always be
> racy with the current timestamp.
> 
> We do have routines that let you make dates relative to a specific 
> time,
> but they're accessible only from t/helper/test-date, not git itself.
> 
> I don't think it's that big a deal, though. We're not testing the time
> code here (which is tested elsewhere with test-date), but just that 
> we're
> passing the dates through to be parsed.
> 
> -Peff