Odd .gitignore behaviour

Odd .gitignore behaviour

[Bruce Stephens]

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?

Re: Odd .gitignore behaviour

[Linus Torvalds]

On Thu, 15 Nov 2007, Bruce Stephens wrote:
> 
> 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?

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.

> 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?

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

Re: Odd .gitignore behaviour

[Bruce Stephens]

Linus Torvalds <torvalds@linux-foundation.org> writes:

[...]

> 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.

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?

[...]

Re: Odd .gitignore behaviour

[Junio C Hamano]

Bruce Stephens <bruce.stephens@isode.com> writes:

> 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(-)

diff --git a/dir.c b/dir.c
index fa9f902..d32f437 100644
--- a/dir.c
+++ b/dir.c
@@ -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;

Re: Odd .gitignore behaviour

[Junio C Hamano]

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

> Bruce Stephens <bruce.stephens@isode.com> writes:
>
>> 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.

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.

Fix per-directory exclude handing for "git add"

[Junio C Hamano]

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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

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

 > 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.

 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(-)

diff --git a/dir.c b/dir.c
index fa9f902..225fdfb 100644
--- a/dir.c
+++ b/dir.c
@@ -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;
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index a328bf5..287e058 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -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
 '
 

Re: Fix per-directory exclude handing for "git add"

[Bruce Stephens]

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

[...]

> 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.

Cool, that fixes the "git add" issue I was seeing.  (So Acked-by:
Bruce Stephens <bruce.stephens@isode.com>, 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.

[...]

Re: Fix per-directory exclude handing for "git add"

[Bruce Stephens]

Bruce Stephens <bruce.stephens@isode.com> writes:

[...]

> 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).

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

[...]