dvcignore: Fix incorrect ignored output computation (#4986)

* dvcignore: Fix incorrect ignored output computation

* dvcignore: Fixed unignored patterns

Unignored patterns were shown in the dvc status output and caused Error.

Also made a bit of a refactoring in `matches` and `ignore`.

* dvcignore: Fix functional tests

Not sure if I follow, but tests assume that unignored files should trigger `check-ignore`. What was the intention?

* dvcignore: Fix stage.wdir being MagicMock

* Use `is_ignored` instead of `check_ignore`

1. use is_ignored to judge DVC ignore status.
2. use check_ignore to show ignore patterns.

* Solve tests fails

* Add a new tests

* Return to the old check_ignore logic.

* Pass the pylint

* Simplify ignore tests

Co-authored-by: karajan1001 <[email protected]>

Author: anotherbugmaster

dvc/ignore.py

@@ -91,45 +91,50 @@ def _get_normalize_path(self, dirname, basename):
             path = normalize_file(path)
         return path
 
-    def matches(self, dirname, basename, is_dir=False):
+    def matches(self, dirname, basename, is_dir=False, details: bool = False):
         path = self._get_normalize_path(dirname, basename)
         if not path:
             return False
+
+        if details:
+            return self._ignore_details(path, is_dir)
         return self.ignore(path, is_dir)
 
     def ignore(self, path, is_dir):
+        def matches(pattern, path, is_dir) -> bool:
+            matches_ = bool(pattern.match(path))
+
+            if is_dir:
+                matches_ |= bool(pattern.match(f"{path}/"))
+
+            return matches_
+
         result = False
-        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 match_details(self, dirname, basename, is_dir=False):
-        path = self._get_normalize_path(dirname, basename)
-        if not path:
-            return False
-        return self._ignore_details(path, is_dir)
+        for ignore, pattern in self.ignore_spec[::-1]:
+            if matches(pattern, path, is_dir):
+                result = ignore
+                break
+        return result
 
-    def _ignore_details(self, path, is_dir):
+    def _ignore_details(self, path, is_dir: bool):
         result = []
-        for ignore, pattern in zip(self.regex_pattern_list, self.pattern_list):
-            regex = re.compile(ignore[0])
+        for (regex, _), pattern_info in list(
+            zip(self.regex_pattern_list, self.pattern_list)
+        ):
             # skip system pattern
-            if not pattern.file_info:
+            if not pattern_info.file_info:
                 continue
+
+            regex = re.compile(regex)
+
+            matches = bool(regex.match(path))
             if is_dir:
-                path_dir = f"{path}/"
-                if regex.match(path) or regex.match(path_dir):
-                    result.append(pattern.file_info)
-            else:
-                if regex.match(path):
-                    result.append(pattern.file_info)
+                matches |= bool(regex.match(f"{path}/"))
+
+            if matches:
+                result.append(pattern_info.file_info)
+
         return result
 
     def __hash__(self):
@@ -323,13 +328,15 @@ def _outside_repo(self, path):
         return False
 
     def check_ignore(self, target):
+        # NOTE: can only be used in `dvc check-ignore`, see
+        # https://github.com/iterative/dvc/issues/5046
         full_target = os.path.abspath(target)
         if not self._outside_repo(full_target):
             dirname, basename = os.path.split(os.path.normpath(full_target))
             pattern = self._get_trie_pattern(dirname)
             if pattern:
-                matches = pattern.match_details(
-                    dirname, basename, os.path.isdir(full_target)
+                matches = pattern.matches(
+                    dirname, basename, os.path.isdir(full_target), True,
                 )
 
                 if matches:
@@ -339,7 +346,11 @@ def check_ignore(self, target):
     def is_ignored(self, path):
         # NOTE: can't use self.check_ignore(path).match for now, see
         # https://github.com/iterative/dvc/issues/4555
-        return self.is_ignored_dir(path) or self.is_ignored_file(path)
+        if os.path.isfile(path):
+            return self.is_ignored_file(path)
+        if os.path.isdir(path):
+            return self.is_ignored_dir(path)
+        return self.is_ignored_file(path) or self.is_ignored_dir(path)
 
 
 def init(path):

dvc/output/base.py

@@ -559,8 +559,9 @@ def _validate_output_path(cls, path, stage=None):
             raise cls.IsStageFileError(path)
 
         if stage:
-            check = stage.repo.tree.dvcignore.check_ignore(path)
-            if check.match:
+            abs_path = os.path.join(stage.wdir, path)
+            if stage.repo.tree.dvcignore.is_ignored(abs_path):
+                check = stage.repo.tree.dvcignore.check_ignore(abs_path)
                 raise cls.IsIgnoredError(check)
 
     def _check_can_merge(self, out):

tests/dir_helpers.py

@@ -332,10 +332,14 @@ def run_copy(tmp_dir, dvc):
     )
 
     def run_copy(src, dst, **run_kwargs):
+        wdir = pathlib.Path(run_kwargs.get("wdir", "."))
+        wdir = pathlib.Path("../" * len(wdir.parts))
+        script_path = wdir / "copy.py"
+
         return dvc.run(
-            cmd=f"python copy.py {src} {dst}",
+            cmd=f"python {script_path} {src} {dst}",
             outs=[dst],
-            deps=[src, "copy.py"],
+            deps=[src, f"{script_path}"],
             **run_kwargs,
         )
 

tests/func/test_ignore.py

@@ -1,5 +1,6 @@
 import os
 import shutil
+from pathlib import Path
 
 import pytest
 
@@ -404,7 +405,17 @@ def test_ignore_in_added_dir(tmp_dir, dvc):
 
 
 def test_ignored_output(tmp_dir, scm, dvc, run_copy):
-    tmp_dir.gen({".dvcignore": "*.log", "foo": "foo content"})
+    tmp_dir.gen({".dvcignore": "*.log\n!foo.log", "foo": "foo content"})
 
     with pytest.raises(OutputIsIgnoredError):
-        run_copy("foo", "foo.log", name="copy")
+        run_copy("foo", "abc.log", name="copy")
+
+    run_copy("foo", "foo.log", name="copy")
+
+
+def test_ignored_output_nested(tmp_dir, scm, dvc, run_copy):
+    tmp_dir.gen({".dvcignore": "/*.log", "copy": {"foo": "foo content"}})
+
+    run_copy("foo", "foo.log", name="copy", wdir="copy")
+
+    assert Path("copy/foo.log").exists()

tests/unit/output/test_output.py

@@ -73,6 +73,10 @@ def test_get_used_cache(exists, expected_message, mocker, caplog):
     stage = mocker.MagicMock()
     mocker.patch.object(stage, "__str__", return_value="stage: 'stage.dvc'")
     mocker.patch.object(stage, "addressing", "stage.dvc")
+    mocker.patch.object(stage, "wdir", ".")
+    mocker.patch.object(
+        stage.repo.tree.dvcignore, "is_ignored", return_value=False,
+    )
     mocker.patch.object(
         stage.repo.tree.dvcignore,
         "check_ignore",