{"thread":{"id":"49807","subject":"[PATCH 0/2] avoid unsigned long for timestamps","startedAt":"2018-11-12T08:41:00Z","lastAt":"2018-11-13T09:44:01Z","messageCount":12,"participants":["Carlo Marcelo Arenas Belón","Eric Sunshine","Junio C Hamano","Johannes Schindelin","Carlo Arenas"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"362971","messageId":"20181112084031.11769-1-carenas@gmail.com","threadId":"49807","inReplyTo":null,"subject":"[PATCH 0/2] avoid unsigned long for timestamps","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2018-11-12T08:40:29Z","receivedAt":"2018-11-12T08:41:00Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"specially problematic in Windows where unsigned long is only 32bit wide\nand therefore the assumption that a time_t would fit will lead to loss\nof precision in a 64bit OS.\n\n builtin/commit.c | 4 ++--\n read-cache.c | 6 +++---\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\n\n"},{"id":"362972","messageId":"20181112084031.11769-2-carenas@gmail.com","threadId":"49807","inReplyTo":"20181112084031.11769-1-carenas@gmail.com","subject":"[PATCH 1/2] builtin/commit: use timestamp_t in parse_force_date","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2018-11-12T08:40:30Z","receivedAt":"2018-11-12T08:41:02Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"when dddbad728c (\"timestamp_t: a new data type for timestamps\", 2017-04-26)\nwas introduced, the fallback to use approxidate that was introduced in\n14ac2864dc (\"commit: accept more date formats for \"--date\"\", 2014-05-01)\nwas not updated to use the new type instead of unsigned long.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n builtin/commit.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 0d9828e29e..a447e08f62 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -537,10 +537,10 @@ static int parse_force_date(const char *in, struct strbuf *out)\n \n \tif (parse_date(in, out) < 0) {\n \t\tint errors = 0;\n-\t\tunsigned long t = approxidate_careful(in, &errors);\n+\t\ttimestamp_t t = approxidate_careful(in, &errors);\n \t\tif (errors)\n \t\t\treturn -1;\n-\t\tstrbuf_addf(out, \"%lu\", t);\n+\t\tstrbuf_addf(out, \"%\"PRItime, t);\n \t}\n \n \treturn 0;\n-- \n2.19.1.856.g8858448bb\n\n"},{"id":"362973","messageId":"20181112084031.11769-3-carenas@gmail.com","threadId":"49807","inReplyTo":"20181112084031.11769-1-carenas@gmail.com","subject":"[PATCH 2/2] read-cache: use time_t instead of unsigned long","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2018-11-12T08:40:31Z","receivedAt":"2018-11-12T08:41:02Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"b968372279 (\"read-cache: unlink old sharedindex files\", 2017-03-06)\nintroduced get_shared_index_expire_date using unsigned long to track\nthe modification times of a shared index.\n\ndddbad728c (\"timestamp_t: a new data type for timestamps\", 2017-04-26)\nshows why that might problematic so move to time_t instead.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n read-cache.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 7b1354d759..5525d8e679 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2625,9 +2625,9 @@ static int write_split_index(struct index_state *istate,\n \n static const char *shared_index_expire = \"2.weeks.ago\";\n \n-static unsigned long get_shared_index_expire_date(void)\n+static time_t get_shared_index_expire_date(void)\n {\n-\tstatic unsigned long shared_index_expire_date;\n+\tstatic time_t shared_index_expire_date;\n \tstatic int shared_index_expire_date_prepared;\n \n \tif (!shared_index_expire_date_prepared) {\n@@ -2643,7 +2643,7 @@ static unsigned long get_shared_index_expire_date(void)\n static int should_delete_shared_index(const char *shared_index_path)\n {\n \tstruct stat st;\n-\tunsigned long expiration;\n+\ttime_t expiration;\n \n \t/* Check timestamp */\n \texpiration = get_shared_index_expire_date();\n-- \n2.19.1.856.g8858448bb\n\n"},{"id":"362974","messageId":"CAPig+cRksYnDxTO9mbH-ieW=M24CuU+J42JxTfC8z8fdWZrJ6Q@mail.gmail.com","threadId":"49807","inReplyTo":"20181112084031.11769-3-carenas@gmail.com","subject":"Re: [PATCH 2/2] read-cache: use time_t instead of unsigned long","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-11-12T08:42:37Z","receivedAt":"2018-11-12T08:42:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 12, 2018 at 3:41 AM Carlo Marcelo Arenas Belón\n wrote:\n> b968372279 (\"read-cache: unlink old sharedindex files\", 2017-03-06)\n> introduced get_shared_index_expire_date using unsigned long to track\n> the modification times of a shared index.\n>\n> dddbad728c (\"timestamp_t: a new data type for timestamps\", 2017-04-26)\n> shows why that might problematic so move to time_t instead.\n\ns/might/& be/\n\n> Signed-off-by: Carlo Marcelo Arenas Belón \n"},{"id":"362986","messageId":"xmqqftw61sa0.fsf@gitster-ct.c.googlers.com","threadId":"49807","inReplyTo":"20181112084031.11769-3-carenas@gmail.com","subject":"Re: [PATCH 2/2] read-cache: use time_t instead of unsigned long","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-12T10:53:11Z","receivedAt":"2018-11-12T10:53:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> b968372279 (\"read-cache: unlink old sharedindex files\", 2017-03-06)\n> introduced get_shared_index_expire_date using unsigned long to track\n> the modification times of a shared index.\n>\n> dddbad728c (\"timestamp_t: a new data type for timestamps\", 2017-04-26)\n> shows why that might problematic so move to time_t instead.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> read-cache.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index 7b1354d759..5525d8e679 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -2625,9 +2625,9 @@ static int write_split_index(struct index_state *istate,\n> \n> static const char *shared_index_expire = \"2.weeks.ago\";\n> \n> -static unsigned long get_shared_index_expire_date(void)\n> +static time_t get_shared_index_expire_date(void)\n> {\n> -\tstatic unsigned long shared_index_expire_date;\n> +\tstatic time_t shared_index_expire_date;\n> \tstatic int shared_index_expire_date_prepared;\n> \n> \tif (!shared_index_expire_date_prepared) {\n\nAfter this line, the post-context reads like this:\n\n\t\tgit_config_get_expiry(\"splitindex.sharedindexexpire\",\n\t\t\t\t &shared_index_expire);\n\t\tshared_index_expire_date = approxidate(shared_index_expire);\n\t\tshared_index_expire_date_prepared = 1;\n\t}\n\n\treturn shared_index_expire_date;\n\nGiven that the function returns the value obtained from\napproxidate(), which is approxidate_careful() in disguise, time_t is\nnot as appropriate as timestamp_t, no?\n\nIOW, what if time_t were narrower than timestamp_t?\n\n\n> @@ -2643,7 +2643,7 @@ static unsigned long get_shared_index_expire_date(void)\n> static int should_delete_shared_index(const char *shared_index_path)\n> {\n> \tstruct stat st;\n> -\tunsigned long expiration;\n> +\ttime_t expiration;\n> \n> \t/* Check timestamp */\n> \texpiration = get_shared_index_expire_date();\n"},{"id":"362995","messageId":"nycvar.QRO.7.76.6.1811121306050.39@tvgsbejvaqbjf.bet","threadId":"49807","inReplyTo":"20181112084031.11769-1-carenas@gmail.com","subject":"Re: [PATCH 0/2] avoid unsigned long for timestamps","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-12T12:06:30Z","receivedAt":"2018-11-12T12:06:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Mon, 12 Nov 2018, Carlo Marcelo Arenas Belón wrote:\n\n> specially problematic in Windows where unsigned long is only 32bit wide\n> and therefore the assumption that a time_t would fit will lead to loss\n> of precision in a 64bit OS.\n\nBoth patches look correct to me.\n\nThanks!\nDscho\n\n> \n> builtin/commit.c | 4 ++--\n> read-cache.c | 6 +++---\n> 2 files changed, 5 insertions(+), 5 deletions(-)\n> \n> \n> "},{"id":"363037","messageId":"nycvar.QRO.7.76.6.1811121554060.39@tvgsbejvaqbjf.bet","threadId":"49807","inReplyTo":"xmqqftw61sa0.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] read-cache: use time_t instead of unsigned long","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-12T14:54:41Z","receivedAt":"2018-11-12T14:54:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 12 Nov 2018, Junio C Hamano wrote:\n\n> Carlo Marcelo Arenas Belón writes:\n> \n> > b968372279 (\"read-cache: unlink old sharedindex files\", 2017-03-06)\n> > introduced get_shared_index_expire_date using unsigned long to track\n> > the modification times of a shared index.\n> >\n> > dddbad728c (\"timestamp_t: a new data type for timestamps\", 2017-04-26)\n> > shows why that might problematic so move to time_t instead.\n> >\n> > Signed-off-by: Carlo Marcelo Arenas Belón \n> > ---\n> > read-cache.c | 6 +++---\n> > 1 file changed, 3 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/read-cache.c b/read-cache.c\n> > index 7b1354d759..5525d8e679 100644\n> > --- a/read-cache.c\n> > +++ b/read-cache.c\n> > @@ -2625,9 +2625,9 @@ static int write_split_index(struct index_state *istate,\n> > \n> > static const char *shared_index_expire = \"2.weeks.ago\";\n> > \n> > -static unsigned long get_shared_index_expire_date(void)\n> > +static time_t get_shared_index_expire_date(void)\n> > {\n> > -\tstatic unsigned long shared_index_expire_date;\n> > +\tstatic time_t shared_index_expire_date;\n> > \tstatic int shared_index_expire_date_prepared;\n> > \n> > \tif (!shared_index_expire_date_prepared) {\n> \n> After this line, the post-context reads like this:\n> \n> \t\tgit_config_get_expiry(\"splitindex.sharedindexexpire\",\n> \t\t\t\t &shared_index_expire);\n> \t\tshared_index_expire_date = approxidate(shared_index_expire);\n> \t\tshared_index_expire_date_prepared = 1;\n> \t}\n> \n> \treturn shared_index_expire_date;\n> \n> Given that the function returns the value obtained from\n> approxidate(), which is approxidate_careful() in disguise, time_t is\n> not as appropriate as timestamp_t, no?\n> \n> IOW, what if time_t were narrower than timestamp_t?\n\nRiiiight. From the patch, I had assumed that the return type of\n`approxidate()` is `time_t`, but it is `timestamp_t`.\n\nCiao,\nJohannes\n\n> \n> \n> > @@ -2643,7 +2643,7 @@ static unsigned long get_shared_index_expire_date(void)\n> > static int should_delete_shared_index(const char *shared_index_path)\n> > {\n> > \tstruct stat st;\n> > -\tunsigned long expiration;\n> > +\ttime_t expiration;\n> > \n> > \t/* Check timestamp */\n> > \texpiration = get_shared_index_expire_date();\n> "},{"id":"363039","messageId":"xmqq7ehi1gpb.fsf@gitster-ct.c.googlers.com","threadId":"49807","inReplyTo":"nycvar.QRO.7.76.6.1811121554060.39@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] read-cache: use time_t instead of unsigned long","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-12T15:03:12Z","receivedAt":"2018-11-12T15:03:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Given that the function returns the value obtained from\n>> approxidate(), which is approxidate_careful() in disguise, time_t is\n>> not as appropriate as timestamp_t, no?\n>> \n>> IOW, what if time_t were narrower than timestamp_t?\n>\n> Riiiight. From the patch, I had assumed that the return type of\n> `approxidate()` is `time_t`, but it is `timestamp_t`.\n\nYes, but if we dig a bit deeper, it turns out that the return value\nof this function is used at only one place, to be compared with the\n.st_mtime field.\n\nSo for this change to truly be consisent, not just the function\nneeds to return timestamp_t, but also its sole caller needs to check\nif its return value exceeds the maximum span that is expressible\nwith the platform's time_t (and if so, treat the expiration to be\n\"infinity- never expire\"), or something like that.\n"},{"id":"363133","messageId":"20181113074017.17292-1-carenas@gmail.com","threadId":"49807","inReplyTo":"xmqq7ehi1gpb.fsf@gitster-ct.c.googlers.com","subject":"[PATCH 2/2] read-cache: use time_t instead of unsigned long","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2018-11-13T07:40:17Z","receivedAt":"2018-11-13T07:40:23Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"There are still some more possible improvements around this code but\nthey are orthogonal to this change :\n\n* migrate to approxidate_careful or parse_expiry_date\n* maybe make sure only approxidate are used for expiration\n\nChanges in v2:\n* improved commit message as suggested by Eric\n* failsafe against time_t truncation as suggested by Junio\n\n-- >8 --\nSubject: [PATCH v2 2/2] read-cache: use time specific types instead of\n unsigned long\n\nb968372279 (\"read-cache: unlink old sharedindex files\", 2017-03-06)\nintroduced get_shared_index_expire_date using unsigned long to track\nthe modification times of a shared index.\n\ndddbad728c (\"timestamp_t: a new data type for timestamps\", 2017-04-26)\nshows why that might be problematic so move to timestamp_t/time_t.\n\nif time_t can't represent a valid time keep the indexes for failsafe\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n read-cache.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 7b1354d759..7d322f11c8 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2625,9 +2625,9 @@ static int write_split_index(struct index_state *istate,\n \n static const char *shared_index_expire = \"2.weeks.ago\";\n \n-static unsigned long get_shared_index_expire_date(void)\n+static timestamp_t get_shared_index_expire_date(void)\n {\n-\tstatic unsigned long shared_index_expire_date;\n+\tstatic timestamp_t shared_index_expire_date;\n \tstatic int shared_index_expire_date_prepared;\n \n \tif (!shared_index_expire_date_prepared) {\n@@ -2643,12 +2643,12 @@ static unsigned long get_shared_index_expire_date(void)\n static int should_delete_shared_index(const char *shared_index_path)\n {\n \tstruct stat st;\n-\tunsigned long expiration;\n+\ttime_t expiration;\n+\ttimestamp_t t = get_shared_index_expire_date();\n \n-\t/* Check timestamp */\n-\texpiration = get_shared_index_expire_date();\n-\tif (!expiration)\n+\tif (!t || date_overflows(t))\n \t\treturn 0;\n+\texpiration = t;\n \tif (stat(shared_index_path, &st))\n \t\treturn error_errno(_(\"could not stat '%s'\"), shared_index_path);\n \tif (st.st_mtime > expiration)\n-- \n2.19.1.856.g8858448bb\n\n"},{"id":"363134","messageId":"nycvar.QRO.7.76.6.1811130948420.39@tvgsbejvaqbjf.bet","threadId":"49807","inReplyTo":"20181113074017.17292-1-carenas@gmail.com","subject":"Re: [PATCH 2/2] read-cache: use time_t instead of unsigned long","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-13T08:49:50Z","receivedAt":"2018-11-13T08:50:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 12 Nov 2018, Carlo Marcelo Arenas Belón wrote:\n\n> There are still some more possible improvements around this code but\n> they are orthogonal to this change :\n> \n> * migrate to approxidate_careful or parse_expiry_date\n> * maybe make sure only approxidate are used for expiration\n> \n> Changes in v2:\n> * improved commit message as suggested by Eric\n> * failsafe against time_t truncation as suggested by Junio\n> \n> -- >8 --\n> Subject: [PATCH v2 2/2] read-cache: use time specific types instead of\n> unsigned long\n> \n> b968372279 (\"read-cache: unlink old sharedindex files\", 2017-03-06)\n> introduced get_shared_index_expire_date using unsigned long to track\n> the modification times of a shared index.\n> \n> dddbad728c (\"timestamp_t: a new data type for timestamps\", 2017-04-26)\n> shows why that might be problematic so move to timestamp_t/time_t.\n> \n> if time_t can't represent a valid time keep the indexes for failsafe\n\nIs this sentence incomplete? What are those \"indexes\"?\n\nThanks,\nJohannes\n\n> \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> read-cache.c | 12 ++++++------\n> 1 file changed, 6 insertions(+), 6 deletions(-)\n> \n> diff --git a/read-cache.c b/read-cache.c\n> index 7b1354d759..7d322f11c8 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -2625,9 +2625,9 @@ static int write_split_index(struct index_state *istate,\n> \n> static const char *shared_index_expire = \"2.weeks.ago\";\n> \n> -static unsigned long get_shared_index_expire_date(void)\n> +static timestamp_t get_shared_index_expire_date(void)\n> {\n> -\tstatic unsigned long shared_index_expire_date;\n> +\tstatic timestamp_t shared_index_expire_date;\n> \tstatic int shared_index_expire_date_prepared;\n> \n> \tif (!shared_index_expire_date_prepared) {\n> @@ -2643,12 +2643,12 @@ static unsigned long get_shared_index_expire_date(void)\n> static int should_delete_shared_index(const char *shared_index_path)\n> {\n> \tstruct stat st;\n> -\tunsigned long expiration;\n> +\ttime_t expiration;\n> +\ttimestamp_t t = get_shared_index_expire_date();\n> \n> -\t/* Check timestamp */\n> -\texpiration = get_shared_index_expire_date();\n> -\tif (!expiration)\n> +\tif (!t || date_overflows(t))\n> \t\treturn 0;\n> +\texpiration = t;\n> \tif (stat(shared_index_path, &st))\n> \t\treturn error_errno(_(\"could not stat '%s'\"), shared_index_path);\n> \tif (st.st_mtime > expiration)\n> -- \n> 2.19.1.856.g8858448bb\n> \n> "},{"id":"363135","messageId":"CAPUEspgqUZ9eKAGug=SGRedR=KBQswpsTZU+0PYHdGsCA4iZLA@mail.gmail.com","threadId":"49807","inReplyTo":"nycvar.QRO.7.76.6.1811130948420.39@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] read-cache: use time_t instead of unsigned long","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2018-11-13T09:10:24Z","receivedAt":"2018-11-13T09:10:38Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Nov 13, 2018 at 12:49 AM Johannes Schindelin\n wrote:\n> On Mon, 12 Nov 2018, Carlo Marcelo Arenas Belón wrote:\n>>\n> > if time_t can't represent a valid time keep the indexes for failsafe\n>\n> Is this sentence incomplete? What are those \"indexes\"?\n\nthe indexes that are created when core.splitIndex = true\n\nthe code that was modified looks for a configuration parameter (or the\ndefault) that says something like \"2 weeks ago\" and that then converts\nthat into a timestamp that can be compared with the modification time\nfor the files that compose that index and then decides to delete\nanyone that is older than the \"expiration date\".\n\nsince time_t is used for the stat.mtime there is a chance that the\ntimestamp might overflow its size, so if an overflow is detected we\nassume the value to be equivalent to \"never\" and keep the files.\n\nnote this scenario will only matter around 2038 and it should be fixed\nby then as part of the Year 2038 problem if we still care about 32-bit\nUNIX by then, 32-bit Windows has until next century.\n\nCarlo\n"},{"id":"363138","messageId":"xmqq36s5wbvn.fsf@gitster-ct.c.googlers.com","threadId":"49807","inReplyTo":"20181113074017.17292-1-carenas@gmail.com","subject":"Re: [PATCH 2/2] read-cache: use time_t instead of unsigned long","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-13T09:43:56Z","receivedAt":"2018-11-13T09:44:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> There are still some more possible improvements around this code but\n> they are orthogonal to this change :\n>\n> * migrate to approxidate_careful or parse_expiry_date\n> * maybe make sure only approxidate are used for expiration\n>\n> Changes in v2:\n> * improved commit message as suggested by Eric\n> * failsafe against time_t truncation as suggested by Junio\n>\n> -- >8 --\n> Subject: [PATCH v2 2/2] read-cache: use time specific types instead of\n> unsigned long\n>\n> b968372279 (\"read-cache: unlink old sharedindex files\", 2017-03-06)\n> introduced get_shared_index_expire_date using unsigned long to track\n> the modification times of a shared index.\n>\n> dddbad728c (\"timestamp_t: a new data type for timestamps\", 2017-04-26)\n> shows why that might be problematic so move to timestamp_t/time_t.\n>\n> if time_t can't represent a valid time keep the indexes for failsafe\n\nI am not sure if this \"failsafe\" is a good idea in the first place.\nFWIW, I had a total opposite in mind when I suggested it.\n\nThe user gave you a timestamp_t value, telling you that anything\nbefore that time is too old to matter and can be discarded.\n\nAnd date_overflows() helper tells you that that the given expiry\ndate is more in the future than _any_ possible timestamp expressed\nin time_t. \n\nSo the result of running stat() on the shared index file _will_ have\na timestamp that is older than the specified expiry.\n\nWhich means that the user essentially is telling you that any shared\nindex that is not referenced can immediately be expired, similar to\nsaying --expire=now, no?\n\nI kind of sort of would understand if we (1) read the configured\nexpiry, (2) checked it with date_overflows(), and (3) told the user\nthat it is an impossible condition, i.e. an error, to use such a\nlarge timestamp far in the future. Then the user has a chance to\nupdate the configuration to a more reasonable time---even the most\naggressive one does not need to specify anything more in the future\nthan \"now\", and that will not \"date_overflows()\" for 20 years or so.\n\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> read-cache.c | 12 ++++++------\n> 1 file changed, 6 insertions(+), 6 deletions(-)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index 7b1354d759..7d322f11c8 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -2625,9 +2625,9 @@ static int write_split_index(struct index_state *istate,\n> \n> static const char *shared_index_expire = \"2.weeks.ago\";\n> \n> -static unsigned long get_shared_index_expire_date(void)\n> +static timestamp_t get_shared_index_expire_date(void)\n> {\n> -\tstatic unsigned long shared_index_expire_date;\n> +\tstatic timestamp_t shared_index_expire_date;\n> \tstatic int shared_index_expire_date_prepared;\n> \n> \tif (!shared_index_expire_date_prepared) {\n> @@ -2643,12 +2643,12 @@ static unsigned long get_shared_index_expire_date(void)\n> static int should_delete_shared_index(const char *shared_index_path)\n> {\n> \tstruct stat st;\n> -\tunsigned long expiration;\n> +\ttime_t expiration;\n> +\ttimestamp_t t = get_shared_index_expire_date();\n> \n> -\t/* Check timestamp */\n> -\texpiration = get_shared_index_expire_date();\n> -\tif (!expiration)\n> +\tif (!t || date_overflows(t))\n> \t\treturn 0;\n> +\texpiration = t;\n> \tif (stat(shared_index_path, &st))\n> \t\treturn error_errno(_(\"could not stat '%s'\"), shared_index_path);\n> \tif (st.st_mtime > expiration)\n"}]}