Re: [PATCH v5 0/8] Introduce timestamp_t for timestamps
- From
René Scharfe <l.s.r@web.de>
- Date
- Apr 25, 2017, 21:54 UTC
- Message-ID
- <7a2271ff-1386-18a6-5f6d-7eb13dc92509@web.de>
- In-Reply-To
- <cover.1493042239.git.johannes.schindelin@gmx.de>
Am 24.04.2017 um 15:57 schrieb Johannes Schindelin:
Show 19 quoted lines
> Git v2.9.2 was released in a hurry to accomodate for platforms like > Windows, where the `unsigned long` data type is 32-bit even for 64-bit > setups. > > The quick fix was to simply disable all the testing with "absurd" future > dates. > > However, we can do much better than that, as we already make use of > 64-bit data types internally. There is no good reason why we should not > use the same for timestamps. Hence, let's use uintmax_t for timestamps. > > Note: while the `time_t` data type exists and is meant to be used for > timestamps, on 32-bit Linux it is *still* 32-bit. An earlier iteration > used `time_t` for that reason, but it came with a few serious downsides: > as `time_t` can be signed (and indeed, on Windows it is an int64_t), > Git's expectation that 0 is the minimal value does no longer hold true, > introducing its own set of interesting challenges. Besides, if we *can* > handle far in the future timestamps (except for formatting them using > the system libraries), it is more consistent to do so.
time_t is signed on Linux and BSDs as well.
Using an unsigned type gives us the ability to represent times beyond the 292 billion years in the future that int64_t would give us, but prevents recording events that occurred before the Epoch. That doesn't sound like a good deal to me -- storing historical works (e.g. law texts) with real time stamps is probably more interesting than fixing the year 292277026596 problem within this decade.
> The upside of using `uintmax_t` for timestamps is that we do a much > better job to support far in the future timestamps across all platforms, > including 32-bit ones. The downside is that those platforms that use a > 32-bit `time_t` will barf when parsing or formatting those timestamps.
IIUC this series has two aims: solving the year 2038 problem on 32-bit Linux by replacing time_t (int32_t), and solving the year 2106 problem on Windows by replacing unsigned long (uint32_t), right? The latter one sounds more interesting, because 32-bit platforms would still be unable to fully use bigger time values as you wrote above.
Can we leave time_t alone and just do the part where you replace unsigned long with timestamp_t defined as uint64_t? That should already help on Windows, correct? When/if timestamp_t is later changed to a signed type then we could easily convert the time_t cases to timestamp_t as well, or the other way around.
Show 7 quoted lines
> This iteration makes the date_overflows() check more stringent again. > > It is arguably a bug to paper over too-large author/committer dates and > to replace them with Jan 1 1970 without even telling the user that we do > that, but this is the behavior that t4212 verifies, so I reinstated that > behavior. The change in behavior was missed because of the missing > unsigned_add_overflows() test.
I can't think of many ways to get future time stamps (broken clock, broken CMOS battery, bit rot, time travel), so I wouldn't expect a change towards better error reporting to affect a lot of users. (Not necessarily as part of this series, of course.)
Show 77 quoted lines
> Changes since v4: > > - in gm_time_t(), we now test specifically that the timezone adjustment > neither underflows nor overflows. > > - the patch introduced in v4 that tried to defer the date_overflows() > check to gm_time_t() rather than replacing the ident timestamp by a 0 > without any warning was dropped again: it broke t4212. > > > Johannes Schindelin (8): > ref-filter: avoid using `unsigned long` for catch-all data type > t0006 & t5000: prepare for 64-bit timestamps > t0006 & t5000: skip "far in the future" test when time_t is too > limited > Specify explicitly where we parse timestamps > Introduce a new "printf format" for timestamps > Introduce a new data type for timestamps > Abort if the system time cannot handle one of our timestamps > Use uintmax_t for timestamps > > Documentation/technical/api-parse-options.txt | 8 +- > archive-tar.c | 5 +- > archive-zip.c | 12 ++- > archive.h | 2 +- > builtin/am.c | 4 +- > builtin/blame.c | 14 ++-- > builtin/fsck.c | 6 +- > builtin/gc.c | 2 +- > builtin/log.c | 4 +- > builtin/merge-base.c | 2 +- > builtin/name-rev.c | 6 +- > builtin/pack-objects.c | 4 +- > builtin/prune.c | 4 +- > builtin/receive-pack.c | 14 ++-- > builtin/reflog.c | 24 +++--- > builtin/rev-list.c | 2 +- > builtin/rev-parse.c | 2 +- > builtin/show-branch.c | 4 +- > builtin/worktree.c | 4 +- > bundle.c | 4 +- > cache.h | 14 ++-- > commit.c | 18 ++-- > commit.h | 2 +- > config.c | 2 +- > credential-cache--daemon.c | 12 +-- > date.c | 113 ++++++++++++++------------ > fetch-pack.c | 8 +- > fsck.c | 2 +- > git-compat-util.h | 9 ++ > http-backend.c | 4 +- > parse-options-cb.c | 4 +- > pretty.c | 4 +- > reachable.c | 9 +- > reachable.h | 4 +- > ref-filter.c | 22 ++--- > reflog-walk.c | 8 +- > refs.c | 14 ++-- > refs.h | 8 +- > refs/files-backend.c | 8 +- > revision.c | 6 +- > revision.h | 4 +- > sha1_name.c | 6 +- > t/helper/test-date.c | 18 ++-- > t/helper/test-parse-options.c | 4 +- > t/helper/test-ref-store.c | 4 +- > t/t0006-date.sh | 4 +- > t/t5000-tar-tree.sh | 6 +- > t/test-lib.sh | 3 + > tag.c | 6 +- > tag.h | 2 +- > upload-pack.c | 8 +- > vcs-svn/fast_export.c | 8 +- > vcs-svn/fast_export.h | 4 +- > vcs-svn/svndump.c | 2 +- > wt-status.c | 2 +- > 55 files changed, 260 insertions(+), 219 deletions(-)
How did you find all the pieces of code that need to be touched? Is there a regex or something that can be used to spot new such places that sneak in, e.g. through in-flight merges?
Show 53 quoted lines
> base-commit: e2cb6ab84c94f147f1259260961513b40c36108a
> Published-As: https://github.com/dscho/git/releases/tag/time_t-may-be-int64-v5
> Fetch-It-Via: git fetch https://github.com/dscho/git time_t-may-be-int64-v5
>
> Interdiff vs v4:
>
> diff --git a/date.c b/date.c
> index 75f6335cd09..63fa99685e2 100644
> --- a/date.c
> +++ b/date.c
> @@ -47,9 +47,16 @@ static time_t gm_time_t(timestamp_t time, int tz)
> minutes = (minutes / 100)*60 + (minutes % 100);
> minutes = tz < 0 ? -minutes : minutes;
>
> - if (date_overflows(time + minutes * 60))
> + if (minutes > 0) {
> + if (unsigned_add_overflows(time, minutes * 60))
> + die("Timestamp+tz too large: %"PRItime" +%04d",
> + time, tz);
> + } else if (time < -minutes * 60)
> + die("Timestamp before Unix epoch: %"PRItime" %04d", time, tz);
> + time += minutes * 60;
> + if (date_overflows(time))
> die("Timestamp too large for this system: %"PRItime, time);
> - return (time_t)time + minutes * 60;
> + return (time_t)time;
> }
>
> /*
> diff --git a/pretty.c b/pretty.c
> index 35fd290096a..587d48371b0 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -410,10 +410,14 @@ const char *show_ident_date(const struct ident_split *ident,
>
> if (ident->date_begin && ident->date_end)
> date = parse_timestamp(ident->date_begin, NULL, 10);
> - if (ident->tz_begin && ident->tz_end)
> - tz = strtol(ident->tz_begin, NULL, 10);
> - if (tz >= INT_MAX || tz <= INT_MIN)
> - tz = 0;
> + if (date_overflows(date))
> + date = 0;
> + else {
> + if (ident->tz_begin && ident->tz_end)
> + tz = strtol(ident->tz_begin, NULL, 10);
> + if (tz >= INT_MAX || tz <= INT_MIN)
> + tz = 0;
> + }
> return show_date(date, tz, mode);
> }
>
>