Re: [PATCH v4 6/8] dir: fix checks on common prefix directory
- From
Jeff King <peff@peff.net>
- Date
- Dec 19, 2019, 22:24 UTC
- Message-ID
- <20191219222403.GA705525@coredump.intra.peff.net>
- In-Reply-To
- <CABPp-BHhAPjL9daPdp7UmVT+vLrcpo6SX0rKt5s1bH4Vz31wxw@mail.gmail.com>
On Thu, Dec 19, 2019 at 12:23:29PM -0800, Elijah Newren wrote:
Show 10 quoted lines
> > > while (len && path[len - 1] == '/') > > > len--; > > > if (!len) > > > return 1; > > > + > > > + de = xcalloc(1, sizeof(struct dirent)+len+1); > > > > That "+len+1" may deserve a comment? > > Good point, I'll add one and send a re-roll.
Please use st_add3() while you are at it.
I'd also usually suggest FLEX_ALLOC_MEM() for even more simplicity, but it looks like filling the string is handled separately (and done many times).
I have to wonder, though, if it wouldn't be simpler to move away from "struct dirent" here (and it looks like Junio suggested the same earlier in the thread). I don't know this code very well, but it looks like it could easily get by passing around a name pointer and a dtype through the cached_dir. The patch below seems like it's not too bad a cleanup, but possibly the names could be better.
--- dir.c | 48 ++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 23 deletions(-)
diff --git a/dir.c b/dir.c index 43e2f47f66..e1cba688f3 100644 --- a/dir.c +++ b/dir.c @@ -41,7 +41,8 @@ struct cached_dir { int nr_files; int nr_dirs; - struct dirent *de; + const char *d_name; + int d_type; const char *file; struct untracked_cache_dir *ucd; }; @@ -50,8 +51,8 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir, struct index_state *istate, const char *path, int len, struct untracked_cache_dir *untracked, int check_only, int stop_at_first_file, const struct pathspec *pathspec); -static int get_dtype(struct dirent *de, struct index_state *istate, - const char *path, int len); +static int resolve_dtype(int dtype, struct index_state *istate, + const char *path, int len); int count_slashes(const char *s) { @@ -1050,8 +1051,7 @@ static struct path_pattern *last_matching_pattern_from_list(const char *pathname int prefix = pattern->nowildcardlen; if (pattern->flags & PATTERN_FLAG_MUSTBEDIR) { - if (*dtype == DT_UNKNOWN) - *dtype = get_dtype(NULL, istate, pathname, pathlen); + *dtype = resolve_dtype(*dtype, istate, pathname, pathlen); if (*dtype != DT_DIR) continue; } @@ -1639,10 +1639,9 @@ static int get_index_dtype(struct index_state *istate, return DT_UNKNOWN; } -static int get_dtype(struct dirent *de, struct index_state *istate, - const char *path, int len) +static int resolve_dtype(int dtype, struct index_state *istate, + const char *path, int len) { - int dtype = de ? DTYPE(de) : DT_UNKNOWN; struct stat st; if (dtype != DT_UNKNOWN) @@ -1667,14 +1666,13 @@ static enum path_treatment treat_one_path(struct dir_struct *dir, struct strbuf *path, int baselen, const struct pathspec *pathspec, - int dtype, struct dirent *de) + int dtype) { int exclude; int has_path_in_index = !!index_file_exists(istate, path->buf, path->len, ignore_case); enum path_treatment path_treatment; - if (dtype == DT_UNKNOWN) - dtype = get_dtype(de, istate, path->buf, path->len); + dtype = resolve_dtype(dtype, istate, path->buf, path->len); /* Always exclude indexed files */ if (dtype != DT_DIR && has_path_in_index) @@ -1782,21 +1780,18 @@ static enum path_treatment treat_path(struct dir_struct *dir, int baselen, const struct pathspec *pathspec) { - int dtype; - struct dirent *de = cdir->de; - - if (!de) + if (!cdir->d_name) return treat_path_fast(dir, untracked, cdir, istate, path, baselen, pathspec); - if (is_dot_or_dotdot(de->d_name) || !fspathcmp(de->d_name, ".git")) + if (is_dot_or_dotdot(cdir->d_name) || !fspathcmp(cdir->d_name, ".git")) return path_none; strbuf_setlen(path, baselen); - strbuf_addstr(path, de->d_name); + strbuf_addstr(path, cdir->d_name); if (simplify_away(path->buf, path->len, pathspec)) return path_none; - dtype = DTYPE(de); - return treat_one_path(dir, untracked, istate, path, baselen, pathspec, dtype, de); + return treat_one_path(dir, untracked, istate, path, baselen, pathspec, + cdir->d_type); } static void add_untracked(struct untracked_cache_dir *dir, const char *name) @@ -1884,10 +1879,17 @@ static int open_cached_dir(struct cached_dir *cdir, static int read_cached_dir(struct cached_dir *cdir) { + struct dirent *de; + if (cdir->fdir) { - cdir->de = readdir(cdir->fdir); - if (!cdir->de) + de = readdir(cdir->fdir); + if (!de) { + cdir->d_name = NULL; + cdir->d_type = DT_UNKNOWN; return -1; + } + cdir->d_name = de->d_name; + cdir->d_type = DTYPE(de); return 0; } while (cdir->nr_dirs < cdir->untracked->dirs_nr) { @@ -1970,7 +1972,7 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir, /* recurse into subdir if instructed by treat_path */ if ((state == path_recurse) || ((state == path_untracked) && - (get_dtype(cdir.de, istate, path.buf, path.len) == DT_DIR) && + (resolve_dtype(cdir.d_type, istate, path.buf, path.len) == DT_DIR) && ((dir->flags & DIR_SHOW_IGNORED_TOO) || (pathspec && do_match_pathspec(istate, pathspec, path.buf, path.len, @@ -2103,7 +2105,7 @@ static int treat_leading_path(struct dir_struct *dir, if (simplify_away(sb.buf, sb.len, pathspec)) break; if (treat_one_path(dir, NULL, istate, &sb, baselen, pathspec, - DT_DIR, NULL) == path_none) + DT_DIR) == path_none) break; /* do not recurse into it */ if (len <= baselen) { rc = 1;