From: hsed@unimetic.com Date: Sun, 12 Nov 2017 19:43:43 GMT Subject: Re: [PATCH] config: added --expiry-date type support Message-ID: <97a9b315c7d187b4f0897f93a8d5f6c3@unimetic.com> In-Reply-To: On 2017-11-12 14:55, Jeff King wrote: > Hi, and welcome to the list. Thanks for working on this (for those of > you on the list, this was one of the tasks at the hackathon this > weekend). It was a pleasure meeting everyone and a great experience! > > Kevin already mentioned a few things about the commit message, which I > agree with. Sorry about that and the commit message formatting, now that my mail is being received by git@vger I will try sending patches with the required text, etc. > > It's great that there are new tests. We'll probably need some > documentation, too (especially users will need to know what the output > format means). > True, looking at the repo I found a document here[0] Should I try editing this to add the new option? >> @@ -80,6 +81,7 @@ static struct option builtin_config_options[] = { >> OPT_BIT(0, "int", &types, N_("value is decimal number"), TYPE_INT), >> OPT_BIT(0, "bool-or-int", &types, N_("value is --bool or --int"), >> TYPE_BOOL_OR_INT), >> OPT_BIT(0, "path", &types, N_("value is a path (file or directory >> name)"), TYPE_PATH), >> + OPT_BIT(0, "expiry-date", &types, N_("value is an expiry date"), >> TYPE_EXPIRY_DATE), >> OPT_GROUP(N_("Other")), >> OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL >> byte")), >> OPT_BOOL(0, "name-only", &omit_values, N_("show variable names >> only")), > > We seem to use both "expire" and "expiry" throughout the code and in > user-facing bits (e.g., "gc.reflogExpire" and "gc.logExpiry"). I don't > have a real preference for one versus the other. I just mention it > since > whatever we choose here will be locked in to the interface forever. > I am not sure why do we need to use the 'expir(e/y)' keyword? I think the parse_expiry_date() function still worked for past dates is that intended? Would having it as just '--date' suffice or do you plan to have --date-type which will be different from expiry dates? Anyways, I will use whatever keyword you think is more suitable. Please let me know. >> @@ -159,6 +161,12 @@ static int format_config(struct strbuf *buf, >> const char *key_, const char *value >> return -1; >> strbuf_addstr(buf, v); >> free((char *)v); >> + } else if (types == TYPE_EXPIRY_DATE) { >> + timestamp_t *t = malloc(sizeof(*t)); >> + if(git_config_expiry_date(&t, key_, value_) < 0) >> + return -1; >> + strbuf_addf(buf, "%"PRItime, *t); >> + free((timestamp_t *)t); >> } else if (value_) { > > Since we only need the timestamp variable within this block, we don't > need to use a pointer. We can just do something like: > > } else if (types == TYPE_EXPIRY_DATE) { > timestamp_t t; > if (git_config_expiry_date(&t, key_, value_) < 0) > return -1; > strbuf_addf(buf, "%"PRItime", t); > } > > Note that your new git_config_expiry_date would want to take just a > regular pointer, rather than a pointer-to-pointer. I suspect you picked > that up from git_config_pathname(). It needs the double pointer because > it's storing a string (which is itself a pointer), but we don't need > that here. Yes, I got it from the pathname function, I'll change this to just pointer. > >> diff --git a/config.c b/config.c >> index 903abf9533b18..caa2fd5fb6915 100644 >> --- a/config.c >> +++ b/config.c >> @@ -990,6 +990,15 @@ int git_config_pathname(const char **dest, const >> char *var, const char *value) >> return 0; >> } >> >> +int git_config_expiry_date(timestamp_t **timestamp, const char *var, >> const char *value) >> +{ >> + if (!value) >> + return config_error_nonbool(var); >> + if (!!parse_expiry_date(value, *timestamp)) >> + die(_("failed to parse date_string in: '%s'"), value); >> + return 0; >> +} > > I was surprised that we don't already have a function that does this, > since we parse expiry config elsewhere. We do, but it's just local to > builtin/reflog.c. So perhaps as a preparatory step we should add this > function and convert reflog.c to use it, dropping its custom > parse_expire_cfg_value(). Ok, I will make these changes in reflog.c. > > What's the purpose of the "!!" before parse_expiry_date()? The usual > idiom for that to normalize a non-zero value into "1", but we don't > care > here. I think just: > > if (parse_expiry_date(value, timestamp)) > die(...); > > would be sufficient. No real purpose, I saw it in prev code but I guess that had a different purpose (as you mentioned) I'll change that. >> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh >> index 364a537000bbb..59a35be89e511 100755 >> --- a/t/t1300-repo-config.sh >> +++ b/t/t1300-repo-config.sh >> @@ -901,6 +901,31 @@ test_expect_success 'get --path barfs on boolean >> variable' ' >> test_must_fail git config --get --path path.bool >> ' >> >> +test_expect_success 'get --expiry-date' ' >> + cat >.git/config <<-\EOF && >> + [date] >> + valid1 = "Fri Jun 4 15:46:55 2010" >> + valid2 = "2017/11/11 11:11:11PM" >> + valid3 = "2017/11/10 09:08:07 PM" >> + valid4 = "never" >> + invalid1 = "abc" >> + EOF >> + cat >expect <<-\EOF && >> + 1275666415 >> + 1510441871 >> + 1510348087 >> + 0 >> + EOF >> + { >> + git config --expiry-date date.valid1 && >> + git config --expiry-date date.valid2 && >> + git config --expiry-date date.valid3 && >> + git config --expiry-date date.valid4 >> + } >actual && >> + test_cmp expect actual && >> + test_must_fail git config --expiry-date date.invalid1 >> +' > > This looks good to me. It would be nice if we could test a relative > value (which after all is what we'd expect to see in such a variable). > But there's no way to do it in a robust way, since it will always be > racy with the current timestamp. > > We do have routines that let you make dates relative to a specific > time, > but they're accessible only from t/helper/test-date, not git itself. > > I don't think it's that big a deal, though. We're not testing the time > code here (which is tested elsewhere with test-date), but just that > we're > passing the dates through to be parsed. > > -Peff Is there a way to incorporate that? I will try calling t/helper/test-date within a test but it would probably need to have some parts fixed like seconds and/or minutes to prevent the race condition. Kind Regards, Haaris [0]: https://github.com/git/git/blob/master/Documentation/git-config.txt