Re: [PATCH] config: added --expiry-date type support
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", , N_("value is decimal number"), TYPE_INT),
OPT_BIT(0, "bool-or-int", , N_("value is --bool or --int"),
TYPE_BOOL_OR_INT),
OPT_BIT(0, "path", , N_("value is a path (file or directory
name)"), TYPE_PATH),
+ OPT_BIT(0, "expiry-date", , N_("value is an expiry date"),
TYPE_EXPIRY_DATE),
OPT_GROUP(N_("Other")),
OPT_BOOL('z', "null", _null, N_("terminate values with NUL
byte")),
OPT_BOOL(0, "name-only", _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(, 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(, 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"
Re: [PATCH] config: added --expiry-date type support
On Sun, Nov 12, 2017 at 12:19:35PM +, 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", , N_("value is decimal number"), TYPE_INT),
> OPT_BIT(0, "bool-or-int", , N_("value is --bool or --int"),
> TYPE_BOOL_OR_INT),
> OPT_BIT(0, "path", , N_("value is a path (file or directory
> name)"), TYPE_PATH),
> + OPT_BIT(0, "expiry-date", , N_("value is an expiry date"),
> TYPE_EXPIRY_DATE),
> OPT_GROUP(N_("Other")),
> OPT_BOOL('z', "null", _null, N_("terminate values with NUL byte")),
> OPT_BOOL(0, "name-only", _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(, 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(, 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
Re: [PATCH] config: added --expiry-date type support
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
On Sun, Nov 12, 2017 at 12:19:35PM +, 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", , N_("value is decimal number"), TYPE_INT),
> OPT_BIT(0, "bool-or-int", , N_("value is --bool or --int"),
> TYPE_BOOL_OR_INT),
> OPT_BIT(0, "path", , N_("value is a path (file or directory
> name)"), TYPE_PATH),
> + OPT_BIT(0, "expiry-date", , N_("value is an expiry date"),
> TYPE_EXPIRY_DATE),
> OPT_GROUP(N_("Other")),
> OPT_BOOL('z', "null", _null, N_("terminate values with NUL byte")),
> OPT_BOOL(0, "name-only", _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(, 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 &&
