{"thread":{"id":"47177","subject":"[PATCH] config: added --expiry-date type support","startedAt":"2017-11-12T12:19:43Z","lastAt":"2017-11-30T17:45:06Z","messageCount":21,"participants":["Haaris","Kevin Daudt","Jeff King","hsed@unimetic.com","Christian Couder","Junio C Hamano","Marc Branchaud","Stefan Beller","Heiko Voigt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"332322","messageId":"0102015fb02bb5be-02c77f83-5a20-4ca1-8bab-5e9519cbd758-000000@eu-west-1.amazonses.com","threadId":"47177","inReplyTo":null,"subject":"[PATCH] config: added --expiry-date type support","fromName":"Haaris","fromEmail":"hsed@unimetic.com","sentAt":"2017-11-12T12:19:35Z","receivedAt":"2017-11-12T12:19:43Z","isPatch":true,"sender":{"key":"hsed@unimetic.com","avatar":null},"body":"---\n builtin/config.c | 11 ++++++++++-\n config.c | 9 +++++++++\n config.h | 1 +\n t/t1300-repo-config.sh | 25 +++++++++++++++++++++++++\n 4 files changed, 45 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex d13daeeb55927..41cd9f5ca7cde 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -52,6 +52,7 @@ static int show_origin;\n #define TYPE_INT (1<<1)\n #define TYPE_BOOL_OR_INT (1<<2)\n #define TYPE_PATH (1<<3)\n+#define TYPE_EXPIRY_DATE (1<<4)\n \n static struct option builtin_config_options[] = {\n \tOPT_GROUP(N_(\"Config file location\")),\n@@ -80,6 +81,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BIT(0, \"int\", &types, N_(\"value is decimal number\"), TYPE_INT),\n \tOPT_BIT(0, \"bool-or-int\", &types, N_(\"value is --bool or --int\"), TYPE_BOOL_OR_INT),\n \tOPT_BIT(0, \"path\", &types, N_(\"value is a path (file or directory name)\"), TYPE_PATH),\n+\tOPT_BIT(0, \"expiry-date\", &types, N_(\"value is an expiry date\"), TYPE_EXPIRY_DATE),\n \tOPT_GROUP(N_(\"Other\")),\n \tOPT_BOOL('z', \"null\", &end_null, N_(\"terminate values with NUL byte\")),\n \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n@@ -159,6 +161,12 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n \t\t\t\treturn -1;\n \t\t\tstrbuf_addstr(buf, v);\n \t\t\tfree((char *)v);\n+\t\t} else if (types == TYPE_EXPIRY_DATE) {\n+\t\t\ttimestamp_t *t = malloc(sizeof(*t));\n+\t\t\tif(git_config_expiry_date(&t, key_, value_) < 0)\n+\t\t\t\treturn -1;\n+\t\t\tstrbuf_addf(buf, \"%\"PRItime, *t);\n+\t\t\tfree((timestamp_t *)t);\n \t\t} else if (value_) {\n \t\t\tstrbuf_addstr(buf, value_);\n \t\t} else {\n@@ -273,12 +281,13 @@ static char *normalize_value(const char *key, const char *value)\n \tif (!value)\n \t\treturn NULL;\n \n-\tif (types == 0 || types == TYPE_PATH)\n+\tif (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)\n \t\t/*\n \t\t * We don't do normalization for TYPE_PATH here: If\n \t\t * the path is like ~/foobar/, we prefer to store\n \t\t * \"~/foobar/\" in the config file, and to expand the ~\n \t\t * when retrieving the value.\n+\t\t * Also don't do normalization for expiry dates.\n \t\t */\n \t\treturn xstrdup(value);\n \tif (types == TYPE_INT)\ndiff --git a/config.c b/config.c\nindex 903abf9533b18..caa2fd5fb6915 100644\n--- a/config.c\n+++ b/config.c\n@@ -990,6 +990,15 @@ int git_config_pathname(const char **dest, const char *var, const char *value)\n \treturn 0;\n }\n \n+int git_config_expiry_date(timestamp_t **timestamp, const char *var, const char *value)\n+{\n+\tif (!value)\n+\t\treturn config_error_nonbool(var);\n+\tif (!!parse_expiry_date(value, *timestamp))\n+\t\tdie(_(\"failed to parse date_string in: '%s'\"), value);\n+\treturn 0;\n+}\n+\n static int git_default_core_config(const char *var, const char *value)\n {\n \t/* This needs a better name */\ndiff --git a/config.h b/config.h\nindex a49d264416225..2d127d9d23c90 100644\n--- a/config.h\n+++ b/config.h\n@@ -58,6 +58,7 @@ extern int git_config_bool_or_int(const char *, const char *, int *);\n extern int git_config_bool(const char *, const char *);\n extern int git_config_string(const char **, const char *, const char *);\n extern int git_config_pathname(const char **, const char *, const char *);\n+extern int git_config_expiry_date(timestamp_t **, const char *, const char *);\n extern int git_config_set_in_file_gently(const char *, const char *, const char *);\n extern void git_config_set_in_file(const char *, const char *, const char *);\n extern int git_config_set_gently(const char *, const char *);\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 364a537000bbb..59a35be89e511 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -901,6 +901,31 @@ test_expect_success 'get --path barfs on boolean variable' '\n \ttest_must_fail git config --get --path path.bool\n '\n \n+test_expect_success 'get --expiry-date' '\n+\tcat >.git/config <<-\\EOF &&\n+\t[date]\n+\tvalid1 = \"Fri Jun 4 15:46:55 2010\"\n+\tvalid2 = \"2017/11/11 11:11:11PM\"\n+\tvalid3 = \"2017/11/10 09:08:07 PM\"\n+\tvalid4 = \"never\"\n+\tinvalid1 = \"abc\"\n+\tEOF\n+\tcat >expect <<-\\EOF &&\n+\t1275666415\n+\t1510441871\n+\t1510348087\n+\t0\n+\tEOF\n+\t{\n+\t\tgit config --expiry-date date.valid1 &&\n+\t\tgit config --expiry-date date.valid2 &&\n+\t\tgit config --expiry-date date.valid3 &&\n+\t\tgit config --expiry-date date.valid4\n+\t} >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git config --expiry-date date.invalid1\n+'\n+\n cat > expect << EOF\n [quote]\n \tleading = \" test\"\n\n--\nhttps://github.com/git/git/pull/433\n"},{"id":"332329","messageId":"20171112135553.GA10563@alpha.vpn.ikke.info","threadId":"47177","inReplyTo":"0102015fb02bb5be-02c77f83-5a20-4ca1-8bab-5e9519cbd758-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH] config: added --expiry-date type support","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-11-12T13:55:53Z","receivedAt":"2017-11-12T13:56:00Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Sun, Nov 12, 2017 at 12:19:35PM +0000, Haaris wrote:\n> ---\n> builtin/config.c | 11 ++++++++++-\n> config.c | 9 +++++++++\n> config.h | 1 +\n> t/t1300-repo-config.sh | 25 +++++++++++++++++++++++++\n> 4 files changed, 45 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/config.c b/builtin/config.c\n> index d13daeeb55927..41cd9f5ca7cde 100644\n> --- a/builtin/config.c\n> +++ b/builtin/config.c\n> @@ -52,6 +52,7 @@ static int show_origin;\n> #define TYPE_INT (1<<1)\n> #define TYPE_BOOL_OR_INT (1<<2)\n> #define TYPE_PATH (1<<3)\n> +#define TYPE_EXPIRY_DATE (1<<4)\n> \n> static struct option builtin_config_options[] = {\n> \tOPT_GROUP(N_(\"Config file location\")),\n> @@ -80,6 +81,7 @@ static struct option builtin_config_options[] = {\n> \tOPT_BIT(0, \"int\", &types, N_(\"value is decimal number\"), TYPE_INT),\n> \tOPT_BIT(0, \"bool-or-int\", &types, N_(\"value is --bool or --int\"), TYPE_BOOL_OR_INT),\n> \tOPT_BIT(0, \"path\", &types, N_(\"value is a path (file or directory name)\"), TYPE_PATH),\n> +\tOPT_BIT(0, \"expiry-date\", &types, N_(\"value is an expiry date\"), TYPE_EXPIRY_DATE),\n> \tOPT_GROUP(N_(\"Other\")),\n> \tOPT_BOOL('z', \"null\", &end_null, N_(\"terminate values with NUL byte\")),\n> \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n> @@ -159,6 +161,12 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n> \t\t\t\treturn -1;\n> \t\t\tstrbuf_addstr(buf, v);\n> \t\t\tfree((char *)v);\n> +\t\t} else if (types == TYPE_EXPIRY_DATE) {\n> +\t\t\ttimestamp_t *t = malloc(sizeof(*t));\n> +\t\t\tif(git_config_expiry_date(&t, key_, value_) < 0)\n> +\t\t\t\treturn -1;\n> +\t\t\tstrbuf_addf(buf, \"%\"PRItime, *t);\n> +\t\t\tfree((timestamp_t *)t);\n> \t\t} else if (value_) {\n> \t\t\tstrbuf_addstr(buf, value_);\n> \t\t} else {\n> @@ -273,12 +281,13 @@ static char *normalize_value(const char *key, const char *value)\n> \tif (!value)\n> \t\treturn NULL;\n> \n> -\tif (types == 0 || types == TYPE_PATH)\n> +\tif (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)\n> \t\t/*\n> \t\t * We don't do normalization for TYPE_PATH here: If\n> \t\t * the path is like ~/foobar/, we prefer to store\n> \t\t * \"~/foobar/\" in the config file, and to expand the ~\n> \t\t * when retrieving the value.\n> +\t\t * Also don't do normalization for expiry dates.\n> \t\t */\n> \t\treturn xstrdup(value);\n> \tif (types == TYPE_INT)\n> diff --git a/config.c b/config.c\n> index 903abf9533b18..caa2fd5fb6915 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -990,6 +990,15 @@ int git_config_pathname(const char **dest, const char *var, const char *value)\n> \treturn 0;\n> }\n> \n> +int git_config_expiry_date(timestamp_t **timestamp, const char *var, const char *value)\n> +{\n> +\tif (!value)\n> +\t\treturn config_error_nonbool(var);\n> +\tif (!!parse_expiry_date(value, *timestamp))\n> +\t\tdie(_(\"failed to parse date_string in: '%s'\"), value);\n> +\treturn 0;\n> +}\n> +\n> static int git_default_core_config(const char *var, const char *value)\n> {\n> \t/* This needs a better name */\n> diff --git a/config.h b/config.h\n> index a49d264416225..2d127d9d23c90 100644\n> --- a/config.h\n> +++ b/config.h\n> @@ -58,6 +58,7 @@ extern int git_config_bool_or_int(const char *, const char *, int *);\n> extern int git_config_bool(const char *, const char *);\n> extern int git_config_string(const char **, const char *, const char *);\n> extern int git_config_pathname(const char **, const char *, const char *);\n> +extern int git_config_expiry_date(timestamp_t **, const char *, const char *);\n> extern int git_config_set_in_file_gently(const char *, const char *, const char *);\n> extern void git_config_set_in_file(const char *, const char *, const char *);\n> extern int git_config_set_gently(const char *, const char *);\n> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\n> index 364a537000bbb..59a35be89e511 100755\n> --- a/t/t1300-repo-config.sh\n> +++ b/t/t1300-repo-config.sh\n> @@ -901,6 +901,31 @@ test_expect_success 'get --path barfs on boolean variable' '\n> \ttest_must_fail git config --get --path path.bool\n> '\n> \n> +test_expect_success 'get --expiry-date' '\n> +\tcat >.git/config <<-\\EOF &&\n> +\t[date]\n> +\tvalid1 = \"Fri Jun 4 15:46:55 2010\"\n> +\tvalid2 = \"2017/11/11 11:11:11PM\"\n> +\tvalid3 = \"2017/11/10 09:08:07 PM\"\n> +\tvalid4 = \"never\"\n> +\tinvalid1 = \"abc\"\n> +\tEOF\n> +\tcat >expect <<-\\EOF &&\n> +\t1275666415\n> +\t1510441871\n> +\t1510348087\n> +\t0\n> +\tEOF\n> +\t{\n> +\t\tgit config --expiry-date date.valid1 &&\n> +\t\tgit config --expiry-date date.valid2 &&\n> +\t\tgit config --expiry-date date.valid3 &&\n> +\t\tgit config --expiry-date date.valid4\n> +\t} >actual &&\n> +\ttest_cmp expect actual &&\n> +\ttest_must_fail git config --expiry-date date.invalid1\n> +'\n> +\n> cat > expect << EOF\n> [quote]\n> \tleading = \" test\"\n> \n> --\n> https://github.com/git/git/pull/433\n\nWelcome and thanks for your submission.\n\nThere are a couple of issues, which you can read about in the\nSubmittingPatches[0] documentation.\n\nThe first and most foremost is that your signed-off-by is missing,\nwhich is a requirement to show that you have the right to submit this\ncode.\n\nThe commit subject should be in the present tense, as a command to the\ncode base, like so:\n\n config: add --expiry-date type support\n\nWhat I'm also missing is a motivation on why you added this option,\nwhich should be part of your commit message. As far as I know, there is\ncurrently no config setting that expects a date format.\n\nKevin\n\n[0]:https://github.com/git/git/blob/master/Documentation/SubmittingPatches\n\n"},{"id":"332333","messageId":"20171112142204.2nuhh726imqksdug@sigill.intra.peff.net","threadId":"47177","inReplyTo":"20171112135553.GA10563@alpha.vpn.ikke.info","subject":"Re: [PATCH] config: added --expiry-date type support","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-12T14:22:04Z","receivedAt":"2017-11-12T14:22:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 12, 2017 at 02:55:53PM +0100, Kevin Daudt wrote:\n\n> What I'm also missing is a motivation on why you added this option,\n> which should be part of your commit message. As far as I know, there is\n> currently no config setting that expects a date format.\n\nThis patch came from submitGit, and there's a bit more at:\n\n https://github.com/git/git/pull/433\n\n(though obviously that informatoin should go into the commit message).\n\nWe do parse expiration dates from config in a few places, like\ngc.reflogexpire, etc. There's no way for a script using git-config to do\nthe same (either adding an option specific to the script, or trying to\ndo some analysis on gc.reflogexpire).\n\n-Peff\n"},{"id":"332334","messageId":"20171112145535.gb4nafdhhdslknex@sigill.intra.peff.net","threadId":"47177","inReplyTo":"0102015fb02bb5be-02c77f83-5a20-4ca1-8bab-5e9519cbd758-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH] config: added --expiry-date type support","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-12T14:55:36Z","receivedAt":"2017-11-12T14:55:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 12, 2017 at 12:19:35PM +0000, Haaris wrote:\n\n> ---\n\nHi, and welcome to the list. Thanks for working on this (for those of\nyou on the list, this was one of the tasks at the hackathon this\nweekend).\n\nKevin already mentioned a few things about the commit message, which I\nagree with.\n\n> builtin/config.c | 11 ++++++++++-\n> config.c | 9 +++++++++\n> config.h | 1 +\n> t/t1300-repo-config.sh | 25 +++++++++++++++++++++++++\n\nIt's great that there are new tests. We'll probably need some\ndocumentation, too (especially users will need to know what the output\nformat means).\n\n> @@ -80,6 +81,7 @@ static struct option builtin_config_options[] = {\n> \tOPT_BIT(0, \"int\", &types, N_(\"value is decimal number\"), TYPE_INT),\n> \tOPT_BIT(0, \"bool-or-int\", &types, N_(\"value is --bool or --int\"), TYPE_BOOL_OR_INT),\n> \tOPT_BIT(0, \"path\", &types, N_(\"value is a path (file or directory name)\"), TYPE_PATH),\n> +\tOPT_BIT(0, \"expiry-date\", &types, N_(\"value is an expiry date\"), TYPE_EXPIRY_DATE),\n> \tOPT_GROUP(N_(\"Other\")),\n> \tOPT_BOOL('z', \"null\", &end_null, N_(\"terminate values with NUL byte\")),\n> \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n\nWe seem to use both \"expire\" and \"expiry\" throughout the code and in\nuser-facing bits (e.g., \"gc.reflogExpire\" and \"gc.logExpiry\"). I don't\nhave a real preference for one versus the other. I just mention it since\nwhatever we choose here will be locked in to the interface forever.\n\n> @@ -159,6 +161,12 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n> \t\t\t\treturn -1;\n> \t\t\tstrbuf_addstr(buf, v);\n> \t\t\tfree((char *)v);\n> +\t\t} else if (types == TYPE_EXPIRY_DATE) {\n> +\t\t\ttimestamp_t *t = malloc(sizeof(*t));\n> +\t\t\tif(git_config_expiry_date(&t, key_, value_) < 0)\n> +\t\t\t\treturn -1;\n> +\t\t\tstrbuf_addf(buf, \"%\"PRItime, *t);\n> +\t\t\tfree((timestamp_t *)t);\n> \t\t} else if (value_) {\n\nSince we only need the timestamp variable within this block, we don't\nneed to use a pointer. We can just do something like:\n\n } else if (types == TYPE_EXPIRY_DATE) {\n\ttimestamp_t t;\n\tif (git_config_expiry_date(&t, key_, value_) < 0)\n\t\treturn -1;\n\tstrbuf_addf(buf, \"%\"PRItime\", t);\n }\n\nNote that your new git_config_expiry_date would want to take just a\nregular pointer, rather than a pointer-to-pointer. I suspect you picked\nthat up from git_config_pathname(). It needs the double pointer because\nit's storing a string (which is itself a pointer), but we don't need\nthat here.\n\n> @@ -273,12 +281,13 @@ static char *normalize_value(const char *key, const char *value)\n> \tif (!value)\n> \t\treturn NULL;\n> \n> -\tif (types == 0 || types == TYPE_PATH)\n> +\tif (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)\n> \t\t/*\n> \t\t * We don't do normalization for TYPE_PATH here: If\n> \t\t * the path is like ~/foobar/, we prefer to store\n> \t\t * \"~/foobar/\" in the config file, and to expand the ~\n> \t\t * when retrieving the value.\n> +\t\t * Also don't do normalization for expiry dates.\n> \t\t */\n> \t\treturn xstrdup(value);\n\nThis makes sense. The expiration values we get from the user are\ntypically relative (like \"2.weeks\"), so it wouldn't make sense to store\nthe absolute value we get from applying that relative offset to the\ncurrent time.\n\n> diff --git a/config.c b/config.c\n> index 903abf9533b18..caa2fd5fb6915 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -990,6 +990,15 @@ int git_config_pathname(const char **dest, const char *var, const char *value)\n> \treturn 0;\n> }\n> \n> +int git_config_expiry_date(timestamp_t **timestamp, const char *var, const char *value)\n> +{\n> +\tif (!value)\n> +\t\treturn config_error_nonbool(var);\n> +\tif (!!parse_expiry_date(value, *timestamp))\n> +\t\tdie(_(\"failed to parse date_string in: '%s'\"), value);\n> +\treturn 0;\n> +}\n\nI was surprised that we don't already have a function that does this,\nsince we parse expiry config elsewhere. We do, but it's just local to\nbuiltin/reflog.c. So perhaps as a preparatory step we should add this\nfunction and convert reflog.c to use it, dropping its custom\nparse_expire_cfg_value().\n\nWhat's the purpose of the \"!!\" before parse_expiry_date()? The usual\nidiom for that to normalize a non-zero value into \"1\", but we don't care\nhere. I think just:\n\n if (parse_expiry_date(value, timestamp))\n\tdie(...);\n\nwould be sufficient.\n\n> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\n> index 364a537000bbb..59a35be89e511 100755\n> --- a/t/t1300-repo-config.sh\n> +++ b/t/t1300-repo-config.sh\n> @@ -901,6 +901,31 @@ test_expect_success 'get --path barfs on boolean variable' '\n> \ttest_must_fail git config --get --path path.bool\n> '\n> \n> +test_expect_success 'get --expiry-date' '\n> +\tcat >.git/config <<-\\EOF &&\n> +\t[date]\n> +\tvalid1 = \"Fri Jun 4 15:46:55 2010\"\n> +\tvalid2 = \"2017/11/11 11:11:11PM\"\n> +\tvalid3 = \"2017/11/10 09:08:07 PM\"\n> +\tvalid4 = \"never\"\n> +\tinvalid1 = \"abc\"\n> +\tEOF\n> +\tcat >expect <<-\\EOF &&\n> +\t1275666415\n> +\t1510441871\n> +\t1510348087\n> +\t0\n> +\tEOF\n> +\t{\n> +\t\tgit config --expiry-date date.valid1 &&\n> +\t\tgit config --expiry-date date.valid2 &&\n> +\t\tgit config --expiry-date date.valid3 &&\n> +\t\tgit config --expiry-date date.valid4\n> +\t} >actual &&\n> +\ttest_cmp expect actual &&\n> +\ttest_must_fail git config --expiry-date date.invalid1\n> +'\n\nThis looks good to me. It would be nice if we could test a relative\nvalue (which after all is what we'd expect to see in such a variable).\nBut there's no way to do it in a robust way, since it will always be\nracy with the current timestamp.\n\nWe do have routines that let you make dates relative to a specific time,\nbut they're accessible only from t/helper/test-date, not git itself.\n\nI don't think it's that big a deal, though. We're not testing the time\ncode here (which is tested elsewhere with test-date), but just that we're\npassing the dates through to be parsed.\n\n-Peff\n"},{"id":"332357","messageId":"97a9b315c7d187b4f0897f93a8d5f6c3@unimetic.com","threadId":"47177","inReplyTo":"a05a8e8020ec31cfd9a0271ce2a00034@unimetic.com","subject":"Re: [PATCH] config: added --expiry-date type support","fromName":"","fromEmail":"hsed@unimetic.com","sentAt":"2017-11-12T19:43:43Z","receivedAt":"2017-11-12T19:43:50Z","isPatch":true,"sender":{"key":"hsed@unimetic.com","avatar":null},"body":"On 2017-11-12 14:55, Jeff King wrote:\n> Hi, and welcome to the list. Thanks for working on this (for those of\n> you on the list, this was one of the tasks at the hackathon this\n> weekend).\n\nIt was a pleasure meeting everyone and a great experience!\n\n> \n> Kevin already mentioned a few things about the commit message, which I\n> agree with.\n\nSorry about that and the commit message formatting,\nnow that my mail is being received by git@vger I will try sending \npatches\nwith the required text, etc.\n\n> \n> It's great that there are new tests. We'll probably need some\n> documentation, too (especially users will need to know what the output\n> format means).\n> \n\nTrue, looking at the repo I found a document here[0]\nShould I try editing this to add the new option?\n\n>> @@ -80,6 +81,7 @@ static struct option builtin_config_options[] = {\n>> \tOPT_BIT(0, \"int\", &types, N_(\"value is decimal number\"), TYPE_INT),\n>> \tOPT_BIT(0, \"bool-or-int\", &types, N_(\"value is --bool or --int\"), \n>> TYPE_BOOL_OR_INT),\n>> \tOPT_BIT(0, \"path\", &types, N_(\"value is a path (file or directory \n>> name)\"), TYPE_PATH),\n>> +\tOPT_BIT(0, \"expiry-date\", &types, N_(\"value is an expiry date\"), \n>> TYPE_EXPIRY_DATE),\n>> \tOPT_GROUP(N_(\"Other\")),\n>> \tOPT_BOOL('z', \"null\", &end_null, N_(\"terminate values with NUL \n>> byte\")),\n>> \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names \n>> only\")),\n> \n> We seem to use both \"expire\" and \"expiry\" throughout the code and in\n> user-facing bits (e.g., \"gc.reflogExpire\" and \"gc.logExpiry\"). I don't\n> have a real preference for one versus the other. I just mention it \n> since\n> whatever we choose here will be locked in to the interface forever.\n> \n\nI am not sure why do we need to use the 'expir(e/y)' keyword?\nI think the parse_expiry_date() function still worked for past dates\nis that intended?\n\nWould having it as just '--date' suffice or do you plan to\nhave --date-type which will be different from expiry dates?\n\nAnyways, I will use whatever keyword you think is more suitable. Please \nlet me know.\n\n>> @@ -159,6 +161,12 @@ static int format_config(struct strbuf *buf, \n>> const char *key_, const char *value\n>> \t\t\t\treturn -1;\n>> \t\t\tstrbuf_addstr(buf, v);\n>> \t\t\tfree((char *)v);\n>> +\t\t} else if (types == TYPE_EXPIRY_DATE) {\n>> +\t\t\ttimestamp_t *t = malloc(sizeof(*t));\n>> +\t\t\tif(git_config_expiry_date(&t, key_, value_) < 0)\n>> +\t\t\t\treturn -1;\n>> +\t\t\tstrbuf_addf(buf, \"%\"PRItime, *t);\n>> +\t\t\tfree((timestamp_t *)t);\n>> \t\t} else if (value_) {\n> \n> Since we only need the timestamp variable within this block, we don't\n> need to use a pointer. We can just do something like:\n> \n> } else if (types == TYPE_EXPIRY_DATE) {\n> \ttimestamp_t t;\n> \tif (git_config_expiry_date(&t, key_, value_) < 0)\n> \t\treturn -1;\n> \tstrbuf_addf(buf, \"%\"PRItime\", t);\n> }\n> \n> Note that your new git_config_expiry_date would want to take just a\n> regular pointer, rather than a pointer-to-pointer. I suspect you picked\n> that up from git_config_pathname(). It needs the double pointer because\n> it's storing a string (which is itself a pointer), but we don't need\n> that here.\n\nYes, I got it from the pathname function, I'll change this to just \npointer.\n\n> \n>> diff --git a/config.c b/config.c\n>> index 903abf9533b18..caa2fd5fb6915 100644\n>> --- a/config.c\n>> +++ b/config.c\n>> @@ -990,6 +990,15 @@ int git_config_pathname(const char **dest, const \n>> char *var, const char *value)\n>> \treturn 0;\n>> }\n>> \n>> +int git_config_expiry_date(timestamp_t **timestamp, const char *var, \n>> const char *value)\n>> +{\n>> +\tif (!value)\n>> +\t\treturn config_error_nonbool(var);\n>> +\tif (!!parse_expiry_date(value, *timestamp))\n>> +\t\tdie(_(\"failed to parse date_string in: '%s'\"), value);\n>> +\treturn 0;\n>> +}\n> \n> I was surprised that we don't already have a function that does this,\n> since we parse expiry config elsewhere. We do, but it's just local to\n> builtin/reflog.c. So perhaps as a preparatory step we should add this\n> function and convert reflog.c to use it, dropping its custom\n> parse_expire_cfg_value().\n\nOk, I will make these changes in reflog.c.\n\n> \n> What's the purpose of the \"!!\" before parse_expiry_date()? The usual\n> idiom for that to normalize a non-zero value into \"1\", but we don't \n> care\n> here. I think just:\n> \n> if (parse_expiry_date(value, timestamp))\n> \tdie(...);\n> \n> would be sufficient.\n\nNo real purpose, I saw it in prev code but I guess that had a different\npurpose (as you mentioned) I'll change that.\n\n>> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\n>> index 364a537000bbb..59a35be89e511 100755\n>> --- a/t/t1300-repo-config.sh\n>> +++ b/t/t1300-repo-config.sh\n>> @@ -901,6 +901,31 @@ test_expect_success 'get --path barfs on boolean \n>> variable' '\n>> \ttest_must_fail git config --get --path path.bool\n>> '\n>> \n>> +test_expect_success 'get --expiry-date' '\n>> +\tcat >.git/config <<-\\EOF &&\n>> +\t[date]\n>> +\tvalid1 = \"Fri Jun 4 15:46:55 2010\"\n>> +\tvalid2 = \"2017/11/11 11:11:11PM\"\n>> +\tvalid3 = \"2017/11/10 09:08:07 PM\"\n>> +\tvalid4 = \"never\"\n>> +\tinvalid1 = \"abc\"\n>> +\tEOF\n>> +\tcat >expect <<-\\EOF &&\n>> +\t1275666415\n>> +\t1510441871\n>> +\t1510348087\n>> +\t0\n>> +\tEOF\n>> +\t{\n>> +\t\tgit config --expiry-date date.valid1 &&\n>> +\t\tgit config --expiry-date date.valid2 &&\n>> +\t\tgit config --expiry-date date.valid3 &&\n>> +\t\tgit config --expiry-date date.valid4\n>> +\t} >actual &&\n>> +\ttest_cmp expect actual &&\n>> +\ttest_must_fail git config --expiry-date date.invalid1\n>> +'\n> \n> This looks good to me. It would be nice if we could test a relative\n> value (which after all is what we'd expect to see in such a variable).\n> But there's no way to do it in a robust way, since it will always be\n> racy with the current timestamp.\n> \n> We do have routines that let you make dates relative to a specific \n> time,\n> but they're accessible only from t/helper/test-date, not git itself.\n> \n> I don't think it's that big a deal, though. We're not testing the time\n> code here (which is tested elsewhere with test-date), but just that \n> we're\n> passing the dates through to be parsed.\n> \n> -Peff\n\n\nIs there a way to incorporate that? I will try calling \nt/helper/test-date\nwithin a test but it would probably need to have some parts fixed like \nseconds\nand/or minutes to prevent the race condition.\n\n\nKind Regards,\nHaaris\n\n[0]: https://github.com/git/git/blob/master/Documentation/git-config.txt\n"},{"id":"332491","messageId":"1510625073-8842-1-git-send-email-hsed@unimetic.com","threadId":"47177","inReplyTo":"20171112145535.gb4nafdhhdslknex@sigill.intra.peff.net","subject":"[PATCH V2] config: add --expiry-date","fromName":"","fromEmail":"hsed@unimetic.com","sentAt":"2017-11-14T02:04:33Z","receivedAt":"2017-11-14T02:05:08Z","isPatch":true,"sender":{"key":"hsed@unimetic.com","avatar":null},"body":"From: Haaris \n\nDescription:\nThis patch adds a new option to the config command.\n\nUses flag --expiry-date as a data-type to covert date-strings to\ntimestamps when reading from config files (GET).\nThis flag is ignored on write (SET) because the date-string is stored in\nconfig without performing any normalization.\n\nCreates a few test cases and documentation since its a new feature.\n\nMotivation:\nA parse_expiry_date() function already existed for api calls,\nthis patch simply allows the function to be used from the command line.\n\nSigned-off-by: Haaris \n---\n Documentation/git-config.txt | 5 +++++\n builtin/config.c | 10 +++++++++-\n builtin/reflog.c | 14 ++------------\n config.c | 9 +++++++++\n config.h | 1 +\n t/helper/test-date.c | 12 ++++++++++++\n t/t1300-repo-config.sh | 30 ++++++++++++++++++++++++++++++\n 7 files changed, 68 insertions(+), 13 deletions(-)\n\nUpdate:\nAdded suggestions, documentation, relative time test case and test\nhelper function to print out timestamps for comparison. Updated reflog.c\nto avoid function duplication.\n\nSorry for duplicate messages, the other one didn't get threaded properly.\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 4edd09f..14da5fc 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -180,6 +180,11 @@ See also <>.\n \tvalue (but you can use `git config section.variable ~/`\n \tfrom the command line to let your shell do the expansion).\n \n+--expiry-date::\n+\t`git config` will ensure that the output is converted from\n+\ta fixed or relative date-string to a timestamp. This option\n+\thas no effect when setting the value.\n+\n -z::\n --null::\n \tFor all options that output values and/or keys, always\ndiff --git a/builtin/config.c b/builtin/config.c\nindex d13daee..afdb021 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -52,6 +52,7 @@ static int show_origin;\n #define TYPE_INT (1<<1)\n #define TYPE_BOOL_OR_INT (1<<2)\n #define TYPE_PATH (1<<3)\n+#define TYPE_EXPIRY_DATE (1<<4)\n \n static struct option builtin_config_options[] = {\n \tOPT_GROUP(N_(\"Config file location\")),\n@@ -80,6 +81,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BIT(0, \"int\", &types, N_(\"value is decimal number\"), TYPE_INT),\n \tOPT_BIT(0, \"bool-or-int\", &types, N_(\"value is --bool or --int\"), TYPE_BOOL_OR_INT),\n \tOPT_BIT(0, \"path\", &types, N_(\"value is a path (file or directory name)\"), TYPE_PATH),\n+\tOPT_BIT(0, \"expiry-date\", &types, N_(\"value is an expiry date\"), TYPE_EXPIRY_DATE),\n \tOPT_GROUP(N_(\"Other\")),\n \tOPT_BOOL('z', \"null\", &end_null, N_(\"terminate values with NUL byte\")),\n \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n@@ -159,6 +161,11 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n \t\t\t\treturn -1;\n \t\t\tstrbuf_addstr(buf, v);\n \t\t\tfree((char *)v);\n+\t\t} else if (types == TYPE_EXPIRY_DATE) {\n+\t\t\ttimestamp_t t;\n+\t\t\tif(git_config_expiry_date(&t, key_, value_) < 0)\n+\t\t\t\treturn -1;\n+\t\t\tstrbuf_addf(buf, \"%\"PRItime, t);\n \t\t} else if (value_) {\n \t\t\tstrbuf_addstr(buf, value_);\n \t\t} else {\n@@ -273,12 +280,13 @@ static char *normalize_value(const char *key, const char *value)\n \tif (!value)\n \t\treturn NULL;\n \n-\tif (types == 0 || types == TYPE_PATH)\n+\tif (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)\n \t\t/*\n \t\t * We don't do normalization for TYPE_PATH here: If\n \t\t * the path is like ~/foobar/, we prefer to store\n \t\t * \"~/foobar/\" in the config file, and to expand the ~\n \t\t * when retrieving the value.\n+\t\t * Also don't do normalization for expiry dates.\n \t\t */\n \t\treturn xstrdup(value);\n \tif (types == TYPE_INT)\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex ab31a3b..2233725 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -416,16 +416,6 @@ static struct reflog_expire_cfg *find_cfg_ent(const char *pattern, size_t len)\n \treturn ent;\n }\n \n-static int parse_expire_cfg_value(const char *var, const char *value, timestamp_t *expire)\n-{\n-\tif (!value)\n-\t\treturn config_error_nonbool(var);\n-\tif (parse_expiry_date(value, expire))\n-\t\treturn error(_(\"'%s' for '%s' is not a valid timestamp\"),\n-\t\t\t value, var);\n-\treturn 0;\n-}\n-\n /* expiry timer slot */\n #define EXPIRE_TOTAL 01\n #define EXPIRE_UNREACH 02\n@@ -443,11 +433,11 @@ static int reflog_expire_config(const char *var, const char *value, void *cb)\n \n \tif (!strcmp(key, \"reflogexpire\")) {\n \t\tslot = EXPIRE_TOTAL;\n-\t\tif (parse_expire_cfg_value(var, value, &expire))\n+\t\tif (git_config_expiry_date(&expire, var, value))\n \t\t\treturn -1;\n \t} else if (!strcmp(key, \"reflogexpireunreachable\")) {\n \t\tslot = EXPIRE_UNREACH;\n-\t\tif (parse_expire_cfg_value(var, value, &expire))\n+\t\tif (git_config_expiry_date(&expire, var, value))\n \t\t\treturn -1;\n \t} else\n \t\treturn git_default_config(var, value, cb);\ndiff --git a/config.c b/config.c\nindex 903abf9..6ded9ce 100644\n--- a/config.c\n+++ b/config.c\n@@ -990,6 +990,15 @@ int git_config_pathname(const char **dest, const char *var, const char *value)\n \treturn 0;\n }\n \n+int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *value)\n+{\n+\tif (!value)\n+\t\treturn config_error_nonbool(var);\n+\tif (parse_expiry_date(value, timestamp))\n+\t\tdie(_(\"failed to parse date_string in: '%s'\"), value);\n+\treturn 0;\n+}\n+\n static int git_default_core_config(const char *var, const char *value)\n {\n \t/* This needs a better name */\ndiff --git a/config.h b/config.h\nindex a49d264..fc66c59 100644\n--- a/config.h\n+++ b/config.h\n@@ -58,6 +58,7 @@ extern int git_config_bool_or_int(const char *, const char *, int *);\n extern int git_config_bool(const char *, const char *);\n extern int git_config_string(const char **, const char *, const char *);\n extern int git_config_pathname(const char **, const char *, const char *);\n+extern int git_config_expiry_date(timestamp_t *, const char *, const char *);\n extern int git_config_set_in_file_gently(const char *, const char *, const char *);\n extern void git_config_set_in_file(const char *, const char *, const char *);\n extern int git_config_set_gently(const char *, const char *);\ndiff --git a/t/helper/test-date.c b/t/helper/test-date.c\nindex f414a3a..ac83687 100644\n--- a/t/helper/test-date.c\n+++ b/t/helper/test-date.c\n@@ -5,6 +5,7 @@ static const char *usage_msg = \"\\n\"\n \" test-date show: [time_t]...\\n\"\n \" test-date parse [date]...\\n\"\n \" test-date approxidate [date]...\\n\"\n+\" test-date timestamp [date]...\\n\"\n \" test-date is64bit\\n\"\n \" test-date time_t-is64bit\\n\";\n \n@@ -71,6 +72,15 @@ static void parse_approxidate(const char **argv, struct timeval *now)\n \t}\n }\n \n+static void parse_approx_timestamp(const char **argv, struct timeval *now)\n+{\n+\tfor (; *argv; argv++) {\n+\t\ttimestamp_t t;\n+\t\tt = approxidate_relative(*argv, now);\n+\t\tprintf(\"%s -> %\"PRItime\"\\n\", *argv, t);\n+\t}\n+}\n+\n int cmd_main(int argc, const char **argv)\n {\n \tstruct timeval now;\n@@ -95,6 +105,8 @@ int cmd_main(int argc, const char **argv)\n \t\tparse_dates(argv+1, &now);\n \telse if (!strcmp(*argv, \"approxidate\"))\n \t\tparse_approxidate(argv+1, &now);\n+\telse if (!strcmp(*argv, \"timestamp\"))\n+\t\tparse_approx_timestamp(argv+1, &now);\n \telse if (!strcmp(*argv, \"is64bit\"))\n \t\treturn sizeof(timestamp_t) == 8 ? 0 : 1;\n \telse if (!strcmp(*argv, \"time_t-is64bit\"))\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 364a537..cbeb9be 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -901,6 +901,36 @@ test_expect_success 'get --path barfs on boolean variable' '\n \ttest_must_fail git config --get --path path.bool\n '\n \n+test_expect_success 'get --expiry-date' '\n+\trel=\"3.weeks.5.days.00:00\" &&\n+\trel_out=\"$rel ->\" &&\n+\tcat >.git/config <<-\\EOF &&\n+\t[date]\n+\tvalid1 = \"3.weeks.5.days 00:00\"\n+\tvalid2 = \"Fri Jun 4 15:46:55 2010\"\n+\tvalid3 = \"2017/11/11 11:11:11PM\"\n+\tvalid4 = \"2017/11/10 09:08:07 PM\"\n+\tvalid5 = \"never\"\n+\tinvalid1 = \"abc\"\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t$(test-date timestamp $rel)\n+\t1275666415\n+\t1510441871\n+\t1510348087\n+\t0\n+\tEOF\n+\t{\n+\t\techo \"$rel_out $(git config --expiry-date date.valid1)\"\n+\t\tgit config --expiry-date date.valid2 &&\n+\t\tgit config --expiry-date date.valid3 &&\n+\t\tgit config --expiry-date date.valid4 &&\n+\t\tgit config --expiry-date date.valid5\n+\t} >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git config --expiry-date date.invalid1\n+'\n+\n cat > expect << EOF\n [quote]\n \tleading = \" test\"\n-- \n2.7.4\n\n"},{"id":"332516","messageId":"CAP8UFD3TbmZ3bRwg-fRoSJWAFaa=UDxVsZphn_3Nt4wMz1N2=A@mail.gmail.com","threadId":"47177","inReplyTo":"1510625073-8842-1-git-send-email-hsed@unimetic.com","subject":"Re: [PATCH V2] config: add --expiry-date","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-11-14T06:21:45Z","receivedAt":"2017-11-14T06:21:51Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Nov 14, 2017 at 3:04 AM, wrote:\n> From: Haaris \n>\n> Description:\n> This patch adds a new option to the config command.\n>\n> Uses flag --expiry-date as a data-type to covert date-strings to\n> timestamps when reading from config files (GET).\n> This flag is ignored on write (SET) because the date-string is stored in\n> config without performing any normalization.\n>\n> Creates a few test cases and documentation since its a new feature.\n>\n> Motivation:\n> A parse_expiry_date() function already existed for api calls,\n> this patch simply allows the function to be used from the command line.\n>\n> Signed-off-by: Haaris \n\nDocumentation/SubmittingPatches contains the following:\n\n\"Also notice that a real name is used in the Signed-off-by: line. Please\ndon't hide your real name.\"\n\nAnd there is the following example before that:\n\n Signed-off-by: Random J Developer \n\nSo it looks like \"a real name\" actually means \"a real firstname and a\nreal surname\".\n\nIt would be nice if your \"Signed-off-by:\" could match this format.\n\nAlso if you have a \"From:\" line at the beginning of the patch, please\nmake sure that the name there is tha same as on the \"Signed-off-by:\".\n\nThanks for working on this,\nChristian.\n"},{"id":"332517","messageId":"xmqqshdh2wln.fsf@gitster.mtv.corp.google.com","threadId":"47177","inReplyTo":"1510625073-8842-1-git-send-email-hsed@unimetic.com","subject":"Re: [PATCH V2] config: add --expiry-date","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-14T06:38:44Z","receivedAt":"2017-11-14T06:38:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"hsed@unimetic.com writes:\n\n> From: Haaris \n>\n> Description:\n> This patch adds a new option to the config command.\n>\n> Uses flag --expiry-date as a data-type to covert date-strings to\n> timestamps when reading from config files (GET).\n> This flag is ignored on write (SET) because the date-string is stored in\n> config without performing any normalization.\n>\n> Creates a few test cases and documentation since its a new feature.\n>\n> Motivation:\n> A parse_expiry_date() function already existed for api calls,\n> this patch simply allows the function to be used from the command line.\n>\n> Signed-off-by: Haaris \n> ---\n\nPlease drop all these section headers; they are irritating. Learn\nfrom \"git log --no-merges\" how the log messages in this project is\nwritten and imitate them. Documentation/SubmittingPatches would be\nhelpful.\n\n\tAdd --expiry-date as a new type 'git config --get' takes,\n\tsimilar to existing --int, --bool, etc. types, so that\n\tscripts can learn values of configuration variables like\n\tgc.reflogexpire (e.g. \"2.weeks\") in a more useful way\n\t(e.g. the timesamp as of two weeks ago, expressed in number\n\tof seconds since epoch).\n\n\tAs a helper function necessary to do this already exists in\n\tthe implementation of builtin/reflog.c, the implementation\n\tis just the matter of moving it to config.c and using it\n\tfrom bultin/config.c, but shuffle the order of the parameter\n\tso that the pointer to the output variable comes first.\n\tThis is to match the convention used by git_config_pathname()\n\tand other helper functions.\n\nor something like that?\n\n> +\t\t} else if (types == TYPE_EXPIRY_DATE) {\n> +\t\t\ttimestamp_t t;\n> +\t\t\tif(git_config_expiry_date(&t, key_, value_) < 0)\n\nStyle.\n\n\tif (git_config_expiry_date(&t, key_, value_) < 0)\n\n> +\t\t\t\treturn -1;\n> +\t\t\tstrbuf_addf(buf, \"%\"PRItime, t);\n> ...\n\nThanks.\n"},{"id":"332548","messageId":"efc9cace-343a-4475-714c-85b499b9f9c9@xiplink.com","threadId":"47177","inReplyTo":"CAP8UFD3TbmZ3bRwg-fRoSJWAFaa=UDxVsZphn_3Nt4wMz1N2=A@mail.gmail.com","subject":"Re: [PATCH V2] config: add --expiry-date","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2017-11-14T16:03:31Z","receivedAt":"2017-11-14T16:03:44Z","isPatch":true,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"On 2017-11-14 01:21 AM, Christian Couder wrote:\n> On Tue, Nov 14, 2017 at 3:04 AM, wrote:\n>> From: Haaris \n>>\n>> Description:\n>> This patch adds a new option to the config command.\n>>\n>> Uses flag --expiry-date as a data-type to covert date-strings to\n>> timestamps when reading from config files (GET).\n>> This flag is ignored on write (SET) because the date-string is stored in\n>> config without performing any normalization.\n>>\n>> Creates a few test cases and documentation since its a new feature.\n>>\n>> Motivation:\n>> A parse_expiry_date() function already existed for api calls,\n>> this patch simply allows the function to be used from the command line.\n>>\n>> Signed-off-by: Haaris \n> \n> Documentation/SubmittingPatches contains the following:\n> \n> \"Also notice that a real name is used in the Signed-off-by: line. Please\n> don't hide your real name.\"\n> \n> And there is the following example before that:\n> \n> Signed-off-by: Random J Developer \n> \n> So it looks like \"a real name\" actually means \"a real firstname and a\n> real surname\".\n> \n> It would be nice if your \"Signed-off-by:\" could match this format.\n\nIt might already match that format if Haaris lives in a society that \nonly uses single names.\n\nStill, such names are unusual enough that it's good to check that new \ncontributors are following the guidelines properly.\n\n\t\tM.\n\n\n> Also if you have a \"From:\" line at the beginning of the patch, please\n> make sure that the name there is tha same as on the \"Signed-off-by:\".\n> \n> Thanks for working on this,\n> Christian.\n> \n"},{"id":"332623","messageId":"d1c0558cd56b4509c3e34daa48fd528d@unimetic.com","threadId":"47177","inReplyTo":"xmqqshdh2wln.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH V2] config: add --expiry-date","fromName":"","fromEmail":"hsed@unimetic.com","sentAt":"2017-11-15T22:10:48Z","receivedAt":"2017-11-15T22:10:58Z","isPatch":true,"sender":{"key":"hsed@unimetic.com","avatar":null},"body":"On 2017-11-14 06:38, Junio C Hamano wrote:\n> hsed@unimetic.com writes:\n> \n>> From: Haaris \n>> \n>> Description:\n>> This patch adds a new option to the config command.\n>> \n>> ...\n>> \n>> Motivation:\n>> A parse_expiry_date() function already existed for api calls,\n>> this patch simply allows the function to be used from the command \n>> line.\n>> \n>> Signed-off-by: Haaris \n>> ---\n> \n> Please drop all these section headers; they are irritating. Learn\n> from \"git log --no-merges\" how the log messages in this project is\n> written and imitate them. Documentation/SubmittingPatches would be\n> helpful.\n> \n> \tAdd --expiry-date as a new type 'git config --get' takes,\n> \tsimilar to existing --int, --bool, etc. types, so that\n> \tscripts can learn values of configuration variables like\n> \tgc.reflogexpire (e.g. \"2.weeks\") in a more useful way\n> \t(e.g. the timesamp as of two weeks ago, expressed in number\n> \tof seconds since epoch).\n> \n> \tAs a helper function necessary to do this already exists in\n> \tthe implementation of builtin/reflog.c, the implementation\n> \tis just the matter of moving it to config.c and using it\n> \tfrom bultin/config.c, but shuffle the order of the parameter\n> \tso that the pointer to the output variable comes first.\n> \tThis is to match the convention used by git_config_pathname()\n> \tand other helper functions.\n> \n> or something like that?\n\nHi,\nI am sorry for not following the format properly. I will change this for\nnext patch update.\n\n> \n>> +\t\t} else if (types == TYPE_EXPIRY_DATE) {\n>> +\t\t\ttimestamp_t t;\n>> +\t\t\tif(git_config_expiry_date(&t, key_, value_) < 0)\n> \n> Style.\n\nSure.\n\n> \n> \tif (git_config_expiry_date(&t, key_, value_) < 0)\n> \n>> +\t\t\t\treturn -1;\n>> +\t\t\tstrbuf_addf(buf, \"%\"PRItime, t);\n>> ...\n> \n> Thanks.\n\n\nKind Regards,\nHaaris\n"},{"id":"332627","messageId":"20171116000547.3246-1-hsed@unimetic.com","threadId":"47177","inReplyTo":"xmqqshdh2wln.fsf@gitster.mtv.corp.google.com","subject":"[PATCH V3] config: add --expiry-date","fromName":"","fromEmail":"hsed@unimetic.com","sentAt":"2017-11-16T00:05:47Z","receivedAt":"2017-11-16T00:06:38Z","isPatch":true,"sender":{"key":"hsed@unimetic.com","avatar":null},"body":"From: Haaris Mehmood \n\nAdd --expiry-date as a data-type for config files when\n'git config --get' is used. This will return any relative\nor fixed dates from config files as a timestamp value.\n\nThis is useful for scripts (e.g. gc.reflogexpire) that work\nwith timestamps so that '2.weeks' can be converted to a format\nacceptable by those scripts/functions.\n\nFollowing the convention of git_config_pathname(), move\nthe helper function required for this feature from\nbuiltin/reflog.c to builtin/config.c where other similar\nfunctions exist (e.g. for --bool or --path), and match\nthe order of parameters with other functions (i.e. output\npointer as first parameter).\n\nSigned-off-by: Haaris Mehmood \n\n---\n Documentation/git-config.txt | 5 +++++\n builtin/config.c | 10 +++++++++-\n builtin/reflog.c | 14 ++------------\n config.c | 9 +++++++++\n config.h | 1 +\n t/helper/test-date.c | 12 ++++++++++++\n t/t1300-repo-config.sh | 30 ++++++++++++++++++++++++++++++\n 7 files changed, 68 insertions(+), 13 deletions(-)\n\nupdate v3: fix style issue\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 4edd09fc6..14da5fc15 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -180,6 +180,11 @@ See also <>.\n \tvalue (but you can use `git config section.variable ~/`\n \tfrom the command line to let your shell do the expansion).\n \n+--expiry-date::\n+\t`git config` will ensure that the output is converted from\n+\ta fixed or relative date-string to a timestamp. This option\n+\thas no effect when setting the value.\n+\n -z::\n --null::\n \tFor all options that output values and/or keys, always\ndiff --git a/builtin/config.c b/builtin/config.c\nindex d13daeeb5..ab5f95476 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -52,6 +52,7 @@ static int show_origin;\n #define TYPE_INT (1<<1)\n #define TYPE_BOOL_OR_INT (1<<2)\n #define TYPE_PATH (1<<3)\n+#define TYPE_EXPIRY_DATE (1<<4)\n \n static struct option builtin_config_options[] = {\n \tOPT_GROUP(N_(\"Config file location\")),\n@@ -80,6 +81,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BIT(0, \"int\", &types, N_(\"value is decimal number\"), TYPE_INT),\n \tOPT_BIT(0, \"bool-or-int\", &types, N_(\"value is --bool or --int\"), TYPE_BOOL_OR_INT),\n \tOPT_BIT(0, \"path\", &types, N_(\"value is a path (file or directory name)\"), TYPE_PATH),\n+\tOPT_BIT(0, \"expiry-date\", &types, N_(\"value is an expiry date\"), TYPE_EXPIRY_DATE),\n \tOPT_GROUP(N_(\"Other\")),\n \tOPT_BOOL('z', \"null\", &end_null, N_(\"terminate values with NUL byte\")),\n \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n@@ -159,6 +161,11 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n \t\t\t\treturn -1;\n \t\t\tstrbuf_addstr(buf, v);\n \t\t\tfree((char *)v);\n+\t\t} else if (types == TYPE_EXPIRY_DATE) {\n+\t\t\ttimestamp_t t;\n+\t\t\tif (git_config_expiry_date(&t, key_, value_) < 0)\n+\t\t\t\treturn -1;\n+\t\t\tstrbuf_addf(buf, \"%\"PRItime, t);\n \t\t} else if (value_) {\n \t\t\tstrbuf_addstr(buf, value_);\n \t\t} else {\n@@ -273,12 +280,13 @@ static char *normalize_value(const char *key, const char *value)\n \tif (!value)\n \t\treturn NULL;\n \n-\tif (types == 0 || types == TYPE_PATH)\n+\tif (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)\n \t\t/*\n \t\t * We don't do normalization for TYPE_PATH here: If\n \t\t * the path is like ~/foobar/, we prefer to store\n \t\t * \"~/foobar/\" in the config file, and to expand the ~\n \t\t * when retrieving the value.\n+\t\t * Also don't do normalization for expiry dates.\n \t\t */\n \t\treturn xstrdup(value);\n \tif (types == TYPE_INT)\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex ab31a3b6a..223372531 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -416,16 +416,6 @@ static struct reflog_expire_cfg *find_cfg_ent(const char *pattern, size_t len)\n \treturn ent;\n }\n \n-static int parse_expire_cfg_value(const char *var, const char *value, timestamp_t *expire)\n-{\n-\tif (!value)\n-\t\treturn config_error_nonbool(var);\n-\tif (parse_expiry_date(value, expire))\n-\t\treturn error(_(\"'%s' for '%s' is not a valid timestamp\"),\n-\t\t\t value, var);\n-\treturn 0;\n-}\n-\n /* expiry timer slot */\n #define EXPIRE_TOTAL 01\n #define EXPIRE_UNREACH 02\n@@ -443,11 +433,11 @@ static int reflog_expire_config(const char *var, const char *value, void *cb)\n \n \tif (!strcmp(key, \"reflogexpire\")) {\n \t\tslot = EXPIRE_TOTAL;\n-\t\tif (parse_expire_cfg_value(var, value, &expire))\n+\t\tif (git_config_expiry_date(&expire, var, value))\n \t\t\treturn -1;\n \t} else if (!strcmp(key, \"reflogexpireunreachable\")) {\n \t\tslot = EXPIRE_UNREACH;\n-\t\tif (parse_expire_cfg_value(var, value, &expire))\n+\t\tif (git_config_expiry_date(&expire, var, value))\n \t\t\treturn -1;\n \t} else\n \t\treturn git_default_config(var, value, cb);\ndiff --git a/config.c b/config.c\nindex 903abf953..6ded9ce98 100644\n--- a/config.c\n+++ b/config.c\n@@ -990,6 +990,15 @@ int git_config_pathname(const char **dest, const char *var, const char *value)\n \treturn 0;\n }\n \n+int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *value)\n+{\n+\tif (!value)\n+\t\treturn config_error_nonbool(var);\n+\tif (parse_expiry_date(value, timestamp))\n+\t\tdie(_(\"failed to parse date_string in: '%s'\"), value);\n+\treturn 0;\n+}\n+\n static int git_default_core_config(const char *var, const char *value)\n {\n \t/* This needs a better name */\ndiff --git a/config.h b/config.h\nindex a49d26441..fc66c5933 100644\n--- a/config.h\n+++ b/config.h\n@@ -58,6 +58,7 @@ extern int git_config_bool_or_int(const char *, const char *, int *);\n extern int git_config_bool(const char *, const char *);\n extern int git_config_string(const char **, const char *, const char *);\n extern int git_config_pathname(const char **, const char *, const char *);\n+extern int git_config_expiry_date(timestamp_t *, const char *, const char *);\n extern int git_config_set_in_file_gently(const char *, const char *, const char *);\n extern void git_config_set_in_file(const char *, const char *, const char *);\n extern int git_config_set_gently(const char *, const char *);\ndiff --git a/t/helper/test-date.c b/t/helper/test-date.c\nindex f414a3ac6..ac8368797 100644\n--- a/t/helper/test-date.c\n+++ b/t/helper/test-date.c\n@@ -5,6 +5,7 @@ static const char *usage_msg = \"\\n\"\n \" test-date show: [time_t]...\\n\"\n \" test-date parse [date]...\\n\"\n \" test-date approxidate [date]...\\n\"\n+\" test-date timestamp [date]...\\n\"\n \" test-date is64bit\\n\"\n \" test-date time_t-is64bit\\n\";\n \n@@ -71,6 +72,15 @@ static void parse_approxidate(const char **argv, struct timeval *now)\n \t}\n }\n \n+static void parse_approx_timestamp(const char **argv, struct timeval *now)\n+{\n+\tfor (; *argv; argv++) {\n+\t\ttimestamp_t t;\n+\t\tt = approxidate_relative(*argv, now);\n+\t\tprintf(\"%s -> %\"PRItime\"\\n\", *argv, t);\n+\t}\n+}\n+\n int cmd_main(int argc, const char **argv)\n {\n \tstruct timeval now;\n@@ -95,6 +105,8 @@ int cmd_main(int argc, const char **argv)\n \t\tparse_dates(argv+1, &now);\n \telse if (!strcmp(*argv, \"approxidate\"))\n \t\tparse_approxidate(argv+1, &now);\n+\telse if (!strcmp(*argv, \"timestamp\"))\n+\t\tparse_approx_timestamp(argv+1, &now);\n \telse if (!strcmp(*argv, \"is64bit\"))\n \t\treturn sizeof(timestamp_t) == 8 ? 0 : 1;\n \telse if (!strcmp(*argv, \"time_t-is64bit\"))\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 364a53700..cbeb9bebe 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -901,6 +901,36 @@ test_expect_success 'get --path barfs on boolean variable' '\n \ttest_must_fail git config --get --path path.bool\n '\n \n+test_expect_success 'get --expiry-date' '\n+\trel=\"3.weeks.5.days.00:00\" &&\n+\trel_out=\"$rel ->\" &&\n+\tcat >.git/config <<-\\EOF &&\n+\t[date]\n+\tvalid1 = \"3.weeks.5.days 00:00\"\n+\tvalid2 = \"Fri Jun 4 15:46:55 2010\"\n+\tvalid3 = \"2017/11/11 11:11:11PM\"\n+\tvalid4 = \"2017/11/10 09:08:07 PM\"\n+\tvalid5 = \"never\"\n+\tinvalid1 = \"abc\"\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t$(test-date timestamp $rel)\n+\t1275666415\n+\t1510441871\n+\t1510348087\n+\t0\n+\tEOF\n+\t{\n+\t\techo \"$rel_out $(git config --expiry-date date.valid1)\"\n+\t\tgit config --expiry-date date.valid2 &&\n+\t\tgit config --expiry-date date.valid3 &&\n+\t\tgit config --expiry-date date.valid4 &&\n+\t\tgit config --expiry-date date.valid5\n+\t} >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git config --expiry-date date.invalid1\n+'\n+\n cat > expect << EOF\n [quote]\n \tleading = \" test\"\n-- \n2.15.0.165.g6fd7fc36d\n\n"},{"id":"332632","messageId":"xmqqlgj7xcuf.fsf@gitster.mtv.corp.google.com","threadId":"47177","inReplyTo":"20171116000547.3246-1-hsed@unimetic.com","subject":"Re: [PATCH V3] config: add --expiry-date","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-16T00:54:16Z","receivedAt":"2017-11-16T00:54:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"hsed@unimetic.com writes:\n\n> From: Haaris Mehmood \n>\n> Add --expiry-date as a data-type for config files when\n> 'git config --get' is used. This will return any relative\n> or fixed dates from config files as a timestamp value.\n>\n> This is useful for scripts (e.g. gc.reflogexpire) that work\n> with timestamps so that '2.weeks' can be converted to a format\n> acceptable by those scripts/functions.\n>\n> Following the convention of git_config_pathname(), move\n> the helper function required for this feature from\n> builtin/reflog.c to builtin/config.c where other similar\n> functions exist (e.g. for --bool or --path), and match\n> the order of parameters with other functions (i.e. output\n> pointer as first parameter).\n>\n> Signed-off-by: Haaris Mehmood \n\nVery nicely explained. I often feel irritated when people further\nrewrite what I wrote for them as an example and make it much worse,\nbut this one definitely is a lot more readable than the \"something\nlike this perhaps?\" in my response to the previous round.\n\n> @@ -273,12 +280,13 @@ static char *normalize_value(const char *key, const char *value)\n> \tif (!value)\n> \t\treturn NULL;\n> \n> -\tif (types == 0 || types == TYPE_PATH)\n> +\tif (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)\n> \t\t/*\n> \t\t * We don't do normalization for TYPE_PATH here: If\n> \t\t * the path is like ~/foobar/, we prefer to store\n> \t\t * \"~/foobar/\" in the config file, and to expand the ~\n> \t\t * when retrieving the value.\n> +\t\t * Also don't do normalization for expiry dates.\n> \t\t */\n> \t\treturn xstrdup(value);\n\nSensible. Just like we want to save \"~u/path\" as-is without\nexpanding the \"~u\"/ part, we want to keep \"2 weeks ago\" as-is.\n\n> -\tif (parse_expiry_date(value, expire))\n> -\t\treturn error(_(\"'%s' for '%s' is not a valid timestamp\"),\n> -\t\t\t value, var);\n> ...\n> +\tif (parse_expiry_date(value, timestamp))\n> +\t\tdie(_(\"failed to parse date_string in: '%s'\"), value);\n\nThis is an unintended change in behaviour (or at least undocumented\nin the log message) for the \"git reflog\" command, no?\n\nNot just the error message is different, but the original gave the\ncalling code a chance to react to the failure by returning -1 from\nthe function, but this makes the command fail outright here.\n\nWould it break anything if you did \"return error()\" just like the\noriginal used to? Are your callers of this new function not\nprepared to see an error return?\n"},{"id":"332765","messageId":"aaff8c91c03bbbc797183f26440496b6@unimetic.com","threadId":"47177","inReplyTo":"xmqqlgj7xcuf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH V3] config: add --expiry-date","fromName":"","fromEmail":"hsed@unimetic.com","sentAt":"2017-11-17T18:15:09Z","receivedAt":"2017-11-17T18:15:15Z","isPatch":true,"sender":{"key":"hsed@unimetic.com","avatar":null},"body":"On 2017-11-16 00:54, Junio C Hamano wrote:\n> \n>> -\tif (parse_expiry_date(value, expire))\n>> -\t\treturn error(_(\"'%s' for '%s' is not a valid timestamp\"),\n>> -\t\t\t value, var);\n>> ...\n>> +\tif (parse_expiry_date(value, timestamp))\n>> +\t\tdie(_(\"failed to parse date_string in: '%s'\"), value);\n> \n> This is an unintended change in behaviour (or at least undocumented\n> in the log message) for the \"git reflog\" command, no?\n> \n> Not just the error message is different, but the original gave the\n> calling code a chance to react to the failure by returning -1 from\n> the function, but this makes the command fail outright here.\n> \n> Would it break anything if you did \"return error()\" just like the\n> original used to? Are your callers of this new function not\n> prepared to see an error return?\n\nI did notice the slight change in the error handling but the new one\nwas copied from one of the other functions in builtin/config.c. I\nwill revert it to the old method and if the new (and other) tests still\npass, I will provide an updated patch.\n\nKind Regards,\nHaaris\n"},{"id":"332786","messageId":"20171118022727.30179-1-hsed@unimetic.com","threadId":"47177","inReplyTo":"xmqqlgj7xcuf.fsf@gitster.mtv.corp.google.com","subject":"[PATCH V4] config: add --expiry-date","fromName":"","fromEmail":"hsed@unimetic.com","sentAt":"2017-11-18T02:27:27Z","receivedAt":"2017-11-18T02:27:50Z","isPatch":true,"sender":{"key":"hsed@unimetic.com","avatar":null},"body":"From: Haaris Mehmood \n\nAdd --expiry-date as a data-type for config files when\n'git config --get' is used. This will return any relative\nor fixed dates from config files as timestamps.\n\nThis is useful for scripts (e.g. gc.reflogexpire) that work\nwith timestamps so that '2.weeks' can be converted to a format\nacceptable by those scripts/functions.\n\nFollowing the convention of git_config_pathname(), move\nthe helper function required for this feature from\nbuiltin/reflog.c to builtin/config.c where other similar\nfunctions exist (e.g. for --bool or --path), and match\nthe order of parameters with other functions (i.e. output\npointer as first parameter).\n\nSigned-off-by: Haaris Mehmood \n\n---\n Documentation/git-config.txt | 5 +++++\n builtin/config.c | 10 +++++++++-\n builtin/reflog.c | 14 ++------------\n config.c | 10 ++++++++++\n config.h | 1 +\n t/helper/test-date.c | 12 ++++++++++++\n t/t1300-repo-config.sh | 30 ++++++++++++++++++++++++++++++\n 7 files changed, 69 insertions(+), 13 deletions(-)\n\nupdate v4: preserve the error handling style of builtin/reflog.c\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 4edd09fc6..14da5fc15 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -180,6 +180,11 @@ See also <>.\n \tvalue (but you can use `git config section.variable ~/`\n \tfrom the command line to let your shell do the expansion).\n \n+--expiry-date::\n+\t`git config` will ensure that the output is converted from\n+\ta fixed or relative date-string to a timestamp. This option\n+\thas no effect when setting the value.\n+\n -z::\n --null::\n \tFor all options that output values and/or keys, always\ndiff --git a/builtin/config.c b/builtin/config.c\nindex d13daeeb5..ab5f95476 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -52,6 +52,7 @@ static int show_origin;\n #define TYPE_INT (1<<1)\n #define TYPE_BOOL_OR_INT (1<<2)\n #define TYPE_PATH (1<<3)\n+#define TYPE_EXPIRY_DATE (1<<4)\n \n static struct option builtin_config_options[] = {\n \tOPT_GROUP(N_(\"Config file location\")),\n@@ -80,6 +81,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BIT(0, \"int\", &types, N_(\"value is decimal number\"), TYPE_INT),\n \tOPT_BIT(0, \"bool-or-int\", &types, N_(\"value is --bool or --int\"), TYPE_BOOL_OR_INT),\n \tOPT_BIT(0, \"path\", &types, N_(\"value is a path (file or directory name)\"), TYPE_PATH),\n+\tOPT_BIT(0, \"expiry-date\", &types, N_(\"value is an expiry date\"), TYPE_EXPIRY_DATE),\n \tOPT_GROUP(N_(\"Other\")),\n \tOPT_BOOL('z', \"null\", &end_null, N_(\"terminate values with NUL byte\")),\n \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n@@ -159,6 +161,11 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n \t\t\t\treturn -1;\n \t\t\tstrbuf_addstr(buf, v);\n \t\t\tfree((char *)v);\n+\t\t} else if (types == TYPE_EXPIRY_DATE) {\n+\t\t\ttimestamp_t t;\n+\t\t\tif (git_config_expiry_date(&t, key_, value_) < 0)\n+\t\t\t\treturn -1;\n+\t\t\tstrbuf_addf(buf, \"%\"PRItime, t);\n \t\t} else if (value_) {\n \t\t\tstrbuf_addstr(buf, value_);\n \t\t} else {\n@@ -273,12 +280,13 @@ static char *normalize_value(const char *key, const char *value)\n \tif (!value)\n \t\treturn NULL;\n \n-\tif (types == 0 || types == TYPE_PATH)\n+\tif (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)\n \t\t/*\n \t\t * We don't do normalization for TYPE_PATH here: If\n \t\t * the path is like ~/foobar/, we prefer to store\n \t\t * \"~/foobar/\" in the config file, and to expand the ~\n \t\t * when retrieving the value.\n+\t\t * Also don't do normalization for expiry dates.\n \t\t */\n \t\treturn xstrdup(value);\n \tif (types == TYPE_INT)\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex ab31a3b6a..223372531 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -416,16 +416,6 @@ static struct reflog_expire_cfg *find_cfg_ent(const char *pattern, size_t len)\n \treturn ent;\n }\n \n-static int parse_expire_cfg_value(const char *var, const char *value, timestamp_t *expire)\n-{\n-\tif (!value)\n-\t\treturn config_error_nonbool(var);\n-\tif (parse_expiry_date(value, expire))\n-\t\treturn error(_(\"'%s' for '%s' is not a valid timestamp\"),\n-\t\t\t value, var);\n-\treturn 0;\n-}\n-\n /* expiry timer slot */\n #define EXPIRE_TOTAL 01\n #define EXPIRE_UNREACH 02\n@@ -443,11 +433,11 @@ static int reflog_expire_config(const char *var, const char *value, void *cb)\n \n \tif (!strcmp(key, \"reflogexpire\")) {\n \t\tslot = EXPIRE_TOTAL;\n-\t\tif (parse_expire_cfg_value(var, value, &expire))\n+\t\tif (git_config_expiry_date(&expire, var, value))\n \t\t\treturn -1;\n \t} else if (!strcmp(key, \"reflogexpireunreachable\")) {\n \t\tslot = EXPIRE_UNREACH;\n-\t\tif (parse_expire_cfg_value(var, value, &expire))\n+\t\tif (git_config_expiry_date(&expire, var, value))\n \t\t\treturn -1;\n \t} else\n \t\treturn git_default_config(var, value, cb);\ndiff --git a/config.c b/config.c\nindex 903abf953..64f8aa42b 100644\n--- a/config.c\n+++ b/config.c\n@@ -990,6 +990,16 @@ int git_config_pathname(const char **dest, const char *var, const char *value)\n \treturn 0;\n }\n \n+int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *value)\n+{\n+\tif (!value)\n+\t\treturn config_error_nonbool(var);\n+\tif (parse_expiry_date(value, timestamp))\n+\t\treturn error(_(\"'%s' for '%s' is not a valid timestamp\"),\n+\t\t\t value, var);\n+\treturn 0;\n+}\n+\n static int git_default_core_config(const char *var, const char *value)\n {\n \t/* This needs a better name */\ndiff --git a/config.h b/config.h\nindex a49d26441..fc66c5933 100644\n--- a/config.h\n+++ b/config.h\n@@ -58,6 +58,7 @@ extern int git_config_bool_or_int(const char *, const char *, int *);\n extern int git_config_bool(const char *, const char *);\n extern int git_config_string(const char **, const char *, const char *);\n extern int git_config_pathname(const char **, const char *, const char *);\n+extern int git_config_expiry_date(timestamp_t *, const char *, const char *);\n extern int git_config_set_in_file_gently(const char *, const char *, const char *);\n extern void git_config_set_in_file(const char *, const char *, const char *);\n extern int git_config_set_gently(const char *, const char *);\ndiff --git a/t/helper/test-date.c b/t/helper/test-date.c\nindex f414a3ac6..ac8368797 100644\n--- a/t/helper/test-date.c\n+++ b/t/helper/test-date.c\n@@ -5,6 +5,7 @@ static const char *usage_msg = \"\\n\"\n \" test-date show: [time_t]...\\n\"\n \" test-date parse [date]...\\n\"\n \" test-date approxidate [date]...\\n\"\n+\" test-date timestamp [date]...\\n\"\n \" test-date is64bit\\n\"\n \" test-date time_t-is64bit\\n\";\n \n@@ -71,6 +72,15 @@ static void parse_approxidate(const char **argv, struct timeval *now)\n \t}\n }\n \n+static void parse_approx_timestamp(const char **argv, struct timeval *now)\n+{\n+\tfor (; *argv; argv++) {\n+\t\ttimestamp_t t;\n+\t\tt = approxidate_relative(*argv, now);\n+\t\tprintf(\"%s -> %\"PRItime\"\\n\", *argv, t);\n+\t}\n+}\n+\n int cmd_main(int argc, const char **argv)\n {\n \tstruct timeval now;\n@@ -95,6 +105,8 @@ int cmd_main(int argc, const char **argv)\n \t\tparse_dates(argv+1, &now);\n \telse if (!strcmp(*argv, \"approxidate\"))\n \t\tparse_approxidate(argv+1, &now);\n+\telse if (!strcmp(*argv, \"timestamp\"))\n+\t\tparse_approx_timestamp(argv+1, &now);\n \telse if (!strcmp(*argv, \"is64bit\"))\n \t\treturn sizeof(timestamp_t) == 8 ? 0 : 1;\n \telse if (!strcmp(*argv, \"time_t-is64bit\"))\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 364a53700..cbeb9bebe 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -901,6 +901,36 @@ test_expect_success 'get --path barfs on boolean variable' '\n \ttest_must_fail git config --get --path path.bool\n '\n \n+test_expect_success 'get --expiry-date' '\n+\trel=\"3.weeks.5.days.00:00\" &&\n+\trel_out=\"$rel ->\" &&\n+\tcat >.git/config <<-\\EOF &&\n+\t[date]\n+\tvalid1 = \"3.weeks.5.days 00:00\"\n+\tvalid2 = \"Fri Jun 4 15:46:55 2010\"\n+\tvalid3 = \"2017/11/11 11:11:11PM\"\n+\tvalid4 = \"2017/11/10 09:08:07 PM\"\n+\tvalid5 = \"never\"\n+\tinvalid1 = \"abc\"\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t$(test-date timestamp $rel)\n+\t1275666415\n+\t1510441871\n+\t1510348087\n+\t0\n+\tEOF\n+\t{\n+\t\techo \"$rel_out $(git config --expiry-date date.valid1)\"\n+\t\tgit config --expiry-date date.valid2 &&\n+\t\tgit config --expiry-date date.valid3 &&\n+\t\tgit config --expiry-date date.valid4 &&\n+\t\tgit config --expiry-date date.valid5\n+\t} >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git config --expiry-date date.invalid1\n+'\n+\n cat > expect << EOF\n [quote]\n \tleading = \" test\"\n-- \n2.15.0.169.g13c4699b6.dirty\n\n"},{"id":"332788","messageId":"xmqq8tf4qmu8.fsf@gitster.mtv.corp.google.com","threadId":"47177","inReplyTo":"20171118022727.30179-1-hsed@unimetic.com","subject":"Re: [PATCH V4] config: add --expiry-date","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-18T03:37:03Z","receivedAt":"2017-11-18T03:37:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"hsed@unimetic.com writes:\n\n> diff --git a/config.c b/config.c\n> index 903abf953..64f8aa42b 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -990,6 +990,16 @@ int git_config_pathname(const char **dest, const char *var, const char *value)\n> \treturn 0;\n> }\n> \n> +int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *value)\n> +{\n> +\tif (!value)\n> +\t\treturn config_error_nonbool(var);\n> +\tif (parse_expiry_date(value, timestamp))\n> +\t\treturn error(_(\"'%s' for '%s' is not a valid timestamp\"),\n> +\t\t\t value, var);\n> +\treturn 0;\n> +}\n> +\n\nI think this is more correct even within the context of this\nfunction than dying, which suggests the need for a slightly related\n(which is not within the scope of this change) clean-up within this\nfile as a #leftoverbits task. I think dying in these value parsers\ngoes against the point of having die_on_error bit in the\nconfig-source structure; Heiko and Peff CC'ed for b2dc0945 (\"do not\ndie when error in config parsing of buf occurs\", 2013-07-12).\n\nThanks; will queue.\n"},{"id":"332906","messageId":"dfa89c0016cc857bee6d9c380c2b2679@unimetic.com","threadId":"47177","inReplyTo":"xmqq8tf4qmu8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH V4] config: add --expiry-date","fromName":"","fromEmail":"hsed@unimetic.com","sentAt":"2017-11-20T14:53:35Z","receivedAt":"2017-11-20T14:53:41Z","isPatch":true,"sender":{"key":"hsed@unimetic.com","avatar":null},"body":"On 2017-11-18 03:37, Junio C Hamano wrote:\n> \n> I think this is more correct even within the context of this\n> function than dying, which suggests the need for a slightly related\n> (which is not within the scope of this change) clean-up within this\n> file as a #leftoverbits task. I think dying in these value parsers\n> goes against the point of having die_on_error bit in the\n> config-source structure; Heiko and Peff CC'ed for b2dc0945 (\"do not\n> die when error in config parsing of buf occurs\", 2013-07-12).\n> \n> Thanks; will queue.\n\nThanks a lot for all your help and I hope to do more patches in future!\n\nKind Regards,\nHaaris\n"},{"id":"332914","messageId":"20171120170443.awpvcuubsi5o6zmp@sigill.intra.peff.net","threadId":"47177","inReplyTo":"xmqq8tf4qmu8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH V4] config: add --expiry-date","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-20T17:04:43Z","receivedAt":"2017-11-20T17:04:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 18, 2017 at 12:37:03PM +0900, Junio C Hamano wrote:\n\n> > +int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *value)\n> > +{\n> > +\tif (!value)\n> > +\t\treturn config_error_nonbool(var);\n> > +\tif (parse_expiry_date(value, timestamp))\n> > +\t\treturn error(_(\"'%s' for '%s' is not a valid timestamp\"),\n> > +\t\t\t value, var);\n> > +\treturn 0;\n> > +}\n> > +\n> \n> I think this is more correct even within the context of this\n> function than dying, which suggests the need for a slightly related\n> (which is not within the scope of this change) clean-up within this\n> file as a #leftoverbits task. I think dying in these value parsers\n> goes against the point of having die_on_error bit in the\n> config-source structure; Heiko and Peff CC'ed for b2dc0945 (\"do not\n> die when error in config parsing of buf occurs\", 2013-07-12).\n\nYes, I agree that ideally the value parsers should avoid dying.\nUnfortunately I think it will involve some heavy refactoring, since\ngit_config_bool(), for instance, does not even have a spot in its\ninterface to return an error.\n\nOf course we can leave those other helpers in place and add a \"gently\"\nform for each. It is really only submodule-config.c that wants to be\ncareful in its callback, so we could just port that. Skimming it over,\nit looks like there are a few git_config_bool() and straight-up die()\ncalls that could be more forgiving.\n\n+cc Stefan, who added the die(). It may be that we don't care that much\nthese days about recovering from broken .gitmodules files.\n\n> Thanks; will queue.\n\nThanks for reviewing. This was from the hackathon last weekend, but I\nmissed the later re-rolls amidst my return travel.\n\n(And thank you, Haaris, for seeing it through!)\n\n-Peff\n"},{"id":"332939","messageId":"CAGZ79ka+5o07cz4A8=Gu_VqO1hYqqO=8Ju1uAaDY23s7xjCWvw@mail.gmail.com","threadId":"47177","inReplyTo":"20171120170443.awpvcuubsi5o6zmp@sigill.intra.peff.net","subject":"Re: [PATCH V4] config: add --expiry-date","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-20T20:28:11Z","receivedAt":"2017-11-20T20:28:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Nov 20, 2017 at 9:04 AM, Jeff King wrote:\n> On Sat, Nov 18, 2017 at 12:37:03PM +0900, Junio C Hamano wrote:\n>\n>> > +int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *value)\n>> > +{\n>> > + if (!value)\n>> > + return config_error_nonbool(var);\n>> > + if (parse_expiry_date(value, timestamp))\n>> > + return error(_(\"'%s' for '%s' is not a valid timestamp\"),\n>> > + value, var);\n>> > + return 0;\n>> > +}\n>> > +\n>>\n>> I think this is more correct even within the context of this\n>> function than dying, which suggests the need for a slightly related\n>> (which is not within the scope of this change) clean-up within this\n>> file as a #leftoverbits task. I think dying in these value parsers\n>> goes against the point of having die_on_error bit in the\n>> config-source structure; Heiko and Peff CC'ed for b2dc0945 (\"do not\n>> die when error in config parsing of buf occurs\", 2013-07-12).\n>\n> Yes, I agree that ideally the value parsers should avoid dying.\n> Unfortunately I think it will involve some heavy refactoring, since\n> git_config_bool(), for instance, does not even have a spot in its\n> interface to return an error.\n>\n> Of course we can leave those other helpers in place and add a \"gently\"\n> form for each. It is really only submodule-config.c that wants to be\n> careful in its callback, so we could just port that. Skimming it over,\n> it looks like there are a few git_config_bool() and straight-up die()\n> calls that could be more forgiving.\n>\n> +cc Stefan, who added the die(). It may be that we don't care that much\n> these days about recovering from broken .gitmodules files.\n\nBy that you mean commits like 37f52e9344 (submodule-config:\nkeep shallow recommendation around, 2016-05-26) for example?\nThat adds a git_config_bool to the submodule config machinery.\n\nI agree that we'd want to be more careful, but for now I'd put it to the\n#leftoverbits.\n\nThanks,\nStefan\n"},{"id":"332943","messageId":"20171120203702.mdd3hkwezxyf7vtg@sigill.intra.peff.net","threadId":"47177","inReplyTo":"CAGZ79ka+5o07cz4A8=Gu_VqO1hYqqO=8Ju1uAaDY23s7xjCWvw@mail.gmail.com","subject":"Re: [PATCH V4] config: add --expiry-date","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-20T20:37:03Z","receivedAt":"2017-11-20T20:37:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 20, 2017 at 12:28:11PM -0800, Stefan Beller wrote:\n\n> > +cc Stefan, who added the die(). It may be that we don't care that much\n> > these days about recovering from broken .gitmodules files.\n> \n> By that you mean commits like 37f52e9344 (submodule-config:\n> keep shallow recommendation around, 2016-05-26) for example?\n> That adds a git_config_bool to the submodule config machinery.\n\nI actually meant ea2fa5a338 (submodule-config: keep update strategy\naround, 2016-02-29), which adds an actual die() into parse_config(). But\nyeah, I think the end result is the same.\n\n> I agree that we'd want to be more careful, but for now I'd put it to the\n> #leftoverbits.\n\nFine by me. While I think the original intent was to be more lenient to\nmalformed .gitmodules, it's not like we're seeing bug reports about it.\n\n-Peff\n"},{"id":"333849","messageId":"20171130111849.GA98794@book.hvoigt.net","threadId":"47177","inReplyTo":"20171120203702.mdd3hkwezxyf7vtg@sigill.intra.peff.net","subject":"Re: [PATCH V4] config: add --expiry-date","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-11-30T11:18:49Z","receivedAt":"2017-11-30T11:58:17Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Mon, Nov 20, 2017 at 03:37:03PM -0500, Jeff King wrote:\n> On Mon, Nov 20, 2017 at 12:28:11PM -0800, Stefan Beller wrote:\n> \n> > > +cc Stefan, who added the die(). It may be that we don't care that much\n> > > these days about recovering from broken .gitmodules files.\n> > \n> > By that you mean commits like 37f52e9344 (submodule-config:\n> > keep shallow recommendation around, 2016-05-26) for example?\n> > That adds a git_config_bool to the submodule config machinery.\n> \n> I actually meant ea2fa5a338 (submodule-config: keep update strategy\n> around, 2016-02-29), which adds an actual die() into parse_config(). But\n> yeah, I think the end result is the same.\n> \n> > I agree that we'd want to be more careful, but for now I'd put it to the\n> > #leftoverbits.\n> \n> Fine by me. While I think the original intent was to be more lenient to\n> malformed .gitmodules, it's not like we're seeing bug reports about it.\n\nMy original intent was not about being more lenient about malformed\n.gitmodules but having a way to deal with repository history that might\nhave a malformed .gitmodules in its history. Since depending on the\nbranch it is on it might be quite carved in stone.\nOn an active project it would not be that easy to rewrite history to get\nout of that situation.\n\nWhen a .gitmodules file in the worktree is malformed it is easy to fix.\nThat is not the case when we are reading configurations from blobs.\n\nMy guess why there are no reports is that maybe not too many users are\nusing this infrastructure yet, plus it is probably seldom that someone\nedits the .gitmodules file by hand which could lead to such a situation.\nBut if such an error occurs it will be very annoying if we die while\nparsing submodule configurations. The only solution I see currently is\nto turn submodule recursion off completely.\n\nBut maybe I am being overly cautious here.\n\nCheers Heiko\n"},{"id":"333860","messageId":"20171130174500.GA16245@sigill.intra.peff.net","threadId":"47177","inReplyTo":"20171130111849.GA98794@book.hvoigt.net","subject":"Re: [PATCH V4] config: add --expiry-date","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-30T17:45:00Z","receivedAt":"2017-11-30T17:45:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 30, 2017 at 12:18:49PM +0100, Heiko Voigt wrote:\n\n> > Fine by me. While I think the original intent was to be more lenient to\n> > malformed .gitmodules, it's not like we're seeing bug reports about it.\n> \n> My original intent was not about being more lenient about malformed\n> .gitmodules but having a way to deal with repository history that might\n> have a malformed .gitmodules in its history. Since depending on the\n> branch it is on it might be quite carved in stone.\n> On an active project it would not be that easy to rewrite history to get\n> out of that situation.\n> \n> When a .gitmodules file in the worktree is malformed it is easy to fix.\n> That is not the case when we are reading configurations from blobs.\n> \n> My guess why there are no reports is that maybe not too many users are\n> using this infrastructure yet, plus it is probably seldom that someone\n> edits the .gitmodules file by hand which could lead to such a situation.\n> But if such an error occurs it will be very annoying if we die while\n> parsing submodule configurations. The only solution I see currently is\n> to turn submodule recursion off completely.\n> \n> But maybe I am being overly cautious here.\n\nAh, OK, that makes a lot of sense to me. Thanks for explaining.\n\nI agree that is a good goal to shoot for in the long term. It's not the\nend of the world if there are a few code paths that may die() for now,\nbut we should try not to add more, and eventually weed out the ones that\ndo.\n\n-Peff\n"}]}