Re: [PATCH] ls-files: fix overeager pathspec optimization

6 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH] ls-files: fix overeager pathspec optimization

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:47:59

Linus Torvalds [off-list ref] writes:
I'd have to see both patches to be able to tell. I do admit that while I
acked your patch, it sure ain't _pretty_ to do that special odd
"has_leading_ignored_dir()" thing.
Revised patch (v4) series is coming shortly.

[PATCH 1/4] t3001: test ls-files -o ignored/dir

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:47:59

When you have "t" directory that is marked as ignored in the top-level
.gitignore file (or $GIT_DIR/info/exclude), running

    $ git ls-files -o --exclude-standard

from the top-level correctly excludes files in "t" directory, but
any of the following:

    $ git ls-files -o --exclude-standard t/
    $ cd t && git ls-files -o --exclude-standard

would show untracked files in that directory.

Signed-off-by: Junio C Hamano <redacted>
---
 t/t3001-ls-files-others-exclude.sh |   39 ++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)
diff --git a/t/t3001-ls-files-others-exclude.sh b/t/t3001-ls-files-others-exclude.sh
index c65bca8..e3e4d71 100755
--- a/t/t3001-ls-files-others-exclude.sh
+++ b/t/t3001-ls-files-others-exclude.sh
@@ -153,4 +153,43 @@ test_expect_success 'negated exclude matches can override previous ones' '
 	grep "^a.1" output
 '
 
+test_expect_success 'subdirectory ignore (setup)' '
+	mkdir -p top/l1/l2 &&
+	(
+		cd top &&
+		git init &&
+		echo /.gitignore >.gitignore &&
+		echo l1 >>.gitignore &&
+		echo l2 >l1/.gitignore &&
+		>l1/l2/l1
+	)
+'
+
+test_expect_success 'subdirectory ignore (toplevel)' '
+	(
+		cd top &&
+		git ls-files -o --exclude-standard
+	) >actual &&
+	>expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'subdirectory ignore (l1/l2)' '
+	(
+		cd top/l1/l2 &&
+		git ls-files -o --exclude-standard
+	) >actual &&
+	>expect &&
+	test_cmp expect actual
+'
+
+test_expect_failure 'subdirectory ignore (l1)' '
+	(
+		cd top/l1 &&
+		git ls-files -o --exclude-standard
+	) >actual &&
+	>expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.6.6.209.g52296.dirty

[PATCH 2/4] read_directory_recursive(): refactor handling of a single path into a separate function

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:47:59

Primarily because I want to reuse it in a separate function later,
but this de-dents a huge function by one tabstop which by itself is
an improvement as well.

Signed-off-by: Junio C Hamano <redacted>
---
 dir.c |  153 ++++++++++++++++++++++++++++++++++++++---------------------------
 1 files changed, 90 insertions(+), 63 deletions(-)
diff --git a/dir.c b/dir.c
index d0999ba..dec8365 100644
--- a/dir.c
+++ b/dir.c
@@ -625,6 +625,84 @@ static int get_dtype(struct dirent *de, const char *path, int len)
 	return dtype;
 }
 
+enum path_treatment {
+	path_ignored,
+	path_handled,
+	path_recurse,
+};
+
+static enum path_treatment treat_path(struct dir_struct *dir,
+				      struct dirent *de,
+				      char *path, int path_max,
+				      int baselen,
+				      const struct path_simplify *simplify,
+				      int *len)
+{
+	int dtype, exclude;
+
+	if (is_dot_or_dotdot(de->d_name) || !strcmp(de->d_name, ".git"))
+		return path_ignored;
+	*len = strlen(de->d_name);
+	/* Ignore overly long pathnames! */
+	if (*len + baselen + 8 > path_max)
+		return path_ignored;
+	memcpy(path + baselen, de->d_name, *len + 1);
+	*len += baselen;
+	if (simplify_away(path, *len, simplify))
+		return path_ignored;
+
+	dtype = DTYPE(de);
+	exclude = excluded(dir, path, &dtype);
+	if (exclude && (dir->flags & DIR_COLLECT_IGNORED)
+	    && in_pathspec(path, *len, simplify))
+		dir_add_ignored(dir, path, *len);
+
+	/*
+	 * Excluded? If we don't explicitly want to show
+	 * ignored files, ignore it
+	 */
+	if (exclude && !(dir->flags & DIR_SHOW_IGNORED))
+		return path_ignored;
+
+	if (dtype == DT_UNKNOWN)
+		dtype = get_dtype(de, path, *len);
+
+	/*
+	 * Do we want to see just the ignored files?
+	 * We still need to recurse into directories,
+	 * even if we don't ignore them, since the
+	 * directory may contain files that we do..
+	 */
+	if (!exclude && (dir->flags & DIR_SHOW_IGNORED)) {
+		if (dtype != DT_DIR)
+			return path_ignored;
+	}
+
+	switch (dtype) {
+	default:
+		return path_ignored;
+	case DT_DIR:
+		memcpy(path + *len, "/", 2);
+		(*len)++;
+		switch (treat_directory(dir, path, *len, simplify)) {
+		case show_directory:
+			if (exclude != !!(dir->flags
+					  & DIR_SHOW_IGNORED))
+				return path_ignored;
+			break;
+		case recurse_into_directory:
+			return path_recurse;
+		case ignore_directory:
+			return path_ignored;
+		}
+		break;
+	case DT_REG:
+	case DT_LNK:
+		break;
+	}
+	return path_handled;
+}
+
 /*
  * Read a directory tree. We currently ignore anything but
  * directories, regular files and symlinks. That's because git
@@ -634,7 +712,10 @@ static int get_dtype(struct dirent *de, const char *path, int len)
  * Also, we ignore the name ".git" (even if it is not a directory).
  * That likely will not change.
  */
-static int read_directory_recursive(struct dir_struct *dir, const char *base, int baselen, int check_only, const struct path_simplify *simplify)
+static int read_directory_recursive(struct dir_struct *dir,
+				    const char *base, int baselen,
+				    int check_only,
+				    const struct path_simplify *simplify)
 {
 	DIR *fdir = opendir(*base ? base : ".");
 	int contents = 0;
@@ -645,70 +726,16 @@ static int read_directory_recursive(struct dir_struct *dir, const char *base, in
 		memcpy(path, base, baselen);
 
 		while ((de = readdir(fdir)) != NULL) {
-			int len, dtype;
-			int exclude;
-
-			if (is_dot_or_dotdot(de->d_name) ||
-			     !strcmp(de->d_name, ".git"))
-				continue;
-			len = strlen(de->d_name);
-			/* Ignore overly long pathnames! */
-			if (len + baselen + 8 > sizeof(path))
+			int len;
+			switch (treat_path(dir, de, path, sizeof(path),
+					   baselen, simplify, &len)) {
+			case path_recurse:
+				contents += read_directory_recursive
+					(dir, path, len, 0, simplify);
 				continue;
-			memcpy(path + baselen, de->d_name, len+1);
-			len = baselen + len;
-			if (simplify_away(path, len, simplify))
+			case path_ignored:
 				continue;
-
-			dtype = DTYPE(de);
-			exclude = excluded(dir, path, &dtype);
-			if (exclude && (dir->flags & DIR_COLLECT_IGNORED)
-			    && in_pathspec(path, len, simplify))
-				dir_add_ignored(dir, path,len);
-
-			/*
-			 * Excluded? If we don't explicitly want to show
-			 * ignored files, ignore it
-			 */
-			if (exclude && !(dir->flags & DIR_SHOW_IGNORED))
-				continue;
-
-			if (dtype == DT_UNKNOWN)
-				dtype = get_dtype(de, path, len);
-
-			/*
-			 * Do we want to see just the ignored files?
-			 * We still need to recurse into directories,
-			 * even if we don't ignore them, since the
-			 * directory may contain files that we do..
-			 */
-			if (!exclude && (dir->flags & DIR_SHOW_IGNORED)) {
-				if (dtype != DT_DIR)
-					continue;
-			}
-
-			switch (dtype) {
-			default:
-				continue;
-			case DT_DIR:
-				memcpy(path + len, "/", 2);
-				len++;
-				switch (treat_directory(dir, path, len, simplify)) {
-				case show_directory:
-					if (exclude != !!(dir->flags
-							& DIR_SHOW_IGNORED))
-						continue;
-					break;
-				case recurse_into_directory:
-					contents += read_directory_recursive(dir,
-						path, len, 0, simplify);
-					continue;
-				case ignore_directory:
-					continue;
-				}
-				break;
-			case DT_REG:
-			case DT_LNK:
+			case path_handled:
 				break;
 			}
 			contents++;
-- 
1.6.6.209.g52296.dirty

[PATCH 3/4] read_directory(): further split treat_path()

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:47:59

The next caller I'll be adding won't have an access to struct dirent
because it won't be reading from a directory stream.  Split the main
part of the function further into a separate function to make it usable
by a caller without passing a dirent as long as it knows what type is
feeding the function.

Signed-off-by: Junio C Hamano <redacted>
---
 dir.c |   50 +++++++++++++++++++++++++++++---------------------
 1 files changed, 29 insertions(+), 21 deletions(-)
diff --git a/dir.c b/dir.c
index dec8365..35cc89b 100644
--- a/dir.c
+++ b/dir.c
@@ -631,28 +631,12 @@ enum path_treatment {
 	path_recurse,
 };
 
-static enum path_treatment treat_path(struct dir_struct *dir,
-				      struct dirent *de,
-				      char *path, int path_max,
-				      int baselen,
-				      const struct path_simplify *simplify,
-				      int *len)
+static enum path_treatment treat_one_path(struct dir_struct *dir,
+					  char *path, int *len,
+					  const struct path_simplify *simplify,
+					  int dtype, struct dirent *de)
 {
-	int dtype, exclude;
-
-	if (is_dot_or_dotdot(de->d_name) || !strcmp(de->d_name, ".git"))
-		return path_ignored;
-	*len = strlen(de->d_name);
-	/* Ignore overly long pathnames! */
-	if (*len + baselen + 8 > path_max)
-		return path_ignored;
-	memcpy(path + baselen, de->d_name, *len + 1);
-	*len += baselen;
-	if (simplify_away(path, *len, simplify))
-		return path_ignored;
-
-	dtype = DTYPE(de);
-	exclude = excluded(dir, path, &dtype);
+	int exclude = excluded(dir, path, &dtype);
 	if (exclude && (dir->flags & DIR_COLLECT_IGNORED)
 	    && in_pathspec(path, *len, simplify))
 		dir_add_ignored(dir, path, *len);
@@ -703,6 +687,30 @@ static enum path_treatment treat_path(struct dir_struct *dir,
 	return path_handled;
 }
 
+static enum path_treatment treat_path(struct dir_struct *dir,
+				      struct dirent *de,
+				      char *path, int path_max,
+				      int baselen,
+				      const struct path_simplify *simplify,
+				      int *len)
+{
+	int dtype;
+
+	if (is_dot_or_dotdot(de->d_name) || !strcmp(de->d_name, ".git"))
+		return path_ignored;
+	*len = strlen(de->d_name);
+	/* Ignore overly long pathnames! */
+	if (*len + baselen + 8 > path_max)
+		return path_ignored;
+	memcpy(path + baselen, de->d_name, *len + 1);
+	*len += baselen;
+	if (simplify_away(path, *len, simplify))
+		return path_ignored;
+
+	dtype = DTYPE(de);
+	return treat_one_path(dir, path, len, simplify, dtype, de);
+}
+
 /*
  * Read a directory tree. We currently ignore anything but
  * directories, regular files and symlinks. That's because git
-- 
1.6.6.209.g52296.dirty

[PATCH 4/4] ls-files: fix overeager pathspec optimization

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:47:59

Given pathspecs that share a common prefix, ls-files optimized its call
into recursive directory reader by starting at the common prefix
directory.

If you have a directory "t" with an untracked file "t/junk" in it, but the
top-level .gitignore file told us to ignore "t/", this resulted in:

    $ git ls-files -o --exclude-standard
    $ git ls-files -o --exclude-standard t/
    t/junk
    $ git ls-files -o --exclude-standard t/junk
    t/junk
    $ cd t && git ls-files -o --exclude-standard
    junk

We could argue that you are overriding the ignore file by giving a
patchspec that matches or being in that directory, but it is somewhat
unexpected.  Worse yet, these behave differently:

    $ git ls-files -o --exclude-standard t/ .
    $ git ls-files -o --exclude-standard t/
    t/junk

This patch changes the optimization so that it notices when the common
prefix directory that it starts reading from is an ignored one.

Signed-off-by: Junio C Hamano <redacted>
---
 dir.c                              |   38 +++++++++++++++++++++++++++++++++++-
 t/t3001-ls-files-others-exclude.sh |    2 +-
 2 files changed, 38 insertions(+), 2 deletions(-)
diff --git a/dir.c b/dir.c
index 35cc89b..00d698d 100644
--- a/dir.c
+++ b/dir.c
@@ -813,6 +813,41 @@ static void free_simplify(struct path_simplify *simplify)
 	free(simplify);
 }
 
+static int treat_leading_path(struct dir_struct *dir,
+			      const char *path, int len,
+			      const struct path_simplify *simplify)
+{
+	char pathbuf[PATH_MAX];
+	int baselen, blen;
+	const char *cp;
+
+	while (len && path[len - 1] == '/')
+		len--;
+	if (!len)
+		return 1;
+	baselen = 0;
+	while (1) {
+		cp = path + baselen + !!baselen;
+		cp = memchr(cp, '/', path + len - cp);
+		if (!cp)
+			baselen = len;
+		else
+			baselen = cp - path;
+		memcpy(pathbuf, path, baselen);
+		pathbuf[baselen] = '\0';
+		if (!is_directory(pathbuf))
+			return 0;
+		if (simplify_away(pathbuf, baselen, simplify))
+			return 0;
+		blen = baselen;
+		if (treat_one_path(dir, pathbuf, &blen, simplify,
+				   DT_DIR, NULL) == path_ignored)
+			return 0; /* do not recurse into it */
+		if (len <= baselen)
+			return 1; /* finished checking */
+	}
+}
+
 int read_directory(struct dir_struct *dir, const char *path, int len, const char **pathspec)
 {
 	struct path_simplify *simplify;
@@ -821,7 +856,8 @@ int read_directory(struct dir_struct *dir, const char *path, int len, const char
 		return dir->nr;
 
 	simplify = create_simplify(pathspec);
-	read_directory_recursive(dir, path, len, 0, simplify);
+	if (!len || treat_leading_path(dir, path, len, simplify))
+		read_directory_recursive(dir, path, len, 0, simplify);
 	free_simplify(simplify);
 	qsort(dir->entries, dir->nr, sizeof(struct dir_entry *), cmp_name);
 	qsort(dir->ignored, dir->ignored_nr, sizeof(struct dir_entry *), cmp_name);
diff --git a/t/t3001-ls-files-others-exclude.sh b/t/t3001-ls-files-others-exclude.sh
index e3e4d71..9e71260 100755
--- a/t/t3001-ls-files-others-exclude.sh
+++ b/t/t3001-ls-files-others-exclude.sh
@@ -183,7 +183,7 @@ test_expect_success 'subdirectory ignore (l1/l2)' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'subdirectory ignore (l1)' '
+test_expect_success 'subdirectory ignore (l1)' '
 	(
 		cd top/l1 &&
 		git ls-files -o --exclude-standard
-- 
1.6.6.209.g52296.dirty

Re: [PATCH 4/4] ls-files: fix overeager pathspec optimization

From: Jeff King <hidden>
Date: 2016-06-15 22:48:00

On Fri, Jan 08, 2010 at 11:35:35PM -0800, Junio C Hamano wrote:
This patch changes the optimization so that it notices when the common
prefix directory that it starts reading from is an ignored one.
Having just produced the similar but more ugly and messy patch earlier
in the thread, this series looks right to me.

-Peff
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help