avoid unsigned long for timestamps
cover avoid unsigned long for timestamps
specially problematic in Windows where unsigned long is only 32bit wide and therefore the assumption that a time_t would fit will lead to loss of precision in a 64bit OS.
builtin/commit.c | 4 ++-- read-cache.c | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-)
PATCH 1/2 builtin/commit: use timestamp_t in parse_force_date
when dddbad728c ("timestamp_t: a new data type for timestamps", 2017-04-26) was introduced, the fallback to use approxidate that was introduced in 14ac2864dc ("commit: accept more date formats for "--date"", 2014-05-01) was not updated to use the new type instead of unsigned long.
--- builtin/commit.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
builtin/commit.c +2 -2
@@ -537,10 +537,10 @@ static int parse_force_date(const char *in, struct strbuf *out)if (parse_date (in, out) <0 ) {int errors =0 ; -unsigned long t =approxidate_careful (in, &errors); +timestamp_t t =approxidate_careful (in, &errors);if (errors)return -1 ; -strbuf_addf (out,"%lu" , t); +strbuf_addf (out,"%" PRItime, t); }return 0 ;
Signature · 1 line
2.19.1.856.g8858448bb
PATCH 2/2 read-cache: use time_t instead of unsigned long
In reply to Carlo Marcelo Arenas Belónb968372279 ("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 problematic so move to time_t instead.
--- read-cache.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
read-cache.c +3 -3
@@ -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 time_t get_shared_index_expire_date (void ) { -static unsigned long shared_index_expire_date; +static time_t shared_index_expire_date;static int shared_index_expire_date_prepared;if (!shared_index_expire_date_prepared) {
@@ -2643,7 +2643,7 @@ 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;/* Check timestamp */ expiration =get_shared_index_expire_date ();
Signature · 1 line
2.19.1.856.g8858448bb
Re: [PATCH 2/2] read-cache: use time_t instead of unsigned long
Quoted 6 lines · Carlo Marcelo Arenas Belón
b968372279("read-cache: unlink old sharedindex files", 2017-03-06) introducedget_shared_index_expire_dateusing 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 problematic so move totime_tinstead.
s/might/& be/
Quoted 1 line
Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>
Re: [PATCH 2/2] read-cache: use time_t instead of unsigned long
In reply to Carlo Marcelo Arenas BelónQuoted 18 lines · Carlo Marcelo Arenas Belón
b968372279("read-cache: unlink old sharedindex files", 2017-03-06) introducedget_shared_index_expire_dateusing 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 problematic so move totime_tinstead.Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>--- read-cache.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)read-cache.c +0
-0@@ -2625,9 +2625,9 @@ static int write_split_index(struct index_state *istate,
read-cache.c static int write_split_index(struct index_state *istate, · line 2633
Quoted 10 lines
read-cache.c +2
-2static const char *shared_index_expire ="2.weeks.ago" ; -static unsigned long get_shared_index_expire_date (void ) +static time_t get_shared_index_expire_date (void ) { -static unsigned long shared_index_expire_date; +static time_t shared_index_expire_date;static int shared_index_expire_date_prepared;if (!shared_index_expire_date_prepared) {
After this line, the post-context reads like this:
git_config_get_expiry("splitindex.sharedindexexpire",
&shared_index_expire);
shared_index_expire_date = approxidate(shared_index_expire);
shared_index_expire_date_prepared = 1;
}
return shared_index_expire_date;Given that the function returns the value obtained from approxidate(), which is approxidate_careful() in disguise, time_t is not as appropriate as timestamp_t, no?
IOW, what if time_t were narrower than timestamp_t?
Quoted 9 lines
read-cache.c +1
-1@@ -2643,7 +2643,7 @@ 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;/* Check timestamp */ expiration =get_shared_index_expire_date ();
Re: [PATCH 2/2] read-cache: use time_t instead of unsigned long
Hi,
Quoted 41 lines · Junio C Hamano
Quoted 18 lines · Carlo Marcelo Arenas Belón
b968372279("read-cache: unlink old sharedindex files", 2017-03-06) introducedget_shared_index_expire_dateusing 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 problematic so move totime_tinstead.Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>--- read-cache.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)read-cache.c +0
-0@@ -2625,9 +2625,9 @@ static int write_split_index(struct index_state *istate,read-cache.c static int write_split_index(struct index_state *istate, · line 2633
Quoted 10 lines
read-cache.c +2
-2static const char *shared_index_expire ="2.weeks.ago" ; -static unsigned long get_shared_index_expire_date (void ) +static time_t get_shared_index_expire_date (void ) { -static unsigned long shared_index_expire_date; +static time_t shared_index_expire_date;static int shared_index_expire_date_prepared;if (!shared_index_expire_date_prepared) {After this line, the post-context reads like this:
git_config_get_expiry("splitindex.sharedindexexpire", &shared_index_expire); shared_index_expire_date = approxidate(shared_index_expire); shared_index_expire_date_prepared = 1; } return shared_index_expire_date;
Quoted 5 lines
Given that the function returns the value obtained from
approxidate(), which isapproxidate_careful()in disguise,time_tis not as appropriate astimestamp_t, no?IOW, what if
time_twere narrower thantimestamp_t?
Riiiight. From the patch, I had assumed that the return type of approxidate() is time_t, but it is timestamp_t.
Ciao,
Johannes
Quoted 12 lines
Quoted 11 lines
read-cache.c +1
-1@@ -2643,7 +2643,7 @@ 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;/* Check timestamp */ expiration =get_shared_index_expire_date ();
Re: [PATCH 2/2] read-cache: use time_t instead of unsigned long
Quoted 8 lines · Johannes Schindelin
Quoted 5 lines
Given that the function returns the value obtained from
approxidate(), which isapproxidate_careful()in disguise,time_tis not as appropriate astimestamp_t, no?IOW, what if
time_twere narrower thantimestamp_t?Riiiight. From the patch, I had assumed that the return type of
approxidate()istime_t, but it istimestamp_t.
Yes, but if we dig a bit deeper, it turns out that the return value of this function is used at only one place, to be compared with the .st_mtime field.
So for this change to truly be consisent, not just the function needs to return timestamp_t, but also its sole caller needs to check if its return value exceeds the maximum span that is expressible with the platform's time_t (and if so, treat the expiration to be "infinity- never expire"), or something like that.
PATCH 2/2 read-cache: use time_t instead of unsigned long
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
--- read-cache.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
read-cache.c +6 -6
@@ -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)
Signature · 1 line
2.19.1.856.g8858448bb
Re: [PATCH 2/2] read-cache: use time_t instead of unsigned long
Hi,
Quoted 14 lines · Carlo Marcelo Arenas Belón
There are still some more possible improvements around this code but they are orthogonal to this change :
* migrate to
approxidate_carefulorparse_expiry_date
* maybe make sure only approxidate are used for expirationChanges in v2:
* improved commit message as suggested by Eric
* failsafe againsttime_ttruncation as suggested by Junio-- >8 --
Subject: [PATCH v2 2/2] read-cache: use time specific types instead of
unsigned long
Quoted 8 lines
b968372279("read-cache: unlink old sharedindex files", 2017-03-06) introducedget_shared_index_expire_dateusing 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_tcan't represent a valid time keep the indexes for failsafe
Is this sentence incomplete? What are those "indexes"?
Thanks,
Johannes
Quoted 43 lines
Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>--- read-cache.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)read-cache.c +6
-6@@ -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)Signature · 1 line
2.19.1.856.g8858448bb
Re: [PATCH 2/2] read-cache: use time_t instead of unsigned long
Quoted 5 lines · Johannes Schindelin
Quoted 2 lines · Carlo Marcelo Arenas Belón
if
time_tcan't represent a valid time keep the indexes for failsafeIs 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
Re: [PATCH 2/2] read-cache: use time_t instead of unsigned long
In reply to Carlo Marcelo Arenas BelónQuoted 14 lines · Carlo Marcelo Arenas Belón
There are still some more possible improvements around this code but they are orthogonal to this change :
* migrate to
approxidate_carefulorparse_expiry_date
* maybe make sure only approxidate are used for expirationChanges in v2:
* improved commit message as suggested by Eric
* failsafe againsttime_ttruncation as suggested by Junio-- >8 --
Subject: [PATCH v2 2/2] read-cache: use time specific types instead of
unsigned long
Quoted 8 lines
b968372279("read-cache: unlink old sharedindex files", 2017-03-06) introducedget_shared_index_expire_dateusing 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_tcan'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.
Quoted 38 lines
Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>--- read-cache.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)read-cache.c +6
-6@@ -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)
Hi Carlo,
Quoted 3 lines · Carlo Marcelo Arenas Belón
specially problematic in Windows where unsigned long is only 32bit wide and therefore the assumption that a
time_twould fit will lead to loss of precision in a 64bit OS.
Both patches look correct to me.
Thanks!
Dscho
Quoted 7 lines
builtin/commit.c | 4 ++-- read-cache.c | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-)