* [PATCH 0/2] use gettimeofday for current time
@ 2023-03-19 6:43 Paul Eggert
2023-03-19 6:43 ` [PATCH 1/2] git-compat-util: time_now " Paul Eggert
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Paul Eggert @ 2023-03-19 6:43 UTC (permalink / raw)
To: git; +Cc: Paul Eggert
In recent GNU/Linux versions on rare occasions the 'time' function
returns a value that is one second less than the seconds component of
the timestamp that would be returned by gettimeofday, clock_gettime
with CLOCK_REALTIME, 'stat' on a recently modified file, etc.
If a program calls both 'time' and any of these other functions, the
system clock will appear to jump backwards by a second temporarily.
This can lead to confusing or inconsistent output.
A simple workaround for Git is to use 'gettimeofday' instead of
'time'. Although other options are possible (see the commit message
of patch 0002), simplest is probably best.
Paul Eggert (2):
git-compat-util: time_now for current time
git-compat-util: use gettimeofday for current time
archive.c | 2 +-
blame.c | 2 +-
builtin/bugreport.c | 2 +-
builtin/credential-cache--daemon.c | 4 ++--
builtin/diagnose.c | 2 +-
builtin/fast-import.c | 2 +-
builtin/fsmonitor--daemon.c | 4 ++--
builtin/gc.c | 2 +-
builtin/log.c | 2 +-
builtin/receive-pack.c | 2 +-
builtin/reflog.c | 2 +-
commit-graph.c | 4 ++--
compat/nedmalloc/malloc.c.h | 2 +-
credential.c | 4 ++--
date.c | 6 +++---
git-compat-util.h | 8 ++++++++
http-backend.c | 2 +-
http-push.c | 6 +++---
rerere.c | 2 +-
run-command.c | 4 ++--
t/helper/test-chmtime.c | 4 ++--
t/helper/test-simple-ipc.c | 4 ++--
22 files changed, 40 insertions(+), 32 deletions(-)
--
2.39.2
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH 1/2] git-compat-util: time_now for current time 2023-03-19 6:43 [PATCH 0/2] use gettimeofday for current time Paul Eggert @ 2023-03-19 6:43 ` Paul Eggert 2023-03-19 6:43 ` [PATCH 2/2] git-compat-util: use gettimeofday " Paul Eggert 2023-03-20 23:05 ` [PATCH v2] git-compat-util: use gettimeofday(2) for time(2) Junio C Hamano 2 siblings, 0 replies; 19+ messages in thread From: Paul Eggert @ 2023-03-19 6:43 UTC (permalink / raw) To: git; +Cc: Paul Eggert Add a function time_now to get the current time as a time_t value. All uses of time(NULL) or time(&xxx) changed to use this new function. This refactoring does not change behavior. Signed-off-by: Paul Eggert <eggert@cs.ucla.edu> --- archive.c | 2 +- blame.c | 2 +- builtin/bugreport.c | 2 +- builtin/credential-cache--daemon.c | 4 ++-- builtin/diagnose.c | 2 +- builtin/fast-import.c | 2 +- builtin/fsmonitor--daemon.c | 4 ++-- builtin/gc.c | 2 +- builtin/log.c | 2 +- builtin/receive-pack.c | 2 +- builtin/reflog.c | 2 +- commit-graph.c | 4 ++-- compat/nedmalloc/malloc.c.h | 2 +- credential.c | 4 ++-- date.c | 6 +++--- git-compat-util.h | 5 +++++ http-backend.c | 2 +- http-push.c | 6 +++--- rerere.c | 2 +- run-command.c | 4 ++-- t/helper/test-chmtime.c | 4 ++-- t/helper/test-simple-ipc.c | 4 ++-- 22 files changed, 37 insertions(+), 32 deletions(-) diff --git a/archive.c b/archive.c index 1c2ca78e52..5cdafc9b56 100644 --- a/archive.c +++ b/archive.c @@ -472,7 +472,7 @@ static void parse_treeish_arg(const char **argv, archive_time = commit->date; } else { commit_oid = NULL; - archive_time = time(NULL); + archive_time = time_now(); } if (ar_args->mtime_option) archive_time = approxidate(ar_args->mtime_option); diff --git a/blame.c b/blame.c index e45d8a3bf9..df14c1dfe4 100644 --- a/blame.c +++ b/blame.c @@ -192,7 +192,7 @@ static struct commit *fake_working_tree_commit(struct repository *r, struct strbuf msg = STRBUF_INIT; repo_read_index(r); - time(&now); + now = time_now(); commit = alloc_commit_node(r); commit->object.parsed = 1; commit->date = now; diff --git a/builtin/bugreport.c b/builtin/bugreport.c index 5bc254be80..bba5820787 100644 --- a/builtin/bugreport.c +++ b/builtin/bugreport.c @@ -98,7 +98,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) struct strbuf buffer = STRBUF_INIT; struct strbuf report_path = STRBUF_INIT; int report = -1; - time_t now = time(NULL); + time_t now = time_now(); struct tm tm; enum diagnose_mode diagnose = DIAGNOSE_NONE; char *option_output = NULL; diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c index 6e509d02c3..7a87df16d2 100644 --- a/builtin/credential-cache--daemon.c +++ b/builtin/credential-cache--daemon.c @@ -27,7 +27,7 @@ static void cache_credential(struct credential *c, int timeout) /* take ownership of pointers */ memcpy(&e->item, c, sizeof(*c)); memset(c, 0, sizeof(*c)); - e->expiration = time(NULL) + timeout; + e->expiration = time_now() + timeout; } static struct credential_cache_entry *lookup_credential(const struct credential *c) @@ -54,7 +54,7 @@ static timestamp_t check_expirations(void) { static timestamp_t wait_for_entry_until; int i = 0; - timestamp_t now = time(NULL); + timestamp_t now = time_now(); timestamp_t next = TIME_MAX; /* diff --git a/builtin/diagnose.c b/builtin/diagnose.c index d52015c67a..c502096e6c 100644 --- a/builtin/diagnose.c +++ b/builtin/diagnose.c @@ -11,7 +11,7 @@ static const char * const diagnose_usage[] = { int cmd_diagnose(int argc, const char **argv, const char *prefix) { struct strbuf zip_path = STRBUF_INIT; - time_t now = time(NULL); + time_t now = time_now(); struct tm tm; enum diagnose_mode mode = DIAGNOSE_STATS; char *option_output = NULL; diff --git a/builtin/fast-import.c b/builtin/fast-import.c index f7548ff4a3..9096f52075 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -335,7 +335,7 @@ static void write_crash_report(const char *err) fprintf(rpt, "fast-import crash report:\n"); fprintf(rpt, " fast-import process: %"PRIuMAX"\n", (uintmax_t) getpid()); fprintf(rpt, " parent process : %"PRIuMAX"\n", (uintmax_t) getppid()); - fprintf(rpt, " at %s\n", show_date(time(NULL), 0, DATE_MODE(ISO8601))); + fprintf(rpt, " at %s\n", show_date(time_now(), 0, DATE_MODE(ISO8601))); fputc('\n', rpt); fputs("fatal: ", rpt); diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c index cae804a190..45cc8134cc 100644 --- a/builtin/fsmonitor--daemon.c +++ b/builtin/fsmonitor--daemon.c @@ -415,7 +415,7 @@ static struct fsmonitor_token_data *fsmonitor_new_token_data(void) * events to accumulate. */ if (test_env_value) - batch->pinned_time = time(NULL); + batch->pinned_time = time_now(); return token; } @@ -772,7 +772,7 @@ static int do_handle_client(struct fsmonitor_daemon_state *state, */ token_data = state->current_token_data; batch_head = token_data->batch_head; - ((struct fsmonitor_batch *)batch_head)->pinned_time = time(NULL); + ((struct fsmonitor_batch *)batch_head)->pinned_time = time_now(); /* * FSMonitor Protocol V2 requires that we send a response header diff --git a/builtin/gc.c b/builtin/gc.c index c58fe8c936..029bd2ba5c 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -450,7 +450,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) * after the user verifies that no gc is * running. */ - time(NULL) - st.st_mtime <= 12 * 3600 && + time_now() - st.st_mtime <= 12 * 3600 && fscanf(fp, scan_fmt, &pid, locking_host) == 2 && /* be gentle to concurrent "gc" on remote hosts */ (strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM); diff --git a/builtin/log.c b/builtin/log.c index b62e629ba8..bdc62d9b9e 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1168,7 +1168,7 @@ static void gen_message_id(struct rev_info *info, char *base) { struct strbuf buf = STRBUF_INIT; strbuf_addf(&buf, "%s.%"PRItime".git.%s", base, - (timestamp_t) time(NULL), + (timestamp_t) time_now(), git_committer_info(IDENT_NO_NAME|IDENT_NO_DATE|IDENT_STRICT)); info->message_id = strbuf_detach(&buf, NULL); } diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index fe68c79ee3..3fedeea65c 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -2504,7 +2504,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) git_config(receive_pack_config, NULL); if (cert_nonce_seed) - push_cert_nonce = prepare_push_cert_nonce(service_dir, time(NULL)); + push_cert_nonce = prepare_push_cert_nonce(service_dir, time_now()); if (0 <= transfer_unpack_limit) unpack_limit = transfer_unpack_limit; diff --git a/builtin/reflog.c b/builtin/reflog.c index 270681dcdf..013673e768 100644 --- a/builtin/reflog.c +++ b/builtin/reflog.c @@ -237,7 +237,7 @@ static int cmd_reflog_show(int argc, const char **argv, const char *prefix) static int cmd_reflog_expire(int argc, const char **argv, const char *prefix) { struct cmd_reflog_expire_cb cmd = { 0 }; - timestamp_t now = time(NULL); + timestamp_t now = time_now(); int i, status, do_all, all_worktrees = 1; unsigned int flags = 0; int verbose = 0; diff --git a/commit-graph.c b/commit-graph.c index 5e6098ff35..4c75d96fc4 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -2196,7 +2196,7 @@ static void merge_commit_graphs(struct write_commit_graph_context *ctx) static void mark_commit_graphs(struct write_commit_graph_context *ctx) { uint32_t i; - time_t now = time(NULL); + time_t now = time_now(); for (i = ctx->num_commit_graphs_after - 1; i < ctx->num_commit_graphs_before; i++) { struct stat st; @@ -2217,7 +2217,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx) DIR *dir; struct dirent *de; size_t dirnamelen; - timestamp_t expire_time = time(NULL); + timestamp_t expire_time = time_now(); if (ctx->opts && ctx->opts->expire_time) expire_time = ctx->opts->expire_time; diff --git a/compat/nedmalloc/malloc.c.h b/compat/nedmalloc/malloc.c.h index 814845d4b3..4c30a49e60 100644 --- a/compat/nedmalloc/malloc.c.h +++ b/compat/nedmalloc/malloc.c.h @@ -3085,7 +3085,7 @@ static int init_mparams(void) { #ifdef WIN32 magic = (size_t)(GetTickCount() ^ (size_t)0x55555555U); #else - magic = (size_t)(time(0) ^ (size_t)0x55555555U); + magic = (size_t)(time_now() ^ (size_t)0x55555555U); #endif magic |= (size_t)8U; /* ensure nonzero */ magic &= ~(size_t)7U; /* improve chances of fault for bad values */ diff --git a/credential.c b/credential.c index ea40a2a410..dc750fa502 100644 --- a/credential.c +++ b/credential.c @@ -356,7 +356,7 @@ void credential_fill(struct credential *c) for (i = 0; i < c->helpers.nr; i++) { credential_do(c, c->helpers.items[i].string, "get"); - if (c->password_expiry_utc < time(NULL)) { + if (c->password_expiry_utc < time_now()) { /* Discard expired password */ FREE_AND_NULL(c->password); /* Reset expiry to maintain consistency */ @@ -380,7 +380,7 @@ void credential_approve(struct credential *c) if (c->approved) return; - if (!c->username || !c->password || c->password_expiry_utc < time(NULL)) + if (!c->username || !c->password || c->password_expiry_utc < time_now()) return; credential_apply_config(c); diff --git a/date.c b/date.c index 6f45eeb356..16717be397 100644 --- a/date.c +++ b/date.c @@ -597,7 +597,7 @@ static int match_multi_number(timestamp_t num, char c, const char *date, case '/': case '.': if (!now) - now = time(NULL); + now = time_now(); refuse_future = NULL; if (gmtime_r(&now, &now_tm)) refuse_future = &now_tm; @@ -712,7 +712,7 @@ static int match_digit(const char *date, struct tm *tm, int *offset, int *tm_gmt unsigned int num2 = (num % 10000) / 100; unsigned int num3 = num % 100; if (n == 8) - set_date(num1, num2, num3, NULL, time(NULL), tm); + set_date(num1, num2, num3, NULL, time_now(), tm); else if (n == 6 && set_time(num1, num2, num3, tm) == 0 && *end == '.' && isdigit(end[1])) strtoul(end + 1, &end, 10); @@ -1041,7 +1041,7 @@ void datestamp(struct strbuf *out) int offset; struct tm tm = { 0 }; - time(&now); + now = time_now(); offset = tm_to_time_t(localtime_r(&now, &tm)) - now; offset /= 60; diff --git a/git-compat-util.h b/git-compat-util.h index 1e6592624d..d05f4bac22 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -339,6 +339,11 @@ static inline const char *precompose_string_if_needed(const char *in) int compat_mkdir_wo_trailing_slash(const char*, mode_t); #endif +static inline time_t time_now(void) +{ + return time(NULL); +} + #ifdef NO_STRUCT_ITIMERVAL struct itimerval { struct timeval it_interval; diff --git a/http-backend.c b/http-backend.c index 9cfc6f2541..d533989e32 100644 --- a/http-backend.c +++ b/http-backend.c @@ -113,7 +113,7 @@ static void hdr_nocache(struct strbuf *hdr) static void hdr_cache_forever(struct strbuf *hdr) { - timestamp_t now = time(NULL); + timestamp_t now = time_now(); hdr_date(hdr, "Date", now); hdr_date(hdr, "Expires", now + 31536000); hdr_str(hdr, "Cache-Control", "public, max-age=31536000"); diff --git a/http-push.c b/http-push.c index 88aa045ecb..d8158667a0 100644 --- a/http-push.c +++ b/http-push.c @@ -459,7 +459,7 @@ static int refresh_lock(struct remote_lock *lock) fprintf(stderr, "LOCK HTTP error %ld\n", results.http_code); } else { - lock->start_time = time(NULL); + lock->start_time = time_now(); rc = 1; } } @@ -473,7 +473,7 @@ static int refresh_lock(struct remote_lock *lock) static void check_locks(void) { struct remote_lock *lock = repo->locks; - time_t current_time = time(NULL); + time_t current_time = time_now(); int time_remaining; while (lock) { @@ -933,7 +933,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout) FREE_AND_NULL(lock); } else { lock->url = url; - lock->start_time = time(NULL); + lock->start_time = time_now(); lock->next = repo->locks; repo->locks = lock; } diff --git a/rerere.c b/rerere.c index a67abaab07..ac36a9c234 100644 --- a/rerere.c +++ b/rerere.c @@ -1179,7 +1179,7 @@ void rerere_gc(struct repository *r, struct string_list *rr) DIR *dir; struct dirent *e; int i; - timestamp_t now = time(NULL); + timestamp_t now = time_now(); timestamp_t cutoff_noresolve = now - 15 * 86400; timestamp_t cutoff_resolve = now - 60 * 86400; diff --git a/run-command.c b/run-command.c index ba617655b2..fbf9afdb27 100644 --- a/run-command.c +++ b/run-command.c @@ -1864,7 +1864,7 @@ enum start_bg_result start_bg_command(struct child_process *cmd, goto done; } - time(&time_limit); + time_limit = time_now(); time_limit += timeout_sec; wait: @@ -1891,7 +1891,7 @@ enum start_bg_result start_bg_command(struct child_process *cmd, */ time_t now; - time(&now); + now = time_now(); if (now < time_limit) goto wait; diff --git a/t/helper/test-chmtime.c b/t/helper/test-chmtime.c index dc28890a18..f48a07f6f7 100644 --- a/t/helper/test-chmtime.c +++ b/t/helper/test-chmtime.c @@ -7,7 +7,7 @@ * * test-tool chmtime =<seconds> file... * - * Relative to the current time as returned by time(3): + * Relative to the current time as returned by time_now(): * * test-tool chmtime =+<seconds> (or =-<seconds>) file... * @@ -60,7 +60,7 @@ static int timespec_arg(const char *arg, long int *set_time, int *set_eq) return 0; } if ((*set_eq && *set_time < 0) || *set_eq == 2) { - time_t now = time(NULL); + time_t now = time_now(); *set_time += now; } return 1; diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c index 3d1436da59..2267cd9fac 100644 --- a/t/helper/test-simple-ipc.c +++ b/t/helper/test-simple-ipc.c @@ -411,7 +411,7 @@ static int client__stop_server(void) time_t time_limit, now; enum ipc_active_state s; - time(&time_limit); + time_limit = time_now(); time_limit += cl_args.max_wait_sec; cl_args.token = "quit"; @@ -434,7 +434,7 @@ static int client__stop_server(void) return 0; } - time(&now); + now = time_now(); if (now > time_limit) return error("daemon has not shutdown yet"); } -- 2.39.2 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/2] git-compat-util: use gettimeofday for current time 2023-03-19 6:43 [PATCH 0/2] use gettimeofday for current time Paul Eggert 2023-03-19 6:43 ` [PATCH 1/2] git-compat-util: time_now " Paul Eggert @ 2023-03-19 6:43 ` Paul Eggert 2023-03-19 19:34 ` Eric Wong 2023-03-20 23:05 ` [PATCH v2] git-compat-util: use gettimeofday(2) for time(2) Junio C Hamano 2 siblings, 1 reply; 19+ messages in thread From: Paul Eggert @ 2023-03-19 6:43 UTC (permalink / raw) To: git; +Cc: Paul Eggert Use gettimeofday instead of time(NULL) to get current time. This avoids clock skew on glibc 2.31+ on Linux, where in the first 1 to 2.5 ms of every second, time(NULL) returns a value that is one less than the tv_sec part of higher-resolution timestamps such as those returned by gettimeofday or timespec_get, or those in the file system. There are similar clock skew problems on AIX and MS-Windows, which have problems in the first 5 ms of every second. Without this patch, users can observe Git issuing a timestamp T+1 before it issues timestamp T, because Git sometimes uses time(NULL) or time(&t) and sometimes uses higher-res methods like gettimeofday. Although strictly speaking users should tolerate this behavuior because a superuser can always change the clock back, this is a quality of implementation issue and users naturally expect Git to issue timestamps in increasing order unless the superuser has fiddled with the system clock. This patch always uses gettimeofday(...) instead of time(...), and I have verified that the resulting .o files never refer to the name 'time'. A trickier patch would change only those calls for which timestamp monotonicity is user-visible. Such a patch would require more expertise about Git internals, though, and would be harder to maintain later. Another possibility would be to change Git's documentation to warn users that Git does not always issue timestamps in increasing order. However, Git users would likely be either dismayed by this possibility, or confused by the level of detail that any such documentation would require. Yet another possibility would be to fix the Linux kernel so that the time syscall is consistent with the other timestamp syscalls. I suppose this has not been done due to performance implications. (Git's use of timestamps is rare enough that performance is not a significant consideration for git.) However, this wouldn't fix Git's problem on older Linux kernels, or on AIX or MS-Windows. Signed-off-by: Paul Eggert <eggert@cs.ucla.edu> --- git-compat-util.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/git-compat-util.h b/git-compat-util.h index d05f4bac22..dcd9e5ca65 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -341,7 +341,10 @@ int compat_mkdir_wo_trailing_slash(const char*, mode_t); static inline time_t time_now(void) { - return time(NULL); + /* Avoid time(NULL), which can disagree with gettimeofday etc. */ + struct timeval tv; + gettimeofday(&tv, NULL); + return tv.tv_sec; } #ifdef NO_STRUCT_ITIMERVAL -- 2.39.2 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] git-compat-util: use gettimeofday for current time 2023-03-19 6:43 ` [PATCH 2/2] git-compat-util: use gettimeofday " Paul Eggert @ 2023-03-19 19:34 ` Eric Wong 2023-03-20 16:33 ` Junio C Hamano 0 siblings, 1 reply; 19+ messages in thread From: Eric Wong @ 2023-03-19 19:34 UTC (permalink / raw) To: git; +Cc: Paul Eggert Paul Eggert <eggert@cs.ucla.edu> wrote: > Use gettimeofday instead of time(NULL) to get current time. > This avoids clock skew on glibc 2.31+ on Linux, where in the > first 1 to 2.5 ms of every second, time(NULL) returns a > value that is one less than the tv_sec part of > higher-resolution timestamps such as those returned by > gettimeofday or timespec_get, or those in the file system. > There are similar clock skew problems on AIX and MS-Windows, > which have problems in the first 5 ms of every second. Wow, this is enlightening... and unfortunate :< I decided to check glibc archives to find more discussion on it. So far, I've found: https://inbox.sourceware.org/libc-alpha/20230306160321.2942372-1-adhemerval.zanella@linaro.org/T/ and the original bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30200 And this is due to the time64 changes in glibc 2.31+? (<= 2.30 isn't affected?) <snip> > Yet another possibility would be to fix the Linux kernel so > that the time syscall is consistent with the other timestamp > syscalls. I suppose this has not been done due to > performance implications. (Git's use of timestamps is rare > enough that performance is not a significant consideration > for git.) However, this wouldn't fix Git's problem on older > Linux kernels, or on AIX or MS-Windows. Agreed on the older kernels and other OSes part. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] git-compat-util: use gettimeofday for current time 2023-03-19 19:34 ` Eric Wong @ 2023-03-20 16:33 ` Junio C Hamano 2023-03-20 17:01 ` Junio C Hamano 2023-03-20 20:35 ` Taylor Blau 0 siblings, 2 replies; 19+ messages in thread From: Junio C Hamano @ 2023-03-20 16:33 UTC (permalink / raw) To: Eric Wong; +Cc: git, Paul Eggert Eric Wong <e@80x24.org> writes: > Paul Eggert <eggert@cs.ucla.edu> wrote: >> Use gettimeofday instead of time(NULL) to get current time. >> This avoids clock skew on glibc 2.31+ on Linux, where in the >> first 1 to 2.5 ms of every second, time(NULL) returns a >> value that is one less than the tv_sec part of >> higher-resolution timestamps such as those returned by >> gettimeofday or timespec_get, or those in the file system. >> There are similar clock skew problems on AIX and MS-Windows, >> which have problems in the first 5 ms of every second. > > Wow, this is enlightening... and unfortunate :< > > I decided to check glibc archives to find more discussion on it. > So far, I've found: > > https://inbox.sourceware.org/libc-alpha/20230306160321.2942372-1-adhemerval.zanella@linaro.org/T/ > > and the original bug: > https://sourceware.org/bugzilla/show_bug.cgi?id=30200 > > And this is due to the time64 changes in glibc 2.31+? > (<= 2.30 isn't affected?) > > <snip> > >> Yet another possibility would be to fix the Linux kernel so >> that the time syscall is consistent with the other timestamp >> syscalls. I suppose this has not been done due to >> performance implications. (Git's use of timestamps is rare >> enough that performance is not a significant consideration >> for git.) However, this wouldn't fix Git's problem on older >> Linux kernels, or on AIX or MS-Windows. > > Agreed on the older kernels and other OSes part. Yeah, this is interesting. I however wonder if we should follow our usual pattern of implementing git_time() with the identical function signature as what we replace (i.e. system's time()), and #undef/#define the symbol we replace with git_time, though. Wouldn't it make [1/2] a lot smaller and future-proof? ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] git-compat-util: use gettimeofday for current time 2023-03-20 16:33 ` Junio C Hamano @ 2023-03-20 17:01 ` Junio C Hamano 2023-03-20 19:00 ` Paul Eggert 2023-03-20 20:35 ` Taylor Blau 1 sibling, 1 reply; 19+ messages in thread From: Junio C Hamano @ 2023-03-20 17:01 UTC (permalink / raw) To: Eric Wong, Paul Eggert; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > Eric Wong <e@80x24.org> writes: > ... >> I decided to check glibc archives to find more discussion on it. >> So far, I've found: >> >> https://inbox.sourceware.org/libc-alpha/20230306160321.2942372-1-adhemerval.zanella@linaro.org/T/ >> >> and the original bug: >> https://sourceware.org/bugzilla/show_bug.cgi?id=30200 >> >> And this is due to the time64 changes in glibc 2.31+? >> (<= 2.30 isn't affected?) >> > ... > > Yeah, this is interesting. I however wonder if we should follow our > usual pattern of implementing git_time() with the identical function > signature as what we replace (i.e. system's time()), and #undef/#define > the symbol we replace with git_time, though. Wouldn't it make [1/2] > a lot smaller and future-proof? And here is how such an approach may look like. The second part is more or less the same as Paul's version so I kept his authorship there (these are mostly for discussion and not for application). ----- >8 ----- [PATCH 1/2] ----- >8 ----- Subject: [PATCH] time(): allow OVERRIDE_TIME to redefine time(2) Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Makefile | 11 +++++++++++ compat/time.c | 11 +++++++++++ config.mak.uname | 1 + git-compat-util.h | 8 ++++++++ 4 files changed, 31 insertions(+) create mode 100644 compat/time.c diff --git a/Makefile b/Makefile index 50ee51fde3..aa6fcd6e04 100644 --- a/Makefile +++ b/Makefile @@ -286,6 +286,12 @@ include shared.mak # crashes due to allocation and free working on different 'heaps'. # It's defined automatically if USE_NED_ALLOCATOR is set. # +# Define OVERRIDE_TIME to override time(2) and replace it with an +# implementation based on gettimeofday(2). THis is necessary when +# glibc 2.31+ on Linux is used, where in the first 1 to 2.5 ms of +# every second, time(NULL) returns a value that is one less than the +# tv_sec part of higher-resolution timestamps used in the file system. +# # Define NO_REGEX if your C library lacks regex support with REG_STARTEND # feature. # @@ -2061,6 +2067,11 @@ ifdef OVERRIDE_STRDUP COMPAT_OBJS += compat/strdup.o endif +ifdef OVERRIDE_TIME + COMPAT_CFLAGS += -DOVERRIDE_TIME + COMPAT_OBJS += compat/time.o +endif + ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT export GIT_TEST_CMP_USE_COPIED_CONTEXT endif diff --git a/compat/time.c b/compat/time.c new file mode 100644 index 0000000000..a3ef5c0e98 --- /dev/null +++ b/compat/time.c @@ -0,0 +1,11 @@ +#include "../git-compat-util.h" + +#undef time +time_t git_time(time_t *tloc) +{ + time_t t = time(NULL); + + if (tloc) + *tloc = t; + return t; +} diff --git a/config.mak.uname b/config.mak.uname index 64c44db805..29398919e8 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -64,6 +64,7 @@ ifeq ($(uname_S),Linux) PROCFS_EXECUTABLE_PATH = /proc/self/exe HAVE_PLATFORM_PROCINFO = YesPlease COMPAT_OBJS += compat/linux/procinfo.o + OVERRIDE_TIME = YesPlease # centos7/rhel7 provides gcc 4.8.5 and zlib 1.2.7. ifneq ($(findstring .el7.,$(uname_R)),) BASIC_CFLAGS += -std=c99 diff --git a/git-compat-util.h b/git-compat-util.h index 1e6592624d..2279f3c90c 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -917,6 +917,14 @@ void *gitmemmem(const void *haystack, size_t haystacklen, char *gitstrdup(const char *s); #endif +#ifdef OVERRIDE_TIME +#ifdef time +#undef time +#endif +#define time git_time +extern time_t git_time(time_t *); +#endif + #ifdef NO_GETPAGESIZE #define getpagesize() sysconf(_SC_PAGESIZE) #endif -- 2.40.0-115-ge25cabbf6b ----- >8 ----- [PATCH 2/2] ----- >8 ----- From: Paul Eggert <eggert@cs.ucla.edu> From: Paul Eggert <eggert@cs.ucla.edu> Subject: [PATCH] time(): use gettimeofday() Use gettimeofday instead of time(NULL) to get current time. This avoids clock skew on glibc 2.31+ on Linux, where in the first 1 to 2.5 ms of every second, time(NULL) returns a value that is one less than the tv_sec part of higher-resolution timestamps such as those returned by gettimeofday or timespec_get, or those in the file system. There are similar clock skew problems on AIX and MS-Windows, which have problems in the first 5 ms of every second. Without this patch, users can observe Git issuing a timestamp T+1 before it issues timestamp T, because Git sometimes uses time(NULL) or time(&t) and sometimes uses higher-res methods like gettimeofday. Although strictly speaking users should tolerate this behavuior because a superuser can always change the clock back, this is a quality of implementation issue and users naturally expect Git to issue timestamps in increasing order unless the superuser has fiddled with the system clock. This patch always uses gettimeofday(...) instead of time(...), and I have verified that the resulting .o files never refer to the name 'time'. A trickier patch would change only those calls for which timestamp monotonicity is user-visible. Such a patch would require more expertise about Git internals, though, and would be harder to maintain later. Another possibility would be to change Git's documentation to warn users that Git does not always issue timestamps in increasing order. However, Git users would likely be either dismayed by this possibility, or confused by the level of detail that any such documentation would require. Yet another possibility would be to fix the Linux kernel so that the time syscall is consistent with the other timestamp syscalls. I suppose this has not been done due to performance implications. (Git's use of timestamps is rare enough that performance is not a significant consideration for git.) However, this wouldn't fix Git's problem on older Linux kernels, or on AIX or MS-Windows. Signed-off-by: Paul Eggert <eggert@cs.ucla.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- compat/time.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/compat/time.c b/compat/time.c index a3ef5c0e98..758a160ee7 100644 --- a/compat/time.c +++ b/compat/time.c @@ -3,9 +3,15 @@ #undef time time_t git_time(time_t *tloc) { - time_t t = time(NULL); + /* + * Avoid time(NULL), which can disagree with gettimeofday() + * and filesystem timestamps. + */ + struct timeval tv; + + gettimeofday(&tv, NULL); if (tloc) - *tloc = t; - return t; + *tloc = tv.tv_sec; + return tv.tv_sec; } -- 2.40.0-115-ge25cabbf6b ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] git-compat-util: use gettimeofday for current time 2023-03-20 17:01 ` Junio C Hamano @ 2023-03-20 19:00 ` Paul Eggert 2023-03-20 19:40 ` Junio C Hamano 0 siblings, 1 reply; 19+ messages in thread From: Paul Eggert @ 2023-03-20 19:00 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Eric Wong On 3/20/23 10:01, Junio C Hamano wrote: >> I however wonder if we should follow our >> usual pattern of implementing git_time() with the identical function >> signature as what we replace (i.e. system's time()), and #undef/#define >> the symbol we replace with git_time, though. Wouldn't it make [1/2] >> a lot smaller and future-proof? Yes, something like that would work too. (Sorry, I didn't know about the usual pattern.) > +# Define OVERRIDE_TIME to override time(2) and replace it with an > +# implementation based on gettimeofday(2). THis is necessary when > +# glibc 2.31+ on Linux is used, where in the first 1 to 2.5 ms of > +# every second, time(NULL) returns a value that is one less than the > +# tv_sec part of higher-resolution timestamps used in the file system. THis -> This It might be simpler to use the gettimeofday workaround on all platforms, rather than having an OVERRIDE_TIME flag and complicating config.mak.uname. gettimeofday should be portable, as it's already used elsewhere in Git without configury. If we're going with the more-complicated solution, config.mak.uname will need changes in its AIX and MS-Windows sections because the problem is known to occur there too. Although AIX configuration is simple, I'm not sure how to go about the MS-Windows part as there seem to be a lot of ifdefs there. Also, because the time problem does not occur with musl libc (used in Alpine Linux 3.17), on the Linux side the workaround could be employed only if glibc 2.31+ is in use. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] git-compat-util: use gettimeofday for current time 2023-03-20 19:00 ` Paul Eggert @ 2023-03-20 19:40 ` Junio C Hamano 2023-03-20 20:36 ` Taylor Blau 0 siblings, 1 reply; 19+ messages in thread From: Junio C Hamano @ 2023-03-20 19:40 UTC (permalink / raw) To: Paul Eggert; +Cc: git, Eric Wong Paul Eggert <eggert@cs.ucla.edu> writes: > It might be simpler to use the gettimeofday workaround on all > platforms, rather than having an OVERRIDE_TIME flag and complicating > config.mak.uname. gettimeofday should be portable, as it's already > used elsewhere in Git without configury. That is an excellent point. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] git-compat-util: use gettimeofday for current time 2023-03-20 19:40 ` Junio C Hamano @ 2023-03-20 20:36 ` Taylor Blau 0 siblings, 0 replies; 19+ messages in thread From: Taylor Blau @ 2023-03-20 20:36 UTC (permalink / raw) To: Junio C Hamano; +Cc: Paul Eggert, git, Eric Wong On Mon, Mar 20, 2023 at 12:40:07PM -0700, Junio C Hamano wrote: > Paul Eggert <eggert@cs.ucla.edu> writes: > > > It might be simpler to use the gettimeofday workaround on all > > platforms, rather than having an OVERRIDE_TIME flag and complicating > > config.mak.uname. gettimeofday should be portable, as it's already > > used elsewhere in Git without configury. > > That is an excellent point. I'd be happy to assume OVERRIDE_TIME is set everywhere and just drop it entirely (using gettimeofday() unconditionally within git_time()). An alternative approach might be to leave OVERRIDE_TIME time in place, but treat it as opt-out instead of opt-in. I can imagine that some exotic platform might want to use time() instead of gettimeofday() for one reason or another. Thanks, Taylor ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] git-compat-util: use gettimeofday for current time 2023-03-20 16:33 ` Junio C Hamano 2023-03-20 17:01 ` Junio C Hamano @ 2023-03-20 20:35 ` Taylor Blau 1 sibling, 0 replies; 19+ messages in thread From: Taylor Blau @ 2023-03-20 20:35 UTC (permalink / raw) To: Junio C Hamano; +Cc: Eric Wong, git, Paul Eggert On Mon, Mar 20, 2023 at 09:33:01AM -0700, Junio C Hamano wrote: > Yeah, this is interesting. Definitely so. You learn something new every day ;-). > I however wonder if we should follow our usual pattern of implementing > git_time() with the identical function signature as what we replace > (i.e. system's time()), and #undef/#define the symbol we replace with > git_time, though. Wouldn't it make [1/2] a lot smaller and > future-proof? Yeah, I agree here, too. It was my first thought when I started reading Paul's patches here. I think that your approach is sound and I would be happy to see you queue it. Thanks, Taylor ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2] git-compat-util: use gettimeofday(2) for time(2) 2023-03-19 6:43 [PATCH 0/2] use gettimeofday for current time Paul Eggert 2023-03-19 6:43 ` [PATCH 1/2] git-compat-util: time_now " Paul Eggert 2023-03-19 6:43 ` [PATCH 2/2] git-compat-util: use gettimeofday " Paul Eggert @ 2023-03-20 23:05 ` Junio C Hamano 2023-03-20 23:21 ` Paul Eggert ` (2 more replies) 2 siblings, 3 replies; 19+ messages in thread From: Junio C Hamano @ 2023-03-20 23:05 UTC (permalink / raw) To: git; +Cc: Paul Eggert, Eric Wong, Taylor Blau From: Paul Eggert <eggert@cs.ucla.edu> Use gettimeofday instead of time(NULL) to get current time. This avoids clock skew on glibc 2.31+ on Linux, where in the first 1 to 2.5 ms of every second, time(NULL) returns a value that is one less than the tv_sec part of higher-resolution timestamps such as those returned by gettimeofday or timespec_get, or those in the file system. There are similar clock skew problems on AIX and MS-Windows, which have problems in the first 5 ms of every second. Without this patch, users can observe Git issuing a timestamp T+1 before it issues timestamp T, because Git sometimes uses time(NULL) or time(&t) and sometimes uses higher-res methods like gettimeofday. Although strictly speaking users should tolerate this behavuior because a superuser can always change the clock back, this is a quality of implementation issue and users naturally expect Git to issue timestamps in increasing order unless the superuser has fiddled with the system clock. This patch always uses gettimeofday(...) instead of time(...), and I have verified that the resulting .o files never refer to the name 'time'. A trickier patch would change only those calls for which timestamp monotonicity is user-visible. Such a patch would require more expertise about Git internals, though, and would be harder to maintain later. Another possibility would be to change Git's documentation to warn users that Git does not always issue timestamps in increasing order. However, Git users would likely be either dismayed by this possibility, or confused by the level of detail that any such documentation would require. Yet another possibility would be to fix the Linux kernel so that the time syscall is consistent with the other timestamp syscalls. I suppose this has not been done due to performance implications. (Git's use of timestamps is rare enough that performance is not a significant consideration for git.) However, this wouldn't fix Git's problem on older Linux kernels, or on AIX or MS-Windows. Signed-off-by: Paul Eggert <eggert@cs.ucla.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * This one is closer to v1 than my "how about" illustration, in that we use gettimeofday() everywhere, so I kept the authorship ident and timestamp from the original. git-compat-util.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/git-compat-util.h b/git-compat-util.h index 4f0028ce60..d6fbe9311d 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -339,6 +339,25 @@ static inline const char *precompose_string_if_needed(const char *in) int compat_mkdir_wo_trailing_slash(const char*, mode_t); #endif +#ifdef time +#undef time +#endif +static inline time_t git_time(time_t *tloc) +{ + struct timeval tv; + + /* + * Avoid time(NULL), which can disagree with gettimeofday(2) + * and filesystem timestamps. + */ + gettimeofday(&tv, NULL); + + if (tloc) + *tloc = tv.tv_sec; + return tv.tv_sec; +} +#define time(x) git_time(x) + #ifdef NO_STRUCT_ITIMERVAL struct itimerval { struct timeval it_interval; -- 2.40.0-115-ge25cabbf6b ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2] git-compat-util: use gettimeofday(2) for time(2) 2023-03-20 23:05 ` [PATCH v2] git-compat-util: use gettimeofday(2) for time(2) Junio C Hamano @ 2023-03-20 23:21 ` Paul Eggert 2023-03-21 16:20 ` Junio C Hamano 2023-03-21 17:44 ` Konstantin Khomoutov 2023-03-21 18:22 ` Jeff King 2 siblings, 1 reply; 19+ messages in thread From: Paul Eggert @ 2023-03-20 23:21 UTC (permalink / raw) To: Junio C Hamano; +Cc: Eric Wong, Taylor Blau, git Thanks, this looks good. As a matter of fact it almost precisely matches what I was about to email you. The only significant difference is that yours has "#define time(x) git_time(x)" whereas mine had "#define time git_time". Since Git never takes the address of 'time' the two macro definitions should have equivalent effects when used in Git. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] git-compat-util: use gettimeofday(2) for time(2) 2023-03-20 23:21 ` Paul Eggert @ 2023-03-21 16:20 ` Junio C Hamano 0 siblings, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2023-03-21 16:20 UTC (permalink / raw) To: Paul Eggert; +Cc: Eric Wong, Taylor Blau, git Paul Eggert <eggert@cs.ucla.edu> writes: > Thanks, this looks good. As a matter of fact it almost precisely > matches what I was about to email you. The only significant difference > is that yours has "#define time(x) git_time(x)" whereas mine had > "#define time git_time". Since Git never takes the address of 'time' > the two macro definitions should have equivalent effects when used in > Git. That is a valid concern. Writing &time would not be caught by compilers, and you would not notice such a mistake until you run "nm -ug" on the result. On the other hand, straight token replacement will risk renaming variables and structure members, and I was not sure if we have such use of the identifier "time". As long as people do not use "time" and "git_time" at the same time as such identifiers, that would not be an issue (except for perhaps expecting to see them in debuggers). Writing "git_time" and "time" at the same time for identifiers not related to the time(2) function would not be caught by compilers, either, but it feels much less likely mistake we would make in the future, so let me drop (x) from the macro. Thanks. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] git-compat-util: use gettimeofday(2) for time(2) 2023-03-20 23:05 ` [PATCH v2] git-compat-util: use gettimeofday(2) for time(2) Junio C Hamano 2023-03-20 23:21 ` Paul Eggert @ 2023-03-21 17:44 ` Konstantin Khomoutov 2023-03-21 18:22 ` Junio C Hamano 2023-03-21 18:22 ` Jeff King 2 siblings, 1 reply; 19+ messages in thread From: Konstantin Khomoutov @ 2023-03-21 17:44 UTC (permalink / raw) To: git On Mon, Mar 20, 2023 at 04:05:07PM -0700, Junio C Hamano wrote: [...] > higher-res methods like gettimeofday. Although strictly > speaking users should tolerate this behavuior because a A small typo: it should probably read "behavior" or "behaviour". [...] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] git-compat-util: use gettimeofday(2) for time(2) 2023-03-21 17:44 ` Konstantin Khomoutov @ 2023-03-21 18:22 ` Junio C Hamano 0 siblings, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2023-03-21 18:22 UTC (permalink / raw) To: Konstantin Khomoutov; +Cc: git Konstantin Khomoutov <kostix@bswap.ru> writes: > On Mon, Mar 20, 2023 at 04:05:07PM -0700, Junio C Hamano wrote: > > [...] >> higher-res methods like gettimeofday. Although strictly >> speaking users should tolerate this behavuior because a > > A small typo: it should probably read "behavior" or "behaviour". > > [...] Thanks. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] git-compat-util: use gettimeofday(2) for time(2) 2023-03-20 23:05 ` [PATCH v2] git-compat-util: use gettimeofday(2) for time(2) Junio C Hamano 2023-03-20 23:21 ` Paul Eggert 2023-03-21 17:44 ` Konstantin Khomoutov @ 2023-03-21 18:22 ` Jeff King 2023-03-21 19:06 ` Taylor Blau 2023-03-21 20:04 ` Junio C Hamano 2 siblings, 2 replies; 19+ messages in thread From: Jeff King @ 2023-03-21 18:22 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Paul Eggert, Eric Wong, Taylor Blau On Mon, Mar 20, 2023 at 04:05:07PM -0700, Junio C Hamano wrote: > +#ifdef time > +#undef time > +#endif > +static inline time_t git_time(time_t *tloc) > +{ > + struct timeval tv; > + > + /* > + * Avoid time(NULL), which can disagree with gettimeofday(2) > + * and filesystem timestamps. > + */ > + gettimeofday(&tv, NULL); > + > + if (tloc) > + *tloc = tv.tv_sec; > + return tv.tv_sec; > +} > +#define time(x) git_time(x) This looks good to me, but I wanted to mention one alternative. If we are declaring that time() sucks and gettimeofday() is how to do it, then we could just use gettimeofday() everywhere, and add time() to banned.h to catch stragglers. It has two mild advantages: 1. gettimeofday() gives the callers extra resolution if they want it (though in practice I guess none of them really do) 2. It more directly describes what's going on, and we'd play fewer games with macros (though we may end up with a git_gettimeofday() wrapper if somebody doesn't support it; I really wonder about Windows here). The disadvantage is that it's longer to type, and that you have to declare a timeval in the caller. So maybe it's a dumb idea. -Peff ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] git-compat-util: use gettimeofday(2) for time(2) 2023-03-21 18:22 ` Jeff King @ 2023-03-21 19:06 ` Taylor Blau 2023-03-21 20:04 ` Junio C Hamano 1 sibling, 0 replies; 19+ messages in thread From: Taylor Blau @ 2023-03-21 19:06 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git, Paul Eggert, Eric Wong On Tue, Mar 21, 2023 at 02:22:52PM -0400, Jeff King wrote: > The disadvantage is that it's longer to type, and that you have to > declare a timeval in the caller. So maybe it's a dumb idea. I don't think it's a dumb idea per-se, but I think that being able to pass `time(NULL)` around without having to create a timeval and pass a pointer to *it* before then giving that timeval to some other function is a nice advantage. So, yeah, we probably should just avoid calling time() altogether, but in practice I like the solution of redefining time() to do the right thing and implement it by calling gettimeofday(). ...Which is a long way of saying that I agree with you that this approach looks good, and that I'd like to avoid putting time() in the list of banned functions. Thanks, Taylor ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] git-compat-util: use gettimeofday(2) for time(2) 2023-03-21 18:22 ` Jeff King 2023-03-21 19:06 ` Taylor Blau @ 2023-03-21 20:04 ` Junio C Hamano 2023-03-22 17:11 ` Jeff King 1 sibling, 1 reply; 19+ messages in thread From: Junio C Hamano @ 2023-03-21 20:04 UTC (permalink / raw) To: Jeff King; +Cc: git, Paul Eggert, Eric Wong, Taylor Blau Jeff King <peff@peff.net> writes: > This looks good to me, but I wanted to mention one alternative. If we > are declaring that time() sucks and gettimeofday() is how to do it, then > we could just use gettimeofday() everywhere, and add time() to banned.h > to catch stragglers. > > It has two mild advantages: > > 1. gettimeofday() gives the callers extra resolution if they want it > (though in practice I guess none of them really do) True. Many of our data structures do not have room to store the extra resolution right now. > 2. It more directly describes what's going on, and we'd play fewer > games with macros (though we may end up with a git_gettimeofday() > wrapper if somebody doesn't support it; I really wonder about > Windows here). I think they already have a compat/ function for their use in compat/mingw.c so that may not be an issue. > The disadvantage is that it's longer to type, and that you have to > declare a timeval in the caller. So maybe it's a dumb idea. Yes, you have to declare and use a timeval, but you are already declaring and using time_t in today's code, so if we were writing like so from scratch, the result wouldn't have been so bad. We just do nto use time_t and use timeval instead consistently. The patch noise to go from here to there may not be worth it, though. Also, unless we are going to actively take advantage of extra resolution, I am not sure if it is wise to ask for extra resolution in the first place. If time(2), at least on some platforms whose maintainers care, gets corrected to avoid going backwards or being inconsistent with filesystem timestamp in the future, converting everything that calls time(2) to call gettimeofday(2) would lose information---we will want to know which ones need only seconds resolution and convert them back when the world advances. So, I am not quite sure I am sold on the idea of rewriting everything to use gettimeofday(2). Thanks. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] git-compat-util: use gettimeofday(2) for time(2) 2023-03-21 20:04 ` Junio C Hamano @ 2023-03-22 17:11 ` Jeff King 0 siblings, 0 replies; 19+ messages in thread From: Jeff King @ 2023-03-22 17:11 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Paul Eggert, Eric Wong, Taylor Blau On Tue, Mar 21, 2023 at 01:04:49PM -0700, Junio C Hamano wrote: > So, I am not quite sure I am sold on the idea of rewriting > everything to use gettimeofday(2). I'm not sure I am either. :) All your points are fair. Let's forget about my suggestion. -Peff ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2023-03-22 17:12 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-19 6:43 [PATCH 0/2] use gettimeofday for current time Paul Eggert 2023-03-19 6:43 ` [PATCH 1/2] git-compat-util: time_now " Paul Eggert 2023-03-19 6:43 ` [PATCH 2/2] git-compat-util: use gettimeofday " Paul Eggert 2023-03-19 19:34 ` Eric Wong 2023-03-20 16:33 ` Junio C Hamano 2023-03-20 17:01 ` Junio C Hamano 2023-03-20 19:00 ` Paul Eggert 2023-03-20 19:40 ` Junio C Hamano 2023-03-20 20:36 ` Taylor Blau 2023-03-20 20:35 ` Taylor Blau 2023-03-20 23:05 ` [PATCH v2] git-compat-util: use gettimeofday(2) for time(2) Junio C Hamano 2023-03-20 23:21 ` Paul Eggert 2023-03-21 16:20 ` Junio C Hamano 2023-03-21 17:44 ` Konstantin Khomoutov 2023-03-21 18:22 ` Junio C Hamano 2023-03-21 18:22 ` Jeff King 2023-03-21 19:06 ` Taylor Blau 2023-03-21 20:04 ` Junio C Hamano 2023-03-22 17:11 ` Jeff King
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).