git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: hsed@unimetic.com
To: Jeff King <peff@peff.net>
Cc: Kevin Daudt <me@ikke.info>, Git <git@vger.kernel.org>
Subject: Re: [PATCH] config: added --expiry-date type support
Date: Sun, 12 Nov 2017 19:43:43 +0000	[thread overview]
Message-ID: <97a9b315c7d187b4f0897f93a8d5f6c3@unimetic.com> (raw)
In-Reply-To: <a05a8e8020ec31cfd9a0271ce2a00034@unimetic.com>

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

  parent reply	other threads:[~2017-11-12 19:43 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-12 12:19 [PATCH] config: added --expiry-date type support Haaris
2017-11-12 13:55 ` Kevin Daudt
2017-11-12 14:22   ` Jeff King
2017-11-12 14:55 ` Jeff King
     [not found]   ` <a05a8e8020ec31cfd9a0271ce2a00034@unimetic.com>
2017-11-12 19:43     ` hsed [this message]
2017-11-14  2:04   ` [PATCH V2] config: add --expiry-date hsed
2017-11-14  6:21     ` Christian Couder
2017-11-14 16:03       ` Marc Branchaud
2017-11-14  6:38     ` Junio C Hamano
2017-11-15 22:10       ` hsed
2017-11-16  0:05       ` [PATCH V3] " hsed
2017-11-16  0:54         ` Junio C Hamano
2017-11-17 18:15           ` hsed
2017-11-18  2:27           ` [PATCH V4] " hsed
2017-11-18  3:37             ` Junio C Hamano
2017-11-20 14:53               ` hsed
2017-11-20 17:04               ` Jeff King
2017-11-20 20:28                 ` Stefan Beller
2017-11-20 20:37                   ` Jeff King
2017-11-30 11:18                     ` Heiko Voigt
2017-11-30 17:45                       ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=97a9b315c7d187b4f0897f93a8d5f6c3@unimetic.com \
    --to=hsed@unimetic.com \
    --cc=git@vger.kernel.org \
    --cc=me@ikke.info \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).