Git 2.7.0 gitignore behaviour regression

Git 2.7.0 gitignore behaviour regression

[Mike McQuaid]

Hi folks,

Firstly, thanks for all your hard work on Git. It makes my life much easier.

Homebrew has a series of convoluted .gitignore rules due to our special/weird use-case of wanting to ignore everything in a working directory except a select few files/directories. We experienced a bug with our .gitignore file for users using Git 2.7.0. This may well be a valid WONTFIX or intentional behaviour change but I wanted to flag it in case it wasn’t.

Here’s a minimal test case:

- Create an empty git repository in a directory with `git init`
- Create a directory named ‘a' containing a file named ‘b' with `mkdir a && touch a/b`
- Create a ‘gitignore’ file with the following contents:
```
*/
/a
!/a/*
```
- Run `git status --short`.

The output with Git 2.6.4 is:
```
?? .gitignore
```

The output with Git 2.7.0 is:
```
?? .gitignore
?? a/
```

Another minimal test case:

- Create an empty git repository in a directory with `git init`
- Create a directory named ‘a' containing a file named ‘b' with `mkdir a && touch a/b`
- Create a ‘gitignore’ file with the following contents:
```
*/
/a
!/a/
```
- Run `git status —short`.

The output with Git 2.6.4 is:
```
?? .gitignore
?? a/
```

The output with Git 2.7.0 is:
```
?? .gitignore
```

Let me know if you need any more information, thanks!

Mike McQuaid
http://mikemcquaid.com

Re: Git 2.7.0 gitignore behaviour regression

[Jeff King]

On Tue, Jan 05, 2016 at 02:40:16PM +0000, Mike McQuaid wrote:

> Homebrew has a series of convoluted .gitignore rules due to our
> special/weird use-case of wanting to ignore everything in a working
> directory except a select few files/directories. We experienced a bug
> with our .gitignore file for users using Git 2.7.0. This may well be a
> valid WONTFIX or intentional behaviour change but I wanted to flag it
> in case it wasn’t.
> 
> Here’s a minimal test case:
> 
> - Create an empty git repository in a directory with `git init`
> - Create a directory named ‘a' containing a file named ‘b' with `mkdir a && touch a/b`
> - Create a ‘gitignore’ file with the following contents:
> ```
> */
> /a
> !/a/*
> ```
> - Run `git status --short`.
> 
> The output with Git 2.6.4 is:
> ```
> ?? .gitignore
> ```
> 
> The output with Git 2.7.0 is:
> ```
> ?? .gitignore
> ?? a/
> ```

Thanks for giving a clear example. This bisects to Duy's 57534ee (dir.c:
don't exclude whole dir prematurely if neg pattern may match,
2015-09-21). AFAICT (and I don't recall looking over this patch
previously), what you are seeing is the intended effect of the patch.

Your final line unignores stuff inside of "a", so we're reporting it (if you gave
"-uall", you'd see the actual file "a/b"). Older versions of git
generally optimized out looking inside "a/" at all. This created a
hassle when people wanted to do things like:

    a/
    !a/precious-file

in their .gitignore.

I'm sympathetic that in making that use-case work, we might have
regressed another one, but it's hard to tell from the small example. Can
you elaborate on your use case? Why are you both ignoring and unignoring
everything in the directory?

-Peff

Re: Git 2.7.0 gitignore behaviour regression

[Duy Nguyen]

On Tue, Jan 5, 2016 at 10:06 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jan 05, 2016 at 02:40:16PM +0000, Mike McQuaid wrote:
>
>> Homebrew has a series of convoluted .gitignore rules due to our
>> special/weird use-case of wanting to ignore everything in a working
>> directory except a select few files/directories. We experienced a bug
>> with our .gitignore file for users using Git 2.7.0. This may well be a
>> valid WONTFIX or intentional behaviour change but I wanted to flag it
>> in case it wasn’t.
>>
>> Here’s a minimal test case:
>>
>> - Create an empty git repository in a directory with `git init`
>> - Create a directory named ‘a' containing a file named ‘b' with `mkdir a && touch a/b`
>> - Create a ‘gitignore’ file with the following contents:
>> ```
>> */
>> /a
>> !/a/*
>> ```
>> - Run `git status --short`.
>>
>> The output with Git 2.6.4 is:
>> ```
>> ?? .gitignore
>> ```
>>
>> The output with Git 2.7.0 is:
>> ```
>> ?? .gitignore
>> ?? a/
>> ```
>
> Thanks for giving a clear example. This bisects to Duy's 57534ee (dir.c:
> don't exclude whole dir prematurely if neg pattern may match,
> 2015-09-21). AFAICT (and I don't recall looking over this patch
> previously), what you are seeing is the intended effect of the patch.

Yeah.. I think it's the only relevant commit in 2.7.0 cycle anyway.
These patterns "/a" followed by "!/a/*" were wrecking my head. But I
finally decided 2.7 output makes more sense. You asked to un-ignore
everything inside 'a' so we can't treat 'a' as (entirely) ignored and
hide it away.

> I'm sympathetic that in making that use-case work, we might have
> regressed another one, but it's hard to tell from the small example. Can
> you elaborate on your use case? Why are you both ignoring and unignoring
> everything in the directory?

Also how bad this affects you (widely-used 'wrong' behavior can become
'right', and my change a regression as a result)
-- 
Duy

Re: Git 2.7.0 gitignore behaviour regression

[Mike McQuaid]

> On 6 Jan 2016, at 09:42, Duy Nguyen <pclouds@gmail.com> wrote:
> 
> Yeah.. I think it's the only relevant commit in 2.7.0 cycle anyway.
> These patterns "/a" followed by "!/a/*" were wrecking my head. But I
> finally decided 2.7 output makes more sense. You asked to un-ignore
> everything inside 'a' so we can't treat 'a' as (entirely) ignored and
> hide it away.
> 
>> I'm sympathetic that in making that use-case work, we might have
>> regressed another one, but it's hard to tell from the small example. Can
>> you elaborate on your use case? Why are you both ignoring and unignoring
>> everything in the directory?
> 
> Also how bad this affects you (widely-used 'wrong' behavior can become
> 'right', and my change a regression as a result)

This doesn’t affect me badly; I was able to work around the original issue before the bug report in a way that was consistent between Git 2.6 and Git 2.7 but wanted to ensure that I filed something upstream just so it was a known issue as it was relatively easy to reproduce.

I agree that all the pattern handling stuff like in my example is pretty awful; it’s also a big area where libgit2 was inconsistent with Git’s behaviour on either of those versions too. I’ve played around and now got a .gitignore file that behaves consistently across Git 2.6, Git 2.7 and libgit2 0.23.4 (https://github.com/Homebrew/homebrew/blob/master/.gitignore) so there’s no outstanding issue on my side.

Thanks!

Mike McQuaid
http://mikemcquaid.com

Re: Git 2.7.0 gitignore behaviour regression

[Duy Nguyen]

On Wed, Jan 6, 2016 at 4:50 PM, Mike McQuaid <mike@mikemcquaid.com> wrote:
> it’s also a big area where libgit2 was inconsistent with Git’s behaviour on either of those versions too.

Yeah.. it looks like libgit2's gitignore support was written new, not
imported from C Git, so behavior differences (especially in corner
cases) and even lack of some feature ("**" support comes to mind). For
isolated features like gitignore, perhaps we can have an option to
replace C Git code with libgit2 and therefore can test libgit2 against
C Git test suite. It could be a good start for libgit2 to invade C
Git. Not sure if anybody's interested in doing it though.
-- 
Duy

Re: Git 2.7.0 gitignore behaviour regression

[Junio C Hamano]

On Wed, Jan 6, 2016 at 2:03 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Wed, Jan 6, 2016 at 4:50 PM, Mike McQuaid <mike@mikemcquaid.com> wrote:
> > it’s also a big area where libgit2 was inconsistent with Git’s behaviour on either of those versions too.
>
> Yeah.. it looks like libgit2's gitignore support was written new, not
> imported from C Git, so behavior differences (especially in corner
> cases) and even lack of some feature ("**" support comes to mind). For
> isolated features like gitignore, perhaps we can have an option to
> replace C Git code with libgit2 and therefore can test libgit2 against
> C Git test suite. It could be a good start for libgit2 to invade C
> Git. Not sure if anybody's interested in doing it though.

Yup, an area that is reasonably isolated from the remainder of the system like
this might be a good starting point. But I suspect that the invasion needs to
happen in the opposite direction in this particular case before it happens.
That is, if libgit2's implementation does not behave like how we do, it needs to
be fixed, possibly by discarding what they did and instead importing code from
us. After the behaviour of libgit2 is fixed, we can talk about the
invasion in the
opposite direction.

Thanks.

Re: Git 2.7.0 gitignore behaviour regression

[Jeff King]

[+cc Carlos and Shawn for libgit2/JGit talk]

On Wed, Jan 06, 2016 at 10:58:37AM -0800, Junio C Hamano wrote:

> On Wed, Jan 6, 2016 at 2:03 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> >
> > Yeah.. it looks like libgit2's gitignore support was written new, not
> > imported from C Git, so behavior differences (especially in corner
> > cases) and even lack of some feature ("**" support comes to mind). For
> > isolated features like gitignore, perhaps we can have an option to
> > replace C Git code with libgit2 and therefore can test libgit2 against
> > C Git test suite. It could be a good start for libgit2 to invade C
> > Git. Not sure if anybody's interested in doing it though.

Yeah, libgit2 is in the difficult position of trying to hit a moving
target. There's a good chance that it _was_ the same as git's behavior
when it was written. :)

JGit is in the same boat, and I wouldn't be surprised if they don't
handle "**" either (I didn't check). Note that git inherited that
feature (and probably some others) by importing a GPLv2 version of
wildmatch. That certainly isn't an option for JGit (because it's not
pure Java), and probably not for libgit2 (which would need the wildmatch
authors to grant the linking exception).

> Yup, an area that is reasonably isolated from the remainder of the system like
> this might be a good starting point. But I suspect that the invasion needs to
> happen in the opposite direction in this particular case before it happens.
> That is, if libgit2's implementation does not behave like how we do, it needs to
> be fixed, possibly by discarding what they did and instead importing code from
> us. After the behaviour of libgit2 is fixed, we can talk about the
> invasion in the
> opposite direction.

Unfortunately, "importing code from us" is not so easy. :(

They'll either need to contact the wildmatch authors, or rewrite
wildmatch from scratch.

-Peff

Re: Git 2.7.0 gitignore behaviour regression

[Duy Nguyen]

On Thu, Jan 7, 2016 at 9:04 AM, Jeff King <peff@peff.net> wrote:
>> Yup, an area that is reasonably isolated from the remainder of the system like
>> this might be a good starting point. But I suspect that the invasion needs to
>> happen in the opposite direction in this particular case before it happens.
>> That is, if libgit2's implementation does not behave like how we do, it needs to
>> be fixed, possibly by discarding what they did and instead importing code from
>> us. After the behaviour of libgit2 is fixed, we can talk about the
>> invasion in the
>> opposite direction.
>
> Unfortunately, "importing code from us" is not so easy. :(
>
> They'll either need to contact the wildmatch authors, or rewrite
> wildmatch from scratch.

wildmatch author relicensed it on request before [1] so he might do it
again (hard to say). I'm not sure if there are other authors though.
But even with wildmatch good to go, there still a lot of work to do.

[1] http://thread.gmane.org/gmane.comp.version-control.git/264312/focus=264328
-- 
Duy

Re: Git 2.7.0 gitignore behaviour regression

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1329 bytes --]

On Tue, Jan 05, 2016 at 10:06:02AM -0500, Jeff King wrote:
> Thanks for giving a clear example. This bisects to Duy's 57534ee (dir.c:
> don't exclude whole dir prematurely if neg pattern may match,
> 2015-09-21). AFAICT (and I don't recall looking over this patch
> previously), what you are seeing is the intended effect of the patch.
> 
> Your final line unignores stuff inside of "a", so we're reporting it (if you gave
> "-uall", you'd see the actual file "a/b"). Older versions of git
> generally optimized out looking inside "a/" at all. This created a
> hassle when people wanted to do things like:
> 
>     a/
>     !a/precious-file
> 
> in their .gitignore.

I think there's still a bug in the code here.  If you do

  git init
  mkdir -p base/a/
  printf 'base/a/\n!base/a/b.txt\n' >.gitignore
  git add .gitignore
  git commit -m 'Add .gitignore'
  >base/a/b.txt
  git add base/a/b.txt
  git commit -m 'Add base/a/b.txt'
  >base/a/c.txt
  git status --porcelain

git status outputs base/a/c.txt as unknown, when it should be ignored.
We saw this in a repository at $DAYJOB.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Git 2.7.0 gitignore behaviour regression

[Duy Nguyen]

On Fri, Jan 8, 2016 at 6:44 AM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> On Tue, Jan 05, 2016 at 10:06:02AM -0500, Jeff King wrote:
>> Thanks for giving a clear example. This bisects to Duy's 57534ee (dir.c:
>> don't exclude whole dir prematurely if neg pattern may match,
>> 2015-09-21). AFAICT (and I don't recall looking over this patch
>> previously), what you are seeing is the intended effect of the patch.
>>
>> Your final line unignores stuff inside of "a", so we're reporting it (if you gave
>> "-uall", you'd see the actual file "a/b"). Older versions of git
>> generally optimized out looking inside "a/" at all. This created a
>> hassle when people wanted to do things like:
>>
>>     a/
>>     !a/precious-file
>>
>> in their .gitignore.
>
> I think there's still a bug in the code here.  If you do
>
>   git init
>   mkdir -p base/a/
>   printf 'base/a/\n!base/a/b.txt\n' >.gitignore

Here we have the ignore rule "base/a/", but gitignore.txt, section
NOTES mentions this

 - The rules to exclude the parent directory must not end with a
   trailing slash.

>   git add .gitignore
>   git commit -m 'Add .gitignore'
>   >base/a/b.txt
>   git add base/a/b.txt
>   git commit -m 'Add base/a/b.txt'
>   >base/a/c.txt
>   git status --porcelain
>
> git status outputs base/a/c.txt as unknown, when it should be ignored.
> We saw this in a repository at $DAYJOB.

If I delete that trailing slash, c.txt is ignored. So it's known
limitation. I think we can make trailing slash case work too, but if I
remember correctly it would involve a lot more changes, so I didn't do
it (there are other conditions to follow anyway to make it work).
-- 
Duy

Re: Git 2.7.0 gitignore behaviour regression

[Carlos Martín Nieto]

On 07 Jan 2016, at 03:04, Jeff King <peff@peff.net> wrote:

> [+cc Carlos and Shawn for libgit2/JGit talk]
> 
> On Wed, Jan 06, 2016 at 10:58:37AM -0800, Junio C Hamano wrote:
> 
>> On Wed, Jan 6, 2016 at 2:03 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>>> 
>>> Yeah.. it looks like libgit2's gitignore support was written new, not
>>> imported from C Git, so behavior differences (especially in corner
>>> cases) and even lack of some feature ("**" support comes to mind). For
>>> isolated features like gitignore, perhaps we can have an option to
>>> replace C Git code with libgit2 and therefore can test libgit2 against
>>> C Git test suite. It could be a good start for libgit2 to invade C
>>> Git. Not sure if anybody's interested in doing it though.
> 
> Yeah, libgit2 is in the difficult position of trying to hit a moving
> target. There's a good chance that it _was_ the same as git's behavior
> when it was written. :)

We did try to match the rules which git followed at the time when we added the functionality, but sometimes small changes in git mean large changes for us. There’s a couple of recent issues complaining that we don’t match git where *git* doesn’t quite seem to match the rules as I know them.

I don’t recall the details of how we brought ignore support in, but when we don’t just copy the code from git, it’s generally because it’s embedded into the rest of the code, or in some other way doesn’t make a lot of sense in a library API.

We do (at least I do) want to test libgit2 more against the git test suite; this either involves porting it to our test suite (generally non-trivial since we can’t use shell, making it harder to compare) or creating a command which pretends to be git but uses libgit2. The latter is what I think we’ll have to do, as we keep hitting complex pieces which git keeps changing like crlf and ignore handling. There’s been plans to do this, but it’s never been the top priority.

> 
> JGit is in the same boat, and I wouldn't be surprised if they don't
> handle "**" either (I didn't check). Note that git inherited that
> feature (and probably some others) by importing a GPLv2 version of
> wildmatch. That certainly isn't an option for JGit (because it's not
> pure Java), and probably not for libgit2 (which would need the wildmatch
> authors to grant the linking exception).
> 
>> Yup, an area that is reasonably isolated from the remainder of the system like
>> this might be a good starting point. But I suspect that the invasion needs to
>> happen in the opposite direction in this particular case before it happens.
>> That is, if libgit2's implementation does not behave like how we do, it needs to
>> be fixed, possibly by discarding what they did and instead importing code from
>> us. After the behaviour of libgit2 is fixed, we can talk about the
>> invasion in the
>> opposite direction.
> 
> Unfortunately, "importing code from us" is not so easy. :(
> 
> They'll either need to contact the wildmatch authors, or rewrite
> wildmatch from scratch.

Yeah, we currently use the fnmatch code from Android IIRC. I’m not looking forward to writing a variant of that for double-star. If the wildmatch authors are receptive to allowing the linking exception, that would be ideal.

   cmn

Re: Git 2.7.0 gitignore behaviour regression

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2123 bytes --]

On Fri, Jan 08, 2016 at 07:38:58AM +0700, Duy Nguyen wrote:
> On Fri, Jan 8, 2016 at 6:44 AM, brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > I think there's still a bug in the code here.  If you do
> >
> >   git init
> >   mkdir -p base/a/
> >   printf 'base/a/\n!base/a/b.txt\n' >.gitignore
> 
> Here we have the ignore rule "base/a/", but gitignore.txt, section
> NOTES mentions this
> 
>  - The rules to exclude the parent directory must not end with a
>    trailing slash.

The text here says, "To re-include files or directories when their
parent directory is excluded, the following conditions must be met".  In
other words, the text implies that it's required for re-inclusion to
work, not exclusion.

> >   git add .gitignore
> >   git commit -m 'Add .gitignore'
> >   >base/a/b.txt
> >   git add base/a/b.txt
> >   git commit -m 'Add base/a/b.txt'
> >   >base/a/c.txt
> >   git status --porcelain
> >
> > git status outputs base/a/c.txt as unknown, when it should be ignored.
> > We saw this in a repository at $DAYJOB.
> 
> If I delete that trailing slash, c.txt is ignored. So it's known
> limitation. I think we can make trailing slash case work too, but if I
> remember correctly it would involve a lot more changes, so I didn't do
> it (there are other conditions to follow anyway to make it work).

The case I'm seeing is that b.txt was already checked into the
repository before being re-added, and c.txt was not.  So it didn't
affect us that b.txt was ignored (as it was already in the repo), but
c.txt not being ignored broke a whole bunch of scripts that checked that
the repository was clean, simply because we upgraded Git.

I think regardless of whether b.txt is re-included, c.txt should be
ignored.  If it isn't possible to re-include b.txt, that's fine, since
that isn't a regression, but ignored files should remain ignored.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Git 2.7.0 gitignore behaviour regression

[Duy Nguyen]

On Fri, Jan 08, 2016 at 02:41:25AM +0000, brian m. carlson wrote:
> On Fri, Jan 08, 2016 at 07:38:58AM +0700, Duy Nguyen wrote:
> > On Fri, Jan 8, 2016 at 6:44 AM, brian m. carlson
> > <sandals@crustytoothpaste.net> wrote:
> > > I think there's still a bug in the code here.  If you do
> > >
> > >   git init
> > >   mkdir -p base/a/
> > >   printf 'base/a/\n!base/a/b.txt\n' >.gitignore
> > 
> > Here we have the ignore rule "base/a/", but gitignore.txt, section
> > NOTES mentions this
> > 
> >  - The rules to exclude the parent directory must not end with a
> >    trailing slash.
> 
> The text here says, "To re-include files or directories when their
> parent directory is excluded, the following conditions must be met".  In
> other words, the text implies that it's required for re-inclusion to
> work, not exclusion.
> 
> > >   git add .gitignore
> > >   git commit -m 'Add .gitignore'
> > >   >base/a/b.txt
> > >   git add base/a/b.txt
> > >   git commit -m 'Add base/a/b.txt'
> > >   >base/a/c.txt
> > >   git status --porcelain
> > >
> > > git status outputs base/a/c.txt as unknown, when it should be ignored.
> > > We saw this in a repository at $DAYJOB.
> > 
> > If I delete that trailing slash, c.txt is ignored. So it's known
> > limitation. I think we can make trailing slash case work too, but if I
> > remember correctly it would involve a lot more changes, so I didn't do
> > it (there are other conditions to follow anyway to make it work).
> 
> The case I'm seeing is that b.txt was already checked into the
> repository before being re-added, and c.txt was not.  So it didn't
> affect us that b.txt was ignored (as it was already in the repo), but
> c.txt not being ignored broke a whole bunch of scripts that checked that
> the repository was clean, simply because we upgraded Git.
> 
> I think regardless of whether b.txt is re-included, c.txt should be
> ignored.  If it isn't possible to re-include b.txt, that's fine, since
> that isn't a regression, but ignored files should remain ignored.

Thanks for clarification. I looked at this the wrong way. I agree it
is a regression.

The following should fix it. It looks correct (and does fix your test
case), but I will have to look harder over the weekend before sending
a proper patch.

-- 8< --
diff --git a/dir.c b/dir.c
index d2a8f06..7934e87 100644
--- a/dir.c
+++ b/dir.c
@@ -1008,6 +1008,7 @@ static struct exclude *last_exclude_matching_from_list(const char *pathname,
 	if (exc &&
 	    !(exc->flags & EXC_FLAG_NEGATIVE) &&
 	    !(exc->flags & EXC_FLAG_NODIR) &&
+	    !(exc->flags & EXC_FLAG_MUSTBEDIR) &&
 	    matched_negative_path)
 		exc = NULL;
 	return exc;
-- 8< --
--
Duy