{"thread":{"id":"32908","subject":"[PATCH] read_directory: avoid invoking exclude machinery on tracked files","startedAt":"2013-02-15T14:17:28Z","lastAt":"2013-02-25T22:01:28Z","messageCount":12,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Duy Nguyen","Pete Wyckoff","Karsten Blees"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"209553","messageId":"1360937848-4426-1-git-send-email-pclouds@gmail.com","threadId":"32908","inReplyTo":null,"subject":"[PATCH] read_directory: avoid invoking exclude machinery on tracked files","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-02-15T14:17:28Z","receivedAt":"2013-02-15T14:17:28Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"read_directory() (and its friendly wrapper fill_directory) collects\nuntracked/ignored files by traversing through the whole worktree (*),\nfeeding every entry to treat_one_path(), where each entry is checked\nagainst .gitignore patterns.\n\nOne may see that tracked files can't be excluded and we do not need to\nrun them through exclude machinery. On repos where there are many\n.gitignore patterns and/or a lot of tracked files, this unnecessary\nprocessing can become expensive.\n\nThis patch avoids it mostly for normal cases. Directories are still\nprocessed as before. DIR_SHOW_IGNORED and DIR_COLLECT_IGNORED are not\nnormally used unless some options are given (e.g. \"checkout\n--overwrite-ignore\", \"add -f\"...) so people still need to pay penalty\nin some cases, just not as often as before.\n\ngit status | webkit linux-2.6 libreoffice-core gentoo-x86\n-------------+----------------------------------------------\nbefore | 1.159s 0.226s 0.415s 0.597s\nafter | 0.778s 0.176s 0.266s 0.556s\nnr. patterns | 89 376 19 0\nnr. tracked | 182k 40k 63k 101k\n\n(*) Not completely true. read_directory may skip recursing into a\n directory if it's entirely excluded and DIR_SHOW_OTHER_DIRECTORIES\n is not set.\n\nTracked-down-by: Karsten Blees \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n For reference:\n http://thread.gmane.org/gmane.comp.version-control.git/215820/focus=216195\n\n dir.c | 17 +++++++++++++----\n 1 file changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 57394e4..bdff256 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1244,7 +1244,19 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,\n \t\t\t\t\t const struct path_simplify *simplify,\n \t\t\t\t\t int dtype, struct dirent *de)\n {\n-\tint exclude = is_excluded(dir, path->buf, &dtype);\n+\tint exclude;\n+\n+\tif (dtype == DT_UNKNOWN)\n+\t\tdtype = get_dtype(de, path->buf, path->len);\n+\n+\tif (!(dir->flags & DIR_SHOW_IGNORED) &&\n+\t !(dir->flags & DIR_COLLECT_IGNORED) &&\n+\t dtype != DT_DIR &&\n+\t cache_name_exists(path->buf, path->len, ignore_case))\n+\t\treturn path_ignored;\n+\n+\texclude = is_excluded(dir, path->buf, &dtype);\n+\n \tif (exclude && (dir->flags & DIR_COLLECT_IGNORED)\n \t && exclude_matches_pathspec(path->buf, path->len, simplify))\n \t\tdir_add_ignored(dir, path->buf, path->len);\n@@ -1256,9 +1268,6 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,\n \tif (exclude && !(dir->flags & DIR_SHOW_IGNORED))\n \t\treturn path_ignored;\n \n-\tif (dtype == DT_UNKNOWN)\n-\t\tdtype = get_dtype(de, path->buf, path->len);\n-\n \tswitch (dtype) {\n \tdefault:\n \t\treturn path_ignored;\n-- \n1.8.1.2.536.gf441e6d\n"},{"id":"209555","messageId":"7vd2w1wmdo.fsf@alter.siamese.dyndns.org","threadId":"32908","inReplyTo":"1360937848-4426-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] read_directory: avoid invoking exclude machinery on tracked files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-15T16:52:03Z","receivedAt":"2013-02-15T16:52:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> read_directory() (and its friendly wrapper fill_directory) collects\n> untracked/ignored files by traversing through the whole worktree (*),\n> feeding every entry to treat_one_path(), where each entry is checked\n> against .gitignore patterns.\n>\n> One may see that tracked files can't be excluded and we do not need to\n> run them through exclude machinery. On repos where there are many\n> .gitignore patterns and/or a lot of tracked files, this unnecessary\n> processing can become expensive.\n>\n> This patch avoids it mostly for normal cases. Directories are still\n> processed as before. DIR_SHOW_IGNORED and DIR_COLLECT_IGNORED are not\n> normally used unless some options are given (e.g. \"checkout\n> --overwrite-ignore\", \"add -f\"...) so people still need to pay penalty\n> in some cases, just not as often as before.\n>\n> git status | webkit linux-2.6 libreoffice-core gentoo-x86\n> -------------+----------------------------------------------\n> before | 1.159s 0.226s 0.415s 0.597s\n> after | 0.778s 0.176s 0.266s 0.556s\n> nr. patterns | 89 376 19 0\n> nr. tracked | 182k 40k 63k 101k\n>\n> (*) Not completely true. read_directory may skip recursing into a\n> directory if it's entirely excluded and DIR_SHOW_OTHER_DIRECTORIES\n> is not set.\n>\n> Tracked-down-by: Karsten Blees \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> For reference:\n> http://thread.gmane.org/gmane.comp.version-control.git/215820/focus=216195\n>\n> dir.c | 17 +++++++++++++----\n> 1 file changed, 13 insertions(+), 4 deletions(-)\n>\n> diff --git a/dir.c b/dir.c\n> index 57394e4..bdff256 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1244,7 +1244,19 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,\n> \t\t\t\t\t const struct path_simplify *simplify,\n> \t\t\t\t\t int dtype, struct dirent *de)\n> {\n> -\tint exclude = is_excluded(dir, path->buf, &dtype);\n> +\tint exclude;\n> +\n> +\tif (dtype == DT_UNKNOWN)\n> +\t\tdtype = get_dtype(de, path->buf, path->len);\n> +\n> +\tif (!(dir->flags & DIR_SHOW_IGNORED) &&\n> +\t !(dir->flags & DIR_COLLECT_IGNORED) &&\n> +\t dtype != DT_DIR &&\n> +\t cache_name_exists(path->buf, path->len, ignore_case))\n> +\t\treturn path_ignored;\n> +\n> +\texclude = is_excluded(dir, path->buf, &dtype);\n> +\n> \tif (exclude && (dir->flags & DIR_COLLECT_IGNORED)\n> \t && exclude_matches_pathspec(path->buf, path->len, simplify))\n> \t\tdir_add_ignored(dir, path->buf, path->len);\n\nInteresting.\n\nIn the current code, we always check if a path is excluded, and when\ndealing with DT_REG/DT_LNK, we call treat_file():\n\n * When such a path is excluded, treat_file() returns true when we\n are not showing ignored directories. This causes treat_one_path()\n to return path_ignored, so for excluded DT_REG/DT_LNK paths when\n no DIR_*_IGNORED is in effect, this change is a correct\n optimization.\n\n * When such a path is not excluded, on the ther hand, and when we\n are not showing ignored directories, treat_file() just returns\n the value of exclude_file, which is initialized to false and is\n not changed in the function. This causes treat_one_path() to\n return path_handled. However, the new code returns path_ignored\n in this case.\n\nWhat guarantees that this change is regression free? I do not seem\nto be able to find anything that checks if the path is already known\nto the index in the original code for the case you special cased\n(i.e. DIR_*_IGNORED is not set and dtype is not DT_DIR). Do all the\ncallers that reach this function in their callgraph, when they get\npath_ignored for a path in the index, behave as if the difference\nbetween path_ignored and path_handled does not matter?\n\n> @@ -1256,9 +1268,6 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,\n> \tif (exclude && !(dir->flags & DIR_SHOW_IGNORED))\n> \t\treturn path_ignored;\n> \n> -\tif (dtype == DT_UNKNOWN)\n> -\t\tdtype = get_dtype(de, path->buf, path->len);\n> -\n> \tswitch (dtype) {\n> \tdefault:\n> \t\treturn path_ignored;\n"},{"id":"209556","messageId":"CACsJy8A6oBjbaX=3iQcSxcwed28KLTk_tN+iuWDLsC512Z2V1Q@mail.gmail.com","threadId":"32908","inReplyTo":"7vd2w1wmdo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] read_directory: avoid invoking exclude machinery on tracked files","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-02-15T18:30:58Z","receivedAt":"2013-02-15T18:30:58Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Feb 15, 2013 at 11:52 PM, Junio C Hamano wrote:\n> In the current code, we always check if a path is excluded, and when\n> dealing with DT_REG/DT_LNK, we call treat_file():\n>\n> * When such a path is excluded, treat_file() returns true when we\n> are not showing ignored directories. This causes treat_one_path()\n> to return path_ignored, so for excluded DT_REG/DT_LNK paths when\n> no DIR_*_IGNORED is in effect, this change is a correct\n> optimization.\n>\n> * When such a path is not excluded, on the ther hand, and when we\n> are not showing ignored directories, treat_file() just returns\n> the value of exclude_file, which is initialized to false and is\n> not changed in the function. This causes treat_one_path() to\n> return path_handled. However, the new code returns path_ignored\n> in this case.\n>\n> What guarantees that this change is regression free?\n\nIf you consider read_directory_recursive alone, there is a regression.\nThe return value of r_d_r depends on path_handled/path_ignored. With\nthis patch, the return value will be different. The return value is\nonly used by treat_directory() in two cases:\n\n - when DIR_SHOW_IGNORED is set, which disables the optimization so no\nregression\n\n - when DIR_HIDE_EMPTY_DIRECTORIES is _not_ set (and neither is\nDIR_SHOW_IGNORED), the optimization is still on and different r_d_r's\nreturn value would lead to different behavior. However I don't think\nit can happen.\n\ntreat_directory checks if the given directory can be found in index.\nIn that case the neither r_d_r calls in treat_directory is reachable.\nIf the given directory cannot be found in the index, the second r_d_r\nis reachable. But then the cache_name_exists() in the patch should\nalways be false (parent not in index, children cannot), so the\noptimization is off and r_d_r returns correctly.\n\nIt's a bit tricky. I'm not sure if I miss anything else.\n\n> I do not seem\n> to be able to find anything that checks if the path is already known\n> to the index in the original code for the case you special cased\n> (i.e. DIR_*_IGNORED is not set and dtype is not DT_DIR). Do all the\n> callers that reach this function in their callgraph, when they get\n> path_ignored for a path in the index, behave as if the difference\n> between path_ignored and path_handled does not matter?\n-- \nDuy\n"},{"id":"209559","messageId":"7vd2w1gyok.fsf@alter.siamese.dyndns.org","threadId":"32908","inReplyTo":"CACsJy8A6oBjbaX=3iQcSxcwed28KLTk_tN+iuWDLsC512Z2V1Q@mail.gmail.com","subject":"Re: [PATCH] read_directory: avoid invoking exclude machinery on tracked files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-15T19:32:59Z","receivedAt":"2013-02-15T19:32:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Fri, Feb 15, 2013 at 11:52 PM, Junio C Hamano wrote:\n>> In the current code, we always check if a path is excluded, and when\n>> dealing with DT_REG/DT_LNK, we call treat_file():\n>>\n>> * When such a path is excluded, treat_file() returns true when we\n>> are not showing ignored directories. This causes treat_one_path()\n>> to return path_ignored, so for excluded DT_REG/DT_LNK paths when\n>> no DIR_*_IGNORED is in effect, this change is a correct\n>> optimization.\n>>\n>> * When such a path is not excluded, on the ther hand, and when we\n>> are not showing ignored directories, treat_file() just returns\n>> the value of exclude_file, which is initialized to false and is\n>> not changed in the function. This causes treat_one_path() to\n>> return path_handled. However, the new code returns path_ignored\n>> in this case.\n>>\n>> What guarantees that this change is regression free?\n>\n> If you consider read_directory_recursive alone, there is a regression.\n> The return value of r_d_r depends on path_handled/path_ignored. With\n> this patch, the return value will be different.\n\nThat is exactly what was missing from the proposed log message, and\nmade me ask \"Do all the callers that reach this function in their\ncallgraph, when they get path_ignored for a path in the index,\nbehave as if the difference between path_ignored and path_handled\ndoes not matter?\" Your answer seems to be\n\n - r-d-r returns 'how many paths in this directory match the\n criteria we are looking for', unless check_only is true. Now in\n some cases we return path_ignored not path_handled, so we may\n return a number that is greater than we used to return.\n\n - treat_directory, the only user of that return value, cares if\n r-d-r returned 0 or non-zero; and\n\n - As long as we keep returning 0 from r-d-r in cases we used to\n return 0 and non-zero in cases we used to return non-zero, exact\n number does not matter. Overall we get the same result.\n\nI think all of the above is true, but I have not convinced myself\nthat r-d-r with the new code never returns 0 when we used to return\nnon-zero.\n\n> ...\n> It's a bit tricky. I'm not sure if I miss anything else.\n\nHrm...\n"},{"id":"209575","messageId":"CACsJy8BgDZCfWLZT4kNa+Qt5n7YZJ_wemh9gKbrtimbjcmj+sg@mail.gmail.com","threadId":"32908","inReplyTo":"7vd2w1gyok.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] read_directory: avoid invoking exclude machinery on tracked files","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-02-16T03:31:36Z","receivedAt":"2013-02-16T03:31:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Feb 16, 2013 at 2:32 AM, Junio C Hamano wrote:\n>> If you consider read_directory_recursive alone, there is a regression.\n>> The return value of r_d_r depends on path_handled/path_ignored. With\n>> this patch, the return value will be different.\n>\n> That is exactly what was missing from the proposed log message, and\n> made me ask \"Do all the callers that reach this function in their\n> callgraph, when they get path_ignored for a path in the index,\n> behave as if the difference between path_ignored and path_handled\n> does not matter?\"\n\nI'll add it the the log message. Although I'm thinking some\nrestructuring to separate tracked file handling from the rest may make\nit clearer (and less error prone in future).\n-- \nDuy\n"},{"id":"209586","messageId":"1360999078-27196-1-git-send-email-pclouds@gmail.com","threadId":"32908","inReplyTo":"1360937848-4426-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2] read_directory: avoid invoking exclude machinery on tracked files","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-02-16T07:17:58Z","receivedAt":"2013-02-16T07:17:58Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"read_directory() (and its friendly wrapper fill_directory) collects\nuntracked/ignored files by traversing through the whole worktree,\nfeeding every entry to treat_one_path(), where each entry is checked\nagainst .gitignore patterns.\n\nOne may see that tracked files can't be excluded and we do not need to\nrun them through exclude machinery. On repos where there are many\n.gitignore patterns and/or a lot of tracked files, this unnecessary\nprocessing can become expensive.\n\nThis patch avoids it mostly for normal cases. Directories are still\nprocessed as before. DIR_SHOW_IGNORED and DIR_COLLECT_IGNORED are not\nnormally used unless some options are given (e.g. \"checkout\n--overwrite-ignore\", \"add -f\"...)\n\ntreat_one_path's behavior changes when taking this shortcut. With\ncurrent code, when a non-directory path is not excluded,\ntreat_one_path calls treat_file, which returns the initial value of\nexclude_file and causes treat_one_path to return path_handled. With\nthis patch, on the same conditions, treat_one_path returns\npath_ignored.\n\nread_directory_recursive() cares about this difference. Check out the\nsnippet:\n\n\twhile (...) {\n\t\tswitch (treat_path(...)) {\n\t\tcase path_ignored:\n\t\t\tcontinue;\n\t\tcase path_handled:\n\t\t\tbreak;\n\t\t}\n\t\tcontents++;\n\t\tif (check_only)\n\t\t\tbreak;\n\t\tdir_add_name(dir, path.buf, path.len);\n\t}\n\nIf path_handled is returned, contents goes up. And if check_only is\ntrue, the loop could be broken early. These will not happen when\ntreat_one_path (and its wrapper treat_path) returns\npath_ignored. dir_add_name internally does a cache_name_exists() check\nso it makes no difference.\n\nTo avoid this behavior change, treat_one_path is instructed to skip\nthe optimization when check_only or contents is used.\n\nFinally some numbers (best of 20 runs) that shows why it's worth all\nthe hassle:\n\ngit status | webkit linux-2.6 libreoffice-core gentoo-x86\n-------------+----------------------------------------------\nbefore | 1.097s 0.208s 0.399s 0.539s\nafter | 0.736s 0.159s 0.248s 0.501s\nnr. patterns | 89 376 19 0\nnr. tracked | 182k 40k 63k 101k\n\nTracked-down-by: Karsten Blees \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Instead of relying on the surrounding code happening to not trigger\n the behavior change in treat_one_path, this round ensures such\n triggers will disable the optimization and fall back to normal code\n path.\n\n There are no big differences in measured numbers, which indicate\n incorrect triggers do not happen, at least in my tests.\n\n dir.c | 79 ++++++++++++++++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 52 insertions(+), 27 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 57394e4..a5fe0a0 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -17,8 +17,11 @@ struct path_simplify {\n \tconst char *path;\n };\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 void read_directory_recursive(struct dir_struct *dir,\n+\t\t\t\t const char *path, int len,\n+\t\t\t\t int check_only,\n+\t\t\t\t const struct path_simplify *simplify,\n+\t\t\t\t int *contents);\n static int get_dtype(struct dirent *de, const char *path, int len);\n \n /* helper string functions with support for the ignore_case flag */\n@@ -1034,6 +1037,7 @@ static enum directory_treatment treat_directory(struct dir_struct *dir,\n \tconst char *dirname, int len, int exclude,\n \tconst struct path_simplify *simplify)\n {\n+\tint contents = 0;\n \t/* The \"len-1\" is to strip the final '/' */\n \tswitch (directory_exists_in_index(dirname, len-1)) {\n \tcase index_directory:\n@@ -1065,19 +1069,19 @@ static enum directory_treatment treat_directory(struct dir_struct *dir,\n \t * check if it contains only ignored files\n \t */\n \tif ((dir->flags & DIR_SHOW_IGNORED) && !exclude) {\n-\t\tint ignored;\n \t\tdir->flags &= ~DIR_SHOW_IGNORED;\n \t\tdir->flags |= DIR_HIDE_EMPTY_DIRECTORIES;\n-\t\tignored = read_directory_recursive(dir, dirname, len, 1, simplify);\n+\t\tread_directory_recursive(dir, dirname, len, 1, simplify, &contents);\n \t\tdir->flags &= ~DIR_HIDE_EMPTY_DIRECTORIES;\n \t\tdir->flags |= DIR_SHOW_IGNORED;\n \n-\t\treturn ignored ? ignore_directory : show_directory;\n+\t\treturn contents ? ignore_directory : show_directory;\n \t}\n \tif (!(dir->flags & DIR_SHOW_IGNORED) &&\n \t !(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))\n \t\treturn show_directory;\n-\tif (!read_directory_recursive(dir, dirname, len, 1, simplify))\n+\tread_directory_recursive(dir, dirname, len, 1, simplify, &contents);\n+\tif (!contents)\n \t\treturn ignore_directory;\n \treturn show_directory;\n }\n@@ -1242,9 +1246,23 @@ enum path_treatment {\n static enum path_treatment treat_one_path(struct dir_struct *dir,\n \t\t\t\t\t struct strbuf *path,\n \t\t\t\t\t const struct path_simplify *simplify,\n-\t\t\t\t\t int dtype, struct dirent *de)\n+\t\t\t\t\t int dtype, struct dirent *de,\n+\t\t\t\t\t int exclude_shortcut_ok)\n {\n-\tint exclude = is_excluded(dir, path->buf, &dtype);\n+\tint exclude;\n+\n+\tif (dtype == DT_UNKNOWN)\n+\t\tdtype = get_dtype(de, path->buf, path->len);\n+\n+\tif (exclude_shortcut_ok &&\n+\t !(dir->flags & DIR_SHOW_IGNORED) &&\n+\t !(dir->flags & DIR_COLLECT_IGNORED) &&\n+\t dtype != DT_DIR &&\n+\t cache_name_exists(path->buf, path->len, ignore_case))\n+\t\treturn path_ignored;\n+\n+\texclude = is_excluded(dir, path->buf, &dtype);\n+\n \tif (exclude && (dir->flags & DIR_COLLECT_IGNORED)\n \t && exclude_matches_pathspec(path->buf, path->len, simplify))\n \t\tdir_add_ignored(dir, path->buf, path->len);\n@@ -1256,9 +1274,6 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,\n \tif (exclude && !(dir->flags & DIR_SHOW_IGNORED))\n \t\treturn path_ignored;\n \n-\tif (dtype == DT_UNKNOWN)\n-\t\tdtype = get_dtype(de, path->buf, path->len);\n-\n \tswitch (dtype) {\n \tdefault:\n \t\treturn path_ignored;\n@@ -1290,7 +1305,8 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n \t\t\t\t struct dirent *de,\n \t\t\t\t struct strbuf *path,\n \t\t\t\t int baselen,\n-\t\t\t\t const struct path_simplify *simplify)\n+\t\t\t\t const struct path_simplify *simplify,\n+\t\t\t\t int exclude_shortcut_ok)\n {\n \tint dtype;\n \n@@ -1302,7 +1318,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n \t\treturn path_ignored;\n \n \tdtype = DTYPE(de);\n-\treturn treat_one_path(dir, path, simplify, dtype, de);\n+\treturn treat_one_path(dir, path, simplify, dtype, de, exclude_shortcut_ok);\n }\n \n /*\n@@ -1314,13 +1330,13 @@ static enum path_treatment treat_path(struct dir_struct *dir,\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,\n-\t\t\t\t const char *base, int baselen,\n-\t\t\t\t int check_only,\n-\t\t\t\t const struct path_simplify *simplify)\n+static void read_directory_recursive(struct dir_struct *dir,\n+\t\t\t\t const char *base, int baselen,\n+\t\t\t\t int check_only,\n+\t\t\t\t const struct path_simplify *simplify,\n+\t\t\t\t int *contents)\n {\n \tDIR *fdir;\n-\tint contents = 0;\n \tstruct dirent *de;\n \tstruct strbuf path = STRBUF_INIT;\n \n@@ -1331,18 +1347,29 @@ static int read_directory_recursive(struct dir_struct *dir,\n \t\tgoto out;\n \n \twhile ((de = readdir(fdir)) != NULL) {\n-\t\tswitch (treat_path(dir, de, &path, baselen, simplify)) {\n+\t\tswitch (treat_path(dir, de, &path, baselen,\n+\t\t\t\t simplify,\n+\t\t\t\t !check_only && !contents)) {\n \t\tcase path_recurse:\n-\t\t\tcontents += read_directory_recursive(dir, path.buf,\n-\t\t\t\t\t\t\t path.len, 0,\n-\t\t\t\t\t\t\t simplify);\n+\t\t\tread_directory_recursive(dir, path.buf,\n+\t\t\t\t\t\t path.len, 0,\n+\t\t\t\t\t\t simplify,\n+\t\t\t\t\t\t contents);\n \t\t\tcontinue;\n \t\tcase path_ignored:\n \t\t\tcontinue;\n \t\tcase path_handled:\n \t\t\tbreak;\n \t\t}\n-\t\tcontents++;\n+\t\t/*\n+\t\t * Update the last argument to treat_path if anything\n+\t\t * else is done after this point. This is because if\n+\t\t * treat_path's exclude_shortcut_ok is true, it may\n+\t\t * incorrectly return path_ignored (and never reaches\n+\t\t * this part) instead of path_handled.\n+\t\t */\n+\t\tif (contents)\n+\t\t\t(*contents)++;\n \t\tif (check_only)\n \t\t\tbreak;\n \t\tdir_add_name(dir, path.buf, path.len);\n@@ -1350,8 +1377,6 @@ static int read_directory_recursive(struct dir_struct *dir,\n \tclosedir(fdir);\n out:\n \tstrbuf_release(&path);\n-\n-\treturn contents;\n }\n \n static int cmp_name(const void *p1, const void *p2)\n@@ -1420,7 +1445,7 @@ static int treat_leading_path(struct dir_struct *dir,\n \t\tif (simplify_away(sb.buf, sb.len, simplify))\n \t\t\tbreak;\n \t\tif (treat_one_path(dir, &sb, simplify,\n-\t\t\t\t DT_DIR, NULL) == path_ignored)\n+\t\t\t\t DT_DIR, NULL, 0) == path_ignored)\n \t\t\tbreak; /* do not recurse into it */\n \t\tif (len <= baselen) {\n \t\t\trc = 1;\n@@ -1440,7 +1465,7 @@ int read_directory(struct dir_struct *dir, const char *path, int len, const char\n \n \tsimplify = create_simplify(pathspec);\n \tif (!len || treat_leading_path(dir, path, len, simplify))\n-\t\tread_directory_recursive(dir, path, len, 0, simplify);\n+\t\tread_directory_recursive(dir, path, len, 0, simplify, NULL);\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);\n-- \n1.8.1.2.536.gf441e6d\n"},{"id":"209587","messageId":"20130216181110.GA27233@padd.com","threadId":"32908","inReplyTo":"1360999078-27196-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2] read_directory: avoid invoking exclude machinery on tracked files","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2013-02-16T18:11:10Z","receivedAt":"2013-02-16T18:11:10Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"pclouds@gmail.com wrote on Sat, 16 Feb 2013 14:17 +0700:\n> Finally some numbers (best of 20 runs) that shows why it's worth all\n> the hassle:\n> \n> git status | webkit linux-2.6 libreoffice-core gentoo-x86\n> -------------+----------------------------------------------\n> before | 1.097s 0.208s 0.399s 0.539s\n> after | 0.736s 0.159s 0.248s 0.501s\n> nr. patterns | 89 376 19 0\n> nr. tracked | 182k 40k 63k 101k\n\nThanks for this work. I repeated some of the tests across NFS,\nwhere I'd expect to see bigger differences. Best of 20 values\nreported in \"min ...\".\n\n webkit\n Stock min 9.61 avg 11.61 +/- 1.35 max 14.26\n Duy min 6.91 avg 7.67 +/- 0.46 max 8.71\n\n linux\n Stock min 2.27 avg 3.16 +/- 0.56 max 4.49\n Duy min 2.04 avg 3.12 +/- 0.69 max 4.87\n\n libreoffice-core\n Stock min 4.56 avg 5.79 +/- 0.79 max 7.08\n Duy min 3.96 avg 5.25 +/- 0.95 max 6.95\n\nSimilar 30%-ish speedup on webkit. And an absolute gain\nof 2.7 seconds is quite nice.\n\n\t\t-- Pete\n"},{"id":"209588","messageId":"CACsJy8C9SGJxwnm1E2N_KyEMg5-MzDt2B+SrX7rygn8X1qq4Wg@mail.gmail.com","threadId":"32908","inReplyTo":"20130216181110.GA27233@padd.com","subject":"Re: [PATCH v2] read_directory: avoid invoking exclude machinery on tracked files","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-02-17T04:39:28Z","receivedAt":"2013-02-17T04:39:28Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Feb 17, 2013 at 1:11 AM, Pete Wyckoff wrote:\n> pclouds@gmail.com wrote on Sat, 16 Feb 2013 14:17 +0700:\n>> Finally some numbers (best of 20 runs) that shows why it's worth all\n>> the hassle:\n>>\n>> git status | webkit linux-2.6 libreoffice-core gentoo-x86\n>> -------------+----------------------------------------------\n>> before | 1.097s 0.208s 0.399s 0.539s\n>> after | 0.736s 0.159s 0.248s 0.501s\n>> nr. patterns | 89 376 19 0\n>> nr. tracked | 182k 40k 63k 101k\n>\n> Thanks for this work. I repeated some of the tests across NFS,\n> where I'd expect to see bigger differences.\n\nThis is about reducing CPU processing time, not I/O time. So no bigger\ndifferences is expected. I/O time can be reduced with inotify, or fam\nin nfs case because inotify does not support nfs.\n\n> Best of 20 values reported in \"min ...\".\n>\n> webkit\n> Stock min 9.61 avg 11.61 +/- 1.35 max 14.26\n> Duy min 6.91 avg 7.67 +/- 0.46 max 8.71\n>\n> linux\n> Stock min 2.27 avg 3.16 +/- 0.56 max 4.49\n> Duy min 2.04 avg 3.12 +/- 0.69 max 4.87\n>\n> libreoffice-core\n> Stock min 4.56 avg 5.79 +/- 0.79 max 7.08\n> Duy min 3.96 avg 5.25 +/- 0.95 max 6.95\n>\n> Similar 30%-ish speedup on webkit. And an absolute gain\n> of 2.7 seconds is quite nice.\n>\n> -- Pete\n\n\n\n-- \nDuy\n"},{"id":"209602","messageId":"20130217154956.GA2395@padd.com","threadId":"32908","inReplyTo":"CACsJy8C9SGJxwnm1E2N_KyEMg5-MzDt2B+SrX7rygn8X1qq4Wg@mail.gmail.com","subject":"Re: [PATCH v2] read_directory: avoid invoking exclude machinery on tracked files","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2013-02-17T15:49:56Z","receivedAt":"2013-02-17T15:49:56Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"pclouds@gmail.com wrote on Sun, 17 Feb 2013 11:39 +0700:\n> On Sun, Feb 17, 2013 at 1:11 AM, Pete Wyckoff wrote:\n> > pclouds@gmail.com wrote on Sat, 16 Feb 2013 14:17 +0700:\n> >> Finally some numbers (best of 20 runs) that shows why it's worth all\n> >> the hassle:\n> >>\n> >> git status | webkit linux-2.6 libreoffice-core gentoo-x86\n> >> -------------+----------------------------------------------\n> >> before | 1.097s 0.208s 0.399s 0.539s\n> >> after | 0.736s 0.159s 0.248s 0.501s\n> >> nr. patterns | 89 376 19 0\n> >> nr. tracked | 182k 40k 63k 101k\n> >\n> > Thanks for this work. I repeated some of the tests across NFS,\n> > where I'd expect to see bigger differences.\n> \n> This is about reducing CPU processing time, not I/O time. So no bigger\n> differences is expected. I/O time can be reduced with inotify, or fam\n> in nfs case because inotify does not support nfs.\n\nNumbers from the last mail were core.preloadindex=true. Here's\n\"time\" output from average runs:\n\n stock = 0m2.28s user 0m4.18s sys 0m11.28s elapsed 57.39 %CPU\n duy = 0m1.25s user 0m4.43s sys 0m7.45s elapsed 76.41 %CPU\n\nWith this huge repo, preloadindex may be stressing directory\ncache behavior on the NFS server or client. Your patch helps\nboth CPU and wait time by avoiding the 6000-odd open() of\nnon-existent .gitignore.\n\nWith core.preloadindex=false, it's a 1 sec speedup, all from CPU:\n\n stock = 0m2.18s user 0m1.59s sys 0m7.78s elapsed 48.45 %CPU\n duy = 0m1.17s user 0m1.63s sys 0m6.91s elapsed 40.59 %CPU\n\n\n\t\t-- Pete\n"},{"id":"209615","messageId":"7vvc9qedh2.fsf@alter.siamese.dyndns.org","threadId":"32908","inReplyTo":"1360999078-27196-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2] read_directory: avoid invoking exclude machinery on tracked files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-17T23:18:33Z","receivedAt":"2013-02-17T23:18:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> If path_handled is returned, contents goes up. And if check_only is\n> true, the loop could be broken early. These will not happen when\n> treat_one_path (and its wrapper treat_path) returns\n> path_ignored. dir_add_name internally does a cache_name_exists() check\n> so it makes no difference.\n>\n> To avoid this behavior change, treat_one_path is instructed to skip\n> the optimization when check_only or contents is used.\n\nOK, that makes more understandable why this is safe.\n\n> @@ -1242,9 +1246,23 @@ enum path_treatment {\n> static enum path_treatment treat_one_path(struct dir_struct *dir,\n> \t\t\t\t\t struct strbuf *path,\n> \t\t\t\t\t const struct path_simplify *simplify,\n> -\t\t\t\t\t int dtype, struct dirent *de)\n> +\t\t\t\t\t int dtype, struct dirent *de,\n> +\t\t\t\t\t int exclude_shortcut_ok)\n> {\n> ... \n> @@ -1331,18 +1347,29 @@ static int read_directory_recursive(struct dir_struct *dir,\n> \t\tgoto out;\n> \n> \twhile ((de = readdir(fdir)) != NULL) {\n> -\t\tswitch (treat_path(dir, de, &path, baselen, simplify)) {\n> +\t\tswitch (treat_path(dir, de, &path, baselen,\n> +\t\t\t\t simplify,\n> +\t\t\t\t !check_only && !contents)) {\n> ...\n\nBetween these two places we may want to say what kind of short-cut\nwe are talking about, but there only is one kind of short-cut at\nthis moment, so let's leave that to other people who want to further\noptimize things in this codepath by adding other short-cuts.\n\nThanks; will queue.\n"},{"id":"209688","messageId":"512259E9.6070703@gmail.com","threadId":"32908","inReplyTo":"7vd2w1gyok.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] read_directory: avoid invoking exclude machinery on tracked files","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2013-02-18T16:42:17Z","receivedAt":"2013-02-18T16:42:17Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 15.02.2013 20:32, schrieb Junio C Hamano:\n> Duy Nguyen writes:\n> \n>> On Fri, Feb 15, 2013 at 11:52 PM, Junio C Hamano wrote:\n>>> In the current code, we always check if a path is excluded, and when\n>>> dealing with DT_REG/DT_LNK, we call treat_file():\n>>>\n>>> * When such a path is excluded, treat_file() returns true when we\n>>> are not showing ignored directories. This causes treat_one_path()\n>>> to return path_ignored, so for excluded DT_REG/DT_LNK paths when\n>>> no DIR_*_IGNORED is in effect, this change is a correct\n>>> optimization.\n>>>\n>>> * When such a path is not excluded, on the ther hand, and when we\n>>> are not showing ignored directories, treat_file() just returns\n>>> the value of exclude_file, which is initialized to false and is\n>>> not changed in the function. This causes treat_one_path() to\n>>> return path_handled. However, the new code returns path_ignored\n>>> in this case.\n>>>\n>>> What guarantees that this change is regression free?\n>>\n>> If you consider read_directory_recursive alone, there is a regression.\n>> The return value of r_d_r depends on path_handled/path_ignored. With\n>> this patch, the return value will be different.\n> \n> That is exactly what was missing from the proposed log message, and\n> made me ask \"Do all the callers that reach this function in their\n> callgraph, when they get path_ignored for a path in the index,\n> behave as if the difference between path_ignored and path_handled\n> does not matter?\" Your answer seems to be\n> \n> - r-d-r returns 'how many paths in this directory match the\n> criteria we are looking for', unless check_only is true. Now in\n> some cases we return path_ignored not path_handled, so we may\n> return a number that is greater than we used to return.\n> \n> - treat_directory, the only user of that return value, cares if\n> r-d-r returned 0 or non-zero; and\n> \n> - As long as we keep returning 0 from r-d-r in cases we used to\n> return 0 and non-zero in cases we used to return non-zero, exact\n> number does not matter. Overall we get the same result.\n> \n> I think all of the above is true, but I have not convinced myself\n> that r-d-r with the new code never returns 0 when we used to return\n> non-zero.\n> \n\ntreat_directory calls read_directory_recursive in tow cases:\n\n1.) The directory is not in the index.\n\n---8<---\nswitch (directory_exists_in_index(dirname, len-1)) {\ncase index_nonexistent:\n\tif (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)\n\t\tbreak;\n}\n...\n---8<---\n\n\nThe directory is not in the index if there are no tracked files in the directory. I.e. cache_name_exists will always be false in this case, so the change won't affect the result of r_d_r.\n\n\n2.) The directory is in the index but is ignored.\n\n---8<---\nswitch (directory_exists_in_index(dirname, len-1)) {\ncase index_directory:\n\tif ((dir->flags & DIR_SHOW_OTHER_DIRECTORIES) && exclude)\n\t\tbreak;\n}\n\nif ((dir->flags & DIR_SHOW_IGNORED) && !exclude) {\n\t...\n}\nif (!(dir->flags & DIR_SHOW_IGNORED) &&\n !(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))\n\treturn show_directory;\nif (!read_directory_recursive(dir, dirname, len, 1, simplify))\n\treturn ignore_directory;\nreturn show_directory;\n---8<---\n\n\nWith exclude==true, only one r_d_r call is reachable, and only if either DIR_SHOW_IGNORED or DIR_HIDE_EMPTY_DIRECTORIES is set.\n\n2a) DIR_SHOW_IGNORED is set: the patch already checks !(dir->flags & DIR_SHOW_IGNORED), so the result of r_d_r is not affected.\n\n2b) DIR_HIDE_EMPTY_DIRECTORIES is set and DIR_SHOW_IGNORED is not set: the directory is already ignored, so all files in the directory should be ignored, too. It doesn't matter whether treat_one_path returns path_ignored because of the excluded() check or cache_name_exists().\n\n\nTherefore, I think the patch (v0) is regression-free.\n\n\n\nAs a side note, I'm quite confused why we would ever want to evaluate .gitignore patterns on tracked files at all, as gitignore(5) clearly states \"Files already tracked by git are not affected\". There is 'git-ls-files --cached --ignored', although this doesn't seem to process .gitignore files but expects exclude patterns on the command line...\n"},{"id":"210299","messageId":"512BDF38.5040400@gmail.com","threadId":"32908","inReplyTo":"1360999078-27196-1-git-send-email-pclouds@gmail.com","subject":"[PATCH/RFC] dir.c: Make git-status --ignored even more consistent","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2013-02-25T22:01:28Z","receivedAt":"2013-02-25T22:01:28Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"The new \"git-status --ignored\" handling introduced with 721ac4ed \"dir.c:\nMake git-status --ignored more consistent\" and a45fb697 \"status: always\nreport ignored tracked directories\" still has a few flaws in the\n\"--untracked-files=normal\" case:\n\n - It lists directories that match the exclude pattern, even if they are\n tracked, instead of the untracked files and directories that are\n _affected_ by the exclude pattern. This is inconsistent with the listing\n of untracked files and directories. Additionally, an entire (tracked)\n directory may be listed as ignored while contained files are listed as\n modified.\n\n - With an untracked directory between the ignored directory and files, the\n directory is dropped.\n\n - With a tracked directory between the ignored directory and files, both\n the directory and the individual files are listed as ignored.\n\nChange \"git-status --ignored --untracked-files=normal\" so that it no longer\nlists tracked directories. This is already in line with gitignore(5) and\napi-directory-listing.txt, so we don't need to update documentation.\n\nIn the git-status case, always use is_path_excluded() instead of\nis_excluded(). The latter doesn't check if one of the parent directories\nis excluded, and we need the full picture when recursing into ignored\ndirectories. As is_path_excluded() is even more complex, only do this for\nuntracked files and directories. Keep the original is_excluded() check in\nthe DIR_COLLECT_IGNORED case so that git-add is not affected.\n\nIn read_directory_recursive, pass along the check_only parameter when\nrecursing into sub directories, so that we don't accidentally call\ndir_add_name in the check_only case.\n\nSigned-off-by: Karsten Blees \n---\n\nSee also: https://github.com/kblees/git/commits/kb/git-status-ignored\n\nRevisiting dir.c, I noticed that the added complexity in .gitignore\nprocessing was just recently introduced by these two changes:\n- a45fb697 status: always report ignored tracked directories\n- 721ac4ed dir.c: Make git-status --ignored more consistent\n\nInstead of skipping gitignore checks under very special 'safe'\ncircumstances, this patch focuses on fixing pre-existing bugs\nfirst. The correctness of lazy gitignore checks is then obvious (I\nhope) by the very definition of gitignore(5):\n\"gitignore - Specifies intentionally *untracked* files to ignore\"\n[emphasis added].\n\nThere's still lots of room for improvement, e.g.:\n- prevent \"git-status --ignored\" from scanning everything twice\n- integrate struct path_exclude_check into struct dir_struct to\n save setup costs of is_path_excluded\n\nCheers,\nKarsten\n\n\n\n dir.c | 138 ++++++++++++++++++++-------------------------\n t/t7061-wtstatus-ignore.sh | 65 +++++++++++++++++++--\n wt-status.c | 2 +-\n 3 files changed, 123 insertions(+), 82 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 57394e4..1a5440f 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -884,6 +884,17 @@ int is_path_excluded(struct path_exclude_check *check,\n \treturn 0;\n }\n \n+static int check_path_excluded(struct dir_struct *dir,\n+\t\t const char *name, int namelen, int *dtype)\n+{\n+\tstruct path_exclude_check check;\n+\tint excluded;\n+\tpath_exclude_check_init(&check, dir);\n+\texcluded = is_path_excluded(&check, name, namelen, dtype);\n+\tpath_exclude_check_clear(&check);\n+\treturn excluded;\n+}\n+\n static struct dir_entry *dir_entry_new(const char *pathname, int len)\n {\n \tstruct dir_entry *ent;\n@@ -897,8 +908,7 @@ static struct dir_entry *dir_entry_new(const char *pathname, int len)\n \n static struct dir_entry *dir_add_name(struct dir_struct *dir, const char *pathname, int len)\n {\n-\tif (!(dir->flags & DIR_SHOW_IGNORED) &&\n-\t cache_name_exists(pathname, len, ignore_case))\n+\tif (cache_name_exists(pathname, len, ignore_case))\n \t\treturn NULL;\n \n \tALLOC_GROW(dir->entries, dir->nr+1, dir->alloc);\n@@ -1000,9 +1010,8 @@ static enum exist_status directory_exists_in_index(const char *dirname, int len)\n * traversal routine.\n *\n * Case 1: If we *already* have entries in the index under that\n- * directory name, we recurse into the directory to see all the files,\n- * unless the directory is excluded and we want to show ignored\n- * directories\n+ * directory name, we always recurse into the directory to see\n+ * all the files.\n *\n * Case 2: If we *already* have that directory name as a gitlink,\n * we always continue to see it as a gitlink, regardless of whether\n@@ -1016,9 +1025,9 @@ static enum exist_status directory_exists_in_index(const char *dirname, int len)\n * just a directory, unless \"hide_empty_directories\" is\n * also true and the directory is empty, in which case\n * we just ignore it entirely.\n- * if we are looking for ignored directories, look if it\n- * contains only ignored files to decide if it must be shown as\n- * ignored or not.\n+ * if we are looking for ignored directories, we also hide\n+ * directories with untracked files (i.e. that are already\n+ * listed in the untracked section).\n * (b) if it looks like a git directory, and we don't have\n * 'no_gitlinks' set we treat it as a gitlink, and show it\n * as a directory.\n@@ -1031,15 +1040,13 @@ enum directory_treatment {\n };\n \n static enum directory_treatment treat_directory(struct dir_struct *dir,\n-\tconst char *dirname, int len, int exclude,\n-\tconst struct path_simplify *simplify)\n+\tconst char *dirname, int len, const struct path_simplify *simplify)\n {\n+\tint dt = DT_DIR, exclude;\n+\n \t/* The \"len-1\" is to strip the final '/' */\n \tswitch (directory_exists_in_index(dirname, len-1)) {\n \tcase index_directory:\n-\t\tif ((dir->flags & DIR_SHOW_OTHER_DIRECTORIES) && exclude)\n-\t\t\tbreak;\n-\n \t\treturn recurse_into_directory;\n \n \tcase index_gitdir:\n@@ -1060,65 +1067,61 @@ static enum directory_treatment treat_directory(struct dir_struct *dir,\n \n \t/* This is the \"show_other_directories\" case */\n \n+\texclude = check_path_excluded(dir, dirname, len-1, &dt);\n+\n \t/*\n \t * We are looking for ignored files and our directory is not ignored,\n \t * check if it contains only ignored files\n \t */\n \tif ((dir->flags & DIR_SHOW_IGNORED) && !exclude) {\n-\t\tint ignored;\n-\t\tdir->flags &= ~DIR_SHOW_IGNORED;\n-\t\tdir->flags |= DIR_HIDE_EMPTY_DIRECTORIES;\n-\t\tignored = read_directory_recursive(dir, dirname, len, 1, simplify);\n-\t\tdir->flags &= ~DIR_HIDE_EMPTY_DIRECTORIES;\n-\t\tdir->flags |= DIR_SHOW_IGNORED;\n-\n-\t\treturn ignored ? ignore_directory : show_directory;\n+\t\tint untracked;\n+\t\tdir->flags &= ~(DIR_SHOW_IGNORED|DIR_SHOW_OTHER_DIRECTORIES);\n+\t\tuntracked = read_directory_recursive(dir, dirname, len, 1, simplify);\n+\t\tdir->flags |= (DIR_SHOW_IGNORED|DIR_SHOW_OTHER_DIRECTORIES);\n+\n+\t\t/*\n+\t\t * Don't list the directory as ignored if it is already listed\n+\t\t * as untracked.\n+\t\t */\n+\t\tif (untracked)\n+\t\t\treturn ignore_directory;\n \t}\n-\tif (!(dir->flags & DIR_SHOW_IGNORED) &&\n-\t !(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))\n+\n+\tif (!(dir->flags & DIR_SHOW_IGNORED) && exclude)\n+\t\treturn ignore_directory;\n+\n+\tif (!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))\n \t\treturn show_directory;\n \tif (!read_directory_recursive(dir, dirname, len, 1, simplify))\n \t\treturn ignore_directory;\n \treturn show_directory;\n }\n \n+enum path_treatment {\n+\tpath_ignored,\n+\tpath_handled,\n+\tpath_recurse\n+};\n+\n /*\n * Decide what to do when we find a file while traversing the\n- * filesystem. Mostly two cases:\n- *\n- * 1. We are looking for ignored files\n- * (a) File is ignored, include it\n- * (b) File is in ignored path, include it\n- * (c) File is not ignored, exclude it\n+ * filesystem.\n *\n- * 2. Other scenarios, include the file if not excluded\n+ * Case 1: The file is in the index: always ignore.\n *\n- * Return 1 for exclude, 0 for include.\n+ * Case 2: The file is not in the index: check if the file is excluded.\n */\n-static int treat_file(struct dir_struct *dir, struct strbuf *path, int exclude, int *dtype)\n+static enum path_treatment treat_file(struct dir_struct *dir,\n+\t\tstruct strbuf *path, const struct path_simplify *simplify,\n+\t\tint *dtype)\n {\n-\tstruct path_exclude_check check;\n-\tint exclude_file = 0;\n-\n-\tif (exclude)\n-\t\texclude_file = !(dir->flags & DIR_SHOW_IGNORED);\n-\telse if (dir->flags & DIR_SHOW_IGNORED) {\n-\t\t/* Always exclude indexed files */\n-\t\tstruct cache_entry *ce = index_name_exists(&the_index,\n-\t\t path->buf, path->len, ignore_case);\n-\n-\t\tif (ce)\n-\t\t\treturn 1;\n-\n-\t\tpath_exclude_check_init(&check, dir);\n-\n-\t\tif (!is_path_excluded(&check, path->buf, path->len, dtype))\n-\t\t\texclude_file = 1;\n-\n-\t\tpath_exclude_check_clear(&check);\n-\t}\n+\tif (cache_name_exists(path->buf, path->len, ignore_case))\n+\t\treturn path_ignored;\n \n-\treturn exclude_file;\n+\tif (check_path_excluded(dir, path->buf, path->len, dtype) !=\n+\t !!(dir->flags & DIR_SHOW_IGNORED))\n+\t\treturn path_ignored;\n+\treturn path_handled;\n }\n \n /*\n@@ -1233,29 +1236,16 @@ static int get_dtype(struct dirent *de, const char *path, int len)\n \treturn dtype;\n }\n \n-enum path_treatment {\n-\tpath_ignored,\n-\tpath_handled,\n-\tpath_recurse\n-};\n-\n static enum path_treatment treat_one_path(struct dir_struct *dir,\n \t\t\t\t\t struct strbuf *path,\n \t\t\t\t\t const struct path_simplify *simplify,\n \t\t\t\t\t int dtype, struct dirent *de)\n {\n-\tint exclude = is_excluded(dir, path->buf, &dtype);\n-\tif (exclude && (dir->flags & DIR_COLLECT_IGNORED)\n+\tif ((dir->flags & DIR_COLLECT_IGNORED)\n+\t && is_excluded(dir, path->buf, &dtype)\n \t && exclude_matches_pathspec(path->buf, path->len, simplify))\n \t\tdir_add_ignored(dir, path->buf, path->len);\n \n-\t/*\n-\t * Excluded? If we don't explicitly want to show\n-\t * ignored files, ignore it\n-\t */\n-\tif (exclude && !(dir->flags & DIR_SHOW_IGNORED))\n-\t\treturn path_ignored;\n-\n \tif (dtype == DT_UNKNOWN)\n \t\tdtype = get_dtype(de, path->buf, path->len);\n \n@@ -1265,7 +1255,7 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,\n \tcase DT_DIR:\n \t\tstrbuf_addch(path, '/');\n \n-\t\tswitch (treat_directory(dir, path->buf, path->len, exclude, simplify)) {\n+\t\tswitch (treat_directory(dir, path->buf, path->len, simplify)) {\n \t\tcase show_directory:\n \t\t\tbreak;\n \t\tcase recurse_into_directory:\n@@ -1276,12 +1266,7 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,\n \t\tbreak;\n \tcase DT_REG:\n \tcase DT_LNK:\n-\t\tswitch (treat_file(dir, path, exclude, &dtype)) {\n-\t\tcase 1:\n-\t\t\treturn path_ignored;\n-\t\tdefault:\n-\t\t\tbreak;\n-\t\t}\n+\t\treturn treat_file(dir, path, simplify, &dtype);\n \t}\n \treturn path_handled;\n }\n@@ -1334,8 +1319,7 @@ static int read_directory_recursive(struct dir_struct *dir,\n \t\tswitch (treat_path(dir, de, &path, baselen, simplify)) {\n \t\tcase path_recurse:\n \t\t\tcontents += read_directory_recursive(dir, path.buf,\n-\t\t\t\t\t\t\t path.len, 0,\n-\t\t\t\t\t\t\t simplify);\n+\t\t\t\t\tpath.len, check_only, simplify);\n \t\t\tcontinue;\n \t\tcase path_ignored:\n \t\t\tcontinue;\ndiff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh\nindex 0da1214..7a73448 100755\n--- a/t/t7061-wtstatus-ignore.sh\n+++ b/t/t7061-wtstatus-ignore.sh\n@@ -96,7 +96,7 @@ cat >expected <<\\EOF\n ?? expected\n EOF\n \n-test_expect_success 'status ignored tracked directory with --ignore' '\n+test_expect_success 'status tracked directory with --ignore' '\n \trm -rf untracked-ignored &&\n \tmkdir tracked &&\n \t: >tracked/committed &&\n@@ -113,7 +113,7 @@ cat >expected <<\\EOF\n ?? expected\n EOF\n \n-test_expect_success 'status ignored tracked directory with --ignore -u' '\n+test_expect_success 'status tracked directory with --ignore -u' '\n \tgit status --porcelain --ignored -u >actual &&\n \ttest_cmp expected actual\n '\n@@ -122,10 +122,10 @@ cat >expected <<\\EOF\n ?? .gitignore\n ?? actual\n ?? expected\n-!! tracked/\n+!! tracked/uncommitted\n EOF\n \n-test_expect_success 'status ignored tracked directory and uncommitted file with --ignore' '\n+test_expect_success 'status tracked directory and uncommitted file with --ignore' '\n \t: >tracked/uncommitted &&\n \tgit status --porcelain --ignored >actual &&\n \ttest_cmp expected actual\n@@ -138,6 +138,63 @@ cat >expected <<\\EOF\n !! tracked/uncommitted\n EOF\n \n+test_expect_success 'status tracked directory and uncommitted file with --ignore -u' '\n+\tgit status --porcelain --ignored -u >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+cat >expected <<\\EOF\n+?? .gitignore\n+?? actual\n+?? expected\n+!! tracked/untracked/\n+EOF\n+\n+test_expect_success 'status ignored untracked directory and uncommitted file with --ignore' '\n+\trm -rf tracked/uncommitted &&\n+\tmkdir tracked/untracked &&\n+\t: >tracked/untracked/uncommitted &&\n+\tgit status --porcelain --ignored >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+cat >expected <<\\EOF\n+?? .gitignore\n+?? actual\n+?? expected\n+!! tracked/untracked/uncommitted\n+EOF\n+\n+test_expect_success 'status ignored untracked directory and uncommitted file with --ignore -u' '\n+\tgit status --porcelain --ignored -u >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+cat >expected <<\\EOF\n+?? .gitignore\n+?? actual\n+?? expected\n+!! tracked/tracked/uncommitted\n+EOF\n+\n+test_expect_success 'status ignored tracked directory and uncommitted file with --ignore' '\n+\trm -rf tracked/untracked &&\n+\tmkdir tracked/tracked &&\n+\t: >tracked/tracked/committed &&\n+\t: >tracked/tracked/uncommitted &&\n+\tgit add -f tracked/tracked/committed &&\n+\tgit commit -m. &&\n+\tgit status --porcelain --ignored >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+cat >expected <<\\EOF\n+?? .gitignore\n+?? actual\n+?? expected\n+!! tracked/tracked/uncommitted\n+EOF\n+\n test_expect_success 'status ignored tracked directory and uncommitted file with --ignore -u' '\n \tgit status --porcelain --ignored -u >actual &&\n \ttest_cmp expected actual\ndiff --git a/wt-status.c b/wt-status.c\nindex ef405d0..79eb124 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -518,7 +518,7 @@ static void wt_status_collect_untracked(struct wt_status *s)\n \t\tdir.nr = 0;\n \t\tdir.flags = DIR_SHOW_IGNORED;\n \t\tif (s->show_untracked_files != SHOW_ALL_UNTRACKED_FILES)\n-\t\t\tdir.flags |= DIR_SHOW_OTHER_DIRECTORIES;\n+\t\t\tdir.flags |= DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n \t\tfill_directory(&dir, s->pathspec);\n \t\tfor (i = 0; i < dir.nr; i++) {\n \t\t\tstruct dir_entry *ent = dir.entries[i];\n-- \n1.8.1.2.7988.ge3e3ca2\n"}]}