[PATCH] add GIT_FAST_STAT mode for Cygwin

[PATCH] add GIT_FAST_STAT mode for Cygwin

[Dmitry Potapov]

This patch introduces the GIT_FAST_STAT environment variable. If this
variable is not set then Git will work as before. However, if it is set
then the Cygwin version of Git will try to use a Win32 API function if
it is possible to speed up stat/lstat.

This fast mode works only for relative paths. It is assumed that the
whole repository is located inside one directory without using Cygwin
mount to bind external paths inside of the current tree.

Symbol links are supported by falling back on the cygwin version of
these functions.

A very superficial testing shows 'git status' in the fast mode works more
than twice faster than in the normal mode, i.e. with about the same speed
as the native MinGW version.

Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>
---

After getting used to how strikingly fast Git is on Linux, using Git on
Windows (even only for a few hours) was not so pleasant. So, here is
this patch.

For those who wonder why I don't know use MinGW version of Git, the
answer is simple -- I have Cygwin install and I happy with it, while
the MinGW version comes with MSYS, so it cannot be used in Cygwin.

 Makefile          |    4 ++
 compat/cygwin.c   |  133 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 compat/cygwin.h   |    9 ++++
 git-compat-util.h |    1 +
 4 files changed, 147 insertions(+), 0 deletions(-)
 create mode 100644 compat/cygwin.c
 create mode 100644 compat/cygwin.h

diff --git a/Makefile b/Makefile
index 3c0664a..0708390 100644
--- a/Makefile
+++ b/Makefile
@@ -347,6 +347,7 @@ LIB_H += cache.h
 LIB_H += cache-tree.h
 LIB_H += commit.h
 LIB_H += compat/mingw.h
+LIB_H += compat/cygwin.h
 LIB_H += csum-file.h
 LIB_H += decorate.h
 LIB_H += delta.h
@@ -747,6 +748,9 @@ ifeq ($(uname_S),HP-UX)
 	NO_SYS_SELECT_H = YesPlease
 	SNPRINTF_RETURNS_BOGUS = YesPlease
 endif
+ifneq (,$(findstring CYGWIN,$(uname_S)))
+	COMPAT_OBJS += compat/cygwin.o
+endif
 ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_MMAP = YesPlease
 	NO_PREAD = YesPlease
diff --git a/compat/cygwin.c b/compat/cygwin.c
new file mode 100644
index 0000000..0b63d2f
--- /dev/null
+++ b/compat/cygwin.c
@@ -0,0 +1,133 @@
+#define WIN32_LEAN_AND_MEAN
+#include "../git-compat-util.h"
+#include <windows.h>
+
+static inline void filetime_to_timespec(const FILETIME *ft, struct timespec *ts)
+{
+	long long winTime = ((long long)ft->dwHighDateTime << 32) + ft->dwLowDateTime;
+	winTime -= 116444736000000000LL; /* Windows to Unix Epoch conversion */
+	ts->tv_sec = (time_t)(winTime/10000000); /* 100-nanosecond interval to seconds */
+	ts->tv_nsec = (long)(winTime - ts->tv_sec) * 100; /* nanoseconds */
+}
+
+#define size_to_blocks(s) (((s)+511)/512)
+
+/* do_stat is a common implementation for cygwin_lstat and cygwin_stat.
+ *
+ * To simplify its logic, in the case of cygwin symlinks, this implementation
+ * falls back to the cygwin version of stat/lstat, which is provided as the
+ * last argument.
+ */
+static int do_stat(const char *file_name, struct stat *buf, stat_fn_t cygstat)
+{
+	WIN32_FILE_ATTRIBUTE_DATA fdata;
+
+	if (file_name[0] == '/')
+		return cygstat (file_name, buf);
+
+	if (GetFileAttributesExA(file_name, GetFileExInfoStandard, &fdata)) {
+		int fMode = S_IREAD;
+		/*
+		 * If the system attribute is set and it is not a directory then
+		 * it could be a symbol link created in the nowinsymlinks mode.
+		 * Normally, Cygwin works in the winsymlinks mode, so this situation
+		 * is very unlikely. For the sake of simplicity of our code, let's
+		 * Cygwin to handle it.
+		 */
+		if ((fdata.dwFileAttributes & FILE_ATTRIBUTE_SYSTEM) &&
+		    !(fdata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY))
+			return cygstat (file_name, buf);
+
+		if (fdata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
+			fMode |= S_IFDIR;
+		else
+			fMode |= S_IFREG;
+		if (!(fdata.dwFileAttributes & FILE_ATTRIBUTE_READONLY))
+			fMode |= S_IWRITE;
+
+		/* st_dev, st_rdev are not used by Git */
+		buf->st_dev = buf->st_rdev = 0;
+		/* it is difficult to obtain the inode number on Windows,
+		 * so let's set it to zero as MinGW Git does. */
+		buf->st_ino = 0;
+		buf->st_mode = fMode;
+		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);
+		errno = 0;
+		return 0;
+	}
+
+	switch (GetLastError()) {
+	case ERROR_ACCESS_DENIED:
+	case ERROR_SHARING_VIOLATION:
+	case ERROR_LOCK_VIOLATION:
+	case ERROR_SHARING_BUFFER_EXCEEDED:
+		errno = EACCES;
+		break;
+	case ERROR_BUFFER_OVERFLOW:
+		errno = ENAMETOOLONG;
+		break;
+	case ERROR_NOT_ENOUGH_MEMORY:
+		errno = ENOMEM;
+		break;
+	default:
+		/* In the winsymlinks mode (which is the default), Cygwin
+		 * emulates symbol links using Windows shortcut files. These
+		 * files are formed by adding .lnk extension. So, if we have
+		 * not found the specified file name, it could be that it is
+		 * a symbol link. Let's Cygwin to deal with that.
+		 */
+		return cygstat (file_name, buf);
+	}
+	return -1;
+}
+
+/* We provide our own lstat/stat functions, since the provided Cygwin versions
+ * of these functions are too slow. These stat functions are tailored for Git's
+ * usage, and therefore they are not meant to be complete and correct emulation
+ * of lstat/stat functionality.
+ */
+static int cygwin_lstat(const char *path, struct stat *buf)
+{
+	return do_stat(path, buf, lstat);
+}
+
+static int cygwin_stat(const char *path, struct stat *buf)
+{
+	return do_stat(path, buf, stat);
+}
+
+/*
+ * This are startup stubs, which choose what implementation of lstat/stat
+ * should be used. If GIT_FAST_STAT is not set then the standard functions
+ * included in the cygwin library are used. If it is set then our fast and
+ * dirty implementation is invoked, which should be 2-3 times faster than
+ * cygwin functions.
+ */
+static int cygwin_stat_choice(const char *file_name, struct stat *buf)
+{
+	cygwin_stat_fn = getenv("GIT_FAST_STAT") ?
+		cygwin_stat : stat;
+	return (*cygwin_stat_fn)(file_name, buf);
+}
+
+static int cygwin_lstat_choice(const char *file_name, struct stat *buf)
+{
+	cygwin_lstat_fn = getenv("GIT_FAST_STAT") ?
+		cygwin_lstat : lstat;
+	return (*cygwin_lstat_fn)(file_name, buf);
+}
+
+stat_fn_t cygwin_stat_fn = cygwin_stat_choice;
+stat_fn_t cygwin_lstat_fn = cygwin_lstat_choice;
+
diff --git a/compat/cygwin.h b/compat/cygwin.h
new file mode 100644
index 0000000..a3229f5
--- /dev/null
+++ b/compat/cygwin.h
@@ -0,0 +1,9 @@
+#include <sys/types.h>
+#include <sys/stat.h>
+
+typedef int (*stat_fn_t)(const char*, struct stat*);
+extern stat_fn_t cygwin_stat_fn;
+extern stat_fn_t cygwin_lstat_fn;
+
+#define stat(path, buf) (*cygwin_stat_fn)(path, buf)
+#define lstat(path, buf) (*cygwin_lstat_fn)(path, buf)
diff --git a/git-compat-util.h b/git-compat-util.h
index db2836f..cd9752c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -85,6 +85,7 @@
 #undef _XOPEN_SOURCE
 #include <grp.h>
 #define _XOPEN_SOURCE 600
+#include "compat/cygwin.h"
 #else
 #undef _ALL_SOURCE /* AIX 5.3L defines a struct list with _ALL_SOURCE. */
 #include <grp.h>
-- 
1.6.0

Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

[Alex Riesen]

2008/9/23 Dmitry Potapov <dpotapov@gmail.com>:
> This patch introduces the GIT_FAST_STAT environment variable. If this
> variable is not set then Git will work as before. However, if it is set
> then the Cygwin version of Git will try to use a Win32 API function if
> it is possible to speed up stat/lstat.
>
> This fast mode works only for relative paths. It is assumed that the
> whole repository is located inside one directory without using Cygwin
> mount to bind external paths inside of the current tree.

Why runtime conditional? Why conditional at all? Why not fallback
to cygwin's slow stat on absolute pathnames like you do for symlinks?

> +/*
> + * This are startup stubs, which choose what implementation of lstat/stat

why do you need two of them? Isn't one not enough?

> +stat_fn_t cygwin_stat_fn = cygwin_stat_choice;
> +stat_fn_t cygwin_lstat_fn = cygwin_lstat_choice;
...
> +typedef int (*stat_fn_t)(const char*, struct stat*);
> +extern stat_fn_t cygwin_stat_fn;
> +extern stat_fn_t cygwin_lstat_fn;

    extern int (*cygwin_stat_fn)(const char *, struct stat *);

Is shorter, easier to read and easier to understand (for a C person).
You don't even use the type anywhere else, it is just for the declaration sake!

Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

[Shawn O. Pearce]

Dmitry Potapov <dpotapov@gmail.com> wrote:
> This patch introduces the GIT_FAST_STAT environment variable. If this
> variable is not set then Git will work as before. However, if it is set
> then the Cygwin version of Git will try to use a Win32 API function if
> it is possible to speed up stat/lstat.
> 
> This fast mode works only for relative paths. It is assumed that the
> whole repository is located inside one directory without using Cygwin
> mount to bind external paths inside of the current tree.
...
> +/*
> + * This are startup stubs, which choose what implementation of lstat/stat
> + * should be used. If GIT_FAST_STAT is not set then the standard functions
> + * included in the cygwin library are used. If it is set then our fast and
> + * dirty implementation is invoked, which should be 2-3 times faster than
> + * cygwin functions.
> + */
> +static int cygwin_stat_choice(const char *file_name, struct stat *buf)
> +{
> +	cygwin_stat_fn = getenv("GIT_FAST_STAT") ?
> +		cygwin_stat : stat;
> +	return (*cygwin_stat_fn)(file_name, buf);
> +}
> +
> +static int cygwin_lstat_choice(const char *file_name, struct stat *buf)
> +{
> +	cygwin_lstat_fn = getenv("GIT_FAST_STAT") ?
> +		cygwin_lstat : lstat;
> +	return (*cygwin_lstat_fn)(file_name, buf);
> +}

I wonder, should this be controlled by an environment variable?

Given your description of the feature it seems to be more a property
of the specific repository, as it is based upon where the repository
lives within the Cygwin namespace.  Should this be controlled instead
by say a "core.cygwinnativestat = true" configuration property?

-- 
Shawn.

Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

[Dmitry Potapov]

On Tue, Sep 23, 2008 at 04:37:14PM +0200, Alex Riesen wrote:
> 2008/9/23 Dmitry Potapov <dpotapov@gmail.com>:
> >
> > This fast mode works only for relative paths. It is assumed that the
> > whole repository is located inside one directory without using Cygwin
> > mount to bind external paths inside of the current tree.
> 
> Why runtime conditional? Why conditional at all?

I thought that in rather unusual circumstances (such as using Cygwin
mount to connect separately directories in one tree), this fast version
may not work. So, I made it conditional. It is runtime conditional,
because most users do not build Git themselves but install a ready
Cygwin package.

> Why not fallback
> to cygwin's slow stat on absolute pathnames like you do for symlinks?

Of course, I do:

+       if (file_name[0] == '/')
+               return cygstat (file_name, buf);

Sorry, if it was not clear from my above comment.

> 
> > +/*
> > + * This are startup stubs, which choose what implementation of lstat/stat
> 
> why do you need two of them? Isn't one not enough?

I did not want to give people reasons to say that I broke lstat :)
You can opt for the standard Cygwin version of it if for some reason,
this new function does not work. Now, I know only one case -- it is
when you use Cygwin mount inside of Git repo. Yet, I don't know enough
about Cygwin to be sure that there is no other cases. So, I just wanted
to be extra careful and not to break anything.

> 
> > +stat_fn_t cygwin_stat_fn = cygwin_stat_choice;
> > +stat_fn_t cygwin_lstat_fn = cygwin_lstat_choice;
> ...
> > +typedef int (*stat_fn_t)(const char*, struct stat*);
> > +extern stat_fn_t cygwin_stat_fn;
> > +extern stat_fn_t cygwin_lstat_fn;
> 
>     extern int (*cygwin_stat_fn)(const char *, struct stat *);
> 
> Is shorter, easier to read and easier to understand (for a C person).
> You don't even use the type anywhere else, it is just for the declaration sake!

I use it in description of a parameter of another function:

static int do_stat(const char *file_name, struct stat *buf, stat_fn_t cygstat)

Of course, you can avoid it here too, but the declaration will become
somewhat longer:

static int do_stat(const char *file_name, struct stat *buf,
       int (*cygstat)(const char *, struct stat *));

so I am not sure that removing stat_fn_t improves readability, but if
there are other people who think so, I will correct that.


Dmitry

Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

[Dmitry Potapov]

On Tue, Sep 23, 2008 at 08:31:48AM -0700, Shawn O. Pearce wrote:
> 
> I wonder, should this be controlled by an environment variable?
> 
> Given your description of the feature it seems to be more a property
> of the specific repository, as it is based upon where the repository
> lives within the Cygwin namespace.  Should this be controlled instead
> by say a "core.cygwinnativestat = true" configuration property?

I am not sure that you will find many people who will want to set this
option per repository, yet Git has the configuration file, and I agree
it is better to place it there.

However, this option is Cygwin specific, so I am not sure where it
should be read. Should I place it in git_default_core_config like
this:

#ifdef __CYGWIN__
	if (!strcmp(var, "core.cygwinnativestat")) {
		cygwin_native_stat = git_config_bool(var, value);
		return 0;
	}
#endif

So far, we have not had any system specific options here. So, perhaps,
it is better to leave git_default_core_config alone and just replace
get_env(GIT_FAST_STAT) with git_config_bool() in the cygwin specific
code.


Dmitry

Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

[Jakub Narebski]

Dmitry Potapov wrote:
> On Tue, Sep 23, 2008 at 04:37:14PM +0200, Alex Riesen wrote:
>> 2008/9/23 Dmitry Potapov <dpotapov@gmail.com>:

>>> +stat_fn_t cygwin_stat_fn = cygwin_stat_choice;
>>> +stat_fn_t cygwin_lstat_fn = cygwin_lstat_choice;
>> ...
>>> +typedef int (*stat_fn_t)(const char*, struct stat*);
>>> +extern stat_fn_t cygwin_stat_fn;
>>> +extern stat_fn_t cygwin_lstat_fn;
>> 
>>     extern int (*cygwin_stat_fn)(const char *, struct stat *);
>> 
>> Is shorter, easier to read and easier to understand (for a C person).
>> You don't even use the type anywhere else, it is just for the declaration sake!
> 
> I use it in description of a parameter of another function:
> 
> static int do_stat(const char *file_name, struct stat *buf, stat_fn_t cygstat)
> 
> Of course, you can avoid it here too, but the declaration will become
> somewhat longer:
> 
> static int do_stat(const char *file_name, struct stat *buf,
>        int (*cygstat)(const char *, struct stat *));
> 
> so I am not sure that removing stat_fn_t improves readability, but if
> there are other people who think so, I will correct that.

I think that using typedef here definitly improves readibility.
You don't have to carefully analyse if you can pass cygwin_stat_fn
to do_stat function or not.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

[Johannes Sixt]

On Dienstag, 23. September 2008, Dmitry Potapov wrote:
> +static inline void filetime_to_timespec(const FILETIME *ft, struct timespec *ts)
> +{ 
> +	long long winTime = ((long long)ft->dwHighDateTime << 32) + ft->dwLowDateTime;
> +	winTime -= 116444736000000000LL; /* Windows to Unix  Epoch conversion */
> +	ts->tv_sec = (time_t)(winTime/10000000); /* 100-nanosecond interval to seconds */
> +	ts->tv_nsec = (long)(winTime - ts->tv_sec) * 100; /* nanoseconds */ +}

+	ts->tv_nsec = (long)(winTime - ts->tv_sec*10000000LL) * 100;

> +static int do_stat(const char *file_name, struct stat *buf, stat_fn_t cygstat)
> +{ 
> +	WIN32_FILE_ATTRIBUTE_DATA fdata;
> +
> +	if (file_name[0] == '/')
> +		return cygstat (file_name, buf);

You should do this in the caller; it would make this function's
semantics much clearer.

> +
> +	if (GetFileAttributesExA(file_name, GetFileExInfoStandard, &fdata)) {
> +		int fMode = S_IREAD;
> +		/*
> +		 * If the system attribute is set and it is not a directory then
> +		 * it could be a symbol link created in the nowinsymlinks mode.
> +		 * Normally, Cygwin works in the winsymlinks mode, so this situation
> +		 * is very unlikely. For the sake of simplicity of our code, let's
> +		 * Cygwin to handle it.
> +		 */
> +		if ((fdata.dwFileAttributes & FILE_ATTRIBUTE_SYSTEM) &&
> +		    !(fdata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY))
> +			return cygstat (file_name, buf);
> +
> +		if (fdata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
> +			fMode |= S_IFDIR;
> +		else
> +			fMode |= S_IFREG;
> +		if (!(fdata.dwFileAttributes & FILE_ATTRIBUTE_READONLY))
> +			fMode |= S_IWRITE;
> +
> +		/* st_dev, st_rdev are not used by Git */
> +		buf->st_dev = buf->st_rdev = 0;
> +		/* it is difficult to obtain the inode number on Windows,
> +		 * so let's set it to zero as MinGW Git does. */
> +		buf->st_ino = 0;
> +		buf->st_mode = fMode;
> +		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);
> +		errno = 0;
> +		return 0;
> +	}
> +
> +	switch (GetLastError()) {
> +	case ERROR_ACCESS_DENIED:
> +	case ERROR_SHARING_VIOLATION:
> +	case ERROR_LOCK_VIOLATION:
> +	case ERROR_SHARING_BUFFER_EXCEEDED:
> +		errno = EACCES;
> +		break;
> +	case ERROR_BUFFER_OVERFLOW:
> +		errno = ENAMETOOLONG;
> +		break;
> +	case ERROR_NOT_ENOUGH_MEMORY:
> +		errno = ENOMEM;
> +		break;
> +	default:
> +		/* In the winsymlinks mode (which is the default), Cygwin
> +		 * emulates symbol links using Windows shortcut files. These
> +		 * files are formed by adding .lnk extension. So, if we have
> +		 * not found the specified file name, it could be that it is
> +		 * a symbol link. Let's Cygwin to deal with that.
> +		 */
> +		return cygstat (file_name, buf);
> +	}
> +	return -1;

You do duplicate a lot of code here. Any chances to factor out the
common parts? Start with platform specific function
filetime_to_stat_time() that is your filetime_to_timespec() on Cygwin,
but filetime_to_time_t() (which needs modification) on MinGW.

-- Hannes

Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

[Shawn O. Pearce]

Dmitry Potapov <dpotapov@gmail.com> wrote:
> On Tue, Sep 23, 2008 at 08:31:48AM -0700, Shawn O. Pearce wrote:
> > 
> > I wonder, should this be controlled by an environment variable?
> > 
> > Given your description of the feature it seems to be more a property
> > of the specific repository, as it is based upon where the repository
> > lives within the Cygwin namespace.  Should this be controlled instead
> > by say a "core.cygwinnativestat = true" configuration property?
> 
> I am not sure that you will find many people who will want to set this
> option per repository, yet Git has the configuration file, and I agree
> it is better to place it there.

If you want it globally you can do:

  git config --global core.cygwinnativestat true

and then disable it on a per-repository basis if you and a specific
repository which has this inner mount problem:

  git config core.cygwinnativestat false

Which is a lot more powerful than an environment variable.
 
> However, this option is Cygwin specific, so I am not sure where it
> should be read. Should I place it in git_default_core_config like
> this:
> 
> #ifdef __CYGWIN__
> 	if (!strcmp(var, "core.cygwinnativestat")) {
> 		cygwin_native_stat = git_config_bool(var, value);
> 		return 0;
> 	}
> #endif

I would have the two initial stat functions swap themselves out with
the default Cygin stat implementations, run a parse over the config
to load that one bool, then install the proper implementations based
upon its value.  Hence all Cygwin code is kept inside of the Cygwin
compat code, and no #ifdef is necessary

Of course that config file parse can only happen after the repository
has been entered, which means you need to somehow rely on the real
Cygwin stat functions until setup_git_directory() has completed,
and then on the next stat call (re)parse the config and swap the
implementation.

-- 
Shawn.

Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

[Dmitry Potapov]

On Tue, Sep 23, 2008 at 09:03:08PM +0200, Johannes Sixt wrote:
> On Dienstag, 23. September 2008, Dmitry Potapov wrote:
> > +static inline void filetime_to_timespec(const FILETIME *ft, struct timespec *ts)
> > +{ 
> > +	long long winTime = ((long long)ft->dwHighDateTime << 32) + ft->dwLowDateTime;
> > +	winTime -= 116444736000000000LL; /* Windows to Unix  Epoch conversion */
> > +	ts->tv_sec = (time_t)(winTime/10000000); /* 100-nanosecond interval to seconds */
> > +	ts->tv_nsec = (long)(winTime - ts->tv_sec) * 100; /* nanoseconds */ +}
> 
> +	ts->tv_nsec = (long)(winTime - ts->tv_sec*10000000LL) * 100;

Thanks.... What was I thought about when wrote this....

> 
> > +static int do_stat(const char *file_name, struct stat *buf, stat_fn_t cygstat)
> > +{ 
> > +	WIN32_FILE_ATTRIBUTE_DATA fdata;
> > +
> > +	if (file_name[0] == '/')
> > +		return cygstat (file_name, buf);
> 
> You should do this in the caller; it would make this function's
> semantics much clearer.

IMHO, the semantic of this function is clear: do_stat performs stat/lstat
using Windows API with falling back on Cygwin implementation in those
rare cases that it cannot handle correctly. Absolute path is just one of
those cases. So, I am not sure what you win by moving this two lines out.


> > +	if (GetFileAttributesExA(file_name, GetFileExInfoStandard, &fdata)) {
> > +		int fMode = S_IREAD;
> > +		/*
> > +		 * If the system attribute is set and it is not a directory then
> > +		 * it could be a symbol link created in the nowinsymlinks mode.
> > +		 * Normally, Cygwin works in the winsymlinks mode, so this situation
> > +		 * is very unlikely. For the sake of simplicity of our code, let's
> > +		 * Cygwin to handle it.
> > +		 */
> > +		if ((fdata.dwFileAttributes & FILE_ATTRIBUTE_SYSTEM) &&
> > +		    !(fdata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY))
> > +			return cygstat (file_name, buf);

This is specific to cygwin.

> > +
> > +		if (fdata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
> > +			fMode |= S_IFDIR;
> > +		else
> > +			fMode |= S_IFREG;
> > +		if (!(fdata.dwFileAttributes & FILE_ATTRIBUTE_READONLY))
> > +			fMode |= S_IWRITE;

These lines the same as mingw

> > +
> > +		/* st_dev, st_rdev are not used by Git */
> > +		buf->st_dev = buf->st_rdev = 0;

I set this to 0, while MinGW Git uses _getdrive(). I have no idea why
it does so. Git does not use this field, and if it did, adding the
_current_ drive number is useless at best when we are trying to
determine whether the file is changed or not.

> > +		/* it is difficult to obtain the inode number on Windows,
> > +		 * so let's set it to zero as MinGW Git does. */
> > +		buf->st_ino = 0;
> > +		buf->st_mode = fMode;
> > +		buf->st_nlink = 1;
> > +		buf->st_uid = buf->st_gid = 0;

This is the same as for MinGW

> > +#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);

This is different: using 64-bit version for st_size, st_blocks does not
exist in MinGW, and finally filetime_to_timespec instead of filetime_to_time_t,
as well as the name of fields is different (st_ctim instead of st_ctime, etc).

> > +		errno = 0;
> > +		return 0;
> > +	}
> > +
> > +	switch (GetLastError()) {
> > +	case ERROR_ACCESS_DENIED:
> > +	case ERROR_SHARING_VIOLATION:
> > +	case ERROR_LOCK_VIOLATION:
> > +	case ERROR_SHARING_BUFFER_EXCEEDED:
> > +		errno = EACCES;
> > +		break;
> > +	case ERROR_BUFFER_OVERFLOW:
> > +		errno = ENAMETOOLONG;
> > +		break;
> > +	case ERROR_NOT_ENOUGH_MEMORY:
> > +		errno = ENOMEM;
> > +		break;
> > +	default:
> > +		/* In the winsymlinks mode (which is the default), Cygwin
> > +		 * emulates symbol links using Windows shortcut files. These
> > +		 * files are formed by adding .lnk extension. So, if we have
> > +		 * not found the specified file name, it could be that it is
> > +		 * a symbol link. Let's Cygwin to deal with that.
> > +		 */
> > +		return cygstat (file_name, buf);
> > +	}

This is the same as in MinGW, except the default case, where MinGW
returns error immediately while this version calls the fallback
function.


> > +	return -1;
> 
> You do duplicate a lot of code here. Any chances to factor out the
> common parts?

I don't see much common code here. Initialization of 5 variables where
four of them are just constants? Perhaps, the biggest common part here
is conversion of dwFileAttributes to st_mode, but it is still 5 lines of
trivial code.

Dmitry

Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

[Dmitry Potapov]

On Tue, Sep 23, 2008 at 12:06:37PM -0700, Shawn O. Pearce wrote:
> 
> and then disable it on a per-repository basis if you and a specific
> repository which has this inner mount problem:
> 
>   git config core.cygwinnativestat false
> 
> Which is a lot more powerful than an environment variable.

I already said that I completely agree that is a good idea even I don't
know the real need for having per-repository configuration in practice.

>  
> > However, this option is Cygwin specific, so I am not sure where it
> > should be read. Should I place it in git_default_core_config like
> > this:
> > 
> > #ifdef __CYGWIN__
> > 	if (!strcmp(var, "core.cygwinnativestat")) {
> > 		cygwin_native_stat = git_config_bool(var, value);
> > 		return 0;
> > 	}
> > #endif
> 
> I would have the two initial stat functions swap themselves out with
> the default Cygin stat implementations, run a parse over the config
> to load that one bool, then install the proper implementations based
> upon its value.  Hence all Cygwin code is kept inside of the Cygwin
> compat code, and no #ifdef is necessary

Do I understand you correctly that you propose to add the code like
this in compat/cygwin.c:

static int native_stat;

static int git_cygwin_config(const char *var, const char *value, void
*cb)
{
	if (!strcmp(var, "core.cygwinnativestat"))
		cygwin_native_stat = git_config_bool(var, value);
	return 0;
}

static void init_stat(void)
{
	git_config(git_cygwin_config, NULL);
	cygwin_stat_fn = native_stat ? cygwin_stat : stat;
	cygwin_lstat_fn = native_stat ? cygwin_lstat : lstat;
}

static int cygwin_stat_choice(const char *file_name, struct stat *buf)
{
	init_stat();
	return (*cygwin_stat_fn)(file_name, buf);
}

static int cygwin_lstat_choice(const char *file_name, struct stat *buf)
{
	init_stat();
	return (*cygwin_lstat_fn)(file_name, buf);
}

Dmitry

Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

[Shawn O. Pearce]

Dmitry Potapov <dpotapov@gmail.com> wrote:
> Do I understand you correctly that you propose to add the code like
> this in compat/cygwin.c:

Yes.  But with minor changes (see below):
 
> static int native_stat;

static int native_stat = -1;

> static int git_cygwin_config(const char *var, const char *value, void
> *cb)
> {
> 	if (!strcmp(var, "core.cygwinnativestat"))
> 		cygwin_native_stat = git_config_bool(var, value);
> 	return 0;
> }
> 
> static void init_stat(void)
> {
> 	git_config(git_cygwin_config, NULL);
> 	cygwin_stat_fn = native_stat ? cygwin_stat : stat;
> 	cygwin_lstat_fn = native_stat ? cygwin_lstat : lstat;

if (native_stat < 0 && have_git_dir()) {
	native_stat = 0;
	git_config(git_cygwin_config, NULL);
	cygwin_stat_fn = native_stat ? cygwin_stat : stat;
	cygwin_lstat_fn = native_stat ? cygwin_lstat : lstat;
}

and then you have to define have_git_dir() inside environment.c as:

int have_git_dir(void)
{
	return !!git_dir;
}
 
> static int cygwin_stat_choice(const char *file_name, struct stat *buf)
> {
> 	init_stat();
> 	return (*cygwin_stat_fn)(file_name, buf);
> }
> 
> static int cygwin_lstat_choice(const char *file_name, struct stat *buf)
> {
> 	init_stat();
> 	return (*cygwin_lstat_fn)(file_name, buf);
> }

-- 
Shawn.

Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

[Johannes Sixt]

On Dienstag, 23. September 2008, Dmitry Potapov wrote:
> On Tue, Sep 23, 2008 at 09:03:08PM +0200, Johannes Sixt wrote:
> > On Dienstag, 23. September 2008, Dmitry Potapov wrote:
> > > +static int do_stat(const char *file_name, struct stat *buf, stat_fn_t
> > > cygstat) +{
> > > +	WIN32_FILE_ATTRIBUTE_DATA fdata;
> > > +
> > > +	if (file_name[0] == '/')
> > > +		return cygstat (file_name, buf);
> >
> > You should do this in the caller; it would make this function's
> > semantics much clearer.
>
> IMHO, the semantic of this function is clear: do_stat performs stat/lstat
> using Windows API with falling back on Cygwin implementation in those
> rare cases that it cannot handle correctly. Absolute path is just one of
> those cases. So, I am not sure what you win by moving this two lines out.

You copied the function from compat/mingw.c. There it has the meaning "Fill in 
struct stat using Win32 API" and nothing else. Here it has the meaning "Fill 
in struct stat using Win32 API if you can, and using cygstat() in certain 
exceptional cases". If you stayed with the original meaning, it would be 
slightly easier to factor out common code.

> > > +		/* st_dev, st_rdev are not used by Git */
> > > +		buf->st_dev = buf->st_rdev = 0;
>
> I set this to 0, while MinGW Git uses _getdrive(). I have no idea why
> it does so. 

Indeed. Calling _getdrive() is absolutely useless.

> > You do duplicate a lot of code here. Any chances to factor out the
> > common parts?
>
> I don't see much common code here. Initialization of 5 variables where
> four of them are just constants? Perhaps, the biggest common part here
> is conversion of dwFileAttributes to st_mode, but it is still 5 lines of
> trivial code.

Sigh. I gave a pointer how to unify the two functions (although I missed the 
fact that the member variables are named differently). I'd appreciate if you 
did not make it more difficult than necessary to factor out common code.

-- Hannes

Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

[Dmitry Potapov]

On Tue, Sep 23, 2008 at 10:41:42PM +0200, Johannes Sixt wrote:
> 
> You copied the function from compat/mingw.c. There it has the meaning "Fill in 
> struct stat using Win32 API" and nothing else. Here it has the meaning "Fill 
> in struct stat using Win32 API if you can, and using cygstat() in certain 
> exceptional cases". If you stayed with the original meaning, it would be 
> slightly easier to factor out common code.

do_stat() always fills in the structure, but it can do that fast using
Win32 API or fallback on cygstat() in exceptional cases. So, I don't
think I change its meaning much, its implementation certainly differs.

> > > You do duplicate a lot of code here. Any chances to factor out the
> > > common parts?
> >
> > I don't see much common code here. Initialization of 5 variables where
> > four of them are just constants? Perhaps, the biggest common part here
> > is conversion of dwFileAttributes to st_mode, but it is still 5 lines of
> > trivial code.
> 
> Sigh. I gave a pointer how to unify the two functions (although I missed the 
> fact that the member variables are named differently). I'd appreciate if you 
> did not make it more difficult than necessary to factor out common code.

Because the stat structure is different and handling exceptional
situation is different, I don't think we can have a single do_stat
function for Cygwin and MinGW. Yet, perhaps, it is possible to
move some code in common functions even if it is just a few lines.

The first candidate is win_attr_to_st_mode(), which converts
dwFileAttributes returned by GetFileAttributesExA to st_mode.
Another possible function is that obtains and converts Win32 error
code to errno value. These function can be placed into some common
header (for example, win32.h), which will included by both
implementations. Does it make sense?


Dmitry

Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

[Dmitry Potapov]

On Tue, Sep 23, 2008 at 01:17:39PM -0700, Shawn O. Pearce wrote:
> Dmitry Potapov <dpotapov@gmail.com> wrote:
> > 
> > static void init_stat(void)
> > {
> > 	git_config(git_cygwin_config, NULL);
> > 	cygwin_stat_fn = native_stat ? cygwin_stat : stat;
> > 	cygwin_lstat_fn = native_stat ? cygwin_lstat : lstat;
> 
> if (native_stat < 0 && have_git_dir()) {
> 	native_stat = 0;
> 	git_config(git_cygwin_config, NULL);
> 	cygwin_stat_fn = native_stat ? cygwin_stat : stat;
> 	cygwin_lstat_fn = native_stat ? cygwin_lstat : lstat;
> }

I am not sure that I understand what you are trying to do here.

First, in my implementation, init_stat was supposed to always set
cygwin_stat_fn() and cygwin_lstat_fn(), otherwise the code is going
to hit the NULL pointer call.

Second, the check of native_stat < 0 is absolutely useless, because once
we set cygwin_stat_fn and cygwin_lstat_fn, we are never going to call
init_stat() again.

Did you mean this:

	if (have_git_dir())
		git_config(git_cygwin_config, NULL);
	else
		native_stat = 0
	cygwin_stat_fn = native_stat ? cygwin_stat : stat;
	cygwin_lstat_fn = native_stat ? cygwin_lstat : lstat;

Or:

if (have_git_dir()) {
	git_config(git_cygwin_config, NULL);
	cygwin_stat_fn = native_stat ? cygwin_stat : stat;
	cygwin_lstat_fn = native_stat ? cygwin_lstat : lstat;
}

and

> > static int cygwin_stat_choice(const char *file_name, struct stat *buf)
> > {
> > 	init_stat();
> > 	return (*cygwin_stat_fn)(file_name, buf);

change the above line to:
	return (cygwin_stat_fn ? cygwin_stat_fn : stat) (file_name, buf);

so init_stat may be called a few times outside of git directory and then
use the default cygwin function, and once we enter to it then load the
configuration option and act accordingly.

Dmitry

Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

[Shawn O. Pearce]

Dmitry Potapov <dpotapov@gmail.com> wrote:
> I am not sure that I understand what you are trying to do here.
...
> Did you mean this:
...
> if (have_git_dir()) {
> 	git_config(git_cygwin_config, NULL);
> 	cygwin_stat_fn = native_stat ? cygwin_stat : stat;
> 	cygwin_lstat_fn = native_stat ? cygwin_lstat : lstat;
> }

Err, yes, something more like that.
 
> > > static int cygwin_stat_choice(const char *file_name, struct stat *buf)
> > > {
> > > 	init_stat();
> > > 	return (*cygwin_stat_fn)(file_name, buf);
> 
> change the above line to:
> 	return (cygwin_stat_fn ? cygwin_stat_fn : stat) (file_name, buf);

Right.
 
> so init_stat may be called a few times outside of git directory and then
> use the default cygwin function, and once we enter to it then load the
> configuration option and act accordingly.

Yup, exactly.  Sorry I wasn't being very clear earlier.

-- 
Shawn.

Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

[Alex Riesen]

2008/9/23 Dmitry Potapov <dpotapov@gmail.com>:
> On Tue, Sep 23, 2008 at 04:37:14PM +0200, Alex Riesen wrote:
>> 2008/9/23 Dmitry Potapov <dpotapov@gmail.com>:
>> >
>> > This fast mode works only for relative paths. It is assumed that the
>> > whole repository is located inside one directory without using Cygwin
>> > mount to bind external paths inside of the current tree.
>>
>> Why runtime conditional? Why conditional at all?
>
> I thought that in rather unusual circumstances (such as using Cygwin
> mount to connect separately directories in one tree), this fast version
> may not work. So, I made it conditional. It is runtime conditional,
> because most users do not build Git themselves but install a ready
> Cygwin package.

So? How about make the fast version _always_ work? We don't seem
to fallback to copy+unlink everytime the POSIX rename fails.

Besides it will remove your setup code, which looks bigger and provoked
more discussion than the real subject itself.

Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

[Dmitry Potapov]

On Wed, Sep 24, 2008 at 01:25:06PM +0200, Alex Riesen wrote:
> 2008/9/23 Dmitry Potapov <dpotapov@gmail.com>:
> >
> > I thought that in rather unusual circumstances (such as using Cygwin
> > mount to connect separately directories in one tree), this fast version
> > may not work. So, I made it conditional. It is runtime conditional,
> > because most users do not build Git themselves but install a ready
> > Cygwin package.
> 
> So? How about make the fast version _always_ work? We don't seem
> to fallback to copy+unlink everytime the POSIX rename fails.

I am not sure that I understand your analogue here. First, rename has
never meant to work as copy+unlink. Second, I don't fall back on some
other code when the implementation provided by Cygwin fails. I replace
the Cygwin implementation with a faster but a bit hackish version. Yes,
it works fine in almost all practical cases I aware of, but I cannot
guarantee identical behavior in _all_ cases.

Frankly, I don't have strong preference here neither for making this
fast version always work nor leave it conditional (perhaps, with the
default setting use-fast-version). So, whatever the majority decides
is fine with me.

> Besides it will remove your setup code, which looks bigger and provoked
> more discussion than the real subject itself.

I believe Shawn wanted it to be configurable on per-repository basis.
I have just finished re-writing the code in the way he suggested, so I
hope all objections with the setup code are resolved now. I will send
the new version a bit later, I did not have time to test it yet.


Dmitry

Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

[Alex Riesen]

2008/9/24 Dmitry Potapov <dpotapov@gmail.com>:
> it works fine in almost all practical cases I aware of, but I cannot
> guarantee identical behavior in _all_ cases.

Well, make it as good as MSys/Cygwin's and no one asks for your guarantee.

> Frankly, I don't have strong preference here neither for making this
> fast version always work nor leave it conditional (perhaps, with the
> default setting use-fast-version). So, whatever the majority decides
> is fine with me.

I'm voting for compile-time configuration then.

>> Besides it will remove your setup code, which looks bigger and provoked
>> more discussion than the real subject itself.
>
> I believe Shawn wanted it to be configurable on per-repository basis.

which, I believe, is pointless.

Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

[Shawn O. Pearce]

Alex Riesen <raa.lkml@gmail.com> wrote:
> 2008/9/24 Dmitry Potapov <dpotapov@gmail.com>:
> 
> > Frankly, I don't have strong preference here neither for making this
> > fast version always work nor leave it conditional (perhaps, with the
> > default setting use-fast-version). So, whatever the majority decides
> > is fine with me.
> 
> I'm voting for compile-time configuration then.

To be consistent with everything else, compile-time sounds like
what we should do, its how just about every other part of Git
is configured.

However Dmitry pointed out that he has cases where this faster
function doesn't work correctly, and it was path specific.  Some
areas of the filesystem work, others don't, on the same system.
A current example of a feature more like this is core.filemode.

A compile-time option makes the feature useful only to those users
who don't ever have a repository which has a mount contained within
the working directory.  My understanding of Dmitry's explanation
is he has such cases, which is why I was voting for a runtime
configuration.

A compile-time option means that Git will work fine for years, until
you put a mount in a working directory and *wham* it suddenly stops
working like it should, because of that compile-time optimization
you made long ago and forgot about.
 
> >> Besides it will remove your setup code, which looks bigger and provoked
> >> more discussion than the real subject itself.
> >
> > I believe Shawn wanted it to be configurable on per-repository basis.
> 
> which, I believe, is pointless.

See above.  I suggested configurable per-repository because
Dmitry seemed to be saying this feature only works in some of his
repositories and not others.  Controlling it by an environment
variable isn't very easy to use as you move between repositories
on the same system.

Maybe I should have leaned more towards compile-time earlier in
the discussion, but Dmitry lead off the patch though with a remark
about users just running the Cygwin package, without building
their own Git.  We can't expect the Cygwin maintainers to enable
a feature in a software package that makes it work on 90% of the
Cygwin installs out there; that's just asking for trouble.

But we can compile in a user-configurable switch, where the user can
shoot their own foot off in the name of speed, especially if they
can easily disable it on the oddball repositories where it fails.

Of course it might be even better if the code could auto-sense
when its busted and just switch itself off.  E.g. if four or
more consecutive "fast" stat calls fail but the original Cygwin
call succeeds then just always use Cygwin calls for the rest of
the process.

-- 
Shawn.

Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

[Alex Riesen]

2008/9/24 Shawn O. Pearce <spearce@spearce.org>:
> Alex Riesen <raa.lkml@gmail.com> wrote:
>> 2008/9/24 Dmitry Potapov <dpotapov@gmail.com>:
>>
>> > Frankly, I don't have strong preference here neither for making this
>> > fast version always work nor leave it conditional (perhaps, with the
>> > default setting use-fast-version). So, whatever the majority decides
>> > is fine with me.
>>
>> I'm voting for compile-time configuration then.
>
> To be consistent with everything else, compile-time sounds like
> what we should do, its how just about every other part of Git
> is configured.
>
> However Dmitry pointed out that he has cases where this faster
> function doesn't work correctly, and it was path specific. Some
> areas of the filesystem work, others don't, on the same system.

Huh?! What are they? What paths require cygwin's handling
which aren't handled already? (the absolute paths are handled).

Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

[Shawn O. Pearce]

Alex Riesen <raa.lkml@gmail.com> wrote:
> 2008/9/24 Shawn O. Pearce <spearce@spearce.org>:
> >
> > However Dmitry pointed out that he has cases where this faster
> > function doesn't work correctly, and it was path specific. Some
> > areas of the filesystem work, others don't, on the same system.
> 
> Huh?! What are they? What paths require cygwin's handling
> which aren't handled already? (the absolute paths are handled).

Cygwin lets you mount a filesystem at a different part of the
filesystem.  Sort of like Linux's mount -t bind (IIRC).

For example its possible to remap C:\foo\bar\widget\srcs into
C:\cygwin\home\lib, so you see the files under /home/lib in Cygwin,
even though that folder is empty (or flat out doesn't exist)
in Windows.

That filesystem remount stuff is part of the reason why the Cygwin
stat/lstat routines are so much slower than the native Windows ones.
They have to evaluate the path space twice (once in Cygwin, again
in the Windows kernel).

-- 
Shawn.

Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

[Alex Riesen]

2008/9/24 Shawn O. Pearce <spearce@spearce.org>:
> Alex Riesen <raa.lkml@gmail.com> wrote:
>> 2008/9/24 Shawn O. Pearce <spearce@spearce.org>:
>> >
>> > However Dmitry pointed out that he has cases where this faster
>> > function doesn't work correctly, and it was path specific. Some
>> > areas of the filesystem work, others don't, on the same system.
>>
>> Huh?! What are they? What paths require cygwin's handling
>> which aren't handled already? (the absolute paths are handled).
>
> Cygwin lets you mount a filesystem at a different part of the
> filesystem.

Ewwww... Yes, you're right. Disgusting feature. Just when I
thought Cygwin cannot get any worse...
Config parameter looks like the only way to workaround that.

> Sort of like Linux's mount -t bind (IIRC).

Except that in Linux the binding is part of the filesystem namespace,
not a special knowledge of some stupid library.

[PATCH v2] Add a "fast stat" mode for Cygwin

[Marcus Griep]

This patch introduces core.cygwinnativestat configuration flag. If this
variable is not set then Git will work as before. However, if it is set
then the Cygwin version of Git will try to use a Win32 API function if
it is possible to speed up stat/lstat.

This fast mode works only for relative paths. It is assumed that the
whole repository is located inside one directory without using Cygwin
mount to bind external paths inside of the current tree.

Symbolic links are supported by falling back on the cygwin version of
these functions.

A very superficial testing shows 'git status' in the fast mode works more
than twice faster than in the normal mode, i.e. with about the same speed
as the native MinGW version.

Also, with this patch, Cygwin and MinGW share the same code for doing
native filesystem stats.  Also incorporates Shawn Pearce's suggestion
to use a config flag rather than an environment variable to control
which method (native or cygwin) is used.

Originally-by: Dmitry Potapov <dpotapov@gmail.com>
Signed-off-by: Marcus Griep <marcus@griep.us>
---

 This is a substitute patch that takes care of many of the concerns already
 posted to this thread regarding the patch.  Sorry if it steps on your toes,
 Dmitry.  You began scratching my itch, so I wanted to jump in and scratch
 some more.

 Overall, the new patch implements many of the ideas from the thread, including
 using a configuration flag to determine which stat (cygwin or native) to use,
 and the code is refactored so that MinGW and Cygwin can both use the same
 base stat code.

 If it looks good, I'd appreciate a regression 'Tested-by' from the MinGW
 folks to make sure I didn't break their compile or such.

 Also, if you'd prefer that I submit a patch that would be applied on top
 of Dmitry's currently pending patch, I can do that as well.

 Finally, here is the output from a little benchmark I ran on my large
 repository at work:
 115769 files in 17626 directories

 Windows native stat: false
 2.17user 8.92system 0:14.40elapsed 76%CPU (0avgtext+0avgdata 22544384maxresident)k
 0inputs+0outputs (161427major+0minor)pagefaults 0swaps

 Windows native stat: true
 1.00user 2.85system 0:06.26elapsed 61%CPU (0avgtext+0avgdata 22544384maxresident)k
 0inputs+0outputs (161427major+0minor)pagefaults 0swaps

 Looks like a nice improvement to me.

 -Marcus

 Makefile          |    7 ++++
 cache.h           |    1 +
 compat/cygwin.c   |   67 ++++++++++++++++++++++++++++++++++++
 compat/cygwin.h   |    7 ++++
 compat/mingw.c    |   64 +---------------------------------
 compat/windows.c  |   98 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 compat/windows.h  |   12 ++++++
 environment.c     |    5 +++
 git-compat-util.h |    1 +
 9 files changed, 200 insertions(+), 62 deletions(-)
 create mode 100644 compat/cygwin.c
 create mode 100644 compat/cygwin.h
 create mode 100644 compat/windows.c
 create mode 100644 compat/windows.h

diff --git a/Makefile b/Makefile
index e0c03c3..685f038 100644
--- a/Makefile
+++ b/Makefile
@@ -346,6 +346,8 @@ LIB_H += cache.h
 LIB_H += cache-tree.h
 LIB_H += commit.h
 LIB_H += compat/mingw.h
+LIB_H += compat/cygwin.h
+LIB_H += compat/windows.h
 LIB_H += csum-file.h
 LIB_H += decorate.h
 LIB_H += delta.h
@@ -748,6 +750,10 @@ ifeq ($(uname_S),HP-UX)
 	NO_SYS_SELECT_H = YesPlease
 	SNPRINTF_RETURNS_BOGUS = YesPlease
 endif
+ifneq (,$(findstring CYGWIN,$(uname_S)))
+	COMPAT_OBJS += compat/cygwin.o
+	COMPAT_OBJS += compat/windows.o
+endif
 ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_MMAP = YesPlease
 	NO_PREAD = YesPlease
@@ -774,6 +780,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	COMPAT_CFLAGS += -DSNPRINTF_SIZE_CORR=1
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
 	COMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/regex/regex.o compat/winansi.o
+	COMPAT_OBJS += compat/windows.o
 	EXTLIBS += -lws2_32
 	X = .exe
 	gitexecdir = ../libexec/git-core
diff --git a/cache.h b/cache.h
index f4b8ddf..b42ca0f 100644
--- a/cache.h
+++ b/cache.h
@@ -321,6 +321,7 @@ extern int set_git_dir(const char *path);
 extern const char *get_git_work_tree(void);
 extern const char *read_gitfile_gently(const char *path);
 extern void set_git_work_tree(const char *tree);
+extern int have_git_dir(void);
 
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
diff --git a/compat/cygwin.c b/compat/cygwin.c
new file mode 100644
index 0000000..db44aa8
--- /dev/null
+++ b/compat/cygwin.c
@@ -0,0 +1,67 @@
+#include "windows.h"
+#include "../cache.h"
+
+#undef stat
+#undef lstat
+
+/* We provide our own lstat/stat functions, since the provided Cygwin versions
+ * of these functions are too slow. These stat functions are tailored for Git's
+ * usage, and therefore they are not meant to be complete and correct emulation
+ * of lstat/stat functionality.
+ */
+static int cygwin_lstat(const char *path, struct stat *buf)
+{
+	return win_stat(path, buf, lstat);
+}
+
+static int cygwin_stat(const char *path, struct stat *buf)
+{
+	return win_stat(path, buf, stat);
+}
+
+static int native_stat = -1;
+static stat_fn_t cygwin_stat_fn = stat;
+static stat_fn_t cygwin_lstat_fn = lstat;
+
+static int git_cygwin_config(const char *var, const char *value, void *cb)
+{
+	if (!strcmp(var, "core.cygwinnativestat"))
+		native_stat = git_config_bool(var, value);
+	return 0;
+}
+
+static void init_stat(void)
+{
+	if (native_stat < 0 && have_git_dir()) {
+		native_stat = 0;
+		git_config(git_cygwin_config, NULL);
+		cygwin_stat_fn = native_stat ? cygwin_stat : stat;
+		cygwin_lstat_fn = native_stat ? cygwin_lstat : lstat;
+	}
+}
+
+/*
+ * This are startup stubs, which choose what implementation of lstat/stat
+ * should be used. If core.cygwinnativestat is not set then the standard
+ * functions included in the cygwin library are used. If it is set then our
+ * fast and dirty implementation is invoked, which should be 2-3 times
+ * faster than cygwin functions.
+ */
+int cygwin_stat_choice(const char *file_name, struct stat *buf)
+{
+	if (file_name[0] == '/')
+		return stat(file_name, buf);
+
+	init_stat();
+	return (cygwin_stat_fn ? cygwin_stat_fn : stat) (file_name, buf);
+}
+
+int cygwin_lstat_choice(const char *file_name, struct stat *buf)
+{
+	if (file_name[0] == '/')
+		return lstat(file_name, buf);
+
+	init_stat();
+	return (cygwin_lstat_fn ? cygwin_lstat_fn : lstat) (file_name, buf);
+}
+
diff --git a/compat/cygwin.h b/compat/cygwin.h
new file mode 100644
index 0000000..14775fd
--- /dev/null
+++ b/compat/cygwin.h
@@ -0,0 +1,7 @@
+#include "windows.h"
+
+int cygwin_stat_choice(const char*, struct stat*);
+int cygwin_lstat_choice(const char*, struct stat*);
+
+#define stat(path,buf) cygwin_stat_choice(path,buf)
+#define lstat cygwin_lstat_choice
diff --git a/compat/mingw.c b/compat/mingw.c
index ccfa2a0..174142c 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -23,66 +23,6 @@ int mingw_open (const char *filename, int oflags, ...)
 	return fd;
 }
 
-static inline time_t filetime_to_time_t(const FILETIME *ft)
-{
-	long long winTime = ((long long)ft->dwHighDateTime << 32) + ft->dwLowDateTime;
-	winTime -= 116444736000000000LL; /* Windows to Unix Epoch conversion */
-	winTime /= 10000000;		 /* Nano to seconds resolution */
-	return (time_t)winTime;
-}
-
-extern int _getdrive( void );
-/* We keep the do_lstat code in a separate function to avoid recursion.
- * When a path ends with a slash, the stat will fail with ENOENT. In
- * this case, we strip the trailing slashes and stat again.
- */
-static int do_lstat(const char *file_name, struct stat *buf)
-{
-	WIN32_FILE_ATTRIBUTE_DATA fdata;
-
-	if (GetFileAttributesExA(file_name, GetFileExInfoStandard, &fdata)) {
-		int fMode = S_IREAD;
-		if (fdata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
-			fMode |= S_IFDIR;
-		else
-			fMode |= S_IFREG;
-		if (!(fdata.dwFileAttributes & FILE_ATTRIBUTE_READONLY))
-			fMode |= S_IWRITE;
-
-		buf->st_ino = 0;
-		buf->st_gid = 0;
-		buf->st_uid = 0;
-		buf->st_nlink = 1;
-		buf->st_mode = fMode;
-		buf->st_size = fdata.nFileSizeLow; /* Can't use nFileSizeHigh, since it's not a stat64 */
-		buf->st_dev = buf->st_rdev = (_getdrive() - 1);
-		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));
-		errno = 0;
-		return 0;
-	}
-
-	switch (GetLastError()) {
-	case ERROR_ACCESS_DENIED:
-	case ERROR_SHARING_VIOLATION:
-	case ERROR_LOCK_VIOLATION:
-	case ERROR_SHARING_BUFFER_EXCEEDED:
-		errno = EACCES;
-		break;
-	case ERROR_BUFFER_OVERFLOW:
-		errno = ENAMETOOLONG;
-		break;
-	case ERROR_NOT_ENOUGH_MEMORY:
-		errno = ENOMEM;
-		break;
-	default:
-		errno = ENOENT;
-		break;
-	}
-	return -1;
-}
-
 /* We provide our own lstat/fstat functions, since the provided
  * lstat/fstat functions are so slow. These stat functions are
  * tailored for Git's usage (read: fast), and are not meant to be
@@ -94,7 +34,7 @@ int mingw_lstat(const char *file_name, struct stat *buf)
 	int namelen;
 	static char alt_name[PATH_MAX];
 
-	if (!do_lstat(file_name, buf))
+	if (!win_stat(file_name, buf, win_stat_fail))
 		return 0;
 
 	/* if file_name ended in a '/', Windows returned ENOENT;
@@ -113,7 +53,7 @@ int mingw_lstat(const char *file_name, struct stat *buf)
 
 	memcpy(alt_name, file_name, namelen);
 	alt_name[namelen] = 0;
-	return do_lstat(alt_name, buf);
+	return win_stat(alt_name, buf, win_stat_fail);
 }
 
 #undef fstat
diff --git a/compat/windows.c b/compat/windows.c
new file mode 100644
index 0000000..8c7d976
--- /dev/null
+++ b/compat/windows.c
@@ -0,0 +1,98 @@
+#define WIN32_LEAN_AND_MEAN
+#include "../git-compat-util.h"
+#include <windows.h>
+
+static inline void filetime_to_timespec(const FILETIME *ft, struct timespec *ts)
+{
+	long long winTime = ((long long)ft->dwHighDateTime << 32) + ft->dwLowDateTime;
+	winTime -= 116444736000000000LL; /* Windows to Unix Epoch conversion */
+	ts->tv_sec = (time_t)(winTime/10000000); /* 100-nanosecond interval to seconds */
+	ts->tv_nsec = (long)(winTime - ts->tv_sec*10000000LL) * 100; /* nanoseconds */
+}
+
+#define size_to_blocks(s) (((s)+511)/512)
+
+/* do_lstat is a common implementation of a faster stat algorithm for Windows
+ *
+ * When the Windows stat fails for an unknown reason, a fallback is called to
+ * handle the error.
+ */
+int win_stat(const char *file_name, struct stat *buf, stat_fn_t fallback)
+{
+	WIN32_FILE_ATTRIBUTE_DATA fdata;
+
+	if (GetFileAttributesExA(file_name, GetFileExInfoStandard, &fdata)) {
+		int fMode = S_IREAD;
+
+#if defined(__CYGWIN__)
+		/*
+		 * If the system attribute is set and it is not a directory then
+		 * it could be a symbol link created in the nowinsymlinks mode.
+		 * Normally, Cygwin works in the winsymlinks mode, so this situation
+		 * is very unlikely. For the sake of simplicity of our code, let's
+		 * Cygwin to handle it.
+		 */
+		if ((fdata.dwFileAttributes & FILE_ATTRIBUTE_SYSTEM) &&
+		    !(fdata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY))
+			return fallback (file_name, buf);
+#endif
+
+		if (fdata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
+			fMode |= S_IFDIR;
+		else
+			fMode |= S_IFREG;
+		if (!(fdata.dwFileAttributes & FILE_ATTRIBUTE_READONLY))
+			fMode |= S_IWRITE;
+
+		buf->st_ino = 0;
+		buf->st_gid = buf->st_uid = 0;
+		buf->st_nlink = 1;
+		buf->st_mode = fMode;
+#ifdef __CYGWIN_USE_BIG_TYPES__
+		buf->st_size = ((_off64_t)fdata.nFileSizeHigh << 32) +
+			fdata.nFileSizeLow;
+#else
+		buf->st_size = fdata.nFileSizeLow; /* Can't use nFileSizeHigh, since it's not a stat64 */
+#endif
+		buf->st_blocks = size_to_blocks(buf->st_size);
+		/* st_dev, st_rdev are not used by Git */
+		buf->st_dev = buf->st_rdev = 0;
+		filetime_to_timespec(&fdata.ftLastAccessTime, &buf->st_atim);
+		filetime_to_timespec(&fdata.ftLastWriteTime, &buf->st_mtim);
+		filetime_to_timespec(&fdata.ftCreationTime, &buf->st_ctim);
+		errno = 0;
+		return 0;
+	}
+
+	switch (GetLastError()) {
+	case ERROR_ACCESS_DENIED:
+	case ERROR_SHARING_VIOLATION:
+	case ERROR_LOCK_VIOLATION:
+	case ERROR_SHARING_BUFFER_EXCEEDED:
+		errno = EACCES;
+		break;
+	case ERROR_BUFFER_OVERFLOW:
+		errno = ENAMETOOLONG;
+		break;
+	case ERROR_NOT_ENOUGH_MEMORY:
+		errno = ENOMEM;
+		break;
+	default:
+		/* In the winsymlinks mode (which is the default), Cygwin
+		 * emulates symbol links using Windows shortcut files. These
+		 * files are formed by adding .lnk extension. So, if we have
+		 * not found the specified file name, it could be that it is
+		 * a symbol link. Let's the fallback deal with that.
+		 * In MinGW, this could also happen if the path ends with a
+		 * slash.
+		 */
+		return fallback (file_name, buf);
+	}
+	return -1;
+}
+
+int win_stat_fail(const char *file_name, struct stat *buf)
+{
+	errno = ENOENT;
+	return -1;
+}
diff --git a/compat/windows.h b/compat/windows.h
new file mode 100644
index 0000000..8412e42
--- /dev/null
+++ b/compat/windows.h
@@ -0,0 +1,12 @@
+#ifndef COMPAT_WINDOWS_H
+#define COMPAT_WINDOWS_H
+
+#include <sys/types.h>
+#include <sys/stat.h>
+
+typedef int (*stat_fn_t)(const char*, struct stat*);
+
+extern int win_stat(const char*, struct stat*, stat_fn_t);
+extern int win_stat_fail(const char*, struct stat *);
+
+#endif
diff --git a/environment.c b/environment.c
index 0c6d11f..cbd8074 100644
--- a/environment.c
+++ b/environment.c
@@ -151,3 +151,8 @@ int set_git_dir(const char *path)
 	setup_git_env();
 	return 0;
 }
+
+int have_git_dir(void)
+{
+	return !!git_dir;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index db2836f..cd9752c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -85,6 +85,7 @@
 #undef _XOPEN_SOURCE
 #include <grp.h>
 #define _XOPEN_SOURCE 600
+#include "compat/cygwin.h"
 #else
 #undef _ALL_SOURCE /* AIX 5.3L defines a struct list with _ALL_SOURCE. */
 #include <grp.h>
-- 
1.6.0.2.405.g3cc38

Re: [PATCH v2] Add a "fast stat" mode for Cygwin

[Alex Riesen]

Marcus Griep, Sat, Sep 27, 2008 09:02:06 +0200:
> This patch introduces core.cygwinnativestat configuration flag. If this

"cygwin.nativestat"? If only to shame them

> variable is not set then Git will work as before. However, if it is set
> then the Cygwin version of Git will try to use a Win32 API function if
> it is possible to speed up stat/lstat.
> 
> This fast mode works only for relative paths. It is assumed that the
> whole repository is located inside one directory without using Cygwin
> mount to bind external paths inside of the current tree.
> 
> Symbolic links are supported by falling back on the cygwin version of
> these functions.

The cygwins .lnk cannot be supported. The names just don't exist for
native GetFileAttributesExA.

Re: [PATCH v2] Add a "fast stat" mode for Cygwin

[Dmitry Potapov]

Hi Marcus,

On Sat, Sep 27, 2008 at 03:02:06AM -0400, Marcus Griep wrote:
> 
>  This is a substitute patch that takes care of many of the concerns already
>  posted to this thread regarding the patch.  Sorry if it steps on your toes,
>  Dmitry.  You began scratching my itch, so I wanted to jump in and scratch
>  some more.

I am sorry I was not able to send my patches earlier. I had them ready
by the end of the day when we had the discussion, but I have not had an
opportunity to test it on Windows till today.

I have only skimmed over your patch, but there are a few changes that
I really dislike about your patch. You changed the semantic of _choice
functions. While I use it as stubs to choose what implementation to use,
you make them part of implementation, which is always called. So I do
not understand why you left the comment saying that they are only stubs
and then why you need function pointers at all then.

Also, you made some changes to MinGW (I don't know if you tested it),
but any change like removing _getdrive() from MinGW version is better
to move into separately patch (or, at least, clearly state them in the
commit comment).

Anyway, thanks for your efforts, but if you want to go ahead with some
other match of mine (especially Windows related), please, let me know,
so we can avoid stepping on each other toes.

Dmitry