{"thread":{"id":"34400","subject":"[RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions","startedAt":"2013-07-10T20:23:11Z","lastAt":"2013-07-19T15:34:05Z","messageCount":16,"participants":["Ramsay Jones","Mark Levedahl","Junio C Hamano","Torsten Bögershausen","Dmitry Potapov"],"isPatch":true,"patchVersion":2,"patchTotal":1},"messages":[{"id":"223038","messageId":"51DDC2AF.9010504@ramsay1.demon.co.uk","threadId":"34400","inReplyTo":null,"subject":"[RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-07-10T20:23:11Z","receivedAt":"2013-07-10T20:23:11Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nCommit adbc0b6b (\"cygwin: Use native Win32 API for stat\", 30-09-2008)\nadded a Win32 specific implementation of the stat functions. In order\nto handle absolute paths, cygwin mount points and symbolic links, this\nimplementation may fall back on the standard cygwin l/stat() functions.\nAlso, the choice of cygwin or Win32 functions is made lazily (by the\nfirst call(s) to l/stat) based on the state of some config variables.\n\nUnfortunately, this \"schizophrenic stat\" implementation has been the\nsource of many problems ever since. For example, see commits 7faee6b8,\n79748439, 452993c2, 085479e7, b8a97333, 924aaf3e, 05bab3ea and 0117c2f0.\n\nIn order to limit the adverse effects caused by this implementation,\nwe provide a new \"fast stat\" interface, which allows us to use this\nonly for interactions with the index (i.e. the cached stat data).\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n builtin/apply.c        |  8 ++++----\n builtin/commit.c       |  2 +-\n builtin/ls-files.c     |  2 +-\n builtin/rm.c           |  2 +-\n builtin/update-index.c |  2 +-\n check-racy.c           |  2 +-\n compat/cygwin.c        | 48 ++++++++++--------------------------------------\n compat/cygwin.h        | 17 +++++++++--------\n diff-lib.c             |  2 +-\n diff.c                 |  2 +-\n entry.c                |  6 +++---\n git-compat-util.h      | 27 +++++++++++++++++++++++++--\n help.c                 |  5 +----\n path.c                 |  9 +--------\n preload-index.c        |  2 +-\n read-cache.c           |  6 +++---\n unpack-trees.c         |  8 ++++----\n 17 files changed, 68 insertions(+), 82 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 0e9b631..f5046a6 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3091,7 +3091,7 @@ static int checkout_target(struct cache_entry *ce, struct stat *st)\n \tmemset(&costate, 0, sizeof(costate));\n \tcostate.base_dir = \"\";\n \tcostate.refresh_cache = 1;\n-\tif (checkout_entry(ce, &costate, NULL) || lstat(ce->name, st))\n+\tif (checkout_entry(ce, &costate, NULL) || fast_lstat(ce->name, st))\n \t\treturn error(_(\"cannot checkout %s\"), ce->name);\n \treturn 0;\n }\n@@ -3253,7 +3253,7 @@ static int load_current(struct image *image, struct patch *patch)\n \tif (pos < 0)\n \t\treturn error(_(\"%s: does not exist in index\"), name);\n \tce = active_cache[pos];\n-\tif (lstat(name, &st)) {\n+\tif (fast_lstat(name, &st)) {\n \t\tif (errno != ENOENT)\n \t\t\treturn error(_(\"%s: %s\"), name, strerror(errno));\n \t\tif (checkout_target(ce, &st))\n@@ -3396,7 +3396,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n \tif (previous) {\n \t\tst_mode = previous->new_mode;\n \t} else if (!cached) {\n-\t\tstat_ret = lstat(old_name, st);\n+\t\tstat_ret = fast_lstat(old_name, st);\n \t\tif (stat_ret && errno != ENOENT)\n \t\t\treturn error(_(\"%s: %s\"), old_name, strerror(errno));\n \t}\n@@ -3850,7 +3850,7 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned\n \t\t\tdie(_(\"corrupt patch for subproject %s\"), path);\n \t} else {\n \t\tif (!cached) {\n-\t\t\tif (lstat(path, &st) < 0)\n+\t\t\tif (fast_lstat(path, &st) < 0)\n \t\t\t\tdie_errno(_(\"unable to stat newly created file '%s'\"),\n \t\t\t\t\t  path);\n \t\t\tfill_stat_cache_info(ce, &st);\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 6b693c1..1d208c6 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -231,7 +231,7 @@ static void add_remove_files(struct string_list *list)\n \t\tif (p->util)\n \t\t\tcontinue;\n \n-\t\tif (!lstat(p->string, &st)) {\n+\t\tif (!fast_lstat(p->string, &st)) {\n \t\t\tif (add_to_cache(p->string, &st, 0))\n \t\t\t\tdie(_(\"updating files failed\"));\n \t\t} else\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 3a410c3..a066719 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -247,7 +247,7 @@ static void show_files(struct dir_struct *dir)\n \t\t\t\tcontinue;\n \t\t\tif (ce_skip_worktree(ce))\n \t\t\t\tcontinue;\n-\t\t\terr = lstat(ce->name, &st);\n+\t\t\terr = fast_lstat(ce->name, &st);\n \t\t\tif (show_deleted && err)\n \t\t\t\tshow_ce_entry(tag_removed, ce);\n \t\t\tif (show_modified && ce_modified(ce, &st, 0))\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 06025a2..4b783e7 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -143,7 +143,7 @@ static int check_local_mod(unsigned char *head, int index_only)\n \t\t}\n \t\tce = active_cache[pos];\n \n-\t\tif (lstat(ce->name, &st) < 0) {\n+\t\tif (fast_lstat(ce->name, &st) < 0) {\n \t\t\tif (errno != ENOENT && errno != ENOTDIR)\n \t\t\t\twarning(\"'%s': %s\", ce->name, strerror(errno));\n \t\t\t/* It already vanished from the working tree */\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 5c7762e..4790e4c 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -206,7 +206,7 @@ static int process_path(const char *path)\n \t * First things first: get the stat information, to decide\n \t * what to do about the pathname!\n \t */\n-\tif (lstat(path, &st) < 0)\n+\tif (fast_lstat(path, &st) < 0)\n \t\treturn process_lstat_error(path, errno);\n \n \tif (S_ISDIR(st.st_mode))\ndiff --git a/check-racy.c b/check-racy.c\nindex 00d92a1..6124355 100644\n--- a/check-racy.c\n+++ b/check-racy.c\n@@ -11,7 +11,7 @@ int main(int ac, char **av)\n \t\tstruct cache_entry *ce = active_cache[i];\n \t\tstruct stat st;\n \n-\t\tif (lstat(ce->name, &st)) {\n+\t\tif (fast_lstat(ce->name, &st)) {\n \t\t\terror(\"lstat(%s): %s\", ce->name, strerror(errno));\n \t\t\tcontinue;\n \t\t}\ndiff --git a/compat/cygwin.c b/compat/cygwin.c\nindex 91ce5d4..661d8f7 100644\n--- a/compat/cygwin.c\n+++ b/compat/cygwin.c\n@@ -1,4 +1,3 @@\n-#define CYGWIN_C\n #define WIN32_LEAN_AND_MEAN\n #include <sys/stat.h>\n #include <sys/errno.h>\n@@ -6,18 +5,6 @@\n #include \"../git-compat-util.h\"\n #include \"../cache.h\" /* to read configuration */\n \n-/*\n- * Return POSIX permission bits, regardless of core.ignorecygwinfstricks\n- */\n-int cygwin_get_st_mode_bits(const char *path, int *mode)\n-{\n-\tstruct stat st;\n-\tif (lstat(path, &st) < 0)\n-\t\treturn -1;\n-\t*mode = st.st_mode;\n-\treturn 0;\n-}\n-\n static inline void filetime_to_timespec(const FILETIME *ft, struct timespec *ts)\n {\n \tlong long winTime = ((long long)ft->dwHighDateTime << 32) +\n@@ -85,29 +72,23 @@ static int do_stat(const char *file_name, struct stat *buf, stat_fn_t cygstat)\n \treturn -1;\n }\n \n-/* We provide our own lstat/stat functions, since the provided Cygwin versions\n- * of these functions are too slow. These stat functions are tailored for Git's\n- * usage, and therefore they are not meant to be complete and correct emulation\n- * of lstat/stat functionality.\n+/* We provide our own lstat function, since the provided Cygwin versions of\n+ * the stat functions are too slow. This lstat function is tailored for Git's\n+ * usage, and therefore it is not meant to be complete and correct emulation\n+ * of lstat functionality.\n  */\n static int cygwin_lstat(const char *path, struct stat *buf)\n {\n \treturn do_stat(path, buf, lstat);\n }\n \n-static int cygwin_stat(const char *path, struct stat *buf)\n-{\n-\treturn do_stat(path, buf, stat);\n-}\n-\n-\n /*\n- * At start up, we are trying to determine whether Win32 API or cygwin stat\n- * functions should be used. The choice is determined by core.ignorecygwinfstricks.\n+ * At start up, we are trying to determine whether Win32 API or cygwin lstat\n+ * function should be used. The choice is determined by core.ignorecygwinfstricks.\n  * Reading this option is not always possible immediately as git_dir may\n- * not be set yet. So until it is set, use cygwin lstat/stat functions.\n+ * not be set yet. So until it is set, use the cygwin lstat function.\n  * However, if core.filemode is set, we must use the Cygwin posix\n- * stat/lstat as the Windows stat functions do not determine posix filemode.\n+ * lstat as the Windows lstat function does not determine posix filemode.\n  *\n  * Note that git_cygwin_config() does NOT call git_default_config() and this\n  * is deliberate.  Many commands read from config to establish initial\n@@ -130,28 +111,19 @@ static int git_cygwin_config(const char *var, const char *value, void *cb)\n static int init_stat(void)\n {\n \tif (have_git_dir() && git_config(git_cygwin_config,NULL)) {\n-\t\tif (!core_filemode && native_stat) {\n-\t\t\tcygwin_stat_fn = cygwin_stat;\n+\t\tif (!core_filemode && native_stat)\n \t\t\tcygwin_lstat_fn = cygwin_lstat;\n-\t\t} else {\n-\t\t\tcygwin_stat_fn = stat;\n+\t\telse\n \t\t\tcygwin_lstat_fn = lstat;\n-\t\t}\n \t\treturn 0;\n \t}\n \treturn 1;\n }\n \n-static int cygwin_stat_stub(const char *file_name, struct stat *buf)\n-{\n-\treturn (init_stat() ? stat : *cygwin_stat_fn)(file_name, buf);\n-}\n-\n static int cygwin_lstat_stub(const char *file_name, struct stat *buf)\n {\n \treturn (init_stat() ? lstat : *cygwin_lstat_fn)(file_name, buf);\n }\n \n-stat_fn_t cygwin_stat_fn = cygwin_stat_stub;\n stat_fn_t cygwin_lstat_fn = cygwin_lstat_stub;\n \ndiff --git a/compat/cygwin.h b/compat/cygwin.h\nindex c04965a..c5955cc 100644\n--- a/compat/cygwin.h\n+++ b/compat/cygwin.h\n@@ -2,13 +2,14 @@\n #include <sys/stat.h>\n \n typedef int (*stat_fn_t)(const char*, struct stat*);\n-extern stat_fn_t cygwin_stat_fn;\n extern stat_fn_t cygwin_lstat_fn;\n-int cygwin_get_st_mode_bits(const char *path, int *mode);\n \n-#define get_st_mode_bits(p,m) cygwin_get_st_mode_bits((p),(m))\n-#ifndef CYGWIN_C\n-/* cygwin.c needs the original lstat() */\n-#define stat(path, buf) (*cygwin_stat_fn)(path, buf)\n-#define lstat(path, buf) (*cygwin_lstat_fn)(path, buf)\n-#endif\n+/*\n+ * Note that the fast_fstat function should never actually\n+ * be called, since cygwin has the UNRELIABLE_FSTAT build\n+ * variable set. Currently, all calls to fast_fstat are\n+ * protected by 'fstat_is_reliable()'.\n+ */\n+#define fast_lstat(path, buf) (*cygwin_lstat_fn)(path, buf)\n+#define fast_fstat(fd, buf) fstat(fd, buf)\n+#define GIT_FAST_STAT\ndiff --git a/diff-lib.c b/diff-lib.c\nindex b6f4b21..401dab6 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -27,7 +27,7 @@\n  */\n static int check_removed(const struct cache_entry *ce, struct stat *st)\n {\n-\tif (lstat(ce->name, st) < 0) {\n+\tif (fast_lstat(ce->name, st) < 0) {\n \t\tif (errno != ENOENT && errno != ENOTDIR)\n \t\t\treturn -1;\n \t\treturn 1;\ndiff --git a/diff.c b/diff.c\nindex 208094f..212d3ff 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2642,7 +2642,7 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int\n \t * If ce matches the file in the work tree, we can reuse it.\n \t */\n \tif (ce_uptodate(ce) ||\n-\t    (!lstat(name, &st) && !ce_match_stat(ce, &st, 0)))\n+\t    (!fast_lstat(name, &st) && !ce_match_stat(ce, &st, 0)))\n \t\treturn 1;\n \n \treturn 0;\ndiff --git a/entry.c b/entry.c\nindex d7c131d..f746d2a 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -109,7 +109,7 @@ static int fstat_output(int fd, const struct checkout *state, struct stat *st)\n \t/* use fstat() only when path == ce->name */\n \tif (fstat_is_reliable() &&\n \t    state->refresh_cache && !state->base_dir_len) {\n-\t\tfstat(fd, st);\n+\t\tfast_fstat(fd, st);\n \t\treturn 1;\n \t}\n \treturn 0;\n@@ -210,7 +210,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n finish:\n \tif (state->refresh_cache) {\n \t\tif (!fstat_done)\n-\t\t\tlstat(ce->name, &st);\n+\t\t\tfast_lstat(ce->name, &st);\n \t\tfill_stat_cache_info(ce, &st);\n \t}\n \treturn 0;\n@@ -230,7 +230,7 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)\n \t\terrno = ENOENT;\n \t\treturn -1;\n \t}\n-\treturn lstat(path, st);\n+\treturn fast_lstat(path, st);\n }\n \n int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath)\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex ff193f4..092a571 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -129,8 +129,6 @@\n #include <poll.h>\n #endif\n \n-extern int get_st_mode_bits(const char *path, int *mode);\n-\n #if defined(__MINGW32__)\n /* pull in Windows compatibility stuff */\n #include \"compat/mingw.h\"\n@@ -179,6 +177,31 @@ typedef unsigned long uintptr_t;\n #endif\n #endif\n \n+#ifndef GIT_FAST_STAT\n+/*\n+ * The \"fast\" stat() variants are used to read the stat data from the\n+ * filesystem that is stored into the index and/or that is compared\n+ * with the cached stat data. In order to provide a fast implementation,\n+ * these functions may not provide meaningful data in all fields of the\n+ * stat structure and should not, therefore, be used for any other purpose.\n+ *\n+ * Platforms which have slow stat functions, which can also provide\n+ * such fast variants (e.g. cygwin), should include the external\n+ * declarations above and define the GIT_FAST_STAT macro.\n+ *\n+ * The static inline definitions below are for platforms which have\n+ * no need of such fast variants.\n+ */\n+static inline int fast_lstat(const char *path, struct stat *st)\n+{\n+\treturn lstat(path, st);\n+}\n+static inline int fast_fstat(int fd, struct stat *st)\n+{\n+\treturn fstat(fd, st);\n+}\n+#endif\n+\n /* used on Mac OS X */\n #ifdef PRECOMPOSE_UNICODE\n #include \"compat/precompose_utf8.h\"\ndiff --git a/help.c b/help.c\nindex 08c54ef..f068925 100644\n--- a/help.c\n+++ b/help.c\n@@ -107,10 +107,7 @@ static int is_executable(const char *name)\n \t    !S_ISREG(st.st_mode))\n \t\treturn 0;\n \n-#if defined(GIT_WINDOWS_NATIVE) || defined(__CYGWIN__)\n-#if defined(__CYGWIN__)\n-if ((st.st_mode & S_IXUSR) == 0)\n-#endif\n+#if defined(GIT_WINDOWS_NATIVE)\n {\t/* cannot trust the executable bit, peek into the file instead */\n \tchar buf[3] = { 0 };\n \tint n;\ndiff --git a/path.c b/path.c\nindex 04ff148..4e7fae4 100644\n--- a/path.c\n+++ b/path.c\n@@ -5,13 +5,7 @@\n #include \"strbuf.h\"\n #include \"string-list.h\"\n \n-#ifndef get_st_mode_bits\n-/*\n- * The replacement lstat(2) we use on Cygwin is incomplete and\n- * may return wrong permission bits. Most of the time we do not care,\n- * but the callsites of this wrapper do care.\n- */\n-int get_st_mode_bits(const char *path, int *mode)\n+static int get_st_mode_bits(const char *path, int *mode)\n {\n \tstruct stat st;\n \tif (lstat(path, &st) < 0)\n@@ -19,7 +13,6 @@ int get_st_mode_bits(const char *path, int *mode)\n \t*mode = st.st_mode;\n \treturn 0;\n }\n-#endif\n \n static char bad_path[] = \"/bad-path/\";\n \ndiff --git a/preload-index.c b/preload-index.c\nindex 49cb08d..1bece91 100644\n--- a/preload-index.c\n+++ b/preload-index.c\n@@ -57,7 +57,7 @@ static void *preload_thread(void *_data)\n \t\t\tcontinue;\n \t\tif (threaded_has_symlink_leading_path(&cache, ce->name, ce_namelen(ce)))\n \t\t\tcontinue;\n-\t\tif (lstat(ce->name, &st))\n+\t\tif (fast_lstat(ce->name, &st))\n \t\t\tcontinue;\n \t\tif (ie_match_stat(index, ce, &st, CE_MATCH_RACY_IS_DIRTY))\n \t\t\tcontinue;\ndiff --git a/read-cache.c b/read-cache.c\nindex d5201f9..ed33d9e 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -689,7 +689,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n int add_file_to_index(struct index_state *istate, const char *path, int flags)\n {\n \tstruct stat st;\n-\tif (lstat(path, &st))\n+\tif (fast_lstat(path, &st))\n \t\tdie_errno(\"unable to stat '%s'\", path);\n \treturn add_to_index(istate, path, &st, flags);\n }\n@@ -1049,7 +1049,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,\n \t\treturn ce;\n \t}\n \n-\tif (lstat(ce->name, &st) < 0) {\n+\tif (fast_lstat(ce->name, &st) < 0) {\n \t\tif (err)\n \t\t\t*err = errno;\n \t\treturn NULL;\n@@ -1635,7 +1635,7 @@ static void ce_smudge_racily_clean_entry(struct cache_entry *ce)\n \t */\n \tstruct stat st;\n \n-\tif (lstat(ce->name, &st) < 0)\n+\tif (fast_lstat(ce->name, &st) < 0)\n \t\treturn;\n \tif (ce_match_stat_basic(ce, &st))\n \t\treturn;\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex b27f2a6..1fe9b63 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1215,7 +1215,7 @@ static int verify_uptodate_1(const struct cache_entry *ce,\n \telse if (o->reset || ce_uptodate(ce))\n \t\treturn 0;\n \n-\tif (!lstat(ce->name, &st)) {\n+\tif (!fast_lstat(ce->name, &st)) {\n \t\tint flags = CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE;\n \t\tunsigned changed = ie_match_stat(o->src_index, ce, &st, flags);\n \t\tif (!changed)\n@@ -1432,13 +1432,13 @@ static int verify_absent_1(const struct cache_entry *ce,\n \t\tchar path[PATH_MAX + 1];\n \t\tmemcpy(path, ce->name, len);\n \t\tpath[len] = 0;\n-\t\tif (lstat(path, &st))\n+\t\tif (fast_lstat(path, &st))\n \t\t\treturn error(\"cannot stat '%s': %s\", path,\n \t\t\t\t\tstrerror(errno));\n \n \t\treturn check_ok_to_remove(path, len, DT_UNKNOWN, NULL, &st,\n \t\t\t\terror_type, o);\n-\t} else if (lstat(ce->name, &st)) {\n+\t} else if (fast_lstat(ce->name, &st)) {\n \t\tif (errno != ENOENT)\n \t\t\treturn error(\"cannot stat '%s': %s\", ce->name,\n \t\t\t\t     strerror(errno));\n@@ -1852,7 +1852,7 @@ int oneway_merge(const struct cache_entry * const *src,\n \t\tint update = 0;\n \t\tif (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {\n \t\t\tstruct stat st;\n-\t\t\tif (lstat(old->name, &st) ||\n+\t\t\tif (fast_lstat(old->name, &st) ||\n \t\t\t    ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))\n \t\t\t\tupdate |= CE_UPDATE;\n \t\t}\n-- \n1.8.3\n"},{"id":"223329","messageId":"51E2CE97.2040900@gmail.com","threadId":"34400","inReplyTo":"51DDC2AF.9010504@ramsay1.demon.co.uk","subject":"Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2013-07-14T16:15:19Z","receivedAt":"2013-07-14T16:15:19Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"On 07/10/2013 04:23 PM, Ramsay Jones wrote:\n> Commit adbc0b6b (\"cygwin: Use native Win32 API for stat\", 30-09-2008)\n> added a Win32 specific implementation of the stat functions. In order\n> to handle absolute paths, cygwin mount points and symbolic links, this\n> implementation may fall back on the standard cygwin l/stat() functions.\n> Also, the choice of cygwin or Win32 functions is made lazily (by the\n> first call(s) to l/stat) based on the state of some config variables.\n>\n> Unfortunately, this \"schizophrenic stat\" implementation has been the\n> source of many problems ever since. For example, see commits 7faee6b8,\n> 79748439, 452993c2, 085479e7, b8a97333, 924aaf3e, 05bab3ea and 0117c2f0.\n>\n> In order to limit the adverse effects caused by this implementation,\n> we provide a new \"fast stat\" interface, which allows us to use this\n> only for interactions with the index (i.e. the cached stat data).\n>\n> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n> ---\n\nI've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results \nusing your prior patch (removing the Cygwin specific lstat entirely) and \nget the same results with both, so this seems ok from me.\n\nMy comparison point was created by reverting your current patch from pu, \nthen reapplying your earlier patch on top, so the only difference was \nwhich approach was used to address the stat functions.\n\nCaveats:\n1) I don't find any speed improvement of the current patch over the \nprevious one (the tests actually ran faster with the earlier patch, \nthough the difference was less than 1%).\n2) I still question this whole approach, especially having this \nnon-POSIX compliant mode be the default. Running in this mode breaks \ninteroperability with Linux, but providing a Linux environment is the \n*primary* goal of Cygwin.\n\nMark\n"},{"id":"223479","messageId":"7vppuja9ip.fsf@alter.siamese.dyndns.org","threadId":"34400","inReplyTo":"51E2CE97.2040900@gmail.com","subject":"Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-15T19:49:34Z","receivedAt":"2013-07-15T19:49:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Levedahl <mlevedahl@gmail.com> writes:\n\n>> In order to limit the adverse effects caused by this implementation,\n>> we provide a new \"fast stat\" interface, which allows us to use this\n>> only for interactions with the index (i.e. the cached stat data).\n>>\n>> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n>> ---\n>\n> I've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results\n> using your prior patch (removing the Cygwin specific lstat entirely)\n> and get the same results with both, so this seems ok from me.\n>\n> My comparison point was created by reverting your current patch from\n> pu, then reapplying your earlier patch on top, so the only difference\n> was which approach was used to address the stat functions.\n>\n> Caveats:\n> 1) I don't find any speed improvement of the current patch over the\n> previous one (the tests actually ran faster with the earlier patch,\n> though the difference was less than 1%).\n> 2) I still question this whole approach, especially having this\n> non-POSIX compliant mode be the default. Running in this mode breaks\n> interoperability with Linux, but providing a Linux environment is the\n> *primary* goal of Cygwin.\n\nSounds like we are better off without this patch, and instead remove\nthe \"schizophrenic stat\"?  I do not have a strong opinion either\nway, except that I tend to agree with your point 2) above.\n"},{"id":"223495","messageId":"51E4AABD.9010701@web.de","threadId":"34400","inReplyTo":"7vppuja9ip.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-07-16T02:06:53Z","receivedAt":"2013-07-16T02:06:53Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2013-07-15 21.49, Junio C Hamano wrote:\n> Mark Levedahl <mlevedahl@gmail.com> writes:\n> \n>>> In order to limit the adverse effects caused by this implementation,\n>>> we provide a new \"fast stat\" interface, which allows us to use this\n>>> only for interactions with the index (i.e. the cached stat data).\n>>>\n>>> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n>>> ---\n>>\n>> I've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results\n>> using your prior patch (removing the Cygwin specific lstat entirely)\n>> and get the same results with both, so this seems ok from me.\n>>\n>> My comparison point was created by reverting your current patch from\n>> pu, then reapplying your earlier patch on top, so the only difference\n>> was which approach was used to address the stat functions.\n>>\n>> Caveats:\n>> 1) I don't find any speed improvement of the current patch over the\n>> previous one (the tests actually ran faster with the earlier patch,\n>> though the difference was less than 1%).\nHm, measuring the time for the test suite is one thing,\ndid you measure the time of \"git status\" with and without the patch?\n\n(I don't have my test system at hand, so I can test in a few days/weeks)\n\n>> 2) I still question this whole approach, especially having this\n>> non-POSIX compliant mode be the default. Running in this mode breaks\n>> interoperability with Linux, but providing a Linux environment is the\n>> *primary* goal of Cygwin.\n> \n> Sounds like we are better off without this patch, and instead remove\n> the \"schizophrenic stat\"?  I do not have a strong opinion either\n> way, except that I tend to agree with your point 2) above.\n\nMy understanding is that we want both:\nIntroduction of fast_lstat() as phase 1,\nand the removal of the \"schizophrenic stat\" in compat/cygwin.c\nas phase 2. (or do I missunderstand something ?)\n\n\nAnd yes, phase 3:\nThe day we have a both reliable and fast \nlstat() in cygwin, we can remove compat/cygwin.[ch]\n"},{"id":"223498","messageId":"51E4C1BA.4010509@gmail.com","threadId":"34400","inReplyTo":"7vppuja9ip.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2013-07-16T03:44:58Z","receivedAt":"2013-07-16T03:44:58Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"On 07/15/2013 03:49 PM, Junio C Hamano wrote:\n> Mark Levedahl <mlevedahl@gmail.com> writes:\n>\n>>> In order to limit the adverse effects caused by this implementation,\n>>> we provide a new \"fast stat\" interface, which allows us to use this\n>>> only for interactions with the index (i.e. the cached stat data).\n>>>\n>>> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n>>> ---\n>> I've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results\n>> using your prior patch (removing the Cygwin specific lstat entirely)\n>> and get the same results with both, so this seems ok from me.\n>>\n>> My comparison point was created by reverting your current patch from\n>> pu, then reapplying your earlier patch on top, so the only difference\n>> was which approach was used to address the stat functions.\n>>\n>> Caveats:\n>> 1) I don't find any speed improvement of the current patch over the\n>> previous one (the tests actually ran faster with the earlier patch,\n>> though the difference was less than 1%).\n>> 2) I still question this whole approach, especially having this\n>> non-POSIX compliant mode be the default. Running in this mode breaks\n>> interoperability with Linux, but providing a Linux environment is the\n>> *primary* goal of Cygwin.\n> Sounds like we are better off without this patch, and instead remove\n> the \"schizophrenic stat\"?  I do not have a strong opinion either\n> way, except that I tend to agree with your point 2) above.\n>\nIn case my opinion is unclear, I think removal of the schizophrenic stat \nis the right approach. Speed is important, but not at the expense of \ncorrectness.\n\nMark\n"},{"id":"223499","messageId":"51E4C400.6000009@gmail.com","threadId":"34400","inReplyTo":"51E4AABD.9010701@web.de","subject":"Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2013-07-16T03:54:40Z","receivedAt":"2013-07-16T03:54:40Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"On 07/15/2013 10:06 PM, Torsten Bögershausen wrote:\n> On 2013-07-15 21.49, Junio C Hamano wrote:\n>> Mark Levedahl <mlevedahl@gmail.com> writes:\n>>\n>>>> In order to limit the adverse effects caused by this implementation,\n>>>> we provide a new \"fast stat\" interface, which allows us to use this\n>>>> only for interactions with the index (i.e. the cached stat data).\n>>>>\n>>>> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n>>>> ---\n>>> I've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results\n>>> using your prior patch (removing the Cygwin specific lstat entirely)\n>>> and get the same results with both, so this seems ok from me.\n>>>\n>>> My comparison point was created by reverting your current patch from\n>>> pu, then reapplying your earlier patch on top, so the only difference\n>>> was which approach was used to address the stat functions.\n>>>\n>>> Caveats:\n>>> 1) I don't find any speed improvement of the current patch over the\n>>> previous one (the tests actually ran faster with the earlier patch,\n>>> though the difference was less than 1%).\n> Hm, measuring the time for the test suite is one thing,\n> did you measure the time of \"git status\" with and without the patch?\n>\n> (I don't have my test system at hand, so I can test in a few days/weeks)\nTiming for 5 rounds of \"git status\" in the git project. First, with the \ncurrent fast_lstat patches:\n/usr/local/src/git>for i in {1..5} ; do time git status >& /dev/null ; done\n\nreal    0m0.218s\nuser    0m0.000s\nsys     0m0.218s\n\nreal    0m0.187s\nuser    0m0.077s\nsys     0m0.109s\n\nreal    0m0.187s\nuser    0m0.030s\nsys     0m0.156s\n\nreal    0m0.203s\nuser    0m0.031s\nsys     0m0.171s\n\nreal    0m0.187s\nuser    0m0.062s\nsys     0m0.124s\n\nNow, with Ramsay's original patch just removing the non-Posix stat \nfunctions:\n/usr/local/src/git>for i in {1..5} ; do time git status >& /dev/null ; done\n\nreal    0m0.218s\nuser    0m0.046s\nsys     0m0.171s\n\nreal    0m0.187s\nuser    0m0.015s\nsys     0m0.171s\n\nreal    0m0.187s\nuser    0m0.015s\nsys     0m0.171s\n\nreal    0m0.187s\nuser    0m0.047s\nsys     0m0.140s\n\nreal    0m0.187s\nuser    0m0.031s\nsys     0m0.156s\n\n\nI see no difference in the above. (Yes, I checked multiple times that I \nwas using different executables).\n>>> 2) I still question this whole approach, especially having this\n>>> non-POSIX compliant mode be the default. Running in this mode breaks\n>>> interoperability with Linux, but providing a Linux environment is the\n>>> *primary* goal of Cygwin.\n>> Sounds like we are better off without this patch, and instead remove\n>> the \"schizophrenic stat\"?  I do not have a strong opinion either\n>> way, except that I tend to agree with your point 2) above.\n> My understanding is that we want both:\n> Introduction of fast_lstat() as phase 1,\n> and the removal of the \"schizophrenic stat\" in compat/cygwin.c\n> as phase 2. (or do I missunderstand something ?)\n>\n>\n> And yes, phase 3:\n> The day we have a both reliable and fast\n> lstat() in cygwin, we can remove compat/cygwin.[ch]\nIf you know how to make Cygwin's stat faster while maintaining Posix \nsemantics, please post a patch to the Cygwin list, they would *love* it.\n\nMark\n"},{"id":"223524","messageId":"CAHkcotjV6BgFP9Z9eeFSU_X2vPiKV=2_D_fnk-jA48d_OCO33Q@mail.gmail.com","threadId":"34400","inReplyTo":"51E4C400.6000009@gmail.com","subject":"Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2013-07-16T15:42:06Z","receivedAt":"2013-07-16T15:42:06Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Tue, Jul 16, 2013 at 7:54 AM, Mark Levedahl <mlevedahl@gmail.com> wrote:\n>\n> I see no difference in the above. (Yes, I checked multiple times that I was\n> using different executables).\n\nAre you sure that you set core.filemode to false before testing?\n\nIf you have core.filemode set to true then you _always_ use Cygwin stat,\nso it does not make any difference for you.\n\n\nDmitry\n"},{"id":"223548","messageId":"51E5BCDF.5070004@ramsay1.demon.co.uk","threadId":"34400","inReplyTo":"51E2CE97.2040900@gmail.com","subject":"Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-07-16T21:36:31Z","receivedAt":"2013-07-16T21:36:31Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Mark Levedahl wrote:\n> On 07/10/2013 04:23 PM, Ramsay Jones wrote:\n>> Commit adbc0b6b (\"cygwin: Use native Win32 API for stat\", 30-09-2008)\n>> added a Win32 specific implementation of the stat functions. In order\n>> to handle absolute paths, cygwin mount points and symbolic links, this\n>> implementation may fall back on the standard cygwin l/stat() functions.\n>> Also, the choice of cygwin or Win32 functions is made lazily (by the\n>> first call(s) to l/stat) based on the state of some config variables.\n>>\n>> Unfortunately, this \"schizophrenic stat\" implementation has been the\n>> source of many problems ever since. For example, see commits 7faee6b8,\n>> 79748439, 452993c2, 085479e7, b8a97333, 924aaf3e, 05bab3ea and 0117c2f0.\n>>\n>> In order to limit the adverse effects caused by this implementation,\n>> we provide a new \"fast stat\" interface, which allows us to use this\n>> only for interactions with the index (i.e. the cached stat data).\n>>\n>> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n>> ---\n> \n> I've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results \n> using your prior patch (removing the Cygwin specific lstat entirely) and \n> get the same results with both, so this seems ok from me.\n\nThank you for testing this.\n\n> My comparison point was created by reverting your current patch from pu, \n> then reapplying your earlier patch on top, so the only difference was \n> which approach was used to address the stat functions.\n> \n> Caveats:\n> 1) I don't find any speed improvement of the current patch over the \n> previous one (the tests actually ran faster with the earlier patch, \n> though the difference was less than 1%).\n> 2) I still question this whole approach, especially having this \n> non-POSIX compliant mode be the default. Running in this mode breaks \n> interoperability with Linux, but providing a Linux environment is the \n> *primary* goal of Cygwin.\n\nYes, I agree. Since I _always_ disable the Win32 stat functions (by\nsetting core.filemode by hand - yes it's a little annoying), I don't\nget any \"benefit\" from the added complexity.\n\nHowever, I don't use git on cygwin with *large* repositories. git.git\nis pretty much as large as it gets. So, the difference in performance\nfor me amounts to something like 0.120s -> 0.260s, which I can barely\nnotice.\n\nOther people may not be so lucky ...\n\nI would be happy if my original patch (removing the win32 stat funcs)\nwas applied, but others may not be. :-P\n\nATB,\nRamsay Jones\n"},{"id":"223553","messageId":"51E5CEC0.4090605@gmail.com","threadId":"34400","inReplyTo":"CAHkcotjV6BgFP9Z9eeFSU_X2vPiKV=2_D_fnk-jA48d_OCO33Q@mail.gmail.com","subject":"Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2013-07-16T22:52:48Z","receivedAt":"2013-07-16T22:52:48Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"On 07/16/2013 11:42 AM, Dmitry Potapov wrote:\n> On Tue, Jul 16, 2013 at 7:54 AM, Mark Levedahl <mlevedahl@gmail.com> wrote:\n>> I see no difference in the above. (Yes, I checked multiple times that I was\n>> using different executables).\n> Are you sure that you set core.filemode to false before testing?\n>\n>\nyes.\n"},{"id":"223554","messageId":"51E5D38E.6080202@gmail.com","threadId":"34400","inReplyTo":"51E5BCDF.5070004@ramsay1.demon.co.uk","subject":"Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2013-07-16T23:13:18Z","receivedAt":"2013-07-16T23:13:18Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"On 07/16/2013 05:36 PM, Ramsay Jones wrote:\n> Caveats:\n> 1) I don't find any speed improvement of the current patch over the\n> previous one (the tests actually ran faster with the earlier patch,\n> though the difference was less than 1%).\n> 2) I still question this whole approach, especially having this\n> non-POSIX compliant mode be the default. Running in this mode breaks\n> interoperability with Linux, but providing a Linux environment is the\n> *primary* goal of Cygwin.\n> Yes, I agree. Since I _always_ disable the Win32 stat functions (by\n> setting core.filemode by hand - yes it's a little annoying), I don't\n> get any \"benefit\" from the added complexity.\n>\n> However, I don't use git on cygwin with *large* repositories. git.git\n> is pretty much as large as it gets. So, the difference in performance\n> for me amounts to something like 0.120s -> 0.260s, which I can barely\n> notice.\n>\n> Other people may not be so lucky ...\n>\n> I would be happy if my original patch (removing the win32 stat funcs)\n> was applied, but others may not be. :-P\n>\n> ATB,\n> Ramsay Jones\n>\nCygwin 1.7 is very different than the earlier, no longer supported, and \nno longer available Cygwin variants in many ways, but stat is one of \nthem. Cygwin 1.7 uses Windows ACLs to represent file permissions, and \ntherefore gets the file permissions directly from the underlying OS \ncalls. Earlier Cygwin versions (attempted to) overlay POSIX permissions \non Windows systems using extended attributes and other means, and in \nmany cases had to resort to opening the file and examining it to \ndetermine executability. This is not true in 1.7.\n\nTherefore, your later patch would be expected to have much less benefit \nfor 1.7 than for 1.5 (I don't detect *any* benefit on 1.7 when I set \ncore.filemode=false). There are many choices, three are:\n\na) Remove the win32 stat funcs, eliminating all of the troublesome code \npaths and maintenance burden (your original patch).\nb) Add your latest patch, with attendant complexity and maintenance \nburden, to support a version of Cygwin that is no longer available and \nwas last updated over four years ago.\nc) Like b, except make this triggered only by a \"CYGWIN_15\" macro, \nlimiting this to use by the legacy cygwin platform.\n\nI strongly vote for a, could support c, but fear b is just going to keep \nus chasing down bugs. Especially so when we consider that this patch can \nonly speed things up when core.filemode=false, which mode:\na) causes git to fail its test suite.\nb) breaks compatibility with Linux\nc) violates the primary goal of the Cygwin project, which is to provide \na Linux environment on Windows.\n\nMark\n"},{"id":"223668","messageId":"7v8v134vc6.fsf@alter.siamese.dyndns.org","threadId":"34400","inReplyTo":"51E5D38E.6080202@gmail.com","subject":"Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-18T17:43:53Z","receivedAt":"2013-07-18T17:43:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Levedahl <mlevedahl@gmail.com> writes:\n\n> Cygwin 1.7 is very different than the earlier, no longer supported,\n> and no longer available Cygwin variants in many ways, but stat is one\n> of them. Cygwin 1.7 uses Windows ACLs to represent file permissions,\n> and therefore gets the file permissions directly from the underlying\n> OS calls. Earlier Cygwin versions (attempted to) overlay POSIX\n> permissions on Windows systems using extended attributes and other\n> means, and in many cases had to resort to opening the file and\n> examining it to determine executability. This is not true in 1.7.\n>\n> Therefore, your later patch would be expected to have much less\n> benefit for 1.7 than for 1.5 (I don't detect *any* benefit on 1.7 when\n> I set core.filemode=false). There are many choices, three are:\n>\n> a) Remove the win32 stat funcs, eliminating all of the troublesome\n> code paths and maintenance burden (your original patch).\n> b) Add your latest patch, with attendant complexity and maintenance\n> burden, to support a version of Cygwin that is no longer available and\n> was last updated over four years ago.\n> c) Like b, except make this triggered only by a \"CYGWIN_15\" macro,\n> limiting this to use by the legacy cygwin platform.\n\nLet's do (a) in a single patch, then.\n\nPeople who do want to keep running older Cygwin installation they\nalready have can revert the removal and rebuild Git, but the number\nof people who have to do so will become only smaller over time if\nolder Cygwin versions are no longer available.\n\nI presume that we _could_ add a CYGWIN_15 macro that conditionally\nkeeps the win32 lstat implementation and get_st_mode_bits() part,\nand that might make it easier for folks with older Cygwin\ninstallations, but I am not sure if it is worth it.\n\n> I strongly vote for a, could support c, but fear b is just going to\n> keep us chasing down bugs. Especially so when we consider that this\n> patch can only speed things up when core.filemode=false, which mode:\n> a) causes git to fail its test suite.\n> b) breaks compatibility with Linux\n> c) violates the primary goal of the Cygwin project, which is to\n> provide a Linux environment on Windows.\n"},{"id":"223689","messageId":"51E82AE0.9050707@ramsay1.demon.co.uk","threadId":"34400","inReplyTo":"51E4C400.6000009@gmail.com","subject":"Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-07-18T17:50:24Z","receivedAt":"2013-07-18T17:50:24Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Mark Levedahl wrote:\n> On 07/15/2013 10:06 PM, Torsten Bögershausen wrote:\n>> On 2013-07-15 21.49, Junio C Hamano wrote:\n>>> Mark Levedahl <mlevedahl@gmail.com> writes:\n>>>\n>>>>> In order to limit the adverse effects caused by this implementation,\n>>>>> we provide a new \"fast stat\" interface, which allows us to use this\n>>>>> only for interactions with the index (i.e. the cached stat data).\n>>>>>\n>>>>> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n>>>>> ---\n>>>> I've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results\n>>>> using your prior patch (removing the Cygwin specific lstat entirely)\n>>>> and get the same results with both, so this seems ok from me.\n>>>>\n>>>> My comparison point was created by reverting your current patch from\n>>>> pu, then reapplying your earlier patch on top, so the only difference\n>>>> was which approach was used to address the stat functions.\n>>>>\n>>>> Caveats:\n>>>> 1) I don't find any speed improvement of the current patch over the\n>>>> previous one (the tests actually ran faster with the earlier patch,\n>>>> though the difference was less than 1%).\n>> Hm, measuring the time for the test suite is one thing,\n>> did you measure the time of \"git status\" with and without the patch?\n>>\n>> (I don't have my test system at hand, so I can test in a few days/weeks)\n> Timing for 5 rounds of \"git status\" in the git project. First, with the \n> current fast_lstat patches:\n> /usr/local/src/git>for i in {1..5} ; do time git status >& /dev/null ; done\n> \n> real    0m0.218s\n> user    0m0.000s\n> sys     0m0.218s\n> \n> real    0m0.187s\n> user    0m0.077s\n> sys     0m0.109s\n> \n> real    0m0.187s\n> user    0m0.030s\n> sys     0m0.156s\n> \n> real    0m0.203s\n> user    0m0.031s\n> sys     0m0.171s\n> \n> real    0m0.187s\n> user    0m0.062s\n> sys     0m0.124s\n> \n> Now, with Ramsay's original patch just removing the non-Posix stat \n> functions:\n> /usr/local/src/git>for i in {1..5} ; do time git status >& /dev/null ; done\n> \n> real    0m0.218s\n> user    0m0.046s\n> sys     0m0.171s\n> \n> real    0m0.187s\n> user    0m0.015s\n> sys     0m0.171s\n> \n> real    0m0.187s\n> user    0m0.015s\n> sys     0m0.171s\n> \n> real    0m0.187s\n> user    0m0.047s\n> sys     0m0.140s\n> \n> real    0m0.187s\n> user    0m0.031s\n> sys     0m0.156s\n> \n> \n> I see no difference in the above. (Yes, I checked multiple times that I \n> was using different executables).\n\nHmm, that looks good. :-D\n\nTorsten reported a performance boost using the win32 stat() implementation\non a linux git repo (2s -> 1s, if I recall correctly) on cygwin 1.7.\nDo you have a larger repo available to test?\n\nIf performance isn't an issue (it isn't for _me_), then I will happily\nre-submit my original patch (removing the win32 stat implementation).\n\n[Hmm, I may do anyway!]\n\nATB,\nRamsay Jones\n"},{"id":"223711","messageId":"51E862FC.4090607@web.de","threadId":"34400","inReplyTo":"51E82AE0.9050707@ramsay1.demon.co.uk","subject":"Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-07-18T21:49:48Z","receivedAt":"2013-07-18T21:49:48Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2013-07-18 19.50, Ramsay Jones wrote:\n> Mark Levedahl wrote:\n>> On 07/15/2013 10:06 PM, Torsten Bögershausen wrote:\n>>> On 2013-07-15 21.49, Junio C Hamano wrote:\n>>>> Mark Levedahl <mlevedahl@gmail.com> writes:\n>>>>\n>>>>>> In order to limit the adverse effects caused by this implementation,\n>>>>>> we provide a new \"fast stat\" interface, which allows us to use this\n>>>>>> only for interactions with the index (i.e. the cached stat data).\n>>>>>>\n>>>>>> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n>>>>>> ---\n>>>>> I've tested this on Cygwin 1.7 on WIndows 7 , comparing to the results\n>>>>> using your prior patch (removing the Cygwin specific lstat entirely)\n>>>>> and get the same results with both, so this seems ok from me.\n>>>>>\n>>>>> My comparison point was created by reverting your current patch from\n>>>>> pu, then reapplying your earlier patch on top, so the only difference\n>>>>> was which approach was used to address the stat functions.\n>>>>>\n>>>>> Caveats:\n>>>>> 1) I don't find any speed improvement of the current patch over the\n>>>>> previous one (the tests actually ran faster with the earlier patch,\n>>>>> though the difference was less than 1%).\n>>> Hm, measuring the time for the test suite is one thing,\n>>> did you measure the time of \"git status\" with and without the patch?\n>>>\n>>> (I don't have my test system at hand, so I can test in a few days/weeks)\n>> Timing for 5 rounds of \"git status\" in the git project. First, with the \n>> current fast_lstat patches:\n>> /usr/local/src/git>for i in {1..5} ; do time git status >& /dev/null ; done\n>>\n>> real    0m0.218s\n>> user    0m0.000s\n>> sys     0m0.218s\n>>\n>> real    0m0.187s\n>> user    0m0.077s\n>> sys     0m0.109s\n>>\n>> real    0m0.187s\n>> user    0m0.030s\n>> sys     0m0.156s\n>>\n>> real    0m0.203s\n>> user    0m0.031s\n>> sys     0m0.171s\n>>\n>> real    0m0.187s\n>> user    0m0.062s\n>> sys     0m0.124s\n>>\n>> Now, with Ramsay's original patch just removing the non-Posix stat \n>> functions:\n>> /usr/local/src/git>for i in {1..5} ; do time git status >& /dev/null ; done\n>>\n>> real    0m0.218s\n>> user    0m0.046s\n>> sys     0m0.171s\n>>\n>> real    0m0.187s\n>> user    0m0.015s\n>> sys     0m0.171s\n>>\n>> real    0m0.187s\n>> user    0m0.015s\n>> sys     0m0.171s\n>>\n>> real    0m0.187s\n>> user    0m0.047s\n>> sys     0m0.140s\n>>\n>> real    0m0.187s\n>> user    0m0.031s\n>> sys     0m0.156s\n>>\n>>\n>> I see no difference in the above. (Yes, I checked multiple times that I \n>> was using different executables).\n> \n> Hmm, that looks good. :-D\n> \n> Torsten reported a performance boost using the win32 stat() implementation\n> on a linux git repo (2s -> 1s, if I recall correctly) on cygwin 1.7.\n> Do you have a larger repo available to test?\n(I have a 5 years old Dual Core, 2.5 Ghz, 1 TB hard disk, Win XP, cygwin 1.7)\nOn that machine I can see the performance boost.\nWhich kind of computers are you guys using?\n\nSSD/hard disk ?\nHow much RAM ?\nWhich OS ?\nIs there a difference between Win XP, Win7, Win8?\n\n[snip]\n"},{"id":"223716","messageId":"51E86E02.4060208@gmail.com","threadId":"34400","inReplyTo":"51E862FC.4090607@web.de","subject":"Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2013-07-18T22:36:50Z","receivedAt":"2013-07-18T22:36:50Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"On 07/18/2013 05:49 PM, Torsten Bögershausen wrote:\n> On 2013-07-18 19.50, Ramsay Jones wrote:\n>>\n>> Hmm, that looks good. :-D\n>>\n>> Torsten reported a performance boost using the win32 stat() implementation\n>> on a linux git repo (2s -> 1s, if I recall correctly) on cygwin 1.7.\n>> Do you have a larger repo available to test?\n> (I have a 5 years old Dual Core, 2.5 Ghz, 1 TB hard disk, Win XP, cygwin 1.7)\n> On that machine I can see the performance boost.\n> Which kind of computers are you guys using?\n>\n> SSD/hard disk ?\n> How much RAM ?\n> Which OS ?\n> Is there a difference between Win XP, Win7, Win8?\n>\n> [snip]\n>\n>\nMy previous results were from a Win 7 laptop, 2.7 GHz 2nd generation I7, \n8 Gig Ram, 250 GByte spinning rust drive, all formatted NTFS.\n\nHere's some more results, running WinXP in VirtualBox on my older Linux \nlaptop (2.5 GHz Penryn dual core, 500 GByte spinning rust, virtual file \nsystem is NTFS). First, results using Ramsay's last patch on pu adding \nthe fast_lstat: Timing results are after first doing 5 'git status runs' \nto assure the cache is hot:\n\n% using the fast_lstat and friends...\n/usr/local/src/git>time git -c core.filemode=false status >& /dev/null\n\nreal    0m0.469s\nuser    0m0.062s\nsys     0m0.436s\n/usr/local/src/git>\n\n/usr/local/src/git>time git -c core.filemode=true status >& /dev/null\n\nreal    0m0.719s\nuser    0m0.030s\nsys     0m0.686s\n/usr/local/src/git>\n\nAnd now the same. but using Ramsay's first patch that removes all win32 \nstat stuff and forces everything to go through Cygwin's normal stat/fstat:\n% stat - with / without core.filemode, no win32 stats\n/usr/local/src/git>time git -c core.filemode=false status >& /dev/null\n\nreal    0m0.328s\nuser    0m0.093s\nsys     0m0.264s\n/usr/local/src/git>\n\n/usr/local/src/git>time git -c core.filemode=true status >& /dev/null\n\nreal    0m0.625s\nuser    0m0.124s\nsys     0m0.500s\n/usr/local/src/git>\n\n\nUnlike the results on the fast Win7 laptop, the above show statistically \nsignificant slow down from the fast_lstat approach. I'm just not seeing \na case for the special case handling, and of course Junio has already \nvoted with his preference of removing the special case stuff as well.\n\nMark\n"},{"id":"223723","messageId":"7vd2qf1m2s.fsf@alter.siamese.dyndns.org","threadId":"34400","inReplyTo":"51E86E02.4060208@gmail.com","subject":"Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-18T23:32:11Z","receivedAt":"2013-07-18T23:32:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Levedahl <mlevedahl@gmail.com> writes:\n\n> Unlike the results on the fast Win7 laptop, the above show\n> statistically significant slow down from the fast_lstat approach. I'm\n> just not seeing a case for the special case handling, and of course\n> Junio has already voted with his preference of removing the special\n> case stuff as well.\n\nPlease don't take what I said as any \"vote\" in this thread.  I do\nnot have a first-hand data to back anything up.\n\nI was primarily trying to see my understanding of the consensus of\nthe thread was correct. If we can do without s/lstat/fast_lstat/\nalmost everywhere in the codebase, of course, I would be happier, as\nit would give us one less thing to worry about.\n\nIf the assumptions like \"they were declining minority and only lose\npopulation over time\", \"it is easy for them to revert the removal\nand keep going\", and \"removal will not hurt them too much in the\nfirst place, only a few hundred milliseconds\", that might trump the\nlonger-term maintainability issue, and we may end up having to carry\nthat win32 stat implementation a bit longer until these users all\nswitch to Cygwin 1.7, but judging from the \"cvs binary seems to be\nbuilt incorrectly\" incident the other day, it might be the case some\nusers still hesitate to update, fearing that 1.7 series may not be\nsolid enough, perhaps?\n"},{"id":"223771","messageId":"51E95C6D.9030806@gmail.com","threadId":"34400","inReplyTo":"7vd2qf1m2s.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH v2 1/1] cygwin: Add fast_lstat() and fast_fstat() functions","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2013-07-19T15:34:05Z","receivedAt":"2013-07-19T15:34:05Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"On 07/18/2013 07:32 PM, Junio C Hamano wrote:\n> Mark Levedahl <mlevedahl@gmail.com> writes:\n>\n>> Unlike the results on the fast Win7 laptop, the above show\n>> statistically significant slow down from the fast_lstat approach. I'm\n>> just not seeing a case for the special case handling, and of course\n>> Junio has already voted with his preference of removing the special\n>> case stuff as well.\n> Please don't take what I said as any \"vote\" in this thread.  I do\n> not have a first-hand data to back anything up.\n>\n> I was primarily trying to see my understanding of the consensus of\n> the thread was correct. If we can do without s/lstat/fast_lstat/\n> almost everywhere in the codebase, of course, I would be happier, as\n> it would give us one less thing to worry about.\n>\n> If the assumptions like \"they were declining minority and only lose\n> population over time\", \"it is easy for them to revert the removal\n> and keep going\", and \"removal will not hurt them too much in the\n> first place, only a few hundred milliseconds\", that might trump the\n> longer-term maintainability issue, and we may end up having to carry\n> that win32 stat implementation a bit longer until these users all\n> switch to Cygwin 1.7, but judging from the \"cvs binary seems to be\n> built incorrectly\" incident the other day, it might be the case some\n> users still hesitate to update, fearing that 1.7 series may not be\n> solid enough, perhaps?\n>\n\nI cannot say how many users of 1.5 exist. I see no evidence of 1.5 users \non the Cygwin lists, the developers noted a total of 14 downloads of the \n1.5 installer in the year prior to removal of 1.5 from the mirrors. The \nstated reason for keeping 1.5 available for four years after its \ndevelopment stopped was support of older Windows variants (which \nMicrosoft dropped support of before Cygwin did, BTW). But, none of this \nis conclusive about the current relevance of v 1.5.\n\nThe status as I understand things:\n1) The existing schizophrenic stat on master is incompatible with the \nnew reference api on pu, therefore some change is required.\n2) Ramsay has graciously provided two separate patches to address the \nabove, one reverting to use only of cygwin stat/lstat, one including a \nfast_lstat that should provide better speed at the expense of POSIX \ncompliance.\n3) We have conflicting reports about the speed of the second patch: \nRamsay shows a good speed up on Cygwin 1.5, with slight performance \nregrets on MINGW, no change on Linux. I found no effect on a current \nbare-metal Window 7 installation using Cygwin 1.7, but degradation on a \nvirtualized WinXP installation using Cygwin 1.7. Ramsay also showed a \nsignificant performance difference between running from the git tree vs \nbeing installed, I looked for this effect but failed to replicate it.\n\nThe maintenance argument between the two patches is clear, the \nperformance argument less so. Perhaps others can help clarify this.\n\nMark\n"}]}