[RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions

[RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions

[Ramsay Jones]

Commit adbc0b6b ("cygwin: Use native Win32 API for stat", 30-09-2008)
added a Win32 specific implementation of the stat functions. In order
to handle absolute paths, cygwin mount points and symbolic links, this
implementation may fall back on the standard cygwin l/stat() functions.
Also, the choice of cygwin or Win32 functions is made lazily (by the
first call(s) to l/stat) based on the state of some config variables.

Unfortunately, this "schizophrenic stat" implementation has been the
source of many problems ever since. For example, see commits 7faee6b8,
79748439, 452993c2, 085479e7, b8a97333, 924aaf3e, 05bab3ea and 0117c2f0.

In order to limit the adverse effects caused by this implementation,
we provide a new "fast stat" interface, which allows us to use this
only for interactions with the index (i.e. the cached stat data).

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
 builtin/apply.c        |  8 ++++----
 builtin/commit.c       |  2 +-
 builtin/ls-files.c     |  2 +-
 builtin/rm.c           |  2 +-
 builtin/update-index.c |  2 +-
 check-racy.c           |  2 +-
 compat/cygwin.c        | 48 ++++++++++--------------------------------------
 compat/cygwin.h        | 17 +++++++++--------
 diff-lib.c             |  2 +-
 diff.c                 |  2 +-
 entry.c                |  6 +++---
 git-compat-util.h      | 27 +++++++++++++++++++++++++--
 help.c                 |  5 +----
 path.c                 |  9 +--------
 preload-index.c        |  2 +-
 read-cache.c           |  6 +++---
 unpack-trees.c         |  8 ++++----
 17 files changed, 68 insertions(+), 82 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 0e9b631..f5046a6 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3091,7 +3091,7 @@ static int checkout_target(struct cache_entry *ce, struct stat *st)
 	memset(&costate, 0, sizeof(costate));
 	costate.base_dir = "";
 	costate.refresh_cache = 1;
-	if (checkout_entry(ce, &costate, NULL) || lstat(ce->name, st))
+	if (checkout_entry(ce, &costate, NULL) || fast_lstat(ce->name, st))
 		return error(_("cannot checkout %s"), ce->name);
 	return 0;
 }
@@ -3253,7 +3253,7 @@ static int load_current(struct image *image, struct patch *patch)
 	if (pos < 0)
 		return error(_("%s: does not exist in index"), name);
 	ce = active_cache[pos];
-	if (lstat(name, &st)) {
+	if (fast_lstat(name, &st)) {
 		if (errno != ENOENT)
 			return error(_("%s: %s"), name, strerror(errno));
 		if (checkout_target(ce, &st))
@@ -3396,7 +3396,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
 	if (previous) {
 		st_mode = previous->new_mode;
 	} else if (!cached) {
-		stat_ret = lstat(old_name, st);
+		stat_ret = fast_lstat(old_name, st);
 		if (stat_ret && errno != ENOENT)
 			return error(_("%s: %s"), old_name, strerror(errno));
 	}
@@ -3850,7 +3850,7 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned
 			die(_("corrupt patch for subproject %s"), path);
 	} else {
 		if (!cached) {
-			if (lstat(path, &st) < 0)
+			if (fast_lstat(path, &st) < 0)
 				die_errno(_("unable to stat newly created file '%s'"),
 					  path);
 			fill_stat_cache_info(ce, &st);
diff --git a/builtin/commit.c b/builtin/commit.c
index 6b693c1..1d208c6 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -231,7 +231,7 @@ static void add_remove_files(struct string_list *list)
 		if (p->util)
 			continue;
 
-		if (!lstat(p->string, &st)) {
+		if (!fast_lstat(p->string, &st)) {
 			if (add_to_cache(p->string, &st, 0))
 				die(_("updating files failed"));
 		} else
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 3a410c3..a066719 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -247,7 +247,7 @@ static void show_files(struct dir_struct *dir)
 				continue;
 			if (ce_skip_worktree(ce))
 				continue;
-			err = lstat(ce->name, &st);
+			err = fast_lstat(ce->name, &st);
 			if (show_deleted && err)
 				show_ce_entry(tag_removed, ce);
 			if (show_modified && ce_modified(ce, &st, 0))
diff --git a/builtin/rm.c b/builtin/rm.c
index 06025a2..4b783e7 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -143,7 +143,7 @@ static int check_local_mod(unsigned char *head, int index_only)
 		}
 		ce = active_cache[pos];
 
-		if (lstat(ce->name, &st) < 0) {
+		if (fast_lstat(ce->name, &st) < 0) {
 			if (errno != ENOENT && errno != ENOTDIR)
 				warning("'%s': %s", ce->name, strerror(errno));
 			/* It already vanished from the working tree */
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 5c7762e..4790e4c 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -206,7 +206,7 @@ static int process_path(const char *path)
 	 * First things first: get the stat information, to decide
 	 * what to do about the pathname!
 	 */
-	if (lstat(path, &st) < 0)
+	if (fast_lstat(path, &st) < 0)
 		return process_lstat_error(path, errno);
 
 	if (S_ISDIR(st.st_mode))
diff --git a/check-racy.c b/check-racy.c
index 00d92a1..6124355 100644
--- a/check-racy.c
+++ b/check-racy.c
@@ -11,7 +11,7 @@ int main(int ac, char **av)
 		struct cache_entry *ce = active_cache[i];
 		struct stat st;
 
-		if (lstat(ce->name, &st)) {
+		if (fast_lstat(ce->name, &st)) {
 			error("lstat(%s): %s", ce->name, strerror(errno));
 			continue;
 		}
diff --git a/compat/cygwin.c b/compat/cygwin.c
index 91ce5d4..661d8f7 100644
--- a/compat/cygwin.c
+++ b/compat/cygwin.c
@@ -1,4 +1,3 @@
-#define CYGWIN_C
 #define WIN32_LEAN_AND_MEAN
 #include <sys/stat.h>
 #include <sys/errno.h>
@@ -6,18 +5,6 @@
 #include "../git-compat-util.h"
 #include "../cache.h" /* to read configuration */
 
-/*
- * Return POSIX permission bits, regardless of core.ignorecygwinfstricks
- */
-int cygwin_get_st_mode_bits(const char *path, int *mode)
-{
-	struct stat st;
-	if (lstat(path, &st) < 0)
-		return -1;
-	*mode = st.st_mode;
-	return 0;
-}
-
 static inline void filetime_to_timespec(const FILETIME *ft, struct timespec *ts)
 {
 	long long winTime = ((long long)ft->dwHighDateTime << 32) +
@@ -85,29 +72,23 @@ static int do_stat(const char *file_name, struct stat *buf, stat_fn_t cygstat)
 	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.
+/* We provide our own lstat function, since the provided Cygwin versions of
+ * the stat functions are too slow. This lstat function is tailored for Git's
+ * usage, and therefore it is not meant to be complete and correct emulation
+ * of lstat 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);
-}
-
-
 /*
- * At start up, we are trying to determine whether Win32 API or cygwin stat
- * functions should be used. The choice is determined by core.ignorecygwinfstricks.
+ * At start up, we are trying to determine whether Win32 API or cygwin lstat
+ * function should be used. The choice is determined by core.ignorecygwinfstricks.
  * Reading this option is not always possible immediately as git_dir may
- * not be set yet. So until it is set, use cygwin lstat/stat functions.
+ * not be set yet. So until it is set, use the cygwin lstat function.
  * However, if core.filemode is set, we must use the Cygwin posix
- * stat/lstat as the Windows stat functions do not determine posix filemode.
+ * lstat as the Windows lstat function does not determine posix filemode.
  *
  * Note that git_cygwin_config() does NOT call git_default_config() and this
  * is deliberate.  Many commands read from config to establish initial
@@ -130,28 +111,19 @@ static int git_cygwin_config(const char *var, const char *value, void *cb)
 static int init_stat(void)
 {
 	if (have_git_dir() && git_config(git_cygwin_config,NULL)) {
-		if (!core_filemode && native_stat) {
-			cygwin_stat_fn = cygwin_stat;
+		if (!core_filemode && native_stat)
 			cygwin_lstat_fn = cygwin_lstat;
-		} else {
-			cygwin_stat_fn = stat;
+		else
 			cygwin_lstat_fn = lstat;
-		}
 		return 0;
 	}
 	return 1;
 }
 
-static int cygwin_stat_stub(const char *file_name, struct stat *buf)
-{
-	return (init_stat() ? stat : *cygwin_stat_fn)(file_name, buf);
-}
-
 static int cygwin_lstat_stub(const char *file_name, struct stat *buf)
 {
 	return (init_stat() ? lstat : *cygwin_lstat_fn)(file_name, buf);
 }
 
-stat_fn_t cygwin_stat_fn = cygwin_stat_stub;
 stat_fn_t cygwin_lstat_fn = cygwin_lstat_stub;
 
diff --git a/compat/cygwin.h b/compat/cygwin.h
index c04965a..c5955cc 100644
--- a/compat/cygwin.h
+++ b/compat/cygwin.h
@@ -2,13 +2,14 @@
 #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;
-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))
-#ifndef CYGWIN_C
-/* cygwin.c needs the original lstat() */
-#define stat(path, buf) (*cygwin_stat_fn)(path, buf)
-#define lstat(path, buf) (*cygwin_lstat_fn)(path, buf)
-#endif
+/*
+ * Note that the fast_fstat function should never actually
+ * be called, since cygwin has the UNRELIABLE_FSTAT build
+ * variable set. Currently, all calls to fast_fstat are
+ * protected by 'fstat_is_reliable()'.
+ */
+#define fast_lstat(path, buf) (*cygwin_lstat_fn)(path, buf)
+#define fast_fstat(fd, buf) fstat(fd, buf)
+#define GIT_FAST_STAT
diff --git a/diff-lib.c b/diff-lib.c
index b6f4b21..401dab6 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -27,7 +27,7 @@
  */
 static int check_removed(const struct cache_entry *ce, struct stat *st)
 {
-	if (lstat(ce->name, st) < 0) {
+	if (fast_lstat(ce->name, st) < 0) {
 		if (errno != ENOENT && errno != ENOTDIR)
 			return -1;
 		return 1;
diff --git a/diff.c b/diff.c
index 208094f..212d3ff 100644
--- a/diff.c
+++ b/diff.c
@@ -2642,7 +2642,7 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int
 	 * If ce matches the file in the work tree, we can reuse it.
 	 */
 	if (ce_uptodate(ce) ||
-	    (!lstat(name, &st) && !ce_match_stat(ce, &st, 0)))
+	    (!fast_lstat(name, &st) && !ce_match_stat(ce, &st, 0)))
 		return 1;
 
 	return 0;
diff --git a/entry.c b/entry.c
index d7c131d..f746d2a 100644
--- a/entry.c
+++ b/entry.c
@@ -109,7 +109,7 @@ static int fstat_output(int fd, const struct checkout *state, struct stat *st)
 	/* use fstat() only when path == ce->name */
 	if (fstat_is_reliable() &&
 	    state->refresh_cache && !state->base_dir_len) {
-		fstat(fd, st);
+		fast_fstat(fd, st);
 		return 1;
 	}
 	return 0;
@@ -210,7 +210,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
 finish:
 	if (state->refresh_cache) {
 		if (!fstat_done)
-			lstat(ce->name, &st);
+			fast_lstat(ce->name, &st);
 		fill_stat_cache_info(ce, &st);
 	}
 	return 0;
@@ -230,7 +230,7 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)
 		errno = ENOENT;
 		return -1;
 	}
-	return lstat(path, st);
+	return fast_lstat(path, st);
 }
 
 int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath)
diff --git a/git-compat-util.h b/git-compat-util.h
index ff193f4..092a571 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -129,8 +129,6 @@
 #include <poll.h>
 #endif
 
-extern int get_st_mode_bits(const char *path, int *mode);
-
 #if defined(__MINGW32__)
 /* pull in Windows compatibility stuff */
 #include "compat/mingw.h"
@@ -179,6 +177,31 @@ typedef unsigned long uintptr_t;
 #endif
 #endif
 
+#ifndef GIT_FAST_STAT
+/*
+ * The "fast" stat() variants are used to read the stat data from the
+ * filesystem that is stored into the index and/or that is compared
+ * with the cached stat data. In order to provide a fast implementation,
+ * these functions may not provide meaningful data in all fields of the
+ * stat structure and should not, therefore, be used for any other purpose.
+ *
+ * Platforms which have slow stat functions, which can also provide
+ * such fast variants (e.g. cygwin), should include the external
+ * declarations above and define the GIT_FAST_STAT macro.
+ *
+ * The static inline definitions below are for platforms which have
+ * no need of such fast variants.
+ */
+static inline int fast_lstat(const char *path, struct stat *st)
+{
+	return lstat(path, st);
+}
+static inline int fast_fstat(int fd, struct stat *st)
+{
+	return fstat(fd, st);
+}
+#endif
+
 /* used on Mac OS X */
 #ifdef PRECOMPOSE_UNICODE
 #include "compat/precompose_utf8.h"
diff --git a/help.c b/help.c
index 08c54ef..f068925 100644
--- a/help.c
+++ b/help.c
@@ -107,10 +107,7 @@ static int is_executable(const char *name)
 	    !S_ISREG(st.st_mode))
 		return 0;
 
-#if defined(GIT_WINDOWS_NATIVE) || defined(__CYGWIN__)
-#if defined(__CYGWIN__)
-if ((st.st_mode & S_IXUSR) == 0)
-#endif
+#if defined(GIT_WINDOWS_NATIVE)
 {	/* cannot trust the executable bit, peek into the file instead */
 	char buf[3] = { 0 };
 	int n;
diff --git a/path.c b/path.c
index 04ff148..4e7fae4 100644
--- a/path.c
+++ b/path.c
@@ -5,13 +5,7 @@
 #include "strbuf.h"
 #include "string-list.h"
 
-#ifndef get_st_mode_bits
-/*
- * The replacement lstat(2) we use on Cygwin is incomplete and
- * may return wrong permission bits. Most of the time we do not care,
- * but the callsites of this wrapper do care.
- */
-int get_st_mode_bits(const char *path, int *mode)
+static int get_st_mode_bits(const char *path, int *mode)
 {
 	struct stat st;
 	if (lstat(path, &st) < 0)
@@ -19,7 +13,6 @@ int get_st_mode_bits(const char *path, int *mode)
 	*mode = st.st_mode;
 	return 0;
 }
-#endif
 
 static char bad_path[] = "/bad-path/";
 
diff --git a/preload-index.c b/preload-index.c
index 49cb08d..1bece91 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -57,7 +57,7 @@ static void *preload_thread(void *_data)
 			continue;
 		if (threaded_has_symlink_leading_path(&cache, ce->name, ce_namelen(ce)))
 			continue;
-		if (lstat(ce->name, &st))
+		if (fast_lstat(ce->name, &st))
 			continue;
 		if (ie_match_stat(index, ce, &st, CE_MATCH_RACY_IS_DIRTY))
 			continue;
diff --git a/read-cache.c b/read-cache.c
index d5201f9..ed33d9e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -689,7 +689,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 int add_file_to_index(struct index_state *istate, const char *path, int flags)
 {
 	struct stat st;
-	if (lstat(path, &st))
+	if (fast_lstat(path, &st))
 		die_errno("unable to stat '%s'", path);
 	return add_to_index(istate, path, &st, flags);
 }
@@ -1049,7 +1049,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 		return ce;
 	}
 
-	if (lstat(ce->name, &st) < 0) {
+	if (fast_lstat(ce->name, &st) < 0) {
 		if (err)
 			*err = errno;
 		return NULL;
@@ -1635,7 +1635,7 @@ static void ce_smudge_racily_clean_entry(struct cache_entry *ce)
 	 */
 	struct stat st;
 
-	if (lstat(ce->name, &st) < 0)
+	if (fast_lstat(ce->name, &st) < 0)
 		return;
 	if (ce_match_stat_basic(ce, &st))
 		return;
diff --git a/unpack-trees.c b/unpack-trees.c
index b27f2a6..1fe9b63 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1215,7 +1215,7 @@ static int verify_uptodate_1(const struct cache_entry *ce,
 	else if (o->reset || ce_uptodate(ce))
 		return 0;
 
-	if (!lstat(ce->name, &st)) {
+	if (!fast_lstat(ce->name, &st)) {
 		int flags = CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE;
 		unsigned changed = ie_match_stat(o->src_index, ce, &st, flags);
 		if (!changed)
@@ -1432,13 +1432,13 @@ static int verify_absent_1(const struct cache_entry *ce,
 		char path[PATH_MAX + 1];
 		memcpy(path, ce->name, len);
 		path[len] = 0;
-		if (lstat(path, &st))
+		if (fast_lstat(path, &st))
 			return error("cannot stat '%s': %s", path,
 					strerror(errno));
 
 		return check_ok_to_remove(path, len, DT_UNKNOWN, NULL, &st,
 				error_type, o);
-	} else if (lstat(ce->name, &st)) {
+	} else if (fast_lstat(ce->name, &st)) {
 		if (errno != ENOENT)
 			return error("cannot stat '%s': %s", ce->name,
 				     strerror(errno));
@@ -1852,7 +1852,7 @@ int oneway_merge(const struct cache_entry * const *src,
 		int update = 0;
 		if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {
 			struct stat st;
-			if (lstat(old->name, &st) ||
+			if (fast_lstat(old->name, &st) ||
 			    ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
 				update |= CE_UPDATE;
 		}
-- 
1.8.3

Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions

[Mark Levedahl]

On 07/10/2013 04:23 PM, Ramsay Jones wrote:
> Commit adbc0b6b ("cygwin: Use native Win32 API for stat", 30-09-2008)
> added a Win32 specific implementation of the stat functions. In order
> to handle absolute paths, cygwin mount points and symbolic links, this
> implementation may fall back on the standard cygwin l/stat() functions.
> Also, the choice of cygwin or Win32 functions is made lazily (by the
> first call(s) to l/stat) based on the state of some config variables.
>
> Unfortunately, this "schizophrenic stat" implementation has been the
> source of many problems ever since. For example, see commits 7faee6b8,
> 79748439, 452993c2, 085479e7, b8a97333, 924aaf3e, 05bab3ea and 0117c2f0.
>
> In order to limit the adverse effects caused by this implementation,
> we provide a new "fast stat" interface, which allows us to use this
> only for interactions with the index (i.e. the cached stat data).
>
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> ---

I've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results 
using your prior patch (removing the Cygwin specific lstat entirely) and 
get the same results with both, so this seems ok from me.

My comparison point was created by reverting your current patch from pu, 
then reapplying your earlier patch on top, so the only difference was 
which approach was used to address the stat functions.

Caveats:
1) I don't find any speed improvement of the current patch over the 
previous one (the tests actually ran faster with the earlier patch, 
though the difference was less than 1%).
2) I still question this whole approach, especially having this 
non-POSIX compliant mode be the default. Running in this mode breaks 
interoperability with Linux, but providing a Linux environment is the 
*primary* goal of Cygwin.

Mark

Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

>> In order to limit the adverse effects caused by this implementation,
>> we provide a new "fast stat" interface, which allows us to use this
>> only for interactions with the index (i.e. the cached stat data).
>>
>> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>> ---
>
> I've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results
> using your prior patch (removing the Cygwin specific lstat entirely)
> and get the same results with both, so this seems ok from me.
>
> My comparison point was created by reverting your current patch from
> pu, then reapplying your earlier patch on top, so the only difference
> was which approach was used to address the stat functions.
>
> Caveats:
> 1) I don't find any speed improvement of the current patch over the
> previous one (the tests actually ran faster with the earlier patch,
> though the difference was less than 1%).
> 2) I still question this whole approach, especially having this
> non-POSIX compliant mode be the default. Running in this mode breaks
> interoperability with Linux, but providing a Linux environment is the
> *primary* goal of Cygwin.

Sounds like we are better off without this patch, and instead remove
the "schizophrenic stat"?  I do not have a strong opinion either
way, except that I tend to agree with your point 2) above.

Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions

[Torsten Bögershausen]

On 2013-07-15 21.49, Junio C Hamano wrote:
> Mark Levedahl <mlevedahl@gmail.com> writes:
> 
>>> In order to limit the adverse effects caused by this implementation,
>>> we provide a new "fast stat" interface, which allows us to use this
>>> only for interactions with the index (i.e. the cached stat data).
>>>
>>> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>>> ---
>>
>> I've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results
>> using your prior patch (removing the Cygwin specific lstat entirely)
>> and get the same results with both, so this seems ok from me.
>>
>> My comparison point was created by reverting your current patch from
>> pu, then reapplying your earlier patch on top, so the only difference
>> was which approach was used to address the stat functions.
>>
>> Caveats:
>> 1) I don't find any speed improvement of the current patch over the
>> previous one (the tests actually ran faster with the earlier patch,
>> though the difference was less than 1%).
Hm, measuring the time for the test suite is one thing,
did you measure the time of "git status" with and without the patch?

(I don't have my test system at hand, so I can test in a few days/weeks)

>> 2) I still question this whole approach, especially having this
>> non-POSIX compliant mode be the default. Running in this mode breaks
>> interoperability with Linux, but providing a Linux environment is the
>> *primary* goal of Cygwin.
> 
> Sounds like we are better off without this patch, and instead remove
> the "schizophrenic stat"?  I do not have a strong opinion either
> way, except that I tend to agree with your point 2) above.

My understanding is that we want both:
Introduction of fast_lstat() as phase 1,
and the removal of the "schizophrenic stat" in compat/cygwin.c
as phase 2. (or do I missunderstand something ?)


And yes, phase 3:
The day we have a both reliable and fast 
lstat() in cygwin, we can remove compat/cygwin.[ch]

Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions

[Mark Levedahl]

On 07/15/2013 10:06 PM, Torsten Bögershausen wrote:
> On 2013-07-15 21.49, Junio C Hamano wrote:
>> Mark Levedahl <mlevedahl@gmail.com> writes:
>>
>>>> In order to limit the adverse effects caused by this implementation,
>>>> we provide a new "fast stat" interface, which allows us to use this
>>>> only for interactions with the index (i.e. the cached stat data).
>>>>
>>>> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>>>> ---
>>> I've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results
>>> using your prior patch (removing the Cygwin specific lstat entirely)
>>> and get the same results with both, so this seems ok from me.
>>>
>>> My comparison point was created by reverting your current patch from
>>> pu, then reapplying your earlier patch on top, so the only difference
>>> was which approach was used to address the stat functions.
>>>
>>> Caveats:
>>> 1) I don't find any speed improvement of the current patch over the
>>> previous one (the tests actually ran faster with the earlier patch,
>>> though the difference was less than 1%).
> Hm, measuring the time for the test suite is one thing,
> did you measure the time of "git status" with and without the patch?
>
> (I don't have my test system at hand, so I can test in a few days/weeks)
Timing for 5 rounds of "git status" in the git project. First, with the 
current fast_lstat patches:
/usr/local/src/git>for i in {1..5} ; do time git status >& /dev/null ; done

real    0m0.218s
user    0m0.000s
sys     0m0.218s

real    0m0.187s
user    0m0.077s
sys     0m0.109s

real    0m0.187s
user    0m0.030s
sys     0m0.156s

real    0m0.203s
user    0m0.031s
sys     0m0.171s

real    0m0.187s
user    0m0.062s
sys     0m0.124s

Now, with Ramsay's original patch just removing the non-Posix stat 
functions:
/usr/local/src/git>for i in {1..5} ; do time git status >& /dev/null ; done

real    0m0.218s
user    0m0.046s
sys     0m0.171s

real    0m0.187s
user    0m0.015s
sys     0m0.171s

real    0m0.187s
user    0m0.015s
sys     0m0.171s

real    0m0.187s
user    0m0.047s
sys     0m0.140s

real    0m0.187s
user    0m0.031s
sys     0m0.156s


I see no difference in the above. (Yes, I checked multiple times that I 
was using different executables).
>>> 2) I still question this whole approach, especially having this
>>> non-POSIX compliant mode be the default. Running in this mode breaks
>>> interoperability with Linux, but providing a Linux environment is the
>>> *primary* goal of Cygwin.
>> Sounds like we are better off without this patch, and instead remove
>> the "schizophrenic stat"?  I do not have a strong opinion either
>> way, except that I tend to agree with your point 2) above.
> My understanding is that we want both:
> Introduction of fast_lstat() as phase 1,
> and the removal of the "schizophrenic stat" in compat/cygwin.c
> as phase 2. (or do I missunderstand something ?)
>
>
> And yes, phase 3:
> The day we have a both reliable and fast
> lstat() in cygwin, we can remove compat/cygwin.[ch]
If you know how to make Cygwin's stat faster while maintaining Posix 
semantics, please post a patch to the Cygwin list, they would *love* it.

Mark

Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions

[Dmitry Potapov]

On Tue, Jul 16, 2013 at 7:54 AM, Mark Levedahl <mlevedahl@gmail.com> wrote:
>
> I see no difference in the above. (Yes, I checked multiple times that I was
> using different executables).

Are you sure that you set core.filemode to false before testing?

If you have core.filemode set to true then you _always_ use Cygwin stat,
so it does not make any difference for you.


Dmitry

Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions

[Mark Levedahl]

On 07/16/2013 11:42 AM, Dmitry Potapov wrote:
> On Tue, Jul 16, 2013 at 7:54 AM, Mark Levedahl <mlevedahl@gmail.com> wrote:
>> I see no difference in the above. (Yes, I checked multiple times that I was
>> using different executables).
> Are you sure that you set core.filemode to false before testing?
>
>
yes.

Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions

[Ramsay Jones]

Mark Levedahl wrote:
> On 07/15/2013 10:06 PM, Torsten Bögershausen wrote:
>> On 2013-07-15 21.49, Junio C Hamano wrote:
>>> Mark Levedahl <mlevedahl@gmail.com> writes:
>>>
>>>>> In order to limit the adverse effects caused by this implementation,
>>>>> we provide a new "fast stat" interface, which allows us to use this
>>>>> only for interactions with the index (i.e. the cached stat data).
>>>>>
>>>>> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>>>>> ---
>>>> I've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results
>>>> using your prior patch (removing the Cygwin specific lstat entirely)
>>>> and get the same results with both, so this seems ok from me.
>>>>
>>>> My comparison point was created by reverting your current patch from
>>>> pu, then reapplying your earlier patch on top, so the only difference
>>>> was which approach was used to address the stat functions.
>>>>
>>>> Caveats:
>>>> 1) I don't find any speed improvement of the current patch over the
>>>> previous one (the tests actually ran faster with the earlier patch,
>>>> though the difference was less than 1%).
>> Hm, measuring the time for the test suite is one thing,
>> did you measure the time of "git status" with and without the patch?
>>
>> (I don't have my test system at hand, so I can test in a few days/weeks)
> Timing for 5 rounds of "git status" in the git project. First, with the 
> current fast_lstat patches:
> /usr/local/src/git>for i in {1..5} ; do time git status >& /dev/null ; done
> 
> real    0m0.218s
> user    0m0.000s
> sys     0m0.218s
> 
> real    0m0.187s
> user    0m0.077s
> sys     0m0.109s
> 
> real    0m0.187s
> user    0m0.030s
> sys     0m0.156s
> 
> real    0m0.203s
> user    0m0.031s
> sys     0m0.171s
> 
> real    0m0.187s
> user    0m0.062s
> sys     0m0.124s
> 
> Now, with Ramsay's original patch just removing the non-Posix stat 
> functions:
> /usr/local/src/git>for i in {1..5} ; do time git status >& /dev/null ; done
> 
> real    0m0.218s
> user    0m0.046s
> sys     0m0.171s
> 
> real    0m0.187s
> user    0m0.015s
> sys     0m0.171s
> 
> real    0m0.187s
> user    0m0.015s
> sys     0m0.171s
> 
> real    0m0.187s
> user    0m0.047s
> sys     0m0.140s
> 
> real    0m0.187s
> user    0m0.031s
> sys     0m0.156s
> 
> 
> I see no difference in the above. (Yes, I checked multiple times that I 
> was using different executables).

Hmm, that looks good. :-D

Torsten reported a performance boost using the win32 stat() implementation
on a linux git repo (2s -> 1s, if I recall correctly) on cygwin 1.7.
Do you have a larger repo available to test?

If performance isn't an issue (it isn't for _me_), then I will happily
re-submit my original patch (removing the win32 stat implementation).

[Hmm, I may do anyway!]

ATB,
Ramsay Jones

Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions

[Torsten Bögershausen]

On 2013-07-18 19.50, Ramsay Jones wrote:
> Mark Levedahl wrote:
>> On 07/15/2013 10:06 PM, Torsten Bögershausen wrote:
>>> On 2013-07-15 21.49, Junio C Hamano wrote:
>>>> Mark Levedahl <mlevedahl@gmail.com> writes:
>>>>
>>>>>> In order to limit the adverse effects caused by this implementation,
>>>>>> we provide a new "fast stat" interface, which allows us to use this
>>>>>> only for interactions with the index (i.e. the cached stat data).
>>>>>>
>>>>>> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>>>>>> ---
>>>>> I've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results
>>>>> using your prior patch (removing the Cygwin specific lstat entirely)
>>>>> and get the same results with both, so this seems ok from me.
>>>>>
>>>>> My comparison point was created by reverting your current patch from
>>>>> pu, then reapplying your earlier patch on top, so the only difference
>>>>> was which approach was used to address the stat functions.
>>>>>
>>>>> Caveats:
>>>>> 1) I don't find any speed improvement of the current patch over the
>>>>> previous one (the tests actually ran faster with the earlier patch,
>>>>> though the difference was less than 1%).
>>> Hm, measuring the time for the test suite is one thing,
>>> did you measure the time of "git status" with and without the patch?
>>>
>>> (I don't have my test system at hand, so I can test in a few days/weeks)
>> Timing for 5 rounds of "git status" in the git project. First, with the 
>> current fast_lstat patches:
>> /usr/local/src/git>for i in {1..5} ; do time git status >& /dev/null ; done
>>
>> real    0m0.218s
>> user    0m0.000s
>> sys     0m0.218s
>>
>> real    0m0.187s
>> user    0m0.077s
>> sys     0m0.109s
>>
>> real    0m0.187s
>> user    0m0.030s
>> sys     0m0.156s
>>
>> real    0m0.203s
>> user    0m0.031s
>> sys     0m0.171s
>>
>> real    0m0.187s
>> user    0m0.062s
>> sys     0m0.124s
>>
>> Now, with Ramsay's original patch just removing the non-Posix stat 
>> functions:
>> /usr/local/src/git>for i in {1..5} ; do time git status >& /dev/null ; done
>>
>> real    0m0.218s
>> user    0m0.046s
>> sys     0m0.171s
>>
>> real    0m0.187s
>> user    0m0.015s
>> sys     0m0.171s
>>
>> real    0m0.187s
>> user    0m0.015s
>> sys     0m0.171s
>>
>> real    0m0.187s
>> user    0m0.047s
>> sys     0m0.140s
>>
>> real    0m0.187s
>> user    0m0.031s
>> sys     0m0.156s
>>
>>
>> I see no difference in the above. (Yes, I checked multiple times that I 
>> was using different executables).
> 
> Hmm, that looks good. :-D
> 
> Torsten reported a performance boost using the win32 stat() implementation
> on a linux git repo (2s -> 1s, if I recall correctly) on cygwin 1.7.
> Do you have a larger repo available to test?
(I have a 5 years old Dual Core, 2.5 Ghz, 1 TB hard disk, Win XP, cygwin 1.7)
On that machine I can see the performance boost.
Which kind of computers are you guys using?

SSD/hard disk ?
How much RAM ?
Which OS ?
Is there a difference between Win XP, Win7, Win8?

[snip]

Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions

[Mark Levedahl]

On 07/18/2013 05:49 PM, Torsten Bögershausen wrote:
> On 2013-07-18 19.50, Ramsay Jones wrote:
>>
>> Hmm, that looks good. :-D
>>
>> Torsten reported a performance boost using the win32 stat() implementation
>> on a linux git repo (2s -> 1s, if I recall correctly) on cygwin 1.7.
>> Do you have a larger repo available to test?
> (I have a 5 years old Dual Core, 2.5 Ghz, 1 TB hard disk, Win XP, cygwin 1.7)
> On that machine I can see the performance boost.
> Which kind of computers are you guys using?
>
> SSD/hard disk ?
> How much RAM ?
> Which OS ?
> Is there a difference between Win XP, Win7, Win8?
>
> [snip]
>
>
My previous results were from a Win 7 laptop, 2.7 GHz 2nd generation I7, 
8 Gig Ram, 250 GByte spinning rust drive, all formatted NTFS.

Here's some more results, running WinXP in VirtualBox on my older Linux 
laptop (2.5 GHz Penryn dual core, 500 GByte spinning rust, virtual file 
system is NTFS). First, results using Ramsay's last patch on pu adding 
the fast_lstat: Timing results are after first doing 5 'git status runs' 
to assure the cache is hot:

% using the fast_lstat and friends...
/usr/local/src/git>time git -c core.filemode=false status >& /dev/null

real    0m0.469s
user    0m0.062s
sys     0m0.436s
/usr/local/src/git>

/usr/local/src/git>time git -c core.filemode=true status >& /dev/null

real    0m0.719s
user    0m0.030s
sys     0m0.686s
/usr/local/src/git>

And now the same. but using Ramsay's first patch that removes all win32 
stat stuff and forces everything to go through Cygwin's normal stat/fstat:
% stat - with / without core.filemode, no win32 stats
/usr/local/src/git>time git -c core.filemode=false status >& /dev/null

real    0m0.328s
user    0m0.093s
sys     0m0.264s
/usr/local/src/git>

/usr/local/src/git>time git -c core.filemode=true status >& /dev/null

real    0m0.625s
user    0m0.124s
sys     0m0.500s
/usr/local/src/git>


Unlike the results on the fast Win7 laptop, the above show statistically 
significant slow down from the fast_lstat approach. I'm just not seeing 
a case for the special case handling, and of course Junio has already 
voted with his preference of removing the special case stuff as well.

Mark

Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> Unlike the results on the fast Win7 laptop, the above show
> statistically significant slow down from the fast_lstat approach. I'm
> just not seeing a case for the special case handling, and of course
> Junio has already voted with his preference of removing the special
> case stuff as well.

Please don't take what I said as any "vote" in this thread.  I do
not have a first-hand data to back anything up.

I was primarily trying to see my understanding of the consensus of
the thread was correct. If we can do without s/lstat/fast_lstat/
almost everywhere in the codebase, of course, I would be happier, as
it would give us one less thing to worry about.

If the assumptions like "they were declining minority and only lose
population over time", "it is easy for them to revert the removal
and keep going", and "removal will not hurt them too much in the
first place, only a few hundred milliseconds", that might trump the
longer-term maintainability issue, and we may end up having to carry
that win32 stat implementation a bit longer until these users all
switch to Cygwin 1.7, but judging from the "cvs binary seems to be
built incorrectly" incident the other day, it might be the case some
users still hesitate to update, fearing that 1.7 series may not be
solid enough, perhaps?

Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions

[Mark Levedahl]

On 07/18/2013 07:32 PM, Junio C Hamano wrote:
> Mark Levedahl <mlevedahl@gmail.com> writes:
>
>> Unlike the results on the fast Win7 laptop, the above show
>> statistically significant slow down from the fast_lstat approach. I'm
>> just not seeing a case for the special case handling, and of course
>> Junio has already voted with his preference of removing the special
>> case stuff as well.
> Please don't take what I said as any "vote" in this thread.  I do
> not have a first-hand data to back anything up.
>
> I was primarily trying to see my understanding of the consensus of
> the thread was correct. If we can do without s/lstat/fast_lstat/
> almost everywhere in the codebase, of course, I would be happier, as
> it would give us one less thing to worry about.
>
> If the assumptions like "they were declining minority and only lose
> population over time", "it is easy for them to revert the removal
> and keep going", and "removal will not hurt them too much in the
> first place, only a few hundred milliseconds", that might trump the
> longer-term maintainability issue, and we may end up having to carry
> that win32 stat implementation a bit longer until these users all
> switch to Cygwin 1.7, but judging from the "cvs binary seems to be
> built incorrectly" incident the other day, it might be the case some
> users still hesitate to update, fearing that 1.7 series may not be
> solid enough, perhaps?
>

I cannot say how many users of 1.5 exist. I see no evidence of 1.5 users 
on the Cygwin lists, the developers noted a total of 14 downloads of the 
1.5 installer in the year prior to removal of 1.5 from the mirrors. The 
stated reason for keeping 1.5 available for four years after its 
development stopped was support of older Windows variants (which 
Microsoft dropped support of before Cygwin did, BTW). But, none of this 
is conclusive about the current relevance of v 1.5.

The status as I understand things:
1) The existing schizophrenic stat on master is incompatible with the 
new reference api on pu, therefore some change is required.
2) Ramsay has graciously provided two separate patches to address the 
above, one reverting to use only of cygwin stat/lstat, one including a 
fast_lstat that should provide better speed at the expense of POSIX 
compliance.
3) We have conflicting reports about the speed of the second patch: 
Ramsay shows a good speed up on Cygwin 1.5, with slight performance 
regrets on MINGW, no change on Linux. I found no effect on a current 
bare-metal Window 7 installation using Cygwin 1.7, but degradation on a 
virtualized WinXP installation using Cygwin 1.7. Ramsay also showed a 
significant performance difference between running from the git tree vs 
being installed, I looked for this effect but failed to replicate it.

The maintenance argument between the two patches is clear, the 
performance argument less so. Perhaps others can help clarify this.

Mark

Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions

[Mark Levedahl]

On 07/15/2013 03:49 PM, Junio C Hamano wrote:
> Mark Levedahl <mlevedahl@gmail.com> writes:
>
>>> In order to limit the adverse effects caused by this implementation,
>>> we provide a new "fast stat" interface, which allows us to use this
>>> only for interactions with the index (i.e. the cached stat data).
>>>
>>> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>>> ---
>> I've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results
>> using your prior patch (removing the Cygwin specific lstat entirely)
>> and get the same results with both, so this seems ok from me.
>>
>> My comparison point was created by reverting your current patch from
>> pu, then reapplying your earlier patch on top, so the only difference
>> was which approach was used to address the stat functions.
>>
>> Caveats:
>> 1) I don't find any speed improvement of the current patch over the
>> previous one (the tests actually ran faster with the earlier patch,
>> though the difference was less than 1%).
>> 2) I still question this whole approach, especially having this
>> non-POSIX compliant mode be the default. Running in this mode breaks
>> interoperability with Linux, but providing a Linux environment is the
>> *primary* goal of Cygwin.
> Sounds like we are better off without this patch, and instead remove
> the "schizophrenic stat"?  I do not have a strong opinion either
> way, except that I tend to agree with your point 2) above.
>
In case my opinion is unclear, I think removal of the schizophrenic stat 
is the right approach. Speed is important, but not at the expense of 
correctness.

Mark

Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions

[Ramsay Jones]

Mark Levedahl wrote:
> On 07/10/2013 04:23 PM, Ramsay Jones wrote:
>> Commit adbc0b6b ("cygwin: Use native Win32 API for stat", 30-09-2008)
>> added a Win32 specific implementation of the stat functions. In order
>> to handle absolute paths, cygwin mount points and symbolic links, this
>> implementation may fall back on the standard cygwin l/stat() functions.
>> Also, the choice of cygwin or Win32 functions is made lazily (by the
>> first call(s) to l/stat) based on the state of some config variables.
>>
>> Unfortunately, this "schizophrenic stat" implementation has been the
>> source of many problems ever since. For example, see commits 7faee6b8,
>> 79748439, 452993c2, 085479e7, b8a97333, 924aaf3e, 05bab3ea and 0117c2f0.
>>
>> In order to limit the adverse effects caused by this implementation,
>> we provide a new "fast stat" interface, which allows us to use this
>> only for interactions with the index (i.e. the cached stat data).
>>
>> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>> ---
> 
> I've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results 
> using your prior patch (removing the Cygwin specific lstat entirely) and 
> get the same results with both, so this seems ok from me.

Thank you for testing this.

> My comparison point was created by reverting your current patch from pu, 
> then reapplying your earlier patch on top, so the only difference was 
> which approach was used to address the stat functions.
> 
> Caveats:
> 1) I don't find any speed improvement of the current patch over the 
> previous one (the tests actually ran faster with the earlier patch, 
> though the difference was less than 1%).
> 2) I still question this whole approach, especially having this 
> non-POSIX compliant mode be the default. Running in this mode breaks 
> interoperability with Linux, but providing a Linux environment is the 
> *primary* goal of Cygwin.

Yes, I agree. Since I _always_ disable the Win32 stat functions (by
setting core.filemode by hand - yes it's a little annoying), I don't
get any "benefit" from the added complexity.

However, I don't use git on cygwin with *large* repositories. git.git
is pretty much as large as it gets. So, the difference in performance
for me amounts to something like 0.120s -> 0.260s, which I can barely
notice.

Other people may not be so lucky ...

I would be happy if my original patch (removing the win32 stat funcs)
was applied, but others may not be. :-P

ATB,
Ramsay Jones

Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions

[Mark Levedahl]

On 07/16/2013 05:36 PM, Ramsay Jones wrote:
> Caveats:
> 1) I don't find any speed improvement of the current patch over the
> previous one (the tests actually ran faster with the earlier patch,
> though the difference was less than 1%).
> 2) I still question this whole approach, especially having this
> non-POSIX compliant mode be the default. Running in this mode breaks
> interoperability with Linux, but providing a Linux environment is the
> *primary* goal of Cygwin.
> Yes, I agree. Since I _always_ disable the Win32 stat functions (by
> setting core.filemode by hand - yes it's a little annoying), I don't
> get any "benefit" from the added complexity.
>
> However, I don't use git on cygwin with *large* repositories. git.git
> is pretty much as large as it gets. So, the difference in performance
> for me amounts to something like 0.120s -> 0.260s, which I can barely
> notice.
>
> Other people may not be so lucky ...
>
> I would be happy if my original patch (removing the win32 stat funcs)
> was applied, but others may not be. :-P
>
> ATB,
> Ramsay Jones
>
Cygwin 1.7 is very different than the earlier, no longer supported, and 
no longer available Cygwin variants in many ways, but stat is one of 
them. Cygwin 1.7 uses Windows ACLs to represent file permissions, and 
therefore gets the file permissions directly from the underlying OS 
calls. Earlier Cygwin versions (attempted to) overlay POSIX permissions 
on Windows systems using extended attributes and other means, and in 
many cases had to resort to opening the file and examining it to 
determine executability. This is not true in 1.7.

Therefore, your later patch would be expected to have much less benefit 
for 1.7 than for 1.5 (I don't detect *any* benefit on 1.7 when I set 
core.filemode=false). There are many choices, three are:

a) Remove the win32 stat funcs, eliminating all of the troublesome code 
paths and maintenance burden (your original patch).
b) Add your latest patch, with attendant complexity and maintenance 
burden, to support a version of Cygwin that is no longer available and 
was last updated over four years ago.
c) Like b, except make this triggered only by a "CYGWIN_15" macro, 
limiting this to use by the legacy cygwin platform.

I strongly vote for a, could support c, but fear b is just going to keep 
us chasing down bugs. Especially so when we consider that this patch can 
only speed things up when core.filemode=false, which mode:
a) causes git to fail its test suite.
b) breaks compatibility with Linux
c) violates the primary goal of the Cygwin project, which is to provide 
a Linux environment on Windows.

Mark

Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> Cygwin 1.7 is very different than the earlier, no longer supported,
> and no longer available Cygwin variants in many ways, but stat is one
> of them. Cygwin 1.7 uses Windows ACLs to represent file permissions,
> and therefore gets the file permissions directly from the underlying
> OS calls. Earlier Cygwin versions (attempted to) overlay POSIX
> permissions on Windows systems using extended attributes and other
> means, and in many cases had to resort to opening the file and
> examining it to determine executability. This is not true in 1.7.
>
> Therefore, your later patch would be expected to have much less
> benefit for 1.7 than for 1.5 (I don't detect *any* benefit on 1.7 when
> I set core.filemode=false). There are many choices, three are:
>
> a) Remove the win32 stat funcs, eliminating all of the troublesome
> code paths and maintenance burden (your original patch).
> b) Add your latest patch, with attendant complexity and maintenance
> burden, to support a version of Cygwin that is no longer available and
> was last updated over four years ago.
> c) Like b, except make this triggered only by a "CYGWIN_15" macro,
> limiting this to use by the legacy cygwin platform.

Let's do (a) in a single patch, then.

People who do want to keep running older Cygwin installation they
already have can revert the removal and rebuild Git, but the number
of people who have to do so will become only smaller over time if
older Cygwin versions are no longer available.

I presume that we _could_ add a CYGWIN_15 macro that conditionally
keeps the win32 lstat implementation and get_st_mode_bits() part,
and that might make it easier for folks with older Cygwin
installations, but I am not sure if it is worth it.

> I strongly vote for a, could support c, but fear b is just going to
> keep us chasing down bugs. Especially so when we consider that this
> patch can only speed things up when core.filemode=false, which mode:
> a) causes git to fail its test suite.
> b) breaks compatibility with Linux
> c) violates the primary goal of the Cygwin project, which is to
> provide a Linux environment on Windows.