Re: Possible bug in git-completion.sh

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Nah, I should have checked the code.
>
> The implementation of ls-files does cd up and uses pathspec, so the intent
> is to apply higher level gitignore.
> 
> It however feeds paths from the already ignored directories, which _is_
> the real bug.
> ...
> I think a proper fix should be in dir.c::read_directory() where it calls
> read_directory_recursive() without first checking if the directory itself
> should be ignored.  read_directory_recursive() itself has a logic to see
> if the dirent it found is worth recursing into and a similar logic should
> be in the toplevel caller.

Actually doing less in fill_directory() turned out to be a simpler
solution.

builtin_add() cares about the return value from fill_directory() and
performs prune_directory() optimization magic, and we may want to change
it not to do so as well, but I didn't want to worry about too many things
at once, so this version still runs the "common_prefix()" that forgets to
take .gitignore at higher levels (or a $GIT_DIR/info/exclude entry that
ignores the common prefix directory of given pathspecs) into account.

Another possibility is to fix common_prefix() and make it walk the path it
returns one level at a time from the top, making sure they are not
ignored, and that would probably be a better fix, but at least this patch
will give you a starting point and tests to check the result against.

diff --git a/dir.c b/dir.c
index d0999ba..56d3f60 100644
--- a/dir.c
+++ b/dir.c
@@ -53,7 +53,6 @@ static int common_prefix(const char **pathspec)
 
 int fill_directory(struct dir_struct *dir, const char **pathspec)
 {
-	const char *path;
 	int len;
 
 	/*
@@ -61,13 +60,8 @@ int fill_directory(struct dir_struct *dir, const char **pathspec)
 	 * use that to optimize the directory walk
 	 */
 	len = common_prefix(pathspec);
-	path = "";
-
-	if (len)
-		path = xmemdupz(*pathspec, len);
-
 	/* Read the directory and prune it */
-	read_directory(dir, path, len, pathspec);
+	read_directory(dir, "", 0, pathspec);
 	return len;
 }
 
diff --git a/t/t3001-ls-files-others-exclude.sh b/t/t3001-ls-files-others-exclude.sh
index c65bca8..17d1764 100755
--- a/t/t3001-ls-files-others-exclude.sh
+++ b/t/t3001-ls-files-others-exclude.sh
@@ -153,4 +153,42 @@ 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
+	)
+'
+
+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_success 'subdirectory ignore (l1)' '
+	(
+		cd top/l1 &&
+		git ls-files -o --exclude-standard
+	) >actual &&
+	>expect &&
+	test_cmp expect actual
+'
+
 test_done