Change in .gitignore handling: intended or bug?

Change in .gitignore handling: intended or bug?

[Charles Strahan]

Hello,

I've found a change in the way .gitignore works, and I'm not sure if
it's a bug
or intended.

Previously, one could use the following .gitignore:

    *
    !/foo
    !/foo/bar.txt
    !/baz
    !/baz/quux
    !/baz/quux/**/*

And these files would be seen by git:

    foo/bar.txt
    baz/quux/grault.txt
    baz/quux/corge/wibble.txt

And these files would be ignored:

    foo/garply.txt
    baz/waldo.txt

At some point (between git 2.6.0 and 2.7.0, I think), the behavior
changed such
that _none_ of the files above would be ignored. Previously, git would
treat
!/foo as an indication that it should not prune /foo, but that
_wouldn't_ be
sufficient to un-ignore the contents thereof. Now, it seems the new
scheme
treats !/foo as functionally equivalent to !/foo followed by !/foo/**/*
in the
old scheme.

I manage my home directory by making it a git repo, and using
~/.gitignore to
selectively permit certain files or subdirectories to be seen by git.
The recent
change in behavior has resulted in sensitive directories like ~/.gpg
being
un-ignored. For reference, I've appended my .gitignore to the end of
this email.

So, is this behavior intended, or is this a bug? If the former, is there
an
announcement explaining this change?

-Charles

(.gitignore is as follows)

*
!/.Xdefaults
!/.Xresources
!/.ackrc
!/.bash_profile
!/.config
!/.config/dunst
!/.config/dunst/dunstrc
!/.config/taffybar
!/.config/taffybar/taffybar.hs
!/.config/taffybar/taffybar.rc
!/.ctags
!/.gdb
!/.gdb/**/*
!/.gdbinit
!/.gemrc
!/.ghci
!/.git_template
!/.git_template/**/*
!/.gitexcludes
!/.gitignore
!/.gnupg
!/.gnupg/dirmngr.conf
!/.gnupg/gpg.conf
!/.goobook/
!/.goobook/decipher
!/.goobook/decipher/goobookrc
!/.inputrc
!/.irbrc
!/.irssi
!/.irssi/**/*
!/.mbsyncrc
!/.msmtprc
!/.mutt
!/.mutt/accounts
!/.mutt/accounts/*
!/.mutt/gpg.rc
!/.mutt/mailcap
!/.mutt/muttrc
!/.mutt/signature
!/.mutt/theme/*
!/.nixpkgs
!/.nixpkgs/**/*
!/.notmuch-config
!/.pystartup
!/.tmux
!/.tmux.conf
!/.tmux/**/*
!/.vim
!/.vim/**/*
!/.vimrc
!/.xmonad
!/.xmonad/xmonad.hs
!/.xsession
!/.zlogin
!/.zprofile
!/.zsh
!/.zsh/**/*
!/.zshenv
!/.zshrc
!/osx

Re: Change in .gitignore handling: intended or bug?

[Duy Nguyen]

On Fri, Mar 4, 2016 at 9:11 AM, Charles Strahan <charles@cstrahan.com> wrote:
> Hello,
>
> I've found a change in the way .gitignore works, and I'm not sure if
> it's a bug
> or intended.

Can't look into this just yet. Quick question, what's the git version
you're currently running?
-- 
Duy

Re: Change in .gitignore handling: intended or bug?

[Kevin Daudt]

On Thu, Mar 03, 2016 at 09:11:56PM -0500, Charles Strahan wrote:
> Hello,
> 
> I've found a change in the way .gitignore works, and I'm not sure if
> it's a bug
> or intended.
> 
> Previously, one could use the following .gitignore:
> 
>     *
>     !/foo
>     !/foo/bar.txt
>     !/baz
>     !/baz/quux
>     !/baz/quux/**/*
> 
> And these files would be seen by git:
> 
>     foo/bar.txt
>     baz/quux/grault.txt
>     baz/quux/corge/wibble.txt
> 
> And these files would be ignored:
> 
>     foo/garply.txt
>     baz/waldo.txt
> 
> At some point (between git 2.6.0 and 2.7.0, I think), the behavior
> changed such
> that _none_ of the files above would be ignored. Previously, git would
> treat
> !/foo as an indication that it should not prune /foo, but that
> _wouldn't_ be
> sufficient to un-ignore the contents thereof. Now, it seems the new
> scheme
> treats !/foo as functionally equivalent to !/foo followed by !/foo/**/*
> in the
> old scheme.
> 
> I manage my home directory by making it a git repo, and using
> ~/.gitignore to
> selectively permit certain files or subdirectories to be seen by git.
> The recent
> change in behavior has resulted in sensitive directories like ~/.gpg
> being
> un-ignored. For reference, I've appended my .gitignore to the end of
> this email.
> 
> So, is this behavior intended, or is this a bug? If the former, is there
> an
> announcement explaining this change?
> 
> -Charles
> 
> [snip]
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Works as intended for me:

├── baz
│   ├── quux
│   │   ├── corge
│   │   │   └── wibble.txt
│   │   └── grault.txt
│   └── waldo.txt
└── foo
    ├── bar.txt
    └── garply.txt

$ git status -s -uall
?? baz/quux/corge/wibble.txt
?? baz/quux/grault.txt
?? foo/bar.txt

garply.txt and waldo.txt are ignore, but the rest is still tracked.

I'm on 2.7.2.

Re: Change in .gitignore handling: intended or bug?

[Charles Strahan]

I'm on 2.7.0.

Here's a quick sanity check:

├── baz
│   ├── quux
│   │   ├── corge
│   │   │   └── wibble.txt
│   │   └── grault.txt
│   └── waldo.txt
└── foo
    ├── bar.txt
    └── garply.txt

$ git --version
git version 2.7.0

$ git status -sb -uall
## Initial commit on master
?? baz/quux/corge/wibble.txt
?? baz/quux/grault.txt
?? baz/waldo.txt
?? foo/bar.txt
?? foo/garply.txt


For the lazy (such as myself), this will set up an identical tree:

mkdir -p foo
mkdir -p baz/quux/corge
touch foo/bar.txt
touch foo/garply.txt
touch baz/waldo.txt
touch baz/quux/grault.txt
touch baz/quux/corge/wibble.txt
cat <<"EOF" > .gitignore
*
!/foo
!/foo/bar.txt
!/baz
!/baz/quux
!/baz/quux/**/*
EOF


I just checked https://git-scm.com/docs/gitignore and the example at the
bottom
suggests that this behavior may be expected:

    $ cat .gitignore
    # exclude everything except directory foo/bar
    /*
    !/foo
    /foo/*
    !/foo/bar

Note the /foo/*, explicitly ignoring the entries below /foo.

This wasn't always the case, though, so I'd love to hear if it was
intentional
(or if I've lost my mind, which is quite possible).

-Charles



On Fri, Mar 4, 2016, at 12:51 AM, Kevin Daudt wrote:
> On Thu, Mar 03, 2016 at 09:11:56PM -0500, Charles Strahan wrote:
> > Hello,
> > 
> > I've found a change in the way .gitignore works, and I'm not sure if
> > it's a bug
> > or intended.
> > 
> > Previously, one could use the following .gitignore:
> > 
> >     *
> >     !/foo
> >     !/foo/bar.txt
> >     !/baz
> >     !/baz/quux
> >     !/baz/quux/**/*
> > 
> > And these files would be seen by git:
> > 
> >     foo/bar.txt
> >     baz/quux/grault.txt
> >     baz/quux/corge/wibble.txt
> > 
> > And these files would be ignored:
> > 
> >     foo/garply.txt
> >     baz/waldo.txt
> > 
> > At some point (between git 2.6.0 and 2.7.0, I think), the behavior
> > changed such
> > that _none_ of the files above would be ignored. Previously, git would
> > treat
> > !/foo as an indication that it should not prune /foo, but that
> > _wouldn't_ be
> > sufficient to un-ignore the contents thereof. Now, it seems the new
> > scheme
> > treats !/foo as functionally equivalent to !/foo followed by !/foo/**/*
> > in the
> > old scheme.
> > 
> > I manage my home directory by making it a git repo, and using
> > ~/.gitignore to
> > selectively permit certain files or subdirectories to be seen by git.
> > The recent
> > change in behavior has resulted in sensitive directories like ~/.gpg
> > being
> > un-ignored. For reference, I've appended my .gitignore to the end of
> > this email.
> > 
> > So, is this behavior intended, or is this a bug? If the former, is there
> > an
> > announcement explaining this change?
> > 
> > -Charles
> > 
> > [snip]
> > --
> > To unsubscribe from this list: send the line "unsubscribe git" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> Works as intended for me:
> 
> ├── baz
> │   ├── quux
> │   │   ├── corge
> │   │   │   └── wibble.txt
> │   │   └── grault.txt
> │   └── waldo.txt
> └── foo
>     ├── bar.txt
>     └── garply.txt
> 
> $ git status -s -uall
> ?? baz/quux/corge/wibble.txt
> ?? baz/quux/grault.txt
> ?? foo/bar.txt
> 
> garply.txt and waldo.txt are ignore, but the rest is still tracked.
> 
> I'm on 2.7.2.

Re: Change in .gitignore handling: intended or bug?

[Kevin Daudt]

On Fri, Mar 04, 2016 at 01:12:37AM -0500, Charles Strahan wrote:
> I'm on 2.7.0.
> 
> Here's a quick sanity check:
> 
> ├── baz
> │   ├── quux
> │   │   ├── corge
> │   │   │   └── wibble.txt
> │   │   └── grault.txt
> │   └── waldo.txt
> └── foo
>     ├── bar.txt
>     └── garply.txt
> 
> $ git --version
> git version 2.7.0
> 
> $ git status -sb -uall
> ## Initial commit on master
> ?? baz/quux/corge/wibble.txt
> ?? baz/quux/grault.txt
> ?? baz/waldo.txt
> ?? foo/bar.txt
> ?? foo/garply.txt
> 
> 
> For the lazy (such as myself), this will set up an identical tree:
> 
> mkdir -p foo
> mkdir -p baz/quux/corge
> touch foo/bar.txt
> touch foo/garply.txt
> touch baz/waldo.txt
> touch baz/quux/grault.txt
> touch baz/quux/corge/wibble.txt
> cat <<"EOF" > .gitignore
> *
> !/foo
> !/foo/bar.txt
> !/baz
> !/baz/quux
> !/baz/quux/**/*
> EOF
> 
> 
> I just checked https://git-scm.com/docs/gitignore and the example at the
> bottom
> suggests that this behavior may be expected:
> 
>     $ cat .gitignore
>     # exclude everything except directory foo/bar
>     /*
>     !/foo
>     /foo/*
>     !/foo/bar
> 
> Note the /foo/*, explicitly ignoring the entries below /foo.
> 
> This wasn't always the case, though, so I'd love to hear if it was
> intentional
> (or if I've lost my mind, which is quite possible).
> 
> -Charles
> 
> 
> 
> On Fri, Mar 4, 2016, at 12:51 AM, Kevin Daudt wrote:
> > On Thu, Mar 03, 2016 at 09:11:56PM -0500, Charles Strahan wrote:
> > > Hello,
> > > 
> > > I've found a change in the way .gitignore works, and I'm not sure if
> > > it's a bug
> > > or intended.
> > > 
> > > Previously, one could use the following .gitignore:
> > > 
> > >     *
> > >     !/foo
> > >     !/foo/bar.txt
> > >     !/baz
> > >     !/baz/quux
> > >     !/baz/quux/**/*
> > > 
> > > And these files would be seen by git:
> > > 
> > >     foo/bar.txt
> > >     baz/quux/grault.txt
> > >     baz/quux/corge/wibble.txt
> > > 
> > > And these files would be ignored:
> > > 
> > >     foo/garply.txt
> > >     baz/waldo.txt
> > > 
> > > At some point (between git 2.6.0 and 2.7.0, I think), the behavior
> > > changed such
> > > that _none_ of the files above would be ignored. Previously, git would
> > > treat
> > > !/foo as an indication that it should not prune /foo, but that
> > > _wouldn't_ be
> > > sufficient to un-ignore the contents thereof. Now, it seems the new
> > > scheme
> > > treats !/foo as functionally equivalent to !/foo followed by !/foo/**/*
> > > in the
> > > old scheme.
> > > 
> > > I manage my home directory by making it a git repo, and using
> > > ~/.gitignore to
> > > selectively permit certain files or subdirectories to be seen by git.
> > > The recent
> > > change in behavior has resulted in sensitive directories like ~/.gpg
> > > being
> > > un-ignored. For reference, I've appended my .gitignore to the end of
> > > this email.
> > > 
> > > So, is this behavior intended, or is this a bug? If the former, is there
> > > an
> > > announcement explaining this change?
> > > 
> > > -Charles
> > > 
> > > [snip]
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe git" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > Works as intended for me:
> > 
> > ├── baz
> > │   ├── quux
> > │   │   ├── corge
> > │   │   │   └── wibble.txt
> > │   │   └── grault.txt
> > │   └── waldo.txt
> > └── foo
> >     ├── bar.txt
> >     └── garply.txt
> > 
> > $ git status -s -uall
> > ?? baz/quux/corge/wibble.txt
> > ?? baz/quux/grault.txt
> > ?? foo/bar.txt
> > 
> > garply.txt and waldo.txt are ignore, but the rest is still tracked.
> > 
> > I'm on 2.7.2.
> --

Verified that it's different in 2.7.0, but 2.7.2 gives expected output.

Re: Change in .gitignore handling: intended or bug?

[Duy Nguyen]

On Fri, Mar 4, 2016 at 6:56 PM, Kevin Daudt <me@ikke.info> wrote:
> Verified that it's different in 2.7.0, but 2.7.2 gives expected output.

Thanks. 2.7.1 reverts the faulty commit from 2.7.0 that generated two
other regression reports before this one. I guess it's all good then
(except for the people still on 2.7.0)
-- 
Duy

Re: Change in .gitignore handling: intended or bug?

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Fri, Mar 4, 2016 at 6:56 PM, Kevin Daudt <me@ikke.info> wrote:
>> Verified that it's different in 2.7.0, but 2.7.2 gives expected output.
>
> Thanks. 2.7.1 reverts the faulty commit from 2.7.0 that generated two
> other regression reports before this one. I guess it's all good then
> (except for the people still on 2.7.0)

Are we good at 2.8.0-rc0, too?  Somehow I had an impression that we
queued "another attempt to do it differently" or something.

 ... goes and looks ...

    $ rungit maint status -suall
    ?? baz/quux/corge/wibble.txt
    ?? baz/quux/grault.txt
    ?? foo/bar.txt
    $ rungit master status -suall
    ?? baz/quux/corge/wibble.txt
    ?? baz/quux/grault.txt
    ?? baz/waldo.txt
    ?? foo/bar.txt
    ?? foo/garply.txt

Re: Change in .gitignore handling: intended or bug?

[Junio C Hamano]

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

> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Fri, Mar 4, 2016 at 6:56 PM, Kevin Daudt <me@ikke.info> wrote:
>>> Verified that it's different in 2.7.0, but 2.7.2 gives expected output.
>>
>> Thanks. 2.7.1 reverts the faulty commit from 2.7.0 that generated two
>> other regression reports before this one. I guess it's all good then
>> (except for the people still on 2.7.0)
>
> Are we good at 2.8.0-rc0, too?  Somehow I had an impression that we
> queued "another attempt to do it differently" or something.
>
>  ... goes and looks ...
>
>     $ rungit maint status -suall
>     ?? baz/quux/corge/wibble.txt
>     ?? baz/quux/grault.txt
>     ?? foo/bar.txt
>     $ rungit master status -suall
>     ?? baz/quux/corge/wibble.txt
>     ?? baz/quux/grault.txt
>     ?? baz/waldo.txt
>     ?? foo/bar.txt
>     ?? foo/garply.txt

It seems to bisect down to this one between maint..master:

commit a60ea8fb66945a886ea53fd3f41e61cc5fb3201e
Author: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Date:   Mon Feb 15 16:03:36 2016 +0700

    dir.c: fix match_pathname()
    
    Given the pattern "1/2/3/4" and the path "1/2/3/4/f", the pattern
    prefix is "1/2/3/4". We will compare and remove the prefix from both
    pattern and path and come to this code
    
        /*
         * If the whole pattern did not have a wildcard,
         * then our prefix match is all we need; we
         * do not need to call fnmatch at all.
         */
        if (!patternlen && !namelen)
                return 1;
    
    where patternlen is zero (full pattern consumed) and the remaining
    path in "name" is "/f". We fail to realize it's matched in this case
    and fall back to fnmatch(), which also fails to catch it. Fix it.
    
    Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

But I do not think this change alone is the culprit; the change
itself does make sense in the context of the series.

At this point, we have two choices.  Either we revert the merge of
the whole series:

    commit 5e57f9c3dfe7dd44a1b56bb5b3327d7a1356ec7c
    Merge: e79112d d589a67
    Author: Junio C Hamano <gitster@pobox.com>
    Date:   Wed Feb 24 13:25:59 2016 -0800

        Merge branch 'nd/exclusion-regression-fix'

        Another try to add support to the ignore mechanism that lets you
        say "this is excluded" and then later say "oh, no, this part (that
        is a subset of the previous part) is not excluded".

        * nd/exclusion-regression-fix:
          dir.c: don't exclude whole dir prematurely
          dir.c: support marking some patterns already matched
          dir.c: support tracing exclude
          dir.c: fix match_pathname()

to go back to the 2.7.2 behaviour, or add a follow-on change to the
nd/exclusion-regression-fix topic to fix what it wanted to fix
without breaking Charles's use case.  I am inclined to go for the
former (unless the follow-on fix is really trivial), but I haven't
dug into the codebase myself yet, so...

Re: Change in .gitignore handling: intended or bug?

[Duy Nguyen]

On Sat, Mar 5, 2016 at 12:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Fri, Mar 4, 2016 at 6:56 PM, Kevin Daudt <me@ikke.info> wrote:
>>> Verified that it's different in 2.7.0, but 2.7.2 gives expected output.
>>
>> Thanks. 2.7.1 reverts the faulty commit from 2.7.0 that generated two
>> other regression reports before this one. I guess it's all good then
>> (except for the people still on 2.7.0)
>
> Are we good at 2.8.0-rc0, too?  Somehow I had an impression that we
> queued "another attempt to do it differently" or something.
>
>  ... goes and looks ...
>
>     $ rungit maint status -suall
>     ?? baz/quux/corge/wibble.txt
>     ?? baz/quux/grault.txt
>     ?? foo/bar.txt
>     $ rungit master status -suall
>     ?? baz/quux/corge/wibble.txt
>     ?? baz/quux/grault.txt
>     ?? baz/waldo.txt
>     ?? foo/bar.txt
>     ?? foo/garply.txt

If you swap a60ea8f and bac65a2 so that we can have tracing even
without the problematic commit a60ea8f (dir.c: fix match_pathname() -
2016-02-15). Without a60ea8f I got

GIT_TRACE_EXCLUDE=1 ~/w/git/temp/git status -suall 2>&1 >/dev/null
|grep waldo
07:25:05.639445 dir.c:952               exclude: baz/waldo.txt vs * at
line 1 => yes

(meaning, baz/waldo.txt matches "*" rule and is decided to be excluded)

with a60ea8f

> /tmp/abc $ GIT_TRACE_EXCLUDE=1 ~/w/git/temp/git status -suall 2>&1 >/dev/null |grep waldo
07:25:24.425125 dir.c:952               exclude: baz/waldo.txt vs /baz
at line 4 => no

the decision is not taken earlier from line "!/baz" and it's decided
to be re-included. Which I would argue is the correct thing because
you ask to re-include the whole directory "baz". It should behave this
way because exclude rules without '!' behave this way.

Because positive any negative rules can be nested, by adding a rule to
reinclude what's inside baz..

    *
    !/foo
    !/foo/bar.txt
    !/baz
    /baz/*
    !/baz/quux
    !/baz/quux/**/*

you'll get baz/waldo.txt excluded.

Yes it's a behavior change. I think the old behavior is unintended.
But it's probably out there long enough that many .gitignore files may
rely on it and by making it right, I break them. Whether to revert the
series is up to you. Let me know if I should send the revert patch.
-- 
Duy

Re: Change in .gitignore handling: intended or bug?

[Duy Nguyen]

typo fixes

On Sat, Mar 5, 2016 at 7:43 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sat, Mar 5, 2016 at 12:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Duy Nguyen <pclouds@gmail.com> writes:
>>
>>> On Fri, Mar 4, 2016 at 6:56 PM, Kevin Daudt <me@ikke.info> wrote:
>>>> Verified that it's different in 2.7.0, but 2.7.2 gives expected output.
>>>
>>> Thanks. 2.7.1 reverts the faulty commit from 2.7.0 that generated two
>>> other regression reports before this one. I guess it's all good then
>>> (except for the people still on 2.7.0)
>>
>> Are we good at 2.8.0-rc0, too?  Somehow I had an impression that we
>> queued "another attempt to do it differently" or something.
>>
>>  ... goes and looks ...
>>
>>     $ rungit maint status -suall
>>     ?? baz/quux/corge/wibble.txt
>>     ?? baz/quux/grault.txt
>>     ?? foo/bar.txt
>>     $ rungit master status -suall
>>     ?? baz/quux/corge/wibble.txt
>>     ?? baz/quux/grault.txt
>>     ?? baz/waldo.txt
>>     ?? foo/bar.txt
>>     ?? foo/garply.txt
>
> If you swap a60ea8f and bac65a2 so that we can have tracing even
> without the problematic commit a60ea8f (dir.c: fix match_pathname() -
> 2016-02-15). Without a60ea8f I got
>
> GIT_TRACE_EXCLUDE=1 ~/w/git/temp/git status -suall 2>&1 >/dev/null
> |grep waldo
> 07:25:05.639445 dir.c:952               exclude: baz/waldo.txt vs * at
> line 1 => yes
>
> (meaning, baz/waldo.txt matches "*" rule and is decided to be excluded)
>
> with a60ea8f
>
>> /tmp/abc $ GIT_TRACE_EXCLUDE=1 ~/w/git/temp/git status -suall 2>&1 >/dev/null |grep waldo
> 07:25:24.425125 dir.c:952               exclude: baz/waldo.txt vs /baz
> at line 4 => no
>
> the decision is not taken earlier from line "!/baz" and it's decided

s/not taken/taken/

> to be re-included. Which I would argue is the correct thing because
> you ask to re-include the whole directory "baz". It should behave this
> way because exclude rules without '!' behave this way.
>
> Because positive any negative rules can be nested, by adding a rule to

s/any/and/

> reinclude what's inside baz..
>
>     *
>     !/foo
>     !/foo/bar.txt
>     !/baz
>     /baz/*
>     !/baz/quux
>     !/baz/quux/**/*
>
> you'll get baz/waldo.txt excluded.
>
> Yes it's a behavior change. I think the old behavior is unintended.
> But it's probably out there long enough that many .gitignore files may
> rely on it and by making it right, I break them. Whether to revert the
> series is up to you. Let me know if I should send the revert patch.
> --
> Duy



-- 
Duy

Re: Change in .gitignore handling: intended or bug?

[Charles Strahan]

The fix on my side was quite easy (and my .gitignore is probably a _lot_
hairier
than most), and as Duy suggests, I think the new behavior makes a bit
more
sense. Personally, I would be pleased with keeping the new behavior, and
chalking it up to an unintentional bug fix (the best kind).

Either way, of course, I'd like for it to not change back and forth
between
releases :).

Perhaps just an announcement of the new behavior would suffice - 2.7.0
has been
out for a while anyway. If people were going to complain, I figure they
would
have done so by now.

-Charles


On Fri, Mar 4, 2016, at 07:50 PM, Duy Nguyen wrote:
> typo fixes
> 
> On Sat, Mar 5, 2016 at 7:43 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> > On Sat, Mar 5, 2016 at 12:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
> >> Duy Nguyen <pclouds@gmail.com> writes:
> >>
> >>> On Fri, Mar 4, 2016 at 6:56 PM, Kevin Daudt <me@ikke.info> wrote:
> >>>> Verified that it's different in 2.7.0, but 2.7.2 gives expected output.
> >>>
> >>> Thanks. 2.7.1 reverts the faulty commit from 2.7.0 that generated two
> >>> other regression reports before this one. I guess it's all good then
> >>> (except for the people still on 2.7.0)
> >>
> >> Are we good at 2.8.0-rc0, too?  Somehow I had an impression that we
> >> queued "another attempt to do it differently" or something.
> >>
> >>  ... goes and looks ...
> >>
> >>     $ rungit maint status -suall
> >>     ?? baz/quux/corge/wibble.txt
> >>     ?? baz/quux/grault.txt
> >>     ?? foo/bar.txt
> >>     $ rungit master status -suall
> >>     ?? baz/quux/corge/wibble.txt
> >>     ?? baz/quux/grault.txt
> >>     ?? baz/waldo.txt
> >>     ?? foo/bar.txt
> >>     ?? foo/garply.txt
> >
> > If you swap a60ea8f and bac65a2 so that we can have tracing even
> > without the problematic commit a60ea8f (dir.c: fix match_pathname() -
> > 2016-02-15). Without a60ea8f I got
> >
> > GIT_TRACE_EXCLUDE=1 ~/w/git/temp/git status -suall 2>&1 >/dev/null
> > |grep waldo
> > 07:25:05.639445 dir.c:952               exclude: baz/waldo.txt vs * at
> > line 1 => yes
> >
> > (meaning, baz/waldo.txt matches "*" rule and is decided to be excluded)
> >
> > with a60ea8f
> >
> >> /tmp/abc $ GIT_TRACE_EXCLUDE=1 ~/w/git/temp/git status -suall 2>&1 >/dev/null |grep waldo
> > 07:25:24.425125 dir.c:952               exclude: baz/waldo.txt vs /baz
> > at line 4 => no
> >
> > the decision is not taken earlier from line "!/baz" and it's decided
> 
> s/not taken/taken/
> 
> > to be re-included. Which I would argue is the correct thing because
> > you ask to re-include the whole directory "baz". It should behave this
> > way because exclude rules without '!' behave this way.
> >
> > Because positive any negative rules can be nested, by adding a rule to
> 
> s/any/and/
> 
> > reinclude what's inside baz..
> >
> >     *
> >     !/foo
> >     !/foo/bar.txt
> >     !/baz
> >     /baz/*
> >     !/baz/quux
> >     !/baz/quux/**/*
> >
> > you'll get baz/waldo.txt excluded.
> >
> > Yes it's a behavior change. I think the old behavior is unintended.
> > But it's probably out there long enough that many .gitignore files may
> > rely on it and by making it right, I break them. Whether to revert the
> > series is up to you. Let me know if I should send the revert patch.
> > --
> > Duy
> 
> 
> 
> -- 
> Duy

Re: Change in .gitignore handling: intended or bug?

[Junio C Hamano]

Charles Strahan <charles@cstrahan.com> writes:

> ...as Duy suggests, I think the new behavior makes a bit
> more sense.

After re-reading your original example, I am inclined to agree with
this.

> Either way, of course, I'd like for it to not change back and
> forth between releases :).
>
> Perhaps just an announcement of the new behavior would suffice -
> 2.7.0 has been out for a while anyway. If people were going to
> complain, I figure they would have done so by now.

Yup, I think a documentation update to clarify how positive and
negative ignore patterns interact with each other may be necessary,
with some examples.

Care to work on a patch?

Thanks.

Re: Change in .gitignore handling: intended or bug?

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Sat, Mar 5, 2016 at 12:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Are we good at 2.8.0-rc0, too?  Somehow I had an impression that we
>> queued "another attempt to do it differently" or something.
>>
>>  ... goes and looks ...
>>
>>     $ rungit maint status -suall
>>     ?? baz/quux/corge/wibble.txt
>>     ?? baz/quux/grault.txt
>>     ?? foo/bar.txt
>>     $ rungit master status -suall
>>     ?? baz/quux/corge/wibble.txt
>>     ?? baz/quux/grault.txt
>>     ?? baz/waldo.txt
>>     ?? foo/bar.txt
>>     ?? foo/garply.txt
> ...
> the decision is not taken earlier from line "!/baz" and it's decided
> to be re-included. Which I would argue is the correct thing because
> you ask to re-include the whole directory "baz". It should behave this
> way because exclude rules without '!' behave this way.

We need documentation update to settle this one before 2.8 final
ships, as we seem to be seeing more and more end-user confusion on
the list.  I tried to come up with a trimmed-down example, which is
shown below, but I suspect that the code is not exactly working the
way it is described in that (1) dir/file1 is ignored and (3)
!dir/file3 entry makes difference.

Where did my example go wrong?

FYI, if I prefix '/' to all the .gitignore entries in the example, i.e.
making it

    *
    !/dir
    /dir/file2
    !/dir/file3

instead, then dir/file1 and dir/file3 do get shown as unignored.

If it is documented somewhere, then I can update the example and
declare victory (but then the text that accompanies the example
still needs to remind the readers why the leading '/' matters.

Thanks.



 Documentation/gitignore.txt | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
index 3ded6fd..b841233 100644
--- a/Documentation/gitignore.txt
+++ b/Documentation/gitignore.txt
@@ -150,6 +150,40 @@ excluded, the following conditions must be met:
  - The directory part in the re-include rules must be literal (i.e. no
    wildcards)
 
+A re-inclusion of a directory makes all files in the directory
+unignored.  For example, suppose you have files `.gitignore`,
+`dir/file1`, `dir/file2`, and `dir/file3`, and have the following in
+your `.gitignore`:
+
+----------------
+*
+!dir
+# dir/file1 is not mentioned in .gitignore
+dir/file2
+!dir/file3
+----------------
+
+Then:
+
+ - `.gitignore` gets ignored, because it matches the `*` at the top
+   level;
+
+ - `dir/file1` gets unignored, because `dir` marks everything
+   underneath `dir/` to be unignored unless otherwise specified;
+
+ - `dir/file2` gets ignored, because `dir/file2` is listed to be
+   ignored;
+
+ - `dir/file3` gets unignored, because `dir/file3` is listed to be
+   ignored.  Note that the entry `!dir/file3` is redundant because
+   everything underneath `dir/` is marked to be unignored already.
+
+Some earlier versions of Git treated `!dir` differently in that it
+did not cause the paths under it unignored, but this has been
+corrected to be consistent with `dir` that says "`dir` and everything
+below are ignored."
+
+
 EXAMPLES
 --------
 

Re: Change in .gitignore handling: intended or bug?

[Junio C Hamano]

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

> We need documentation update to settle this one before 2.8 final
> ships, as we seem to be seeing more and more end-user confusion on
> the list.  I tried to come up with a trimmed-down example, which is
> shown below, but I suspect that the code is not exactly working the
> way it is described in that (1) dir/file1 is ignored and (3)
> !dir/file3 entry makes difference.
>
> Where did my example go wrong?
>
> FYI, if I prefix '/' to all the .gitignore entries in the example, i.e.
> making it
>
>     *
>     !/dir
>     /dir/file2
>     !/dir/file3
>
> instead, then dir/file1 and dir/file3 do get shown as unignored.
>
> If it is documented somewhere, then I can update the example and
> declare victory (but then the text that accompanies the example
> still needs to remind the readers why the leading '/' matters.

I also found that having an extra slash at the end of the
"everything underneath 'dir' directory is included", i.e.

     *
     !/dir/
     /dir/file2
     !/dir/file3

breaks it.  dir/file1 is ignored, dir/file3 isn't but the latter is
only because there is an explicit !/dir/file3.  This is contrary to
this rule in the documentation:

 - If the pattern ends with a slash, it is removed for the
   purpose of the following description, but it would only find
   a match with a directory.  In other words, `foo/` will match a
   directory `foo` and paths underneath it, but will not match a
   regular file or a symbolic link `foo` (this is consistent
   with the way how pathspec works in general in Git).

In other words, '!/dir/' does not seem to match the directory dir
and paths underneath it.

Thanks.

>  Documentation/gitignore.txt | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
> index 3ded6fd..b841233 100644
> --- a/Documentation/gitignore.txt
> +++ b/Documentation/gitignore.txt
> @@ -150,6 +150,40 @@ excluded, the following conditions must be met:
>   - The directory part in the re-include rules must be literal (i.e. no
>     wildcards)
>  
> +A re-inclusion of a directory makes all files in the directory
> +unignored.  For example, suppose you have files `.gitignore`,
> +`dir/file1`, `dir/file2`, and `dir/file3`, and have the following in
> +your `.gitignore`:
> +
> +----------------
> +*
> +!dir
> +# dir/file1 is not mentioned in .gitignore
> +dir/file2
> +!dir/file3
> +----------------
> +
> +Then:
> +
> + - `.gitignore` gets ignored, because it matches the `*` at the top
> +   level;
> +
> + - `dir/file1` gets unignored, because `dir` marks everything
> +   underneath `dir/` to be unignored unless otherwise specified;
> +
> + - `dir/file2` gets ignored, because `dir/file2` is listed to be
> +   ignored;
> +
> + - `dir/file3` gets unignored, because `dir/file3` is listed to be
> +   ignored.  Note that the entry `!dir/file3` is redundant because
> +   everything underneath `dir/` is marked to be unignored already.
> +
> +Some earlier versions of Git treated `!dir` differently in that it
> +did not cause the paths under it unignored, but this has been
> +corrected to be consistent with `dir` that says "`dir` and everything
> +below are ignored."
> +
> +
>  EXAMPLES
>  --------
>  

Re: Change in .gitignore handling: intended or bug?

[Duy Nguyen]

On Tue, Mar 8, 2016 at 1:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> We need documentation update to settle this one before 2.8 final
>> ships, as we seem to be seeing more and more end-user confusion on
>> the list.  I tried to come up with a trimmed-down example, which is
>> shown below, but I suspect that the code is not exactly working the
>> way it is described in that (1) dir/file1 is ignored and (3)
>> !dir/file3 entry makes difference.
>>
>> Where did my example go wrong?
>>
>> FYI, if I prefix '/' to all the .gitignore entries in the example, i.e.
>> making it
>>
>>     *
>>     !/dir
>>     /dir/file2
>>     !/dir/file3
>>
>> instead, then dir/file1 and dir/file3 do get shown as unignored.

Arghhh.. bug!!!

The difference between "dir" and "/dir" is, the former is basename
matching while the latter is pathname matching. When we check
dir/file1 after we enter "dir", we do try to check rule "!/dir" (or
"!dir") before rule "*". In the pathname matching case, it works
thanks to a60ea8f.

In the basename matching case (i.e. rule "!dir"), the code does not do
the right thing. It blindly checks the base name of dir/file1, which
is "file1", against the pattern "dir". The right thing to do is check
the "dir" in "dir/file1" part against pattern "dir". Failing that, we
fall back to pattern "*" and excludes dir/file1 as well.

>> If it is documented somewhere, then I can update the example and
>> declare victory (but then the text that accompanies the example
>> still needs to remind the readers why the leading '/' matters.
>
> I also found that having an extra slash at the end of the
> "everything underneath 'dir' directory is included", i.e.
>
>      *
>      !/dir/
>      /dir/file2
>      !/dir/file3
>
> breaks it.  dir/file1 is ignored, dir/file3 isn't but the latter is
> only because there is an explicit !/dir/file3.  This is contrary to
> this rule in the documentation:

This is pretty much the same. But instead basename matching unable to
deal with nested rules, the trailing slash means "check if it is a
directory". Again, the code tries to check if "dir/file1" is a
directory instead of "dir".

I haven't dug back in history, but my impression is it has been so
probably from the beginning of "!" introduction. This has nothing to
do with nd/exclusion-regression-fix, which is more about

    dir
    !dir/file

than

    !dir
    dir/file

Although the first patch in that series happens to fix the pathname
matching case in this example.

I don't know. It seems to me I should fix this anyway, by making both
"NODIR" and "MUSTBEDIR" code work well inside a directory. That should
fix it. I hope I don't have to turn dir.c up side down for that. Last
time I looked, it wasn't easy, which led to hack/avoid it with 57534ee
and that was eventually reverted.
-- 
Duy

Re: Change in .gitignore handling: intended or bug?

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Tue, Mar 8, 2016 at 1:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> We need documentation update to settle this one before 2.8 final
>>> ships, as we seem to be seeing more and more end-user confusion on
>>> the list.  I tried to come up with a trimmed-down example, which is
>>> shown below, but I suspect that the code is not exactly working the
>>> way it is described in that (1) dir/file1 is ignored and (3)
>>> !dir/file3 entry makes difference.
>>>
>>> Where did my example go wrong?
>>>
>>> FYI, if I prefix '/' to all the .gitignore entries in the example, i.e.
>>> making it
>>>
>>>     *
>>>     !/dir
>>>     /dir/file2
>>>     !/dir/file3
>>>
>>> instead, then dir/file1 and dir/file3 do get shown as unignored.
>
> Arghhh.. bug!!!
>
> The difference between "dir" and "/dir" is, the former is basename
> matching while the latter is pathname matching. When we check
> dir/file1 after we enter "dir", we do try to check rule "!/dir" (or
> "!dir") before rule "*". In the pathname matching case, it works
> thanks to a60ea8f.

So what do we want to do for the upcoming release?  I am OK to leave
the code as-is for now and describe it as a known bug that is still
being worked on (as long as it indeed is being worked on, that is),
as the desired endgame of making "!dir" act in a way more similar to
how "dir" acts does sound sensible.

If we are going that route, perhaps something like this is the
minimum we would need before 2.8 final.

Thoughts?


 Documentation/gitignore.txt | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
index 3ded6fd..91d1ce2 100644
--- a/Documentation/gitignore.txt
+++ b/Documentation/gitignore.txt
@@ -148,7 +148,43 @@ excluded, the following conditions must be met:
    be in the same .gitignore file.
 
  - The directory part in the re-include rules must be literal (i.e. no
-   wildcards)
+   wildcards and has to start with a `/`).
+
+A re-inclusion of a directory makes all files in the directory
+unignored.  For example, suppose you have files `.gitignore`,
+`dir/file1`, `dir/file2`, and `dir/file3`, and have the following in
+your `.gitignore`:
+
+----------------
+# .gitignore is not mentioned in .gitignore
+*
+!/dir
+# dir/file1 is not mentioned in .gitignore
+dir/file2
+!dir/file3
+----------------
+
+Then:
+
+ - `.gitignore` gets ignored, because it matches the `*` at the top
+   level;
+
+ - `dir/file1` does not get ignored, because `/dir` marks everything
+   underneath `dir/` directory to be 're-included' unless otherwise
+   specified;
+
+ - `dir/file2` gets ignored, because `dir/file2` matches it.
+
+ - `dir/file3` does not get ignored, because `!dir/file3` matches it.
+   Note that the entry `!dir/file3` is redundant because everything
+   underneath `dir/` is marked to be 're-included' already.
+
+Some earlier versions of Git treated `!/dir` above differently in
+that it did not cause the paths under it unignored (but merely told
+Git that patterns that begin with dir/ should not be ignored), but
+this has been corrected to be consistent with `/dir` that says "the
+directory `dir/` and everything below are ignored."
+
 
 EXAMPLES
 --------

Re: Change in .gitignore handling: intended or bug?

[Duy Nguyen]

On Wed, Mar 9, 2016 at 1:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
> So what do we want to do for the upcoming release?

I don't know. Befoire 2.8.0, all three matching cases are broken. With
the current changes on 2.8.0, one case is fixed with the other cases
broken. I guess it can create even more confusion. Yeah documentation
helps a bit.

> I am OK to leave
> the code as-is for now and describe it as a known bug that is still
> being worked on (as long as it indeed is being worked on, that is),

I do want to fix it. I don't know how much code is impacted yet (and
how many more bugs I'll be introducing while attempting to fix it). It
may take a few cycles before the fix can be released.
-- 
Duy

Re: Change in .gitignore handling: intended or bug?

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Wed, Mar 9, 2016 at 1:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> So what do we want to do for the upcoming release?
>
> I don't know. Befoire 2.8.0, all three matching cases are broken. With
> the current changes on 2.8.0, one case is fixed with the other cases
> broken. I guess it can create even more confusion. Yeah documentation
> helps a bit.
>
>> I am OK to leave
>> the code as-is for now and describe it as a known bug that is still
>> being worked on (as long as it indeed is being worked on, that is),
>
> I do want to fix it. I don't know how much code is impacted yet (and
> how many more bugs I'll be introducing while attempting to fix it). It
> may take a few cycles before the fix can be released.

Sorry, but I should have been more clear.  At this point in the
release cycle, I do not think it is an option to pile on more
"fixes" to risk destabilizing the end-user experience even more
before 2.8.0 final happens.

It is between (1) the current code is good enough that with a
(temporary) limitation clearly described in the documentation users
can work around the implementation deficiency and get a benaviour
that is closer than 2.7.2, or (2) the half-way implementation we
have does not give enough advancement toward the final goal
(i.e. the !dir re-inclusion behaves consistently with the dir
that ignores the whole thing underneath, while allowing subpaths
ignored with follow-on entries in the same .gitignore file), and we
are better off reverting the whole thing to go back to 2.7.2
behaviour, planning to do a better job in the next cycle.

I was hoping that (1) would be the case.

Re: Change in .gitignore handling: intended or bug?

[Junio C Hamano]

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

> It is between (1) the current code is good enough ...
> or (2) the half-way implementation we
> have does not give enough advancement ... and we
> are better off reverting the whole thing to go back to 2.7.2
> behaviour, planning to do a better job in the next cycle.
>
> I was hoping that (1) would be the case.

Oh and of course I still am--otherwise I wouldn't have been
attempting these documentation updates.  I just wanted to make sure
that I covered most (if not all) of the corner cases that we know
the current code does not work by documenting workarounds.

Re: Change in .gitignore handling: intended or bug?

[Duy Nguyen]

On Wed, Mar 9, 2016 at 1:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
> diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
> index 3ded6fd..91d1ce2 100644
> --- a/Documentation/gitignore.txt
> +++ b/Documentation/gitignore.txt
> @@ -148,7 +148,43 @@ excluded, the following conditions must be met:
>     be in the same .gitignore file.
>
>   - The directory part in the re-include rules must be literal (i.e. no
> -   wildcards)
> +   wildcards and has to start with a `/`).

Technically '/' can just appear anywhere in the pattern, except at the
end. But because the patterns in question must look like this

    dir # or any pattern, even "*"
    !dir/someth*ng

even if there is a slash at the end (and is ignored), we are still
good. Not sure how to phrase that though.

> +
> +A re-inclusion of a directory makes all files in the directory
> +unignored.  For example, suppose you have files `.gitignore`,
> +`dir/file1`, `dir/file2`, and `dir/file3`, and have the following in
> +your `.gitignore`:
> +
> +----------------
> +# .gitignore is not mentioned in .gitignore
> +*
> +!/dir
> +# dir/file1 is not mentioned in .gitignore
> +dir/file2
> +!dir/file3
> +----------------
> +
> +Then:
> +
> + - `.gitignore` gets ignored, because it matches the `*` at the top
> +   level;
> +
> + - `dir/file1` does not get ignored, because `/dir` marks everything
> +   underneath `dir/` directory to be 're-included' unless otherwise
> +   specified;
> +
> + - `dir/file2` gets ignored, because `dir/file2` matches it.
> +
> + - `dir/file3` does not get ignored, because `!dir/file3` matches it.
> +   Note that the entry `!dir/file3` is redundant because everything
> +   underneath `dir/` is marked to be 're-included' already.
> +
> +Some earlier versions of Git treated `!/dir` above differently in
> +that it did not cause the paths under it unignored (but merely told
> +Git that patterns that begin with dir/ should not be ignored), but
> +this has been corrected to be consistent with `/dir` that says "the
> +directory `dir/` and everything below are ignored."
> +

Looks good.
-- 
Duy

[PATCH] dir.c: fix dir re-inclusion rules with "NODIR" and "MUSTBEDIR"

[Nguyễn Thái Ngọc Duy]

For NODIR case, the patterns look like this

    *          # exclude dir, dir/file1 and dir/file2..
    !dir       # ..except that dir and everything inside is re-included..
    dir/file2  # ..except (again!) that dir/file2 is excluded
               # ..which means dir/file1 stays included

When we stay at topdir and test "dir", everything is hunky dory, current
code returns "re-include dir" as expected. When we stay in "dir" and
examine "dir/file1", however, match_basename() checks if the base name
component, which is "file1", matches "dir" from the second rule.

This is wrong. We contradict ourselves because earlier we decided to
re-include dir/file1 (from the second rule) when we were at toplevel.
Because the second rule is ignored, we hit the first one and return
"excluded" even though "dir/file1" should be re-included.

Side note, it's probably a bad idea to use basename matching on a
negative rule (ie. no slashes in the pattern) because what the pattern
says is "re-include _any_ directory named 'dir'", not just the directory
"dir" at right below this directory.

In the MUSTBEDIR case, the patterns look like this

    *          # exclude dir, dir/file1 and dir/file2..
    !dir/      #  ..except that dir and everything inside is re-included..
    dir/file2  # ..except (again!) that dir/file2 is excluded
               # ..which means dir/file1 stays included

Again, we're ok at the toplevel, then we enter "dir" and test
"dir/file1". The MUSTBEDIR code tests if the _full_ path (ie. "dir/file1")
is a directory. We want it to test the "dir" part from "dir/file1"
instead.

In both cases, we want to test if the pattern matches a parent
directory. match_dir_component() is for this purpose.

We do want to be careful not to get back too far. Given the file
foo/bar/.gitignore, we should only check as far back as foo/bar because
this .gitignore file can never match outside that directory, which is
"toplevel" in the above paragraphs, to begin with.

The remaining matching case (neither NODIR nor MUSTBEDIR is set) is
already fixed in a60ea8f (dir.c: fix match_pathname() - 2016-02-15)

Reported-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 The fix may look like this (I didn't think about the "**" trick in
 wildmatch, which makes things simpler).

 No it's not meant for 2.8.0. I didn't even optimize for the
 no-wildcard case, or add tests. But we can still stare at it in the
 meantime to see if I analyzed anything wrong. I almost thought I was
 wrong to the declare the bug while I was on my ride back home..

 dir.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/dir.c b/dir.c
index 69e0be6..5123483 100644
--- a/dir.c
+++ b/dir.c
@@ -992,6 +992,51 @@ static struct exclude *should_descend(const char *pathname, int pathlen,
 }
 
 /*
+ * Given a "NODIR" pattern, check if it matches any directory
+ * component after the x->base part.
+ *
+ * If the pattern is not NODIR (ie. pathname matching) _and_ is
+ * MUSTBE, check if it matches a directory as well.
+ *
+ * Note that "path" and "len" must cover the basename part as well,
+ * looking from last_exclude_matching_from_list(), not just the dirname.
+ */
+static int match_dir_component(const char *path, int len, struct exclude *x)
+{
+	struct strbuf new_pattern = STRBUF_INIT;
+	int ret;
+
+	if (x->flags & EXC_FLAG_NODIR) {
+		if (!x->baselen) {
+			strbuf_addf(&new_pattern, "%s/**", x->pattern);
+			ret = !fnmatch_icase_mem(new_pattern.buf, new_pattern.len,
+						 path, strlen(path),
+						 WM_PATHNAME);
+			strbuf_reset(&new_pattern);
+			if (ret)
+				return ret;
+		}
+
+		strbuf_addf(&new_pattern, "%.*s**/%s/**", x->baselen, x->base, x->pattern);
+		ret = !fnmatch_icase_mem(new_pattern.buf, new_pattern.len,
+					 path, strlen(path),
+					 WM_PATHNAME);
+		strbuf_reset(&new_pattern);
+		return ret;
+	}
+
+	assert(x->flags & EXC_FLAG_MUSTBEDIR);
+	strbuf_addf(&new_pattern, "%.*s%s/**",
+		    x->baselen, x->base,
+		    *x->pattern == '/' ? x->pattern+1 : x->pattern);
+	ret = !fnmatch_icase_mem(new_pattern.buf, new_pattern.len,
+				 path, strlen(path),
+				 WM_PATHNAME);
+	strbuf_release(&new_pattern);
+	return ret;
+}
+
+/*
  * Scan the given exclude list in reverse to see whether pathname
  * should be ignored.  The first match (i.e. the last on the list), if
  * any, determines the fate.  Returns the exclude_list element which
@@ -1033,7 +1078,8 @@ static struct exclude *last_exclude_matching_from_list(const char *pathname,
 		if (x->flags & EXC_FLAG_MUSTBEDIR) {
 			if (*dtype == DT_UNKNOWN)
 				*dtype = get_dtype(NULL, pathname, pathlen);
-			if (*dtype != DT_DIR)
+			if (*dtype != DT_DIR &&
+			    !match_dir_component(pathname, strlen(pathname), x))
 				continue;
 		}
 
@@ -1041,7 +1087,8 @@ static struct exclude *last_exclude_matching_from_list(const char *pathname,
 			if (match_basename(basename,
 					   pathlen - (basename - pathname),
 					   exclude, prefix, x->patternlen,
-					   x->flags)) {
+					   x->flags) ||
+			    match_dir_component(pathname, strlen(pathname), x)) {
 				exc = x;
 				break;
 			}
-- 
2.8.0.rc0.208.g69d9f93

Re: [PATCH] dir.c: fix dir re-inclusion rules with "NODIR" and "MUSTBEDIR"

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Side note, it's probably a bad idea to use basename matching on a
> negative rule (ie. no slashes in the pattern) because what the pattern
> says is "re-include _any_ directory named 'dir'", not just the directory
> "dir" at right below this directory.

I am not sure I agree with this comment.  When we say '.depend/' in
our .gitignore file, we do want to ignore everything in the
directory with that name anywhere in that tree.  Perhaps '!include/'
followed by '*' may be a good way to pick only the header files that
are found in any directory with that name anywhere in the tree in a
similar fashion.  It certainly is a disappointing comment if made by
somebody who wants to make 'dir' and '!dir' behave in a more similar
way, I'd have to say.

> In the MUSTBEDIR case, the patterns look like this
>
>     *          # exclude dir, dir/file1 and dir/file2..
>     !dir/      #  ..except that dir and everything inside is re-included..
>     dir/file2  # ..except (again!) that dir/file2 is excluded
>                # ..which means dir/file1 stays included
>
> Again, we're ok at the toplevel, then we enter "dir" and test
> "dir/file1". The MUSTBEDIR code tests if the _full_ path (ie. "dir/file1")
> is a directory. We want it to test the "dir" part from "dir/file1"
> instead.

That certainly sounds like a breakage worth fixing.

> In both cases, we want to test if the pattern matches a parent
> directory. match_dir_component() is for this purpose.

I do not follow.  I would understand "In all cases, we want to
behave as if we are testing the _full_ path against the pattern",
though.

IOW, "dir/file1" matches '*' (because 'file1' and '*' matches),
'!dir/' (because the pattern is "everything under dir/ directory),
and nothing else in the pattern list, and the last match
wins---which happens to be negative ignore, hence dir/file1 is not
ignored.

> We do want to be careful not to get back too far. Given the file
> foo/bar/.gitignore, we should only check as far back as foo/bar because
> this .gitignore file can never match outside that directory, which is
> "toplevel" in the above paragraphs, to begin with.

Yes.  But isn't that what exclude_stack mechanism already gives us?
That is, when you are not looking at a path inside foo/bar/, entries
stored in foo/bar/.gitignore will not participate the determination
of the fate of the path.

I am not sure why you have to say this.  Puzzled...

Re: Change in .gitignore handling: intended or bug?

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Wed, Mar 9, 2016 at 1:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
>> index 3ded6fd..91d1ce2 100644
>> --- a/Documentation/gitignore.txt
>> +++ b/Documentation/gitignore.txt
>> @@ -148,7 +148,43 @@ excluded, the following conditions must be met:
>>     be in the same .gitignore file.
>>
>>   - The directory part in the re-include rules must be literal (i.e. no
>> -   wildcards)
>> +   wildcards and has to start with a `/`).
>
> Technically '/' can just appear anywhere in the pattern, except at the
> end. But because the patterns in question must look like this
>
>     dir # or any pattern, even "*"
>     !dir/someth*ng
>
> even if there is a slash at the end (and is ignored), we are still
> good. Not sure how to phrase that though.

Post 2.8, we'd be correcting this properly anyway, we should aim for
the simplest-to-explain way to work around the limitation in the
current code, that will still work once the bug is fixed.  I am not
sure if "Technically it can be other things" helps, unless it makes
it a lot easier to use.

With that in mind, do you think we need to find a better phrase to
loosen what I wrote that is overly-strict?  That is, is "must begin
with '/' to anchor it to the level .gitignore appears" too strict to
make it too hard to use?

Re: Change in .gitignore handling: intended or bug?

[Duy Nguyen]

On Thu, Mar 10, 2016 at 1:02 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Wed, Mar 9, 2016 at 1:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
>>> index 3ded6fd..91d1ce2 100644
>>> --- a/Documentation/gitignore.txt
>>> +++ b/Documentation/gitignore.txt
>>> @@ -148,7 +148,43 @@ excluded, the following conditions must be met:
>>>     be in the same .gitignore file.
>>>
>>>   - The directory part in the re-include rules must be literal (i.e. no
>>> -   wildcards)
>>> +   wildcards and has to start with a `/`).
>>
>> Technically '/' can just appear anywhere in the pattern, except at the
>> end. But because the patterns in question must look like this
>>
>>     dir # or any pattern, even "*"
>>     !dir/someth*ng
>>
>> even if there is a slash at the end (and is ignored), we are still
>> good. Not sure how to phrase that though.
>
> Post 2.8, we'd be correcting this properly anyway, we should aim for
> the simplest-to-explain way to work around the limitation in the
> current code, that will still work once the bug is fixed.  I am not
> sure if "Technically it can be other things" helps, unless it makes
> it a lot easier to use.
>
> With that in mind, do you think we need to find a better phrase to
> loosen what I wrote that is overly-strict?  That is, is "must begin
> with '/' to anchor it to the level .gitignore appears" too strict to
> make it too hard to use?
>

If we can name rules from the syntax part, then we can simply say
"patterns that meet rules x and y".

A bit off topic, but these two paragraphs may need rephrasing, I don't
really understand what it's trying to say

 - If the pattern does not contain a slash '/', Git treats it as
   a shell glob pattern and checks for a match against the
   pathname relative to the location of the `.gitignore` file
   (relative to the toplevel of the work tree if not from a
   `.gitignore` file).

Not sure why "relative to the location of .gitignore file" matters. We
basically just take `basename $path` out and try to match it.

 - Otherwise, Git treats the pattern as a shell glob suitable
   for consumption by fnmatch(3) with the FNM_PATHNAME flag:
   wildcards in the pattern will not match a / in the pathname.
   For example, "Documentation/{asterisk}.html" matches
   "Documentation/git.html" but not "Documentation/ppc/ppc.html"
   or "tools/perf/Documentation/perf.html".

Perhaps "Otherwise" can be stated explicitly that "if the pattern does
contain any slash, besides the trailing one". The "wildcards not
matching /" is also true for the "pattern does not contain a slash"
rule above. We can make it a generic note about wildcards and remove
this "otherwise" rule, maybe...
-- 
Duy

Re: Change in .gitignore handling: intended or bug?

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> A bit off topic, but these two paragraphs may need rephrasing, I don't
> really understand what it's trying to say
>
>  - If the pattern does not contain a slash '/', Git treats it as
>    a shell glob pattern and checks for a match against the
>    pathname relative to the location of the `.gitignore` file
>    (relative to the toplevel of the work tree if not from a
>    `.gitignore` file).
>
> Not sure why "relative to the location of .gitignore file" matters. We
> basically just take `basename $path` out and try to match it.

That is because the documentation was written with a single
consistent matching mode in mind: you match various patterns against
the FULL path in the repository, no matter where you find the path.
If you find a pattern "*.c" in d/.gitignore file, a path d/hello.c
matches, even though '*.c' does not match 'd/hello.c' in the shell
glob sense, and the rule needs to clarify that the leading directory
part "d/" in the path "d/hello.c" does not participate when matching
it against the pattern "*.c" taken from "d/.gitignore" file.

If you start from a mindset to match hello.c part with *.c when you
find both in d/ directory, "relative to the location" part would
sound redundant or even confusing.

>  - Otherwise, Git treats the pattern as a shell glob suitable
>    for consumption by fnmatch(3) with the FNM_PATHNAME flag:
>    wildcards in the pattern will not match a / in the pathname.
>    For example, "Documentation/{asterisk}.html" matches
>    "Documentation/git.html" but not "Documentation/ppc/ppc.html"
>    or "tools/perf/Documentation/perf.html".
>
> Perhaps "Otherwise" can be stated explicitly that "if the pattern does
> contain any slash, besides the trailing one". 

Yeah, I think there originally was just two (it has slasn or it
doesn't) and "If ..." followed by "Otherwise" was clear enough, but
spellin it out would not hurt.

Re: [PATCH] dir.c: fix dir re-inclusion rules with "NODIR" and "MUSTBEDIR"

[Duy Nguyen]

On Thu, Mar 10, 2016 at 12:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Side note, it's probably a bad idea to use basename matching on a
>> negative rule (ie. no slashes in the pattern) because what the pattern
>> says is "re-include _any_ directory named 'dir'", not just the directory
>> "dir" at right below this directory.
>
> I am not sure I agree with this comment.  When we say '.depend/' in
> our .gitignore file, we do want to ignore everything in the
> directory with that name anywhere in that tree.  Perhaps '!include/'
> followed by '*' may be a good way to pick only the header files that
> are found in any directory with that name anywhere in the tree in a
> similar fashion.  It certainly is a disappointing comment if made by
> somebody who wants to make 'dir' and '!dir' behave in a more similar
> way, I'd have to say.

Hmm.. yeah personal view. I'll take this out.

>> In both cases, we want to test if the pattern matches a parent
>> directory. match_dir_component() is for this purpose.
>
> I do not follow.  I would understand "In all cases, we want to
> behave as if we are testing the _full_ path against the pattern",
> though.
>
> IOW, "dir/file1" matches '*' (because 'file1' and '*' matches),
> '!dir/' (because the pattern is "everything under dir/ directory),
> and nothing else in the pattern list, and the last match
> wins---which happens to be negative ignore, hence dir/file1 is not
> ignored.

Yep, bad phrasing.

>> We do want to be careful not to get back too far. Given the file
>> foo/bar/.gitignore, we should only check as far back as foo/bar because
>> this .gitignore file can never match outside that directory, which is
>> "toplevel" in the above paragraphs, to begin with.
>
> Yes.  But isn't that what exclude_stack mechanism already gives us?
> That is, when you are not looking at a path inside foo/bar/, entries
> stored in foo/bar/.gitignore will not participate the determination
> of the fate of the path.
>
> I am not sure why you have to say this.  Puzzled...

Naively speaking, in order to fix this without reorganizing a lot of
code, when given path 'foo/bar/dir/file1' and the pattern '!dir/', we
first still test if the full path "foo/bar/dir/file1" is a directory,
then we cut "file1" out (pretend that we are in foo/bar instead of in
foo/bar/dir) and test again the full path, which is just foo/bar/dir,
if it's a directory. Repeat again and again. This trick does not go
through the normal prep_exclude(), so patterns are not popping out
when .gitignore should not be used any more.

The implementation is a bit different. Instead of going up one level,
test, and go up again, I construct the new pattern "foo/bar/**/dir/**"
and match it (only once) against the original full path
foo/bar/dir/file1. The note was there because I would have just
constructed "**/dir/**" instead and matched too much.
-- 
Duy

Re: Change in .gitignore handling: intended or bug?

[Junio C Hamano]

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

> Duy Nguyen <pclouds@gmail.com> writes:
>
>> A bit off topic, but these two paragraphs may need rephrasing, I don't
>> really understand what it's trying to say
>>
>>  - If the pattern does not contain a slash '/', Git treats it as
>>    a shell glob pattern and checks for a match against the
>>    pathname relative to the location of the `.gitignore` file
>>    (relative to the toplevel of the work tree if not from a
>>    `.gitignore` file).
>>
>> Not sure why "relative to the location of .gitignore file" matters. We
>> basically just take `basename $path` out and try to match it.
>
> That is because the documentation was written with ...
> matches, even though '*.c' does not match 'd/hello.c' in the shell
> glob sense,...

Sorry, this does not make much sense, as d/e/hello.c would still
match '*.c' in d/.gitignore.  So it is a straight-forward match
against the basename, and there is nothing relative to the location
of .gitignore.  If anything, relative to where the pattern is found
should be relevant to a pattern _with_ slash in it.

I should have been more careful and thought things through, but as
this was an "off topic" comment, I didn't.  Sorry about that.

In any case, back to "on topic" part again; I couldn't come up with
a better rewrite using named rules (partly because you need to
clearly define each rule before referring them, and some of the
rules are temporary workarounds for the 2.8 regression that will
hopefully disappear in near future).  I think you understand the bug
and the limitation of the current code a lot better than I do, so if
you can please send a final version of the documentation update in
the coming 18 hours (I have an option of using what is already
queued on 'pu' as a fall-back-good-enough version but I want to keep
the last-resort option as that--if I know a potential source of a
better version, I'd choose to ask first ;-).

Thanks.

Re: Change in .gitignore handling: intended or bug?

[Duy Nguyen]

On Thu, Mar 10, 2016 at 7:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
> In any case, back to "on topic" part again; I couldn't come up with
> a better rewrite using named rules (partly because you need to
> clearly define each rule before referring them, and some of the
> rules are temporary workarounds for the 2.8 regression that will
> hopefully disappear in near future).  I think you understand the bug
> and the limitation of the current code a lot better than I do, so if
> you can please send a final version of the documentation update in
> the coming 18 hours (I have an option of using what is already
> queued on 'pu' as a fall-back-good-enough version but I want to keep
> the last-resort option as that--if I know a potential source of a
> better version, I'd choose to ask first ;-).

I'm never good with words, especially in a rush. Let's merge yours.
I'll fix the bug and update gitignore.txt in one go.
-- 
Duy