Skip to contentSkip to conversation

dir: restructure in a way to avoid passing around a struct dirent

PATCH Superseded Elijah Newren via GitGitGadget 5 participants 15 messages Jan 14, 2020 – Jan 17, 2020

lore
Share & tools
lore

Elijah Newren via GitGitGadget

author
#0 lore raw

PATCH 1/1 dir: restructure in a way to avoid passing around a struct dirent

Restructure the code slightly to avoid passing around a struct dirent anywhere, which also enables us to avoid trying to manufacture one.

Signed-off-by Elijah Newren <[email protected]>

---
dir: restructure in a way to avoid passing around a struct dirent

This is a follow up to en/fill-directory-fixes; a somewhat embarrassing one...

When I was more than a bit burned out on dir.c in late December, I had looked at making another change to remove the need for a dirent but punted because it looked like "way too much work". Junio and Peff both later suggested the same cleanup, but I said it was too much work. Peff posted a patch which demonstrated that it was actually pretty simple[1], but I somehow read his email wrong and assumed he was commenting on my patch (if I had looked even slightly closer...). His patch was actually for before en/fill-directory-fixes was applied, so I have updated it slightly to apply after that series.

I'm not sure if I'm supposed to add a Reviewed-by me or a Signed-off-by or both, but I read through it closely to avoid any hidden surprises. I probably could have saved Dscho some time last month if I would have just looked a little closer. Sorry about all that...

(And yeah, we'll need Peff's Signed-off-by, since I'm leaving him as the author; I really only made a small tweak to update his patch.)

CC: Jeff King [email protected] [[email protected]], Johannes Schindelin [email protected] [[email protected]]

[1]
https://lore.kernel.org/git/[email protected]/

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-692%2Fnewren%2Favoid-dirent-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-692/newren/avoid-dirent-v1
Pull-Request: https://github.com/git/git/pull/692

 dir.c | 73 +++++++++++++++++++++++++----------------------------------
 1 file changed, 31 insertions(+), 42 deletions(-)
dir.c +31 -42
index 7d255227b1..356d24bff8 100644
@@ -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)
 {

@@ -1215,8 +1216,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;
 		}

@@ -1842,10 +1842,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)

@@ -1870,14 +1869,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)

@@ -1985,21 +1983,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)

@@ -2087,10 +2082,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) {

@@ -2216,7 +2218,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,

@@ -2314,10 +2316,10 @@ static int treat_leading_path(struct dir_struct *dir,
 	 */
 
 	struct strbuf sb = STRBUF_INIT;
+	struct strbuf subdir = STRBUF_INIT;
 	int prevlen, baselen;
 	const char *cp;
 	struct cached_dir cdir;
-	struct dirent *de;
 	enum path_treatment state = path_none;
 
 	/*

@@ -2342,22 +2344,8 @@ static int treat_leading_path(struct dir_struct *dir,
 	if (!len)
 		return 1;
 
-	/*
-	 * We need a manufactured dirent with sufficient space to store a
-	 * leading directory component of path in its d_name.  Here, we
-	 * assume that the dirent's d_name is either declared as
-	 *    char d_name[BIG_ENOUGH]
-	 * or that it is declared at the end of the struct as
-	 *    char d_name[]
-	 * For either case, padding with len+1 bytes at the end will ensure
-	 * sufficient storage space.
-	 */
-	de = xcalloc(1, st_add3(sizeof(struct dirent), len, 1));
 	memset(&cdir, 0, sizeof(cdir));
-	cdir.de = de;
-#if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
-	de->d_type = DT_DIR;
-#endif
+	cdir.d_type = DT_DIR;
 	baselen = 0;
 	prevlen = 0;
 	while (1) {

@@ -2374,12 +2362,13 @@ static int treat_leading_path(struct dir_struct *dir,
 			break;
 		strbuf_reset(&sb);
 		strbuf_add(&sb, path, prevlen);
-		memcpy(de->d_name, path+prevlen, baselen-prevlen);
-		de->d_name[baselen-prevlen] = '\0';
+		strbuf_reset(&subdir);
+		strbuf_add(&subdir, path+prevlen, baselen-prevlen);
+		cdir.d_name = subdir.buf;
 		state = treat_path(dir, NULL, &cdir, istate, &sb, prevlen,
 				    pathspec);
 		if (state == path_untracked &&
-		    get_dtype(cdir.de, istate, sb.buf, sb.len) == DT_DIR &&
+		    resolve_dtype(cdir.d_type, istate, sb.buf, sb.len) == DT_DIR &&
 		    (dir->flags & DIR_SHOW_IGNORED_TOO ||
 		     do_match_pathspec(istate, pathspec, sb.buf, sb.len,
 				       baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC)) {

@@ -2399,7 +2388,7 @@ static int treat_leading_path(struct dir_struct *dir,
 					    &sb, baselen, pathspec,
 					    state);
 
-	free(de);
+	strbuf_release(&subdir);
 	strbuf_release(&sb);
 	return state == path_recurse;
 }

base-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783

Signature · 1 line
gitgitgadget
#1 lore raw

Hi,

Quoted 2 lines · Elijah Newren via GitGitGadget

Restructure the code slightly to avoid passing around a struct dirent anywhere, which also enables us to avoid trying to manufacture one.

Please note that due a bug (which has been fixed in the meantime, see
https://github.com/gitgitgadget/gitgitgadget/pull/188 for details), this GitGitGadget mail is missing the line

From: Jeff King <[email protected]>

The patch itself looks fine to me.

Thanks,
Dscho

#2 lore raw

Hi Dscho,

Quoted 6 lines · Johannes Schindelin

Hi,

On Tue, 14 Jan 2020, Elijah Newren via GitGitGadget wrote:

Quoted 8 lines
Quoted 2 lines

Restructure the code slightly to avoid passing around a struct dirent anywhere, which also enables us to avoid trying to manufacture one.

Please note that due a bug (which has been fixed in the meantime, see
https://github.com/gitgitgadget/gitgitgadget/pull/188 for details), this GitGitGadget mail is missing the line

From: Jeff King <[email protected]>

Thanks for the heads up. Once Peff provides his Signed-off-by, I'll add it to the patch and resubmit which will hopefully fix both the from and the signed-off-by at the same time.

Quoted 4 lines

The patch itself looks fine to me.

Thanks,
Dscho

#3 lore raw
In reply to Elijah Newren via GitGitGadget
Quoted 9 lines · Elijah Newren via GitGitGadget

When I was more than a bit burned out on dir.c in late December, I had looked at making another change to remove the need for a dirent but punted because it looked like "way too much work". Junio and Peff both later suggested the same cleanup, but I said it was too much work. Peff posted a patch which demonstrated that it was actually pretty simple[1], but I somehow read his email wrong and assumed he was commenting on my patch (if I had looked even slightly closer...). His patch was actually for before en/fill-directory-fixes was applied, so I have updated it slightly to apply after that series.

Yeah, I hadn't really grokked your patch, so I was hoping you'd build your stuff on top. :)

Quoted 10 lines

I'm not sure if I'm supposed to add a Reviewed-by me or a Signed-off-by or both, but I read through it closely to avoid any hidden surprises. I probably could have saved Dscho some time last month if I would have just looked a little closer. Sorry about all that...

(And yeah, we'll need Peff's Signed-off-by, since I'm leaving him as the author; I really only made a small tweak to update his patch.)

CC: Jeff King [email protected] [[email protected]], Johannes Schindelin [email protected] [[email protected]]

Hmm, looks like you maybe meant for more headers (and cc's) to happen than did. Anyway, I'm certainly fine with this direction, and you have my:

Signed-off-by Jeff King <[email protected]>

-Peff

author
#4 lore raw

PATCH v2 1/1 dir: restructure in a way to avoid passing around a struct dirent

In reply to Elijah Newren via GitGitGadget

From: Jeff King <[email protected]>

Restructure the code slightly to avoid passing around a struct dirent anywhere, which also enables us to avoid trying to manufacture one.

Signed-off-by Jeff King <[email protected]>
Signed-off-by Elijah Newren <[email protected]>

---

    dir: restructure in a way to avoid passing around a struct dirent
    
    This is a follow up to en/fill-directory-fixes, and consists of a small
    tweak made by me to a patch proposed by Peff[1] (signoff from [2]).
    
    [1] 
    https://lore.kernel.org/git/[email protected]/
    [2] 
    https://lore.kernel.org/git/[email protected]/

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-692%2Fnewren%2Favoid-dirent-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-692/newren/avoid-dirent-v2
Pull-Request: https://github.com/git/git/pull/692

Range-diff vs v1:

1: f63d591d3d ! 1: edb18304b2 dir: restructure in a way to avoid passing around a struct dirent
@@ -5,6 +5,7 @@
Restructure the code slightly to avoid passing around a struct dirent anywhere, which also enables us to avoid trying to manufacture one.

+ Signed-off-by: Jeff King <[email protected]>

Signed-off-by Elijah Newren <[email protected]>

diff --git a/dir.c b/dir.c

 dir.c | 73 +++++++++++++++++++++++++----------------------------------
 1 file changed, 31 insertions(+), 42 deletions(-)
dir.c +31 -42
index 7d255227b1..356d24bff8 100644
@@ -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)
 {

@@ -1215,8 +1216,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;
 		}

@@ -1842,10 +1842,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)

@@ -1870,14 +1869,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)

@@ -1985,21 +1983,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)

@@ -2087,10 +2082,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) {

@@ -2216,7 +2218,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,

@@ -2314,10 +2316,10 @@ static int treat_leading_path(struct dir_struct *dir,
 	 */
 
 	struct strbuf sb = STRBUF_INIT;
+	struct strbuf subdir = STRBUF_INIT;
 	int prevlen, baselen;
 	const char *cp;
 	struct cached_dir cdir;
-	struct dirent *de;
 	enum path_treatment state = path_none;
 
 	/*

@@ -2342,22 +2344,8 @@ static int treat_leading_path(struct dir_struct *dir,
 	if (!len)
 		return 1;
 
-	/*
-	 * We need a manufactured dirent with sufficient space to store a
-	 * leading directory component of path in its d_name.  Here, we
-	 * assume that the dirent's d_name is either declared as
-	 *    char d_name[BIG_ENOUGH]
-	 * or that it is declared at the end of the struct as
-	 *    char d_name[]
-	 * For either case, padding with len+1 bytes at the end will ensure
-	 * sufficient storage space.
-	 */
-	de = xcalloc(1, st_add3(sizeof(struct dirent), len, 1));
 	memset(&cdir, 0, sizeof(cdir));
-	cdir.de = de;
-#if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
-	de->d_type = DT_DIR;
-#endif
+	cdir.d_type = DT_DIR;
 	baselen = 0;
 	prevlen = 0;
 	while (1) {

@@ -2374,12 +2362,13 @@ static int treat_leading_path(struct dir_struct *dir,
 			break;
 		strbuf_reset(&sb);
 		strbuf_add(&sb, path, prevlen);
-		memcpy(de->d_name, path+prevlen, baselen-prevlen);
-		de->d_name[baselen-prevlen] = '\0';
+		strbuf_reset(&subdir);
+		strbuf_add(&subdir, path+prevlen, baselen-prevlen);
+		cdir.d_name = subdir.buf;
 		state = treat_path(dir, NULL, &cdir, istate, &sb, prevlen,
 				    pathspec);
 		if (state == path_untracked &&
-		    get_dtype(cdir.de, istate, sb.buf, sb.len) == DT_DIR &&
+		    resolve_dtype(cdir.d_type, istate, sb.buf, sb.len) == DT_DIR &&
 		    (dir->flags & DIR_SHOW_IGNORED_TOO ||
 		     do_match_pathspec(istate, pathspec, sb.buf, sb.len,
 				       baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC)) {

@@ -2399,7 +2388,7 @@ static int treat_leading_path(struct dir_struct *dir,
 					    &sb, baselen, pathspec,
 					    state);
 
-	free(de);
+	strbuf_release(&subdir);
 	strbuf_release(&sb);
 	return state == path_recurse;
 }

base-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783

Signature · 1 line
gitgitgadget
#5 lore raw

PATCH 1/1 dir: point treat_leading_path() warning to the right place

Quoted 7 lines · Elijah Newren via GitGitGadget

From: Jeff King <[email protected]>

Restructure the code slightly to avoid passing around a struct dirent anywhere, which also enables us to avoid trying to manufacture one.

Signed-off-by Jeff King <[email protected]>
Signed-off-by Elijah Newren <[email protected]>

Thanks, this looks good.

I wondered briefly whether this:

Review static int treat_leading_path(struct dir_struct *dir, · line 2367

Quoted 9 lines
(quoted diff) +3 -2
@@ -2374,12 +2362,13 @@ static int treat_leading_path(struct dir_struct *dir,
 			break;
 		strbuf_reset(&sb);
 		strbuf_add(&sb, path, prevlen);
-		memcpy(de->d_name, path+prevlen, baselen-prevlen);
-		de->d_name[baselen-prevlen] = '\0';
+		strbuf_reset(&subdir);
+		strbuf_add(&subdir, path+prevlen, baselen-prevlen);
+		cdir.d_name = subdir.buf;

...could avoid the extra strbuf by pointing into an existing string (since d_name is now a pointer, and not part of a dirent). But I think the answer is "no", because in a path like "a/b/c", the loop may see just "b" (so offsetting into path isn't sufficient, because we also have to cut off the trailing part).

I did notice one other small thing while looking at this code:

-- >8 --
Subject: [PATCH] dir: point treat_leading_path() warning to the right place

Commit 777b420347 (dir: synchronize treat_leading_path() and read_directory_recursive(), 2019-12-19) tried to add two warning comments in those functions, pointing at each other. But the one in treat_leading_path() just points at itself.

Let's fix that. Since the comment also redirects the reader for more details to "the commit that added this warning", and since we're now modifying the warning (creating a new commit without those details), let's mention the actual commit id.

Signed-off-by Jeff King <[email protected]>
---
 dir.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
dir.c +3 -3
index 7d255227b1..31e83d982a 100644
@@ -2308,9 +2308,9 @@ static int treat_leading_path(struct dir_struct *dir,
 	 * WARNING WARNING WARNING:
 	 *
 	 * Any updates to the traversal logic here may need corresponding
-	 * updates in treat_leading_path().  See the commit message for the
-	 * commit adding this warning as well as the commit preceding it
-	 * for details.
+	 * updates in read_directory_recursive().  See 777b420347 (dir:
+	 * synchronize treat_leading_path() and read_directory_recursive(),
+	 * 2019-12-19) and its parent commit for details.
 	 */
 
 	struct strbuf sb = STRBUF_INIT;
Signature · 1 line
2.25.0.639.gb9b1511416
#6 lore raw

Re: [PATCH] dir: point treat_leading_path() warning to the right place

Quoted 69 lines · Jeff King
Quoted 7 lines · Elijah Newren via GitGitGadget

From: Jeff King <[email protected]>

Restructure the code slightly to avoid passing around a struct dirent anywhere, which also enables us to avoid trying to manufacture one.

Signed-off-by Jeff King <[email protected]>
Signed-off-by Elijah Newren <[email protected]>

Thanks, this looks good.

I wondered briefly whether this:

Review static int treat_leading_path(struct dir_struct *dir, · line 2367

Quoted 9 lines
(quoted diff) +3 -2
@@ -2374,12 +2362,13 @@ static int treat_leading_path(struct dir_struct *dir,
                      break;
              strbuf_reset(&sb);
              strbuf_add(&sb, path, prevlen);
-             memcpy(de->d_name, path+prevlen, baselen-prevlen);
-             de->d_name[baselen-prevlen] = '\0';
+             strbuf_reset(&subdir);
+             strbuf_add(&subdir, path+prevlen, baselen-prevlen);
+             cdir.d_name = subdir.buf;

...could avoid the extra strbuf by pointing into an existing string (since d_name is now a pointer, and not part of a dirent). But I think the answer is "no", because in a path like "a/b/c", the loop may see just "b" (so offsetting into path isn't sufficient, because we also have to cut off the trailing part).

I did notice one other small thing while looking at this code:

-- >8 --
Subject: [PATCH] dir: point treat_leading_path() warning to the right place

Commit 777b420347 (dir: synchronize treat_leading_path() and read_directory_recursive(), 2019-12-19) tried to add two warning comments in those functions, pointing at each other. But the one in treat_leading_path() just points at itself.

Let's fix that. Since the comment also redirects the reader for more details to "the commit that added this warning", and since we're now modifying the warning (creating a new commit without those details), let's mention the actual commit id.

Signed-off-by Jeff King <[email protected]>
---
 dir.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
dir.c +3 -4
index 7d255227b1..31e83d982a 100644
@@ -2308,9 +2308,9 @@ static int treat_leading_path(struct dir_struct *dir,
         * WARNING WARNING WARNING:
         *
         * Any updates to the traversal logic here may need corresponding
-        * updates in treat_leading_path().  See the commit message for the
-        * commit adding this warning as well as the commit preceding it
-        * for details.
+        * updates in read_directory_recursive().  See 777b420347 (dir:
+        * synchronize treat_leading_path() and read_directory_recursive(),
+        * 2019-12-19) and its parent commit for details.
         */
 
        struct strbuf sb = STRBUF_INIT;
--
Quoted 1 line

2.25.0.639.gb9b1511416

Looks good to me; thanks.

author
#7 lore raw

v3 cover dir: more fill_directory() fixes

In reply to Elijah Newren via GitGitGadget

This is a follow up to en/fill-directory-fixes, and has grown from 1 patch in v1 to 4 patches in v2, 3 of which are submissions by others to the git list that I've just combined into one series since they are all about fill_directory().

The first two patches may make sense to include in maint at some point, though Stolee isn't that concerned about this regression[5][6].

* The first patch is Kevin and Stolee's testcase showing a regression in "git clean -f "[1]
* The second patch is a "simple" fix for that testcase with a pretty long (and slightly embarrassing though entertaining) backstory.
* The third patch was from v1, and is a small tweak made by me to a patch proposed by Peff[2] (signoff from [3]) to avoid needing to create a dirent.
* The fourth patch was a follow-up from Peff in response to V1 fixing a code comment[4].

[1]
https://lore.kernel.org/git/[email protected]/
[2]
https://lore.kernel.org/git/[email protected]/
[3]
https://lore.kernel.org/git/[email protected]/
[4]
https://lore.kernel.org/git/[email protected]/
[5]
https://lore.kernel.org/git/[email protected]/
[6]
https://lore.kernel.org/git/[email protected]/

Derrick Stolee (1):
clean: demonstrate a bug with pathspecs

Elijah Newren (1):
dir: treat_leading_path() and read_directory_recursive(), round 2

Jeff King (2):
dir: restructure in a way to avoid passing around a struct dirent
dir: point treat_leading_path() warning to the right place

 dir.c            | 83 ++++++++++++++++++++++--------------------------
 t/t7300-clean.sh |  9 ++++++
 2 files changed, 47 insertions(+), 45 deletions(-)

base-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-692%2Fnewren%2Favoid-dirent-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-692/newren/avoid-dirent-v3
Pull-Request: https://github.com/git/git/pull/692

Range-diff vs v2:

 -:  ---------- > 1:  9efed585ef clean: demonstrate a bug with pathspecs
 -:  ---------- > 2:  ea95186774 dir: treat_leading_path() and read_directory_recursive(), round 2
 1:  edb18304b2 = 3:  5b8fa6e5a7 dir: restructure in a way to avoid passing around a struct dirent
 -:  ---------- > 4:  9d346f8b6b dir: point treat_leading_path() warning to the right place
Signature · 1 line
gitgitgadget
author
#8 lore raw

PATCH v3 1/4 clean: demonstrate a bug with pathspecs

From: Derrick Stolee <[email protected]>

b9670c1f5e (dir: fix checks on common prefix directory, 2019-12-19) modified the way pathspecs are handled when handling a directory during "git clean -f <path>". While this improved the behavior for known test breakages, it also regressed in how the clean command handles cleaning a specified file.

Add a test case that demonstrates this behavior. This test passes before b9670c1f5e then fails after.

Helped-by Kevin Willford <[email protected]>
Signed-off-by Derrick Stolee <[email protected]>
Reviewed-by Elijah Newren <[email protected]>
---
 t/t7300-clean.sh | 9 +++++++++
 1 file changed, 9 insertions(+)
t/t7300-clean.sh +9 -0
index 6e6d24c1c3..782e125c89 100755
@@ -737,4 +737,13 @@ test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '
 	test_i18ngrep "too long" .git/err
 '
 
+test_expect_failure 'clean untracked paths by pathspec' '
+	git init untracked &&
+	mkdir untracked/dir &&
+	echo >untracked/dir/file.txt &&
+	git -C untracked clean -f dir/file.txt &&
+	ls untracked/dir >actual &&
+	test_must_be_empty actual
+'
+
 test_done
Signature · 1 line
gitgitgadget
#9 lore raw

Re: [PATCH v3 1/4] clean: demonstrate a bug with pathspecs

Quoted 3 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

b9670c1f5e (dir: fix checks on common prefix directory, 2019-12-19)

Thanks for fixing my typo here.

Quoted 11 lines

modified the way pathspecs are handled when handling a directory during "git clean -f <path>". While this improved the behavior for known test breakages, it also regressed in how the clean command handles cleaning a specified file.

Add a test case that demonstrates this behavior. This test passes before b9670c1f5e then fails after.

Helped-by Kevin Willford <[email protected]>
Signed-off-by Derrick Stolee <[email protected]>
Reviewed-by Elijah Newren <[email protected]>

Do we need your sign-off here, too?

-Stolee

#10 lore raw

Re: [PATCH v3 1/4] clean: demonstrate a bug with pathspecs

Quoted 13 lines · Derrick Stolee
Quoted 3 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

b9670c1f5e (dir: fix checks on common prefix directory, 2019-12-19)

Thanks for fixing my typo here.

Quoted 5 lines

modified the way pathspecs are handled when handling a directory during "git clean -f <path>". While this improved the behavior for known test breakages, it also regressed in how the clean command handles cleaning a specified file.

Quoted 8 lines
Quoted 6 lines

Add a test case that demonstrates this behavior. This test passes before b9670c1f5e then fails after.

Helped-by Kevin Willford <[email protected]>
Signed-off-by Derrick Stolee <[email protected]>
Reviewed-by Elijah Newren <[email protected]>

Do we need your sign-off here, too?

I figured I didn't change the code, and the list already independently saw your submission and sign-off, so I assumed that a "Reviewed-by" was more appropriate. If I'm wrong or people would just prefer me to also add a Signed-off-by, I'm happy to add one. And if it is, then it should also be added to the last patch in the series from Peff.

If Junio wants it and it's easier for him to just edit-in for both of these commits:

Signed-off-by Elijah Newren <[email protected]>

or I can send a re-roll. But I'll assume it's not necessary if I don't hear otherwise.

author
#11 lore raw

PATCH v3 2/4 dir: treat_leading_path() and read_directory_recursive(), round 2

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

I was going to title this "dir: more synchronizing of treat_leading_path() and read_directory_recursive()", a nod to commit 777b42034764 ("dir: synchronize treat_leading_path() and read_directory_recursive()", 2019-12-19), but the title was too long.

Anyway, first the backstory...

fill_directory() has always had a slightly error-prone interface: it returns a subset of paths which *might* match the specified pathspec; it was intended to prune away some paths which didn't match the specified pathspec and keep at least all the ones that did match it. Given this interface, callers were responsible to post-process the results and check whether each actually matched the pathspec.

builtin/clean.c did this. It would first prune out duplicates (e.g. if "dir" was returned as well as all files under "dir/", then it would simplify this to just "dir"), and after pruning duplicates it would compare the remaining paths to the specified pathspec(s). This post-processing itself could run into problems, though, as noted in commit 404ebceda01c ("dir: also check directories for matching pathspecs", 2019-09-17):

For the case of git-clean and a set of pathspecs of "dir/file" and "more", this caused a problem because we'd end up with dir entries for both of
"dir"
"dir/file"
Then correct_untracked_entries() would try to helpfully prune duplicates for us by removing "dir/file" since it's under "dir", leaving us with
"dir"
Since the original pathspec only had "dir/file", the only entry left doesn't match and leaves nothing to be removed. (Note that if only one pathspec was specified, e.g. only "dir/file", then the common_prefix_len optimizations in fill_directory would cause us to bypass this problem, making it appear in simple tests that we could correctly remove manually specified pathspecs.)

That commit fixed the issue -- when multiple pathspecs were specified -- by making sure fill_directory() wouldn't return both "dir" and "dir/file" outside the common_prefix_len optimization path. This is where it starts to get fun.

In commit b9670c1f5e6b ("dir: fix checks on common prefix directory", 2019-12-19), we noticed that the common_prefix_len wasn't doing appropriate checks and letting all kinds of stuff through, resulting in recursing into .git/ directories and other craziness. So it started locking down and doing checks on pathnames within that code path. That continued with commit 777b42034764 ("dir: synchronize treat_leading_path() and read_directory_recursive()", 2019-12-19), which noted the following:

Our optimization to avoid calling into read_directory_recursive() when all pathspecs have a common leading directory mean that we need to match the logic that read_directory_recursive() would use if we had just called it from the root. Since it does more than call treat_path() we need to copy that same logic.

...and then it more forcefully addressed the issue with this wonderfully ironic statement:

Needing to duplicate logic like this means it is guaranteed someone will eventually need to make further changes and forget to update both locations. It is tempting to just nuke the leading_directory special casing to avoid such bugs and simplify the code, but unpack_trees' verify_clean_subdirectory() also calls read_directory() and does so with a non-empty leading path, so I'm hesitant to try to restructure further. Add obnoxious warnings to treat_leading_path() and read_directory_recursive() to try to warn people of such problems.

You would think that with such a strongly worded description, that its author would have actually ensured that the logic in treat_leading_path() and read_directory_recursive() did actually match and that *everything* that was needed had at least been copied over at the time that this paragraph was written. But you'd be wrong, I messed it up by missing part of the logic.

Copy the missing bits to fix the new final test in t7300.

Signed-off-by Elijah Newren <[email protected]>
---
 dir.c            | 4 ++++
 t/t7300-clean.sh | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)
dir.c +4 -0
index 7d255227b1..5d4c92d3aa 100644
@@ -2383,6 +2383,10 @@ static int treat_leading_path(struct dir_struct *dir,
 		    (dir->flags & DIR_SHOW_IGNORED_TOO ||
 		     do_match_pathspec(istate, pathspec, sb.buf, sb.len,
 				       baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC)) {
+			if (!match_pathspec(istate, pathspec, sb.buf, sb.len,
+					    0 /* prefix */, NULL,
+					    0 /* do NOT special case dirs */))
+				state = path_none;
 			add_path_to_appropriate_result_list(dir, NULL, &cdir,
 							    istate,
 							    &sb, baselen,
t/t7300-clean.sh +1 -1
index 782e125c89..cb5e34d94c 100755
@@ -737,7 +737,7 @@ test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '
 	test_i18ngrep "too long" .git/err
 '
 
-test_expect_failure 'clean untracked paths by pathspec' '
+test_expect_success 'clean untracked paths by pathspec' '
 	git init untracked &&
 	mkdir untracked/dir &&
 	echo >untracked/dir/file.txt &&
Signature · 1 line
gitgitgadget
#12 lore raw

Re: [PATCH v3 2/4] dir: treat_leading_path() and read_directory_recursive(), round 2

Quoted 8 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

I was going to title this "dir: more synchronizing of treat_leading_path() and read_directory_recursive()", a nod to commit 777b42034764 ("dir: synchronize treat_leading_path() and read_directory_recursive()", 2019-12-19), but the title was too long.

Anyway, first the backstory...

The backstory was helpful, thanks.

Quoted 1 line

Copy the missing bits to fix the new final test in t7300.

Thanks for the quick turnaround!

-Stolee

author
#13 lore raw

PATCH v3 3/4 dir: restructure in a way to avoid passing around a struct dirent

In reply to Elijah Newren via GitGitGadget

From: Jeff King <[email protected]>

Restructure the code slightly to avoid passing around a struct dirent anywhere, which also enables us to avoid trying to manufacture one.

Signed-off-by Jeff King <[email protected]>
Signed-off-by Elijah Newren <[email protected]>
---
 dir.c | 73 +++++++++++++++++++++++++----------------------------------
 1 file changed, 31 insertions(+), 42 deletions(-)
dir.c +31 -42
index 5d4c92d3aa..8437030006 100644
@@ -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)
 {

@@ -1215,8 +1216,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;
 		}

@@ -1842,10 +1842,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)

@@ -1870,14 +1869,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)

@@ -1985,21 +1983,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)

@@ -2087,10 +2082,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) {

@@ -2216,7 +2218,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,

@@ -2314,10 +2316,10 @@ static int treat_leading_path(struct dir_struct *dir,
 	 */
 
 	struct strbuf sb = STRBUF_INIT;
+	struct strbuf subdir = STRBUF_INIT;
 	int prevlen, baselen;
 	const char *cp;
 	struct cached_dir cdir;
-	struct dirent *de;
 	enum path_treatment state = path_none;
 
 	/*

@@ -2342,22 +2344,8 @@ static int treat_leading_path(struct dir_struct *dir,
 	if (!len)
 		return 1;
 
-	/*
-	 * We need a manufactured dirent with sufficient space to store a
-	 * leading directory component of path in its d_name.  Here, we
-	 * assume that the dirent's d_name is either declared as
-	 *    char d_name[BIG_ENOUGH]
-	 * or that it is declared at the end of the struct as
-	 *    char d_name[]
-	 * For either case, padding with len+1 bytes at the end will ensure
-	 * sufficient storage space.
-	 */
-	de = xcalloc(1, st_add3(sizeof(struct dirent), len, 1));
 	memset(&cdir, 0, sizeof(cdir));
-	cdir.de = de;
-#if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
-	de->d_type = DT_DIR;
-#endif
+	cdir.d_type = DT_DIR;
 	baselen = 0;
 	prevlen = 0;
 	while (1) {

@@ -2374,12 +2362,13 @@ static int treat_leading_path(struct dir_struct *dir,
 			break;
 		strbuf_reset(&sb);
 		strbuf_add(&sb, path, prevlen);
-		memcpy(de->d_name, path+prevlen, baselen-prevlen);
-		de->d_name[baselen-prevlen] = '\0';
+		strbuf_reset(&subdir);
+		strbuf_add(&subdir, path+prevlen, baselen-prevlen);
+		cdir.d_name = subdir.buf;
 		state = treat_path(dir, NULL, &cdir, istate, &sb, prevlen,
 				    pathspec);
 		if (state == path_untracked &&
-		    get_dtype(cdir.de, istate, sb.buf, sb.len) == DT_DIR &&
+		    resolve_dtype(cdir.d_type, istate, sb.buf, sb.len) == DT_DIR &&
 		    (dir->flags & DIR_SHOW_IGNORED_TOO ||
 		     do_match_pathspec(istate, pathspec, sb.buf, sb.len,
 				       baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC)) {

@@ -2403,7 +2392,7 @@ static int treat_leading_path(struct dir_struct *dir,
 					    &sb, baselen, pathspec,
 					    state);
 
-	free(de);
+	strbuf_release(&subdir);
 	strbuf_release(&sb);
 	return state == path_recurse;
 }
Signature · 1 line
gitgitgadget
author
#14 lore raw

PATCH v3 4/4 dir: point treat_leading_path() warning to the right place

In reply to Elijah Newren via GitGitGadget

From: Jeff King <[email protected]>

Commit 777b420347 (dir: synchronize treat_leading_path() and read_directory_recursive(), 2019-12-19) tried to add two warning comments in those functions, pointing at each other. But the one in treat_leading_path() just points at itself.

Let's fix that. Since the comment also redirects the reader for more details to "the commit that added this warning", and since we're now modifying the warning (creating a new commit without those details), let's mention the actual commit id.

Signed-off-by Jeff King <[email protected]>
Reviewed-by Elijah Newren <[email protected]>
---
 dir.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
dir.c +3 -3
index 8437030006..b460211e61 100644
@@ -2310,9 +2310,9 @@ static int treat_leading_path(struct dir_struct *dir,
 	 * WARNING WARNING WARNING:
 	 *
 	 * Any updates to the traversal logic here may need corresponding
-	 * updates in treat_leading_path().  See the commit message for the
-	 * commit adding this warning as well as the commit preceding it
-	 * for details.
+	 * updates in read_directory_recursive().  See 777b420347 (dir:
+	 * synchronize treat_leading_path() and read_directory_recursive(),
+	 * 2019-12-19) and its parent commit for details.
 	 */
 
 	struct strbuf sb = STRBUF_INIT;
Signature · 1 line
gitgitgadget