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

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

[Haaris]

---
 builtin/config.c       | 11 ++++++++++-
 config.c               |  9 +++++++++
 config.h               |  1 +
 t/t1300-repo-config.sh | 25 +++++++++++++++++++++++++
 4 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/builtin/config.c b/builtin/config.c
index d13daeeb55927..41cd9f5ca7cde 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -52,6 +52,7 @@ static int show_origin;
 #define TYPE_INT (1<<1)
 #define TYPE_BOOL_OR_INT (1<<2)
 #define TYPE_PATH (1<<3)
+#define TYPE_EXPIRY_DATE (1<<4)
 
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
@@ -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")),
@@ -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_) {
 			strbuf_addstr(buf, value_);
 		} else {
@@ -273,12 +281,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);
 	if (types == TYPE_INT)
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;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
 	/* This needs a better name */
diff --git a/config.h b/config.h
index a49d264416225..2d127d9d23c90 100644
--- a/config.h
+++ b/config.h
@@ -58,6 +58,7 @@ extern int git_config_bool_or_int(const char *, const char *, int *);
 extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
 extern int git_config_pathname(const char **, const char *, const char *);
+extern int git_config_expiry_date(timestamp_t **, const char *, const char *);
 extern int git_config_set_in_file_gently(const char *, const char *, const char *);
 extern void git_config_set_in_file(const char *, const char *, const char *);
 extern int git_config_set_gently(const char *, const char *);
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
+'
+
 cat > expect << EOF
 [quote]
 	leading = " test"

--
https://github.com/git/git/pull/433

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

[Kevin Daudt]

On Sun, Nov 12, 2017 at 12:19:35PM +0000, Haaris wrote:
> ---
>  builtin/config.c       | 11 ++++++++++-
>  config.c               |  9 +++++++++
>  config.h               |  1 +
>  t/t1300-repo-config.sh | 25 +++++++++++++++++++++++++
>  4 files changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/config.c b/builtin/config.c
> index d13daeeb55927..41cd9f5ca7cde 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -52,6 +52,7 @@ static int show_origin;
>  #define TYPE_INT (1<<1)
>  #define TYPE_BOOL_OR_INT (1<<2)
>  #define TYPE_PATH (1<<3)
> +#define TYPE_EXPIRY_DATE (1<<4)
>  
>  static struct option builtin_config_options[] = {
>  	OPT_GROUP(N_("Config file location")),
> @@ -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")),
> @@ -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_) {
>  			strbuf_addstr(buf, value_);
>  		} else {
> @@ -273,12 +281,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);
>  	if (types == TYPE_INT)
> 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;
> +}
> +
>  static int git_default_core_config(const char *var, const char *value)
>  {
>  	/* This needs a better name */
> diff --git a/config.h b/config.h
> index a49d264416225..2d127d9d23c90 100644
> --- a/config.h
> +++ b/config.h
> @@ -58,6 +58,7 @@ extern int git_config_bool_or_int(const char *, const char *, int *);
>  extern int git_config_bool(const char *, const char *);
>  extern int git_config_string(const char **, const char *, const char *);
>  extern int git_config_pathname(const char **, const char *, const char *);
> +extern int git_config_expiry_date(timestamp_t **, const char *, const char *);
>  extern int git_config_set_in_file_gently(const char *, const char *, const char *);
>  extern void git_config_set_in_file(const char *, const char *, const char *);
>  extern int git_config_set_gently(const char *, const char *);
> 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
> +'
> +
>  cat > expect << EOF
>  [quote]
>  	leading = " test"
> 
> --
> https://github.com/git/git/pull/433

Welcome and thanks for your submission.

There are a couple of issues, which you can read about in the
SubmittingPatches[0] documentation.

The first and most foremost is that your signed-off-by is missing,
which is a requirement to show that you have the right to submit this
code.

The commit subject should be in the present tense, as a command to the
code base, like so:

    config: add --expiry-date type support

What I'm also missing is a motivation on why you added this option,
which should be part of your commit message. As far as I know, there is
currently no config setting that expects a date format.

Kevin

[0]:https://github.com/git/git/blob/master/Documentation/SubmittingPatches

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

[Jeff King]

On Sun, Nov 12, 2017 at 02:55:53PM +0100, Kevin Daudt wrote:

> What I'm also missing is a motivation on why you added this option,
> which should be part of your commit message. As far as I know, there is
> currently no config setting that expects a date format.

This patch came from submitGit, and there's a bit more at:

  https://github.com/git/git/pull/433

(though obviously that informatoin should go into the commit message).

We do parse expiration dates from config in a few places, like
gc.reflogexpire, etc. There's no way for a script using git-config to do
the same (either adding an option specific to the script, or trying to
do some analysis on gc.reflogexpire).

-Peff

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

[Jeff King]

On Sun, Nov 12, 2017 at 12:19:35PM +0000, Haaris wrote:

> ---

Hi, and welcome to the list. Thanks for working on this (for those of
you on the list, this was one of the tasks at the hackathon this
weekend).

Kevin already mentioned a few things about the commit message, which I
agree with.

>  builtin/config.c       | 11 ++++++++++-
>  config.c               |  9 +++++++++
>  config.h               |  1 +
>  t/t1300-repo-config.sh | 25 +++++++++++++++++++++++++

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.

> @@ -273,12 +281,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);

This makes sense. The expiration values we get from the user are
typically relative (like "2.weeks"), so it wouldn't make sense to store
the absolute value we get from applying that relative offset to the
current time.

> 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

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

[hsed]

On 2017-11-12 14:55, Jeff King wrote:
> Hi, and welcome to the list. Thanks for working on this (for those of
> you on the list, this was one of the tasks at the hackathon this
> weekend).

It was a pleasure meeting everyone and a great experience!

> 
> Kevin already mentioned a few things about the commit message, which I
> agree with.

Sorry about that and the commit message formatting,
now that my mail is being received by git@vger I will try sending 
patches
with the required text, etc.

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

True, looking at the repo I found a document here[0]
Should I try editing this to add the new option?

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

I am not sure why do we need to use the 'expir(e/y)' keyword?
I think the parse_expiry_date() function still worked for past dates
is that intended?

Would having it as just '--date' suffice or do you plan to
have --date-type which will be different from expiry dates?

Anyways, I will use whatever keyword you think is more suitable. Please 
let me know.

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

Yes, I got it from the pathname function, I'll change this to just 
pointer.

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

Ok, I will make these changes in reflog.c.

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

No real purpose, I saw it in prev code but I guess that had a different
purpose (as you mentioned) I'll change that.

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


Is there a way to incorporate that? I will try calling 
t/helper/test-date
within a test but it would probably need to have some parts fixed like 
seconds
and/or minutes to prevent the race condition.


Kind Regards,
Haaris

[0]: https://github.com/git/git/blob/master/Documentation/git-config.txt

[PATCH V2] config: add --expiry-date

[hsed]

From: Haaris <hsed@unimetic.com>

Description:
This patch adds a new option to the config command.

Uses flag --expiry-date as a data-type to covert date-strings to
timestamps when reading from config files (GET).
This flag is ignored on write (SET) because the date-string is stored in
config without performing any normalization.

Creates a few test cases and documentation since its a new feature.

Motivation:
A parse_expiry_date() function already existed for api calls,
this patch simply allows the function to be used from the command line.

Signed-off-by: Haaris <hsed@unimetic.com>
---
 Documentation/git-config.txt |  5 +++++
 builtin/config.c             | 10 +++++++++-
 builtin/reflog.c             | 14 ++------------
 config.c                     |  9 +++++++++
 config.h                     |  1 +
 t/helper/test-date.c         | 12 ++++++++++++
 t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++++++
 7 files changed, 68 insertions(+), 13 deletions(-)

Update:
Added suggestions, documentation, relative time test case and test
helper function to print out timestamps for comparison. Updated reflog.c
to avoid function duplication.

Sorry for duplicate messages, the other one didn't get threaded properly.

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 4edd09f..14da5fc 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -180,6 +180,11 @@ See also <<FILES>>.
 	value (but you can use `git config section.variable ~/`
 	from the command line to let your shell do the expansion).
 
+--expiry-date::
+	`git config` will ensure that the output is converted from
+	a fixed or relative date-string to a timestamp. This option
+	has no effect when setting the value.
+
 -z::
 --null::
 	For all options that output values and/or keys, always
diff --git a/builtin/config.c b/builtin/config.c
index d13daee..afdb021 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -52,6 +52,7 @@ static int show_origin;
 #define TYPE_INT (1<<1)
 #define TYPE_BOOL_OR_INT (1<<2)
 #define TYPE_PATH (1<<3)
+#define TYPE_EXPIRY_DATE (1<<4)
 
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
@@ -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")),
@@ -159,6 +161,11 @@ 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;
+			if(git_config_expiry_date(&t, key_, value_) < 0)
+				return -1;
+			strbuf_addf(buf, "%"PRItime, t);
 		} else if (value_) {
 			strbuf_addstr(buf, value_);
 		} else {
@@ -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);
 	if (types == TYPE_INT)
diff --git a/builtin/reflog.c b/builtin/reflog.c
index ab31a3b..2233725 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -416,16 +416,6 @@ static struct reflog_expire_cfg *find_cfg_ent(const char *pattern, size_t len)
 	return ent;
 }
 
-static int parse_expire_cfg_value(const char *var, const char *value, timestamp_t *expire)
-{
-	if (!value)
-		return config_error_nonbool(var);
-	if (parse_expiry_date(value, expire))
-		return error(_("'%s' for '%s' is not a valid timestamp"),
-			     value, var);
-	return 0;
-}
-
 /* expiry timer slot */
 #define EXPIRE_TOTAL   01
 #define EXPIRE_UNREACH 02
@@ -443,11 +433,11 @@ static int reflog_expire_config(const char *var, const char *value, void *cb)
 
 	if (!strcmp(key, "reflogexpire")) {
 		slot = EXPIRE_TOTAL;
-		if (parse_expire_cfg_value(var, value, &expire))
+		if (git_config_expiry_date(&expire, var, value))
 			return -1;
 	} else if (!strcmp(key, "reflogexpireunreachable")) {
 		slot = EXPIRE_UNREACH;
-		if (parse_expire_cfg_value(var, value, &expire))
+		if (git_config_expiry_date(&expire, var, value))
 			return -1;
 	} else
 		return git_default_config(var, value, cb);
diff --git a/config.c b/config.c
index 903abf9..6ded9ce 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;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
 	/* This needs a better name */
diff --git a/config.h b/config.h
index a49d264..fc66c59 100644
--- a/config.h
+++ b/config.h
@@ -58,6 +58,7 @@ extern int git_config_bool_or_int(const char *, const char *, int *);
 extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
 extern int git_config_pathname(const char **, const char *, const char *);
+extern int git_config_expiry_date(timestamp_t *, const char *, const char *);
 extern int git_config_set_in_file_gently(const char *, const char *, const char *);
 extern void git_config_set_in_file(const char *, const char *, const char *);
 extern int git_config_set_gently(const char *, const char *);
diff --git a/t/helper/test-date.c b/t/helper/test-date.c
index f414a3a..ac83687 100644
--- a/t/helper/test-date.c
+++ b/t/helper/test-date.c
@@ -5,6 +5,7 @@ static const char *usage_msg = "\n"
 "  test-date show:<format> [time_t]...\n"
 "  test-date parse [date]...\n"
 "  test-date approxidate [date]...\n"
+"  test-date timestamp [date]...\n"
 "  test-date is64bit\n"
 "  test-date time_t-is64bit\n";
 
@@ -71,6 +72,15 @@ static void parse_approxidate(const char **argv, struct timeval *now)
 	}
 }
 
+static void parse_approx_timestamp(const char **argv, struct timeval *now)
+{
+	for (; *argv; argv++) {
+		timestamp_t t;
+		t = approxidate_relative(*argv, now);
+		printf("%s -> %"PRItime"\n", *argv, t);
+	}
+}
+
 int cmd_main(int argc, const char **argv)
 {
 	struct timeval now;
@@ -95,6 +105,8 @@ int cmd_main(int argc, const char **argv)
 		parse_dates(argv+1, &now);
 	else if (!strcmp(*argv, "approxidate"))
 		parse_approxidate(argv+1, &now);
+	else if (!strcmp(*argv, "timestamp"))
+		parse_approx_timestamp(argv+1, &now);
 	else if (!strcmp(*argv, "is64bit"))
 		return sizeof(timestamp_t) == 8 ? 0 : 1;
 	else if (!strcmp(*argv, "time_t-is64bit"))
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 364a537..cbeb9be 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -901,6 +901,36 @@ test_expect_success 'get --path barfs on boolean variable' '
 	test_must_fail git config --get --path path.bool
 '
 
+test_expect_success 'get --expiry-date' '
+	rel="3.weeks.5.days.00:00" &&
+	rel_out="$rel ->" &&
+	cat >.git/config <<-\EOF &&
+	[date]
+	valid1 = "3.weeks.5.days 00:00"
+	valid2 = "Fri Jun 4 15:46:55 2010"
+	valid3 = "2017/11/11 11:11:11PM"
+	valid4 = "2017/11/10 09:08:07 PM"
+	valid5 = "never"
+	invalid1 = "abc"
+	EOF
+	cat >expect <<-EOF &&
+	$(test-date timestamp $rel)
+	1275666415
+	1510441871
+	1510348087
+	0
+	EOF
+	{
+		echo "$rel_out $(git config --expiry-date date.valid1)"
+		git config --expiry-date date.valid2 &&
+		git config --expiry-date date.valid3 &&
+		git config --expiry-date date.valid4 &&
+		git config --expiry-date date.valid5
+	} >actual &&
+	test_cmp expect actual &&
+	test_must_fail git config --expiry-date date.invalid1
+'
+
 cat > expect << EOF
 [quote]
 	leading = " test"
-- 
2.7.4

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

[Christian Couder]

On Tue, Nov 14, 2017 at 3:04 AM,  <hsed@unimetic.com> wrote:
> From: Haaris <hsed@unimetic.com>
>
> Description:
> This patch adds a new option to the config command.
>
> Uses flag --expiry-date as a data-type to covert date-strings to
> timestamps when reading from config files (GET).
> This flag is ignored on write (SET) because the date-string is stored in
> config without performing any normalization.
>
> Creates a few test cases and documentation since its a new feature.
>
> Motivation:
> A parse_expiry_date() function already existed for api calls,
> this patch simply allows the function to be used from the command line.
>
> Signed-off-by: Haaris <hsed@unimetic.com>

Documentation/SubmittingPatches contains the following:

"Also notice that a real name is used in the Signed-off-by: line. Please
don't hide your real name."

And there is the following example before that:

        Signed-off-by: Random J Developer <random@developer.example.org>

So it looks like "a real name" actually means "a real firstname and a
real surname".

It would be nice if your "Signed-off-by:" could match this format.

Also if you have a "From:" line at the beginning of the patch, please
make sure that the name there is tha same as on the "Signed-off-by:".

Thanks for working on this,
Christian.

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

[Marc Branchaud]

On 2017-11-14 01:21 AM, Christian Couder wrote:
> On Tue, Nov 14, 2017 at 3:04 AM,  <hsed@unimetic.com> wrote:
>> From: Haaris <hsed@unimetic.com>
>>
>> Description:
>> This patch adds a new option to the config command.
>>
>> Uses flag --expiry-date as a data-type to covert date-strings to
>> timestamps when reading from config files (GET).
>> This flag is ignored on write (SET) because the date-string is stored in
>> config without performing any normalization.
>>
>> Creates a few test cases and documentation since its a new feature.
>>
>> Motivation:
>> A parse_expiry_date() function already existed for api calls,
>> this patch simply allows the function to be used from the command line.
>>
>> Signed-off-by: Haaris <hsed@unimetic.com>
> 
> Documentation/SubmittingPatches contains the following:
> 
> "Also notice that a real name is used in the Signed-off-by: line. Please
> don't hide your real name."
> 
> And there is the following example before that:
> 
>          Signed-off-by: Random J Developer <random@developer.example.org>
> 
> So it looks like "a real name" actually means "a real firstname and a
> real surname".
> 
> It would be nice if your "Signed-off-by:" could match this format.

It might already match that format if Haaris lives in a society that 
only uses single names.

Still, such names are unusual enough that it's good to check that new 
contributors are following the guidelines properly.

		M.


> Also if you have a "From:" line at the beginning of the patch, please
> make sure that the name there is tha same as on the "Signed-off-by:".
> 
> Thanks for working on this,
> Christian.
> 

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

[Junio C Hamano]

hsed@unimetic.com writes:

> From: Haaris <hsed@unimetic.com>
>
> Description:
> This patch adds a new option to the config command.
>
> Uses flag --expiry-date as a data-type to covert date-strings to
> timestamps when reading from config files (GET).
> This flag is ignored on write (SET) because the date-string is stored in
> config without performing any normalization.
>
> Creates a few test cases and documentation since its a new feature.
>
> Motivation:
> A parse_expiry_date() function already existed for api calls,
> this patch simply allows the function to be used from the command line.
>
> Signed-off-by: Haaris <hsed@unimetic.com>
> ---

Please drop all these section headers; they are irritating.  Learn
from "git log --no-merges" how the log messages in this project is
written and imitate them.  Documentation/SubmittingPatches would be
helpful.

	Add --expiry-date as a new type 'git config --get' takes,
	similar to existing --int, --bool, etc. types, so that
	scripts can learn values of configuration variables like
	gc.reflogexpire (e.g. "2.weeks") in a more useful way
	(e.g. the timesamp as of two weeks ago, expressed in number
	of seconds since epoch).

	As a helper function necessary to do this already exists in
	the implementation of builtin/reflog.c, the implementation
	is just the matter of moving it to config.c and using it
	from bultin/config.c, but shuffle the order of the parameter
	so that the pointer to the output variable comes first.
	This is to match the convention used by git_config_pathname()
	and other helper functions.

or something like that?

> +		} else if (types == TYPE_EXPIRY_DATE) {
> +			timestamp_t t;
> +			if(git_config_expiry_date(&t, key_, value_) < 0)

Style.

	if (git_config_expiry_date(&t, key_, value_) < 0)

> +				return -1;
> +			strbuf_addf(buf, "%"PRItime, t);
> ...

Thanks.

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

[hsed]

On 2017-11-14 06:38, Junio C Hamano wrote:
> hsed@unimetic.com writes:
> 
>> From: Haaris <hsed@unimetic.com>
>> 
>> Description:
>> This patch adds a new option to the config command.
>> 
>> ...
>> 
>> Motivation:
>> A parse_expiry_date() function already existed for api calls,
>> this patch simply allows the function to be used from the command 
>> line.
>> 
>> Signed-off-by: Haaris <hsed@unimetic.com>
>> ---
> 
> Please drop all these section headers; they are irritating.  Learn
> from "git log --no-merges" how the log messages in this project is
> written and imitate them.  Documentation/SubmittingPatches would be
> helpful.
> 
> 	Add --expiry-date as a new type 'git config --get' takes,
> 	similar to existing --int, --bool, etc. types, so that
> 	scripts can learn values of configuration variables like
> 	gc.reflogexpire (e.g. "2.weeks") in a more useful way
> 	(e.g. the timesamp as of two weeks ago, expressed in number
> 	of seconds since epoch).
> 
> 	As a helper function necessary to do this already exists in
> 	the implementation of builtin/reflog.c, the implementation
> 	is just the matter of moving it to config.c and using it
> 	from bultin/config.c, but shuffle the order of the parameter
> 	so that the pointer to the output variable comes first.
> 	This is to match the convention used by git_config_pathname()
> 	and other helper functions.
> 
> or something like that?

Hi,
I am sorry for not following the format properly. I will change this for
next patch update.

> 
>> +		} else if (types == TYPE_EXPIRY_DATE) {
>> +			timestamp_t t;
>> +			if(git_config_expiry_date(&t, key_, value_) < 0)
> 
> Style.

Sure.

> 
> 	if (git_config_expiry_date(&t, key_, value_) < 0)
> 
>> +				return -1;
>> +			strbuf_addf(buf, "%"PRItime, t);
>> ...
> 
> Thanks.


Kind Regards,
Haaris

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

[hsed]

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>

---
 Documentation/git-config.txt |  5 +++++
 builtin/config.c             | 10 +++++++++-
 builtin/reflog.c             | 14 ++------------
 config.c                     |  9 +++++++++
 config.h                     |  1 +
 t/helper/test-date.c         | 12 ++++++++++++
 t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++++++
 7 files changed, 68 insertions(+), 13 deletions(-)

update v3: fix style issue

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 4edd09fc6..14da5fc15 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -180,6 +180,11 @@ See also <<FILES>>.
 	value (but you can use `git config section.variable ~/`
 	from the command line to let your shell do the expansion).
 
+--expiry-date::
+	`git config` will ensure that the output is converted from
+	a fixed or relative date-string to a timestamp. This option
+	has no effect when setting the value.
+
 -z::
 --null::
 	For all options that output values and/or keys, always
diff --git a/builtin/config.c b/builtin/config.c
index d13daeeb5..ab5f95476 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -52,6 +52,7 @@ static int show_origin;
 #define TYPE_INT (1<<1)
 #define TYPE_BOOL_OR_INT (1<<2)
 #define TYPE_PATH (1<<3)
+#define TYPE_EXPIRY_DATE (1<<4)
 
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
@@ -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")),
@@ -159,6 +161,11 @@ 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;
+			if (git_config_expiry_date(&t, key_, value_) < 0)
+				return -1;
+			strbuf_addf(buf, "%"PRItime, t);
 		} else if (value_) {
 			strbuf_addstr(buf, value_);
 		} else {
@@ -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);
 	if (types == TYPE_INT)
diff --git a/builtin/reflog.c b/builtin/reflog.c
index ab31a3b6a..223372531 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -416,16 +416,6 @@ static struct reflog_expire_cfg *find_cfg_ent(const char *pattern, size_t len)
 	return ent;
 }
 
-static int parse_expire_cfg_value(const char *var, const char *value, timestamp_t *expire)
-{
-	if (!value)
-		return config_error_nonbool(var);
-	if (parse_expiry_date(value, expire))
-		return error(_("'%s' for '%s' is not a valid timestamp"),
-			     value, var);
-	return 0;
-}
-
 /* expiry timer slot */
 #define EXPIRE_TOTAL   01
 #define EXPIRE_UNREACH 02
@@ -443,11 +433,11 @@ static int reflog_expire_config(const char *var, const char *value, void *cb)
 
 	if (!strcmp(key, "reflogexpire")) {
 		slot = EXPIRE_TOTAL;
-		if (parse_expire_cfg_value(var, value, &expire))
+		if (git_config_expiry_date(&expire, var, value))
 			return -1;
 	} else if (!strcmp(key, "reflogexpireunreachable")) {
 		slot = EXPIRE_UNREACH;
-		if (parse_expire_cfg_value(var, value, &expire))
+		if (git_config_expiry_date(&expire, var, value))
 			return -1;
 	} else
 		return git_default_config(var, value, cb);
diff --git a/config.c b/config.c
index 903abf953..6ded9ce98 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;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
 	/* This needs a better name */
diff --git a/config.h b/config.h
index a49d26441..fc66c5933 100644
--- a/config.h
+++ b/config.h
@@ -58,6 +58,7 @@ extern int git_config_bool_or_int(const char *, const char *, int *);
 extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
 extern int git_config_pathname(const char **, const char *, const char *);
+extern int git_config_expiry_date(timestamp_t *, const char *, const char *);
 extern int git_config_set_in_file_gently(const char *, const char *, const char *);
 extern void git_config_set_in_file(const char *, const char *, const char *);
 extern int git_config_set_gently(const char *, const char *);
diff --git a/t/helper/test-date.c b/t/helper/test-date.c
index f414a3ac6..ac8368797 100644
--- a/t/helper/test-date.c
+++ b/t/helper/test-date.c
@@ -5,6 +5,7 @@ static const char *usage_msg = "\n"
 "  test-date show:<format> [time_t]...\n"
 "  test-date parse [date]...\n"
 "  test-date approxidate [date]...\n"
+"  test-date timestamp [date]...\n"
 "  test-date is64bit\n"
 "  test-date time_t-is64bit\n";
 
@@ -71,6 +72,15 @@ static void parse_approxidate(const char **argv, struct timeval *now)
 	}
 }
 
+static void parse_approx_timestamp(const char **argv, struct timeval *now)
+{
+	for (; *argv; argv++) {
+		timestamp_t t;
+		t = approxidate_relative(*argv, now);
+		printf("%s -> %"PRItime"\n", *argv, t);
+	}
+}
+
 int cmd_main(int argc, const char **argv)
 {
 	struct timeval now;
@@ -95,6 +105,8 @@ int cmd_main(int argc, const char **argv)
 		parse_dates(argv+1, &now);
 	else if (!strcmp(*argv, "approxidate"))
 		parse_approxidate(argv+1, &now);
+	else if (!strcmp(*argv, "timestamp"))
+		parse_approx_timestamp(argv+1, &now);
 	else if (!strcmp(*argv, "is64bit"))
 		return sizeof(timestamp_t) == 8 ? 0 : 1;
 	else if (!strcmp(*argv, "time_t-is64bit"))
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 364a53700..cbeb9bebe 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -901,6 +901,36 @@ test_expect_success 'get --path barfs on boolean variable' '
 	test_must_fail git config --get --path path.bool
 '
 
+test_expect_success 'get --expiry-date' '
+	rel="3.weeks.5.days.00:00" &&
+	rel_out="$rel ->" &&
+	cat >.git/config <<-\EOF &&
+	[date]
+	valid1 = "3.weeks.5.days 00:00"
+	valid2 = "Fri Jun 4 15:46:55 2010"
+	valid3 = "2017/11/11 11:11:11PM"
+	valid4 = "2017/11/10 09:08:07 PM"
+	valid5 = "never"
+	invalid1 = "abc"
+	EOF
+	cat >expect <<-EOF &&
+	$(test-date timestamp $rel)
+	1275666415
+	1510441871
+	1510348087
+	0
+	EOF
+	{
+		echo "$rel_out $(git config --expiry-date date.valid1)"
+		git config --expiry-date date.valid2 &&
+		git config --expiry-date date.valid3 &&
+		git config --expiry-date date.valid4 &&
+		git config --expiry-date date.valid5
+	} >actual &&
+	test_cmp expect actual &&
+	test_must_fail git config --expiry-date date.invalid1
+'
+
 cat > expect << EOF
 [quote]
 	leading = " test"
-- 
2.15.0.165.g6fd7fc36d

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

[Junio C Hamano]

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?

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

[hsed]

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

[PATCH V4] config: add --expiry-date

[hsed]

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

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>

---
 Documentation/git-config.txt |  5 +++++
 builtin/config.c             | 10 +++++++++-
 builtin/reflog.c             | 14 ++------------
 config.c                     | 10 ++++++++++
 config.h                     |  1 +
 t/helper/test-date.c         | 12 ++++++++++++
 t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++++++
 7 files changed, 69 insertions(+), 13 deletions(-)

update v4: preserve the error handling style of builtin/reflog.c

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 4edd09fc6..14da5fc15 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -180,6 +180,11 @@ See also <<FILES>>.
 	value (but you can use `git config section.variable ~/`
 	from the command line to let your shell do the expansion).
 
+--expiry-date::
+	`git config` will ensure that the output is converted from
+	a fixed or relative date-string to a timestamp. This option
+	has no effect when setting the value.
+
 -z::
 --null::
 	For all options that output values and/or keys, always
diff --git a/builtin/config.c b/builtin/config.c
index d13daeeb5..ab5f95476 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -52,6 +52,7 @@ static int show_origin;
 #define TYPE_INT (1<<1)
 #define TYPE_BOOL_OR_INT (1<<2)
 #define TYPE_PATH (1<<3)
+#define TYPE_EXPIRY_DATE (1<<4)
 
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
@@ -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")),
@@ -159,6 +161,11 @@ 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;
+			if (git_config_expiry_date(&t, key_, value_) < 0)
+				return -1;
+			strbuf_addf(buf, "%"PRItime, t);
 		} else if (value_) {
 			strbuf_addstr(buf, value_);
 		} else {
@@ -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);
 	if (types == TYPE_INT)
diff --git a/builtin/reflog.c b/builtin/reflog.c
index ab31a3b6a..223372531 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -416,16 +416,6 @@ static struct reflog_expire_cfg *find_cfg_ent(const char *pattern, size_t len)
 	return ent;
 }
 
-static int parse_expire_cfg_value(const char *var, const char *value, timestamp_t *expire)
-{
-	if (!value)
-		return config_error_nonbool(var);
-	if (parse_expiry_date(value, expire))
-		return error(_("'%s' for '%s' is not a valid timestamp"),
-			     value, var);
-	return 0;
-}
-
 /* expiry timer slot */
 #define EXPIRE_TOTAL   01
 #define EXPIRE_UNREACH 02
@@ -443,11 +433,11 @@ static int reflog_expire_config(const char *var, const char *value, void *cb)
 
 	if (!strcmp(key, "reflogexpire")) {
 		slot = EXPIRE_TOTAL;
-		if (parse_expire_cfg_value(var, value, &expire))
+		if (git_config_expiry_date(&expire, var, value))
 			return -1;
 	} else if (!strcmp(key, "reflogexpireunreachable")) {
 		slot = EXPIRE_UNREACH;
-		if (parse_expire_cfg_value(var, value, &expire))
+		if (git_config_expiry_date(&expire, var, value))
 			return -1;
 	} else
 		return git_default_config(var, value, cb);
diff --git a/config.c b/config.c
index 903abf953..64f8aa42b 100644
--- a/config.c
+++ b/config.c
@@ -990,6 +990,16 @@ 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))
+		return error(_("'%s' for '%s' is not a valid timestamp"),
+			     value, var);
+	return 0;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
 	/* This needs a better name */
diff --git a/config.h b/config.h
index a49d26441..fc66c5933 100644
--- a/config.h
+++ b/config.h
@@ -58,6 +58,7 @@ extern int git_config_bool_or_int(const char *, const char *, int *);
 extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
 extern int git_config_pathname(const char **, const char *, const char *);
+extern int git_config_expiry_date(timestamp_t *, const char *, const char *);
 extern int git_config_set_in_file_gently(const char *, const char *, const char *);
 extern void git_config_set_in_file(const char *, const char *, const char *);
 extern int git_config_set_gently(const char *, const char *);
diff --git a/t/helper/test-date.c b/t/helper/test-date.c
index f414a3ac6..ac8368797 100644
--- a/t/helper/test-date.c
+++ b/t/helper/test-date.c
@@ -5,6 +5,7 @@ static const char *usage_msg = "\n"
 "  test-date show:<format> [time_t]...\n"
 "  test-date parse [date]...\n"
 "  test-date approxidate [date]...\n"
+"  test-date timestamp [date]...\n"
 "  test-date is64bit\n"
 "  test-date time_t-is64bit\n";
 
@@ -71,6 +72,15 @@ static void parse_approxidate(const char **argv, struct timeval *now)
 	}
 }
 
+static void parse_approx_timestamp(const char **argv, struct timeval *now)
+{
+	for (; *argv; argv++) {
+		timestamp_t t;
+		t = approxidate_relative(*argv, now);
+		printf("%s -> %"PRItime"\n", *argv, t);
+	}
+}
+
 int cmd_main(int argc, const char **argv)
 {
 	struct timeval now;
@@ -95,6 +105,8 @@ int cmd_main(int argc, const char **argv)
 		parse_dates(argv+1, &now);
 	else if (!strcmp(*argv, "approxidate"))
 		parse_approxidate(argv+1, &now);
+	else if (!strcmp(*argv, "timestamp"))
+		parse_approx_timestamp(argv+1, &now);
 	else if (!strcmp(*argv, "is64bit"))
 		return sizeof(timestamp_t) == 8 ? 0 : 1;
 	else if (!strcmp(*argv, "time_t-is64bit"))
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 364a53700..cbeb9bebe 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -901,6 +901,36 @@ test_expect_success 'get --path barfs on boolean variable' '
 	test_must_fail git config --get --path path.bool
 '
 
+test_expect_success 'get --expiry-date' '
+	rel="3.weeks.5.days.00:00" &&
+	rel_out="$rel ->" &&
+	cat >.git/config <<-\EOF &&
+	[date]
+	valid1 = "3.weeks.5.days 00:00"
+	valid2 = "Fri Jun 4 15:46:55 2010"
+	valid3 = "2017/11/11 11:11:11PM"
+	valid4 = "2017/11/10 09:08:07 PM"
+	valid5 = "never"
+	invalid1 = "abc"
+	EOF
+	cat >expect <<-EOF &&
+	$(test-date timestamp $rel)
+	1275666415
+	1510441871
+	1510348087
+	0
+	EOF
+	{
+		echo "$rel_out $(git config --expiry-date date.valid1)"
+		git config --expiry-date date.valid2 &&
+		git config --expiry-date date.valid3 &&
+		git config --expiry-date date.valid4 &&
+		git config --expiry-date date.valid5
+	} >actual &&
+	test_cmp expect actual &&
+	test_must_fail git config --expiry-date date.invalid1
+'
+
 cat > expect << EOF
 [quote]
 	leading = " test"
-- 
2.15.0.169.g13c4699b6.dirty

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

[Junio C Hamano]

hsed@unimetic.com writes:

> diff --git a/config.c b/config.c
> index 903abf953..64f8aa42b 100644
> --- a/config.c
> +++ b/config.c
> @@ -990,6 +990,16 @@ 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))
> +		return error(_("'%s' for '%s' is not a valid timestamp"),
> +			     value, var);
> +	return 0;
> +}
> +

I think this is more correct even within the context of this
function than dying, which suggests the need for a slightly related
(which is not within the scope of this change) clean-up within this
file as a #leftoverbits task.  I think dying in these value parsers
goes against the point of having die_on_error bit in the
config-source structure; Heiko and Peff CC'ed for b2dc0945 ("do not
die when error in config parsing of buf occurs", 2013-07-12).

Thanks; will queue.

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

[hsed]

On 2017-11-18 03:37, Junio C Hamano wrote:
> 
> I think this is more correct even within the context of this
> function than dying, which suggests the need for a slightly related
> (which is not within the scope of this change) clean-up within this
> file as a #leftoverbits task.  I think dying in these value parsers
> goes against the point of having die_on_error bit in the
> config-source structure; Heiko and Peff CC'ed for b2dc0945 ("do not
> die when error in config parsing of buf occurs", 2013-07-12).
> 
> Thanks; will queue.

Thanks a lot for all your help and I hope to do more patches in future!

Kind Regards,
Haaris

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

[Jeff King]

On Sat, Nov 18, 2017 at 12:37:03PM +0900, Junio C Hamano wrote:

> > +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))
> > +		return error(_("'%s' for '%s' is not a valid timestamp"),
> > +			     value, var);
> > +	return 0;
> > +}
> > +
> 
> I think this is more correct even within the context of this
> function than dying, which suggests the need for a slightly related
> (which is not within the scope of this change) clean-up within this
> file as a #leftoverbits task.  I think dying in these value parsers
> goes against the point of having die_on_error bit in the
> config-source structure; Heiko and Peff CC'ed for b2dc0945 ("do not
> die when error in config parsing of buf occurs", 2013-07-12).

Yes, I agree that ideally the value parsers should avoid dying.
Unfortunately I think it will involve some heavy refactoring, since
git_config_bool(), for instance, does not even have a spot in its
interface to return an error.

Of course we can leave those other helpers in place and add a "gently"
form for each. It is really only submodule-config.c that wants to be
careful in its callback, so we could just port that. Skimming it over,
it looks like there are a few git_config_bool() and straight-up die()
calls that could be more forgiving.

+cc Stefan, who added the die(). It may be that we don't care that much
these days about recovering from broken .gitmodules files.

> Thanks; will queue.

Thanks for reviewing. This was from the hackathon last weekend, but I
missed the later re-rolls amidst my return travel.

(And thank you, Haaris, for seeing it through!)

-Peff

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

[Stefan Beller]

On Mon, Nov 20, 2017 at 9:04 AM, Jeff King <peff@peff.net> wrote:
> On Sat, Nov 18, 2017 at 12:37:03PM +0900, Junio C Hamano wrote:
>
>> > +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))
>> > +           return error(_("'%s' for '%s' is not a valid timestamp"),
>> > +                        value, var);
>> > +   return 0;
>> > +}
>> > +
>>
>> I think this is more correct even within the context of this
>> function than dying, which suggests the need for a slightly related
>> (which is not within the scope of this change) clean-up within this
>> file as a #leftoverbits task.  I think dying in these value parsers
>> goes against the point of having die_on_error bit in the
>> config-source structure; Heiko and Peff CC'ed for b2dc0945 ("do not
>> die when error in config parsing of buf occurs", 2013-07-12).
>
> Yes, I agree that ideally the value parsers should avoid dying.
> Unfortunately I think it will involve some heavy refactoring, since
> git_config_bool(), for instance, does not even have a spot in its
> interface to return an error.
>
> Of course we can leave those other helpers in place and add a "gently"
> form for each. It is really only submodule-config.c that wants to be
> careful in its callback, so we could just port that. Skimming it over,
> it looks like there are a few git_config_bool() and straight-up die()
> calls that could be more forgiving.
>
> +cc Stefan, who added the die(). It may be that we don't care that much
> these days about recovering from broken .gitmodules files.

By that you mean commits like 37f52e9344 (submodule-config:
keep shallow recommendation around, 2016-05-26) for example?
That adds a git_config_bool to the submodule config machinery.

I agree that we'd want to be more careful, but for now I'd put it to the
#leftoverbits.

Thanks,
Stefan

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

[Jeff King]

On Mon, Nov 20, 2017 at 12:28:11PM -0800, Stefan Beller wrote:

> > +cc Stefan, who added the die(). It may be that we don't care that much
> > these days about recovering from broken .gitmodules files.
> 
> By that you mean commits like 37f52e9344 (submodule-config:
> keep shallow recommendation around, 2016-05-26) for example?
> That adds a git_config_bool to the submodule config machinery.

I actually meant ea2fa5a338 (submodule-config: keep update strategy
around, 2016-02-29), which adds an actual die() into parse_config(). But
yeah, I think the end result is the same.

> I agree that we'd want to be more careful, but for now I'd put it to the
> #leftoverbits.

Fine by me. While I think the original intent was to be more lenient to
malformed .gitmodules, it's not like we're seeing bug reports about it.

-Peff

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

[Heiko Voigt]

On Mon, Nov 20, 2017 at 03:37:03PM -0500, Jeff King wrote:
> On Mon, Nov 20, 2017 at 12:28:11PM -0800, Stefan Beller wrote:
> 
> > > +cc Stefan, who added the die(). It may be that we don't care that much
> > > these days about recovering from broken .gitmodules files.
> > 
> > By that you mean commits like 37f52e9344 (submodule-config:
> > keep shallow recommendation around, 2016-05-26) for example?
> > That adds a git_config_bool to the submodule config machinery.
> 
> I actually meant ea2fa5a338 (submodule-config: keep update strategy
> around, 2016-02-29), which adds an actual die() into parse_config(). But
> yeah, I think the end result is the same.
> 
> > I agree that we'd want to be more careful, but for now I'd put it to the
> > #leftoverbits.
> 
> Fine by me. While I think the original intent was to be more lenient to
> malformed .gitmodules, it's not like we're seeing bug reports about it.

My original intent was not about being more lenient about malformed
.gitmodules but having a way to deal with repository history that might
have a malformed .gitmodules in its history. Since depending on the
branch it is on it might be quite carved in stone.
On an active project it would not be that easy to rewrite history to get
out of that situation.

When a .gitmodules file in the worktree is malformed it is easy to fix.
That is not the case when we are reading configurations from blobs.

My guess why there are no reports is that maybe not too many users are
using this infrastructure yet, plus it is probably seldom that someone
edits the .gitmodules file by hand which could lead to such a situation.
But if such an error occurs it will be very annoying if we die while
parsing submodule configurations. The only solution I see currently is
to turn submodule recursion off completely.

But maybe I am being overly cautious here.

Cheers Heiko

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

[Jeff King]

On Thu, Nov 30, 2017 at 12:18:49PM +0100, Heiko Voigt wrote:

> > Fine by me. While I think the original intent was to be more lenient to
> > malformed .gitmodules, it's not like we're seeing bug reports about it.
> 
> My original intent was not about being more lenient about malformed
> .gitmodules but having a way to deal with repository history that might
> have a malformed .gitmodules in its history. Since depending on the
> branch it is on it might be quite carved in stone.
> On an active project it would not be that easy to rewrite history to get
> out of that situation.
> 
> When a .gitmodules file in the worktree is malformed it is easy to fix.
> That is not the case when we are reading configurations from blobs.
> 
> My guess why there are no reports is that maybe not too many users are
> using this infrastructure yet, plus it is probably seldom that someone
> edits the .gitmodules file by hand which could lead to such a situation.
> But if such an error occurs it will be very annoying if we die while
> parsing submodule configurations. The only solution I see currently is
> to turn submodule recursion off completely.
> 
> But maybe I am being overly cautious here.

Ah, OK, that makes a lot of sense to me. Thanks for explaining.

I agree that is a good goal to shoot for in the long term. It's not the
end of the world if there are a few code paths that may die() for now,
but we should try not to add more, and eventually weed out the ones that
do.

-Peff