[2/2] read-cache: use time_t instead of unsigned long
diff mbox series

Message ID 20181113074017.17292-1-carenas@gmail.com
State New
Headers show
Series
  • Untitled series #42181
Related show

Commit Message

Carlo Marcelo Arenas Belón Nov. 13, 2018, 7:40 a.m. UTC
There are still some more possible improvements around this code but
they are orthogonal to this change :

* migrate to approxidate_careful or parse_expiry_date
* maybe make sure only approxidate are used for expiration

Changes in v2:
* improved commit message as suggested by Eric
* failsafe against time_t truncation as suggested by Junio

-- >8 --
Subject: [PATCH v2 2/2] read-cache: use time specific types instead of
 unsigned long

b968372279 ("read-cache: unlink old sharedindex files", 2017-03-06)
introduced get_shared_index_expire_date using unsigned long to track
the modification times of a shared index.

dddbad728c ("timestamp_t: a new data type for timestamps", 2017-04-26)
shows why that might be problematic so move to timestamp_t/time_t.

if time_t can't represent a valid time keep the indexes for failsafe

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 read-cache.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Johannes Schindelin Nov. 13, 2018, 8:49 a.m. UTC | #1
Hi,

On Mon, 12 Nov 2018, Carlo Marcelo Arenas Belón wrote:

> There are still some more possible improvements around this code but
> they are orthogonal to this change :
> 
> * migrate to approxidate_careful or parse_expiry_date
> * maybe make sure only approxidate are used for expiration
> 
> Changes in v2:
> * improved commit message as suggested by Eric
> * failsafe against time_t truncation as suggested by Junio
> 
> -- >8 --
> Subject: [PATCH v2 2/2] read-cache: use time specific types instead of
>  unsigned long
> 
> b968372279 ("read-cache: unlink old sharedindex files", 2017-03-06)
> introduced get_shared_index_expire_date using unsigned long to track
> the modification times of a shared index.
> 
> dddbad728c ("timestamp_t: a new data type for timestamps", 2017-04-26)
> shows why that might be problematic so move to timestamp_t/time_t.
> 
> if time_t can't represent a valid time keep the indexes for failsafe

Is this sentence incomplete? What are those "indexes"?

Thanks,
Johannes

> 
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  read-cache.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/read-cache.c b/read-cache.c
> index 7b1354d759..7d322f11c8 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2625,9 +2625,9 @@ static int write_split_index(struct index_state *istate,
>  
>  static const char *shared_index_expire = "2.weeks.ago";
>  
> -static unsigned long get_shared_index_expire_date(void)
> +static timestamp_t get_shared_index_expire_date(void)
>  {
> -	static unsigned long shared_index_expire_date;
> +	static timestamp_t shared_index_expire_date;
>  	static int shared_index_expire_date_prepared;
>  
>  	if (!shared_index_expire_date_prepared) {
> @@ -2643,12 +2643,12 @@ static unsigned long get_shared_index_expire_date(void)
>  static int should_delete_shared_index(const char *shared_index_path)
>  {
>  	struct stat st;
> -	unsigned long expiration;
> +	time_t expiration;
> +	timestamp_t t = get_shared_index_expire_date();
>  
> -	/* Check timestamp */
> -	expiration = get_shared_index_expire_date();
> -	if (!expiration)
> +	if (!t || date_overflows(t))
>  		return 0;
> +	expiration = t;
>  	if (stat(shared_index_path, &st))
>  		return error_errno(_("could not stat '%s'"), shared_index_path);
>  	if (st.st_mtime > expiration)
> -- 
> 2.19.1.856.g8858448bb
> 
>
Carlo Marcelo Arenas Belón Nov. 13, 2018, 9:10 a.m. UTC | #2
On Tue, Nov 13, 2018 at 12:49 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Mon, 12 Nov 2018, Carlo Marcelo Arenas Belón wrote:
>>
> > if time_t can't represent a valid time keep the indexes for failsafe
>
> Is this sentence incomplete? What are those "indexes"?

the indexes that are created when core.splitIndex = true

the code that was modified looks for a configuration parameter (or the
default) that says something like "2 weeks ago" and that then converts
that into a timestamp that can be compared with the modification time
for the files that compose that index and then decides to delete
anyone that is older than the "expiration date".

since time_t is used for the stat.mtime there is a chance that the
timestamp might overflow its size, so if an overflow is detected we
assume the value to be equivalent to "never" and keep the files.

note this scenario will only matter around 2038 and it should be fixed
by then as part of the Year 2038 problem if we still care about 32-bit
UNIX by then, 32-bit Windows has until next century.

Carlo
Junio C Hamano Nov. 13, 2018, 9:43 a.m. UTC | #3
Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> There are still some more possible improvements around this code but
> they are orthogonal to this change :
>
> * migrate to approxidate_careful or parse_expiry_date
> * maybe make sure only approxidate are used for expiration
>
> Changes in v2:
> * improved commit message as suggested by Eric
> * failsafe against time_t truncation as suggested by Junio
>
> -- >8 --
> Subject: [PATCH v2 2/2] read-cache: use time specific types instead of
>  unsigned long
>
> b968372279 ("read-cache: unlink old sharedindex files", 2017-03-06)
> introduced get_shared_index_expire_date using unsigned long to track
> the modification times of a shared index.
>
> dddbad728c ("timestamp_t: a new data type for timestamps", 2017-04-26)
> shows why that might be problematic so move to timestamp_t/time_t.
>
> if time_t can't represent a valid time keep the indexes for failsafe

I am not sure if this "failsafe" is a good idea in the first place.
FWIW, I had a total opposite in mind when I suggested it.

The user gave you a timestamp_t value, telling you that anything
before that time is too old to matter and can be discarded.

And date_overflows() helper tells you that that the given expiry
date is more in the future than _any_ possible timestamp expressed
in time_t.  

So the result of running stat() on the shared index file _will_ have
a timestamp that is older than the specified expiry.

Which means that the user essentially is telling you that any shared
index that is not referenced can immediately be expired, similar to
saying --expire=now, no?

I kind of sort of would understand if we (1) read the configured
expiry, (2) checked it with date_overflows(), and (3) told the user
that it is an impossible condition, i.e. an error, to use such a
large timestamp far in the future.  Then the user has a chance to
update the configuration to a more reasonable time---even the most
aggressive one does not need to specify anything more in the future
than "now", and that will not "date_overflows()" for 20 years or so.

> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  read-cache.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 7b1354d759..7d322f11c8 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2625,9 +2625,9 @@ static int write_split_index(struct index_state *istate,
>  
>  static const char *shared_index_expire = "2.weeks.ago";
>  
> -static unsigned long get_shared_index_expire_date(void)
> +static timestamp_t get_shared_index_expire_date(void)
>  {
> -	static unsigned long shared_index_expire_date;
> +	static timestamp_t shared_index_expire_date;
>  	static int shared_index_expire_date_prepared;
>  
>  	if (!shared_index_expire_date_prepared) {
> @@ -2643,12 +2643,12 @@ static unsigned long get_shared_index_expire_date(void)
>  static int should_delete_shared_index(const char *shared_index_path)
>  {
>  	struct stat st;
> -	unsigned long expiration;
> +	time_t expiration;
> +	timestamp_t t = get_shared_index_expire_date();
>  
> -	/* Check timestamp */
> -	expiration = get_shared_index_expire_date();
> -	if (!expiration)
> +	if (!t || date_overflows(t))
>  		return 0;
> +	expiration = t;
>  	if (stat(shared_index_path, &st))
>  		return error_errno(_("could not stat '%s'"), shared_index_path);
>  	if (st.st_mtime > expiration)

Patch
diff mbox series

diff --git a/read-cache.c b/read-cache.c
index 7b1354d759..7d322f11c8 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2625,9 +2625,9 @@  static int write_split_index(struct index_state *istate,
 
 static const char *shared_index_expire = "2.weeks.ago";
 
-static unsigned long get_shared_index_expire_date(void)
+static timestamp_t get_shared_index_expire_date(void)
 {
-	static unsigned long shared_index_expire_date;
+	static timestamp_t shared_index_expire_date;
 	static int shared_index_expire_date_prepared;
 
 	if (!shared_index_expire_date_prepared) {
@@ -2643,12 +2643,12 @@  static unsigned long get_shared_index_expire_date(void)
 static int should_delete_shared_index(const char *shared_index_path)
 {
 	struct stat st;
-	unsigned long expiration;
+	time_t expiration;
+	timestamp_t t = get_shared_index_expire_date();
 
-	/* Check timestamp */
-	expiration = get_shared_index_expire_date();
-	if (!expiration)
+	if (!t || date_overflows(t))
 		return 0;
+	expiration = t;
 	if (stat(shared_index_path, &st))
 		return error_errno(_("could not stat '%s'"), shared_index_path);
 	if (st.st_mtime > expiration)