git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* fatal: bad numeric config value '60 days' for 'gc.rerereresolved': invalid unit
@ 2017-07-21 12:59 Uwe Hausbrand
  2017-07-21 14:30 ` Martin Ågren
  2017-07-21 14:33 ` Junio C Hamano
  0 siblings, 2 replies; 16+ messages in thread
From: Uwe Hausbrand @ 2017-07-21 12:59 UTC (permalink / raw)
  To: git

Hi all,

seems like there is a bug with "git rerere gc" not understanding grace
periods like "60 days" defined in the config.

What I did:

git config gc.rerereresolved "60 days"
git gc

results in:

Counting objects: 158790, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (26849/26849), done.
Writing objects: 100% (158790/158790), done.
Total 158790 (delta 116114), reused 158790 (delta 116114)
fatal: bad numeric config value '60 days' for 'gc.rerereresolved': invalid unit
error: failed to run rerere

git --version = git version 2.13.0

Best regards,

Uwe

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: fatal: bad numeric config value '60 days' for 'gc.rerereresolved': invalid unit
  2017-07-21 12:59 fatal: bad numeric config value '60 days' for 'gc.rerereresolved': invalid unit Uwe Hausbrand
@ 2017-07-21 14:30 ` Martin Ågren
  2017-07-21 14:33 ` Junio C Hamano
  1 sibling, 0 replies; 16+ messages in thread
From: Martin Ågren @ 2017-07-21 14:30 UTC (permalink / raw)
  To: Uwe Hausbrand; +Cc: Git Mailing List

On 21 July 2017 at 14:59, Uwe Hausbrand <uwe.hausbrand@gmx.de> wrote:
> git config gc.rerereresolved "60 days"
> git gc
>
> results in:
[...]
> fatal: bad numeric config value '60 days' for 'gc.rerereresolved': invalid unit
> error: failed to run rerere

It's not entirely clear, but "man git-gc" does seem to suggest -- at
least to me -- that one can say "60 days". Especially after reading
about gc.reflogExpire a few paragraphs earlier, where "90 days" and "3
months" are given as examples. Luckily, "man git-config" is a bit
clearer. It says that "records ... are kept for this many days". The
latter matches the implementation and the tests, which are all written
to work with just an integer.

So "git config gc.rerereresolved 60" should work. Hope that helps.

Martin

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: fatal: bad numeric config value '60 days' for 'gc.rerereresolved': invalid unit
  2017-07-21 12:59 fatal: bad numeric config value '60 days' for 'gc.rerereresolved': invalid unit Uwe Hausbrand
  2017-07-21 14:30 ` Martin Ågren
@ 2017-07-21 14:33 ` Junio C Hamano
  2017-07-21 17:35   ` Uwe Hausbrand
  2017-08-19 20:30   ` [PATCH 0/2] accept non-integer for "this many days" expiry specification Junio C Hamano
  1 sibling, 2 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-07-21 14:33 UTC (permalink / raw)
  To: Uwe Hausbrand; +Cc: git

Uwe Hausbrand <uwe.hausbrand@gmx.de> writes:

> seems like there is a bug with "git rerere gc" not understanding grace
> periods like "60 days" defined in the config.
>
> What I did:
>
> git config gc.rerereresolved "60 days"

Let's see how the variable is explained in the documentation.

    gc.rerereResolved::
            Records of conflicted merge you resolved earlier are
            kept for this many days when 'git rerere gc' is run.
            The default is 60 days.  See linkgit:git-rerere[1].

Notice that "for this many days" tries to (and probably
unsuccessfully) tell you that this variable is expected to be set to
an integer [*1*], counted in "days".  IOW, you'd want "60" instead.

Having said that, it may not be a bad idea to enumerate these
"expected to be an integer that counts in some unit" variables that
are described in a similar way (i.e. look for "this many" in
Documentation/config.txt), and then for each of them that could be
counted in different unit (e.g. it is not outrageously wrong to
expect that you could specify that rerere records that are older
than 3 months are expired):

 - decide what kind of quantity the variable specifies (e.g. "this
   many days" and "this many seconds" variables are giving a
   "timeperiod").

 - keep the code that reacts to an integer without any unit to
   behave the same (e.g. "[gc] rerereresolved = 30" will keep
   meaning "30 days");

 - extend the code so that when the value given is not an integer,
   it tries to parse it as a specification for the expected quantity
   (e.g. "this many days" and "this many seconds" variables would
   understand if you said "60 days" or "2 months")


[Footnote]

 *1* I think we actually expect a scaled integer whenever we expect
     an integral value, so you probably could say "6k" to specify
     "6,000 days"; "days" not being any of the recognised unit
     suffix like k, M, G, etc. is where "invalid unit" comes from.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: fatal: bad numeric config value '60 days' for 'gc.rerereresolved': invalid unit
  2017-07-21 14:33 ` Junio C Hamano
@ 2017-07-21 17:35   ` Uwe Hausbrand
  2017-08-19 20:30   ` [PATCH 0/2] accept non-integer for "this many days" expiry specification Junio C Hamano
  1 sibling, 0 replies; 16+ messages in thread
From: Uwe Hausbrand @ 2017-07-21 17:35 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

Yeah, after I checked the code I saw that this is interpreted as
integer and fixed my configuration

2017-07-21 16:33 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
> Uwe Hausbrand <uwe.hausbrand@gmx.de> writes:
>
>> seems like there is a bug with "git rerere gc" not understanding grace
>> periods like "60 days" defined in the config.
>>
>> What I did:
>>
>> git config gc.rerereresolved "60 days"
>
> Let's see how the variable is explained in the documentation.
>
>     gc.rerereResolved::
>             Records of conflicted merge you resolved earlier are
>             kept for this many days when 'git rerere gc' is run.
>             The default is 60 days.  See linkgit:git-rerere[1].
>
> Notice that "for this many days" tries to (and probably
> unsuccessfully) tell you that this variable is expected to be set to
> an integer [*1*], counted in "days".  IOW, you'd want "60" instead.
>
> Having said that, it may not be a bad idea to enumerate these
> "expected to be an integer that counts in some unit" variables that
> are described in a similar way (i.e. look for "this many" in
> Documentation/config.txt), and then for each of them that could be
> counted in different unit (e.g. it is not outrageously wrong to
> expect that you could specify that rerere records that are older
> than 3 months are expired):
>
>  - decide what kind of quantity the variable specifies (e.g. "this
>    many days" and "this many seconds" variables are giving a
>    "timeperiod").
>
>  - keep the code that reacts to an integer without any unit to
>    behave the same (e.g. "[gc] rerereresolved = 30" will keep
>    meaning "30 days");
>
>  - extend the code so that when the value given is not an integer,
>    it tries to parse it as a specification for the expected quantity
>    (e.g. "this many days" and "this many seconds" variables would
>    understand if you said "60 days" or "2 months")
>
>
> [Footnote]
>
>  *1* I think we actually expect a scaled integer whenever we expect
>      an integral value, so you probably could say "6k" to specify
>      "6,000 days"; "days" not being any of the recognised unit
>      suffix like k, M, G, etc. is where "invalid unit" comes from.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 0/2] accept non-integer for "this many days" expiry specification
  2017-07-21 14:33 ` Junio C Hamano
  2017-07-21 17:35   ` Uwe Hausbrand
@ 2017-08-19 20:30   ` Junio C Hamano
  2017-08-19 20:30     ` [PATCH 1/2] rerere: represent time duration in timestamp_t internally Junio C Hamano
                       ` (2 more replies)
  1 sibling, 3 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-08-19 20:30 UTC (permalink / raw)
  To: git; +Cc: Uwe Hausbrand

About a month ago, we wondered why

	[gc]
		rerereResolved = 5.days

does not work and if we want to do something better.  

Here is a pair of patches that attempt to improve the situation.

Junio C Hamano (2):
  rerere: represent time duration in timestamp_t internally
  rerere: allow approxidate in gc.rerereResolved/gc.rerereUnresolved

 Documentation/config.txt |  2 ++
 config.c                 |  4 ++--
 config.h                 |  3 +++
 rerere.c                 | 46 +++++++++++++++++++++++++++++++++-------------
 4 files changed, 40 insertions(+), 15 deletions(-)

-- 
2.14.1-405-g52c75fc716


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/2] rerere: represent time duration in timestamp_t internally
  2017-08-19 20:30   ` [PATCH 0/2] accept non-integer for "this many days" expiry specification Junio C Hamano
@ 2017-08-19 20:30     ` Junio C Hamano
  2017-08-19 20:30     ` [PATCH 2/2] rerere: allow approxidate in gc.rerereResolved/gc.rerereUnresolved Junio C Hamano
  2017-08-22 21:46     ` [PATCH v2 0/6] accept non-integer for "this many days" expiry specification Junio C Hamano
  2 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-08-19 20:30 UTC (permalink / raw)
  To: git; +Cc: Uwe Hausbrand

The two configuration variables, gc.rerereResolved and
gc.rerereUnresolved, are measured in days and are passed as such
into the prune_one() helper function, which worked in time_t to see
if an entry in the rerere database is past its expiry.

Instead, have the caller turn the number of days into the expiry
timestamp.  Further, use timestamp_t instead of time_t.  This will
make it possible to extend the way the configuration variable is
spelled by using date.c::parse_expiry_date() that gives the expiry
timestamp in timestamp_t.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 rerere.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/rerere.c b/rerere.c
index 70634d456c..f0b4bce881 100644
--- a/rerere.c
+++ b/rerere.c
@@ -1133,14 +1133,14 @@ int rerere_forget(struct pathspec *pathspec)
  * Garbage collection support
  */
 
-static time_t rerere_created_at(struct rerere_id *id)
+static timestamp_t rerere_created_at(struct rerere_id *id)
 {
 	struct stat st;
 
 	return stat(rerere_path(id, "preimage"), &st) ? (time_t) 0 : st.st_mtime;
 }
 
-static time_t rerere_last_used_at(struct rerere_id *id)
+static timestamp_t rerere_last_used_at(struct rerere_id *id)
 {
 	struct stat st;
 
@@ -1157,11 +1157,11 @@ static void unlink_rr_item(struct rerere_id *id)
 	id->collection->status[id->variant] = 0;
 }
 
-static void prune_one(struct rerere_id *id, time_t now,
-		      int cutoff_resolve, int cutoff_noresolve)
+static void prune_one(struct rerere_id *id,
+		      timestamp_t cutoff_resolve, timestamp_t cutoff_noresolve)
 {
-	time_t then;
-	int cutoff;
+	timestamp_t then;
+	timestamp_t cutoff;
 
 	then = rerere_last_used_at(id);
 	if (then)
@@ -1172,25 +1172,35 @@ static void prune_one(struct rerere_id *id, time_t now,
 			return;
 		cutoff = cutoff_noresolve;
 	}
-	if (then < now - cutoff * 86400)
+	if (then < cutoff)
 		unlink_rr_item(id);
 }
 
+static void config_get_expiry(const char *key, timestamp_t *cutoff, timestamp_t now)
+{
+	int days;
+
+	if (!git_config_get_int(key, &days)) {
+		const int scale = 86400;
+		*cutoff = now - days * scale;
+	}
+}
+
 void rerere_gc(struct string_list *rr)
 {
 	struct string_list to_remove = STRING_LIST_INIT_DUP;
 	DIR *dir;
 	struct dirent *e;
 	int i;
-	time_t now = time(NULL);
-	int cutoff_noresolve = 15;
-	int cutoff_resolve = 60;
+	timestamp_t now = time(NULL);
+	timestamp_t cutoff_noresolve = now - 15 * 86400;
+	timestamp_t cutoff_resolve = now - 60 * 86400;
 
 	if (setup_rerere(rr, 0) < 0)
 		return;
 
-	git_config_get_int("gc.rerereresolved", &cutoff_resolve);
-	git_config_get_int("gc.rerereunresolved", &cutoff_noresolve);
+	config_get_expiry("gc.rerereresolved", &cutoff_resolve, now);
+	config_get_expiry("gc.rerereunresolved", &cutoff_noresolve, now);
 	git_config(git_default_config, NULL);
 	dir = opendir(git_path("rr-cache"));
 	if (!dir)
@@ -1211,7 +1221,7 @@ void rerere_gc(struct string_list *rr)
 		for (id.variant = 0, id.collection = rr_dir;
 		     id.variant < id.collection->status_nr;
 		     id.variant++) {
-			prune_one(&id, now, cutoff_resolve, cutoff_noresolve);
+			prune_one(&id, cutoff_resolve, cutoff_noresolve);
 			if (id.collection->status[id.variant])
 				now_empty = 0;
 		}
-- 
2.14.1-405-g52c75fc716


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/2] rerere: allow approxidate in gc.rerereResolved/gc.rerereUnresolved
  2017-08-19 20:30   ` [PATCH 0/2] accept non-integer for "this many days" expiry specification Junio C Hamano
  2017-08-19 20:30     ` [PATCH 1/2] rerere: represent time duration in timestamp_t internally Junio C Hamano
@ 2017-08-19 20:30     ` Junio C Hamano
  2017-08-20 21:45       ` Ramsay Jones
  2017-08-22 21:46     ` [PATCH v2 0/6] accept non-integer for "this many days" expiry specification Junio C Hamano
  2 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2017-08-19 20:30 UTC (permalink / raw)
  To: git; +Cc: Uwe Hausbrand

These two configuration variables are described in the documentation
to take an expiry period expressed in the number of days:

    gc.rerereResolved::
	    Records of conflicted merge you resolved earlier are
	    kept for this many days when 'git rerere gc' is run.
	    The default is 60 days.

    gc.rerereUnresolved::
	    Records of conflicted merge you have not resolved are
	    kept for this many days when 'git rerere gc' is run.
	    The default is 15 days.

There is no strong reason not to allow a more general "approxidate"
expiry specification, e.g. "5.days.ago", or "never".

Tweak the config_get_expiry() helper introduced in the previous step
to use date.c::parse_expiry_date() to do so.

In the future, we may find other variables that only allow an
integer that specifies "this many days" (or other unit of time) and
allow them to also do the same, and at that point we probably would
want to move the helper to a place that is not specific to the
rerere machinery.  Perhaps config.c would be such a good neutral
place, as it will allow git_parse_signed() to go back to static to
the file.

But this will do for now.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt |  2 ++
 config.c                 |  4 ++--
 config.h                 |  3 +++
 rerere.c                 | 14 ++++++++++++--
 4 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d5c9c4cab6..ac95f5f954 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1553,11 +1553,13 @@ gc.<pattern>.reflogExpireUnreachable::
 gc.rerereResolved::
 	Records of conflicted merge you resolved earlier are
 	kept for this many days when 'git rerere gc' is run.
+	You can also use more human-readable "1.month.ago", etc.
 	The default is 60 days.  See linkgit:git-rerere[1].
 
 gc.rerereUnresolved::
 	Records of conflicted merge you have not resolved are
 	kept for this many days when 'git rerere gc' is run.
+	You can also use more human-readable "1.month.ago", etc.
 	The default is 15 days.  See linkgit:git-rerere[1].
 
 gitcvs.commitMsgAnnotation::
diff --git a/config.c b/config.c
index 231f9a750b..ac9071c5cf 100644
--- a/config.c
+++ b/config.c
@@ -769,7 +769,7 @@ static int parse_unit_factor(const char *end, uintmax_t *val)
 	return 0;
 }
 
-static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
+int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
 {
 	if (value && *value) {
 		char *end;
@@ -799,7 +799,7 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
 	return 0;
 }
 
-static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)
+int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)
 {
 	if (value && *value) {
 		char *end;
diff --git a/config.h b/config.h
index 0352da117b..039a9295de 100644
--- a/config.h
+++ b/config.h
@@ -215,4 +215,7 @@ struct key_value_info {
 extern NORETURN void git_die_config(const char *key, const char *err, ...) __attribute__((format(printf, 2, 3)));
 extern NORETURN void git_die_config_linenr(const char *key, const char *filename, int linenr);
 
+int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max);
+int git_parse_signed(const char *value, intmax_t *ret, intmax_t max);
+
 #endif /* CONFIG_H */
diff --git a/rerere.c b/rerere.c
index f0b4bce881..8bbdfe8569 100644
--- a/rerere.c
+++ b/rerere.c
@@ -1178,11 +1178,21 @@ static void prune_one(struct rerere_id *id,
 
 static void config_get_expiry(const char *key, timestamp_t *cutoff, timestamp_t now)
 {
-	int days;
+	char *expiry_string;
+	intmax_t days;
+	timestamp_t when;
 
-	if (!git_config_get_int(key, &days)) {
+	if (git_config_get_string(key, &expiry_string))
+		return;
+
+	if (git_parse_signed(expiry_string, &days, maximum_signed_value_of_type(int))) {
 		const int scale = 86400;
 		*cutoff = now - days * scale;
+		return;
+	}
+
+	if (!parse_expiry_date(expiry_string, &when)) {
+		*cutoff = when;
 	}
 }
 
-- 
2.14.1-405-g52c75fc716


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] rerere: allow approxidate in gc.rerereResolved/gc.rerereUnresolved
  2017-08-19 20:30     ` [PATCH 2/2] rerere: allow approxidate in gc.rerereResolved/gc.rerereUnresolved Junio C Hamano
@ 2017-08-20 21:45       ` Ramsay Jones
  2017-08-21  0:20         ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: Ramsay Jones @ 2017-08-20 21:45 UTC (permalink / raw)
  To: Junio C Hamano, git; +Cc: Uwe Hausbrand



On 19/08/17 21:30, Junio C Hamano wrote:
> These two configuration variables are described in the documentation
> to take an expiry period expressed in the number of days:
> 
>     gc.rerereResolved::
> 	    Records of conflicted merge you resolved earlier are
> 	    kept for this many days when 'git rerere gc' is run.
> 	    The default is 60 days.
> 
>     gc.rerereUnresolved::
> 	    Records of conflicted merge you have not resolved are
> 	    kept for this many days when 'git rerere gc' is run.
> 	    The default is 15 days.
> 
> There is no strong reason not to allow a more general "approxidate"
> expiry specification, e.g. "5.days.ago", or "never".
> 
> Tweak the config_get_expiry() helper introduced in the previous step
> to use date.c::parse_expiry_date() to do so.
> 
> In the future, we may find other variables that only allow an
> integer that specifies "this many days" (or other unit of time) and
> allow them to also do the same, and at that point we probably would
> want to move the helper to a place that is not specific to the
> rerere machinery.  Perhaps config.c would be such a good neutral
> place, as it will allow git_parse_signed() to go back to static to
> the file.

You make git_parse_unsigned() external in this patch, in addition
to git_parse_signed(), but don't actually call it. Was that intended?

ATB,
Ramsay Jones

> 
> But this will do for now.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/config.txt |  2 ++
>  config.c                 |  4 ++--
>  config.h                 |  3 +++
>  rerere.c                 | 14 ++++++++++++--
>  4 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index d5c9c4cab6..ac95f5f954 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1553,11 +1553,13 @@ gc.<pattern>.reflogExpireUnreachable::
>  gc.rerereResolved::
>  	Records of conflicted merge you resolved earlier are
>  	kept for this many days when 'git rerere gc' is run.
> +	You can also use more human-readable "1.month.ago", etc.
>  	The default is 60 days.  See linkgit:git-rerere[1].
>  
>  gc.rerereUnresolved::
>  	Records of conflicted merge you have not resolved are
>  	kept for this many days when 'git rerere gc' is run.
> +	You can also use more human-readable "1.month.ago", etc.
>  	The default is 15 days.  See linkgit:git-rerere[1].
>  
>  gitcvs.commitMsgAnnotation::
> diff --git a/config.c b/config.c
> index 231f9a750b..ac9071c5cf 100644
> --- a/config.c
> +++ b/config.c
> @@ -769,7 +769,7 @@ static int parse_unit_factor(const char *end, uintmax_t *val)
>  	return 0;
>  }
>  
> -static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
> +int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
>  {
>  	if (value && *value) {
>  		char *end;
> @@ -799,7 +799,7 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
>  	return 0;
>  }
>  
> -static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)
> +int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)
>  {
>  	if (value && *value) {
>  		char *end;
> diff --git a/config.h b/config.h
> index 0352da117b..039a9295de 100644
> --- a/config.h
> +++ b/config.h
> @@ -215,4 +215,7 @@ struct key_value_info {
>  extern NORETURN void git_die_config(const char *key, const char *err, ...) __attribute__((format(printf, 2, 3)));
>  extern NORETURN void git_die_config_linenr(const char *key, const char *filename, int linenr);
>  
> +int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max);
> +int git_parse_signed(const char *value, intmax_t *ret, intmax_t max);
> +
>  #endif /* CONFIG_H */
> diff --git a/rerere.c b/rerere.c
> index f0b4bce881..8bbdfe8569 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -1178,11 +1178,21 @@ static void prune_one(struct rerere_id *id,
>  
>  static void config_get_expiry(const char *key, timestamp_t *cutoff, timestamp_t now)
>  {
> -	int days;
> +	char *expiry_string;
> +	intmax_t days;
> +	timestamp_t when;
>  
> -	if (!git_config_get_int(key, &days)) {
> +	if (git_config_get_string(key, &expiry_string))
> +		return;
> +
> +	if (git_parse_signed(expiry_string, &days, maximum_signed_value_of_type(int))) {
>  		const int scale = 86400;
>  		*cutoff = now - days * scale;
> +		return;
> +	}
> +
> +	if (!parse_expiry_date(expiry_string, &when)) {
> +		*cutoff = when;
>  	}
>  }
>  
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] rerere: allow approxidate in gc.rerereResolved/gc.rerereUnresolved
  2017-08-20 21:45       ` Ramsay Jones
@ 2017-08-21  0:20         ` Junio C Hamano
  0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-08-21  0:20 UTC (permalink / raw)
  To: Ramsay Jones; +Cc: git, Uwe Hausbrand

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> On 19/08/17 21:30, Junio C Hamano wrote:
>
>> In the future, we may find other variables that only allow an
>> integer that specifies "this many days" (or other unit of time) and
>> allow them to also do the same, and at that point we probably would
>> want to move the helper to a place that is not specific to the
>> rerere machinery.  Perhaps config.c would be such a good neutral
>> place, as it will allow git_parse_signed() to go back to static to
>> the file.
>
> You make git_parse_unsigned() external in this patch, in addition
> to git_parse_signed(), but don't actually call it. Was that intended?

Yes, it was done on purpose for symmetry and was done before I had
that "perhaps this can be moved to config.c" vision.

Perhaps I should follow up the topic with [PATCH 3/2] to really move
it to config.c to clean it up.  But not today.

Thanks.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 0/6] accept non-integer for "this many days" expiry specification
  2017-08-19 20:30   ` [PATCH 0/2] accept non-integer for "this many days" expiry specification Junio C Hamano
  2017-08-19 20:30     ` [PATCH 1/2] rerere: represent time duration in timestamp_t internally Junio C Hamano
  2017-08-19 20:30     ` [PATCH 2/2] rerere: allow approxidate in gc.rerereResolved/gc.rerereUnresolved Junio C Hamano
@ 2017-08-22 21:46     ` Junio C Hamano
  2017-08-22 21:46       ` [PATCH v2 1/6] t4200: give us a clean slate after "rerere gc" tests Junio C Hamano
                         ` (5 more replies)
  2 siblings, 6 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-08-22 21:46 UTC (permalink / raw)
  To: git; +Cc: Uwe Hausbrand, Ramsay Jones

About a month ago, we wondered why

	[gc]
		rerereResolved = 5.days

does not work and if we want to do something better.  

This is an update to my first attempt.  It turns out that I needed
more patches to clean up the tests before I can write new tests for
this feature.

The implementation of the feature itself has not changed much,
except that it now lives in config.c for easier reuse by other
codepaths in the future.

Junio C Hamano (6):
  t4200: give us a clean slate after "rerere gc" tests
  t4200: make "rerere gc" test more robust
  t4200: gather "rerere gc" together
  t4200: parameterize "rerere gc" custom expiry test
  rerere: represent time duration in timestamp_t internally
  rerere: allow approxidate in gc.rerereResolved/gc.rerereUnresolved

 Documentation/config.txt |  2 ++
 config.c                 | 22 +++++++++++++++++++
 config.h                 |  3 +++
 rerere.c                 | 26 +++++++++++-----------
 t/t4200-rerere.sh        | 57 +++++++++++++++++++++++++++++++++---------------
 5 files changed, 79 insertions(+), 31 deletions(-)


The interdiff between the original one and this version looks like
this.

 config.c          | 26 +++++++++++++++++++++++--
 config.h          |  6 +++---
 rerere.c          | 24 ++---------------------
 t/t4200-rerere.sh | 57 +++++++++++++++++++++++++++++++++++++------------------
 4 files changed, 68 insertions(+), 45 deletions(-)

diff --git a/config.c b/config.c
index ac9071c5cf..ffca15f594 100644
--- a/config.c
+++ b/config.c
@@ -769,7 +769,7 @@ static int parse_unit_factor(const char *end, uintmax_t *val)
 	return 0;
 }
 
-int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
+static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
 {
 	if (value && *value) {
 		char *end;
@@ -799,7 +799,7 @@ int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
 	return 0;
 }
 
-int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)
+static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)
 {
 	if (value && *value) {
 		char *end;
@@ -2066,6 +2066,28 @@ int git_config_get_expiry(const char *key, const char **output)
 	return ret;
 }
 
+int git_config_get_expiry_in_days(const char *key, timestamp_t *expiry, timestamp_t now)
+{
+	char *expiry_string;
+	intmax_t days;
+	timestamp_t when;
+
+	if (git_config_get_string(key, &expiry_string))
+		return 1; /* no such thing */
+
+	if (git_parse_signed(expiry_string, &days, maximum_signed_value_of_type(int))) {
+		const int scale = 86400;
+		*expiry = now - days * scale;
+		return 0;
+	}
+
+	if (!parse_expiry_date(expiry_string, &when)) {
+		*expiry = when;
+		return 0;
+	}
+	return -1; /* thing exists but cannot be parsed */
+}
+
 int git_config_get_untracked_cache(void)
 {
 	int val = -1;
diff --git a/config.h b/config.h
index 039a9295de..34ddd3eb8d 100644
--- a/config.h
+++ b/config.h
@@ -205,6 +205,9 @@ extern int git_config_get_max_percent_split_change(void);
 /* This dies if the configured or default date is in the future */
 extern int git_config_get_expiry(const char *key, const char **output);
 
+/* parse either "this many days" integer, or "5.days.ago" approxidate */
+extern int git_config_get_expiry_in_days(const char *key, timestamp_t *, timestamp_t now);
+
 struct key_value_info {
 	const char *filename;
 	int linenr;
@@ -215,7 +218,4 @@ struct key_value_info {
 extern NORETURN void git_die_config(const char *key, const char *err, ...) __attribute__((format(printf, 2, 3)));
 extern NORETURN void git_die_config_linenr(const char *key, const char *filename, int linenr);
 
-int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max);
-int git_parse_signed(const char *value, intmax_t *ret, intmax_t max);
-
 #endif /* CONFIG_H */
diff --git a/rerere.c b/rerere.c
index 8bbdfe8569..d77235645e 100644
--- a/rerere.c
+++ b/rerere.c
@@ -1176,26 +1176,6 @@ static void prune_one(struct rerere_id *id,
 		unlink_rr_item(id);
 }
 
-static void config_get_expiry(const char *key, timestamp_t *cutoff, timestamp_t now)
-{
-	char *expiry_string;
-	intmax_t days;
-	timestamp_t when;
-
-	if (git_config_get_string(key, &expiry_string))
-		return;
-
-	if (git_parse_signed(expiry_string, &days, maximum_signed_value_of_type(int))) {
-		const int scale = 86400;
-		*cutoff = now - days * scale;
-		return;
-	}
-
-	if (!parse_expiry_date(expiry_string, &when)) {
-		*cutoff = when;
-	}
-}
-
 void rerere_gc(struct string_list *rr)
 {
 	struct string_list to_remove = STRING_LIST_INIT_DUP;
@@ -1209,8 +1189,8 @@ void rerere_gc(struct string_list *rr)
 	if (setup_rerere(rr, 0) < 0)
 		return;
 
-	config_get_expiry("gc.rerereresolved", &cutoff_resolve, now);
-	config_get_expiry("gc.rerereunresolved", &cutoff_noresolve, now);
+	git_config_get_expiry_in_days("gc.rerereresolved", &cutoff_resolve, now);
+	git_config_get_expiry_in_days("gc.rerereunresolved", &cutoff_noresolve, now);
 	git_config(git_default_config, NULL);
 	dir = opendir(git_path("rr-cache"));
 	if (!dir)
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 1a080e7823..d97d2bebc9 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -239,6 +239,43 @@ test_expect_success 'old records rest in peace' '
 	! test -f $rr2/preimage
 '
 
+rerere_gc_custom_expiry_test () {
+	five_days="$1" right_now="$2"
+	test_expect_success "rerere gc with custom expiry ($five_days, $right_now)" '
+		rm -fr .git/rr-cache &&
+		rr=.git/rr-cache/$_z40 &&
+		mkdir -p "$rr" &&
+		>"$rr/preimage" &&
+		>"$rr/postimage" &&
+
+		two_days_ago=$((-2*86400)) &&
+		test-chmtime =$two_days_ago "$rr/preimage" &&
+		test-chmtime =$two_days_ago "$rr/postimage" &&
+
+		find .git/rr-cache -type f | sort >original &&
+
+		git -c "gc.rerereresolved=$five_days" \
+		    -c "gc.rerereunresolved=$five_days" rerere gc &&
+		find .git/rr-cache -type f | sort >actual &&
+		test_cmp original actual &&
+
+		git -c "gc.rerereresolved=$five_days" \
+		    -c "gc.rerereunresolved=$right_now" rerere gc &&
+		find .git/rr-cache -type f | sort >actual &&
+		test_cmp original actual &&
+
+		git -c "gc.rerereresolved=$right_now" \
+		    -c "gc.rerereunresolved=$right_now" rerere gc &&
+		find .git/rr-cache -type f | sort >actual &&
+		>expect &&
+		test_cmp expect actual
+	'
+}
+
+rerere_gc_custom_expiry_test 5 0
+
+rerere_gc_custom_expiry_test 5.days.ago now
+
 test_expect_success 'setup: file2 added differently in two branches' '
 	git reset --hard &&
 
@@ -419,24 +456,6 @@ count_pre_post () {
 	test_line_count = "$2" actual
 }
 
-test_expect_success 'rerere gc' '
-	find .git/rr-cache -type f >original &&
-	xargs test-chmtime -172800 <original &&
-
-	git -c gc.rerereresolved=5 -c gc.rerereunresolved=5 rerere gc &&
-	find .git/rr-cache -type f >actual &&
-	test_cmp original actual &&
-
-	git -c gc.rerereresolved=5 -c gc.rerereunresolved=0 rerere gc &&
-	find .git/rr-cache -type f >actual &&
-	test_cmp original actual &&
-
-	git -c gc.rerereresolved=0 -c gc.rerereunresolved=0 rerere gc &&
-	find .git/rr-cache -type f >actual &&
-	>expect &&
-	test_cmp expect actual
-'
-
 merge_conflict_resolve () {
 	git reset --hard &&
 	test_must_fail git merge six.1 &&
@@ -446,6 +465,8 @@ merge_conflict_resolve () {
 }
 
 test_expect_success 'multiple identical conflicts' '
+	rm -fr .git/rr-cache &&
+	mkdir .git/rr-cache &&
 	git reset --hard &&
 
 	test_seq 1 6 >early &&

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 1/6] t4200: give us a clean slate after "rerere gc" tests
  2017-08-22 21:46     ` [PATCH v2 0/6] accept non-integer for "this many days" expiry specification Junio C Hamano
@ 2017-08-22 21:46       ` Junio C Hamano
  2017-08-22 21:46       ` [PATCH v2 2/6] t4200: make "rerere gc" test more robust Junio C Hamano
                         ` (4 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-08-22 21:46 UTC (permalink / raw)
  To: git

The "multiple identical conflicts" test counts the number of entries
in the rerere database after trying a handful of mergy operations
and recording their resolutions, but without initializing the rerere
database to a known state, allowing the state left by previous tests
to trigger a false failure.  Make it robust by cleaning the database
before it starts.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4200-rerere.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 1a080e7823..8f5f268baf 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -446,6 +446,8 @@ merge_conflict_resolve () {
 }
 
 test_expect_success 'multiple identical conflicts' '
+	rm -fr .git/rr-cache &&
+	mkdir .git/rr-cache &&
 	git reset --hard &&
 
 	test_seq 1 6 >early &&
-- 
2.14.1-427-g5711bb0564


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 2/6] t4200: make "rerere gc" test more robust
  2017-08-22 21:46     ` [PATCH v2 0/6] accept non-integer for "this many days" expiry specification Junio C Hamano
  2017-08-22 21:46       ` [PATCH v2 1/6] t4200: give us a clean slate after "rerere gc" tests Junio C Hamano
@ 2017-08-22 21:46       ` Junio C Hamano
  2017-08-22 21:46       ` [PATCH v2 3/6] t4200: gather "rerere gc" together Junio C Hamano
                         ` (3 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-08-22 21:46 UTC (permalink / raw)
  To: git

The test blindly trusted that there may be _some_ entries left in
the rerere database, and used them by updating their timestamps to
see if the gc threshold variables are honoured correctly.  This
won't work if there is no entries in the database when the test
begins.

Instead, clear the rerere database, and populate it with a few known
entries (which are bogus, but for the purpose of testing "garbage
collection", it does not matter---we want to make sure we collect
old cruft, even if the files are corrupt rerere database entries),
and use them for the expiry test.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4200-rerere.sh | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 8f5f268baf..1e23031cdb 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -420,19 +420,28 @@ count_pre_post () {
 }
 
 test_expect_success 'rerere gc' '
-	find .git/rr-cache -type f >original &&
-	xargs test-chmtime -172800 <original &&
+	rm -fr .git/rr-cache &&
+	rr=.git/rr-cache/$_z40 &&
+	mkdir -p "$rr" &&
+	>"$rr/preimage" &&
+	>"$rr/postimage" &&
+
+	two_days_ago=$((-2*86400)) &&
+	test-chmtime =$two_days_ago "$rr/preimage" &&
+	test-chmtime =$two_days_ago "$rr/postimage" &&
+
+	find .git/rr-cache -type f | sort >original &&
 
 	git -c gc.rerereresolved=5 -c gc.rerereunresolved=5 rerere gc &&
-	find .git/rr-cache -type f >actual &&
+	find .git/rr-cache -type f | sort >actual &&
 	test_cmp original actual &&
 
 	git -c gc.rerereresolved=5 -c gc.rerereunresolved=0 rerere gc &&
-	find .git/rr-cache -type f >actual &&
+	find .git/rr-cache -type f | sort >actual &&
 	test_cmp original actual &&
 
 	git -c gc.rerereresolved=0 -c gc.rerereunresolved=0 rerere gc &&
-	find .git/rr-cache -type f >actual &&
+	find .git/rr-cache -type f | sort >actual &&
 	>expect &&
 	test_cmp expect actual
 '
-- 
2.14.1-427-g5711bb0564


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 3/6] t4200: gather "rerere gc" together
  2017-08-22 21:46     ` [PATCH v2 0/6] accept non-integer for "this many days" expiry specification Junio C Hamano
  2017-08-22 21:46       ` [PATCH v2 1/6] t4200: give us a clean slate after "rerere gc" tests Junio C Hamano
  2017-08-22 21:46       ` [PATCH v2 2/6] t4200: make "rerere gc" test more robust Junio C Hamano
@ 2017-08-22 21:46       ` Junio C Hamano
  2017-08-22 21:46       ` [PATCH v2 4/6] t4200: parameterize "rerere gc" custom expiry test Junio C Hamano
                         ` (2 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-08-22 21:46 UTC (permalink / raw)
  To: git

Move the "rerere gc with custom expiry" test up, so that it is close
to the more "rerere gc" tests.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4200-rerere.sh | 54 +++++++++++++++++++++++++++---------------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 1e23031cdb..b007b67e9a 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -239,6 +239,33 @@ test_expect_success 'old records rest in peace' '
 	! test -f $rr2/preimage
 '
 
+test_expect_success 'rerere gc with custom expiry' '
+	rm -fr .git/rr-cache &&
+	rr=.git/rr-cache/$_z40 &&
+	mkdir -p "$rr" &&
+	>"$rr/preimage" &&
+	>"$rr/postimage" &&
+
+	two_days_ago=$((-2*86400)) &&
+	test-chmtime =$two_days_ago "$rr/preimage" &&
+	test-chmtime =$two_days_ago "$rr/postimage" &&
+
+	find .git/rr-cache -type f | sort >original &&
+
+	git -c gc.rerereresolved=5 -c gc.rerereunresolved=5 rerere gc &&
+	find .git/rr-cache -type f | sort >actual &&
+	test_cmp original actual &&
+
+	git -c gc.rerereresolved=5 -c gc.rerereunresolved=0 rerere gc &&
+	find .git/rr-cache -type f | sort >actual &&
+	test_cmp original actual &&
+
+	git -c gc.rerereresolved=0 -c gc.rerereunresolved=0 rerere gc &&
+	find .git/rr-cache -type f | sort >actual &&
+	>expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'setup: file2 added differently in two branches' '
 	git reset --hard &&
 
@@ -419,33 +446,6 @@ count_pre_post () {
 	test_line_count = "$2" actual
 }
 
-test_expect_success 'rerere gc' '
-	rm -fr .git/rr-cache &&
-	rr=.git/rr-cache/$_z40 &&
-	mkdir -p "$rr" &&
-	>"$rr/preimage" &&
-	>"$rr/postimage" &&
-
-	two_days_ago=$((-2*86400)) &&
-	test-chmtime =$two_days_ago "$rr/preimage" &&
-	test-chmtime =$two_days_ago "$rr/postimage" &&
-
-	find .git/rr-cache -type f | sort >original &&
-
-	git -c gc.rerereresolved=5 -c gc.rerereunresolved=5 rerere gc &&
-	find .git/rr-cache -type f | sort >actual &&
-	test_cmp original actual &&
-
-	git -c gc.rerereresolved=5 -c gc.rerereunresolved=0 rerere gc &&
-	find .git/rr-cache -type f | sort >actual &&
-	test_cmp original actual &&
-
-	git -c gc.rerereresolved=0 -c gc.rerereunresolved=0 rerere gc &&
-	find .git/rr-cache -type f | sort >actual &&
-	>expect &&
-	test_cmp expect actual
-'
-
 merge_conflict_resolve () {
 	git reset --hard &&
 	test_must_fail git merge six.1 &&
-- 
2.14.1-427-g5711bb0564


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 4/6] t4200: parameterize "rerere gc" custom expiry test
  2017-08-22 21:46     ` [PATCH v2 0/6] accept non-integer for "this many days" expiry specification Junio C Hamano
                         ` (2 preceding siblings ...)
  2017-08-22 21:46       ` [PATCH v2 3/6] t4200: gather "rerere gc" together Junio C Hamano
@ 2017-08-22 21:46       ` Junio C Hamano
  2017-08-22 21:46       ` [PATCH v2 5/6] rerere: represent time duration in timestamp_t internally Junio C Hamano
  2017-08-22 21:46       ` [PATCH v2 6/6] rerere: allow approxidate in gc.rerereResolved/gc.rerereUnresolved Junio C Hamano
  5 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-08-22 21:46 UTC (permalink / raw)
  To: git

The test creates a rerere database entry that is two days old, and
tries to expire with three different custom expiry configuration
(keep ones less than 5 days old, keep ones used less than 5 days
ago, and expire everything right now).

We'll be introducing a different way to spell the same "5 days" and
"right now" parameter in a later step; parameterize the test to make
it easier to test the new spelling when it happens.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4200-rerere.sh | 58 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 25 deletions(-)

diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index b007b67e9a..8d437534f2 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -239,32 +239,40 @@ test_expect_success 'old records rest in peace' '
 	! test -f $rr2/preimage
 '
 
-test_expect_success 'rerere gc with custom expiry' '
-	rm -fr .git/rr-cache &&
-	rr=.git/rr-cache/$_z40 &&
-	mkdir -p "$rr" &&
-	>"$rr/preimage" &&
-	>"$rr/postimage" &&
-
-	two_days_ago=$((-2*86400)) &&
-	test-chmtime =$two_days_ago "$rr/preimage" &&
-	test-chmtime =$two_days_ago "$rr/postimage" &&
-
-	find .git/rr-cache -type f | sort >original &&
-
-	git -c gc.rerereresolved=5 -c gc.rerereunresolved=5 rerere gc &&
-	find .git/rr-cache -type f | sort >actual &&
-	test_cmp original actual &&
-
-	git -c gc.rerereresolved=5 -c gc.rerereunresolved=0 rerere gc &&
-	find .git/rr-cache -type f | sort >actual &&
-	test_cmp original actual &&
+rerere_gc_custom_expiry_test () {
+	five_days="$1" right_now="$2"
+	test_expect_success "rerere gc with custom expiry ($five_days, $right_now)" '
+		rm -fr .git/rr-cache &&
+		rr=.git/rr-cache/$_z40 &&
+		mkdir -p "$rr" &&
+		>"$rr/preimage" &&
+		>"$rr/postimage" &&
+
+		two_days_ago=$((-2*86400)) &&
+		test-chmtime =$two_days_ago "$rr/preimage" &&
+		test-chmtime =$two_days_ago "$rr/postimage" &&
+
+		find .git/rr-cache -type f | sort >original &&
+
+		git -c "gc.rerereresolved=$five_days" \
+		    -c "gc.rerereunresolved=$five_days" rerere gc &&
+		find .git/rr-cache -type f | sort >actual &&
+		test_cmp original actual &&
+
+		git -c "gc.rerereresolved=$five_days" \
+		    -c "gc.rerereunresolved=$right_now" rerere gc &&
+		find .git/rr-cache -type f | sort >actual &&
+		test_cmp original actual &&
+
+		git -c "gc.rerereresolved=$right_now" \
+		    -c "gc.rerereunresolved=$right_now" rerere gc &&
+		find .git/rr-cache -type f | sort >actual &&
+		>expect &&
+		test_cmp expect actual
+	'
+}
 
-	git -c gc.rerereresolved=0 -c gc.rerereunresolved=0 rerere gc &&
-	find .git/rr-cache -type f | sort >actual &&
-	>expect &&
-	test_cmp expect actual
-'
+rerere_gc_custom_expiry_test 5 0
 
 test_expect_success 'setup: file2 added differently in two branches' '
 	git reset --hard &&
-- 
2.14.1-427-g5711bb0564


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 5/6] rerere: represent time duration in timestamp_t internally
  2017-08-22 21:46     ` [PATCH v2 0/6] accept non-integer for "this many days" expiry specification Junio C Hamano
                         ` (3 preceding siblings ...)
  2017-08-22 21:46       ` [PATCH v2 4/6] t4200: parameterize "rerere gc" custom expiry test Junio C Hamano
@ 2017-08-22 21:46       ` Junio C Hamano
  2017-08-22 21:46       ` [PATCH v2 6/6] rerere: allow approxidate in gc.rerereResolved/gc.rerereUnresolved Junio C Hamano
  5 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-08-22 21:46 UTC (permalink / raw)
  To: git

The two configuration variables, gc.rerereResolved and
gc.rerereUnresolved, are measured in days and are passed as such
into the prune_one() helper function, which worked in time_t to see
if an entry in the rerere database is past its expiry.

Instead, have the caller turn the number of days into the expiry
timestamp.  Further, use timestamp_t instead of time_t.  This will
make it possible to extend the way the configuration variable is
spelled by using date.c::parse_expiry_date() that gives the expiry
timestamp in timestamp_t.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 rerere.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/rerere.c b/rerere.c
index 70634d456c..f0b4bce881 100644
--- a/rerere.c
+++ b/rerere.c
@@ -1133,14 +1133,14 @@ int rerere_forget(struct pathspec *pathspec)
  * Garbage collection support
  */
 
-static time_t rerere_created_at(struct rerere_id *id)
+static timestamp_t rerere_created_at(struct rerere_id *id)
 {
 	struct stat st;
 
 	return stat(rerere_path(id, "preimage"), &st) ? (time_t) 0 : st.st_mtime;
 }
 
-static time_t rerere_last_used_at(struct rerere_id *id)
+static timestamp_t rerere_last_used_at(struct rerere_id *id)
 {
 	struct stat st;
 
@@ -1157,11 +1157,11 @@ static void unlink_rr_item(struct rerere_id *id)
 	id->collection->status[id->variant] = 0;
 }
 
-static void prune_one(struct rerere_id *id, time_t now,
-		      int cutoff_resolve, int cutoff_noresolve)
+static void prune_one(struct rerere_id *id,
+		      timestamp_t cutoff_resolve, timestamp_t cutoff_noresolve)
 {
-	time_t then;
-	int cutoff;
+	timestamp_t then;
+	timestamp_t cutoff;
 
 	then = rerere_last_used_at(id);
 	if (then)
@@ -1172,25 +1172,35 @@ static void prune_one(struct rerere_id *id, time_t now,
 			return;
 		cutoff = cutoff_noresolve;
 	}
-	if (then < now - cutoff * 86400)
+	if (then < cutoff)
 		unlink_rr_item(id);
 }
 
+static void config_get_expiry(const char *key, timestamp_t *cutoff, timestamp_t now)
+{
+	int days;
+
+	if (!git_config_get_int(key, &days)) {
+		const int scale = 86400;
+		*cutoff = now - days * scale;
+	}
+}
+
 void rerere_gc(struct string_list *rr)
 {
 	struct string_list to_remove = STRING_LIST_INIT_DUP;
 	DIR *dir;
 	struct dirent *e;
 	int i;
-	time_t now = time(NULL);
-	int cutoff_noresolve = 15;
-	int cutoff_resolve = 60;
+	timestamp_t now = time(NULL);
+	timestamp_t cutoff_noresolve = now - 15 * 86400;
+	timestamp_t cutoff_resolve = now - 60 * 86400;
 
 	if (setup_rerere(rr, 0) < 0)
 		return;
 
-	git_config_get_int("gc.rerereresolved", &cutoff_resolve);
-	git_config_get_int("gc.rerereunresolved", &cutoff_noresolve);
+	config_get_expiry("gc.rerereresolved", &cutoff_resolve, now);
+	config_get_expiry("gc.rerereunresolved", &cutoff_noresolve, now);
 	git_config(git_default_config, NULL);
 	dir = opendir(git_path("rr-cache"));
 	if (!dir)
@@ -1211,7 +1221,7 @@ void rerere_gc(struct string_list *rr)
 		for (id.variant = 0, id.collection = rr_dir;
 		     id.variant < id.collection->status_nr;
 		     id.variant++) {
-			prune_one(&id, now, cutoff_resolve, cutoff_noresolve);
+			prune_one(&id, cutoff_resolve, cutoff_noresolve);
 			if (id.collection->status[id.variant])
 				now_empty = 0;
 		}
-- 
2.14.1-427-g5711bb0564


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 6/6] rerere: allow approxidate in gc.rerereResolved/gc.rerereUnresolved
  2017-08-22 21:46     ` [PATCH v2 0/6] accept non-integer for "this many days" expiry specification Junio C Hamano
                         ` (4 preceding siblings ...)
  2017-08-22 21:46       ` [PATCH v2 5/6] rerere: represent time duration in timestamp_t internally Junio C Hamano
@ 2017-08-22 21:46       ` Junio C Hamano
  5 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-08-22 21:46 UTC (permalink / raw)
  To: git; +Cc: Ramsay Jones

These two configuration variables are described in the documentation
to take an expiry period expressed in the number of days:

    gc.rerereResolved::
	    Records of conflicted merge you resolved earlier are
	    kept for this many days when 'git rerere gc' is run.
	    The default is 60 days.

    gc.rerereUnresolved::
	    Records of conflicted merge you have not resolved are
	    kept for this many days when 'git rerere gc' is run.
	    The default is 15 days.

There is no strong reason not to allow a more general "approxidate"
expiry specification, e.g. "5.days.ago", or "never".

Rename the config_get_expiry() helper introduced in the previous
step to git_config_get_expiry_in_days() and move it to a more
generic place, config.c, and use date.c::parse_expiry_date() to do
so.  Give it an ability to allow the caller to tell among three
cases (i.e. there is no "gc.rerereResolved" config, there is and it
is correctly parsed into the *expiry variable, and there was an
error in parsing the given value).  The current caller can work
correctly without using the return value, though.

In the future, we may find other variables that only allow an
integer that specifies "this many days" or other unit of time, and
when it happens we may need to drop "_days" suffix from the name of
the function and instead pass the "scale" value as another parameter.

But this will do for now.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt |  2 ++
 config.c                 | 22 ++++++++++++++++++++++
 config.h                 |  3 +++
 rerere.c                 | 14 ++------------
 t/t4200-rerere.sh        |  2 ++
 5 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d5c9c4cab6..ac95f5f954 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1553,11 +1553,13 @@ gc.<pattern>.reflogExpireUnreachable::
 gc.rerereResolved::
 	Records of conflicted merge you resolved earlier are
 	kept for this many days when 'git rerere gc' is run.
+	You can also use more human-readable "1.month.ago", etc.
 	The default is 60 days.  See linkgit:git-rerere[1].
 
 gc.rerereUnresolved::
 	Records of conflicted merge you have not resolved are
 	kept for this many days when 'git rerere gc' is run.
+	You can also use more human-readable "1.month.ago", etc.
 	The default is 15 days.  See linkgit:git-rerere[1].
 
 gitcvs.commitMsgAnnotation::
diff --git a/config.c b/config.c
index 231f9a750b..ffca15f594 100644
--- a/config.c
+++ b/config.c
@@ -2066,6 +2066,28 @@ int git_config_get_expiry(const char *key, const char **output)
 	return ret;
 }
 
+int git_config_get_expiry_in_days(const char *key, timestamp_t *expiry, timestamp_t now)
+{
+	char *expiry_string;
+	intmax_t days;
+	timestamp_t when;
+
+	if (git_config_get_string(key, &expiry_string))
+		return 1; /* no such thing */
+
+	if (git_parse_signed(expiry_string, &days, maximum_signed_value_of_type(int))) {
+		const int scale = 86400;
+		*expiry = now - days * scale;
+		return 0;
+	}
+
+	if (!parse_expiry_date(expiry_string, &when)) {
+		*expiry = when;
+		return 0;
+	}
+	return -1; /* thing exists but cannot be parsed */
+}
+
 int git_config_get_untracked_cache(void)
 {
 	int val = -1;
diff --git a/config.h b/config.h
index 0352da117b..34ddd3eb8d 100644
--- a/config.h
+++ b/config.h
@@ -205,6 +205,9 @@ extern int git_config_get_max_percent_split_change(void);
 /* This dies if the configured or default date is in the future */
 extern int git_config_get_expiry(const char *key, const char **output);
 
+/* parse either "this many days" integer, or "5.days.ago" approxidate */
+extern int git_config_get_expiry_in_days(const char *key, timestamp_t *, timestamp_t now);
+
 struct key_value_info {
 	const char *filename;
 	int linenr;
diff --git a/rerere.c b/rerere.c
index f0b4bce881..d77235645e 100644
--- a/rerere.c
+++ b/rerere.c
@@ -1176,16 +1176,6 @@ static void prune_one(struct rerere_id *id,
 		unlink_rr_item(id);
 }
 
-static void config_get_expiry(const char *key, timestamp_t *cutoff, timestamp_t now)
-{
-	int days;
-
-	if (!git_config_get_int(key, &days)) {
-		const int scale = 86400;
-		*cutoff = now - days * scale;
-	}
-}
-
 void rerere_gc(struct string_list *rr)
 {
 	struct string_list to_remove = STRING_LIST_INIT_DUP;
@@ -1199,8 +1189,8 @@ void rerere_gc(struct string_list *rr)
 	if (setup_rerere(rr, 0) < 0)
 		return;
 
-	config_get_expiry("gc.rerereresolved", &cutoff_resolve, now);
-	config_get_expiry("gc.rerereunresolved", &cutoff_noresolve, now);
+	git_config_get_expiry_in_days("gc.rerereresolved", &cutoff_resolve, now);
+	git_config_get_expiry_in_days("gc.rerereunresolved", &cutoff_noresolve, now);
 	git_config(git_default_config, NULL);
 	dir = opendir(git_path("rr-cache"));
 	if (!dir)
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 8d437534f2..d97d2bebc9 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -274,6 +274,8 @@ rerere_gc_custom_expiry_test () {
 
 rerere_gc_custom_expiry_test 5 0
 
+rerere_gc_custom_expiry_test 5.days.ago now
+
 test_expect_success 'setup: file2 added differently in two branches' '
 	git reset --hard &&
 
-- 
2.14.1-427-g5711bb0564


^ permalink raw reply related	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2017-08-22 21:47 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-21 12:59 fatal: bad numeric config value '60 days' for 'gc.rerereresolved': invalid unit Uwe Hausbrand
2017-07-21 14:30 ` Martin Ågren
2017-07-21 14:33 ` Junio C Hamano
2017-07-21 17:35   ` Uwe Hausbrand
2017-08-19 20:30   ` [PATCH 0/2] accept non-integer for "this many days" expiry specification Junio C Hamano
2017-08-19 20:30     ` [PATCH 1/2] rerere: represent time duration in timestamp_t internally Junio C Hamano
2017-08-19 20:30     ` [PATCH 2/2] rerere: allow approxidate in gc.rerereResolved/gc.rerereUnresolved Junio C Hamano
2017-08-20 21:45       ` Ramsay Jones
2017-08-21  0:20         ` Junio C Hamano
2017-08-22 21:46     ` [PATCH v2 0/6] accept non-integer for "this many days" expiry specification Junio C Hamano
2017-08-22 21:46       ` [PATCH v2 1/6] t4200: give us a clean slate after "rerere gc" tests Junio C Hamano
2017-08-22 21:46       ` [PATCH v2 2/6] t4200: make "rerere gc" test more robust Junio C Hamano
2017-08-22 21:46       ` [PATCH v2 3/6] t4200: gather "rerere gc" together Junio C Hamano
2017-08-22 21:46       ` [PATCH v2 4/6] t4200: parameterize "rerere gc" custom expiry test Junio C Hamano
2017-08-22 21:46       ` [PATCH v2 5/6] rerere: represent time duration in timestamp_t internally Junio C Hamano
2017-08-22 21:46       ` [PATCH v2 6/6] rerere: allow approxidate in gc.rerereResolved/gc.rerereUnresolved Junio C Hamano

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