Skip to contentSkip to conversation

1.8.3 - gitignore not being parsed correctly on OS X; regex support is broken?

PATCH In review Misty De Meo 7 participants 16 messages Jun 15, 2016

lore
Share & tools
lore

Misty De Meo

author
#0 lore raw

Hi,

Gitignore parsing no longer seems to work properly in git 1.8.3.

One of my repositories has the following gitignore:

/*
!/.gitignore
!/Library/
!/CONTRIBUTING.md
!/README.md
!/SUPPORTERS.md
!/bin
/bin/*
!/bin/brew
!/share/man/man1/brew.1
.DS_Store
/Library/LinkedKegs
/Library/PinnedKegs
/Library/Taps
/Library/Formula/.gitignore

In 1.8.2.3 and earlier, this works as expected. However, in 1.8.3 I'm seeing every file in /bin/ being marked as an untracked file.

I asked about this in #git, and was told that the culprit was the regex support; apparently recompiling without regex support fixes the specific gitignore issue. However, this doesn't seem to have been reported anywhere on the mailing list that I can see. I was also told that the issue is OS X-specific, and doesn't happen on other platforms.

Thanks,
Misty De Meo

#1 lore raw
Quoted 22 lines · Misty De Meo

Hi,

Gitignore parsing no longer seems to work properly in git 1.8.3.

One of my repositories has the following gitignore:

/*
!/.gitignore
!/Library/
!/CONTRIBUTING.md
!/README.md
!/SUPPORTERS.md
!/bin
/bin/*
!/bin/brew
!/share/man/man1/brew.1
.DS_Store
/Library/LinkedKegs
/Library/PinnedKegs
/Library/Taps
/Library/Formula/.gitignore

Quoted 2 lines

In 1.8.2.3 and earlier, this works as expected. However, in 1.8.3 I'm seeing every file in /bin/ being marked as an untracked file.

The changes in this area since 1.8.2.3 seem to be Karsten's (I'm not blaming, just wanted to narrow down the problem). The patterns of interest seem to be

!/bin
/bin/*
!/bin/brew

Without "!/bin" v1.8.3 seems to behave the same as v1.8.2.3. Can you verify it?

Quoted 6 lines

I asked about this in #git, and was told that the culprit was the regex support; apparently recompiling without regex support fixes the specific gitignore issue. However, this doesn't seem to have been reported anywhere on the mailing list that I can see. I was also told that the issue is OS X-specific, and doesn't happen on other platforms.

Puzzled. regex has nothing to do with gitignore (glob does). How do you recompile without regex support? Setting NO_REGEX? I'm on Linux btw, no chance of touching OS X.
--
Duy

#2 lore raw
Quoted 9 lines · Duy Nguyen

The changes in this area since 1.8.2.3 seem to be Karsten's (I'm not blaming, just wanted to narrow down the problem). The patterns of interest seem to be

!/bin
/bin/*
!/bin/brew

Without "!/bin" v1.8.3 seems to behave the same as v1.8.2.3.

Karsten, the block "/* Abort if the directory is excluded */" in prep_exclude() seems to cause this. I think it goes through the exclude patterns, hits "!/bin", believes the patterns do not make sense in this context and throws all away. I think Øystein's case falls into the same path. Commenting out the block seems to gain the old behavior back (and probably breaks other stuff). Contrary to what Junio said, I'm clueless about this. I wanted to read your series through and eventually gave up. I think I now have the motivation to look at it again this weekend.
--
Duy

#3 lore raw

Am 29.05.2013 06:19, schrieb Duy Nguyen:

Quoted 9 lines · Duy Nguyen
Quoted 8 lines

The changes in this area since 1.8.2.3 seem to be Karsten's (I'm not blaming, just wanted to narrow down the problem). The patterns of interest seem to be

!/bin
/bin/*
!/bin/brew

Quoted 6 lines
Quoted 1 line

Without "!/bin" v1.8.3 seems to behave the same as v1.8.2.3.

Karsten, the block "/* Abort if the directory is excluded */" in prep_exclude() seems to cause this. I think it goes through the exclude patterns, hits "!/bin", believes the patterns do not make sense in this context and throws all away.

Yes, I forgot to check the "!" flag to determine if the directory is really excluded. I'll prepare a patch + test case for this.

@Øystein: in the meantime, could you check if this fixes the problem for you?

--- 8< ---

dir.c +3 -0
index a5926fb..13858fe 100644
@@ -821,6 +821,9 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 				dir->basebuf, stk->baselen - 1,
 				dir->basebuf + current, &dt);
 			dir->basebuf[stk->baselen - 1] = '/';
+			if (dir->exclude &&
+			    dir->exclude->flags & EXC_FLAG_NEGATIVE)
+				dir->exclude = NULL;
 			if (dir->exclude) {
 				dir->basebuf[stk->baselen] = 0;
 				dir->exclude_stack = stk;
#4 lore raw

PATCH 1/1 dir.c: fix ignore processing within not-ignored directories

As of 95c6f271 "dir.c: unify is_excluded and is_path_excluded APIs", the is_excluded API no longer recurses into directories that match an ignore pattern, and returns the directory's ignored state for all contained paths.

This is OK for normal ignore patterns, i.e. ignoring a directory affects the entire contents recursively.

Unfortunately, this also "works" for negated ignore patterns ('!dir'), i.e. the entire contents is "not-ignored" recursively, regardless of ignore patterns that match the contents directly.

In prep_exclude, skip recursing into a directory only if it is really ignored (i.e. the ignore pattern is not negated).

Signed-off-by Karsten Blees <[email protected]>

---

Also available here:
https://github.com/kblees/git/tree/kb/ignore-within-not-ignored-dir
git pull git://github.com/kblees/git.git kb/ignore-within-not-ignored-dir

 dir.c                              |  3 +++
 t/t3001-ls-files-others-exclude.sh | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+)
dir.c +3 -0
index a5926fb..13858fe 100644
@@ -821,6 +821,9 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 				dir->basebuf, stk->baselen - 1,
 				dir->basebuf + current, &dt);
 			dir->basebuf[stk->baselen - 1] = '/';
+			if (dir->exclude &&
+			    dir->exclude->flags & EXC_FLAG_NEGATIVE)
+				dir->exclude = NULL;
 			if (dir->exclude) {
 				dir->basebuf[stk->baselen] = 0;
 				dir->exclude_stack = stk;
t/t3001-ls-files-others-exclude.sh +18 -0
index 4e3735f..f0421c0 100755
@@ -175,6 +175,24 @@ test_expect_success 'negated exclude matches can override previous ones' '
 	grep "^a.1" output
 '
 
+test_expect_success 'excluded directory overrides content patterns' '
+
+	git ls-files --others --exclude="one" --exclude="!one/a.1" >output &&
+	if grep "^one/a.1" output
+	then
+		false
+	fi
+'
+
+test_expect_success 'negated directory doesn'\''t affect content patterns' '
+
+	git ls-files --others --exclude="!one" --exclude="one/a.1" >output &&
+	if grep "^one/a.1" output
+	then
+		false
+	fi
+'
+
 test_expect_success 'subdirectory ignore (setup)' '
 	mkdir -p top/l1/l2 &&
 	(
Signature · 1 line
1.8.3.8097.gdc25f02.dirty
Reviewed-by on v1 1/1
#5 lore raw

Re: [PATCH] dir.c: fix ignore processing within not-ignored directories

Quoted 7 lines · Karsten Blees

As of 95c6f271 "dir.c: unify is_excluded and is_path_excluded APIs", the is_excluded API no longer recurses into directories that match an ignore pattern, and returns the directory's ignored state for all contained paths.

This is OK for normal ignore patterns, i.e. ignoring a directory affects the entire contents recursively.

Quoted 8 lines

Unfortunately, this also "works" for negated ignore patterns ('!dir'), i.e. the entire contents is "not-ignored" recursively, regardless of ignore patterns that match the contents directly.

In prep_exclude, skip recursing into a directory only if it is really ignored (i.e. the ignore pattern is not negated).

Signed-off-by Karsten Blees <[email protected]>

I think I've got a hang on the "unify" patch now.

Reviewed-by Duy Nguyen <[email protected]>

t/t3001-ls-files-others-exclude.sh test_expect_success 'negated exclude matches can override previous ones' ' · line 181

Quoted 5 lines
t/t3001-ls-files-others-exclude.sh +4 -0
+test_expect_success 'excluded directory overrides content patterns' '
+
+       git ls-files --others --exclude="one" --exclude="!one/a.1" >output &&
+       if grep "^one/a.1" output

Actually I think this is a shortcoming of gitignore. You ask to "exclude one except one/a.1" and one/a.1 should show up. '!' is designed from day one to deal with the other way around ("include one except one/a.1"). And it's arguable (and it was in the mail archive) that if you already exclude "one", we should never ever descend there to pick up "!a.1" from one/.gitignore. But we should do it with already collected patterns, at least if we detect there are negated patterns following the pattern that excludes a directory, e.g. !one/a.1 or even !*.c. For the latter case, the user can always move "!*.c" up before "one" if they don't want git to misinterpret and descend in every excluded directory.

t/t3001-ls-files-others-exclude.sh test_expect_success 'negated exclude matches can override previous ones' ' · line 185

Quoted 4 lines
t/t3001-ls-files-others-exclude.sh +4 -0
+       then
+               false
+       fi
+'

Nit pick, maybe this instead?

test_must_fail grep "^one/a.1" output

t/t3001-ls-files-others-exclude.sh test_expect_success 'negated exclude matches can override previous ones' ' · line 194

Quoted 9 lines
t/t3001-ls-files-others-exclude.sh +9 -0
+
+test_expect_success 'negated directory doesn'\''t affect content patterns' '
+
+       git ls-files --others --exclude="!one" --exclude="one/a.1" >output &&
+       if grep "^one/a.1" output
+       then
+               false
+       fi
+'

Same.
--
Duy

#6 lore raw

Re: [PATCH] dir.c: fix ignore processing within not-ignored directories

Quoted 8 lines · Duy Nguyen

t/t3001-ls-files-others-exclude.sh test_expect_success 'negated exclude matches can override previous ones' ' · line 185

Quoted 4 lines
t/t3001-ls-files-others-exclude.sh +4 -0
+       then
+               false
+       fi
+'

Nit pick, maybe this instead?

test_must_fail grep "^one/a.1" output

Neither.

	! grep "^one/a.1" output

The second bullet point in the "Don't" section of t/README may want to be updated to clarify that test_must_fail is to test Git, and we do not mean to catch segfaults by platform tools like grep.

#7 lore raw

Re: [PATCH] dir.c: fix ignore processing within not-ignored directories

Am 02.06.2013 21:25, schrieb Junio C Hamano:

Quoted 7 lines
Quoted 5 lines · Duy Nguyen
Quoted 4 lines
t/t3001-ls-files-others-exclude.sh +4 -0
+       then
+               false
+       fi
+'
Quoted 8 lines
Quoted 3 lines

Nit pick, maybe this instead?

test_must_fail grep "^one/a.1" output

Neither.

	! grep "^one/a.1" output

Nice. I actually tried "!" but without the space - which didn't work so I took the other t3001 tests as example...

#8 lore raw

PATCH 1/1 t/README: test_must_fail is for testing Git

In reply to Junio C Hamano

When a test wants to make sure there is no <string> in an output file, we should just say "! grep string output"; "test_must_fail" is there only to test Git command and catch unusual deaths we know about (e.g. segv) as an error, not as an expected failure.

Signed-off-by Junio C Hamano <[email protected]>
---
 t/README | 3 +++
 1 file changed, 3 insertions(+)
t/README +3 -0
index e669bb3..35b3c5c 100644
@@ -324,6 +324,9 @@ Don't:
    use 'test_must_fail git cmd'.  This will signal a failure if git
    dies in an unexpected way (e.g. segfault).
 
+   On the other hand, don't use test_must_fail for running regular
+   platform commands; just use '! cmd'.
+
  - use perl without spelling it as "$PERL_PATH". This is to help our
    friends on Windows where the platform Perl often adds CR before
    the end of line, and they bundle Git with a version of Perl that
#9 lore raw

Re: [PATCH] t/README: test_must_fail is for testing Git

From: "Junio C Hamano" <[email protected]>
Sent: Tuesday, June 04, 2013 5:50 PM

Quoted 2 lines

When a test wants to make sure there is no <string> in an output file, we should just say "! grep string output";

Small nit: It took me two readings of the commit message to correctly parse this break point. The flowing together of the two parts with the semicolon fooled me. Separate them?

Quoted 14 lines

"test_must_fail"
is there only to test Git command and catch unusual deaths we know about (e.g. segv) as an error, not as an expected failure.

Signed-off-by Junio C Hamano <[email protected]>

---
t/README | 3 +++

1 file changed, 3 insertions(+)
t/README +0 -0
index e669bb3..35b3c5c 100644

t/README Don't:

Quoted 10 lines
t/README +3 -1
@@ -324,6 +324,9 @@ Don't:
   use 'test_must_fail git cmd'.  This will signal a failure if git
   dies in an unexpected way (e.g. segfault).
 
+   On the other hand, don't use test_must_fail for running regular
+   platform commands; just use '! cmd'.
+
 - use perl without spelling it as "$PERL_PATH". This is to help our
   friends on Windows where the platform Perl often adds CR before
   the end of line, and they bundle Git with a version of Perl that
--

Philip

#10 lore raw

Re: [PATCH] t/README: test_must_fail is for testing Git

Quoted 12 lines · Philip Oakley

From: "Junio C Hamano" <[email protected]>
Sent: Tuesday, June 04, 2013 5:50 PM

Quoted 2 lines

When a test wants to make sure there is no <string> in an output file, we should just say "! grep string output";

Small nit: It took me two readings of the commit message to correctly parse this break point. The flowing together of the two parts with the semicolon fooled me. Separate them?

Quoted 3 lines

"test_must_fail"
is there only to test Git command and catch unusual deaths we know about (e.g. segv) as an error, not as an expected failure.

Thanks. Does this read better?

t/README: test_must_fail is for testing Git

When a test wants to make sure there is no <string> in an output file, we should just say "! grep string output".

"test_must_fail" is there only to test Git command and catch unusual deaths we know about (e.g. segv) as an error, not as an expected failure. "test_must_fail grep string output" is unnecessary, as we are not making sure the system binaries do not dump core or anything like that.

Signed-off-by Junio C Hamano <[email protected]>
#11 lore raw

Re: [PATCH] t/README: test_must_fail is for testing Git

From: "Junio C Hamano" <[email protected]>
Sent: Tuesday, June 04, 2013 9:49 PM

Quoted 11 lines
Quoted 9 lines · Philip Oakley

From: "Junio C Hamano" <[email protected]>
Sent: Tuesday, June 04, 2013 5:50 PM

Quoted 2 lines

When a test wants to make sure there is no <string> in an output file, we should just say "! grep string output";

Small nit: It took me two readings of the commit message to correctly parse this break point. The flowing together of the two parts with the semicolon fooled me. Separate them?

Quoted 5 lines
Quoted 3 lines
Quoted 3 lines

"test_must_fail"
is there only to test Git command and catch unusual deaths we know about (e.g. segv) as an error, not as an expected failure.

Thanks. Does this read better?

Yes. Thanks.

Quoted 13 lines

t/README: test_must_fail is for testing Git

When a test wants to make sure there is no <string> in an output file, we should just say "! grep string output".

"test_must_fail" is there only to test Git command and catch
unusual
deaths we know about (e.g. segv) as an error, not as an expected failure. "test_must_fail grep string output" is unnecessary, as we are not making sure the system binaries do not dump core or anything like that.

Quoted 2 lines
Signed-off-by Junio C Hamano <[email protected]>

--

Philip

#12 lore raw
In reply to Karsten Blees
Quoted 1 line · Karsten Blees <karsten.blees <at> gmail.com> writes:

<at> Øystein: in the meantime, could you check if this fixes the problem

for you?

dir.c

Quoted 7 lines

--- 8< ---

dir.c +0 -0
index a5926fb..13858fe 100644
 <at>  <at>  -821,6 +821,9  <at>  <at>  static void prep_exclude(struct

dir_struct *dir, const char *base, int baselen)

dir.c static void prep_exclude(struct dir_struct *dir, const char *base, int baselen) · line 829

Quoted 9 lines
dir.c +3 -0
 				dir->basebuf, stk->baselen - 1,
 				dir->basebuf + current, &dt);
 			dir->basebuf[stk->baselen - 1] = '/';
+			if (dir->exclude &&
+			    dir->exclude->flags & EXC_FLAG_NEGATIVE)
+				dir->exclude = NULL;
 			if (dir->exclude) {
 				dir->basebuf[stk->baselen] = 0;
 				dir->exclude_stack = stk;

Hi, Karsten

I applied your fix on v1.8.3 on both systems I mentioned earlier and from my tests the issue I reported is fixed.

Thank you very much! :)

Regards
Øsse

#13 lore raw
In reply to Duy Nguyen
Quoted 11 lines · Duy Nguyen
Quoted 9 lines · Duy Nguyen

The changes in this area since 1.8.2.3 seem to be Karsten's (I'm not blaming, just wanted to narrow down the problem). The patterns of interest seem to be

!/bin
/bin/*
!/bin/brew

Without "!/bin" v1.8.3 seems to behave the same as v1.8.2.3.

Quoted 9 lines

Karsten, the block "/* Abort if the directory is excluded */" in prep_exclude() seems to cause this. I think it goes through the exclude patterns, hits "!/bin", believes the patterns do not make sense in this context and throws all away. I think Øystein's case falls into the same path. Commenting out the block seems to gain the old behavior back (and probably breaks other stuff). Contrary to what Junio said, I'm clueless about this. I wanted to read your series through and eventually gave up. I think I now have the motivation to look at it again this weekend.

The very first patch in the da/darwin series in "next" is one possible fix.

See 29de20504e9790785fe1698300755323f74972aa

Makefile: fix default regex settings on Darwin

t0070-fundamental.sh fails on Mac OS X 10.8:

$ uname -a
Darwin lustrous 12.2.0 Darwin Kernel Version 12.2.0:
Sat Aug 25 00:48:52 PDT 2012; root:xnu-2050.18.24~1/RELEASE_X86_64 x86_64

$ ./t0070-fundamental.sh -v
fatal: regex bug confirmed: re-build git with NO_REGEX=1

Fix it by using Git's regex library.

Does that patch also fix this issue?

Karsten, you mentioned that compiling without regex support fixes it for you. Does the above commit in git.git's "next" branch fix it too?

If so, I think it would be worth merging this early part into a "maint" release. --
David

#14 lore raw
In reply to Misty De Meo
Quoted 33 lines · Misty De Meo <misty <at> brew.sh> writes:

Hi,

Gitignore parsing no longer seems to work properly in git 1.8.3.

One of my repositories has the following gitignore:

/*
!/.gitignore
!/Library/
!/CONTRIBUTING.md
!/README.md
!/SUPPORTERS.md
!/bin
/bin/*
!/bin/brew
!/share/man/man1/brew.1
.DS_Store
/Library/LinkedKegs
/Library/PinnedKegs
/Library/Taps
/Library/Formula/.gitignore

In 1.8.2.3 and earlier, this works as expected. However, in 1.8.3 I'm seeing every file in /bin/ being marked as an untracked file.

I asked about this in #git, and was told that the culprit was the regex support; apparently recompiling without regex support fixes the specific gitignore issue. However, this doesn't seem to have been reported anywhere on the mailing list that I can see. I was also told that the issue is OS X-specific, and doesn't happen on other platforms.

Quoted 3 lines

Thanks,
Misty De Meo

I see a similar problem using e.g. the following .gitignore to exclude everything except C source files and header files:

    *
    !*/
    !*.c
    !*.h

In Git 1.8.3 'git status' will show other files as untracked while in Git 1.8.2.3 I don't have that problem. I bisected to find that the offending commit is v1.8.2.1-402-g95c6f27.

I am not on OSX, however, but on Linux (Ubuntu 12.04 and RHEL 5.8) so this may be a separate issue. I've also gotten the impression that this is intentional. In any case I cannot create a .gitignore that achieves the same for both older and newer versions of Git.

Best regards,
Øystein Walle

#15 lore raw
Quoted 47 lines · Øystein Walle
Quoted 33 lines · Misty De Meo <misty <at> brew.sh> writes:

Hi,

Gitignore parsing no longer seems to work properly in git 1.8.3.

One of my repositories has the following gitignore:

/*
!/.gitignore
!/Library/
!/CONTRIBUTING.md
!/README.md
!/SUPPORTERS.md
!/bin
/bin/*
!/bin/brew
!/share/man/man1/brew.1
.DS_Store
/Library/LinkedKegs
/Library/PinnedKegs
/Library/Taps
/Library/Formula/.gitignore

In 1.8.2.3 and earlier, this works as expected. However, in 1.8.3 I'm seeing every file in /bin/ being marked as an untracked file.

I asked about this in #git, and was told that the culprit was the regex support; apparently recompiling without regex support fixes the specific gitignore issue. However, this doesn't seem to have been reported anywhere on the mailing list that I can see. I was also told that the issue is OS X-specific, and doesn't happen on other platforms.

Quoted 3 lines

Thanks,
Misty De Meo

I see a similar problem using e.g. the following .gitignore to exclude everything except C source files and header files:

    *
    !*/
    !*.c
    !*.h
Quoted 8 lines

In Git 1.8.3 'git status' will show other files as untracked while in Git 1.8.2.3 I don't have that problem. I bisected to find that the offending commit is v1.8.2.1-402-g95c6f27.

I am not on OSX, however, but on Linux (Ubuntu 12.04 and RHEL 5.8) so this may be a separate issue. I've also gotten the impression that this is intentional. In any case I cannot create a .gitignore that achieves the same for both older and newer versions of Git.

Thanks; 95c6f27 is from Karsten and I think Duy is the most clueful one in this area in the discussion that had this change; I am forwarding this message to them.

Cf. http://thread.gmane.org/gmane.comp.version-control.git/218440/focus=221289