| Message ID | 20181112084031.11769-3-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | avoid unsigned long for timestamps | expand |
On Mon, Nov 12, 2018 at 3:41 AM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote: > 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 problematic so move to time_t instead. s/might/& be/ > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > 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 problematic so move to time_t instead. > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > read-cache.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/read-cache.c b/read-cache.c > index 7b1354d759..5525d8e679 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 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? > @@ -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();
Hi, On Mon, 12 Nov 2018, Junio C Hamano wrote: > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > > > 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 problematic so move to time_t instead. > > > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > > --- > > read-cache.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/read-cache.c b/read-cache.c > > index 7b1354d759..5525d8e679 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 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? Riiiight. From the patch, I had assumed that the return type of `approxidate()` is `time_t`, but it is `timestamp_t`. Ciao, Johannes > > > > @@ -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(); >
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> 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? > > Riiiight. From the patch, I had assumed that the return type of > `approxidate()` is `time_t`, but it is `timestamp_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.
diff --git a/read-cache.c b/read-cache.c index 7b1354d759..5525d8e679 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 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();
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 problematic so move to time_t instead. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- read-cache.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)