{"thread":{"id":"20041","subject":"Too many 'stat' calls by git-status on Windows","startedAt":"2009-07-07T00:05:01Z","lastAt":"2009-07-12T21:33:36Z","messageCount":39,"participants":["Dmitry Potapov","Ramsay Jones","Linus Torvalds","Junio C Hamano","Eric Blake","Paolo Bonzini","Kjetil Barvik"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"117530","messageId":"20090707000500.GA5594@dpotapov.dyndns.org","threadId":"20041","inReplyTo":null,"subject":"Too many 'stat' calls by git-status on Windows","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2009-07-07T00:05:01Z","receivedAt":"2009-07-07T00:05:01Z","isPatch":false,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"I have used the Cygwin version of Git on one Windows computer and\nnoticed that git-status is sluggish. So, I have run the Process Monitor\nto see what is going on.\n\nThe below, you can see the result of testing on Windows and Linux on the\nsame repository using the same version of Git. It is rather easy to\ncompare if you notice that the following match between syscalls:\n\nWindows Linux\n\nQueryOpen lstat or fstat\nCreateFile open\nCloseFile close\nQueryDirectory getdents\n\nI have also tested git-diff to verify that the number of system calls\nmatches pretty well. (In fact, I got practical identical list for stat\nsyscalls for files inside of the working directory on Windows and Linux\nwhen ran git-diff.) But something strange is going on with git-status.\nThe beginning of the log is identical on Windows and Linux, but then I\nsee more 'stat's in the Windows log that did not happen on Linux. In\ntotal, I see about 3 times increase of 'stat' calls, with all files\nbeing stat twice and directories (which are numerous) being stat 3 and\nmore times (some of them as many 39 times...) It seems that every\ndirectory is stat as many times as the number of subdirectories it has\nplus 3.\n\nIt appears that the second 'stat' for files on Windows caused by lack\nof d_type in dirent. When I recompiled the Linux version with\nNO_D_TYPE_IN_DIRENT = YesPlease, I got the same result for files.\n(Still I am not sure what caused those extra stat calls for\ndirectory, maybe, it is Cygwin specific...)\n\nThe question is whether it is possible to avoid this redundant 'stat'\nfor files on system that do not have d_type in dirent or that would\nrequire too much modification? Is it possible to use the cache where\nd_stat is not available provided that the entry is marked as uptodate?\n\n\n==== Git on Windows (CYGWIN) =====\n\n$ wc -l git-diff.csv git-status.csv\n 5186 git-diff.csv\n 21694 git-status.csv\n\n$ csvtool col 5 git-diff.csv | sort | uniq -c | sort -nr | head -10\n 4656 QueryOpen\n 100 CreateFile\n 94 CloseFile\n 80 QuerySecurityFile\n 61 ReadFile\n 30 QueryInformationVolume\n 28 QueryAllInformationFile\n 26 RegOpenKey\n 24 RegCloseKey\n 20 QueryStandardInformationFile\n\n$ csvtool col 5 git-status.csv | sort | uniq -c | sort -nr | head -10\n 12984 QueryOpen\n 3086 CreateFile\n 2103 CloseFile\n 1984 QueryDirectory\n 988 QueryFileInternalInformationFile\n 132 QuerySecurityFile\n 100 ReadFile\n 77 WriteFile\n 55 QueryInformationVolume\n 53 QueryAllInformationFile\n\nSuccessful open:\n$ csvtool col 5,7,8 git-diff.csv | grep CreateFile,SUCCESS, | wc -l\n94\n$ csvtool col 5,7,8 git-status.csv | grep CreateFile,SUCCESS, | wc -l\n2103\n\nSuccessful open for directories:\n$ csvtool col 5,7,8 git-diff.csv | grep CreateFile,SUCCESS,.*Options:.*Directory | wc -l\n37\n$ csvtool col 5,7,8 git-status.csv | grep CreateFile,SUCCESS,.*Options:.*Directory | wc -l\n1024\n\nNot successful attempts to open\n$ csvtool col 5,7,8 git-diff.csv | grep CreateFile | grep -v ,SUCCESS, | wc -l\n6\n$ csvtool col 5,7,8 git-status.csv | grep CreateFile | grep -v ,SUCCESS, | wc -l\n983\n\nAttempts to open .gitignore\n$ csvtool col 5,6 git-diff.csv | grep 'CreateFile,.*\\\\\\.gitignore' | wc -l\n0\n$ csvtool col 5,6 git-status.csv | grep 'CreateFile,.*\\\\\\.gitignore' | wc -l\n986\n\n=== GIT on Linux ===\n\n$ wc -l linux-git-*\n 4674 linux-git-diff.log\n 9807 linux-git-status.log\n\n$ sed -e 's/(.*//' < linux-git-diff.log | sort | uniq -c | sort -rn | head -10\n 4237 lstat\n 88 mmap\n 56 open\n 50 close\n 50 access\n 48 fstat\n 45 mprotect\n 43 read\n 15 stat\n 13 munmap\n\nThe number of lstat+fstat is equal 4285 for git-diff\n\n$ sed -e 's/(.*//' < linux-git-status.log | sort | uniq -c | sort -rn | head -10\n 3279 lstat\n 2048 open\n 1976 getdents\n 1062 close\n 1058 fstat\n 97 mmap\n 67 read\n 48 access\n 45 mprotect\n 40 write\n\nThe number of lstat+fstat is equal 4337 for git-status.\n\nSuccessful open:\n$ grep -c '^open(.*= [^-]' linux-*\nlinux-git-diff.log:50\nlinux-git-status.log:1064\n\nSuccessful open for directories:\n$ grep -c '^open(.*O_DIRECTORY.*= [^-]' linux-*\nlinux-git-diff.log:1\nlinux-git-status.log:989\n\nNot successful attempts to open:\n$ grep -c '^open(.*= -1' linux-*\nlinux-git-diff.log:6\nlinux-git-status.log:984\n\nAttempts to open .gitignore:\n$ grep -c '^open(.*.\\.gitignore\"' linux-*\nlinux-git-diff.log:0\nlinux-git-status.log:987\n\n=== Linux with NO_D_TYPE_IN_DIRENT = YesPlease ===\n\n$ wc -l linux-git-*no-dtype.log\n 4674 linux-git-diff-no-dtype.log\n 14040 linux-git-status-no-dtype.log\n\n$ sed -e 's/(.*//' < linux-git-diff-no-dtype.log | sort | uniq -c | sort -rn | head -10\n\n 4237 lstat\n 88 mmap\n 56 open\n 50 close\n 50 access\n 48 fstat\n 45 mprotect\n 43 read\n 15 stat\n 13 munmap\n\nThe number of lstat+fstat is equal 4285 for git-diff\n\n$ sed -e 's/(.*//' < linux-git-status-no-dtype.log | sort | uniq -c | sort -rn | head -10\n\n 7512 lstat\n 2048 open\n 1976 getdents\n 1062 close\n 1058 fstat\n 97 mmap\n 67 read\n 48 access\n 45 mprotect\n 40 write\n\nThe number of lstat+fstat is equal 8570 for git-status.\n\nSuccessful open:\n$ grep -c '^open(.*= [^-]' linux-*-no-dtype.log\nlinux-git-diff-no-dtype.log:50\nlinux-git-status-no-dtype.log:1064\n\nSuccessful open for directories:\n$ grep -c '^open(.*O_DIRECTORY.*= [^-]' linux-*-no-dtype.log\nlinux-git-diff-no-dtype.log:1\nlinux-git-status-no-dtype.log:989\n\nNot successful attempts to open:\n$ grep -c '^open(.*= -1' linux-*-no-dtype.log\nlinux-git-diff-no-dtype.log:6\nlinux-git-status-no-dtype.log:984\n\nAttempts to open .gitignore:\n$ grep -c '^open(.*.\\.gitignore\"' linux-*-no-dtype.log\nlinux-git-diff-no-dtype.log:0\nlinux-git-status-no-dtype.log:987\n\n=======\n\nDmitry\n"},{"id":"117630","messageId":"4A54F859.2080407@ramsay1.demon.co.uk","threadId":"20041","inReplyTo":"20090707000500.GA5594@dpotapov.dyndns.org","subject":"Re: Too many 'stat' calls by git-status on Windows","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2009-07-08T19:49:45Z","receivedAt":"2009-07-08T19:49:45Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Dmitry Potapov wrote:\n[snip]\n> It appears that the second 'stat' for files on Windows caused by lack\n> of d_type in dirent. When I recompiled the Linux version with\n> NO_D_TYPE_IN_DIRENT = YesPlease, I got the same result for files.\n\nI believe that the next version of cygwin, currently in beta, will have\nthe d_type field in dirent. I know that's not much help now...\n(I don't think it would be a good idea to try and reto-fit d_type,\nala compat/mingw.[ch], since cygwin does some funky stuff behind the\nscenes).\n\nNice profiling BTW.\n\nATB,\nRamsay Jones\n"},{"id":"117644","messageId":"alpine.LFD.2.01.0907081902371.3352@localhost.localdomain","threadId":"20041","inReplyTo":"20090707000500.GA5594@dpotapov.dyndns.org","subject":"Re: Too many 'stat' calls by git-status on Windows","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-09T02:04:42Z","receivedAt":"2009-07-09T02:04:42Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 7 Jul 2009, Dmitry Potapov wrote:\n> \n> It appears that the second 'stat' for files on Windows caused by lack\n> of d_type in dirent. When I recompiled the Linux version with\n> NO_D_TYPE_IN_DIRENT = YesPlease, I got the same result for files.\n> (Still I am not sure what caused those extra stat calls for\n> directory, maybe, it is Cygwin specific...)\n> \n> The question is whether it is possible to avoid this redundant 'stat'\n> for files on system that do not have d_type in dirent or that would\n> require too much modification? Is it possible to use the cache where\n> d_stat is not available provided that the entry is marked as uptodate?\n\nHmm. Sure. Something like this?\n\n\t\tLinus\n\n---\n dir.c | 14 +++++++++-----\n 1 files changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 74b3bbf..aaf269b 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -17,7 +17,7 @@ struct path_simplify {\n static int read_directory_recursive(struct dir_struct *dir,\n \tconst char *path, const char *base, int baselen,\n \tint check_only, const struct path_simplify *simplify);\n-static int get_dtype(struct dirent *de, const char *path);\n+static int get_dtype(struct dirent *de, const char *path, int pathlen);\n \n int common_prefix(const char **pathspec)\n {\n@@ -307,7 +307,7 @@ static int excluded_1(const char *pathname,\n \n \t\t\tif (x->flags & EXC_FLAG_MUSTBEDIR) {\n \t\t\t\tif (*dtype == DT_UNKNOWN)\n-\t\t\t\t\t*dtype = get_dtype(NULL, pathname);\n+\t\t\t\t\t*dtype = get_dtype(NULL, pathname, pathlen);\n \t\t\t\tif (*dtype != DT_DIR)\n \t\t\t\t\tcontinue;\n \t\t\t}\n@@ -547,14 +547,18 @@ static int in_pathspec(const char *path, int len, const struct path_simplify *si\n \treturn 0;\n }\n \n-static int get_dtype(struct dirent *de, const char *path)\n+static int get_dtype(struct dirent *de, const char *path, int pathlen)\n {\n \tint dtype = de ? DTYPE(de) : DT_UNKNOWN;\n+\tstruct cache_entry *ce;\n \tstruct stat st;\n \n \tif (dtype != DT_UNKNOWN)\n \t\treturn dtype;\n-\tif (lstat(path, &st))\n+\tce = cache_name_exists(path, pathlen, 0);\n+\tif (ce && ce_uptodate(ce))\n+\t\tst.st_mode = ce->ce_mode;\n+\telse if (lstat(path, &st))\n \t\treturn dtype;\n \tif (S_ISREG(st.st_mode))\n \t\treturn DT_REG;\n@@ -613,7 +617,7 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co\n \t\t\t\tcontinue;\n \n \t\t\tif (dtype == DT_UNKNOWN)\n-\t\t\t\tdtype = get_dtype(de, fullname);\n+\t\t\t\tdtype = get_dtype(de, fullname, baselen + len);\n \n \t\t\t/*\n \t\t\t * Do we want to see just the ignored files?\n"},{"id":"117645","messageId":"alpine.LFD.2.01.0907081933530.3352@localhost.localdomain","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907081902371.3352@localhost.localdomain","subject":"Re: Too many 'stat' calls by git-status on Windows","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-09T02:35:51Z","receivedAt":"2009-07-09T02:35:51Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 8 Jul 2009, Linus Torvalds wrote:\n> \n> Hmm. Sure. Something like this?\n\nOk, so having done some testing on it, it seems to work.\n\nAnd we might as well clean up some of dir.c at the same time. I'll reply \nto this email with a series of three patches: two cleanup ones, and then a \nnew version of this one (I hated how it looked to have those duplicated \n\"baselen+len\" things in read_directory_recursive()).\n\n\t\tLinus\n"},{"id":"117646","messageId":"alpine.LFD.2.01.0907081936470.3352@localhost.localdomain","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907081933530.3352@localhost.localdomain","subject":"[PATCH 1/3] Add 'fill_directory()' helper function for directory traversal","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-09T02:40:15Z","receivedAt":"2009-07-09T02:40:15Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nFrom: Linus Torvalds \nDate: Thu, 14 May 2009 13:22:36 -0700\nSubject: [PATCH 1/3] Add 'fill_directory()' helper function for directory traversal\n\nMost of the users of \"read_directory()\" actually want a much simpler\ninterface than the whole complex (but rather powerful) one.\n\nIn fact 'git add' had already largely abstracted out the core interface\nissues into a private \"fill_directory()\" function that was largely\napplicable almost as-is to a number of callers. Yes, 'git add' wants to\ndo some extra work of its own, specific to the add semantics, but we can\neasily split that out, and use the core as a generic function.\n\nThis function does exactly that, and now that much simplified\n'fill_directory()' function can be shared with a number of callers,\nwhile also ensuring that the rather more complex calling conventions of\nread_directory() are used by fewer call-sites.\n\nThis also makes the 'common_prefix()' helper function private to dir.c,\nsince all callers are now in that file.\n\nSigned-off-by: Linus Torvalds \n---\n\nThis is a cleaned-up version of a patch that I had done earlier for the \npathname character set conversion series. It's basically a cleanup of \nb99acc690de27aaf437676c9e3077493a885b642 in 'pu'.\n\n builtin-add.c | 45 ++++++++++++++-------------------------------\n builtin-clean.c | 12 +-----------\n builtin-ls-files.c | 7 +------\n dir.c | 23 ++++++++++++++++++++++-\n dir.h | 3 +--\n wt-status.c | 2 +-\n 6 files changed, 40 insertions(+), 52 deletions(-)\n\ndiff --git a/builtin-add.c b/builtin-add.c\nindex 78989da..581a2a1 100644\n--- a/builtin-add.c\n+++ b/builtin-add.c\n@@ -97,35 +97,6 @@ static void treat_gitlinks(const char **pathspec)\n \t}\n }\n \n-static void fill_directory(struct dir_struct *dir, const char **pathspec,\n-\t\tint ignored_too)\n-{\n-\tconst char *path, *base;\n-\tint baselen;\n-\n-\t/* Set up the default git porcelain excludes */\n-\tmemset(dir, 0, sizeof(*dir));\n-\tif (!ignored_too) {\n-\t\tdir->flags |= DIR_COLLECT_IGNORED;\n-\t\tsetup_standard_excludes(dir);\n-\t}\n-\n-\t/*\n-\t * Calculate common prefix for the pathspec, and\n-\t * use that to optimize the directory walk\n-\t */\n-\tbaselen = common_prefix(pathspec);\n-\tpath = \".\";\n-\tbase = \"\";\n-\tif (baselen)\n-\t\tpath = base = xmemdupz(*pathspec, baselen);\n-\n-\t/* Read the directory and prune it */\n-\tread_directory(dir, path, base, baselen, pathspec);\n-\tif (pathspec)\n-\t\tprune_directory(dir, pathspec, baselen);\n-}\n-\n static void refresh(int verbose, const char **pathspec)\n {\n \tchar *seen;\n@@ -343,9 +314,21 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\tdie(\"index file corrupt\");\n \ttreat_gitlinks(pathspec);\n \n-\tif (add_new_files)\n+\tif (add_new_files) {\n+\t\tint baselen;\n+\n+\t\t/* Set up the default git porcelain excludes */\n+\t\tmemset(&dir, 0, sizeof(dir));\n+\t\tif (!ignored_too) {\n+\t\t\tdir.flags |= DIR_COLLECT_IGNORED;\n+\t\t\tsetup_standard_excludes(&dir);\n+\t\t}\n+\n \t\t/* This picks up the paths that are not tracked */\n-\t\tfill_directory(&dir, pathspec, ignored_too);\n+\t\tbaselen = fill_directory(&dir, pathspec);\n+\t\tif (pathspec)\n+\t\t\tprune_directory(&dir, pathspec, baselen);\n+\t}\n \n \tif (refresh_only) {\n \t\trefresh(verbose, pathspec);\ndiff --git a/builtin-clean.c b/builtin-clean.c\nindex 1c1b6d2..2d8c735 100644\n--- a/builtin-clean.c\n+++ b/builtin-clean.c\n@@ -33,7 +33,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \tint ignored_only = 0, baselen = 0, config_set = 0, errors = 0;\n \tstruct strbuf directory = STRBUF_INIT;\n \tstruct dir_struct dir;\n-\tconst char *path, *base;\n \tstatic const char **pathspec;\n \tstruct strbuf buf = STRBUF_INIT;\n \tconst char *qname;\n@@ -78,16 +77,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \tpathspec = get_pathspec(prefix, argv);\n \tread_cache();\n \n-\t/*\n-\t * Calculate common prefix for the pathspec, and\n-\t * use that to optimize the directory walk\n-\t */\n-\tbaselen = common_prefix(pathspec);\n-\tpath = \".\";\n-\tbase = \"\";\n-\tif (baselen)\n-\t\tpath = base = xmemdupz(*pathspec, baselen);\n-\tread_directory(&dir, path, base, baselen, pathspec);\n+\tfill_directory(&dir, pathspec);\n \n \tif (pathspec)\n \t\tseen = xmalloc(argc > 0 ? argc : 1);\ndiff --git a/builtin-ls-files.c b/builtin-ls-files.c\nindex 2312866..f473220 100644\n--- a/builtin-ls-files.c\n+++ b/builtin-ls-files.c\n@@ -161,12 +161,7 @@ static void show_files(struct dir_struct *dir, const char *prefix)\n \n \t/* For cached/deleted files we don't need to even do the readdir */\n \tif (show_others || show_killed) {\n-\t\tconst char *path = \".\", *base = \"\";\n-\t\tint baselen = prefix_len;\n-\n-\t\tif (baselen)\n-\t\t\tpath = base = prefix;\n-\t\tread_directory(dir, path, base, baselen, pathspec);\n+\t\tfill_directory(dir, pathspec);\n \t\tif (show_others)\n \t\t\tshow_other_files(dir);\n \t\tif (show_killed)\ndiff --git a/dir.c b/dir.c\nindex 74b3bbf..0c8553b 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -19,7 +19,7 @@ static int read_directory_recursive(struct dir_struct *dir,\n \tint check_only, const struct path_simplify *simplify);\n static int get_dtype(struct dirent *de, const char *path);\n \n-int common_prefix(const char **pathspec)\n+static int common_prefix(const char **pathspec)\n {\n \tconst char *path, *slash, *next;\n \tint prefix;\n@@ -52,6 +52,27 @@ int common_prefix(const char **pathspec)\n \treturn prefix;\n }\n \n+int fill_directory(struct dir_struct *dir, const char **pathspec)\n+{\n+\tconst char *path, *base;\n+\tint baselen;\n+\n+\t/*\n+\t * Calculate common prefix for the pathspec, and\n+\t * use that to optimize the directory walk\n+\t */\n+\tbaselen = common_prefix(pathspec);\n+\tpath = \"\";\n+\tbase = \"\";\n+\n+\tif (baselen)\n+\t\tpath = base = xmemdupz(*pathspec, baselen);\n+\n+\t/* Read the directory and prune it */\n+\tread_directory(dir, path, base, baselen, pathspec);\n+\treturn baselen;\n+}\n+\n /*\n * Does 'match' match the given name?\n * A match is found if\ndiff --git a/dir.h b/dir.h\nindex 541286a..f9d69dd 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -61,13 +61,12 @@ struct dir_struct {\n \tchar basebuf[PATH_MAX];\n };\n \n-extern int common_prefix(const char **pathspec);\n-\n #define MATCHED_RECURSIVELY 1\n #define MATCHED_FNMATCH 2\n #define MATCHED_EXACTLY 3\n extern int match_pathspec(const char **pathspec, const char *name, int namelen, int prefix, char *seen);\n \n+extern int fill_directory(struct dir_struct *dir, const char **pathspec);\n extern int read_directory(struct dir_struct *, const char *path, const char *base, int baselen, const char **pathspec);\n \n extern int excluded(struct dir_struct *, const char *, int *);\ndiff --git a/wt-status.c b/wt-status.c\nindex 0ca4b13..47735d8 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -255,7 +255,7 @@ static void wt_status_print_untracked(struct wt_status *s)\n \t\t\tDIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n \tsetup_standard_excludes(&dir);\n \n-\tread_directory(&dir, \".\", \"\", 0, NULL);\n+\tfill_directory(&dir, NULL);\n \tfor(i = 0; i < dir.nr; i++) {\n \t\tstruct dir_entry *ent = dir.entries[i];\n \t\tif (!cache_name_is_other(ent->name, ent->len))\n-- \n1.6.3.3.412.gf581d\n"},{"id":"117647","messageId":"alpine.LFD.2.01.0907081940220.3352@localhost.localdomain","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907081936470.3352@localhost.localdomain","subject":"[PATCH 2/3] Simplify read_directory[_recursive]() arguments","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-09T02:42:33Z","receivedAt":"2009-07-09T02:42:33Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nFrom: Linus Torvalds \nDate: Wed, 8 Jul 2009 19:24:39 -0700\nSubject: [PATCH 2/3] Simplify read_directory[_recursive]() arguments\n\nStop the insanity with separate 'path' and 'base' arguments that must\nmatch. We don't need that crazy interface any more, since we cleaned up\nhandling of 'path' in commit da4b3e8c28b1dc2b856d2555ac7bb47ab712598c.\n\nSigned-off-by: Linus Torvalds \n---\n\nThe diffstat says it only removes a single line, but it _simplifies_ a lot \nof them, and gets rid of the horrible confusion about what 'path' vs \n'base' means.\n\n dir.c | 57 +++++++++++++++++++++++++++----------------------------\n dir.h | 2 +-\n unpack-trees.c | 2 +-\n 3 files changed, 30 insertions(+), 31 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 0c8553b..b0671f5 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -14,8 +14,7 @@ struct path_simplify {\n \tconst char *path;\n };\n \n-static int read_directory_recursive(struct dir_struct *dir,\n-\tconst char *path, const char *base, int baselen,\n+static int read_directory_recursive(struct dir_struct *dir, const char *path, int len,\n \tint check_only, const struct path_simplify *simplify);\n static int get_dtype(struct dirent *de, const char *path);\n \n@@ -54,23 +53,22 @@ static int common_prefix(const char **pathspec)\n \n int fill_directory(struct dir_struct *dir, const char **pathspec)\n {\n-\tconst char *path, *base;\n-\tint baselen;\n+\tconst char *path;\n+\tint len;\n \n \t/*\n \t * Calculate common prefix for the pathspec, and\n \t * use that to optimize the directory walk\n \t */\n-\tbaselen = common_prefix(pathspec);\n+\tlen = common_prefix(pathspec);\n \tpath = \"\";\n-\tbase = \"\";\n \n-\tif (baselen)\n-\t\tpath = base = xmemdupz(*pathspec, baselen);\n+\tif (len)\n+\t\tpath = xmemdupz(*pathspec, len);\n \n \t/* Read the directory and prune it */\n-\tread_directory(dir, path, base, baselen, pathspec);\n-\treturn baselen;\n+\tread_directory(dir, path, len, pathspec);\n+\treturn len;\n }\n \n /*\n@@ -526,7 +524,7 @@ static enum directory_treatment treat_directory(struct dir_struct *dir,\n \t/* This is the \"show_other_directories\" case */\n \tif (!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))\n \t\treturn show_directory;\n-\tif (!read_directory_recursive(dir, dirname, dirname, len, 1, simplify))\n+\tif (!read_directory_recursive(dir, dirname, len, 1, simplify))\n \t\treturn ignore_directory;\n \treturn show_directory;\n }\n@@ -595,15 +593,15 @@ static int get_dtype(struct dirent *de, const char *path)\n * Also, we ignore the name \".git\" (even if it is not a directory).\n * That likely will not change.\n */\n-static int read_directory_recursive(struct dir_struct *dir, const char *path, const char *base, int baselen, int check_only, const struct path_simplify *simplify)\n+static int read_directory_recursive(struct dir_struct *dir, const char *base, int baselen, int check_only, const struct path_simplify *simplify)\n {\n-\tDIR *fdir = opendir(*path ? path : \".\");\n+\tDIR *fdir = opendir(*base ? base : \".\");\n \tint contents = 0;\n \n \tif (fdir) {\n \t\tstruct dirent *de;\n-\t\tchar fullname[PATH_MAX + 1];\n-\t\tmemcpy(fullname, base, baselen);\n+\t\tchar path[PATH_MAX + 1];\n+\t\tmemcpy(path, base, baselen);\n \n \t\twhile ((de = readdir(fdir)) != NULL) {\n \t\t\tint len, dtype;\n@@ -614,17 +612,18 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co\n \t\t\t\tcontinue;\n \t\t\tlen = strlen(de->d_name);\n \t\t\t/* Ignore overly long pathnames! */\n-\t\t\tif (len + baselen + 8 > sizeof(fullname))\n+\t\t\tif (len + baselen + 8 > sizeof(path))\n \t\t\t\tcontinue;\n-\t\t\tmemcpy(fullname + baselen, de->d_name, len+1);\n-\t\t\tif (simplify_away(fullname, baselen + len, simplify))\n+\t\t\tmemcpy(path + baselen, de->d_name, len+1);\n+\t\t\tlen = baselen + len;\n+\t\t\tif (simplify_away(path, len, simplify))\n \t\t\t\tcontinue;\n \n \t\t\tdtype = DTYPE(de);\n-\t\t\texclude = excluded(dir, fullname, &dtype);\n+\t\t\texclude = excluded(dir, path, &dtype);\n \t\t\tif (exclude && (dir->flags & DIR_COLLECT_IGNORED)\n-\t\t\t && in_pathspec(fullname, baselen + len, simplify))\n-\t\t\t\tdir_add_ignored(dir, fullname, baselen + len);\n+\t\t\t && in_pathspec(path, len, simplify))\n+\t\t\t\tdir_add_ignored(dir, path,len);\n \n \t\t\t/*\n \t\t\t * Excluded? If we don't explicitly want to show\n@@ -634,7 +633,7 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co\n \t\t\t\tcontinue;\n \n \t\t\tif (dtype == DT_UNKNOWN)\n-\t\t\t\tdtype = get_dtype(de, fullname);\n+\t\t\t\tdtype = get_dtype(de, path);\n \n \t\t\t/*\n \t\t\t * Do we want to see just the ignored files?\n@@ -651,9 +650,9 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co\n \t\t\tdefault:\n \t\t\t\tcontinue;\n \t\t\tcase DT_DIR:\n-\t\t\t\tmemcpy(fullname + baselen + len, \"/\", 2);\n+\t\t\t\tmemcpy(path + len, \"/\", 2);\n \t\t\t\tlen++;\n-\t\t\t\tswitch (treat_directory(dir, fullname, baselen + len, simplify)) {\n+\t\t\t\tswitch (treat_directory(dir, path, len, simplify)) {\n \t\t\t\tcase show_directory:\n \t\t\t\t\tif (exclude != !!(dir->flags\n \t\t\t\t\t\t\t& DIR_SHOW_IGNORED))\n@@ -661,7 +660,7 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co\n \t\t\t\t\tbreak;\n \t\t\t\tcase recurse_into_directory:\n \t\t\t\t\tcontents += read_directory_recursive(dir,\n-\t\t\t\t\t\tfullname, fullname, baselen + len, 0, simplify);\n+\t\t\t\t\t\tpath, len, 0, simplify);\n \t\t\t\t\tcontinue;\n \t\t\t\tcase ignore_directory:\n \t\t\t\t\tcontinue;\n@@ -675,7 +674,7 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co\n \t\t\tif (check_only)\n \t\t\t\tgoto exit_early;\n \t\t\telse\n-\t\t\t\tdir_add_name(dir, fullname, baselen + len);\n+\t\t\t\tdir_add_name(dir, path, len);\n \t\t}\n exit_early:\n \t\tclosedir(fdir);\n@@ -738,15 +737,15 @@ static void free_simplify(struct path_simplify *simplify)\n \tfree(simplify);\n }\n \n-int read_directory(struct dir_struct *dir, const char *path, const char *base, int baselen, const char **pathspec)\n+int read_directory(struct dir_struct *dir, const char *path, int len, const char **pathspec)\n {\n \tstruct path_simplify *simplify;\n \n-\tif (has_symlink_leading_path(path, strlen(path)))\n+\tif (has_symlink_leading_path(path, len))\n \t\treturn dir->nr;\n \n \tsimplify = create_simplify(pathspec);\n-\tread_directory_recursive(dir, path, base, baselen, 0, simplify);\n+\tread_directory_recursive(dir, path, len, 0, simplify);\n \tfree_simplify(simplify);\n \tqsort(dir->entries, dir->nr, sizeof(struct dir_entry *), cmp_name);\n \tqsort(dir->ignored, dir->ignored_nr, sizeof(struct dir_entry *), cmp_name);\ndiff --git a/dir.h b/dir.h\nindex f9d69dd..a631446 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -67,7 +67,7 @@ struct dir_struct {\n extern int match_pathspec(const char **pathspec, const char *name, int namelen, int prefix, char *seen);\n \n extern int fill_directory(struct dir_struct *dir, const char **pathspec);\n-extern int read_directory(struct dir_struct *, const char *path, const char *base, int baselen, const char **pathspec);\n+extern int read_directory(struct dir_struct *, const char *path, int len, const char **pathspec);\n \n extern int excluded(struct dir_struct *, const char *, int *);\n extern void add_excludes_from_file(struct dir_struct *, const char *fname);\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 05d0bb1..42c7d7d 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -551,7 +551,7 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,\n \tmemset(&d, 0, sizeof(d));\n \tif (o->dir)\n \t\td.exclude_per_dir = o->dir->exclude_per_dir;\n-\ti = read_directory(&d, ce->name, pathbuf, namelen+1, NULL);\n+\ti = read_directory(&d, pathbuf, namelen+1, NULL);\n \tif (i)\n \t\treturn o->gently ? -1 :\n \t\t\terror(ERRORMSG(o, not_uptodate_dir), ce->name);\n-- \n1.6.3.3.412.gf581d\n"},{"id":"117648","messageId":"alpine.LFD.2.01.0907081942380.3352@localhost.localdomain","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907081940220.3352@localhost.localdomain","subject":"[PATCH 3/3] Avoid doing extra 'lstat()'s for d_type if we have an up-to-date cache entry","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-09T02:43:50Z","receivedAt":"2009-07-09T02:43:50Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nFrom: Linus Torvalds \nDate: Wed, 8 Jul 2009 19:31:49 -0700\nSubject: [PATCH 3/3] Avoid doing extra 'lstat()'s for d_type if we have an up-to-date cache entry\n\nOn filesystems without d_type, we can look at the cache entry first.\nDoing an lstat() can be expensive.\n\nReported by Dmitry Potapov for Cygwin.\n\nSigned-off-by: Linus Torvalds \n---\n\nThis is the same patch I already sent Dmitry, but now it applies on top of \nthe cleaned-up read_directory_recursive() code.\n\n dir.c | 14 +++++++++-----\n 1 files changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex b0671f5..8a9e7d8 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -16,7 +16,7 @@ struct path_simplify {\n \n static int read_directory_recursive(struct dir_struct *dir, const char *path, int len,\n \tint check_only, const struct path_simplify *simplify);\n-static int get_dtype(struct dirent *de, const char *path);\n+static int get_dtype(struct dirent *de, const char *path, int len);\n \n static int common_prefix(const char **pathspec)\n {\n@@ -326,7 +326,7 @@ static int excluded_1(const char *pathname,\n \n \t\t\tif (x->flags & EXC_FLAG_MUSTBEDIR) {\n \t\t\t\tif (*dtype == DT_UNKNOWN)\n-\t\t\t\t\t*dtype = get_dtype(NULL, pathname);\n+\t\t\t\t\t*dtype = get_dtype(NULL, pathname, pathlen);\n \t\t\t\tif (*dtype != DT_DIR)\n \t\t\t\t\tcontinue;\n \t\t\t}\n@@ -566,14 +566,18 @@ static int in_pathspec(const char *path, int len, const struct path_simplify *si\n \treturn 0;\n }\n \n-static int get_dtype(struct dirent *de, const char *path)\n+static int get_dtype(struct dirent *de, const char *path, int len)\n {\n \tint dtype = de ? DTYPE(de) : DT_UNKNOWN;\n+\tstruct cache_entry *ce;\n \tstruct stat st;\n \n \tif (dtype != DT_UNKNOWN)\n \t\treturn dtype;\n-\tif (lstat(path, &st))\n+\tce = cache_name_exists(path, len, 0);\n+\tif (ce && ce_uptodate(ce))\n+\t\tst.st_mode = ce->ce_mode;\n+\telse if (lstat(path, &st))\n \t\treturn dtype;\n \tif (S_ISREG(st.st_mode))\n \t\treturn DT_REG;\n@@ -633,7 +637,7 @@ static int read_directory_recursive(struct dir_struct *dir, const char *base, in\n \t\t\t\tcontinue;\n \n \t\t\tif (dtype == DT_UNKNOWN)\n-\t\t\t\tdtype = get_dtype(de, path);\n+\t\t\t\tdtype = get_dtype(de, path, len);\n \n \t\t\t/*\n \t\t\t * Do we want to see just the ignored files?\n-- \n1.6.3.3.412.gf581d\n"},{"id":"117657","messageId":"7vskh646bw.fsf@alter.siamese.dyndns.org","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907081942380.3352@localhost.localdomain","subject":"Re: [PATCH 3/3] Avoid doing extra 'lstat()'s for d_type if we have an up-to-date cache entry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-09T08:18:59Z","receivedAt":"2009-07-09T08:18:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> From: Linus Torvalds \n> Date: Wed, 8 Jul 2009 19:31:49 -0700\n> Subject: [PATCH 3/3] Avoid doing extra 'lstat()'s for d_type if we have an up-to-date cache entry\n>\n> On filesystems without d_type, we can look at the cache entry first.\n> Doing an lstat() can be expensive.\n\nThanks.\n\nI was wondering if we could also say that D exists as a directory when we\nknow there is D/F in the index and is up to date.\n"},{"id":"117690","messageId":"20090709135010.GA19425@dpotapov.dyndns.org","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907081942380.3352@localhost.localdomain","subject":"Re: [PATCH 3/3] Avoid doing extra 'lstat()'s for d_type if we have an up-to-date cache entry","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2009-07-09T13:50:10Z","receivedAt":"2009-07-09T13:50:10Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Wed, Jul 08, 2009 at 07:43:50PM -0700, Linus Torvalds wrote:\n> \n> On filesystems without d_type, we can look at the cache entry first.\n> Doing an lstat() can be expensive.\n> \n> Reported by Dmitry Potapov for Cygwin.\n\nI have tested it on Cygwin. The number of 'stat' for files is now 1, so\nit works fine :)\n\nI still have the same large number of 'stat' calls for directories, but I\nsuspect that due to that due to some Cygwin specific. I will investigate\nthat issue later when I have more time.\n\nBecause the repositoty on which I did testing has too many directories\n(one directory per each 3.5 files) the effect was not as prominent as\nit would be otherwise. Yet, it is 24.9% decrease of the number of 'stat'\nor 14.8% descreased of the total number of syscalls. And my measurement\nshows 14% descrease of run-time. So, it appears that on Windows the run\ntime almost directly proportional of the total number of syscalls...\n\nBTW, I believe that this patch should help MinGW too, because AFAIK\nMinGW does not have d_type either.\n\n\nThanks,\nDmitry\n"},{"id":"117698","messageId":"alpine.LFD.2.01.0907090832200.3352@localhost.localdomain","threadId":"20041","inReplyTo":"7vskh646bw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] Avoid doing extra 'lstat()'s for d_type if we have an up-to-date cache entry","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-09T15:52:37Z","receivedAt":"2009-07-09T15:52:37Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 9 Jul 2009, Junio C Hamano wrote:\n> \n> I was wondering if we could also say that D exists as a directory when we\n> know there is D/F in the index and is up to date.\n\nYeah, that would probably be a good thing, but is slightly slower to look \nup (we have the name hashing for the case-ignoring code anyway, but that \nonly works for exact names, so you can't look up directories that way).\n\nYou'd have to use the regular binary search for that (or we'd have to \nchange it to hash directories too - which we might want to do for other \nreasons, but don't do now).\n\nSomething like this?\n\n\t\tLinus\n\n---\n dir.c | 39 ++++++++++++++++++++++++++++++++++-----\n 1 files changed, 34 insertions(+), 5 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 8a9e7d8..fb7432e 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -566,18 +566,47 @@ static int in_pathspec(const char *path, int len, const struct path_simplify *si\n \treturn 0;\n }\n \n+static int get_index_mode(const char *path, int len)\n+{\n+\tint pos;\n+\tstruct cache_entry *ce;\n+\n+\tce = cache_name_exists(path, len, 0);\n+\tif (ce) {\n+\t\tif (ce_uptodate(ce))\n+\t\t\treturn ce->ce_mode;\n+\t\treturn 0;\n+\t}\n+\n+\t/* Try to look it up as a directory */\n+\tpos = cache_name_pos(path, len);\n+\tif (pos >= 0)\n+\t\treturn 0;\n+\tpos = -pos-1;\n+\twhile (pos < active_nr) {\n+\t\tce = active_cache[pos++];\n+\t\tif (strncmp(ce->name, path, len))\n+\t\t\tbreak;\n+\t\tif (ce->name[len] > '/')\n+\t\t\tbreak;\n+\t\tif (ce->name[len] < '/')\n+\t\t\tcontinue;\n+\t\tif (!ce_uptodate(ce))\n+\t\t\tbreak;\t/* continue? */\n+\t\treturn S_IFDIR;\n+\t}\n+\treturn 0;\n+}\n+\n static int get_dtype(struct dirent *de, const char *path, int len)\n {\n \tint dtype = de ? DTYPE(de) : DT_UNKNOWN;\n-\tstruct cache_entry *ce;\n \tstruct stat st;\n \n \tif (dtype != DT_UNKNOWN)\n \t\treturn dtype;\n-\tce = cache_name_exists(path, len, 0);\n-\tif (ce && ce_uptodate(ce))\n-\t\tst.st_mode = ce->ce_mode;\n-\telse if (lstat(path, &st))\n+\tst.st_mode = get_index_mode(path, len);\n+\tif (!st.st_mode && lstat(path, &st))\n \t\treturn dtype;\n \tif (S_ISREG(st.st_mode))\n \t\treturn DT_REG;\n"},{"id":"117704","messageId":"7vws6h3ji4.fsf@alter.siamese.dyndns.org","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907090832200.3352@localhost.localdomain","subject":"Re: [PATCH 3/3] Avoid doing extra 'lstat()'s for d_type if we have an up-to-date cache entry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-09T16:32:03Z","receivedAt":"2009-07-09T16:32:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Thu, 9 Jul 2009, Junio C Hamano wrote:\n>> \n>> I was wondering if we could also say that D exists as a directory when we\n>> know there is D/F in the index and is up to date.\n>\n> Yeah, that would probably be a good thing, but is slightly slower to look \n> up (we have the name hashing for the case-ignoring code anyway, but that \n> only works for exact names, so you can't look up directories that way).\n>\n> You'd have to use the regular binary search for that (or we'd have to \n> change it to hash directories too - which we might want to do for other \n> reasons, but don't do now).\n>\n> Something like this?\n\nYeah, in Dmitry's response that crossed with this update patch from you,\nhe says lstat() on directories are still problem---it would be interesting to\nhear what he sees after applying this patch and retesting.\n\n> +static int get_index_mode(const char *path, int len)\n> +{\n> +\tint pos;\n> +\tstruct cache_entry *ce;\n> +\n> +\tce = cache_name_exists(path, len, 0);\n> +\tif (ce) {\n> +\t\tif (ce_uptodate(ce))\n> +\t\t\treturn ce->ce_mode;\n\nYou return ce->ce_mode for up-to-date entries. I do not remember what\nce_uptodate(ce) says for gitlinks, but ce->ce_mode for them would be\n160000 that is not very kosher to give to S_ISDIR(). I realize that this\nworry actually applies to your patch from yesterday, the one Dmitry\nalready tested.\n\n> +\t\treturn 0;\n> +\t}\n> +\n> +\t/* Try to look it up as a directory */\n> +\tpos = cache_name_pos(path, len);\n> +\tif (pos >= 0)\n> +\t\treturn 0;\n\nHow can this find an exact entry for the path? Assuming that the name\nhash cache_name_exists() is not out of sync? Shouldn't this be a BUG()\ninstead of \"It somehow exists as a blob or submodule, and we'll let the\nregular lstat() codepath take care of it by returning 0\"?\n\n> +\tpos = -pos-1;\n> +\twhile (pos < active_nr) {\n> +\t\tce = active_cache[pos++];\n> +\t\tif (strncmp(ce->name, path, len))\n> +\t\t\tbreak;\n> +\t\tif (ce->name[len] > '/')\n> +\t\t\tbreak;\n> +\t\tif (ce->name[len] < '/')\n> +\t\t\tcontinue;\n> +\t\tif (!ce_uptodate(ce))\n> +\t\t\tbreak;\t/* continue? */\n\nI think this should be continue, as the directory D you are interested in\nmay have two files, one modified, the other uptodate.\n\n> +\t\treturn S_IFDIR;\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> static int get_dtype(struct dirent *de, const char *path, int len)\n> {\n> \tint dtype = de ? DTYPE(de) : DT_UNKNOWN;\n> -\tstruct cache_entry *ce;\n> \tstruct stat st;\n> \n> \tif (dtype != DT_UNKNOWN)\n> \t\treturn dtype;\n> -\tce = cache_name_exists(path, len, 0);\n> -\tif (ce && ce_uptodate(ce))\n> -\t\tst.st_mode = ce->ce_mode;\n> -\telse if (lstat(path, &st))\n> +\tst.st_mode = get_index_mode(path, len);\n> +\tif (!st.st_mode && lstat(path, &st))\n> \t\treturn dtype;\n> \tif (S_ISREG(st.st_mode))\n> \t\treturn DT_REG;\n"},{"id":"117706","messageId":"alpine.LFD.2.01.0907090954090.3352@localhost.localdomain","threadId":"20041","inReplyTo":"7vws6h3ji4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] Avoid doing extra 'lstat()'s for d_type if we have an up-to-date cache entry","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-09T16:59:10Z","receivedAt":"2009-07-09T16:59:10Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 9 Jul 2009, Junio C Hamano wrote:\n> > +\n> > +\t/* Try to look it up as a directory */\n> > +\tpos = cache_name_pos(path, len);\n> > +\tif (pos >= 0)\n> > +\t\treturn 0;\n> \n> How can this find an exact entry for the path? Assuming that the name\n> hash cache_name_exists() is not out of sync?\n\nHopefully it would never trigger. But I'd rather write robust code that \ndoesn't make any fancy assumptions. Keep it simple - and keep it working \neven if surprising things happen. \n\n> > +\t\tif (!ce_uptodate(ce))\n> > +\t\t\tbreak;\t/* continue? */\n> \n> I think this should be continue, as the directory D you are interested in\n> may have two files, one modified, the other uptodate.\n\nThe thing is, the directory may have subdirectories, and there may be \ntens of thousands of files there. And maybe this gets called by code that \nhasn't done any cache preloading at all, so nothing will be up-to-date.\n\nDo we want to loop over thousands of entries? Or do we want to loop as \nlittle as possible, and just say \"most of the time the first entry will be \nrepresentative\".\n\nBut I did put the 'continue' in a comment, because it's not a correctness \nissue, it's a gut feel. \n\n\t\t\tLinus\n"},{"id":"117709","messageId":"alpine.LFD.2.01.0907091011280.3352@localhost.localdomain","threadId":"20041","inReplyTo":"7vws6h3ji4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] Avoid doing extra 'lstat()'s for d_type if we have an up-to-date cache entry","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-09T17:13:32Z","receivedAt":"2009-07-09T17:13:32Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\n> > +\tce = cache_name_exists(path, len, 0);\n> > +\tif (ce) {\n> > +\t\tif (ce_uptodate(ce))\n> > +\t\t\treturn ce->ce_mode;\n> \n> You return ce->ce_mode for up-to-date entries. I do not remember what\n> ce_uptodate(ce) says for gitlinks, but ce->ce_mode for them would be\n> 160000 that is not very kosher to give to S_ISDIR(). I realize that this\n> worry actually applies to your patch from yesterday, the one Dmitry\n> already tested.\n\nYeah. I guess we don't have a lot of coverage for subprojects.\n\nHere's an alternative version that just makes the thing return the DT_xyz \nflag rather than the mode (and it returns DT_REG for symlinks too, because \nit knows nobody cares - we only really care about \"directory or not\")\n\n\t\tLinus\n\n---\n dir.c | 47 ++++++++++++++++++++++++++++++++++++++++++-----\n 1 files changed, 42 insertions(+), 5 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 8a9e7d8..e05b850 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -566,18 +566,55 @@ static int in_pathspec(const char *path, int len, const struct path_simplify *si\n \treturn 0;\n }\n \n+static int get_index_dtype(const char *path, int len)\n+{\n+\tint pos;\n+\tstruct cache_entry *ce;\n+\n+\tce = cache_name_exists(path, len, 0);\n+\tif (ce) {\n+\t\tif (!ce_uptodate(ce))\n+\t\t\treturn DT_UNKNOWN;\n+\t\tif (S_ISGITLINK(ce->ce_mode))\n+\t\t\treturn DT_DIR;\n+\t\t/*\n+\t\t * Nobody actually cares about the\n+\t\t * difference between DT_LNK and DT_REG\n+\t\t */\n+\t\treturn DT_REG;\n+\t}\n+\n+\t/* Try to look it up as a directory */\n+\tpos = cache_name_pos(path, len);\n+\tif (pos >= 0)\n+\t\treturn DT_UNKNOWN;\n+\tpos = -pos-1;\n+\twhile (pos < active_nr) {\n+\t\tce = active_cache[pos++];\n+\t\tif (strncmp(ce->name, path, len))\n+\t\t\tbreak;\n+\t\tif (ce->name[len] > '/')\n+\t\t\tbreak;\n+\t\tif (ce->name[len] < '/')\n+\t\t\tcontinue;\n+\t\tif (!ce_uptodate(ce))\n+\t\t\tbreak;\t/* continue? */\n+\t\treturn DT_DIR;\n+\t}\n+\treturn DT_UNKNOWN;\n+}\n+\n static int get_dtype(struct dirent *de, const char *path, int len)\n {\n \tint dtype = de ? DTYPE(de) : DT_UNKNOWN;\n-\tstruct cache_entry *ce;\n \tstruct stat st;\n \n \tif (dtype != DT_UNKNOWN)\n \t\treturn dtype;\n-\tce = cache_name_exists(path, len, 0);\n-\tif (ce && ce_uptodate(ce))\n-\t\tst.st_mode = ce->ce_mode;\n-\telse if (lstat(path, &st))\n+\tdtype = get_index_dtype(path, len);\n+\tif (dtype != DT_UNKNOWN)\n+\t\treturn dtype;\n+\tif (lstat(path, &st))\n \t\treturn dtype;\n \tif (S_ISREG(st.st_mode))\n \t\treturn DT_REG;\n"},{"id":"117711","messageId":"alpine.LFD.2.01.0907091013540.3352@localhost.localdomain","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907091011280.3352@localhost.localdomain","subject":"Re: [PATCH 3/3] Avoid doing extra 'lstat()'s for d_type if we have an up-to-date cache entry","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-09T17:18:31Z","receivedAt":"2009-07-09T17:18:31Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 9 Jul 2009, Linus Torvalds wrote:\n> \n> Here's an alternative version that just makes the thing return the DT_xyz \n> flag rather than the mode (and it returns DT_REG for symlinks too, because \n> it knows nobody cares - we only really care about \"directory or not\")\n\nBtw, I'm wondering whether this \"look if 'dir/file' exists in index and is \nup-to-date\" is really safe.\n\nWe don't really verify the whole path when we mark things ce_uptodate(). \nPart of what read_directory() does is to find directory entries, and in \nthe process things like \"git add\" will notice if there's a conflict with \nexisting index entries.\n\nSo if a directory has changed into a symlink to a directory, this \nparticular optimization will actually hide that, I suspect. I haven't \ntested, though. But it might be worth-while to see what happens when you \nhad a directory structure, and then do\n\n\tmkdir dir\n\ttouch dir/a\n\ttouch dir/b\n\tgit add dir\n\n\tmv dir new-dir\n\tln -s new-dir dir\n\tgit status\n\nQuite frankly, I'd personally be perfectly ok with git _not_ noticing \nsubtle things like this automatically, but..\n\n\t\tLinus\n"},{"id":"117715","messageId":"7vbpnt3dth.fsf@alter.siamese.dyndns.org","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907090954090.3352@localhost.localdomain","subject":"Re: [PATCH 3/3] Avoid doing extra 'lstat()'s for d_type if we have an up-to-date cache entry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-09T18:34:50Z","receivedAt":"2009-07-09T18:34:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n>> > +\t\tif (!ce_uptodate(ce))\n>> > +\t\t\tbreak;\t/* continue? */\n>> \n>> I think this should be continue, as the directory D you are interested in\n>> may have two files, one modified, the other uptodate.\n>\n> The thing is, the directory may have subdirectories, and there may be \n> tens of thousands of files there. And maybe this gets called by code that \n> hasn't done any cache preloading at all, so nothing will be up-to-date.\n>\n> Do we want to loop over thousands of entries? Or do we want to loop as \n> little as possible, and just say \"most of the time the first entry will be \n> representative\".\n\nAh, I see.\n\nIt depends on how expensive it is to iterate over an in-core array to\ncheck a single bit (that may not even have been updated) in the cache\nentries, compared to an extra lstat(). Perhaps we could autotune that,\nbut it probably is not worth it. ;-)\n"},{"id":"117716","messageId":"7vab3d3dpc.fsf@alter.siamese.dyndns.org","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907091013540.3352@localhost.localdomain","subject":"Re: [PATCH 3/3] Avoid doing extra 'lstat()'s for d_type if we have an up-to-date cache entry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-09T18:37:19Z","receivedAt":"2009-07-09T18:37:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> We don't really verify the whole path when we mark things ce_uptodate(). \n> Part of what read_directory() does is to find directory entries, and in \n> the process things like \"git add\" will notice if there's a conflict with \n> existing index entries.\n>\n> So if a directory has changed into a symlink to a directory, this \n> particular optimization will actually hide that, I suspect. I haven't \n> tested, though. But it might be worth-while to see what happens when you \n> had a directory structure, and then do\n>\n> \tmkdir dir\n> \ttouch dir/a\n> \ttouch dir/b\n> \tgit add dir\n>\n> \tmv dir new-dir\n> \tln -s new-dir dir\n> \tgit status\n\nIn existing codepaths, we have \"has_symlink_leading_path()\" checks to\nnotice that tracked dir/[ab] have disappeared. \"git diff\" before or after\n\"git status\" in the above sequence does notice what you did.\n\nWould dir/a be marked as uptodate in the index, if somebody preloads the\nindex, after the above sequence? I hope not.\n"},{"id":"117717","messageId":"alpine.LFD.2.01.0907091153130.3352@localhost.localdomain","threadId":"20041","inReplyTo":"7vab3d3dpc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] Avoid doing extra 'lstat()'s for d_type if we have an up-to-date cache entry","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-09T18:53:22Z","receivedAt":"2009-07-09T18:53:22Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 9 Jul 2009, Junio C Hamano wrote:\n> \n> Would dir/a be marked as uptodate in the index, if somebody preloads the\n> index, after the above sequence? I hope not.\n\nIndex preloading does not care about directories. It does the standard\n\n\tif (ie_match_stat(index, ce, &st, CE_MATCH_RACY_IS_DIRTY))\n\t\tcontinue;\n\nand since it's all threaded (and the whole _point_ is that it's threaded), \nit can't do anything fancier. Our lstat cache is _not_ thread-safe.\n\nBut preloading isn't even the only thing to do that. All the merge logics \nalso just do \"ie_match_stat()\", as does git checkout, although maybe the \ndirectory gets validated separately for those cases before recursion.\n\nLooking at \"ce_mark_uptodate()\", I think diff-lib.c is the only one that \nactually does that whole \"has_symlink_leading_path()\" thing (in \n\"check_removed()\").\n\nI guess we could make out lstat cache thread-safe, and have the callers \npass in a per-thread \"struct cache_def *\". That would work well enough for \npreloading (and everybody else could just use some random static one and \npass that in).\n\nAdded Kjetil to cc.\n\n\t\t\tLinus\n"},{"id":"117720","messageId":"alpine.LFD.2.01.0907091344340.3352@localhost.localdomain","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907091153130.3352@localhost.localdomain","subject":"[PATCH 4/3] Avoid using 'lstat()' to figure out directories","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-09T20:44:46Z","receivedAt":"2009-07-09T20:44:46Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nFrom: Linus Torvalds \nDate: Thu, 9 Jul 2009 13:14:28 -0700\nSubject: [PATCH 4/3] Avoid using 'lstat()' to figure out directories\n\nIf we have an up-to-date index entry for a file in that directory, we\ncan know that the directories leading up to that file must be\ndirectories. No need to do an lstat() on the directory.\n\nSigned-off-by: Linus Torvalds \n---\n\nThis is the patch I already sent out earlier. Now it's just numbered. \nThere's going to be an additional three patches to actually give the right \nbehavior for index preloading, so that we can really say \"if CE_UPTODATE \nis set, then the whole directory structure is valid\".\n\n dir.c | 47 ++++++++++++++++++++++++++++++++++++++++++-----\n 1 files changed, 42 insertions(+), 5 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 8a9e7d8..e05b850 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -566,18 +566,55 @@ static int in_pathspec(const char *path, int len, const struct path_simplify *si\n \treturn 0;\n }\n \n+static int get_index_dtype(const char *path, int len)\n+{\n+\tint pos;\n+\tstruct cache_entry *ce;\n+\n+\tce = cache_name_exists(path, len, 0);\n+\tif (ce) {\n+\t\tif (!ce_uptodate(ce))\n+\t\t\treturn DT_UNKNOWN;\n+\t\tif (S_ISGITLINK(ce->ce_mode))\n+\t\t\treturn DT_DIR;\n+\t\t/*\n+\t\t * Nobody actually cares about the\n+\t\t * difference between DT_LNK and DT_REG\n+\t\t */\n+\t\treturn DT_REG;\n+\t}\n+\n+\t/* Try to look it up as a directory */\n+\tpos = cache_name_pos(path, len);\n+\tif (pos >= 0)\n+\t\treturn DT_UNKNOWN;\n+\tpos = -pos-1;\n+\twhile (pos < active_nr) {\n+\t\tce = active_cache[pos++];\n+\t\tif (strncmp(ce->name, path, len))\n+\t\t\tbreak;\n+\t\tif (ce->name[len] > '/')\n+\t\t\tbreak;\n+\t\tif (ce->name[len] < '/')\n+\t\t\tcontinue;\n+\t\tif (!ce_uptodate(ce))\n+\t\t\tbreak;\t/* continue? */\n+\t\treturn DT_DIR;\n+\t}\n+\treturn DT_UNKNOWN;\n+}\n+\n static int get_dtype(struct dirent *de, const char *path, int len)\n {\n \tint dtype = de ? DTYPE(de) : DT_UNKNOWN;\n-\tstruct cache_entry *ce;\n \tstruct stat st;\n \n \tif (dtype != DT_UNKNOWN)\n \t\treturn dtype;\n-\tce = cache_name_exists(path, len, 0);\n-\tif (ce && ce_uptodate(ce))\n-\t\tst.st_mode = ce->ce_mode;\n-\telse if (lstat(path, &st))\n+\tdtype = get_index_dtype(path, len);\n+\tif (dtype != DT_UNKNOWN)\n+\t\treturn dtype;\n+\tif (lstat(path, &st))\n \t\treturn dtype;\n \tif (S_ISREG(st.st_mode))\n \t\treturn DT_REG;\n-- \n1.6.3.3.415.ga8877\n"},{"id":"117721","messageId":"alpine.LFD.2.01.0907091344530.3352@localhost.localdomain","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907091344340.3352@localhost.localdomain","subject":"[PATCH 5/3] Prepare symlink caching for thread-safety","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-09T20:47:01Z","receivedAt":"2009-07-09T20:47:01Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nFrom: Linus Torvalds \nDate: Thu, 9 Jul 2009 13:23:59 -0700\nSubject: [PATCH 5/3] Prepare symlink caching for thread-safety\n\nThis doesn't actually change the external interfaces, so they are still\nthread-unsafe, but it makes the code internally pass a pointer to a\nlocal 'struct cache_def' around, so that the core code can be made\nthread-safe.\n\nThe threaded index preloading will want to verify that the paths leading\nup to a pathname are all real directories.\n\nSigned-off-by: Linus Torvalds \n---\n\nNo real changes, but I renamed the static 'cache' data structure \n'default_cache', and made all the internal functions take a pointer \ninstead of using the static version.\n\nThe functions with external linkage are left semantically unchanged by \njust making them do a simple\n\n\tstruct cache_def *cache = &default_cache;\n\nand then using that.\n\n symlinks.c | 75 ++++++++++++++++++++++++++++++++----------------------------\n 1 files changed, 40 insertions(+), 35 deletions(-)\n\ndiff --git a/symlinks.c b/symlinks.c\nindex 8dcd632..08ad353 100644\n--- a/symlinks.c\n+++ b/symlinks.c\n@@ -38,13 +38,13 @@ static struct cache_def {\n \tint flags;\n \tint track_flags;\n \tint prefix_len_stat_func;\n-} cache;\n+} default_cache;\n \n-static inline void reset_lstat_cache(void)\n+static inline void reset_lstat_cache(struct cache_def *cache)\n {\n-\tcache.path[0] = '\\0';\n-\tcache.len = 0;\n-\tcache.flags = 0;\n+\tcache->path[0] = '\\0';\n+\tcache->len = 0;\n+\tcache->flags = 0;\n \t/*\n \t * The track_flags and prefix_len_stat_func members is only\n \t * set by the safeguard rule inside lstat_cache()\n@@ -70,23 +70,23 @@ static inline void reset_lstat_cache(void)\n * of the prefix, where the cache should use the stat() function\n * instead of the lstat() function to test each path component.\n */\n-static int lstat_cache(const char *name, int len,\n+static int lstat_cache(struct cache_def *cache, const char *name, int len,\n \t\t int track_flags, int prefix_len_stat_func)\n {\n \tint match_len, last_slash, last_slash_dir, previous_slash;\n \tint match_flags, ret_flags, save_flags, max_len, ret;\n \tstruct stat st;\n \n-\tif (cache.track_flags != track_flags ||\n-\t cache.prefix_len_stat_func != prefix_len_stat_func) {\n+\tif (cache->track_flags != track_flags ||\n+\t cache->prefix_len_stat_func != prefix_len_stat_func) {\n \t\t/*\n \t\t * As a safeguard rule we clear the cache if the\n \t\t * values of track_flags and/or prefix_len_stat_func\n \t\t * does not match with the last supplied values.\n \t\t */\n-\t\treset_lstat_cache();\n-\t\tcache.track_flags = track_flags;\n-\t\tcache.prefix_len_stat_func = prefix_len_stat_func;\n+\t\treset_lstat_cache(cache);\n+\t\tcache->track_flags = track_flags;\n+\t\tcache->prefix_len_stat_func = prefix_len_stat_func;\n \t\tmatch_len = last_slash = 0;\n \t} else {\n \t\t/*\n@@ -94,10 +94,10 @@ static int lstat_cache(const char *name, int len,\n \t\t * the 2 \"excluding\" path types.\n \t\t */\n \t\tmatch_len = last_slash =\n-\t\t\tlongest_path_match(name, len, cache.path, cache.len,\n+\t\t\tlongest_path_match(name, len, cache->path, cache->len,\n \t\t\t\t\t &previous_slash);\n-\t\tmatch_flags = cache.flags & track_flags & (FL_NOENT|FL_SYMLINK);\n-\t\tif (match_flags && match_len == cache.len)\n+\t\tmatch_flags = cache->flags & track_flags & (FL_NOENT|FL_SYMLINK);\n+\t\tif (match_flags && match_len == cache->len)\n \t\t\treturn match_flags;\n \t\t/*\n \t\t * If we now have match_len > 0, we would know that\n@@ -121,18 +121,18 @@ static int lstat_cache(const char *name, int len,\n \tmax_len = len < PATH_MAX ? len : PATH_MAX;\n \twhile (match_len < max_len) {\n \t\tdo {\n-\t\t\tcache.path[match_len] = name[match_len];\n+\t\t\tcache->path[match_len] = name[match_len];\n \t\t\tmatch_len++;\n \t\t} while (match_len < max_len && name[match_len] != '/');\n \t\tif (match_len >= max_len && !(track_flags & FL_FULLPATH))\n \t\t\tbreak;\n \t\tlast_slash = match_len;\n-\t\tcache.path[last_slash] = '\\0';\n+\t\tcache->path[last_slash] = '\\0';\n \n \t\tif (last_slash <= prefix_len_stat_func)\n-\t\t\tret = stat(cache.path, &st);\n+\t\t\tret = stat(cache->path, &st);\n \t\telse\n-\t\t\tret = lstat(cache.path, &st);\n+\t\t\tret = lstat(cache->path, &st);\n \n \t\tif (ret) {\n \t\t\tret_flags = FL_LSTATERR;\n@@ -156,9 +156,9 @@ static int lstat_cache(const char *name, int len,\n \t */\n \tsave_flags = ret_flags & track_flags & (FL_NOENT|FL_SYMLINK);\n \tif (save_flags && last_slash > 0 && last_slash <= PATH_MAX) {\n-\t\tcache.path[last_slash] = '\\0';\n-\t\tcache.len = last_slash;\n-\t\tcache.flags = save_flags;\n+\t\tcache->path[last_slash] = '\\0';\n+\t\tcache->len = last_slash;\n+\t\tcache->flags = save_flags;\n \t} else if ((track_flags & FL_DIR) &&\n \t\t last_slash_dir > 0 && last_slash_dir <= PATH_MAX) {\n \t\t/*\n@@ -172,11 +172,11 @@ static int lstat_cache(const char *name, int len,\n \t\t * can still cache the path components before the last\n \t\t * one (the found symlink or non-existing component).\n \t\t */\n-\t\tcache.path[last_slash_dir] = '\\0';\n-\t\tcache.len = last_slash_dir;\n-\t\tcache.flags = FL_DIR;\n+\t\tcache->path[last_slash_dir] = '\\0';\n+\t\tcache->len = last_slash_dir;\n+\t\tcache->flags = FL_DIR;\n \t} else {\n-\t\treset_lstat_cache();\n+\t\treset_lstat_cache(cache);\n \t}\n \treturn ret_flags;\n }\n@@ -188,16 +188,17 @@ static int lstat_cache(const char *name, int len,\n void invalidate_lstat_cache(const char *name, int len)\n {\n \tint match_len, previous_slash;\n+\tstruct cache_def *cache = &default_cache;\t/* FIXME */\n \n-\tmatch_len = longest_path_match(name, len, cache.path, cache.len,\n+\tmatch_len = longest_path_match(name, len, cache->path, cache->len,\n \t\t\t\t &previous_slash);\n \tif (len == match_len) {\n-\t\tif ((cache.track_flags & FL_DIR) && previous_slash > 0) {\n-\t\t\tcache.path[previous_slash] = '\\0';\n-\t\t\tcache.len = previous_slash;\n-\t\t\tcache.flags = FL_DIR;\n+\t\tif ((cache->track_flags & FL_DIR) && previous_slash > 0) {\n+\t\t\tcache->path[previous_slash] = '\\0';\n+\t\t\tcache->len = previous_slash;\n+\t\t\tcache->flags = FL_DIR;\n \t\t} else {\n-\t\t\treset_lstat_cache();\n+\t\t\treset_lstat_cache(cache);\n \t\t}\n \t}\n }\n@@ -207,7 +208,8 @@ void invalidate_lstat_cache(const char *name, int len)\n */\n void clear_lstat_cache(void)\n {\n-\treset_lstat_cache();\n+\tstruct cache_def *cache = &default_cache;\t/* FIXME */\n+\treset_lstat_cache(cache);\n }\n \n #define USE_ONLY_LSTAT 0\n@@ -217,7 +219,8 @@ void clear_lstat_cache(void)\n */\n int has_symlink_leading_path(const char *name, int len)\n {\n-\treturn lstat_cache(name, len,\n+\tstruct cache_def *cache = &default_cache;\t/* FIXME */\n+\treturn lstat_cache(cache, name, len,\n \t\t\t FL_SYMLINK|FL_DIR, USE_ONLY_LSTAT) &\n \t\tFL_SYMLINK;\n }\n@@ -228,7 +231,8 @@ int has_symlink_leading_path(const char *name, int len)\n */\n int has_symlink_or_noent_leading_path(const char *name, int len)\n {\n-\treturn lstat_cache(name, len,\n+\tstruct cache_def *cache = &default_cache;\t/* FIXME */\n+\treturn lstat_cache(cache, name, len,\n \t\t\t FL_SYMLINK|FL_NOENT|FL_DIR, USE_ONLY_LSTAT) &\n \t\t(FL_SYMLINK|FL_NOENT);\n }\n@@ -242,7 +246,8 @@ int has_symlink_or_noent_leading_path(const char *name, int len)\n */\n int has_dirs_only_path(const char *name, int len, int prefix_len)\n {\n-\treturn lstat_cache(name, len,\n+\tstruct cache_def *cache = &default_cache;\t/* FIXME */\n+\treturn lstat_cache(cache, name, len,\n \t\t\t FL_DIR|FL_FULLPATH, prefix_len) &\n \t\tFL_DIR;\n }\n-- \n1.6.3.3.415.ga8877\n"},{"id":"117722","messageId":"alpine.LFD.2.01.0907091347080.3352@localhost.localdomain","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907091344530.3352@localhost.localdomain","subject":"[PATCH 6/3] Export thread-safe version of 'has_symlink_leading_path()'","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-09T20:48:41Z","receivedAt":"2009-07-09T20:48:41Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nFrom: Linus Torvalds \nDate: Thu, 9 Jul 2009 13:35:31 -0700\nSubject: [PATCH 6/3] Export thread-safe version of 'has_symlink_leading_path()'\n\nThe threaded index preloading will want it, so that it can avoid\nlocking by simply using a per-thread symlink/directory cache.\n\nSigned-off-by: Linus Torvalds \n---\nThis just exposes a thread-safe version of the symlink checking by \nallowing a caller to pass in its own local 'struct cache_def' to the \nfunction.\n\nNo users of this yet, but the next step is trivial and obvious..\n\n cache.h | 10 ++++++++++\n symlinks.c | 21 ++++++++++-----------\n 2 files changed, 20 insertions(+), 11 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 871c984..f1e5ede 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -744,7 +744,17 @@ struct checkout {\n };\n \n extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);\n+\n+struct cache_def {\n+\tchar path[PATH_MAX + 1];\n+\tint len;\n+\tint flags;\n+\tint track_flags;\n+\tint prefix_len_stat_func;\n+};\n+\n extern int has_symlink_leading_path(const char *name, int len);\n+extern int threaded_has_symlink_leading_path(struct cache_def *, const char *, int);\n extern int has_symlink_or_noent_leading_path(const char *name, int len);\n extern int has_dirs_only_path(const char *name, int len, int prefix_len);\n extern void invalidate_lstat_cache(const char *name, int len);\ndiff --git a/symlinks.c b/symlinks.c\nindex 08ad353..4bdded3 100644\n--- a/symlinks.c\n+++ b/symlinks.c\n@@ -32,13 +32,7 @@ static int longest_path_match(const char *name_a, int len_a,\n \treturn match_len;\n }\n \n-static struct cache_def {\n-\tchar path[PATH_MAX + 1];\n-\tint len;\n-\tint flags;\n-\tint track_flags;\n-\tint prefix_len_stat_func;\n-} default_cache;\n+static struct cache_def default_cache;\n \n static inline void reset_lstat_cache(struct cache_def *cache)\n {\n@@ -217,12 +211,17 @@ void clear_lstat_cache(void)\n /*\n * Return non-zero if path 'name' has a leading symlink component\n */\n+int threaded_has_symlink_leading_path(struct cache_def *cache, const char *name, int len)\n+{\n+\treturn lstat_cache(cache, name, len, FL_SYMLINK|FL_DIR, USE_ONLY_LSTAT) & FL_SYMLINK;\n+}\n+\n+/*\n+ * Return non-zero if path 'name' has a leading symlink component\n+ */\n int has_symlink_leading_path(const char *name, int len)\n {\n-\tstruct cache_def *cache = &default_cache;\t/* FIXME */\n-\treturn lstat_cache(cache, name, len,\n-\t\t\t FL_SYMLINK|FL_DIR, USE_ONLY_LSTAT) &\n-\t\tFL_SYMLINK;\n+\treturn threaded_has_symlink_leading_path(&default_cache, name, len);\n }\n \n /*\n-- \n1.6.3.3.415.ga8877\n"},{"id":"117723","messageId":"alpine.LFD.2.01.0907091348490.3352@localhost.localdomain","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907091347080.3352@localhost.localdomain","subject":"[PATCH 7/3] Make index preloading check the whole path to the file","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-09T20:50:26Z","receivedAt":"2009-07-09T20:50:26Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nFrom: Linus Torvalds \nDate: Thu, 9 Jul 2009 13:37:02 -0700\nSubject: [PATCH 7/3] Make index preloading check the whole path to the file\n\nThis uses the new thread-safe 'threaded_has_symlink_leading_path()'\nfunction to efficiently verify that the whole path leading up to the\nfilename is a proper path, and does not contain symlinks.\n\nThis makes 'ce_uptodate()' a much stronger guarantee: it no longer just\nguarantees that the 'lstat()' of the path would match, it also means\nthat we know that people haven't played games with moving directories\naround and covered it up with symlinks.\n\nSigned-off-by: Linus Torvalds \n---\n\nTotally trivial, now that we have a thread-safe symlink checker.\n\nIf we have leading symlinks in the cache-entry path, we will refuse to \nmark it up-to-date. There's no need to even try to stat anything under \nthat directory.\n\n preload-index.c | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/preload-index.c b/preload-index.c\nindex 88edc5f..c3462dc 100644\n--- a/preload-index.c\n+++ b/preload-index.c\n@@ -34,6 +34,7 @@ static void *preload_thread(void *_data)\n \tstruct thread_data *p = _data;\n \tstruct index_state *index = p->index;\n \tstruct cache_entry **cep = index->cache + p->offset;\n+\tstruct cache_def cache;\n \n \tnr = p->nr;\n \tif (nr + p->offset > index->cache_nr)\n@@ -49,6 +50,8 @@ static void *preload_thread(void *_data)\n \t\t\tcontinue;\n \t\tif (!ce_path_match(ce, p->pathspec))\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\t\tcontinue;\n \t\tif (ie_match_stat(index, ce, &st, CE_MATCH_RACY_IS_DIRTY))\n-- \n1.6.3.3.415.ga8877\n"},{"id":"117725","messageId":"alpine.LFD.2.01.0907091351000.3352@localhost.localdomain","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907091348490.3352@localhost.localdomain","subject":"Re: [PATCH 7/3] Make index preloading check the whole path to the file","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-09T20:56:17Z","receivedAt":"2009-07-09T20:56:17Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOk, with these patches, the strace of the index preload looks very clean, \nand has the required tests for the directory components too:\n\n\t...\n\t26504 lstat(\"connect.c\", {st_mode=S_IFREG|0664, st_size=14312, ...}) = 0\n\t26504 lstat(\"contrib\", {st_mode=S_IFDIR|0775, st_size=4096, ...}) = 0\n\t26504 lstat(\"contrib/README\", {st_mode=S_IFREG|0664, st_size=2113, ...}) = 0\n\t26504 lstat(\"contrib/blameview\", {st_mode=S_IFDIR|0775, st_size=4096, ...}) = 0\n\t26504 lstat(\"contrib/blameview/blameview.perl\", {st_mode=S_IFREG|0775, st_size=3776, ...}) = 0\n\t...\n\nie now it actualyl verifies that the directories leading up to filenames \nare really directories by doing lstat() on them. And the symlink cache \nmeans that it doesn't do it for every single pathname, only for the first \nlookup per thread and directory.\n\nMaybe Kjetil wants to check the changes, but quite frankly, it looked \npretty trivial to make that whole has_symlink_leading_path() be \nthread-safe.\n\n\t\t\tLinus\n"},{"id":"117726","messageId":"20090709210513.GB19425@dpotapov.dyndns.org","threadId":"20041","inReplyTo":"7vws6h3ji4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] Avoid doing extra 'lstat()'s for d_type if we have an up-to-date cache entry","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2009-07-09T21:05:13Z","receivedAt":"2009-07-09T21:05:13Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Thu, Jul 09, 2009 at 09:32:03AM -0700, Junio C Hamano wrote:\n> \n> Yeah, in Dmitry's response that crossed with this update patch from you,\n> he says lstat() on directories are still problem---it would be interesting to\n> hear what he sees after applying this patch and retesting.\n\nWith this patch, I see one 'stat' less for each directory, which on my\nrepo resulted in about 10.7% less 'stat' or 4.8% less of the total\nnumber of syscalls. The total run time decreased by 4.6%.\n\nStill, there are many stats for directories -- for each directory I see\n2 + number of subdirectories it has, but I am not sure about its cause.\n\nThere is one strange thing though. Before that patch the number of\n'open' for each directory was always the same in each run. But after\nthat patch, it slightly differs in each run... Comparing with results\nwithout this patch, the number of open for some directories in some\nbe less by one... which is puzzling...\n\n\nDmitry\n"},{"id":"117730","messageId":"loom.20090709T214734-78@post.gmane.org","threadId":"20041","inReplyTo":"20090709210513.GB19425@dpotapov.dyndns.org","subject":"Re: [PATCH 3/3] Avoid doing extra 'lstat()'s for d_type if we have an up-to-date cache entry","fromName":"Eric Blake","fromEmail":"ebb9@byu.net","sentAt":"2009-07-09T21:52:24Z","receivedAt":"2009-07-09T21:52:24Z","isPatch":true,"sender":{"key":"eblake@redhat.com","avatar":"https://avatars.githubusercontent.com/u/32933908?v=4"},"body":"Dmitry Potapov gmail.com> writes:\n\n> With this patch, I see one 'stat' less for each directory, which on my\n> repo resulted in about 10.7% less 'stat' or 4.8% less of the total\n> number of syscalls. The total run time decreased by 4.6%.\n> \n> Still, there are many stats for directories -- for each directory I see\n> 2 + number of subdirectories it has, but I am not sure about its cause.\n\nThat would probably be the fact that in cygwin 1.5, a stat() of a directory\nresults in querying all the contents of the directory so as to correctly\npopulate the st_link member based on the number of subdirectories. In cygwin\n1.7, in addition to adding the d_type member to readdir, stat was also changed\nto blindly return st_link of 1 for all directories rather than wasting time\npopulating the st_link member (since Windows provides no efficient way of\naccessing that number).\n\n-- \nEric Blake\n"},{"id":"117731","messageId":"4A5670F3.9020309@gnu.org","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907091344340.3352@localhost.localdomain","subject":"Re: [PATCH 4/3] Avoid using 'lstat()' to figure out directories","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2009-07-09T22:36:35Z","receivedAt":"2009-07-09T22:36:35Z","isPatch":true,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"> +\t\tif (ce->name[len]> '/')\n> +\t\t\tbreak;\n> +\t\tif (ce->name[len]< '/')\n> +\t\t\tcontinue;\n\nWhat about\n\n\tif (ce->name[len] < '/') {\n\t\tif (strchr(ce->name + len + 1, '/'))\n\t\t\tbreak;\n\t\telse\n\t\t\tcontinue;\n\t}\n\nto just punt if we'd go into a directory? I'm not much worried about \naccessing foo-0001, foo-0002, foo-0003 while looking for foo/a (that \nwould be O(number of files in a directory), which is bearable), but \nrisking to go down a huge subtree is not very nice.\n\nPaolo\n"},{"id":"117734","messageId":"alpine.LFD.2.01.0907091626080.3352@localhost.localdomain","threadId":"20041","inReplyTo":"4A5670F3.9020309@gnu.org","subject":"Re: [PATCH 4/3] Avoid using 'lstat()' to figure out directories","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-09T23:26:19Z","receivedAt":"2009-07-09T23:26:19Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nOn Fri, 10 Jul 2009, Paolo Bonzini wrote:\n> \n> I'm not much worried about accessing foo-0001, foo-0002, foo-0003 while \n> looking for foo/a (that would be O(number of files in a directory), \n> which is bearable), but risking to go down a huge subtree is not very \n> nice.\n\nThat sounds rather unlikely, and the thing is, even if it were to happen, \nit really wouldn't be that slow. Our data structures are pretty efficient, \nand it wouldn't be _that_ slow to traverse them.\n\nThat said, I don't love that loop. It would be better to do that whole \ncache_name_pos() call with the '/' simply appended to the path, and then \nwe'd do the binary search directly to the first entry.\n\nOf course, since 'path' is a 'const char *', we'd need to either do a \nsilly copy, or we'd need to change a whole lot of the code to make it \nclear that we can actually add a slash to the end (which we can: I think \nit's already always going to be an array that we _will_ add a slash to in \ncase it turns out to be a directory).\n\nSo there's definitely room for improvement there. I just think that the \nimprovement isn't the patch you suggest.\n\n\t\t\tLinus\n"},{"id":"117736","messageId":"20090709232921.GC19425@dpotapov.dyndns.org","threadId":"20041","inReplyTo":"20090709210513.GB19425@dpotapov.dyndns.org","subject":"Re: [PATCH 3/3] Avoid doing extra 'lstat()'s for d_type if we have an up-to-date cache entry","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2009-07-09T23:29:21Z","receivedAt":"2009-07-09T23:29:21Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Fri, Jul 10, 2009 at 01:05:13AM +0400, Dmitry Potapov wrote:\n> \n> There is one strange thing though. Before that patch the number of\n> 'open' for each directory was always the same in each run. But after\n> that patch, it slightly differs in each run... Comparing with results\n> without this patch, the number of open for some directories in some\n> be less by one... which is puzzling...\n\nIt appears that is a purely Windows thing... It seems extra opens for\ndirectories inside of the working tree are caused by Windows Prefetcher.\nhttp://en.wikipedia.org/wiki/Prefetcher\n\nAccordingly to the Process Monitor, during start-up, it opens and reads\nmost directories in the repo that have subdirectories but sometimes it\nskips some of them... So, the patch works as expected... Perhaps, I\nshould disable this prefetcher for testing to get more reproduceable\nresults. Anyway, this prefetecher does not issue QueryOpen (stat) for\nfiles in the repo, so my numbers for 'stat' are not affected by it.\n\n\nDmitry\n"},{"id":"117737","messageId":"20090709233024.GD19425@dpotapov.dyndns.org","threadId":"20041","inReplyTo":"loom.20090709T214734-78@post.gmane.org","subject":"Re: [PATCH 3/3] Avoid doing extra 'lstat()'s for d_type if we have?an up-to-date cache entry","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2009-07-09T23:30:24Z","receivedAt":"2009-07-09T23:30:24Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Thu, Jul 09, 2009 at 09:52:24PM +0000, Eric Blake wrote:\n> Dmitry Potapov gmail.com> writes:\n> \n> > With this patch, I see one 'stat' less for each directory, which on my\n> > repo resulted in about 10.7% less 'stat' or 4.8% less of the total\n> > number of syscalls. The total run time decreased by 4.6%.\n> > \n> > Still, there are many stats for directories -- for each directory I see\n> > 2 + number of subdirectories it has, but I am not sure about its cause.\n> \n> That would probably be the fact that in cygwin 1.5, a stat() of a directory\n> results in querying all the contents of the directory so as to correctly\n> populate the st_link member based on the number of subdirectories.\n\nWe do not use lstat or fstat provided by Cygwin. Instead of it, we use\ntheir fast and dirty analogues, which you can find in compat/cygwin.c\n(they do not provide all information that normal functions provide, but\nthis information is sufficient for Git.\n\nBut we still use readdir() from Cygwin and that may be source of extra\nsyscalls that I observe...\n\nDmitry\n"},{"id":"117738","messageId":"7vd489zavf.fsf@alter.siamese.dyndns.org","threadId":"20041","inReplyTo":"4A5670F3.9020309@gnu.org","subject":"Re: [PATCH 4/3] Avoid using 'lstat()' to figure out directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-09T23:37:24Z","receivedAt":"2009-07-09T23:37:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paolo Bonzini writes:\n\n>> +\t\tif (ce->name[len]> '/')\n>> +\t\t\tbreak;\n>> +\t\tif (ce->name[len]< '/')\n>> +\t\t\tcontinue;\n>\n> What about\n>\n> \tif (ce->name[len] < '/') {\n> \t\tif (strchr(ce->name + len + 1, '/'))\n> \t\t\tbreak;\n> \t\telse\n> \t\t\tcontinue;\n> \t}\n>\n> to just punt if we'd go into a directory? I'm not much worried about\n> accessing foo-0001, foo-0002, foo-0003 while looking for foo/a (that\n> would be O(number of files in a directory), which is bearable), but\n> risking to go down a huge subtree is not very nice.\n\nI am not so sure about \"go down\" part. After all, what the loop does is\nto scan an array of pointers to cache entries and the \"continue\" causes\nthe loop to iterate until you find a path that is in the directory in\nquestion. It is all in userspace code walking on a flat namespace and\nthere is no \"we are going down into a subdirectory and need to open\nanother directory node\" kind of overhead associated with it.\n\nHow expensive is it to do this, compared to an lstat() on a system that\ndoes not have dtype in \"struct stat\" (which means \"lstat() is very cheap\non Linux\" does not even get into the picture)? IOW, how many cache\nentries can we afford to check their names with strncmp, before the cost\nof doing so gets more expensive than a single lstat() on say Cygwin?\n\nI am hoping that the userland is userland and even on Windows it will run\nat full CPU speed, while lstat() may need to pay penalty on Windows due to\nPOSIXy emulation layer, so the tradeoff might turn out to be that we can\nafford to test quite many cache entries and still win if we can save a\nsingle lstat().\n"},{"id":"117740","messageId":"alpine.LFD.2.01.0907091647400.3352@localhost.localdomain","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907091626080.3352@localhost.localdomain","subject":"Re: [PATCH 4/3] Avoid using 'lstat()' to figure out directories","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-09T23:52:47Z","receivedAt":"2009-07-09T23:52:47Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 9 Jul 2009, Linus Torvalds wrote:\n>\n> Of course, since 'path' is a 'const char *', we'd need to either do a \n> silly copy, or we'd need to change a whole lot of the code to make it \n> clear that we can actually add a slash to the end (which we can: I think \n> it's already always going to be an array that we _will_ add a slash to in \n> case it turns out to be a directory).\n\nNo, I was wrong. We really do give it an array that we can't change \nthrough the 'excluded()' function.\n\nSo we'd need to do the whole \"copy name and add '/' at the end\" thing. But \nthe upside would then be that after that, we'd not need any looping to \nfind the right ce. So it might be the right thing to do despite the \nextra copy.\n\n\t\t\tLinus\n"},{"id":"117741","messageId":"alpine.LFD.2.01.0907091658210.3352@localhost.localdomain","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907091647400.3352@localhost.localdomain","subject":"Re: [PATCH 4/3] Avoid using 'lstat()' to figure out directories","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-10T00:13:52Z","receivedAt":"2009-07-10T00:13:52Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 9 Jul 2009, Linus Torvalds wrote:\n> \n> So we'd need to do the whole \"copy name and add '/' at the end\" thing. But \n> the upside would then be that after that, we'd not need any looping to \n> find the right ce. So it might be the right thing to do despite the \n> extra copy.\n\nNaah. I did the numbers. For any normal repository, the 'loop' is going to \nhit exactly once. Trying to be smarter about the initial binary search \nisn't going to help, and copying the pathname around is only going to \nhurt.\n\nIn the Linux repo, there's a small handful of cases like this, eg\n\n - \"arch/x86/vdso32\"\n\tarch/x86/vdso/vdso32-setup.c\n\tarch/x86/vdso/vdso32.S\n\tarch/x86/vdso/vdsp32/\n - \"drivers/scsi/megaraid\"\n\tdrivers/scsi/megaraid.c\n\tdrivers/scsi/megaraid.h\n\tdrivers/scsi/megaraid/\n - \"include/linux/i2c\"\n\tinclude/linux/i2c-algo-bit.h\n\tinclude/linux/i2c-algo-pca.h\n\tinclude/linux/i2c-algo-pcf.h\n\tinclude/linux/i2c-dev.h\n\tinclude/linux/i2c-gpio.h\n\tinclude/linux/i2c-id.h\n\tinclude/linux/i2c-ocores.h\n\tinclude/linux/i2c-pca-platform.h\n\tinclude/linux/i2c-pnx.h\n\tinclude/linux/i2c-pxa.h\n\tinclude/linux/i2c.h\n\tinclude/linux/i2c/\n\netc (for a total of 45 cases in the whole kernel, if I did my script \nright), where we'd loop a few times. But we'd spend more effort trying to \navoid looping than we spend now on the loop.\n\n\t\tLinus\n"},{"id":"117745","messageId":"7v8wixw7s0.fsf@alter.siamese.dyndns.org","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907091348490.3352@localhost.localdomain","subject":"Re: [PATCH 7/3] Make index preloading check the whole path to the file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-10T03:12:31Z","receivedAt":"2009-07-10T03:12:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> From: Linus Torvalds \n> Date: Thu, 9 Jul 2009 13:37:02 -0700\n> Subject: [PATCH 7/3] Make index preloading check the whole path to the file\n>\n> This uses the new thread-safe 'threaded_has_symlink_leading_path()'\n> function to efficiently verify that the whole path leading up to the\n> filename is a proper path, and does not contain symlinks.\n>\n> This makes 'ce_uptodate()' a much stronger guarantee: it no longer just\n> guarantees that the 'lstat()' of the path would match, it also means\n> that we know that people haven't played games with moving directories\n> around and covered it up with symlinks.\n>\n> Signed-off-by: Linus Torvalds \n> ---\n>\n> Totally trivial, now that we have a thread-safe symlink checker.\n>\n> If we have leading symlinks in the cache-entry path, we will refuse to \n> mark it up-to-date. There's no need to even try to stat anything under \n> that directory.\n>\n> preload-index.c | 3 +++\n> 1 files changed, 3 insertions(+), 0 deletions(-)\n>\n> diff --git a/preload-index.c b/preload-index.c\n> index 88edc5f..c3462dc 100644\n> --- a/preload-index.c\n> +++ b/preload-index.c\n> @@ -34,6 +34,7 @@ static void *preload_thread(void *_data)\n> \tstruct thread_data *p = _data;\n> \tstruct index_state *index = p->index;\n> \tstruct cache_entry **cep = index->cache + p->offset;\n> +\tstruct cache_def cache;\n> \n> \tnr = p->nr;\n> \tif (nr + p->offset > index->cache_nr)\n> @@ -49,6 +50,8 @@ static void *preload_thread(void *_data)\n> \t\t\tcontinue;\n> \t\tif (!ce_path_match(ce, p->pathspec))\n> \t\t\tcontinue;\n> +\t\tif (threaded_has_symlink_leading_path(&cache, ce->name, ce_namelen(ce)))\n> +\t\t\tcontinue;\n\nI must be missing something very obvious, but how would this call behave\non an uninitialized cache defined above, or do we need reset_lstat_cache()\non it before the first use?\n\n> \t\tif (lstat(ce->name, &st))\n> \t\t\tcontinue;\n> \t\tif (ie_match_stat(index, ce, &st, CE_MATCH_RACY_IS_DIRTY))\n> -- \n> 1.6.3.3.415.ga8877\n"},{"id":"117746","messageId":"alpine.LFD.2.01.0907092028480.3352@localhost.localdomain","threadId":"20041","inReplyTo":"7v8wixw7s0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/3] Make index preloading check the whole path to the file","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-10T03:29:05Z","receivedAt":"2009-07-10T03:29:05Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 9 Jul 2009, Junio C Hamano wrote:\n> \n> I must be missing something very obvious, but how would this call behave\n> on an uninitialized cache defined above, or do we need reset_lstat_cache()\n> on it before the first use?\n\nNeither.\n\nIt should be memset() to zero. Good catch.\n\n\t\t\tLinus\n"},{"id":"117749","messageId":"alpine.LFD.2.01.0907092039140.3352@localhost.localdomain","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907092028480.3352@localhost.localdomain","subject":"Re: [PATCH 7/3] Make index preloading check the whole path to the file","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-10T03:40:09Z","receivedAt":"2009-07-10T03:40:09Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 9 Jul 2009, Linus Torvalds wrote:\n> \n> It should be memset() to zero. Good catch.\n\nIOW, just this on top. It's the same initialization that 'default_cache' \nhas, except in the case of default_cahe it was implicit in the \"static\", \nwhich is why I missed it when I did the \"obvious\" version.\n\n\t\tLinus\n---\n preload-index.c | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/preload-index.c b/preload-index.c\nindex c3462dc..14d5281 100644\n--- a/preload-index.c\n+++ b/preload-index.c\n@@ -36,6 +36,7 @@ static void *preload_thread(void *_data)\n \tstruct cache_entry **cep = index->cache + p->offset;\n \tstruct cache_def cache;\n \n+\tmemset(&cache, 0, sizeof(cache));\n \tnr = p->nr;\n \tif (nr + p->offset > index->cache_nr)\n \t\tnr = index->cache_nr - p->offset;\n"},{"id":"117767","messageId":"20090710130407.GE19425@dpotapov.dyndns.org","threadId":"20041","inReplyTo":"20090709233024.GD19425@dpotapov.dyndns.org","subject":"Re: [PATCH 3/3] Avoid doing extra 'lstat()'s for d_type if we have?an up-to-date cache entry","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2009-07-10T13:04:07Z","receivedAt":"2009-07-10T13:04:07Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Fri, Jul 10, 2009 at 03:30:24AM +0400, Dmitry Potapov wrote:\n> \n> But we still use readdir() from Cygwin and that may be source of extra\n> syscalls that I observe...\n\nopendir gives an extra 'stat' before opening directory\nreaddir produces one more extra 'stat' on the parent directory before\n returning '..'\nopen(.gitignore) does one extra 'stat' on the directory where it tries\n to open .gitignore (it did not exist in my tests)\n\nSo, the number of 'stat' on each directory is 2 plus the number of\nsubidectories that it has. Thus, the total number of 'stat' for all\ndirectories is 3 multiple the number of directories in your repo. All\nthose 'stat' are artifacts of Cygwin. Also, you have 2 open per each\ndirectory and one of them are redundant (at least, for Git purposes).\nOverall (including syscalls for .gitignore), you have the following\nnumber of syscalls for each directory in your repo:\n 5 - QueryOpen (stat)\n 3 - CreateFile (open)\n 2 - CloseFile (close)\n 1 - QueryFileInternalInformationFile\n\nHere is the detail listing of testing of read_directory_recursive:\n=====\nopendir(.)\n\tQueryOpen,E:\\dpotapov\\repo\n\tCreateFile,E:\\dpotapov\\repo\nfirst readdir call\n\tQueryDirectory,E:\\dpotapov\\repo\nsecond readdir call that returns '..'\n\tQueryOpen,E:\\dpotapov\n\tCreateFile,E:\\dpotapov\n\tQueryFileInternalInformationFile,E:\\dpotapov\n\tCloseFile,E:\\dpotapov\nopen(.gitignore) -- .gitignore does not exist\n\tQueryOpen,E:\\dpotapov\\repo\\.gitignore\n\tQueryOpen,E:\\dpotapov\\repo\\.gitignore.lnk\n\tQueryOpen,E:\\dpotapov\\repo\n\tCreateFile,E:\\dpotapov\\repo\\.gitignore\nstat for untracked file\n\tQueryOpen,E:\\dpotapov\\repo\\bar\nopendir(dir1)\n\tQueryOpen,E:\\dpotapov\\repo\\dir1\n\tCreateFile,E:\\dpotapov\\repo\\dir1\nfirst readdir call\n\tQueryDirectory,E:\\dpotapov\\repo\\dir1\nsecond readdir call that returns '..'\n\tQueryOpen,E:\\dpotapov\\repo\n\tCreateFile,E:\\dpotapov\\repo\n\tQueryFileInternalInformationFile,E:\\dpotapov\\repo\n\tCloseFile,E:\\dpotapov\\repo\nopen(.gitignore) -- .gitignore does not exist\n\tQueryOpen,E:\\dpotapov\\repo\\dir1\\.gitignore\n\tQueryOpen,E:\\dpotapov\\repo\\dir1\\.gitignore.lnk\n\tQueryOpen,E:\\dpotapov\\repo\\dir1\n\tCreateFile,E:\\dpotapov\\repo\\dir1\\.gitignore\nlast readdir call that returns NULL\n\tQueryDirectory,E:\\dpotapov\\repo\\dir1\nclosedir\n\tCloseFile,E:\\dpotapov\\repo\\dir1\nstat for some modified file\n\tQueryOpen,E:\\dpotapov\\repo\\foo\nlast readdir call that returns NULL\n\tQueryDirectory,E:\\dpotapov\\repo\nclosedir\n\tCloseFile,E:\\dpotapov\\repo\n=====\n\nDmitry\n"},{"id":"117807","messageId":"7veisorkux.fsf@alter.siamese.dyndns.org","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907092028480.3352@localhost.localdomain","subject":"Re: [PATCH 7/3] Make index preloading check the whole path to the file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-11T02:53:26Z","receivedAt":"2009-07-11T02:53:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Thu, 9 Jul 2009, Junio C Hamano wrote:\n>> \n>> I must be missing something very obvious, but how would this call behave\n>> on an uninitialized cache defined above, or do we need reset_lstat_cache()\n>> on it before the first use?\n>\n> Neither.\n>\n> It should be memset() to zero. Good catch.\n\nI actually was hoping to hear \"Didn't you notice that this is the first\nfunction run by the pthread and its stack is zeroed by thread creation\" or\nsomething clever like that ;-)\n\nSquashed in. Thanks.\n"},{"id":"117808","messageId":"alpine.LFD.2.01.0907101957200.3552@localhost.localdomain","threadId":"20041","inReplyTo":"7veisorkux.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/3] Make index preloading check the whole path to the file","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-11T03:04:05Z","receivedAt":"2009-07-11T03:04:05Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 10 Jul 2009, Junio C Hamano wrote:\n> \n> I actually was hoping to hear \"Didn't you notice that this is the first\n> function run by the pthread and its stack is zeroed by thread creation\" or\n> something clever like that ;-)\n\nIt's probably true that it is often zero in practice. I certainly saw no \nproblems in my testing, even though I do have preloading on (partly for \ntesting, partly because it actually helps a bit on my machine).\n\nI also suspect that the way the whole 'cache_def' thing works, even if \nit's initialized with random crud, you'll probably never notice. There are \nall those safety rules that check that 'cache->track_flags' has to match \nthe new value etc in order for the cache to be used. And even when it is \nused, it has no pointers in it, it has that static array and the lengths.\n\nSo I don't think you really even need to have the \"it was zeroed by \naccident\" explanation. It's probably as simple as \"even if it is totally \nuninitialized, that will basically never trigger anything odd in \npractice\".\n\nNot to mention that the whole new index preloading addition was just a new \nsafety feature that we didn't even use to have before - and one that only \nimpacted an _optimization_ that didn't change semantics. So in the end: \neven in the really unlikely situation that the cache would have triggered, \nand returned an incorrect return value, the worst that would have happened \nwould be that the preloading wasn't quite as efficient.\n\nEnd result: you did well by noticing the lack of initializers, but I \n_really_ don't think it could probably ever possibly have mattered in \npractice.\n\n\t\t\tLinus\n"},{"id":"117848","messageId":"86r5wmvk17.fsf@broadpark.no","threadId":"20041","inReplyTo":"alpine.LFD.2.01.0907091347080.3352@localhost.localdomain","subject":"Re: [PATCH 6/3] Export thread-safe version of 'has_symlink_leading_path()'","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-07-12T00:09:56Z","receivedAt":"2009-07-12T00:09:56Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Linus Torvalds writes:\n\n> From: Linus Torvalds \n> Date: Thu, 9 Jul 2009 13:35:31 -0700\n> Subject: [PATCH 6/3] Export thread-safe version of 'has_symlink_leading_path()'\n>\n> The threaded index preloading will want it, so that it can avoid\n> locking by simply using a per-thread symlink/directory cache.\n>\n> Signed-off-by: Linus Torvalds \n> ---\n> This just exposes a thread-safe version of the symlink checking by \n> allowing a caller to pass in its own local 'struct cache_def' to the \n> function.\n>\n> No users of this yet, but the next step is trivial and obvious..\n>\n> cache.h | 10 ++++++++++\n> symlinks.c | 21 ++++++++++-----------\n> 2 files changed, 20 insertions(+), 11 deletions(-)\n>\n> diff --git a/cache.h b/cache.h\n> index 871c984..f1e5ede 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -744,7 +744,17 @@ struct checkout {\n> };\n> \n> extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);\n> +\n> +struct cache_def {\n> +\tchar path[PATH_MAX + 1];\n> +\tint len;\n> +\tint flags;\n> +\tint track_flags;\n> +\tint prefix_len_stat_func;\n> +};\n> +\n> extern int has_symlink_leading_path(const char *name, int len);\n> +extern int threaded_has_symlink_leading_path(struct cache_def *, const char *, int);\n> extern int has_symlink_or_noent_leading_path(const char *name, int len);\n> extern int has_dirs_only_path(const char *name, int len, int prefix_len);\n> extern void invalidate_lstat_cache(const char *name, int len);\n> diff --git a/symlinks.c b/symlinks.c\n> index 08ad353..4bdded3 100644\n> --- a/symlinks.c\n> +++ b/symlinks.c\n> @@ -32,13 +32,7 @@ static int longest_path_match(const char *name_a, int len_a,\n> \treturn match_len;\n> }\n> \n> -static struct cache_def {\n> -\tchar path[PATH_MAX + 1];\n> -\tint len;\n> -\tint flags;\n> -\tint track_flags;\n> -\tint prefix_len_stat_func;\n> -} default_cache;\n> +static struct cache_def default_cache;\n> \n> static inline void reset_lstat_cache(struct cache_def *cache)\n> {\n> @@ -217,12 +211,17 @@ void clear_lstat_cache(void)\n> /*\n> * Return non-zero if path 'name' has a leading symlink component\n> */\n> +int threaded_has_symlink_leading_path(struct cache_def *cache, const char *name, int len)\n> +{\n> +\treturn lstat_cache(cache, name, len, FL_SYMLINK|FL_DIR, USE_ONLY_LSTAT) & FL_SYMLINK;\n\n OK, to follow the style the 3 previous lstat_cache() calls was made\n with (and also let the line length be less than 80), it should have\n been written like this:\n\n \treturn lstat_cache(cache, name, len,\n\t\t\t FL_SYMLINK|FL_DIR, USE_ONLY_LSTAT) &\n\t\tFL_SYMLINK;\n\n Notice that the parmeters which is just copied as arguments to l_c()\n is in the same order and on the first line for it self. The next line\n contains the rest of the arguments, and the &-part is also on it\n a separate line.\n\n Stylefix only, so not a big deal.\n\n> +}\n> +\n> +/*\n> + * Return non-zero if path 'name' has a leading symlink component\n> + */\n> int has_symlink_leading_path(const char *name, int len)\n> {\n> -\tstruct cache_def *cache = &default_cache;\t/* FIXME */\n\n This would make it inconsistent with the 2 has_*_() functions below,\n which both have such a line. Only stylefix, no change in semantics.\n\n I personally liked this line, since it will then be easier to\n \"threadify\" the function with an extra parameter named \"cache\".\n\n> -\treturn lstat_cache(cache, name, len,\n> -\t\t\t FL_SYMLINK|FL_DIR, USE_ONLY_LSTAT) &\n> -\t\tFL_SYMLINK;\n> +\treturn threaded_has_symlink_leading_path(&default_cache, name, len);\n> }\n> \n> /*\n\n I have looked at and tested (the version from the origin/pu branch, so\n it contains the memset() line squashed in) patch 5/3, 6/3 and 7/3, and\n all 3 patches looks correct, so you can add\n\n Reviewed-and-tested-by: Kjetil Barvik\n\n if you want to.\n\n But, I guess it is me which is a litle late to comment things, since I\n already see that all 3 patches is in the pu, next and master branches\n already, less than 3 days after beeing posted to the malinglist.\n\n But, would'nt it be a good thing to let all patches at least be in the\n pu branch for minimum x days before entering next and master? Or: let\n it go minimum x days after beeing posted to the list before entering\n the next and master branch? x = 4?\n\n Since the patches is already in master and next, I guess it is not as\n easy as if the patche(es) has been in pu to make a new version of a\n patch, since both master and next is expected to be fast-forward\n branches.\n\n -- kjetil, which was too late this time, too :-)\n"},{"id":"117875","messageId":"7vfxd1lh73.fsf@alter.siamese.dyndns.org","threadId":"20041","inReplyTo":"86r5wmvk17.fsf@broadpark.no","subject":"Re: [PATCH 6/3] Export thread-safe version of 'has_symlink_leading_path()'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-12T21:33:36Z","receivedAt":"2009-07-12T21:33:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kjetil Barvik writes:\n\n> I have looked at and tested (the version from the origin/pu branch, so\n> it contains the memset() line squashed in) patch 5/3, 6/3 and 7/3, and\n> all 3 patches looks correct, so you can add\n>\n> Reviewed-and-tested-by: Kjetil Barvik\n>\n> if you want to.\n\nThanks.\n\n> But, I guess it is me which is a litle late to comment things, since I\n> already see that all 3 patches is in the pu, next and master branches\n> already, less than 3 days after beeing posted to the malinglist.\n>\n> But, would'nt it be a good thing to let all patches at least be in the\n> pu branch for minimum x days before entering next and master? Or: let\n> it go minimum x days after beeing posted to the list before entering\n> the next and master branch? x = 4?\n\nThat number x depends on the quality of the patches and to a great extent\nhow well I know the area of the code affected by the patch.\n"}]}