Odd .gitignore behaviour

Perhaps I'm misreading the manpage, but I think this is wrong:

% mkdir base; cd base
% git init
% mkdir -p sub1/sub2
% cd sub1
% echo foo > .gitignore; echo '!sub2/foo' >> .gitignore % touch sub2/foo
% git add sub2/foo
The following paths are ignored by one of your .gitignore files:
sub1/sub2/foo
Use -f if you really want to add them.
fatal: no files added

So sub1/sub2/foo matches the first pattern in sub1/.gitignore, but it also matches the negated pattern '!sub2/foo' (in the same file, so precedence isn't an issue). And the manpage says

o An optional prefix ! which negates the pattern; any matching file excluded by a previous pattern will become included again. If a negated pattern matches, this will override lower precedence patterns sources.

So surely sub1/sub2/foo ought to be included again? Or is the first line in sub1/.gitignore not "a previous pattern" in this sense?

If I move the "foo" pattern up a level, creating a .gitignore in base just containing "foo", then sub1/sub2/foo is still regarded as ignored, even though it surely matches the negating pattern sub1/.gitignore, and that should be of higher precedence than the pattern in base/.gitignore?

Yes.

The negated patterns have to show up *before* the patterns that they override, because the first pattern that matches is the one that is used.

The priority between nested .gitignore files should be that inner files have higher priority than outer files (since the inner ones "know more", and the outer ones are "generic").

So what you describe sounds wrong.

But my quick test didn't actually support the behaviour you see. In this situation:

	[torvalds@woody git-test]$ cat .gitignore
	a*
	[torvalds@woody git-test]$ cat subdir/.gitignore
	!a-ok
	[torvalds@woody git-test]$ find *
	all-files
	subdir
	subdir/a-ok
	[torvalds@woody git-test]$ git ls-files -o --exclude-per-directory=.gitignore
	.gitignore
	subdir/.gitignore
	subdir/a-ok

ie we *do* show "showdir/a-ok" (but we don't show "all-files") because a-ok is explicitly marked to be not ignored by a higher-priority rule.

Side note: "git status" uses the "--directory" flag, so it will not even recurse into unknown directories, and will instead apply the gitignore rules to the directory names, so you get:

	[torvalds@woody git-test]$ git ls-files -o --exclude-per-directory=.gitignore --directory
	.gitignore
	subdir/

which doesn't show "a-ok", but that's for a totally unrelated reason and has nothing to do with .gitignore!

Linus

Linus Torvalds <[email protected]> writes:

[...]

OK, and if I say "git add subdir/a-ok" I get no error, as expected.

So extend it just a tiny bit, moving a-ok down a directory and using the negative pattern "!subsubdir/a-ok" to match:

brs% cat .gitignore
a*
brs% cat subdir/.gitignore
!subsubdir/a-ok
brs% find *
all-files
subdir
subdir/.gitignore
subdir/subsubdir
subdir/subsubdir/a-ok
subdir/a-ok
brs% git ls-files -o --exclude-per-directory=.gitignore .gitignore
subdir/.gitignore
subdir/subsubdir/a-ok
brs% git add subdir/subsubdir/a-ok
The following paths are ignored by one of your .gitignore files:
subdir/subsubdir/a-ok
Use -f if you really want to add them.
fatal: no files added

So I think the output from git-ls-files is as expected (as I interpret the manpage and your explanation). So is git-add just using some different code?

[...]

No, you found one of the longstanding bugs in dir.c:read_directory().

The funny thing is that I just sent out a message pointing out bogus handling of per-directory exclude files in ls-files last night. Somehow people have a tendency to encounter the bugs in the same vicinity independently.

The initial loop in read_directory() to push per-directory exclusion elements into the stack for directories above the given base forgets that push() does not make a copy of the path given as its parameter but stores the pointer to it instead, so multiple calls to push() need to use separate path buffers.

Here is a tentative patch. I do not think the patch is broken but I call it tentative because:

- It is ugly -- I never get this "walking path delimited by slashes" loop right;

- It leaks the path buffer given to push(), but it is inherent in the design of "push/pop exclude per-directory" API. They were designed to be called from the recursive directory walking, and the path buffers are placed on the function call stack to be reclaimed automatically upon function return;

---

 dir.c |   55 ++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 34 insertions(+), 21 deletions(-)

index fa9f902..d32f437 100644
@@ -651,38 +651,51 @@ static void free_simplify(struct path_simplify *simplify)
 		free(simplify);
 }
 
+static int push_excludes(struct dir_struct *dir, const char *base, int len)
+{
+	/*
+	 * base is like "a/b/c/" -- cause .gitignore, b/.gitignore and
+	 * b/c/.gitignore to be read in this order, as if we recursed
+	 * into it.
+	 */
+	int stk = -1;
+	int partlen = 0;
+
+	if (!(dir->exclude_per_dir && len))
+		return stk;
+
+	while (1) {
+		char *part = xmalloc(partlen + 1);
+		memcpy(part, base, partlen);
+		part[partlen] = '\0';
+		stk = push_exclude_per_directory(dir, part, partlen);
+
+		if (len <= partlen++)
+			break;
+
+		while (partlen < len && base[partlen] != '/')
+			partlen++;
+		partlen++; /* point at one past the found '/' */
+	}
+	return stk;
+}
+
 int read_directory(struct dir_struct *dir, const char *path, const char *base, int baselen, const char **pathspec)
 {
 	struct path_simplify *simplify = create_simplify(pathspec);
+	int stk;
 
 	/*
 	 * Make sure to do the per-directory exclude for all the
 	 * directories leading up to our base.
 	 */
-	if (baselen) {
-		if (dir->exclude_per_dir) {
-			char *p, *pp = xmalloc(baselen+1);
-			memcpy(pp, base, baselen+1);
-			p = pp;
-			while (1) {
-				char save = *p;
-				*p = 0;
-				push_exclude_per_directory(dir, pp, p-pp);
-				*p++ = save;
-				if (!save)
-					break;
-				p = strchr(p, '/');
-				if (p)
-					p++;
-				else
-					p = pp + baselen;
-			}
-			free(pp);
-		}
-	}
+	stk = push_excludes(dir, base, baselen);
 
 	read_directory_recursive(dir, path, base, baselen, 0, simplify);
 	free_simplify(simplify);
+	if (0 <= stk)
+		pop_exclude_per_directory(dir, stk);
+
 	qsort(dir->entries, dir->nr, sizeof(struct dir_entry *), cmp_name);
 	qsort(dir->ignored, dir->ignored_nr, sizeof(struct dir_entry *), cmp_name);
 	return dir->nr;

By the way, about the problem I described briefly last night.

I never understood the intended use of -i option to ls-files, but in your test repository (the one that has subsubdir), you can do:

	$ git ls-files -i --exclude='a*'

to see "What paths have I already _staged_ that would have been ignored if the exclude pattern were 'a*'". You can abuse this to list all the staged header files with:

	$ git ls-files -i --exclude='*.h'

but

	$ git ls-files -- '*.h'

is much simpler for that ;-).

In any case, it appears to me that the codepath used for that "feature", and also the codepath used for -d (show deleted files) and -m (show modified files) makes calls to excluded() function to consult the exclusion mechanism without setting it up properly, and I do not think

	$ git ls-files -i --exclude-per-directory=.gitignore

does what we would want.

The codepath for -o (show others) do use read_directory() which sets up the exclusion mechanism with push/pop per-directory exclude API, so that option should work. But I suspect even it did not work from a subdirectory because of the problem the message I am responding to addresses.

In "dir_struct", each exclusion element in the exclusion stack records a base string (pointer to the beginning with length) so that we can tell where it came from, but this pointer is just pointing at the parameter that is given by the caller to the push_exclude_per_directory() function.

While read_directory_recursive() runs, calls to excluded() makes use the data in the exclusion elements, including this base string. The caller of read_directory_recursive() is not supposed to free the buffer it gave to push_exclude_per_directory() earlier, until it returns.

The test case Bruce Stephens gave in the mailing list discussion was simplified and added to the t3700 test.

---

It turns out that a minimally invasive fix was a lot simpler than I thought.

This still does not fix the other codepaths in ls-files that does not use read_directory() but walks the cache.

 dir.c          |    6 ++++--
 t/t3700-add.sh |   24 ++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 2 deletions(-)

index fa9f902..225fdfb 100644
@@ -654,6 +654,7 @@ static void free_simplify(struct path_simplify *simplify)
 int read_directory(struct dir_struct *dir, const char *path, const char *base, int baselen, const char **pathspec)
 {
 	struct path_simplify *simplify = create_simplify(pathspec);
+	char *pp = NULL;
 
 	/*
 	 * Make sure to do the per-directory exclude for all the

@@ -661,7 +662,8 @@ int read_directory(struct dir_struct *dir, const char *path, const char *base, i
 	 */
 	if (baselen) {
 		if (dir->exclude_per_dir) {
-			char *p, *pp = xmalloc(baselen+1);
+			char *p;
+			pp = xmalloc(baselen+1);
 			memcpy(pp, base, baselen+1);
 			p = pp;
 			while (1) {

@@ -677,12 +679,12 @@ int read_directory(struct dir_struct *dir, const char *path, const char *base, i
 				else
 					p = pp + baselen;
 			}
-			free(pp);
 		}
 	}
 
 	read_directory_recursive(dir, path, base, baselen, 0, simplify);
 	free_simplify(simplify);
+	free(pp);
 	qsort(dir->entries, dir->nr, sizeof(struct dir_entry *), cmp_name);
 	qsort(dir->ignored, dir->ignored_nr, sizeof(struct dir_entry *), cmp_name);
 	return dir->nr;

index a328bf5..287e058 100755
@@ -104,9 +104,33 @@ test_expect_success 'add ignored ones with -f' '
 	git ls-files --error-unmatch d.ig/d.if d.ig/d.ig
 '
 
+test_expect_success 'add ignored ones with -f' '
+	rm -f .git/index &&
+	git add -f d.?? &&
+	git ls-files --error-unmatch d.ig/d.if d.ig/d.ig
+'
+
+test_expect_success '.gitignore with subdirectory' '
+
+	rm -f .git/index &&
+	mkdir -p sub/dir &&
+	echo "!dir/a.*" >sub/.gitignore &&
+	>sub/a.ig &&
+	>sub/dir/a.ig &&
+	git add sub/dir &&
+	git ls-files --error-unmatch sub/dir/a.ig &&
+	rm -f .git/index &&
+	(
+		cd sub/dir &&
+		git add .
+	) &&
+	git ls-files --error-unmatch sub/dir/a.ig
+'
+
 mkdir 1 1/2 1/3
 touch 1/2/a 1/3/b 1/2/c
 test_expect_success 'check correct prefix detection' '
+	rm -f .git/index &&
 	git add 1/2/a 1/3/b 1/2/c
 '
 

Junio C Hamano <[email protected]> writes:

[...]

Cool, that fixes the "git add" issue I was seeing. (So Acked-by:
Bruce Stephens <[email protected]>, for what it's worth.)

I guess really the output of "git status" (or "git runstatus") is more significant since that's what we'd normally be running (that's presumably what "git gui" and similar tools run, though perhaps they use "git ls-files"---probably the underlying code's the same, I guess).

However, that doesn't mean the issue with git add shouldn't be resolved.

[...]

Bruce Stephens <[email protected]> writes:

[...]

Bah. "git status" also works correctly, I think. I was just testing stupidly.

[...]