Skip to contentSkip to conversation

Use nanosecond-precision file times on Windows

PATCH In review Johannes Schindelin via GitGitGadget 4 participants 8 messages Oct 23, 2018 – Oct 25, 2018

lore
Share & tools
lore
#0 lore raw

cover Use nanosecond-precision file times on Windows

This is yet another patch series in the slow wave of patches coming over from Git for Windows.

With this change, we now use preciser timestamps to determine e.g. whether the Git index is out of date. This change made it into Git for Windows already in version 2.6.0, i.e. for a little over three years.

Please note that this change originally caused a lot of trouble, as e.g. libgit2 was unaware of our plans and used second-precision file times. So if you used Git for Windows as well as a libgit2-based program to, say, update the Git index, there would be a back-and-forth between index updates with and without the fractional second parts, causing quite a bit of bad performance.

These issues have been ironed out long ago, though, so it is high time to contribute these patches to core Git.

Johannes Schindelin (1):
mingw: factor out code to set stat() data

Karsten Blees (2):
mingw: replace MSVCRT's fstat() with a Win32-based implementation
mingw: implement nanosecond-precision file times

 compat/mingw.c   | 76 +++++++++++++++++++++++++++++++-----------------
 compat/mingw.h   | 36 ++++++++++++++++-------
 config.mak.uname |  2 --
 3 files changed, 76 insertions(+), 38 deletions(-)

base-commit: c4df23f7927d8d00e666a3c8d1b3375f1dc8a3c1
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-53%2Fdscho%2Fnanosecond-file-times-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-53/dscho/nanosecond-file-times-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/53

Signature · 1 line
gitgitgadget
#1 lore raw

PATCH 1/3 mingw: factor out code to set stat() data

From: Johannes Schindelin <[email protected]>

In our fstat() emulation, we convert the file metadata from Win32 data structures to an emulated POSIX structure.

To structure the code better, let's factor that part out into its own function.

Note: it would be tempting to try to unify this code with the part of do_lstat() that does the same thing, but they operate on different data
structures: BY_HANDLE_FILE_INFORMATION vs WIN32_FILE_ATTRIBUTE_DATA. So unfortunately, they cannot be unified.

---
 compat/mingw.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)
compat/mingw.c +25 -14
index 18caf2196..d2e7d86db 100644
@@ -736,6 +736,29 @@ static int do_stat_internal(int follow, const char *file_name, struct stat *buf)
 	return do_lstat(follow, alt_name, buf);
 }
 
+static int get_file_info_by_handle(HANDLE hnd, struct stat *buf)
+{
+	BY_HANDLE_FILE_INFORMATION fdata;
+
+	if (!GetFileInformationByHandle(hnd, &fdata)) {
+		errno = err_win_to_posix(GetLastError());
+		return -1;
+	}
+
+	buf->st_ino = 0;
+	buf->st_gid = 0;
+	buf->st_uid = 0;
+	buf->st_nlink = 1;
+	buf->st_mode = file_attr_to_st_mode(fdata.dwFileAttributes);
+	buf->st_size = fdata.nFileSizeLow |
+		(((off_t)fdata.nFileSizeHigh)<<32);
+	buf->st_dev = buf->st_rdev = 0; /* not used by Git */
+	buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
+	buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
+	buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
+	return 0;
+}
+
 int mingw_lstat(const char *file_name, struct stat *buf)
 {
 	return do_stat_internal(0, file_name, buf);
@@ -748,7 +771,6 @@ int mingw_stat(const char *file_name, struct stat *buf)
 int mingw_fstat(int fd, struct stat *buf)
 {
 	HANDLE fh = (HANDLE)_get_osfhandle(fd);
-	BY_HANDLE_FILE_INFORMATION fdata;
 
 	if (fh == INVALID_HANDLE_VALUE) {
 		errno = EBADF;
@@ -758,20 +780,9 @@ int mingw_fstat(int fd, struct stat *buf)
 	if (GetFileType(fh) != FILE_TYPE_DISK)
 		return _fstati64(fd, buf);
 
-	if (GetFileInformationByHandle(fh, &fdata)) {
-		buf->st_ino = 0;
-		buf->st_gid = 0;
-		buf->st_uid = 0;
-		buf->st_nlink = 1;
-		buf->st_mode = file_attr_to_st_mode(fdata.dwFileAttributes);
-		buf->st_size = fdata.nFileSizeLow |
-			(((off_t)fdata.nFileSizeHigh)<<32);
-		buf->st_dev = buf->st_rdev = 0; /* not used by Git */
-		buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
-		buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
-		buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
+	if (!get_file_info_by_handle(fh, buf))
 		return 0;
-	}
+
 	errno = EBADF;
 	return -1;
 }
Signature · 1 line
gitgitgadget
#2 lore raw

PATCH 2/3 mingw: replace MSVCRT's fstat() with a Win32-based implementation

In reply to Johannes Schindelin via GitGitGadget

From: Karsten Blees <[email protected]>

fstat() is the only stat-related CRT function for which we don't have a full replacement yet (and thus the only reason to stick with MSVCRT's 'struct stat' definition).

Fully implement fstat(), in preparation of implementing a POSIX 2013 compatible 'struct stat' with nanosecond-precision file times.

This allows us also to implement some clever code to handle pipes and character devices in our own way.

---
 compat/mingw.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)
compat/mingw.c +21 -10
index d2e7d86db..07fc0b79a 100644
@@ -771,20 +771,31 @@ int mingw_stat(const char *file_name, struct stat *buf)
 int mingw_fstat(int fd, struct stat *buf)
 {
 	HANDLE fh = (HANDLE)_get_osfhandle(fd);
+	DWORD avail, type = GetFileType(fh) & ~FILE_TYPE_REMOTE;
 
-	if (fh == INVALID_HANDLE_VALUE) {
-		errno = EBADF;
-		return -1;
-	}
-	/* direct non-file handles to MS's fstat() */
-	if (GetFileType(fh) != FILE_TYPE_DISK)
-		return _fstati64(fd, buf);
+	switch (type) {
+	case FILE_TYPE_DISK:
+		return get_file_info_by_handle(fh, buf);
 
-	if (!get_file_info_by_handle(fh, buf))
+	case FILE_TYPE_CHAR:
+	case FILE_TYPE_PIPE:
+		/* initialize stat fields */
+		memset(buf, 0, sizeof(*buf));
+		buf->st_nlink = 1;
+
+		if (type == FILE_TYPE_CHAR) {
+			buf->st_mode = _S_IFCHR;
+		} else {
+			buf->st_mode = _S_IFIFO;
+			if (PeekNamedPipe(fh, NULL, 0, NULL, &avail, NULL))
+				buf->st_size = avail;
+		}
 		return 0;
 
-	errno = EBADF;
-	return -1;
+	default:
+		errno = EBADF;
+		return -1;
+	}
 }
 
 static inline void time_t_to_filetime(time_t t, FILETIME *ft)
Signature · 1 line
gitgitgadget
#3 lore raw

Re: [PATCH 2/3] mingw: replace MSVCRT's fstat() with a Win32-based implementation

compat/mingw.c int mingw_stat(const char *file_name, struct stat *buf) · line 791

Quoted 13 lines · Karsten Blees via GitGitGadget
compat/mingw.c +12 -1
-	if (!get_file_info_by_handle(fh, buf))
+	case FILE_TYPE_CHAR:
+	case FILE_TYPE_PIPE:
+		/* initialize stat fields */
+		memset(buf, 0, sizeof(*buf));
+		buf->st_nlink = 1;
+
+		if (type == FILE_TYPE_CHAR) {
+			buf->st_mode = _S_IFCHR;
+		} else {
+			buf->st_mode = _S_IFIFO;
+			if (PeekNamedPipe(fh, NULL, 0, NULL, &avail, NULL))
+				buf->st_size = avail;

These lines strike me as a bit odd. As far as I'm aware, Unix systems don't return anything useful in this field when calling fstat on a pipe. Is there a reason we fill this in on Windows? If so, could the commit message explain what that is?

Signature · 2 lines
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
#4 lore raw

Re: [PATCH 2/3] mingw: replace MSVCRT's fstat() with a Win32-based implementation

Hi brian,

Quoted 11 lines · brian m. carlson
Quoted 10 lines · Karsten Blees via GitGitGadget
compat/mingw.c +9 -1
-	if (!get_file_info_by_handle(fh, buf))
+	case FILE_TYPE_CHAR:
+	case FILE_TYPE_PIPE:
+		/* initialize stat fields */
+		memset(buf, 0, sizeof(*buf));
+		buf->st_nlink = 1;
+
+		if (type == FILE_TYPE_CHAR) {
+			buf->st_mode = _S_IFCHR;
+		} else {
Quoted 8 lines

compat/mingw.c int mingw_stat(const char *file_name, struct stat *buf) · line 791

Quoted 3 lines
compat/mingw.c +3 -0
+			buf->st_mode = _S_IFIFO;
+			if (PeekNamedPipe(fh, NULL, 0, NULL, &avail, NULL))
+				buf->st_size = avail;

These lines strike me as a bit odd. As far as I'm aware, Unix systems don't return anything useful in this field when calling fstat on a pipe. Is there a reason we fill this in on Windows? If so, could the commit message explain what that is?

AFAICT the idea was to imitate MSVCRT's fstat() in these cases.

But a quick web search suggests that you are right:
https://bugzilla.redhat.com/show_bug.cgi?id=58768#c4 (I could not find any official documentation talking about fstat() and pipes, but I trust Alan to know their stuff).

Do note, please, that according to the issue described in that link, at least *some* glibc/Linux combinations behave in exactly the way this patch implements it.

At this point, I am wary of changing this, too, as the code in question has been in production (read: tested thoroughly) in the current form for *years*, and I am really loathe to introduce a bug where even Windows-specific code in compat/ might rely on this behavior. (And no, I do not trust our test suite to find all of those use cases.)

Ciao,
Dscho

#5 lore raw

Re: [PATCH 2/3] mingw: replace MSVCRT's fstat() with a Win32-based implementation

Quoted 8 lines · Johannes Schindelin

Hi brian,

Quoted 4 lines · brian m. carlson

These lines strike me as a bit odd. As far as I'm aware, Unix systems don't return anything useful in this field when calling fstat on a pipe. Is there a reason we fill this in on Windows? If so, could the commit message explain what that is?

Quoted 6 lines

AFAICT the idea was to imitate MSVCRT's fstat() in these cases.

But a quick web search suggests that you are right:
https://bugzilla.redhat.com/show_bug.cgi?id=58768#c4 (I could not find any official documentation talking about fstat() and pipes, but I trust Alan to know their stuff).

Yeah, that behavior is quite old. I'm surprised that Linux ever did that.

Quoted 9 lines

Do note, please, that according to the issue described in that link, at least *some* glibc/Linux combinations behave in exactly the way this patch implements it.

At this point, I am wary of changing this, too, as the code in question has been in production (read: tested thoroughly) in the current form for *years*, and I am really loathe to introduce a bug where even Windows-specific code in compat/ might rely on this behavior. (And no, I do not trust our test suite to find all of those use cases.)

I don't feel strongly either way. I feel confident the rest of Git doesn't use that field, so I don't see any downsides to keeping it other than the slight overhead of populating it. I just thought I'd ask in case there was something important I was missing.

Signature · 2 lines
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
#6 lore raw

Re: [PATCH 2/3] mingw: replace MSVCRT's fstat() with a Win32-based implementation

Quoted 7 lines · brian m. carlson

Yeah, that behavior is quite old. I'm surprised that Linux ever did that.
...
I don't feel strongly either way. I feel confident the rest of Git doesn't use that field, so I don't see any downsides to keeping it other than the slight overhead of populating it. I just thought I'd ask in case there was something important I was missing.

OK, I'd consider that this part of the review settled for taking the patch as-is. Let's mark the topic for merging to 'next' soonish in the what's cooking report.

Thanks.

#7 lore raw

PATCH 3/3 mingw: implement nanosecond-precision file times

In reply to Johannes Schindelin via GitGitGadget

From: Karsten Blees <[email protected]>

We no longer use any of MSVCRT's stat-functions, so there's no need to stick to a CRT-compatible 'struct stat' either.

Define and use our own POSIX-2013-compatible 'struct stat' with nanosecond- precision file times.

Note: This can cause performance issues when using Git variants with different file time resolutions, as the timestamps are stored in the Git
index: after updating the index with a Git variant that uses second-precision file times, a nanosecond-aware Git will think that pretty much every single file listed in the index is out of date.

---
 compat/mingw.c   | 18 ++++++++++--------
 compat/mingw.h   | 36 ++++++++++++++++++++++++++----------
 config.mak.uname |  2 --
 3 files changed, 36 insertions(+), 20 deletions(-)
compat/mingw.c +10 -8
index 07fc0b79a..26016d02e 100644
@@ -592,9 +592,11 @@ static inline long long filetime_to_hnsec(const FILETIME *ft)
 	return winTime - 116444736000000000LL;
 }
 
-static inline time_t filetime_to_time_t(const FILETIME *ft)
+static inline void filetime_to_timespec(const FILETIME *ft, struct timespec *ts)
 {
-	return (time_t)(filetime_to_hnsec(ft) / 10000000);
+	long long hnsec = filetime_to_hnsec(ft);
+	ts->tv_sec = (time_t)(hnsec / 10000000);
+	ts->tv_nsec = (hnsec % 10000000) * 100;
 }
 
 /**
@@ -653,9 +655,9 @@ static int do_lstat(int follow, const char *file_name, struct stat *buf)
 		buf->st_size = fdata.nFileSizeLow |
 			(((off_t)fdata.nFileSizeHigh)<<32);
 		buf->st_dev = buf->st_rdev = 0; /* not used by Git */
-		buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
-		buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
-		buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
+		filetime_to_timespec(&(fdata.ftLastAccessTime), &(buf->st_atim));
+		filetime_to_timespec(&(fdata.ftLastWriteTime), &(buf->st_mtim));
+		filetime_to_timespec(&(fdata.ftCreationTime), &(buf->st_ctim));
 		if (fdata.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) {
 			WIN32_FIND_DATAW findbuf;
 			HANDLE handle = FindFirstFileW(wfilename, &findbuf);
@@ -753,9 +755,9 @@ static int get_file_info_by_handle(HANDLE hnd, struct stat *buf)
 	buf->st_size = fdata.nFileSizeLow |
 		(((off_t)fdata.nFileSizeHigh)<<32);
 	buf->st_dev = buf->st_rdev = 0; /* not used by Git */
-	buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
-	buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
-	buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
+	filetime_to_timespec(&(fdata.ftLastAccessTime), &(buf->st_atim));
+	filetime_to_timespec(&(fdata.ftLastWriteTime), &(buf->st_mtim));
+	filetime_to_timespec(&(fdata.ftCreationTime), &(buf->st_ctim));
 	return 0;
 }
 
compat/mingw.h +26 -10
index 571019d0b..9419b27e1 100644
@@ -327,18 +327,41 @@ static inline int getrlimit(int resource, struct rlimit *rlp)
 }
 
 /*
- * Use mingw specific stat()/lstat()/fstat() implementations on Windows.
+ * Use mingw specific stat()/lstat()/fstat() implementations on Windows,
+ * including our own struct stat with 64 bit st_size and nanosecond-precision
+ * file times.
  */
 #ifndef __MINGW64_VERSION_MAJOR
 #define off_t off64_t
 #define lseek _lseeki64
+struct timespec {
+	time_t tv_sec;
+	long tv_nsec;
+};
 #endif
 
-/* use struct stat with 64 bit st_size */
+struct mingw_stat {
+    _dev_t st_dev;
+    _ino_t st_ino;
+    _mode_t st_mode;
+    short st_nlink;
+    short st_uid;
+    short st_gid;
+    _dev_t st_rdev;
+    off64_t st_size;
+    struct timespec st_atim;
+    struct timespec st_mtim;
+    struct timespec st_ctim;
+};
+
+#define st_atime st_atim.tv_sec
+#define st_mtime st_mtim.tv_sec
+#define st_ctime st_ctim.tv_sec
+
 #ifdef stat
 #undef stat
 #endif
-#define stat _stati64
+#define stat mingw_stat
 int mingw_lstat(const char *file_name, struct stat *buf);
 int mingw_stat(const char *file_name, struct stat *buf);
 int mingw_fstat(int fd, struct stat *buf);
@@ -351,13 +374,6 @@ int mingw_fstat(int fd, struct stat *buf);
 #endif
 #define lstat mingw_lstat
 
-#ifndef _stati64
-# define _stati64(x,y) mingw_stat(x,y)
-#elif defined (_USE_32BIT_TIME_T)
-# define _stat32i64(x,y) mingw_stat(x,y)
-#else
-# define _stat64(x,y) mingw_stat(x,y)
-#endif
 
 int mingw_utime(const char *file_name, const struct utimbuf *times);
 #define utime mingw_utime
config.mak.uname +0 -2
index 8acdeb71f..f179d7a1d 100644
@@ -370,7 +370,6 @@ ifeq ($(uname_S),Windows)
 	RUNTIME_PREFIX = YesPlease
 	HAVE_WPGMPTR = YesWeDo
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
-	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
 	MMAP_PREVENTS_DELETE = UnfortunatelyYes
 	# USE_NED_ALLOCATOR = YesPlease
@@ -518,7 +517,6 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	RUNTIME_PREFIX = YesPlease
 	HAVE_WPGMPTR = YesWeDo
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
-	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
 	MMAP_PREVENTS_DELETE = UnfortunatelyYes
 	USE_NED_ALLOCATOR = YesPlease
Signature · 1 line
gitgitgadget