ignore: simplify and reuse same implementation across check-ignore and is_ignored API (#10856)

Author: skshetry

dvc/commands/check_ignore.py

@@ -1,31 +1,36 @@
+from typing import TYPE_CHECKING
+
 from dvc.cli import completion, formatter
 from dvc.cli.command import CmdBase
 from dvc.cli.utils import append_doc_link
 from dvc.ui import ui
 
+if TYPE_CHECKING:
+    from dvc.ignore import CheckIgnoreResult
+
 
 class CmdCheckIgnore(CmdBase):
     def __init__(self, args):
         super().__init__(args)
         self.ignore_filter = self.repo.dvcignore
 
-    def _show_results(self, result):
+    def _show_results(self, result: "CheckIgnoreResult"):
         if not result.match and not self.args.details:
             return
 
-        if not result.patterns and not self.args.non_matching:
-            return
-
         if self.args.details:
-            patterns = result.patterns or ["::"]
+            pattern_infos = result.pattern_infos
+            patterns = [str(pi) for pi in pattern_infos]
+            if not patterns and self.args.non_matching:
+                patterns = ["::"]
             if not self.args.all:
                 patterns = patterns[-1:]
 
             for pattern in patterns:
                 ui.write(pattern, result.file, sep="\t")
         else:
             ui.write(result.file)
-        return bool(result.patterns)
+        return bool(result.pattern_infos)
 
     def _check_one_file(self, target):
         result = self.ignore_filter.check_ignore(target)
@@ -77,6 +82,8 @@ def run(self):
 
 
 def add_parser(subparsers, parent_parser):
+    import argparse
+
     ADD_HELP = "Check whether files or directories are excluded due to `.dvcignore`."
 
     parser = subparsers.add_parser(
@@ -94,14 +101,7 @@ def add_parser(subparsers, parent_parser):
         help="Show the exclude patterns along with each target path.",
     )
     parser.add_argument(
-        "-a",
-        "--all",
-        action="store_true",
-        default=False,
-        help=(
-            "Include the target paths which don't match any pattern "
-            "in the `--details` list."
-        ),
+        "-a", "--all", action="store_true", default=False, help=argparse.SUPPRESS
     )
     parser.add_argument(
         "-n",

dvc/ignore.py

@@ -1,3 +1,4 @@
+import functools
 import os
 import re
 from collections.abc import Iterable, Iterator
@@ -41,21 +42,58 @@ def __init__(
         ]
 
         self.sep = sep
-        self.pattern_list: list[PatternInfo] = pattern_infos
+        self.pattern_list: list[PatternInfo] = []
         self.dirname = dirname
+        self.find_matching_pattern = functools.cache(self._find_matching_pattern)
 
-        self.regex_pattern_list: list[tuple[str, bool]] = []
-        for count, pattern in enumerate(pattern_infos):
-            regex, ignore = GitWildMatchPattern.pattern_to_regex(pattern.patterns)
+        regex_pattern_list: list[tuple[str, bool, bool, PatternInfo]] = []
+        for count, pattern_info in enumerate(pattern_infos):
+            regex, ignore = GitWildMatchPattern.pattern_to_regex(pattern_info.patterns)
             if regex is not None and ignore is not None:
+                self.pattern_list.append(pattern_info)
                 regex = regex.replace(f"<{_DIR_MARK}>", f"<{_DIR_MARK}{count}>")
-                self.regex_pattern_list.append((regex, ignore))
+                regex_pattern_list.append(
+                    (regex, ignore, pattern_info.patterns.endswith("/"), pattern_info)
+                )
+
+        def keyfunc(item: tuple[str, bool, bool, PatternInfo]) -> tuple[bool, bool]:
+            _, ignore, dir_only_pattern, _ = item
+            return ignore, dir_only_pattern
 
-        self.ignore_spec = [
-            (ignore, re.compile("|".join(regex for regex, _ in group)))
-            for ignore, group in groupby(self.regex_pattern_list, lambda x: x[1])
-            if ignore is not None
+        self.ignore_spec: list[
+            tuple[
+                re.Pattern[str],
+                bool,
+                bool,
+                dict[Optional[str], tuple[str, PatternInfo]],
+            ]
         ]
+        self.ignore_spec = []
+        for (ignore, dir_only_pattern), group in groupby(
+            regex_pattern_list, key=keyfunc
+        ):
+            if ignore:
+                # For performance, we combine all exclude patterns.
+                # But we still need to figure out which pattern matched which rule,
+                # (eg: to show in `dvc check-ignore`).
+                # So, we use named groups and keep a map of group name to pattern.
+                pattern_map: dict[Optional[str], tuple[str, PatternInfo]] = {
+                    f"rule_{i}": (regex, pi)
+                    for i, (regex, _, _, pi) in enumerate(group)
+                }
+                combined_regex = "|".join(
+                    f"(?P<{name}>{regex})" for name, (regex, _) in pattern_map.items()
+                )
+                self.ignore_spec.append(
+                    (re.compile(combined_regex), ignore, dir_only_pattern, pattern_map)
+                )
+            else:
+                # unignored patterns are not combined with `|`.
+                for regex, _, _, pi in group:
+                    pattern_map = {None: (regex, pi)}
+                    self.ignore_spec.append(
+                        (re.compile(regex), ignore, dir_only_pattern, pattern_map)
+                    )
 
     @classmethod
     def from_file(cls, path: str, fs: "FileSystem", name: str) -> "Self":
@@ -113,60 +151,78 @@ def matches(
         basename: str,
         is_dir: bool = False,
         details: Literal[True] = ...,
-    ) -> tuple[bool, list[str]]: ...
+    ) -> tuple[bool, list[PatternInfo]]: ...
 
     @overload
     def matches(
-        self, dirname: str, basename: str, is_dir: bool = False, details: bool = False
-    ) -> Union[bool, tuple[bool, list[str]]]: ...
+        self,
+        dirname: str,
+        basename: str,
+        is_dir: bool = False,
+        details: bool = False,
+    ) -> Union[bool, tuple[bool, list[PatternInfo]]]: ...
 
     def matches(
-        self, dirname: str, basename: str, is_dir: bool = False, details: bool = False
-    ) -> Union[bool, tuple[bool, list[str]]]:
+        self,
+        dirname: str,
+        basename: str,
+        is_dir: bool = False,
+        details: bool = False,
+    ) -> Union[bool, tuple[bool, list[PatternInfo]]]:
         path = self._get_normalize_path(dirname, basename)
-        if not path:
-            return (False, []) if details else False
-        if details:
-            return self._ignore_details(path, is_dir)
-        return self.ignore(path, is_dir)
-
-    def ignore(self, path: str, is_dir: bool) -> bool:
-        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
-
-        for ignore, pattern in self.ignore_spec[::-1]:
-            if matches(pattern, path, is_dir):
-                result = ignore
-                break
-        return result
-
-    def _ignore_details(self, path: str, is_dir: bool) -> tuple[bool, list[str]]:
         result = False
-        matched_patterns = []
-        for (regex, ignore), pattern_info in list(
-            zip(self.regex_pattern_list, self.pattern_list)
+        _match: list[PatternInfo] = []
+        if path:
+            result, _match = self._ignore(path, is_dir)
+        return (result, _match) if details else result
+
+    def _find_matching_pattern(
+        self, path: str, is_dir: bool
+    ) -> tuple[bool, list[PatternInfo]]:
+        paths = [path]
+        if is_dir and not path.endswith("/"):
+            paths.append(f"{path}/")
+
+        for pattern, ignore, dir_only_pattern, pattern_map in reversed(
+            self.ignore_spec
         ):
-            # skip system pattern
-            if not pattern_info.file_info:
+            if dir_only_pattern and not is_dir:
                 continue
-
-            pattern = re.compile(regex)
-
-            matches = bool(pattern.match(path))
-            if is_dir:
-                matches |= bool(pattern.match(f"{path}/"))
-
-            if matches:
-                matched_patterns.append(pattern_info.file_info)
-                result = ignore
-        return result, matched_patterns
+            for p in paths:
+                match = pattern.match(p)
+                if not match:
+                    continue
+                if ignore:
+                    group_name, _match = next(
+                        (
+                            (name, _match)
+                            for name, _match in match.groupdict().items()
+                            if name.startswith("rule_") and _match is not None
+                        )
+                    )
+                else:
+                    # unignored patterns are not combined with `|`,
+                    # so there are no groups.
+                    group_name = None
+                _regex, pattern_info = pattern_map[group_name]
+                return ignore, [pattern_info]
+        return False, []
+
+    def _ignore(self, path: str, is_dir: bool) -> tuple[bool, list[PatternInfo]]:
+        parts = path.split("/")
+        result = False
+        matches: list[PatternInfo] = []
+        for i in range(1, len(parts) + 1):
+            rel_path = "/".join(parts[:i])
+            result, _matches = self.find_matching_pattern(
+                rel_path, is_dir or i < len(parts)
+            )
+            if i < len(parts) and not result:
+                continue
+            matches.extend(_matches)
+            if result:
+                break
+        return result, matches
 
     def __hash__(self) -> int:
         return hash(self.dirname + ":" + str(self.pattern_list))
@@ -186,7 +242,7 @@ def __bool__(self) -> bool:
 class CheckIgnoreResult(NamedTuple):
     file: str
     match: bool
-    patterns: list[str]
+    pattern_infos: list[PatternInfo]
 
 
 class DvcIgnoreFilter:
@@ -454,14 +510,14 @@ def check_ignore(self, target: str) -> CheckIgnoreResult:
         # NOTE: can only be used in `dvc check-ignore`, see
         # https://github.com/iterative/dvc/issues/5046
         full_target = self.fs.abspath(target)
-        matched_patterns: list[str] = []
+        matched_patterns: list[PatternInfo] = []
         ignore = False
         if not self._outside_repo(full_target):
             dirname, basename = self.fs.split(self.fs.normpath(full_target))
             pattern = self._get_trie_pattern(dirname)
             if pattern:
                 ignore, matched_patterns = pattern.matches(
-                    dirname, basename, self.fs.isdir(full_target), True
+                    dirname, basename, self.fs.isdir(full_target), details=True
                 )
         return CheckIgnoreResult(target, ignore, matched_patterns)
 

dvc/output.py

@@ -45,7 +45,7 @@
     from dvc_data.hashfile.obj import HashFile
     from dvc_data.index import DataIndexKey
 
-    from .ignore import DvcIgnoreFilter
+    from .ignore import CheckIgnoreResult, DvcIgnoreFilter
 
 logger = logger.getChild(__name__)
 
@@ -267,9 +267,9 @@ def __init__(self, path):
 
 
 class OutputIsIgnoredError(DvcException):
-    def __init__(self, match):
-        lines = "\n".join(match.patterns)
-        super().__init__(f"Path '{match.file}' is ignored by\n{lines}")
+    def __init__(self, result: "CheckIgnoreResult"):
+        pi = result.pattern_infos[-1]
+        super().__init__(f"Path {result.file!r} is ignored by {pi!s}")
 
 
 class CheckoutCallback(TqdmCallback):
@@ -1203,8 +1203,9 @@ def _validate_output_path(self, path, stage=None):
         if stage:
             abs_path = os.path.join(stage.wdir, path)
             if self._is_path_dvcignore(abs_path):
-                check = stage.repo.dvcignore.check_ignore(abs_path)
-                raise self.IsIgnoredError(check)
+                check: CheckIgnoreResult = stage.repo.dvcignore.check_ignore(abs_path)
+                if check.match:
+                    raise self.IsIgnoredError(check)
 
     def _check_can_merge(self, out):
         if self.protocol != out.protocol:

dvc/pathspec_math.py

@@ -13,6 +13,9 @@ class PatternInfo(NamedTuple):
     patterns: str
     file_info: str
 
+    def __str__(self) -> str:
+        return self.file_info or f":{self.patterns}"
+
 
 def _not_ignore(rule):
     return (True, rule[1:]) if rule.startswith("!") else (False, rule)

tests/func/test_check_ignore.py

@@ -78,7 +78,7 @@ def test_check_ignore_dir(tmp_dir, dvc, path, ret):
 
 
 def test_check_ignore_default_dir(tmp_dir, dvc):
-    assert main(["check-ignore", "-q", ".dvc"]) == 1
+    assert main(["check-ignore", "-q", ".dvc"]) == 0
 
 
 def test_check_ignore_out_side_repo(tmp_dir, dvc):
@@ -125,7 +125,6 @@ def test_check_ignore_details_all(tmp_dir, dvc, capsys):
 
     assert main(["check-ignore", "-d", "-a", "foo"]) == 0
     out, _ = capsys.readouterr()
-    assert f"{DvcIgnore.DVCIGNORE_FILE}:1:f*\tfoo\n" in out
     assert f"{DvcIgnore.DVCIGNORE_FILE}:2:!foo\tfoo\n" in out