git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

From
Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Date
Jun 27, 2013, 22:58 UTC
Message-ID
<51CCC397.3060705@ramsay1.demon.co.uk>
In-Reply-To
<20130626224336.GA22486@sigill.intra.peff.net>
Jeff King wrote:
Show 57 quoted lines
> On Wed, Jun 26, 2013 at 06:35:52PM -0400, Jeff King wrote:
> 
>> I am curious how often Cygwin gives us the false positive. If it is
>> every time, then the check is not doing much good at all. Is it possible
>> for you to instrument stat_validity_check to report how often it does or
>> does not do anything useful?
> 
> Maybe like this:
> 
> diff --git a/read-cache.c b/read-cache.c
> index d5201f9..19dcb69 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1958,6 +1958,14 @@ void stat_validity_clear(struct stat_validity *sv)
>  	sv->sd = NULL;
>  }
>  
> +static int unchanged;
> +static int attempts;
> +static void print_stats(void)
> +{
> +	fprintf(stderr, "stat data was unchanged %d/%d\n",
> +		unchanged, attempts);
> +}
> +
>  int stat_validity_check(struct stat_validity *sv, const char *path)
>  {
>  	struct stat st;
> @@ -1966,7 +1974,16 @@ int stat_validity_check(struct stat_validity *sv, const char *path)
>  		return sv->sd == NULL;
>  	if (!sv->sd)
>  		return 0;
> -	return S_ISREG(st.st_mode) && !match_stat_data(sv->sd, &st);
> +	if (!S_ISREG(st.st_mode))
> +		return 0;
> +	if (!attempts++)
> +		atexit(print_stats);
> +	if (!match_stat_data(sv->sd, &st)) {
> +		unchanged++;
> +		return 1;
> +	}
> +	else
> +		return 0;
>  }
>  
>  void stat_validity_update(struct stat_validity *sv, int fd)
> 
> Running "t3211 -v", I see things like:
> 
>   stat data was unchanged 3/3
>   stat data was unchanged 20/20
>   stat data was unchanged 2/2
>   Deleted branch yadda (was d1ff1c9).
>   stat data was unchanged 8/8
>   ok 8 - peeled refs survive deletion of packed ref
> 
> I am curious if you will see 0/20 or 19/20 there on Cygwin.
Ah, no ... the problem is not as subtle as this! :-D

I'm sorry if I failed to mention that I already had a patch to solve the problem, but that I *don't* want to submit it, since it is ugly and just serves to spread the madness! ;-)

This is why I tried the "cygwin: Remove the Win32 l/stat() functions" patch first; I think this is the correct approach to fixing this problem (and similar *future* problems).

However, since that is no longer an option, on performance grounds, I have other ideas I want to try. (see later email)

The "schizophrenic stat" functions have caused many subtle problems, but this is not one of them; a brick to the head would be more subtle. The problem is caused by the "stat validity" code using both fstat() and stat() on the same file. Note that there is no Win32 implementation of the fstat() function.

So, a solution to the problem would be to provide just such an implementation. The patch to do so is given below, just for illustration purposes.

This fixes the failures to t3210, t3211 and t5500. However, I have not run the full test suite. This may cause some further *subtle* problems (just grep for fstat and consider what may go wrong; I have not done that), especially when you consider that cygwin has the UNRELIABLE_FSTAT build variable set.

HTH

ATB, Ramsay Jones

-- >8 --
Subject: [PATCH] cygwin: Add an Win32 fstat implementation
Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
 compat/cygwin.c | 41 +++++++++++++++++++++++++++++++++++++++++
 compat/cygwin.h |  3 +++
 2 files changed, 44 insertions(+)
diff --git a/compat/cygwin.c b/compat/cygwin.c
index 91ce5d4..d59c9fb 100644
--- a/compat/cygwin.c
+++ b/compat/cygwin.c
@@ -4,6 +4,7 @@
 #include <sys/errno.h>
 #include "win32.h"
 #include "../git-compat-util.h"
+#include <io.h>
 #include "../cache.h" /* to read configuration */
 
 /*
@@ -100,6 +101,38 @@ static int cygwin_stat(const char *path, struct stat *buf)
 	return do_stat(path, buf, stat);
 }
 
+static int cygwin_fstat(int fd, struct stat *buf)
+{
+	HANDLE fh = (HANDLE)get_osfhandle(fd);
+	BY_HANDLE_FILE_INFORMATION fdata;
+
+	if (fh == INVALID_HANDLE_VALUE) {
+		errno = EBADF;
+		return -1;
+	}
+	if (GetFileType(fh) != FILE_TYPE_DISK)
+		return (fstat)(fd, buf);
+	if (GetFileInformationByHandle(fh, &fdata)) {
+		buf->st_dev = buf->st_rdev = 0; /* not used by Git */
+		buf->st_ino = 0;
+		buf->st_mode = file_attr_to_st_mode(fdata.dwFileAttributes);
+		buf->st_nlink = 1;
+		buf->st_uid = buf->st_gid = 0;
+#ifdef __CYGWIN_USE_BIG_TYPES__
+		buf->st_size = ((_off64_t)fdata.nFileSizeHigh << 32) +
+			fdata.nFileSizeLow;
+#else
+		buf->st_size = (off_t)fdata.nFileSizeLow;
+#endif
+		buf->st_blocks = size_to_blocks(buf->st_size);
+		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;
+	}
+	errno = EBADF;
+	return -1;
+}
 
 /*
  * At start up, we are trying to determine whether Win32 API or cygwin stat
@@ -133,9 +166,11 @@ static int init_stat(void)
 		if (!core_filemode && native_stat) {
 			cygwin_stat_fn = cygwin_stat;
 			cygwin_lstat_fn = cygwin_lstat;
+			cygwin_fstat_fn = cygwin_fstat;
 		} else {
 			cygwin_stat_fn = stat;
 			cygwin_lstat_fn = lstat;
+			cygwin_fstat_fn = fstat;
 		}
 		return 0;
 	}
@@ -152,6 +187,12 @@ static int cygwin_lstat_stub(const char *file_name, struct stat *buf)
 	return (init_stat() ? lstat : *cygwin_lstat_fn)(file_name, buf);
 }
 
+static int cygwin_fstat_stub(int fd, struct stat *buf)
+{
+	return (init_stat() ? fstat : *cygwin_fstat_fn)(fd, buf);
+}
+
 stat_fn_t cygwin_stat_fn = cygwin_stat_stub;
 stat_fn_t cygwin_lstat_fn = cygwin_lstat_stub;
+fstat_fn_t cygwin_fstat_fn = cygwin_fstat_stub;
 
diff --git a/compat/cygwin.h b/compat/cygwin.h
index c04965a..f3ca3d2 100644
--- a/compat/cygwin.h
+++ b/compat/cygwin.h
@@ -2,8 +2,10 @@
 #include <sys/stat.h>
 
 typedef int (*stat_fn_t)(const char*, struct stat*);
+typedef int (*fstat_fn_t)(int, struct stat*);
 extern stat_fn_t cygwin_stat_fn;
 extern stat_fn_t cygwin_lstat_fn;
+extern fstat_fn_t cygwin_fstat_fn;
 int cygwin_get_st_mode_bits(const char *path, int *mode);
 
 #define get_st_mode_bits(p,m) cygwin_get_st_mode_bits((p),(m))
@@ -11,4 +13,5 @@ int cygwin_get_st_mode_bits(const char *path, int *mode);
 /* cygwin.c needs the original lstat() */
 #define stat(path, buf) (*cygwin_stat_fn)(path, buf)
 #define lstat(path, buf) (*cygwin_lstat_fn)(path, buf)
+#define fstat(fd, buf) (*cygwin_fstat_fn)(fd, buf)
 #endif

-- 
1.8.3
Previous: Jeff KingNext: Mark Levedahl
Message 13 of 24 in “Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions”
  1. Johannes SixtJun 25, 2013
  2. Torsten BögershausenJun 25, 2013
  3. Junio C HamanoJun 25, 2013
  4. Torsten BögershausenJun 26, 2013
  5. Ramsay JonesJun 26, 2013
  6. Torsten BögershausenJun 27, 2013
  7. Ramsay JonesJun 27, 2013
  8. Mark LevedahlJun 27, 2013
  9. Ramsay JonesJun 26, 2013
  10. Ramsay JonesJun 26, 2013
  11. Jeff KingJun 26, 2013
  12. Jeff KingJun 26, 2013
  13. Ramsay JonesJun 27, 2013
  14. Mark LevedahlJun 28, 2013
  15. Michael HaggertyJun 27, 2013
  16. Jeff KingJun 27, 2013
  17. Junio C HamanoJun 27, 2013
  18. Ramsay JonesJun 27, 2013
  19. Ramsay JonesJun 30, 2013
  20. Junio C HamanoJun 30, 2013
  21. Ramsay JonesJul 4, 2013
  22. Torsten BögershausenJul 9, 2013
  23. Ramsay JonesJul 11, 2013
  24. Ramsay JonesJun 27, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.