[PATCH 0/3] Use nanosecond-precision file times on Windows

[PATCH 0/3] Use nanosecond-precision file times on Windows

[Johannes Schindelin via GitGitGadget]

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
-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 18caf2196..d2e7d86db 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -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;
 }
-- 
gitgitgadget

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

[Karsten Blees via GitGitGadget]

From: Karsten Blees <blees@dcon.de>

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.

Signed-off-by: Karsten Blees <blees@dcon.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index d2e7d86db..07fc0b79a 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -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)
-- 
gitgitgadget

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

[Karsten Blees via GitGitGadget]

From: Karsten Blees <blees@dcon.de>

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.

Signed-off-by: Karsten Blees <blees@dcon.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c   | 18 ++++++++++--------
 compat/mingw.h   | 36 ++++++++++++++++++++++++++----------
 config.mak.uname |  2 --
 3 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 07fc0b79a..26016d02e 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -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;
 }
 
diff --git a/compat/mingw.h b/compat/mingw.h
index 571019d0b..9419b27e1 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -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
diff --git a/config.mak.uname b/config.mak.uname
index 8acdeb71f..f179d7a1d 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -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
-- 
gitgitgadget

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 812 bytes --]

On Tue, Oct 23, 2018 at 03:23:21AM -0700, Karsten Blees via GitGitGadget wrote:
> -	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?
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

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

[Johannes Schindelin]

Hi brian,

On Wed, 24 Oct 2018, brian m. carlson wrote:

> On Tue, Oct 23, 2018 at 03:23:21AM -0700, Karsten Blees via GitGitGadget wrote:
> > -	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?

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

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1667 bytes --]

On Wed, Oct 24, 2018 at 09:37:43AM +0200, Johannes Schindelin wrote:
> Hi brian,
> 
> On Wed, 24 Oct 2018, brian m. carlson wrote:
> > 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).

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

> 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.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

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

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> 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.