Repository: iterative/dvc

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


Discussion:
karajan1001: Two issues.

1. Other consecutive asterisks are considered regular asterisks and will match according to the previous rules.

> This is an issue of `PathSpec`.

2. 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.
> Because path didn't distinguish with files in `dvcignore`. And `dir/` can't match pathname `dir`.


efiop: @karajan1001 Sorry for the delay. This is outstanding work! Thank you so much for looking into this! 

> Other consecutive asterisks are considered regular asterisks and will match according to the previous rules.

Should we report it to pathspec and wait for the solution on their side? Or how do you think we should tackle it?

> 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.

>  Because paths didn't distinguish with files in dvcignore. So dir/ can't match pathname dir.

Nice catch! So that one is solved and we are doing it the same way gitignore does it, right?
pared: Seems like builds are failing.
karajan1001: > Should we report it to pathspec and wait for the solution on their side? Or how do you think we should tackle it?
> 
> >
@efiop 
I'd like to report it to `PathSpec` and fix it myself, but only after work could I do it.

> Nice catch! So that one is solved and we are doing it the same way gitignore does it, right?
Yes, I had tested with git If the pattern format description is complex or not clear.
![image](https://user-images.githubusercontent.com/6745454/86678000-b66f5680-c02e-11ea-96a9-8f9577d724b2.png)
![image](https://user-images.githubusercontent.com/6745454/86678054-c38c4580-c02e-11ea-811a-4d0685b5d888.png)
![image](https://user-images.githubusercontent.com/6745454/86678125-d4d55200-c02e-11ea-884d-adf5a256a857.png)
 

> Seems like builds are failing.
@pared  A bug from `pathspec` is waiting for fixing.




efiop: > I'd like to report it to PathSpec and fix it myself, but only after work could I do it.

@karajan1001 Sounds great! :pray: So maybe let's remove or comment-out that particular test case for now, so we could merge? 
karajan1001: > > I'd like to report it to PathSpec and fix it myself, but only after work could I do it.
> 
> @karajan1001 Sounds great! 🙏 So maybe let's remove or comment-out that particular test case for now, so we could merge?

There is still something wrong with triple asterisks,  According to the document.

> Other consecutive asterisks are considered regular asterisks and will match according to the previous rules.

What the result from my computer shows that triple asterisks act like double ones.

![image](https://user-images.githubusercontent.com/6745454/86688483-b6745400-c038-11ea-87e9-22461b7867f8.png)
![image](https://user-images.githubusercontent.com/6745454/86688702-ea4f7980-c038-11ea-9090-5e32b7ef22e9.png)
![image](https://user-images.githubusercontent.com/6745454/86688807-018e6700-c039-11ea-98af-c5834dcc6d65.png)

And I found another problem.`data/*` can match `data/1/2` just like `data/**`. 

Seems that `Git`, the documents of `gitignore`, and `Pathspec` go different ways. 

I don't know which is the standard ones now.
pared: Checked latest git master (2.26.0-rc1) and @karajan1001 is right, behavior of x/* is different that explicitly stated in gitignore docs. @karajan1001 that seems like a bug and potential contribution opportunity.

As to current build, I think we should comment out failing ones and wait till it gets done on PathSpec side. As to triple asterisk I would just leave it. If some will ever come to us with valid use case of triple asterisk other than 'match all', then we can consider it. There is no point in blindly following git docs.

Patch:
diff --git a/dvc/cli.py b/dvc/cli.py
index 70f8e0fdb..44af80d6a 100644
--- a/dvc/cli.py
+++ b/dvc/cli.py
@@ -80,34 +80,36 @@ COMMANDS = [
 ]
 
 
+def _find_parser(parser, cmd_cls):
+    defaults = parser._defaults  # pylint: disable=protected-access
+    if not cmd_cls or cmd_cls == defaults.get("func"):
+        parser.print_help()
+        raise DvcParserError()
+
+    actions = parser._actions  # pylint: disable=protected-access
+    for action in actions:
+        if not isinstance(action.choices, dict):
+            # NOTE: we are only interested in subparsers
+            continue
+        for subparser in action.choices.values():
+            _find_parser(subparser, cmd_cls)
+
+
 class DvcParser(argparse.ArgumentParser):
     """Custom parser class for dvc CLI."""
 
-    def error(self, message, command=None):  # pylint: disable=arguments-differ
-        """Custom error method.
-        Args:
-            message (str): error message.
-            command (str): subcommand name for help message
-        Raises:
-            dvc.exceptions.DvcParser: dvc parser exception.
-
-        """
+    def error(self, message, cmd_cls=None):  # pylint: disable=arguments-differ
         logger.error(message)
-        if command is not None:
-            for action in self._actions:
-                if action.dest == "cmd" and command in action.choices:
-                    subparser = action.choices[command]
-                    subparser.print_help()
-                    raise DvcParserError()
-        self.print_help()
-        raise DvcParserError()
+        _find_parser(self, cmd_cls)
 
-    # override this to send subcommand name to error method
     def parse_args(self, args=None, namespace=None):
+        # NOTE: overriding to provide a more granular help message.
+        # E.g. `dvc plots diff --bad-flag` would result in a `dvc plots diff`
+        # help message instead of generic `dvc` usage.
         args, argv = self.parse_known_args(args, namespace)
         if argv:
             msg = "unrecognized arguments: %s"
-            self.error(msg % " ".join(argv), args.cmd)
+            self.error(msg % " ".join(argv), getattr(args, "func", None))
         return args
 
 
diff --git a/dvc/ignore.py b/dvc/ignore.py
index c9257e520..35d7ede2d 100644
--- a/dvc/ignore.py
+++ b/dvc/ignore.py
@@ -44,11 +44,11 @@ class DvcIgnorePatterns(DvcIgnore):
 
     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 @@ class DvcIgnorePatterns(DvcIgnore):
 
         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):


Test patch:
diff --git a/tests/func/test_ignore.py b/tests/func/test_ignore.py
index 03b1e9567..ee9abd943 100644
--- a/tests/func/test_ignore.py
+++ b/tests/func/test_ignore.py
@@ -173,3 +173,66 @@ 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.
+@pytest.mark.parametrize(
+    "data_struct, pattern_list, result_set",
+    [
+        (
+            {"dir": {"subdir": {"not_ignore": "121"}}},
+            ["subdir/*", "!not_ignore"],
+            {"dir/subdir/not_ignore"},
+        ),
+        (
+            {"dir": {"subdir": {"should_ignore": "121"}}},
+            ["subdir", "!should_ignore"],
+            set(),
+        ),
+        (
+            {"dir": {"subdir": {"should_ignore": "121"}}},
+            ["subdir/", "!should_ignore"],
+            set(),
+        ),
+    ],
+)
+def test_ignore_file_in_parent_path(
+    tmp_dir, dvc, data_struct, pattern_list, result_set
+):
+    tmp_dir.gen(data_struct)
+    tmp_dir.gen(DvcIgnore.DVCIGNORE_FILE, "\n".join(pattern_list))
+    assert _files_set("dir", dvc.tree) == result_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 40f936d09..85c46edee 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,96 @@ 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),
+        # wait for Git
+        # (os.path.join("data", "sub", "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 +192,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)