{"thread":{"id":"17678","subject":"[PATCH v4 3/9] lstat_cache(): swap func(length, string) into func(string, length)","startedAt":"2009-02-09T20:54:03Z","lastAt":"2009-02-09T20:54:53Z","messageCount":10,"participants":["Kjetil Barvik"],"isPatch":true,"patchVersion":4,"patchTotal":9},"messages":[{"id":"103888","messageId":"cover.1234211594.git.barvik@broadpark.no","threadId":"17678","inReplyTo":null,"subject":"[PATCH v4 0/9] git checkout: more cleanups, optimisation, less lstat() calls","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-09T20:54:03Z","receivedAt":"2009-02-09T20:54:03Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Changes since v3\n\n-- patch 4/9 ---\n - use memcpy() instead of memmove()\n\n-- patch 9/9 ---\n - NOTE/NB: this patch is only a debug patch, not be included in the\n final GIT release version.\n\nOK, sorry that it took a while, but I was investigating why it seems\nthat patch 1/9 increases the user time. As it is hard to get the\nexact same test results on each run (I guess that \"racy git\" has\nsomething to say in this case), I think that I should let it go, and\nif necessary post a patch later on.\n\nI have not done anything with patch 7/9, since Johannes Sixt wrote:\n\n \"In the case of this patch, the timestamp is queried via the handle\n that made the change, and in this case special case the timestamp\n could be correct nevertheless. The guarantee doesn't cover this\n case, but it would be natural, and perhaps it Just Works?\"\n\nSo, I let it be up to Johannes to decide if there should be an\n\"#ifndef FSTAT_UNRELIABLE\" test around the fstat() usage inside patch\n7/9.\n\n\nKjetil Barvik (9):\n lstat_cache(): small cleanup and optimisation\n lstat_cache(): generalise longest_match_lstat_cache()\n lstat_cache(): swap func(length, string) into func(string, length)\n unlink_entry(): introduce schedule_dir_for_removal()\n create_directories(): remove some memcpy() and strchr() calls\n write_entry(): cleanup of some duplicated code\n write_entry(): use fstat() instead of lstat() when file is open\n show_patch_diff(): remove a call to fstat()\n lstat_cache(): print a warning if doing ping-pong between cache types\n\n Documentation/CodingGuidelines | 3 +\n builtin-add.c | 2 +-\n builtin-apply.c | 2 +-\n builtin-update-index.c | 2 +-\n cache.h | 10 ++-\n combine-diff.c | 4 +-\n diff-lib.c | 2 +-\n dir.c | 2 +-\n entry.c | 108 ++++++++++++-------------\n symlinks.c | 176 ++++++++++++++++++++++++++++++----------\n unpack-trees.c | 34 ++------\n 11 files changed, 207 insertions(+), 138 deletions(-)\n"},{"id":"103889","messageId":"7ee8ddb982314e35da819d0280cf0121bc20fe77.1234211595.git.barvik@broadpark.no","threadId":"17678","inReplyTo":"cover.1234211594.git.barvik@broadpark.no","subject":"[PATCH v4 1/9] lstat_cache(): small cleanup and optimisation","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-09T20:54:04Z","receivedAt":"2009-02-09T20:54:04Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Simplify the if-else test in longest_match_lstat_cache() such that we\nonly have one simple if test. Instead of testing for 'i == cache.len'\nor 'i == len', we transform this to a common test for 'i == max_len'.\n\nAnd to further optimise we use 'i >= max_len' instead of 'i ==\nmax_len', the reason is that it is now the exact opposite of one part\ninside the while-loop termination expression 'i < max_len && name[i]\n== cache.path[i]', and then the compiler can probably reuse a test\ninstruction from it.\n\nWe also throw away the arguments to reset_lstat_cache(), such that all\nthe safeguard logic inside lstat_cache() is handled at one place.\n\nSigned-off-by: Kjetil Barvik \n---\n symlinks.c | 44 ++++++++++++++++++++++++--------------------\n 1 files changed, 24 insertions(+), 20 deletions(-)\n\ndiff --git a/symlinks.c b/symlinks.c\nindex f262b7c..ae57e56 100644\n--- a/symlinks.c\n+++ b/symlinks.c\n@@ -25,27 +25,30 @@ static inline int longest_match_lstat_cache(int len, const char *name,\n \t\t}\n \t\ti++;\n \t}\n-\t/* Is the cached path string a substring of 'name'? */\n-\tif (i == cache.len && cache.len < len && name[cache.len] == '/') {\n-\t\tmatch_len_prev = match_len;\n-\t\tmatch_len = cache.len;\n-\t/* Is 'name' a substring of the cached path string? */\n-\t} else if ((i == len && len < cache.len && cache.path[len] == '/') ||\n-\t\t (i == len && len == cache.len)) {\n+\t/*\n+\t * Is the cached path string a substring of 'name', is 'name'\n+\t * a substring of the cached path string, or is 'name' and the\n+\t * cached path string the exact same string?\n+\t */\n+\tif (i >= max_len && ((len > cache.len && name[cache.len] == '/') ||\n+\t\t\t (len < cache.len && cache.path[len] == '/') ||\n+\t\t\t (len == cache.len))) {\n \t\tmatch_len_prev = match_len;\n-\t\tmatch_len = len;\n+\t\tmatch_len = i;\n \t}\n \t*previous_slash = match_len_prev;\n \treturn match_len;\n }\n \n-static inline void reset_lstat_cache(int track_flags, int prefix_len_stat_func)\n+static inline void reset_lstat_cache(void)\n {\n \tcache.path[0] = '\\0';\n \tcache.len = 0;\n \tcache.flags = 0;\n-\tcache.track_flags = track_flags;\n-\tcache.prefix_len_stat_func = prefix_len_stat_func;\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+\t */\n }\n \n #define FL_DIR (1 << 0)\n@@ -77,11 +80,13 @@ static int lstat_cache(int len, const char *name,\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 we clear the cache if the values of\n-\t\t * track_flags and/or prefix_len_stat_func does not\n-\t\t * match with the last supplied values.\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(track_flags, prefix_len_stat_func);\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\tmatch_len = last_slash = 0;\n \t} else {\n \t\t/*\n@@ -153,7 +158,7 @@ static int lstat_cache(int len, const char *name,\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} else if ((track_flags & FL_DIR) &&\n \t\t last_slash_dir > 0 && last_slash_dir <= PATH_MAX) {\n \t\t/*\n \t\t * We have a separate test for the directory case,\n@@ -170,7 +175,7 @@ static int lstat_cache(int len, const char *name,\n \t\tcache.len = last_slash_dir;\n \t\tcache.flags = FL_DIR;\n \t} else {\n-\t\treset_lstat_cache(track_flags, prefix_len_stat_func);\n+\t\treset_lstat_cache();\n \t}\n \treturn ret_flags;\n }\n@@ -190,8 +195,7 @@ void invalidate_lstat_cache(int len, const char *name)\n \t\t\tcache.len = previous_slash;\n \t\t\tcache.flags = FL_DIR;\n \t\t} else\n-\t\t\treset_lstat_cache(cache.track_flags,\n-\t\t\t\t\t cache.prefix_len_stat_func);\n+\t\t\treset_lstat_cache();\n \t}\n }\n \n@@ -200,7 +204,7 @@ void invalidate_lstat_cache(int len, const char *name)\n */\n void clear_lstat_cache(void)\n {\n-\treset_lstat_cache(0, 0);\n+\treset_lstat_cache();\n }\n \n #define USE_ONLY_LSTAT 0\n-- \n1.6.1.349.g99fa5\n"},{"id":"103890","messageId":"a660c05cd92cd87d7fb08a884b2e5450b34eee45.1234211595.git.barvik@broadpark.no","threadId":"17678","inReplyTo":"cover.1234211594.git.barvik@broadpark.no","subject":"[PATCH v4 2/9] lstat_cache(): generalise longest_match_lstat_cache()","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-09T20:54:05Z","receivedAt":"2009-02-09T20:54:05Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Rename the function to longst_path_match() and generalise it such that\nit can also be used by other functions.\n\nSigned-off-by: Kjetil Barvik \n---\n symlinks.c | 46 ++++++++++++++++++++++++----------------------\n 1 files changed, 24 insertions(+), 22 deletions(-)\n\ndiff --git a/symlinks.c b/symlinks.c\nindex ae57e56..4596aee 100644\n--- a/symlinks.c\n+++ b/symlinks.c\n@@ -1,38 +1,30 @@\n #include \"cache.h\"\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-} cache;\n-\n /*\n * Returns the length (on a path component basis) of the longest\n- * common prefix match of 'name' and the cached path string.\n+ * common prefix match of 'name_a' and 'name_b'.\n */\n-static inline int longest_match_lstat_cache(int len, const char *name,\n-\t\t\t\t\t int *previous_slash)\n+static int longest_path_match(const char *name_a, int len_a,\n+\t\t\t const char *name_b, int len_b,\n+\t\t\t int *previous_slash)\n {\n \tint max_len, match_len = 0, match_len_prev = 0, i = 0;\n \n-\tmax_len = len < cache.len ? len : cache.len;\n-\twhile (i < max_len && name[i] == cache.path[i]) {\n-\t\tif (name[i] == '/') {\n+\tmax_len = len_a < len_b ? len_a : len_b;\n+\twhile (i < max_len && name_a[i] == name_b[i]) {\n+\t\tif (name_a[i] == '/') {\n \t\t\tmatch_len_prev = match_len;\n \t\t\tmatch_len = i;\n \t\t}\n \t\ti++;\n \t}\n \t/*\n-\t * Is the cached path string a substring of 'name', is 'name'\n-\t * a substring of the cached path string, or is 'name' and the\n-\t * cached path string the exact same string?\n+\t * Is 'name_b' a substring of 'name_a', the other way around,\n+\t * or is 'name_a' and 'name_b' the exact same string?\n \t */\n-\tif (i >= max_len && ((len > cache.len && name[cache.len] == '/') ||\n-\t\t\t (len < cache.len && cache.path[len] == '/') ||\n-\t\t\t (len == cache.len))) {\n+\tif (i >= max_len && ((len_a > len_b && name_a[len_b] == '/') ||\n+\t\t\t (len_a < len_b && name_b[len_a] == '/') ||\n+\t\t\t (len_a == len_b))) {\n \t\tmatch_len_prev = match_len;\n \t\tmatch_len = i;\n \t}\n@@ -40,6 +32,14 @@ static inline int longest_match_lstat_cache(int len, const char *name,\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+} cache;\n+\n static inline void reset_lstat_cache(void)\n {\n \tcache.path[0] = '\\0';\n@@ -94,7 +94,8 @@ static int lstat_cache(int len, const char *name,\n \t\t * the 2 \"excluding\" path types.\n \t\t */\n \t\tmatch_len = last_slash =\n-\t\t\tlongest_match_lstat_cache(len, name, &previous_slash);\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\t\treturn match_flags;\n@@ -188,7 +189,8 @@ void invalidate_lstat_cache(int len, const char *name)\n {\n \tint match_len, previous_slash;\n \n-\tmatch_len = longest_match_lstat_cache(len, name, &previous_slash);\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-- \n1.6.1.349.g99fa5\n"},{"id":"103887","messageId":"f81744504a7f7c42fc877b6ae5811afab798ab77.1234211595.git.barvik@broadpark.no","threadId":"17678","inReplyTo":"cover.1234211594.git.barvik@broadpark.no","subject":"[PATCH v4 3/9] lstat_cache(): swap func(length, string) into func(string, length)","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-09T20:54:06Z","receivedAt":"2009-02-09T20:54:06Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Swap function argument pair (length, string) into (string, length) to\nconform with the commonly used order inside the GIT source code.\n\nAlso, add a note about this fact into the coding guidelines.\n\nSigned-off-by: Kjetil Barvik \n---\n Documentation/CodingGuidelines | 3 +++\n builtin-add.c | 2 +-\n builtin-apply.c | 2 +-\n builtin-update-index.c | 2 +-\n cache.h | 8 ++++----\n diff-lib.c | 2 +-\n dir.c | 2 +-\n entry.c | 2 +-\n symlinks.c | 16 ++++++++--------\n unpack-trees.c | 4 ++--\n 10 files changed, 23 insertions(+), 20 deletions(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 0d7fa9c..b8bf618 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -129,3 +129,6 @@ For C programs:\n used in the git core command set (unless your command is clearly\n separate from it, such as an importer to convert random-scm-X\n repositories to git).\n+\n+ - When we pass pair to functions, we should try to\n+ pass them in that order.\ndiff --git a/builtin-add.c b/builtin-add.c\nindex ac98c83..a23ad96 100644\n--- a/builtin-add.c\n+++ b/builtin-add.c\n@@ -148,7 +148,7 @@ static const char **validate_pathspec(int argc, const char **argv, const char *p\n \tif (pathspec) {\n \t\tconst char **p;\n \t\tfor (p = pathspec; *p; p++) {\n-\t\t\tif (has_symlink_leading_path(strlen(*p), *p)) {\n+\t\t\tif (has_symlink_leading_path(*p, strlen(*p))) {\n \t\t\t\tint len = prefix ? strlen(prefix) : 0;\n \t\t\t\tdie(\"'%s' is beyond a symbolic link\", *p + len);\n \t\t\t}\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex f312798..106be94 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -2360,7 +2360,7 @@ static int check_to_create_blob(const char *new_name, int ok_if_exists)\n \t\t * In such a case, path \"new_name\" does not exist as\n \t\t * far as git is concerned.\n \t\t */\n-\t\tif (has_symlink_leading_path(strlen(new_name), new_name))\n+\t\tif (has_symlink_leading_path(new_name, strlen(new_name)))\n \t\t\treturn 0;\n \n \t\treturn error(\"%s: already exists in working directory\", new_name);\ndiff --git a/builtin-update-index.c b/builtin-update-index.c\nindex 5604977..6c55527 100644\n--- a/builtin-update-index.c\n+++ b/builtin-update-index.c\n@@ -195,7 +195,7 @@ static int process_path(const char *path)\n \tstruct stat st;\n \n \tlen = strlen(path);\n-\tif (has_symlink_leading_path(len, path))\n+\tif (has_symlink_leading_path(path, len))\n \t\treturn error(\"'%s' is beyond a symbolic link\", path);\n \n \t/*\ndiff --git a/cache.h b/cache.h\nindex 2d889de..80eeeb7 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -724,10 +724,10 @@ struct checkout {\n };\n \n extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);\n-extern int has_symlink_leading_path(int len, const char *name);\n-extern int has_symlink_or_noent_leading_path(int len, const char *name);\n-extern int has_dirs_only_path(int len, const char *name, int prefix_len);\n-extern void invalidate_lstat_cache(int len, const char *name);\n+extern int has_symlink_leading_path(const char *name, int len);\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 extern void clear_lstat_cache(void);\n \n extern struct alternate_object_database {\ndiff --git a/diff-lib.c b/diff-lib.c\nindex a41e1ec..a3ba20e 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -31,7 +31,7 @@ static int check_removed(const struct cache_entry *ce, struct stat *st)\n \t\t\treturn -1;\n \t\treturn 1;\n \t}\n-\tif (has_symlink_leading_path(ce_namelen(ce), ce->name))\n+\tif (has_symlink_leading_path(ce->name, ce_namelen(ce)))\n \t\treturn 1;\n \tif (S_ISDIR(st->st_mode)) {\n \t\tunsigned char sub[20];\ndiff --git a/dir.c b/dir.c\nindex cfd1ea5..8fb5226 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -720,7 +720,7 @@ int read_directory(struct dir_struct *dir, const char *path, const char *base, i\n {\n \tstruct path_simplify *simplify;\n \n-\tif (has_symlink_leading_path(strlen(path), path))\n+\tif (has_symlink_leading_path(path, strlen(path)))\n \t\treturn dir->nr;\n \n \tsimplify = create_simplify(pathspec);\ndiff --git a/entry.c b/entry.c\nindex 05aa58d..bb6bdb9 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -20,7 +20,7 @@ static void create_directories(const char *path, const struct checkout *state)\n \t\t * we test the path components of the prefix with the\n \t\t * stat() function instead of the lstat() function.\n \t\t */\n-\t\tif (has_dirs_only_path(len, buf, state->base_dir_len))\n+\t\tif (has_dirs_only_path(buf, len, state->base_dir_len))\n \t\t\tcontinue; /* ok, it is already a directory. */\n \n \t\t/*\ndiff --git a/symlinks.c b/symlinks.c\nindex 4596aee..5167286 100644\n--- a/symlinks.c\n+++ b/symlinks.c\n@@ -70,7 +70,7 @@ 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(int len, const char *name,\n+static int lstat_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@@ -185,7 +185,7 @@ static int lstat_cache(int len, const char *name,\n * Invalidate the given 'name' from the cache, if 'name' matches\n * completely with the cache.\n */\n-void invalidate_lstat_cache(int len, const char *name)\n+void invalidate_lstat_cache(const char *name, int len)\n {\n \tint match_len, previous_slash;\n \n@@ -214,9 +214,9 @@ void clear_lstat_cache(void)\n /*\n * Return non-zero if path 'name' has a leading symlink component\n */\n-int has_symlink_leading_path(int len, const char *name)\n+int has_symlink_leading_path(const char *name, int len)\n {\n-\treturn lstat_cache(len, name,\n+\treturn lstat_cache(name, len,\n \t\t\t FL_SYMLINK|FL_DIR, USE_ONLY_LSTAT) &\n \t\tFL_SYMLINK;\n }\n@@ -225,9 +225,9 @@ int has_symlink_leading_path(int len, const char *name)\n * Return non-zero if path 'name' has a leading symlink component or\n * if some leading path component does not exists.\n */\n-int has_symlink_or_noent_leading_path(int len, const char *name)\n+int has_symlink_or_noent_leading_path(const char *name, int len)\n {\n-\treturn lstat_cache(len, name,\n+\treturn lstat_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@@ -239,9 +239,9 @@ int has_symlink_or_noent_leading_path(int len, const char *name)\n * 'prefix_len', thus we then allow for symlinks in the prefix part as\n * long as those points to real existing directories.\n */\n-int has_dirs_only_path(int len, const char *name, int prefix_len)\n+int has_dirs_only_path(const char *name, int len, int prefix_len)\n {\n-\treturn lstat_cache(len, name,\n+\treturn lstat_cache(name, len,\n \t\t\t FL_DIR|FL_FULLPATH, prefix_len) &\n \t\tFL_DIR;\n }\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex e547282..2293158 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -61,7 +61,7 @@ static void unlink_entry(struct cache_entry *ce)\n \tchar *cp, *prev;\n \tchar *name = ce->name;\n \n-\tif (has_symlink_or_noent_leading_path(ce_namelen(ce), ce->name))\n+\tif (has_symlink_or_noent_leading_path(ce->name, ce_namelen(ce)))\n \t\treturn;\n \tif (unlink(name))\n \t\treturn;\n@@ -583,7 +583,7 @@ static int verify_absent(struct cache_entry *ce, const char *action,\n \tif (o->index_only || o->reset || !o->update)\n \t\treturn 0;\n \n-\tif (has_symlink_or_noent_leading_path(ce_namelen(ce), ce->name))\n+\tif (has_symlink_or_noent_leading_path(ce->name, ce_namelen(ce)))\n \t\treturn 0;\n \n \tif (!lstat(ce->name, &st)) {\n-- \n1.6.1.349.g99fa5\n"},{"id":"103892","messageId":"9ccf4c748ea527b8645770c290d53e9f061b1810.1234211595.git.barvik@broadpark.no","threadId":"17678","inReplyTo":"cover.1234211594.git.barvik@broadpark.no","subject":"[PATCH v4 4/9] unlink_entry(): introduce schedule_dir_for_removal()","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-09T20:54:07Z","receivedAt":"2009-02-09T20:54:07Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Currently inside unlink_entry() if we get a successful removal of one\nfile with unlink(), we try to remove the leading directories each and\nevery time. So if one directory containing 200 files is moved to an\nother location we get 199 failed calls to rmdir() and 1 successful\ncall.\n\nTo fix this and avoid some unnecessary calls to rmdir(), we schedule\neach directory for removal and wait much longer before we do the real\ncall to rmdir().\n\nSince the unlink_entry() function is called with alphabetically sorted\nnames, this new function end up being very effective to avoid\nunnecessary calls to rmdir(). In some cases over 95% of all calls to\nrmdir() is removed with this patch.\n\nSigned-off-by: Kjetil Barvik \n---\n cache.h | 2 +\n symlinks.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n unpack-trees.c | 30 +++++----------------------\n 3 files changed, 67 insertions(+), 24 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 80eeeb7..1bf2d4b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -729,6 +729,8 @@ 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 extern void clear_lstat_cache(void);\n+extern void schedule_dir_for_removal(const char *name, int len);\n+extern void remove_scheduled_dirs(void);\n \n extern struct alternate_object_database {\n \tstruct alternate_object_database *next;\ndiff --git a/symlinks.c b/symlinks.c\nindex 5167286..1d6b35b 100644\n--- a/symlinks.c\n+++ b/symlinks.c\n@@ -245,3 +245,62 @@ int has_dirs_only_path(const char *name, int len, int prefix_len)\n \t\t\t FL_DIR|FL_FULLPATH, prefix_len) &\n \t\tFL_DIR;\n }\n+\n+static struct removal_def {\n+\tchar path[PATH_MAX];\n+\tint len;\n+} removal;\n+\n+static void do_remove_scheduled_dirs(int new_len)\n+{\n+\twhile (removal.len > new_len) {\n+\t\tremoval.path[removal.len] = '\\0';\n+\t\tif (rmdir(removal.path))\n+\t\t\tbreak;\n+\t\tdo {\n+\t\t\tremoval.len--;\n+\t\t} while (removal.len > new_len &&\n+\t\t\t removal.path[removal.len] != '/');\n+\t}\n+\tremoval.len = new_len;\n+\treturn;\n+}\n+\n+void schedule_dir_for_removal(const char *name, int len)\n+{\n+\tint match_len, last_slash, i, previous_slash;\n+\n+\tmatch_len = last_slash = i =\n+\t\tlongest_path_match(name, len, removal.path, removal.len,\n+\t\t\t\t &previous_slash);\n+\t/* Find last slash inside 'name' */\n+\twhile (i < len) {\n+\t\tif (name[i] == '/')\n+\t\t\tlast_slash = i;\n+\t\ti++;\n+\t}\n+\n+\t/*\n+\t * If we are about to go down the directory tree, we check if\n+\t * we must first go upwards the tree, such that we then can\n+\t * remove possible empty directories as we go upwards.\n+\t */\n+\tif (match_len < last_slash && match_len < removal.len)\n+\t\tdo_remove_scheduled_dirs(match_len);\n+\t/*\n+\t * If we go deeper down the directory tree, we only need to\n+\t * save the new path components as we go down.\n+\t */\n+\tif (match_len < last_slash) {\n+\t\tmemcpy(&removal.path[match_len], &name[match_len],\n+\t\t last_slash - match_len);\n+\t\tremoval.len = last_slash;\n+\t}\n+\treturn;\n+}\n+\n+void remove_scheduled_dirs(void)\n+{\n+\tdo_remove_scheduled_dirs(0);\n+\treturn;\n+}\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 2293158..e3c3fa1 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -52,36 +52,17 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n \tadd_index_entry(&o->result, new, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|ADD_CACHE_SKIP_DFCHECK);\n }\n \n-/* Unlink the last component and attempt to remove leading\n- * directories, in case this unlink is the removal of the\n- * last entry in the directory -- empty directories are removed.\n+/*\n+ * Unlink the last component and schedule the leading directories for\n+ * removal, such that empty directories get removed.\n */\n static void unlink_entry(struct cache_entry *ce)\n {\n-\tchar *cp, *prev;\n-\tchar *name = ce->name;\n-\n \tif (has_symlink_or_noent_leading_path(ce->name, ce_namelen(ce)))\n \t\treturn;\n-\tif (unlink(name))\n+\tif (unlink(ce->name))\n \t\treturn;\n-\tprev = NULL;\n-\twhile (1) {\n-\t\tint status;\n-\t\tcp = strrchr(name, '/');\n-\t\tif (prev)\n-\t\t\t*prev = '/';\n-\t\tif (!cp)\n-\t\t\tbreak;\n-\n-\t\t*cp = 0;\n-\t\tstatus = rmdir(name);\n-\t\tif (status) {\n-\t\t\t*cp = '/';\n-\t\t\tbreak;\n-\t\t}\n-\t\tprev = cp;\n-\t}\n+\tschedule_dir_for_removal(ce->name, ce_namelen(ce));\n }\n \n static struct checkout state;\n@@ -117,6 +98,7 @@ static int check_updates(struct unpack_trees_options *o)\n \t\t\tcontinue;\n \t\t}\n \t}\n+\tremove_scheduled_dirs();\n \n \tfor (i = 0; i < index->cache_nr; i++) {\n \t\tstruct cache_entry *ce = index->cache[i];\n-- \n1.6.1.349.g99fa5\n"},{"id":"103891","messageId":"03f0dcd2030c4f48a59ae98beda039bf1930aaf5.1234211595.git.barvik@broadpark.no","threadId":"17678","inReplyTo":"cover.1234211594.git.barvik@broadpark.no","subject":"[PATCH v4 5/9] create_directories(): remove some memcpy() and strchr() calls","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-09T20:54:08Z","receivedAt":"2009-02-09T20:54:08Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Remove the call to memcpy() and strchr() for each path component\ntested, and instead add each path component as we go forward inside\nthe while-loop.\n\nImpact: small optimisation\n\nSigned-off-by: Kjetil Barvik \n---\n entry.c | 23 ++++++++++++++---------\n 1 files changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex bb6bdb9..cc8f0c6 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -2,15 +2,19 @@\n #include \"blob.h\"\n #include \"dir.h\"\n \n-static void create_directories(const char *path, const struct checkout *state)\n+static void create_directories(const char *path, int path_len,\n+\t\t\t const struct checkout *state)\n {\n-\tint len = strlen(path);\n-\tchar *buf = xmalloc(len + 1);\n-\tconst char *slash = path;\n-\n-\twhile ((slash = strchr(slash+1, '/')) != NULL) {\n-\t\tlen = slash - path;\n-\t\tmemcpy(buf, path, len);\n+\tchar *buf = xmalloc(path_len + 1);\n+\tint len = 0;\n+\n+\twhile (len < path_len) {\n+\t\tdo {\n+\t\t\tbuf[len] = path[len];\n+\t\t\tlen++;\n+\t\t} while (len < path_len && path[len] != '/');\n+\t\tif (len >= path_len)\n+\t\t\tbreak;\n \t\tbuf[len] = 0;\n \n \t\t/*\n@@ -190,6 +194,7 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t\n \n \tmemcpy(path, state->base_dir, len);\n \tstrcpy(path + len, ce->name);\n+\tlen += ce_namelen(ce);\n \n \tif (!lstat(path, &st)) {\n \t\tunsigned changed = ce_match_stat(ce, &st, CE_MATCH_IGNORE_VALID);\n@@ -218,6 +223,6 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t\n \t\t\treturn error(\"unable to unlink old '%s' (%s)\", path, strerror(errno));\n \t} else if (state->not_new)\n \t\treturn 0;\n-\tcreate_directories(path, state);\n+\tcreate_directories(path, len, state);\n \treturn write_entry(ce, path, state, 0);\n }\n-- \n1.6.1.349.g99fa5\n"},{"id":"103895","messageId":"90c93271deb4372cedf2051a48d1f67f35aeff48.1234211595.git.barvik@broadpark.no","threadId":"17678","inReplyTo":"cover.1234211594.git.barvik@broadpark.no","subject":"[PATCH v4 6/9] write_entry(): cleanup of some duplicated code","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-09T20:54:50Z","receivedAt":"2009-02-09T20:54:50Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"The switch-cases for S_IFREG and S_IFLNK was so similar that it will\nbe better to do some cleanup and use the common parts of it.\n\nAnd the entry.c file should now be clean for 'gcc -Wextra' warnings.\n\nSigned-off-by: Kjetil Barvik \n---\n entry.c | 75 +++++++++++++++++++++++++-------------------------------------\n 1 files changed, 30 insertions(+), 45 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex cc8f0c6..1f53588 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -78,7 +78,7 @@ static int create_file(const char *path, unsigned int mode)\n \treturn open(path, O_WRONLY | O_CREAT | O_EXCL, mode);\n }\n \n-static void *read_blob_entry(struct cache_entry *ce, const char *path, unsigned long *size)\n+static void *read_blob_entry(struct cache_entry *ce, unsigned long *size)\n {\n \tenum object_type type;\n \tvoid *new = read_sha1_file(ce->sha1, &type, size);\n@@ -93,36 +93,51 @@ static void *read_blob_entry(struct cache_entry *ce, const char *path, unsigned\n \n static int write_entry(struct cache_entry *ce, char *path, const struct checkout *state, int to_tempfile)\n {\n-\tint fd;\n-\tlong wrote;\n-\n-\tswitch (ce->ce_mode & S_IFMT) {\n-\t\tchar *new;\n-\t\tstruct strbuf buf;\n-\t\tunsigned long size;\n-\n+\tunsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT;\n+\tint fd, ret;\n+\tchar *new;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tunsigned long size;\n+\tsize_t wrote, newsize = 0;\n+\n+\tswitch (ce_mode_s_ifmt) {\n \tcase S_IFREG:\n-\t\tnew = read_blob_entry(ce, path, &size);\n+\tcase S_IFLNK:\n+\t\tnew = read_blob_entry(ce, &size);\n \t\tif (!new)\n \t\t\treturn error(\"git checkout-index: unable to read sha1 file of %s (%s)\",\n \t\t\t\tpath, sha1_to_hex(ce->sha1));\n \n+\t\tif (ce_mode_s_ifmt == S_IFLNK && has_symlinks && !to_tempfile) {\n+\t\t\tret = symlink(new, path);\n+\t\t\tfree(new);\n+\t\t\tif (ret)\n+\t\t\t\treturn error(\"git checkout-index: unable to create symlink %s (%s)\",\n+\t\t\t\t\t path, strerror(errno));\n+\t\t\tbreak;\n+\t\t}\n+\n \t\t/*\n \t\t * Convert from git internal format to working tree format\n \t\t */\n-\t\tstrbuf_init(&buf, 0);\n-\t\tif (convert_to_working_tree(ce->name, new, size, &buf)) {\n-\t\t\tsize_t newsize = 0;\n+\t\tif (ce_mode_s_ifmt == S_IFREG &&\n+\t\t convert_to_working_tree(ce->name, new, size, &buf)) {\n \t\t\tfree(new);\n \t\t\tnew = strbuf_detach(&buf, &newsize);\n \t\t\tsize = newsize;\n \t\t}\n \n \t\tif (to_tempfile) {\n-\t\t\tstrcpy(path, \".merge_file_XXXXXX\");\n+\t\t\tif (ce_mode_s_ifmt == S_IFREG)\n+\t\t\t\tstrcpy(path, \".merge_file_XXXXXX\");\n+\t\t\telse\n+\t\t\t\tstrcpy(path, \".merge_link_XXXXXX\");\n \t\t\tfd = mkstemp(path);\n-\t\t} else\n+\t\t} else if (ce_mode_s_ifmt == S_IFREG) {\n \t\t\tfd = create_file(path, ce->ce_mode);\n+\t\t} else {\n+\t\t\tfd = create_file(path, 0666);\n+\t\t}\n \t\tif (fd < 0) {\n \t\t\tfree(new);\n \t\t\treturn error(\"git checkout-index: unable to create file %s (%s)\",\n@@ -135,36 +150,6 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \t\tif (wrote != size)\n \t\t\treturn error(\"git checkout-index: unable to write file %s\", path);\n \t\tbreak;\n-\tcase S_IFLNK:\n-\t\tnew = read_blob_entry(ce, path, &size);\n-\t\tif (!new)\n-\t\t\treturn error(\"git checkout-index: unable to read sha1 file of %s (%s)\",\n-\t\t\t\tpath, sha1_to_hex(ce->sha1));\n-\t\tif (to_tempfile || !has_symlinks) {\n-\t\t\tif (to_tempfile) {\n-\t\t\t\tstrcpy(path, \".merge_link_XXXXXX\");\n-\t\t\t\tfd = mkstemp(path);\n-\t\t\t} else\n-\t\t\t\tfd = create_file(path, 0666);\n-\t\t\tif (fd < 0) {\n-\t\t\t\tfree(new);\n-\t\t\t\treturn error(\"git checkout-index: unable to create \"\n-\t\t\t\t\t\t \"file %s (%s)\", path, strerror(errno));\n-\t\t\t}\n-\t\t\twrote = write_in_full(fd, new, size);\n-\t\t\tclose(fd);\n-\t\t\tfree(new);\n-\t\t\tif (wrote != size)\n-\t\t\t\treturn error(\"git checkout-index: unable to write file %s\",\n-\t\t\t\t\tpath);\n-\t\t} else {\n-\t\t\twrote = symlink(new, path);\n-\t\t\tfree(new);\n-\t\t\tif (wrote)\n-\t\t\t\treturn error(\"git checkout-index: unable to create \"\n-\t\t\t\t\t\t \"symlink %s (%s)\", path, strerror(errno));\n-\t\t}\n-\t\tbreak;\n \tcase S_IFGITLINK:\n \t\tif (to_tempfile)\n \t\t\treturn error(\"git checkout-index: cannot create temporary subproject %s\", path);\n-- \n1.6.1.349.g99fa5\n"},{"id":"103894","messageId":"989e0b547a09f372d16375ea08397d1115331470.1234211595.git.barvik@broadpark.no","threadId":"17678","inReplyTo":"cover.1234211594.git.barvik@broadpark.no","subject":"[PATCH v4 7/9] write_entry(): use fstat() instead of lstat() when file is open","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-09T20:54:51Z","receivedAt":"2009-02-09T20:54:51Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Currently inside write_entry() we do an lstat(path, &st) call on a\nfile which have just been opened inside the exact same function. It\nshould be better to call fstat(fd, &st) on the file while it is open,\nand it should be at least as fast as the lstat() method.\n\nSigned-off-by: Kjetil Barvik \n---\n entry.c | 12 +++++++++---\n 1 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 1f53588..5daacc2 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -94,11 +94,12 @@ static void *read_blob_entry(struct cache_entry *ce, unsigned long *size)\n static int write_entry(struct cache_entry *ce, char *path, const struct checkout *state, int to_tempfile)\n {\n \tunsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT;\n-\tint fd, ret;\n+\tint fd, ret, fstat_done = 0;\n \tchar *new;\n \tstruct strbuf buf = STRBUF_INIT;\n \tunsigned long size;\n \tsize_t wrote, newsize = 0;\n+\tstruct stat st;\n \n \tswitch (ce_mode_s_ifmt) {\n \tcase S_IFREG:\n@@ -145,6 +146,11 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \t\t}\n \n \t\twrote = write_in_full(fd, new, size);\n+\t\t/* use fstat() only when path == ce->name */\n+\t\tif (state->refresh_cache && !to_tempfile && !state->base_dir_len) {\n+\t\t\tfstat(fd, &st);\n+\t\t\tfstat_done = 1;\n+\t\t}\n \t\tclose(fd);\n \t\tfree(new);\n \t\tif (wrote != size)\n@@ -161,8 +167,8 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \t}\n \n \tif (state->refresh_cache) {\n-\t\tstruct stat st;\n-\t\tlstat(ce->name, &st);\n+\t\tif (!fstat_done)\n+\t\t\tlstat(ce->name, &st);\n \t\tfill_stat_cache_info(ce, &st);\n \t}\n \treturn 0;\n-- \n1.6.1.349.g99fa5\n"},{"id":"103893","messageId":"0b2bf9500731f079b5e711a8ee7da692da1ee865.1234211595.git.barvik@broadpark.no","threadId":"17678","inReplyTo":"cover.1234211594.git.barvik@broadpark.no","subject":"[PATCH v4 8/9] show_patch_diff(): remove a call to fstat()","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-09T20:54:52Z","receivedAt":"2009-02-09T20:54:52Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Currently inside show_patch_diff() we have an fstat() call after an\nok lstat() call. Since before the call to fstat() we have already\ntested for the link case with S_ISLNK(), the fstat() can be removed.\n\nSigned-off-by: Kjetil Barvik \n---\n combine-diff.c | 4 +---\n 1 files changed, 1 insertions(+), 3 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex bccc018..4300319 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -713,9 +713,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\tresult_size = buf.len;\n \t\t\tresult = strbuf_detach(&buf, NULL);\n \t\t\telem->mode = canon_mode(st.st_mode);\n-\t\t}\n-\t\telse if (0 <= (fd = open(elem->path, O_RDONLY)) &&\n-\t\t\t !fstat(fd, &st)) {\n+\t\t} else if (0 <= (fd = open(elem->path, O_RDONLY))) {\n \t\t\tsize_t len = xsize_t(st.st_size);\n \t\t\tssize_t done;\n \t\t\tint is_file, i;\n-- \n1.6.1.349.g99fa5\n"},{"id":"103896","messageId":"82c70ec625052b93dae3f7b24a79fe058257f494.1234211595.git.barvik@broadpark.no","threadId":"17678","inReplyTo":"cover.1234211594.git.barvik@broadpark.no","subject":"[PATCH v4 9/9] lstat_cache(): print a warning if doing ping-pong between cache types","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-09T20:54:53Z","receivedAt":"2009-02-09T20:54:53Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"This is a debug patch which is only to be used while the lstat_cache()\nis in the test stage, and should be removed/reverted before the final\nrelase.\n\nI think it should be useful to catch these warnings, as I it could be\nan indication of that the cache would not be very effective if it is\ndoing ping-pong by switching between different cache types too many\ntimes.\n\nAlso, if someone is experimenting with the lstat_cache(), this patch\nwill maybe be useful while debugging.\n\nIf someone is able to trigger the warning, then send a mail to the GIT\nmailing list, containing the first 15 lines of the warning, and a\nshort description of the GIT commands to trigger the warnings.\n\nI hope someone is willing to use this patch for a while, to be able to\ncatch possible ping-pong's.\n\nSigned-off-by: Kjetil Barvik \n---\n symlinks.c | 23 +++++++++++++++++++++++\n 1 files changed, 23 insertions(+), 0 deletions(-)\n\ndiff --git a/symlinks.c b/symlinks.c\nindex 1d6b35b..cb255a3 100644\n--- a/symlinks.c\n+++ b/symlinks.c\n@@ -51,6 +51,11 @@ static inline void reset_lstat_cache(void)\n \t */\n }\n \n+#define SWITCHES_BEFORE_WARNING 10\n+static unsigned int cache_switches, number_of_warnings;\n+static unsigned int current_cache_func, last_cache_func;\n+static unsigned int total_calls;\n+\n #define FL_DIR (1 << 0)\n #define FL_NOENT (1 << 1)\n #define FL_SYMLINK (1 << 2)\n@@ -77,6 +82,7 @@ static int lstat_cache(const char *name, int len,\n \tint match_flags, ret_flags, save_flags, max_len, ret;\n \tstruct stat st;\n \n+\ttotal_calls++;\n \tif (cache.track_flags != track_flags ||\n \t cache.prefix_len_stat_func != prefix_len_stat_func) {\n \t\t/*\n@@ -88,6 +94,17 @@ static int lstat_cache(const char *name, int len,\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\tcache_switches++;\n+\t\tif (cache_switches > SWITCHES_BEFORE_WARNING) {\n+\t\t\tif (number_of_warnings < 10 || number_of_warnings % 1000 == 0)\n+\t\t\t\tprintf(\"warning from %s:%d cache_switches:%u > %u \"\\\n+\t\t\t\t \"(current:%u last:%u total:%u)\\n\",\n+\t\t\t\t __FILE__, __LINE__,\n+\t\t\t\t cache_switches, SWITCHES_BEFORE_WARNING,\n+\t\t\t\t current_cache_func, last_cache_func,\n+\t\t\t\t total_calls);\n+\t\t\tnumber_of_warnings++;\n+\t\t}\n \t} else {\n \t\t/*\n \t\t * Check to see if we have a match from the cache for\n@@ -216,6 +233,8 @@ void clear_lstat_cache(void)\n */\n int has_symlink_leading_path(const char *name, int len)\n {\n+\tlast_cache_func = current_cache_func;\n+\tcurrent_cache_func = 1;\n \treturn lstat_cache(name, len,\n \t\t\t FL_SYMLINK|FL_DIR, USE_ONLY_LSTAT) &\n \t\tFL_SYMLINK;\n@@ -227,6 +246,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+\tlast_cache_func = current_cache_func;\n+\tcurrent_cache_func = 2;\n \treturn lstat_cache(name, len,\n \t\t\t FL_SYMLINK|FL_NOENT|FL_DIR, USE_ONLY_LSTAT) &\n \t\t(FL_SYMLINK|FL_NOENT);\n@@ -241,6 +262,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+\tlast_cache_func = current_cache_func;\n+\tcurrent_cache_func = 3;\n \treturn lstat_cache(name, len,\n \t\t\t FL_DIR|FL_FULLPATH, prefix_len) &\n \t\tFL_DIR;\n-- \n1.6.1.349.g99fa5\n"}]}