Re: [PATCH] ls-files: fix overeager pathspec optimization
From: Jeff King <hidden>
Date: 2016-06-15 22:47:59
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH] ls-files: fix overeager pathspec optimization · Junio C Hamano <hidden>
- 2016-06-15 · Re: [PATCH] ls-files: fix overeager pathspec optimization · Linus Torvalds <torvalds@linux-foundation.org>
- 2016-06-15 · Re: [PATCH] ls-files: fix overeager pathspec optimization · Junio C Hamano <hidden>
- 2016-06-15 · Re: [PATCH] ls-files: fix overeager pathspec optimization · Junio C Hamano <hidden>
- 2016-06-15 · Re: [PATCH] ls-files: fix overeager pathspec optimization · Linus Torvalds <torvalds@linux-foundation.org>
On Fri, Jan 08, 2010 at 05:07:46PM -0800, Linus Torvalds wrote:
quoted
But I am starting to wonder if we might be better off restructuring read_directory_recursive(). Currently it assumes that the path it was given _must_ be of interest (i.e. not ignored) and runs excluded() on subdirectories it finds to make that same decision before recursing into them or skipping them. It might make more sense if it first checked if the path given by the caller should be ignored and act accordingly.Hmm. I can't make myself care one way or the other, I have to admit. I assume you mean basically taking the path and using the first component of it _instead_ of doing a readdir() - and getting rid of the simplification up front? I agree that that should work. Would it be simpler and cleaner? Perhaps. 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.
It would look something like this:
diff --git a/dir.c b/dir.c
index 3a8d3e6..306d354 100644
--- a/dir.c
+++ b/dir.c@@ -811,12 +811,19 @@ static void free_simplify(struct path_simplify *simplify) int read_directory(struct dir_struct *dir, const char *path, int len, const char **pathspec) { struct path_simplify *simplify; + int d_type = DT_DIR; + int exclude; if (has_symlink_leading_path(path, len)) return dir->nr; simplify = create_simplify(pathspec); - read_directory_recursive(dir, path, len, 0, simplify); + exclude = excluded(dir, path, &d_type); + if (exclude && (dir->flags & DIR_COLLECT_IGNORED) && + in_pathspec(path, len, simplify)) + dir_add_ignored(dir, path, len); + if (!exclude || (dir->flags & DIR_SHOW_IGNORED)) + 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);
But unfortunately excluded() is not happy with the trailing slash on the path given to read_directory, so we also need on top:
diff --git a/dir.c b/dir.c
index 306d354..6045a84 100644
--- a/dir.c
+++ b/dir.c@@ -813,12 +813,17 @@ int read_directory(struct dir_struct *dir, const char *path, int len, const char struct path_simplify *simplify; int d_type = DT_DIR; int exclude; + char *path_without_slash; if (has_symlink_leading_path(path, len)) return dir->nr; simplify = create_simplify(pathspec); - exclude = excluded(dir, path, &d_type); + path_without_slash = xstrdup(path); + if (path_without_slash[strlen(path_without_slash)-1] == '/') + path_without_slash[strlen(path_without_slash)-1] = '\0'; + exclude = excluded(dir, path_without_slash, &d_type); + free(path_without_slash); if (exclude && (dir->flags & DIR_COLLECT_IGNORED) && in_pathspec(path, len, simplify)) dir_add_ignored(dir, path, len);
And that does fix the case that triggered this whole discussion, but I haven't tested thoroughly to make sure we are not adversely affecting other cases. -Peff