Repository: iterative/dvc
Pull request: Add more tests according to gitignore
fix #4103

* [x] ❗ I have followed the [Contributing to DVC](https://dvc.org/doc/user-guide/contributing/core) checklist.

* [x] 📖 If this PR requires [documentation](https://dvc.org/doc) updates, I have created a separate PR (or issue, at least) in [dvc.org](https://github.com/iterative/dvc.org) and linked it here.

* [x] ❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)

Thank you for the contribution - we'll try to review it as soon as possible. 🙏


Problem statement:
.dvcignore is broken on negation when blacklisting all
## Bug Report

### Please provide information about your setup

I am not sure how far this extends to, but a lot of trials for `.dvcignore` failed when I blacklisted all and tried to whitelist some:

```
*
!scripts
```

```
*
!/scripts
```

```
/*
!/scripts/
```

```
/*
!/scripts/*
```

```
/*
!/scripts/**
```

```
/*
!scripts/
```

### What worked:
```
/*
!/scripts
```

Why do I feel suddenly the spelling of `scripts` is wrong? :smile:

Hints from issue discussion:
CC @pared is this expected?
Considering that we have been designing `.dvcignore` to comply with `.gitignore`, I will refer to original:
It seems to me that we have some disrepancies in behaviour between `git` and `dvc`. I prepared a script to illustrate that:
```
#!/bin/bash

rm -rf repo
mkdir repo

pushd repo

git init --quiet
dvc init --quiet

git commit -am "ble"

mkdir data
echo 1 >> data/1
echo 2 >> data/2
echo 3 >> data/3
echo not_ignored >> data/file
mkdir data/dir
echo not_ignored >> data/dir/file

echo 'dvci*' >> .gitignore 

echo unrelated >> dvci0

echo '*' > dvci1

echo '*' > dvci2
echo '!/dir' >> dvci2
echo '!/file' >> dvci2

echo '*' > dvci3
echo 'dir' >> dvci3
echo 'file' >> dvci3

echo '/*' > dvci4
echo '!/dir/' >> dvci4
echo '!/file/' >> dvci4

echo '/*' > dvci5
echo '!/dir/*' >> dvci5

echo '/*' > dvci6
echo '!/dir/**' >> dvci6

echo '/*' > dvci7
echo '!dir/' >> dvci7
echo '!file/' >> dvci7

echo '/*' > dvci8
echo '!/dir' >> dvci8
echo '!/file' >> dvci8


for i in {0..8}
do
	echo '### START ###'
	echo 'ignore content:'
	cat dvci$i
	echo '----------------'
	cp dvci$i data/.dvcignore
	echo 'dvc results:' 
	python -c "from dvc.repo import Repo;print(list(Repo().tree.walk_files('data')))"
	

	# git test
	echo ''
	echo 'git results:'
	cp dvci$i data/.gitignore
	res=()
	for value in "data/1" "data/2" "data/3" "data/dir/file" "data/file"
	do
		if [[ ! $(git check-ignore $value) ]]; then
			res+="'$value', "
		fi
	done
	echo "[${res[@]}]"

	echo '### STOP ###'
	echo ''
	echo ''
done
```

And the two differences occur in following situations:
```
ignore content:
/*
!/dir/
!/file/
----------------
dvc results:
[]

git results:
['data/dir/file', ]
```
and 
```
ignore content:
/*
!dir/
!file/
----------------
dvc results:
[]

git results:
['data/dir/file', ]

```

I also compared the results from before latest dvc optimization, https://github.com/iterative/dvc/pull/3967 results were the same as for master.

So, in conclusion, I think its a bug.
I compared the result to #4120  and there is no difference.
@karajan1001, I think this issue is with the upstream library, as this issue looks similar to: https://github.com/cpburnz/python-path-specification/issues/19, but I haven't looked closely to verify. 
> @karajan1001, I think this issue is with the upstream library, as this issue looks similar to: [cpburnz/python-path-specification#19](https://github.com/cpburnz/python-path-specification/issues/19), but I haven't looked closely to verify.

@skshetry I agreed.

I had looked into the `PathSpec` code and the following result is weird to me. It is very different from the logic I learned from the `PathSpec` code.

ignore content:
/*
!dir/
!file/

----------------

git results:
['data/dir/file', ]

Commit message: Add one tests

Patch to review:
diff --git a/dvc/ignore.py b/dvc/ignore.py
index c9257e5204..35d7ede2d4 100644
--- a/dvc/ignore.py
+++ b/dvc/ignore.py
@@ -44,11 +44,11 @@ def __init__(self, ignore_file_path, tree):
 
     def __call__(self, root, dirs, files):
         files = [f for f in files if not self.matches(root, f)]
-        dirs = [d for d in dirs if not self.matches(root, d)]
+        dirs = [d for d in dirs if not self.matches(root, d, True)]
 
         return dirs, files
 
-    def matches(self, dirname, basename):
+    def matches(self, dirname, basename, is_dir=False):
         # NOTE: `relpath` is too slow, so we have to assume that both
         # `dirname` and `self.dirname` are relative or absolute together.
         prefix = self.dirname + os.sep
@@ -63,13 +63,19 @@ def matches(self, dirname, basename):
 
         if not System.is_unix():
             path = normalize_file(path)
-        return self.ignore(path)
+        return self.ignore(path, is_dir)
 
-    def ignore(self, path):
+    def ignore(self, path, is_dir):
         result = False
-        for ignore, pattern in self.ignore_spec:
-            if pattern.match(path):
-                result = ignore
+        if is_dir:
+            path_dir = f"{path}/"
+            for ignore, pattern in self.ignore_spec:
+                if pattern.match(path) or pattern.match(path_dir):
+                    result = ignore
+        else:
+            for ignore, pattern in self.ignore_spec:
+                if pattern.match(path):
+                    result = ignore
         return result
 
     def __hash__(self):
diff --git a/tests/func/test_ignore.py b/tests/func/test_ignore.py
index 03b1e95678..9de38fe619 100644
--- a/tests/func/test_ignore.py
+++ b/tests/func/test_ignore.py
@@ -173,3 +173,58 @@ def test_ignore_blank_line(tmp_dir, dvc):
     tmp_dir.gen(DvcIgnore.DVCIGNORE_FILE, "foo\n\ndir/ignored")
 
     assert _files_set("dir", dvc.tree) == {"dir/other"}
+
+
+# It is not possible to re-include a file if a parent directory of
+# that file is excluded.
+# Git doesn’t list excluded directories for performance reasons,
+# so any patterns on contained files have no effect,
+# no matter where they are defined.
+def test_ignore_file_in_parent_path(tmp_dir, dvc):
+    tmp_dir.gen({"dir": {"subdir": {"not_ignore": "121"}}})
+    tmp_dir.gen(DvcIgnore.DVCIGNORE_FILE, "subdir/*\n\n!not_ignore")
+    assert _files_set("dir", dvc.tree) == {
+        "dir/subdir/not_ignore",
+    }
+
+
+def test_ignore_parent_path(tmp_dir, dvc):
+    tmp_dir.gen({"dir": {"subdir": {"should_ignore": "121"}}})
+    tmp_dir.gen(DvcIgnore.DVCIGNORE_FILE, "subdir\n\n!should_ignore")
+    assert _files_set("dir", dvc.tree) == set()
+
+
+def test_ignore_parent_path_anotherform(tmp_dir, dvc):
+    tmp_dir.gen({"dir": {"subdir": {"should_ignore": "121"}}})
+    tmp_dir.gen(DvcIgnore.DVCIGNORE_FILE, "subdir/\n\n!should_ignore")
+    assert _files_set("dir", dvc.tree) == set()
+
+
+# If there is a separator at the end of the pattern then the pattern
+# will only match directories,
+# otherwise the pattern can match both files and directories.
+# For example, a pattern doc/frotz/ matches doc/frotz directory,
+# but not a/doc/frotz directory;
+def test_ignore_sub_directory(tmp_dir, dvc):
+    tmp_dir.gen(
+        {
+            "dir": {
+                "doc": {"fortz": {"b": "b"}},
+                "a": {"doc": {"fortz": {"a": "a"}}},
+            }
+        }
+    )
+    tmp_dir.gen({"dir": {DvcIgnore.DVCIGNORE_FILE: "doc/fortz"}})
+    assert _files_set("dir", dvc.tree) == {
+        "dir/a/doc/fortz/a",
+        "dir/{}".format(DvcIgnore.DVCIGNORE_FILE),
+    }
+
+
+# however frotz/ matches frotz and a/frotz that is a directory
+def test_ignore_directory(tmp_dir, dvc):
+    tmp_dir.gen({"dir": {"fortz": {}, "a": {"fortz": {}}}})
+    tmp_dir.gen({"dir": {DvcIgnore.DVCIGNORE_FILE: "fortz"}})
+    assert _files_set("dir", dvc.tree) == {
+        "dir/{}".format(DvcIgnore.DVCIGNORE_FILE),
+    }
diff --git a/tests/unit/test_ignore.py b/tests/unit/test_ignore.py
index 40f936d091..2b3b7cb1ca 100644
--- a/tests/unit/test_ignore.py
+++ b/tests/unit/test_ignore.py
@@ -14,34 +14,51 @@ def mock_dvcignore(dvcignore_path, patterns):
     return ignore_patterns
 
 
-def test_ignore_from_file_should_filter_dirs_and_files():
-    dvcignore_path = os.path.join(
-        os.path.sep, "full", "path", "to", "ignore", "file", ".dvcignore"
-    )
-
-    patterns = ["dir_to_ignore", "file_to_ignore"]
-
-    root = os.path.dirname(dvcignore_path)
-    dirs = ["dir1", "dir2", "dir_to_ignore"]
-    files = ["file1", "file2", "file_to_ignore"]
-
-    ignore = mock_dvcignore(dvcignore_path, patterns)
-    new_dirs, new_files = ignore(root, dirs, files)
-
-    assert {"dir1", "dir2"} == set(new_dirs)
-    assert {"file1", "file2"} == set(new_files)
-
-
 @pytest.mark.parametrize(
     "file_to_ignore_relpath, patterns,  expected_match",
     [
+        # all rules from https://git-scm.com/docs/gitignore
         ("to_ignore", ["to_ignore"], True),
+        ("dont_ignore.txt", ["dont_ignore"], False),
+        # A blank line matches no files, so it can serve as a separator for
+        # readability.
+        ("to_ignore", ["", "to_ignore"], True),
+        # A line starting with # serves as a comment.
+        # Put a backslash ("\") in front of the first hash for patterns
+        # that begin with a hash.
+        ("#to_ignore", ["\\#to_ignore"], True),
+        ("#to_ignore", ["#to_ignore"], False),
+        # Trailing spaces are ignored unless they are quoted with
+        # backslash ("\").
+        (" to_ignore", [" to_ignore"], False),
+        (" to_ignore", ["\\ to_ignore"], True),
+        # An optional prefix "!" which negates the pattern; any matching file
+        # excluded by a previous pattern will become included again.
         ("to_ignore.txt", ["to_ignore*"], True),
-        (
-            os.path.join("rel", "p", "p2", "to_ignore"),
-            ["rel/**/to_ignore"],
-            True,
-        ),
+        ("to_ignore.txt", ["to_ignore*", "!to_ignore.txt"], False),
+        ("to_ignore.txt", ["!to_ignore.txt", "to_ignore*"], True),
+        # It is not possible to re-include a file if a parent directory of
+        # that file is excluded.
+        # Git doesn’t list excluded directories for performance reasons,
+        # so any patterns on contained files have no effect,
+        # no matter where they are defined.
+        # see (`tests/func/test_ignore.py::test_ignore_parent_path`)
+        # Put a backslash ("\") in front of the first "!"
+        # for patterns that begin with a literal "!",
+        # for example, "\!important!.txt".
+        ("!to_ignore.txt", ["\\!to_ignore.txt"], True),
+        # The slash / is used as the directory separator.
+        # Separators may occur at the beginning, middle or end of the
+        # .gitignore search pattern.
+        # If there is a separator at the beginning or middle (or both)
+        # of the pattern, then the pattern is relative to the directory
+        # level of the particular .gitignore file itself.
+        # Otherwise the pattern may also match at any level below
+        # the .gitignore level.
+        ("file", ["/file"], True),
+        (os.path.join("data", "file"), ["/file"], False),
+        (os.path.join("data", "file"), ["data/file"], True),
+        (os.path.join("other", "data", "file"), ["data/file"], False),
         (
             os.path.join(
                 os.path.sep,
@@ -55,20 +72,94 @@ def test_ignore_from_file_should_filter_dirs_and_files():
             ["to_ignore"],
             True,
         ),
+        # If there is a separator at the end of the pattern then the pattern
+        # will only match directories,
+        # otherwise the pattern can match both files and directories.
+        # For example, a pattern doc/frotz/ matches doc/frotz directory,
+        # but not a/doc/frotz directory;
+        # see (`tests/func/test_ignore.py::test_ignore_sub_directory`)
+        # however frotz/ matches frotz and a/frotz that is a directory
+        # (all paths are relative from the .gitignore file).
+        # see (`tests/func/test_ignore.py::test_ignore_directory`)
+        # An asterisk "*" matches anything except a slash.
         ("to_ignore.txt", ["/*.txt"], True),
+        (os.path.join("path", "to_ignore.txt"), ["/*.txt"], False),
+        (os.path.join("data", "file.txt"), ["data/*"], True),
         (
             os.path.join("rel", "path", "path2", "to_ignore"),
             ["rel/*/to_ignore"],
             False,
         ),
-        (os.path.join("path", "to_ignore.txt"), ["/*.txt"], False),
+        ("file.txt", ["file.*"], True),
+        # The character "?" matches any one character except "/".
+        ("file.txt", ["fi?e.t?t"], True),
+        ("fi/e.txt", ["fi?e.t?t"], False),
+        # The range notation, e.g. [a-zA-Z], can be used
+        # to match one of the characters in a range. See fnmatch(3) and
+        # the FNM_PATHNAME flag for a more detailed description.
+        ("file.txt", ["[a-zA-Z]ile.txt"], True),
+        ("2ile.txt", ["[a-zA-Z]ile.txt"], False),
+        # Two consecutive asterisks ("**") in patterns matched against
+        # full pathname may have special meaning:
+        # A leading "**" followed by a slash means match in all directories.
+        # For example, "**/foo" matches file or directory "foo" anywhere, the
+        # same as pattern "foo".
+        # "**/foo/bar" matches file or directory "bar" anywhere that is
+        # directly under directory "foo".
+        (os.path.join("rel", "p", "p2", "to_ignore"), ["**/to_ignore"], True,),
+        (
+            os.path.join("rel", "p", "p2", "to_ignore"),
+            ["**/p2/to_ignore"],
+            True,
+        ),
+        (
+            os.path.join("rel", "path", "path2", "dont_ignore"),
+            ["**/to_ignore"],
+            False,
+        ),
+        # A trailing "/**" matches everything inside.
+        # For example, "abc/**" matches all files inside directory "abc",
+        # relative to the location of the .gitignore file, with infinite depth.
+        (os.path.join("rel", "p", "p2", "to_ignore"), ["rel/**"], True,),
+        (os.path.join("rel", "p", "p2", "to_ignore"), ["p/**"], False,),
+        (
+            os.path.join("rel", "path", "path2", "dont_ignore"),
+            ["rel/**"],
+            True,
+        ),
+        # A slash followed by two consecutive asterisks then a slash matches
+        # zero or more directories.
+        # For example, "a/**/b" matches "a/b", "a/x/b", "a/x/y/b" and so on.
+        (os.path.join("rel", "p", "to_ignore"), ["rel/**/to_ignore"], True,),
+        (
+            os.path.join("rel", "p", "p2", "to_ignore"),
+            ["rel/**/to_ignore"],
+            True,
+        ),
         (
             os.path.join("rel", "path", "path2", "dont_ignore"),
             ["rel/**/to_ignore"],
             False,
         ),
-        ("dont_ignore.txt", ["dont_ignore"], False),
-        ("dont_ignore.txt", ["dont*", "!dont_ignore.txt"], False),
+        (
+            os.path.join("rel", "path", "path2", "dont_ignore"),
+            ["path/**/dont_ignore"],
+            False,
+        ),
+        # Other consecutive asterisks are considered regular asterisks
+        # and will match according to the previous rules.
+        ("to_ignore.txt", ["/***.txt"], True),
+        (os.path.join("path", "to_ignore.txt"), ["/****.txt"], False),
+        (os.path.join("path", "to_ignore.txt"), ["****.txt"], True),
+        (os.path.join("data", "file.txt"), ["data/***"], True),
+        # bug from PathSpec
+        (os.path.join("data", "p", "file.txt"), ["data/***"], False),
+        (os.path.join("data", "p", "file.txt"), ["***/file.txt"], False),
+        (
+            os.path.join("rel", "path", "path2", "to_ignore"),
+            ["rel/***/to_ignore"],
+            False,
+        ),
     ],
 )
 def test_match_ignore_from_file(
@@ -99,18 +190,3 @@ def test_should_ignore_dir(omit_dir):
     new_dirs, _ = ignore(root, dirs, files)
 
     assert set(new_dirs) == {"dir1", "dir2"}
-
-
-def test_ignore_order():
-    dvcignore_path = os.path.join(os.path.sep, "ignore_order", ".dvcignore")
-
-    patterns = ["!ac*", "a*", "!ab*"]
-
-    root = os.path.dirname(dvcignore_path)
-    dirs = ["ignore_order"]
-    files = ["ac", "ab", "aa"]
-
-    ignore = mock_dvcignore(dvcignore_path, patterns)
-    _, new_files = ignore(root, dirs, files)
-
-    assert {"ab"} == set(new_files)


Code review comments:
diff_hunk: @@ -173,3 +173,58 @@ def test_ignore_blank_line(tmp_dir, dvc):
     tmp_dir.gen(DvcIgnore.DVCIGNORE_FILE, "foo\n\ndir/ignored")
 
     assert _files_set("dir", dvc.tree) == {"dir/other"}
+
+
+# It is not possible to re-include a file if a parent directory of
+# that file is excluded.
+# Git doesn’t list excluded directories for performance reasons,
+# so any patterns on contained files have no effect,
+# no matter where they are defined.
+def test_ignore_file_in_parent_path(tmp_dir, dvc):
+    tmp_dir.gen({"dir": {"subdir": {"not_ignore": "121"}}})
+    tmp_dir.gen(DvcIgnore.DVCIGNORE_FILE, "subdir/*\n\n!not_ignore")
+    assert _files_set("dir", dvc.tree) == {
+        "dir/subdir/not_ignore",
+    }
+
+
+def test_ignore_parent_path(tmp_dir, dvc):
+    tmp_dir.gen({"dir": {"subdir": {"should_ignore": "121"}}})
+    tmp_dir.gen(DvcIgnore.DVCIGNORE_FILE, "subdir\n\n!should_ignore")
+    assert _files_set("dir", dvc.tree) == set()
+
+
+def test_ignore_parent_path_anotherform(tmp_dir, dvc):
+    tmp_dir.gen({"dir": {"subdir": {"should_ignore": "121"}}})
+    tmp_dir.gen(DvcIgnore.DVCIGNORE_FILE, "subdir/\n\n!should_ignore")
+    assert _files_set("dir", dvc.tree) == set()
path: tests/func/test_ignore.py
text: @user1:
Maybe lets parametrize `.dvcignore` content here?