Re: [PATCH 2/2] git-compat-util: use gettimeofday for current time

[Junio C Hamano <gitster@pobox.com>]

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