{"thread":{"id":"52636","subject":"[PATCH] dir: restructure in a way to avoid passing around a struct dirent","startedAt":"2020-01-14T16:32:19Z","lastAt":"2020-01-17T16:54:14Z","messageCount":15,"participants":["Elijah Newren via GitGitGadget","Johannes Schindelin","Elijah Newren","Jeff King","Derrick Stolee via GitGitGadget","Jeff King via GitGitGadget","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"389748","messageId":"pull.692.git.git.1579019532809.gitgitgadget@gmail.com","threadId":"52636","inReplyTo":null,"subject":"[PATCH] dir: restructure in a way to avoid passing around a struct dirent","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-14T16:32:12Z","receivedAt":"2020-01-14T16:32:19Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Restructure the code slightly to avoid passing around a struct dirent\nanywhere, which also enables us to avoid trying to manufacture one.\n\nSigned-off-by: Elijah Newren \n---\n dir: restructure in a way to avoid passing around a struct dirent\n \n This is a follow up to en/fill-directory-fixes; a somewhat embarrassing\n one...\n \n When I was more than a bit burned out on dir.c in late December, I had\n looked at making another change to remove the need for a dirent but\n punted because it looked like \"way too much work\". Junio and Peff both\n later suggested the same cleanup, but I said it was too much work. Peff\n posted a patch which demonstrated that it was actually pretty simple[1],\n but I somehow read his email wrong and assumed he was commenting on my\n patch (if I had looked even slightly closer...). His patch was actually\n for before en/fill-directory-fixes was applied, so I have updated it\n slightly to apply after that series.\n \n I'm not sure if I'm supposed to add a Reviewed-by me or a Signed-off-by\n or both, but I read through it closely to avoid any hidden surprises. I\n probably could have saved Dscho some time last month if I would have\n just looked a little closer. Sorry about all that...\n \n (And yeah, we'll need Peff's Signed-off-by, since I'm leaving him as the\n author; I really only made a small tweak to update his patch.)\n \n CC: Jeff King peff@peff.net [peff@peff.net], Johannes Schindelin \n Johannes.Schindelin@gmx.de [Johannes.Schindelin@gmx.de]\n \n [1] \n https://lore.kernel.org/git/20191219222403.GA705525@coredump.intra.peff.net/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-692%2Fnewren%2Favoid-dirent-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-692/newren/avoid-dirent-v1\nPull-Request: https://github.com/git/git/pull/692\n\n dir.c | 73 +++++++++++++++++++++++++----------------------------------\n 1 file changed, 31 insertions(+), 42 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 7d255227b1..356d24bff8 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -41,7 +41,8 @@ struct cached_dir {\n \tint nr_files;\n \tint nr_dirs;\n \n-\tstruct dirent *de;\n+\tconst char *d_name;\n+\tint d_type;\n \tconst char *file;\n \tstruct untracked_cache_dir *ucd;\n };\n@@ -50,8 +51,8 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \tstruct index_state *istate, const char *path, int len,\n \tstruct untracked_cache_dir *untracked,\n \tint check_only, int stop_at_first_file, const struct pathspec *pathspec);\n-static int get_dtype(struct dirent *de, struct index_state *istate,\n-\t\t const char *path, int len);\n+static int resolve_dtype(int dtype, struct index_state *istate,\n+\t\t\t const char *path, int len);\n \n int count_slashes(const char *s)\n {\n@@ -1215,8 +1216,7 @@ static struct path_pattern *last_matching_pattern_from_list(const char *pathname\n \t\tint prefix = pattern->nowildcardlen;\n \n \t\tif (pattern->flags & PATTERN_FLAG_MUSTBEDIR) {\n-\t\t\tif (*dtype == DT_UNKNOWN)\n-\t\t\t\t*dtype = get_dtype(NULL, istate, pathname, pathlen);\n+\t\t\t*dtype = resolve_dtype(*dtype, istate, pathname, pathlen);\n \t\t\tif (*dtype != DT_DIR)\n \t\t\t\tcontinue;\n \t\t}\n@@ -1842,10 +1842,9 @@ static int get_index_dtype(struct index_state *istate,\n \treturn DT_UNKNOWN;\n }\n \n-static int get_dtype(struct dirent *de, struct index_state *istate,\n-\t\t const char *path, int len)\n+static int resolve_dtype(int dtype, struct index_state *istate,\n+\t\t\t const char *path, int len)\n {\n-\tint dtype = de ? DTYPE(de) : DT_UNKNOWN;\n \tstruct stat st;\n \n \tif (dtype != DT_UNKNOWN)\n@@ -1870,14 +1869,13 @@ 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 int baselen,\n \t\t\t\t\t const struct pathspec *pathspec,\n-\t\t\t\t\t int dtype, struct dirent *de)\n+\t\t\t\t\t int dtype)\n {\n \tint exclude;\n \tint has_path_in_index = !!index_file_exists(istate, path->buf, path->len, ignore_case);\n \tenum path_treatment path_treatment;\n \n-\tif (dtype == DT_UNKNOWN)\n-\t\tdtype = get_dtype(de, istate, path->buf, path->len);\n+\tdtype = resolve_dtype(dtype, istate, path->buf, path->len);\n \n \t/* Always exclude indexed files */\n \tif (dtype != DT_DIR && has_path_in_index)\n@@ -1985,21 +1983,18 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n \t\t\t\t int baselen,\n \t\t\t\t const struct pathspec *pathspec)\n {\n-\tint dtype;\n-\tstruct dirent *de = cdir->de;\n-\n-\tif (!de)\n+\tif (!cdir->d_name)\n \t\treturn treat_path_fast(dir, untracked, cdir, istate, path,\n \t\t\t\t baselen, pathspec);\n-\tif (is_dot_or_dotdot(de->d_name) || !fspathcmp(de->d_name, \".git\"))\n+\tif (is_dot_or_dotdot(cdir->d_name) || !fspathcmp(cdir->d_name, \".git\"))\n \t\treturn path_none;\n \tstrbuf_setlen(path, baselen);\n-\tstrbuf_addstr(path, de->d_name);\n+\tstrbuf_addstr(path, cdir->d_name);\n \tif (simplify_away(path->buf, path->len, pathspec))\n \t\treturn path_none;\n \n-\tdtype = DTYPE(de);\n-\treturn treat_one_path(dir, untracked, istate, path, baselen, pathspec, dtype, de);\n+\treturn treat_one_path(dir, untracked, istate, path, baselen, pathspec,\n+\t\t\t cdir->d_type);\n }\n \n static void add_untracked(struct untracked_cache_dir *dir, const char *name)\n@@ -2087,10 +2082,17 @@ static int open_cached_dir(struct cached_dir *cdir,\n \n static int read_cached_dir(struct cached_dir *cdir)\n {\n+\tstruct dirent *de;\n+\n \tif (cdir->fdir) {\n-\t\tcdir->de = readdir(cdir->fdir);\n-\t\tif (!cdir->de)\n+\t\tde = readdir(cdir->fdir);\n+\t\tif (!de) {\n+\t\t\tcdir->d_name = NULL;\n+\t\t\tcdir->d_type = DT_UNKNOWN;\n \t\t\treturn -1;\n+\t\t}\n+\t\tcdir->d_name = de->d_name;\n+\t\tcdir->d_type = DTYPE(de);\n \t\treturn 0;\n \t}\n \twhile (cdir->nr_dirs < cdir->untracked->dirs_nr) {\n@@ -2216,7 +2218,7 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \t\t/* recurse into subdir if instructed by treat_path */\n \t\tif ((state == path_recurse) ||\n \t\t\t((state == path_untracked) &&\n-\t\t\t (get_dtype(cdir.de, istate, path.buf, path.len) == DT_DIR) &&\n+\t\t\t (resolve_dtype(cdir.d_type, istate, path.buf, path.len) == DT_DIR) &&\n \t\t\t ((dir->flags & DIR_SHOW_IGNORED_TOO) ||\n \t\t\t (pathspec &&\n \t\t\t do_match_pathspec(istate, pathspec, path.buf, path.len,\n@@ -2314,10 +2316,10 @@ static int treat_leading_path(struct dir_struct *dir,\n \t */\n \n \tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf subdir = STRBUF_INIT;\n \tint prevlen, baselen;\n \tconst char *cp;\n \tstruct cached_dir cdir;\n-\tstruct dirent *de;\n \tenum path_treatment state = path_none;\n \n \t/*\n@@ -2342,22 +2344,8 @@ static int treat_leading_path(struct dir_struct *dir,\n \tif (!len)\n \t\treturn 1;\n \n-\t/*\n-\t * We need a manufactured dirent with sufficient space to store a\n-\t * leading directory component of path in its d_name. Here, we\n-\t * assume that the dirent's d_name is either declared as\n-\t * char d_name[BIG_ENOUGH]\n-\t * or that it is declared at the end of the struct as\n-\t * char d_name[]\n-\t * For either case, padding with len+1 bytes at the end will ensure\n-\t * sufficient storage space.\n-\t */\n-\tde = xcalloc(1, st_add3(sizeof(struct dirent), len, 1));\n \tmemset(&cdir, 0, sizeof(cdir));\n-\tcdir.de = de;\n-#if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)\n-\tde->d_type = DT_DIR;\n-#endif\n+\tcdir.d_type = DT_DIR;\n \tbaselen = 0;\n \tprevlen = 0;\n \twhile (1) {\n@@ -2374,12 +2362,13 @@ static int treat_leading_path(struct dir_struct *dir,\n \t\t\tbreak;\n \t\tstrbuf_reset(&sb);\n \t\tstrbuf_add(&sb, path, prevlen);\n-\t\tmemcpy(de->d_name, path+prevlen, baselen-prevlen);\n-\t\tde->d_name[baselen-prevlen] = '\\0';\n+\t\tstrbuf_reset(&subdir);\n+\t\tstrbuf_add(&subdir, path+prevlen, baselen-prevlen);\n+\t\tcdir.d_name = subdir.buf;\n \t\tstate = treat_path(dir, NULL, &cdir, istate, &sb, prevlen,\n \t\t\t\t pathspec);\n \t\tif (state == path_untracked &&\n-\t\t get_dtype(cdir.de, istate, sb.buf, sb.len) == DT_DIR &&\n+\t\t resolve_dtype(cdir.d_type, istate, sb.buf, sb.len) == DT_DIR &&\n \t\t (dir->flags & DIR_SHOW_IGNORED_TOO ||\n \t\t do_match_pathspec(istate, pathspec, sb.buf, sb.len,\n \t\t\t\t baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC)) {\n@@ -2399,7 +2388,7 @@ static int treat_leading_path(struct dir_struct *dir,\n \t\t\t\t\t &sb, baselen, pathspec,\n \t\t\t\t\t state);\n \n-\tfree(de);\n+\tstrbuf_release(&subdir);\n \tstrbuf_release(&sb);\n \treturn state == path_recurse;\n }\n\nbase-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783\n-- \ngitgitgadget\n"},{"id":"389778","messageId":"nycvar.QRO.7.76.6.2001142203450.46@tvgsbejvaqbjf.bet","threadId":"52636","inReplyTo":"pull.692.git.git.1579019532809.gitgitgadget@gmail.com","subject":"Re: [PATCH] dir: restructure in a way to avoid passing around a struct dirent","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-14T21:07:09Z","receivedAt":"2020-01-14T21:07:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\n\nOn Tue, 14 Jan 2020, Elijah Newren via GitGitGadget wrote:\n\n> Restructure the code slightly to avoid passing around a struct dirent\n> anywhere, which also enables us to avoid trying to manufacture one.\n\nPlease note that due a bug (which has been fixed in the meantime, see\nhttps://github.com/gitgitgadget/gitgitgadget/pull/188 for details), this\nGitGitGadget mail is missing the line\n\n\tFrom: Jeff King \n\nThe patch itself looks fine to me.\n\nThanks,\nDscho\n"},{"id":"389779","messageId":"CABPp-BEG-z4cQujWtvcfMPh8fDZeScbr=OLEW5FhXQKkFcjbWw@mail.gmail.com","threadId":"52636","inReplyTo":"nycvar.QRO.7.76.6.2001142203450.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] dir: restructure in a way to avoid passing around a struct dirent","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-01-14T21:13:10Z","receivedAt":"2020-01-14T21:13:24Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Dscho,\n\nOn Tue, Jan 14, 2020 at 1:07 PM Johannes Schindelin\n wrote:\n>\n> Hi,\n>\n>\n> On Tue, 14 Jan 2020, Elijah Newren via GitGitGadget wrote:\n>\n> > Restructure the code slightly to avoid passing around a struct dirent\n> > anywhere, which also enables us to avoid trying to manufacture one.\n>\n> Please note that due a bug (which has been fixed in the meantime, see\n> https://github.com/gitgitgadget/gitgitgadget/pull/188 for details), this\n> GitGitGadget mail is missing the line\n>\n> From: Jeff King \n\nThanks for the heads up. Once Peff provides his Signed-off-by, I'll\nadd it to the patch and resubmit which will hopefully fix both the\nfrom and the signed-off-by at the same time.\n\n> The patch itself looks fine to me.\n>\n> Thanks,\n> Dscho\n"},{"id":"389788","messageId":"20200114220357.GA3957260@coredump.intra.peff.net","threadId":"52636","inReplyTo":"pull.692.git.git.1579019532809.gitgitgadget@gmail.com","subject":"Re: [PATCH] dir: restructure in a way to avoid passing around a struct dirent","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-14T22:03:57Z","receivedAt":"2020-01-14T22:03:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 14, 2020 at 04:32:12PM +0000, Elijah Newren via GitGitGadget wrote:\n\n> When I was more than a bit burned out on dir.c in late December, I had\n> looked at making another change to remove the need for a dirent but\n> punted because it looked like \"way too much work\". Junio and Peff both\n> later suggested the same cleanup, but I said it was too much work. Peff\n> posted a patch which demonstrated that it was actually pretty simple[1],\n> but I somehow read his email wrong and assumed he was commenting on my\n> patch (if I had looked even slightly closer...). His patch was actually\n> for before en/fill-directory-fixes was applied, so I have updated it\n> slightly to apply after that series.\n\nYeah, I hadn't really grokked your patch, so I was hoping you'd build\nyour stuff on top. :)\n\n> I'm not sure if I'm supposed to add a Reviewed-by me or a Signed-off-by\n> or both, but I read through it closely to avoid any hidden surprises. I\n> probably could have saved Dscho some time last month if I would have\n> just looked a little closer. Sorry about all that...\n> \n> (And yeah, we'll need Peff's Signed-off-by, since I'm leaving him as the\n> author; I really only made a small tweak to update his patch.)\n> \n> CC: Jeff King peff@peff.net [peff@peff.net], Johannes Schindelin \n> Johannes.Schindelin@gmx.de [Johannes.Schindelin@gmx.de]\n\nHmm, looks like you maybe meant for more headers (and cc's) to happen\nthan did. Anyway, I'm certainly fine with this direction, and you have\nmy:\n\n Signed-off-by: Jeff King \n\n-Peff\n"},{"id":"389802","messageId":"pull.692.v2.git.git.1579098078501.gitgitgadget@gmail.com","threadId":"52636","inReplyTo":"pull.692.git.git.1579019532809.gitgitgadget@gmail.com","subject":"[PATCH v2] dir: restructure in a way to avoid passing around a struct dirent","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-15T14:21:18Z","receivedAt":"2020-01-15T14:21:24Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Jeff King \n\nRestructure the code slightly to avoid passing around a struct dirent\nanywhere, which also enables us to avoid trying to manufacture one.\n\nSigned-off-by: Jeff King \nSigned-off-by: Elijah Newren \n---\n dir: restructure in a way to avoid passing around a struct dirent\n \n This is a follow up to en/fill-directory-fixes, and consists of a small\n tweak made by me to a patch proposed by Peff[1] (signoff from [2]).\n \n [1] \n https://lore.kernel.org/git/20191219222403.GA705525@coredump.intra.peff.net/\n [2] \n https://lore.kernel.org/git/20200114220357.GA3957260@coredump.intra.peff.net/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-692%2Fnewren%2Favoid-dirent-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-692/newren/avoid-dirent-v2\nPull-Request: https://github.com/git/git/pull/692\n\nRange-diff vs v1:\n\n 1: f63d591d3d ! 1: edb18304b2 dir: restructure in a way to avoid passing around a struct dirent\n @@ -5,6 +5,7 @@\n Restructure the code slightly to avoid passing around a struct dirent\n anywhere, which also enables us to avoid trying to manufacture one.\n \n + Signed-off-by: Jeff King \n Signed-off-by: Elijah Newren \n \n diff --git a/dir.c b/dir.c\n\n\n dir.c | 73 +++++++++++++++++++++++++----------------------------------\n 1 file changed, 31 insertions(+), 42 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 7d255227b1..356d24bff8 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -41,7 +41,8 @@ struct cached_dir {\n \tint nr_files;\n \tint nr_dirs;\n \n-\tstruct dirent *de;\n+\tconst char *d_name;\n+\tint d_type;\n \tconst char *file;\n \tstruct untracked_cache_dir *ucd;\n };\n@@ -50,8 +51,8 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \tstruct index_state *istate, const char *path, int len,\n \tstruct untracked_cache_dir *untracked,\n \tint check_only, int stop_at_first_file, const struct pathspec *pathspec);\n-static int get_dtype(struct dirent *de, struct index_state *istate,\n-\t\t const char *path, int len);\n+static int resolve_dtype(int dtype, struct index_state *istate,\n+\t\t\t const char *path, int len);\n \n int count_slashes(const char *s)\n {\n@@ -1215,8 +1216,7 @@ static struct path_pattern *last_matching_pattern_from_list(const char *pathname\n \t\tint prefix = pattern->nowildcardlen;\n \n \t\tif (pattern->flags & PATTERN_FLAG_MUSTBEDIR) {\n-\t\t\tif (*dtype == DT_UNKNOWN)\n-\t\t\t\t*dtype = get_dtype(NULL, istate, pathname, pathlen);\n+\t\t\t*dtype = resolve_dtype(*dtype, istate, pathname, pathlen);\n \t\t\tif (*dtype != DT_DIR)\n \t\t\t\tcontinue;\n \t\t}\n@@ -1842,10 +1842,9 @@ static int get_index_dtype(struct index_state *istate,\n \treturn DT_UNKNOWN;\n }\n \n-static int get_dtype(struct dirent *de, struct index_state *istate,\n-\t\t const char *path, int len)\n+static int resolve_dtype(int dtype, struct index_state *istate,\n+\t\t\t const char *path, int len)\n {\n-\tint dtype = de ? DTYPE(de) : DT_UNKNOWN;\n \tstruct stat st;\n \n \tif (dtype != DT_UNKNOWN)\n@@ -1870,14 +1869,13 @@ 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 int baselen,\n \t\t\t\t\t const struct pathspec *pathspec,\n-\t\t\t\t\t int dtype, struct dirent *de)\n+\t\t\t\t\t int dtype)\n {\n \tint exclude;\n \tint has_path_in_index = !!index_file_exists(istate, path->buf, path->len, ignore_case);\n \tenum path_treatment path_treatment;\n \n-\tif (dtype == DT_UNKNOWN)\n-\t\tdtype = get_dtype(de, istate, path->buf, path->len);\n+\tdtype = resolve_dtype(dtype, istate, path->buf, path->len);\n \n \t/* Always exclude indexed files */\n \tif (dtype != DT_DIR && has_path_in_index)\n@@ -1985,21 +1983,18 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n \t\t\t\t int baselen,\n \t\t\t\t const struct pathspec *pathspec)\n {\n-\tint dtype;\n-\tstruct dirent *de = cdir->de;\n-\n-\tif (!de)\n+\tif (!cdir->d_name)\n \t\treturn treat_path_fast(dir, untracked, cdir, istate, path,\n \t\t\t\t baselen, pathspec);\n-\tif (is_dot_or_dotdot(de->d_name) || !fspathcmp(de->d_name, \".git\"))\n+\tif (is_dot_or_dotdot(cdir->d_name) || !fspathcmp(cdir->d_name, \".git\"))\n \t\treturn path_none;\n \tstrbuf_setlen(path, baselen);\n-\tstrbuf_addstr(path, de->d_name);\n+\tstrbuf_addstr(path, cdir->d_name);\n \tif (simplify_away(path->buf, path->len, pathspec))\n \t\treturn path_none;\n \n-\tdtype = DTYPE(de);\n-\treturn treat_one_path(dir, untracked, istate, path, baselen, pathspec, dtype, de);\n+\treturn treat_one_path(dir, untracked, istate, path, baselen, pathspec,\n+\t\t\t cdir->d_type);\n }\n \n static void add_untracked(struct untracked_cache_dir *dir, const char *name)\n@@ -2087,10 +2082,17 @@ static int open_cached_dir(struct cached_dir *cdir,\n \n static int read_cached_dir(struct cached_dir *cdir)\n {\n+\tstruct dirent *de;\n+\n \tif (cdir->fdir) {\n-\t\tcdir->de = readdir(cdir->fdir);\n-\t\tif (!cdir->de)\n+\t\tde = readdir(cdir->fdir);\n+\t\tif (!de) {\n+\t\t\tcdir->d_name = NULL;\n+\t\t\tcdir->d_type = DT_UNKNOWN;\n \t\t\treturn -1;\n+\t\t}\n+\t\tcdir->d_name = de->d_name;\n+\t\tcdir->d_type = DTYPE(de);\n \t\treturn 0;\n \t}\n \twhile (cdir->nr_dirs < cdir->untracked->dirs_nr) {\n@@ -2216,7 +2218,7 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \t\t/* recurse into subdir if instructed by treat_path */\n \t\tif ((state == path_recurse) ||\n \t\t\t((state == path_untracked) &&\n-\t\t\t (get_dtype(cdir.de, istate, path.buf, path.len) == DT_DIR) &&\n+\t\t\t (resolve_dtype(cdir.d_type, istate, path.buf, path.len) == DT_DIR) &&\n \t\t\t ((dir->flags & DIR_SHOW_IGNORED_TOO) ||\n \t\t\t (pathspec &&\n \t\t\t do_match_pathspec(istate, pathspec, path.buf, path.len,\n@@ -2314,10 +2316,10 @@ static int treat_leading_path(struct dir_struct *dir,\n \t */\n \n \tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf subdir = STRBUF_INIT;\n \tint prevlen, baselen;\n \tconst char *cp;\n \tstruct cached_dir cdir;\n-\tstruct dirent *de;\n \tenum path_treatment state = path_none;\n \n \t/*\n@@ -2342,22 +2344,8 @@ static int treat_leading_path(struct dir_struct *dir,\n \tif (!len)\n \t\treturn 1;\n \n-\t/*\n-\t * We need a manufactured dirent with sufficient space to store a\n-\t * leading directory component of path in its d_name. Here, we\n-\t * assume that the dirent's d_name is either declared as\n-\t * char d_name[BIG_ENOUGH]\n-\t * or that it is declared at the end of the struct as\n-\t * char d_name[]\n-\t * For either case, padding with len+1 bytes at the end will ensure\n-\t * sufficient storage space.\n-\t */\n-\tde = xcalloc(1, st_add3(sizeof(struct dirent), len, 1));\n \tmemset(&cdir, 0, sizeof(cdir));\n-\tcdir.de = de;\n-#if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)\n-\tde->d_type = DT_DIR;\n-#endif\n+\tcdir.d_type = DT_DIR;\n \tbaselen = 0;\n \tprevlen = 0;\n \twhile (1) {\n@@ -2374,12 +2362,13 @@ static int treat_leading_path(struct dir_struct *dir,\n \t\t\tbreak;\n \t\tstrbuf_reset(&sb);\n \t\tstrbuf_add(&sb, path, prevlen);\n-\t\tmemcpy(de->d_name, path+prevlen, baselen-prevlen);\n-\t\tde->d_name[baselen-prevlen] = '\\0';\n+\t\tstrbuf_reset(&subdir);\n+\t\tstrbuf_add(&subdir, path+prevlen, baselen-prevlen);\n+\t\tcdir.d_name = subdir.buf;\n \t\tstate = treat_path(dir, NULL, &cdir, istate, &sb, prevlen,\n \t\t\t\t pathspec);\n \t\tif (state == path_untracked &&\n-\t\t get_dtype(cdir.de, istate, sb.buf, sb.len) == DT_DIR &&\n+\t\t resolve_dtype(cdir.d_type, istate, sb.buf, sb.len) == DT_DIR &&\n \t\t (dir->flags & DIR_SHOW_IGNORED_TOO ||\n \t\t do_match_pathspec(istate, pathspec, sb.buf, sb.len,\n \t\t\t\t baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC)) {\n@@ -2399,7 +2388,7 @@ static int treat_leading_path(struct dir_struct *dir,\n \t\t\t\t\t &sb, baselen, pathspec,\n \t\t\t\t\t state);\n \n-\tfree(de);\n+\tstrbuf_release(&subdir);\n \tstrbuf_release(&sb);\n \treturn state == path_recurse;\n }\n\nbase-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783\n-- \ngitgitgadget\n"},{"id":"389819","messageId":"20200115202146.GA4091171@coredump.intra.peff.net","threadId":"52636","inReplyTo":"pull.692.v2.git.git.1579098078501.gitgitgadget@gmail.com","subject":"[PATCH] dir: point treat_leading_path() warning to the right place","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-15T20:21:46Z","receivedAt":"2020-01-15T20:21:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 15, 2020 at 02:21:18PM +0000, Elijah Newren via GitGitGadget wrote:\n\n> From: Jeff King \n> \n> Restructure the code slightly to avoid passing around a struct dirent\n> anywhere, which also enables us to avoid trying to manufacture one.\n> \n> Signed-off-by: Jeff King \n> Signed-off-by: Elijah Newren \n\nThanks, this looks good.\n\nI wondered briefly whether this:\n\n> @@ -2374,12 +2362,13 @@ static int treat_leading_path(struct dir_struct *dir,\n> \t\t\tbreak;\n> \t\tstrbuf_reset(&sb);\n> \t\tstrbuf_add(&sb, path, prevlen);\n> -\t\tmemcpy(de->d_name, path+prevlen, baselen-prevlen);\n> -\t\tde->d_name[baselen-prevlen] = '\\0';\n> +\t\tstrbuf_reset(&subdir);\n> +\t\tstrbuf_add(&subdir, path+prevlen, baselen-prevlen);\n> +\t\tcdir.d_name = subdir.buf;\n\n...could avoid the extra strbuf by pointing into an existing string\n(since d_name is now a pointer, and not part of a dirent). But I think\nthe answer is \"no\", because in a path like \"a/b/c\", the loop may see\njust \"b\" (so offsetting into path isn't sufficient, because we also have\nto cut off the trailing part).\n\nI did notice one other small thing while looking at this code:\n\n-- >8 --\nSubject: [PATCH] dir: point treat_leading_path() warning to the right place\n\nCommit 777b420347 (dir: synchronize treat_leading_path() and\nread_directory_recursive(), 2019-12-19) tried to add two warning\ncomments in those functions, pointing at each other. But the one in\ntreat_leading_path() just points at itself.\n\nLet's fix that. Since the comment also redirects the reader for more\ndetails to \"the commit that added this warning\", and since we're now\nmodifying the warning (creating a new commit without those details),\nlet's mention the actual commit id.\n\nSigned-off-by: Jeff King \n---\n dir.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 7d255227b1..31e83d982a 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2308,9 +2308,9 @@ static int treat_leading_path(struct dir_struct *dir,\n \t * WARNING WARNING WARNING:\n \t *\n \t * Any updates to the traversal logic here may need corresponding\n-\t * updates in treat_leading_path(). See the commit message for the\n-\t * commit adding this warning as well as the commit preceding it\n-\t * for details.\n+\t * updates in read_directory_recursive(). See 777b420347 (dir:\n+\t * synchronize treat_leading_path() and read_directory_recursive(),\n+\t * 2019-12-19) and its parent commit for details.\n \t */\n \n \tstruct strbuf sb = STRBUF_INIT;\n-- \n2.25.0.639.gb9b1511416\n\n"},{"id":"389821","messageId":"CABPp-BFQDkpcm57dTwH+AiMsocxTro4ZuvhK2eAZzfHutWjq3Q@mail.gmail.com","threadId":"52636","inReplyTo":"20200115202146.GA4091171@coredump.intra.peff.net","subject":"Re: [PATCH] dir: point treat_leading_path() warning to the right place","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-01-15T20:28:48Z","receivedAt":"2020-01-15T20:29:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 15, 2020 at 12:21 PM Jeff King wrote:\n>\n> On Wed, Jan 15, 2020 at 02:21:18PM +0000, Elijah Newren via GitGitGadget wrote:\n>\n> > From: Jeff King \n> >\n> > Restructure the code slightly to avoid passing around a struct dirent\n> > anywhere, which also enables us to avoid trying to manufacture one.\n> >\n> > Signed-off-by: Jeff King \n> > Signed-off-by: Elijah Newren \n>\n> Thanks, this looks good.\n>\n> I wondered briefly whether this:\n>\n> > @@ -2374,12 +2362,13 @@ static int treat_leading_path(struct dir_struct *dir,\n> > break;\n> > strbuf_reset(&sb);\n> > strbuf_add(&sb, path, prevlen);\n> > - memcpy(de->d_name, path+prevlen, baselen-prevlen);\n> > - de->d_name[baselen-prevlen] = '\\0';\n> > + strbuf_reset(&subdir);\n> > + strbuf_add(&subdir, path+prevlen, baselen-prevlen);\n> > + cdir.d_name = subdir.buf;\n>\n> ...could avoid the extra strbuf by pointing into an existing string\n> (since d_name is now a pointer, and not part of a dirent). But I think\n> the answer is \"no\", because in a path like \"a/b/c\", the loop may see\n> just \"b\" (so offsetting into path isn't sufficient, because we also have\n> to cut off the trailing part).\n>\n> I did notice one other small thing while looking at this code:\n>\n> -- >8 --\n> Subject: [PATCH] dir: point treat_leading_path() warning to the right place\n>\n> Commit 777b420347 (dir: synchronize treat_leading_path() and\n> read_directory_recursive(), 2019-12-19) tried to add two warning\n> comments in those functions, pointing at each other. But the one in\n> treat_leading_path() just points at itself.\n>\n> Let's fix that. Since the comment also redirects the reader for more\n> details to \"the commit that added this warning\", and since we're now\n> modifying the warning (creating a new commit without those details),\n> let's mention the actual commit id.\n>\n> Signed-off-by: Jeff King \n> ---\n> dir.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/dir.c b/dir.c\n> index 7d255227b1..31e83d982a 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -2308,9 +2308,9 @@ static int treat_leading_path(struct dir_struct *dir,\n> * WARNING WARNING WARNING:\n> *\n> * Any updates to the traversal logic here may need corresponding\n> - * updates in treat_leading_path(). See the commit message for the\n> - * commit adding this warning as well as the commit preceding it\n> - * for details.\n> + * updates in read_directory_recursive(). See 777b420347 (dir:\n> + * synchronize treat_leading_path() and read_directory_recursive(),\n> + * 2019-12-19) and its parent commit for details.\n> */\n>\n> struct strbuf sb = STRBUF_INIT;\n> --\n> 2.25.0.639.gb9b1511416\n\nLooks good to me; thanks.\n"},{"id":"389918","messageId":"pull.692.v3.git.git.1579206117.gitgitgadget@gmail.com","threadId":"52636","inReplyTo":"pull.692.v2.git.git.1579098078501.gitgitgadget@gmail.com","subject":"[PATCH v3 0/4] dir: more fill_directory() fixes","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-16T20:21:52Z","receivedAt":"2020-01-16T20:22:02Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This is a follow up to en/fill-directory-fixes, and has grown from 1 patch\nin v1 to 4 patches in v2, 3 of which are submissions by others to the git\nlist that I've just combined into one series since they are all about\nfill_directory().\n\nThe first two patches may make sense to include in maint at some point,\nthough Stolee isn't that concerned about this regression[5][6].\n\n * The first patch is Kevin and Stolee's testcase showing a regression in\n \"git clean -f \"[1]\n * The second patch is a \"simple\" fix for that testcase with a pretty long\n (and slightly embarrassing though entertaining) backstory.\n * The third patch was from v1, and is a small tweak made by me to a patch\n proposed by Peff[2] (signoff from [3]) to avoid needing to create a\n dirent.\n * The fourth patch was a follow-up from Peff in response to V1 fixing a\n code comment[4].\n\n[1] \nhttps://lore.kernel.org/git/pull.526.git.1579119946211.gitgitgadget@gmail.com/\n[2] \nhttps://lore.kernel.org/git/20191219222403.GA705525@coredump.intra.peff.net/\n[3] \nhttps://lore.kernel.org/git/20200114220357.GA3957260@coredump.intra.peff.net/\n[4] \nhttps://lore.kernel.org/git/20200115202146.GA4091171@coredump.intra.peff.net/\n[5] \nhttps://lore.kernel.org/git/354fa43b-0e62-1ee5-a63f-59d9b2da7d3f@gmail.com/\n[6] \nhttps://lore.kernel.org/git/e008da66-defe-d2b0-410b-64b7754b9c6e@gmail.com/\n\nDerrick Stolee (1):\n clean: demonstrate a bug with pathspecs\n\nElijah Newren (1):\n dir: treat_leading_path() and read_directory_recursive(), round 2\n\nJeff King (2):\n dir: restructure in a way to avoid passing around a struct dirent\n dir: point treat_leading_path() warning to the right place\n\n dir.c | 83 ++++++++++++++++++++++--------------------------\n t/t7300-clean.sh | 9 ++++++\n 2 files changed, 47 insertions(+), 45 deletions(-)\n\n\nbase-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-692%2Fnewren%2Favoid-dirent-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-692/newren/avoid-dirent-v3\nPull-Request: https://github.com/git/git/pull/692\n\nRange-diff vs v2:\n\n -: ---------- > 1: 9efed585ef clean: demonstrate a bug with pathspecs\n -: ---------- > 2: ea95186774 dir: treat_leading_path() and read_directory_recursive(), round 2\n 1: edb18304b2 = 3: 5b8fa6e5a7 dir: restructure in a way to avoid passing around a struct dirent\n -: ---------- > 4: 9d346f8b6b dir: point treat_leading_path() warning to the right place\n\n-- \ngitgitgadget\n"},{"id":"389919","messageId":"ea95186774762a5e1cf3cb882cff45fc904e3bf6.1579206117.git.gitgitgadget@gmail.com","threadId":"52636","inReplyTo":"pull.692.v3.git.git.1579206117.gitgitgadget@gmail.com","subject":"[PATCH v3 2/4] dir: treat_leading_path() and read_directory_recursive(), round 2","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-16T20:21:54Z","receivedAt":"2020-01-16T20:22:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nI was going to title this \"dir: more synchronizing of\ntreat_leading_path() and read_directory_recursive()\", a nod to commit\n777b42034764 (\"dir: synchronize treat_leading_path() and\nread_directory_recursive()\", 2019-12-19), but the title was too long.\n\nAnyway, first the backstory...\n\nfill_directory() has always had a slightly error-prone interface: it\nreturns a subset of paths which *might* match the specified pathspec; it\nwas intended to prune away some paths which didn't match the specified\npathspec and keep at least all the ones that did match it. Given this\ninterface, callers were responsible to post-process the results and\ncheck whether each actually matched the pathspec.\n\nbuiltin/clean.c did this. It would first prune out duplicates (e.g. if\n\"dir\" was returned as well as all files under \"dir/\", then it would\nsimplify this to just \"dir\"), and after pruning duplicates it would\ncompare the remaining paths to the specified pathspec(s). This\npost-processing itself could run into problems, though, as noted in\ncommit 404ebceda01c (\"dir: also check directories for matching\npathspecs\", 2019-09-17):\n\n For the case of git-clean and a set of pathspecs of \"dir/file\" and\n \"more\", this caused a problem because we'd end up with dir entries\n for both of\n \"dir\"\n \"dir/file\"\n Then correct_untracked_entries() would try to helpfully prune\n duplicates for us by removing \"dir/file\" since it's under \"dir\",\n leaving us with\n \"dir\"\n Since the original pathspec only had \"dir/file\", the only entry left\n doesn't match and leaves nothing to be removed. (Note that if only\n one pathspec was specified, e.g. only \"dir/file\", then the\n common_prefix_len optimizations in fill_directory would cause us to\n bypass this problem, making it appear in simple tests that we could\n correctly remove manually specified pathspecs.)\n\nThat commit fixed the issue -- when multiple pathspecs were specified --\nby making sure fill_directory() wouldn't return both \"dir\" and\n\"dir/file\" outside the common_prefix_len optimization path. This is\nwhere it starts to get fun.\n\nIn commit b9670c1f5e6b (\"dir: fix checks on common prefix directory\",\n2019-12-19), we noticed that the common_prefix_len wasn't doing\nappropriate checks and letting all kinds of stuff through, resulting in\nrecursing into .git/ directories and other craziness. So it started\nlocking down and doing checks on pathnames within that code path. That\ncontinued with commit 777b42034764 (\"dir: synchronize\ntreat_leading_path() and read_directory_recursive()\", 2019-12-19), which\nnoted the following:\n\n Our optimization to avoid calling into read_directory_recursive()\n when all pathspecs have a common leading directory mean that we need\n to match the logic that read_directory_recursive() would use if we\n had just called it from the root. Since it does more than call\n treat_path() we need to copy that same logic.\n\n...and then it more forcefully addressed the issue with this wonderfully\nironic statement:\n\n Needing to duplicate logic like this means it is guaranteed someone\n will eventually need to make further changes and forget to update\n both locations. It is tempting to just nuke the leading_directory\n special casing to avoid such bugs and simplify the code, but\n unpack_trees' verify_clean_subdirectory() also calls\n read_directory() and does so with a non-empty leading path, so I'm\n hesitant to try to restructure further. Add obnoxious warnings to\n treat_leading_path() and read_directory_recursive() to try to warn\n people of such problems.\n\nYou would think that with such a strongly worded description, that its\nauthor would have actually ensured that the logic in\ntreat_leading_path() and read_directory_recursive() did actually match\nand that *everything* that was needed had at least been copied over at\nthe time that this paragraph was written. But you'd be wrong, I messed\nit up by missing part of the logic.\n\nCopy the missing bits to fix the new final test in t7300.\n\nSigned-off-by: Elijah Newren \n---\n dir.c | 4 ++++\n t/t7300-clean.sh | 2 +-\n 2 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex 7d255227b1..5d4c92d3aa 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2383,6 +2383,10 @@ static int treat_leading_path(struct dir_struct *dir,\n \t\t (dir->flags & DIR_SHOW_IGNORED_TOO ||\n \t\t do_match_pathspec(istate, pathspec, sb.buf, sb.len,\n \t\t\t\t baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC)) {\n+\t\t\tif (!match_pathspec(istate, pathspec, sb.buf, sb.len,\n+\t\t\t\t\t 0 /* prefix */, NULL,\n+\t\t\t\t\t 0 /* do NOT special case dirs */))\n+\t\t\t\tstate = path_none;\n \t\t\tadd_path_to_appropriate_result_list(dir, NULL, &cdir,\n \t\t\t\t\t\t\t istate,\n \t\t\t\t\t\t\t &sb, baselen,\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 782e125c89..cb5e34d94c 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -737,7 +737,7 @@ test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '\n \ttest_i18ngrep \"too long\" .git/err\n '\n \n-test_expect_failure 'clean untracked paths by pathspec' '\n+test_expect_success 'clean untracked paths by pathspec' '\n \tgit init untracked &&\n \tmkdir untracked/dir &&\n \techo >untracked/dir/file.txt &&\n-- \ngitgitgadget\n\n"},{"id":"389920","messageId":"9efed585ef7a1c09ed62998882236e7dbb58383a.1579206117.git.gitgitgadget@gmail.com","threadId":"52636","inReplyTo":"pull.692.v3.git.git.1579206117.gitgitgadget@gmail.com","subject":"[PATCH v3 1/4] clean: demonstrate a bug with pathspecs","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-16T20:21:53Z","receivedAt":"2020-01-16T20:22:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nb9670c1f5e (dir: fix checks on common prefix directory, 2019-12-19)\nmodified the way pathspecs are handled when handling a directory\nduring \"git clean -f \". While this improved the behavior for\nknown test breakages, it also regressed in how the clean command\nhandles cleaning a specified file.\n\nAdd a test case that demonstrates this behavior. This test passes\nbefore b9670c1f5e then fails after.\n\nHelped-by: Kevin Willford \nSigned-off-by: Derrick Stolee \nReviewed-by: Elijah Newren \n---\n t/t7300-clean.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 6e6d24c1c3..782e125c89 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -737,4 +737,13 @@ test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '\n \ttest_i18ngrep \"too long\" .git/err\n '\n \n+test_expect_failure 'clean untracked paths by pathspec' '\n+\tgit init untracked &&\n+\tmkdir untracked/dir &&\n+\techo >untracked/dir/file.txt &&\n+\tgit -C untracked clean -f dir/file.txt &&\n+\tls untracked/dir >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"389921","messageId":"9d346f8b6bf8ba59e5dcb5460048934030634c1d.1579206117.git.gitgitgadget@gmail.com","threadId":"52636","inReplyTo":"pull.692.v3.git.git.1579206117.gitgitgadget@gmail.com","subject":"[PATCH v3 4/4] dir: point treat_leading_path() warning to the right place","fromName":"Jeff King via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-16T20:21:56Z","receivedAt":"2020-01-16T20:22:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Jeff King \n\nCommit 777b420347 (dir: synchronize treat_leading_path() and\nread_directory_recursive(), 2019-12-19) tried to add two warning\ncomments in those functions, pointing at each other. But the one in\ntreat_leading_path() just points at itself.\n\nLet's fix that. Since the comment also redirects the reader for more\ndetails to \"the commit that added this warning\", and since we're now\nmodifying the warning (creating a new commit without those details),\nlet's mention the actual commit id.\n\nSigned-off-by: Jeff King \nReviewed-by: Elijah Newren \n---\n dir.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 8437030006..b460211e61 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2310,9 +2310,9 @@ static int treat_leading_path(struct dir_struct *dir,\n \t * WARNING WARNING WARNING:\n \t *\n \t * Any updates to the traversal logic here may need corresponding\n-\t * updates in treat_leading_path(). See the commit message for the\n-\t * commit adding this warning as well as the commit preceding it\n-\t * for details.\n+\t * updates in read_directory_recursive(). See 777b420347 (dir:\n+\t * synchronize treat_leading_path() and read_directory_recursive(),\n+\t * 2019-12-19) and its parent commit for details.\n \t */\n \n \tstruct strbuf sb = STRBUF_INIT;\n-- \ngitgitgadget\n"},{"id":"389922","messageId":"5b8fa6e5a74b7d27d815978572ed17940bfd1198.1579206117.git.gitgitgadget@gmail.com","threadId":"52636","inReplyTo":"pull.692.v3.git.git.1579206117.gitgitgadget@gmail.com","subject":"[PATCH v3 3/4] dir: restructure in a way to avoid passing around a struct dirent","fromName":"Jeff King via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-16T20:21:55Z","receivedAt":"2020-01-16T20:22:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Jeff King \n\nRestructure the code slightly to avoid passing around a struct dirent\nanywhere, which also enables us to avoid trying to manufacture one.\n\nSigned-off-by: Jeff King \nSigned-off-by: Elijah Newren \n---\n dir.c | 73 +++++++++++++++++++++++++----------------------------------\n 1 file changed, 31 insertions(+), 42 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 5d4c92d3aa..8437030006 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -41,7 +41,8 @@ struct cached_dir {\n \tint nr_files;\n \tint nr_dirs;\n \n-\tstruct dirent *de;\n+\tconst char *d_name;\n+\tint d_type;\n \tconst char *file;\n \tstruct untracked_cache_dir *ucd;\n };\n@@ -50,8 +51,8 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \tstruct index_state *istate, const char *path, int len,\n \tstruct untracked_cache_dir *untracked,\n \tint check_only, int stop_at_first_file, const struct pathspec *pathspec);\n-static int get_dtype(struct dirent *de, struct index_state *istate,\n-\t\t const char *path, int len);\n+static int resolve_dtype(int dtype, struct index_state *istate,\n+\t\t\t const char *path, int len);\n \n int count_slashes(const char *s)\n {\n@@ -1215,8 +1216,7 @@ static struct path_pattern *last_matching_pattern_from_list(const char *pathname\n \t\tint prefix = pattern->nowildcardlen;\n \n \t\tif (pattern->flags & PATTERN_FLAG_MUSTBEDIR) {\n-\t\t\tif (*dtype == DT_UNKNOWN)\n-\t\t\t\t*dtype = get_dtype(NULL, istate, pathname, pathlen);\n+\t\t\t*dtype = resolve_dtype(*dtype, istate, pathname, pathlen);\n \t\t\tif (*dtype != DT_DIR)\n \t\t\t\tcontinue;\n \t\t}\n@@ -1842,10 +1842,9 @@ static int get_index_dtype(struct index_state *istate,\n \treturn DT_UNKNOWN;\n }\n \n-static int get_dtype(struct dirent *de, struct index_state *istate,\n-\t\t const char *path, int len)\n+static int resolve_dtype(int dtype, struct index_state *istate,\n+\t\t\t const char *path, int len)\n {\n-\tint dtype = de ? DTYPE(de) : DT_UNKNOWN;\n \tstruct stat st;\n \n \tif (dtype != DT_UNKNOWN)\n@@ -1870,14 +1869,13 @@ 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 int baselen,\n \t\t\t\t\t const struct pathspec *pathspec,\n-\t\t\t\t\t int dtype, struct dirent *de)\n+\t\t\t\t\t int dtype)\n {\n \tint exclude;\n \tint has_path_in_index = !!index_file_exists(istate, path->buf, path->len, ignore_case);\n \tenum path_treatment path_treatment;\n \n-\tif (dtype == DT_UNKNOWN)\n-\t\tdtype = get_dtype(de, istate, path->buf, path->len);\n+\tdtype = resolve_dtype(dtype, istate, path->buf, path->len);\n \n \t/* Always exclude indexed files */\n \tif (dtype != DT_DIR && has_path_in_index)\n@@ -1985,21 +1983,18 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n \t\t\t\t int baselen,\n \t\t\t\t const struct pathspec *pathspec)\n {\n-\tint dtype;\n-\tstruct dirent *de = cdir->de;\n-\n-\tif (!de)\n+\tif (!cdir->d_name)\n \t\treturn treat_path_fast(dir, untracked, cdir, istate, path,\n \t\t\t\t baselen, pathspec);\n-\tif (is_dot_or_dotdot(de->d_name) || !fspathcmp(de->d_name, \".git\"))\n+\tif (is_dot_or_dotdot(cdir->d_name) || !fspathcmp(cdir->d_name, \".git\"))\n \t\treturn path_none;\n \tstrbuf_setlen(path, baselen);\n-\tstrbuf_addstr(path, de->d_name);\n+\tstrbuf_addstr(path, cdir->d_name);\n \tif (simplify_away(path->buf, path->len, pathspec))\n \t\treturn path_none;\n \n-\tdtype = DTYPE(de);\n-\treturn treat_one_path(dir, untracked, istate, path, baselen, pathspec, dtype, de);\n+\treturn treat_one_path(dir, untracked, istate, path, baselen, pathspec,\n+\t\t\t cdir->d_type);\n }\n \n static void add_untracked(struct untracked_cache_dir *dir, const char *name)\n@@ -2087,10 +2082,17 @@ static int open_cached_dir(struct cached_dir *cdir,\n \n static int read_cached_dir(struct cached_dir *cdir)\n {\n+\tstruct dirent *de;\n+\n \tif (cdir->fdir) {\n-\t\tcdir->de = readdir(cdir->fdir);\n-\t\tif (!cdir->de)\n+\t\tde = readdir(cdir->fdir);\n+\t\tif (!de) {\n+\t\t\tcdir->d_name = NULL;\n+\t\t\tcdir->d_type = DT_UNKNOWN;\n \t\t\treturn -1;\n+\t\t}\n+\t\tcdir->d_name = de->d_name;\n+\t\tcdir->d_type = DTYPE(de);\n \t\treturn 0;\n \t}\n \twhile (cdir->nr_dirs < cdir->untracked->dirs_nr) {\n@@ -2216,7 +2218,7 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \t\t/* recurse into subdir if instructed by treat_path */\n \t\tif ((state == path_recurse) ||\n \t\t\t((state == path_untracked) &&\n-\t\t\t (get_dtype(cdir.de, istate, path.buf, path.len) == DT_DIR) &&\n+\t\t\t (resolve_dtype(cdir.d_type, istate, path.buf, path.len) == DT_DIR) &&\n \t\t\t ((dir->flags & DIR_SHOW_IGNORED_TOO) ||\n \t\t\t (pathspec &&\n \t\t\t do_match_pathspec(istate, pathspec, path.buf, path.len,\n@@ -2314,10 +2316,10 @@ static int treat_leading_path(struct dir_struct *dir,\n \t */\n \n \tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf subdir = STRBUF_INIT;\n \tint prevlen, baselen;\n \tconst char *cp;\n \tstruct cached_dir cdir;\n-\tstruct dirent *de;\n \tenum path_treatment state = path_none;\n \n \t/*\n@@ -2342,22 +2344,8 @@ static int treat_leading_path(struct dir_struct *dir,\n \tif (!len)\n \t\treturn 1;\n \n-\t/*\n-\t * We need a manufactured dirent with sufficient space to store a\n-\t * leading directory component of path in its d_name. Here, we\n-\t * assume that the dirent's d_name is either declared as\n-\t * char d_name[BIG_ENOUGH]\n-\t * or that it is declared at the end of the struct as\n-\t * char d_name[]\n-\t * For either case, padding with len+1 bytes at the end will ensure\n-\t * sufficient storage space.\n-\t */\n-\tde = xcalloc(1, st_add3(sizeof(struct dirent), len, 1));\n \tmemset(&cdir, 0, sizeof(cdir));\n-\tcdir.de = de;\n-#if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)\n-\tde->d_type = DT_DIR;\n-#endif\n+\tcdir.d_type = DT_DIR;\n \tbaselen = 0;\n \tprevlen = 0;\n \twhile (1) {\n@@ -2374,12 +2362,13 @@ static int treat_leading_path(struct dir_struct *dir,\n \t\t\tbreak;\n \t\tstrbuf_reset(&sb);\n \t\tstrbuf_add(&sb, path, prevlen);\n-\t\tmemcpy(de->d_name, path+prevlen, baselen-prevlen);\n-\t\tde->d_name[baselen-prevlen] = '\\0';\n+\t\tstrbuf_reset(&subdir);\n+\t\tstrbuf_add(&subdir, path+prevlen, baselen-prevlen);\n+\t\tcdir.d_name = subdir.buf;\n \t\tstate = treat_path(dir, NULL, &cdir, istate, &sb, prevlen,\n \t\t\t\t pathspec);\n \t\tif (state == path_untracked &&\n-\t\t get_dtype(cdir.de, istate, sb.buf, sb.len) == DT_DIR &&\n+\t\t resolve_dtype(cdir.d_type, istate, sb.buf, sb.len) == DT_DIR &&\n \t\t (dir->flags & DIR_SHOW_IGNORED_TOO ||\n \t\t do_match_pathspec(istate, pathspec, sb.buf, sb.len,\n \t\t\t\t baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC)) {\n@@ -2403,7 +2392,7 @@ static int treat_leading_path(struct dir_struct *dir,\n \t\t\t\t\t &sb, baselen, pathspec,\n \t\t\t\t\t state);\n \n-\tfree(de);\n+\tstrbuf_release(&subdir);\n \tstrbuf_release(&sb);\n \treturn state == path_recurse;\n }\n-- \ngitgitgadget\n\n"},{"id":"389988","messageId":"37815f50-35df-ef02-4def-ad857fd55520@gmail.com","threadId":"52636","inReplyTo":"9efed585ef7a1c09ed62998882236e7dbb58383a.1579206117.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/4] clean: demonstrate a bug with pathspecs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-01-17T15:20:26Z","receivedAt":"2020-01-17T15:20:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/16/2020 3:21 PM, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> b9670c1f5e (dir: fix checks on common prefix directory, 2019-12-19)\n\nThanks for fixing my typo here.\n\n> modified the way pathspecs are handled when handling a directory\n> during \"git clean -f \". While this improved the behavior for\n> known test breakages, it also regressed in how the clean command\n> handles cleaning a specified file.\n> \n> Add a test case that demonstrates this behavior. This test passes\n> before b9670c1f5e then fails after.\n> \n> Helped-by: Kevin Willford \n> Signed-off-by: Derrick Stolee \n> Reviewed-by: Elijah Newren \n\nDo we need your sign-off here, too?\n\n-Stolee\n\n"},{"id":"389989","messageId":"bd3892c6-98fb-d0eb-2e0a-f7d6971dd4d3@gmail.com","threadId":"52636","inReplyTo":"ea95186774762a5e1cf3cb882cff45fc904e3bf6.1579206117.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/4] dir: treat_leading_path() and read_directory_recursive(), round 2","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-01-17T15:25:42Z","receivedAt":"2020-01-17T15:25:46Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/16/2020 3:21 PM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> I was going to title this \"dir: more synchronizing of\n> treat_leading_path() and read_directory_recursive()\", a nod to commit\n> 777b42034764 (\"dir: synchronize treat_leading_path() and\n> read_directory_recursive()\", 2019-12-19), but the title was too long.\n> \n> Anyway, first the backstory...\n\nThe backstory was helpful, thanks.\n\n> Copy the missing bits to fix the new final test in t7300.\n\nThanks for the quick turnaround!\n\n-Stolee\n\n"},{"id":"389998","messageId":"CABPp-BEodg2wJU5m-GJ+zTifnEyRJ7eoKx86wZzaOtRfD91_vA@mail.gmail.com","threadId":"52636","inReplyTo":"37815f50-35df-ef02-4def-ad857fd55520@gmail.com","subject":"Re: [PATCH v3 1/4] clean: demonstrate a bug with pathspecs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-01-17T16:53:59Z","receivedAt":"2020-01-17T16:54:14Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jan 17, 2020 at 7:20 AM Derrick Stolee wrote:\n>\n> On 1/16/2020 3:21 PM, Derrick Stolee via GitGitGadget wrote:\n> > From: Derrick Stolee \n> >\n> > b9670c1f5e (dir: fix checks on common prefix directory, 2019-12-19)\n>\n> Thanks for fixing my typo here.\n>\n> > modified the way pathspecs are handled when handling a directory\n> > during \"git clean -f \". While this improved the behavior for\n> > known test breakages, it also regressed in how the clean command\n> > handles cleaning a specified file.\n> >\n> > Add a test case that demonstrates this behavior. This test passes\n> > before b9670c1f5e then fails after.\n> >\n> > Helped-by: Kevin Willford \n> > Signed-off-by: Derrick Stolee \n> > Reviewed-by: Elijah Newren \n>\n> Do we need your sign-off here, too?\n\nI figured I didn't change the code, and the list already independently\nsaw your submission and sign-off, so I assumed that a \"Reviewed-by\"\nwas more appropriate. If I'm wrong or people would just prefer me to\nalso add a Signed-off-by, I'm happy to add one. And if it is, then it\nshould also be added to the last patch in the series from Peff.\n\nIf Junio wants it and it's easier for him to just edit-in for both of\nthese commits:\n Signed-off-by: Elijah Newren \n\nor I can send a re-roll. But I'll assume it's not necessary if I\ndon't hear otherwise.\n"}]}