Repository: iterative/dvc

Issue:
dvcignore: not using tree
https://github.com/iterative/dvc/blob/0.66.6/dvc/ignore.py#L67
It is using os.walk, which means that no matter which branch we are checked into in the repo.tree, we are still using dvcignores from the current workspace. Need to make it use `repo.tree`. Probably by passing `self` instead of `self.root_dir` here https://github.com/iterative/dvc/blob/0.66.6/dvc/repo/__init__.py#L499 .

Discovered during the investigation for https://github.com/iterative/dvc/issues/2737 

Discussion:

Patch:
diff --git a/dvc/ignore.py b/dvc/ignore.py
index 3d111e868..fbfcff3ac 100644
--- a/dvc/ignore.py
+++ b/dvc/ignore.py
@@ -6,9 +6,7 @@ import os
 from pathspec import PathSpec
 from pathspec.patterns import GitWildMatchPattern
 
-from dvc.utils import dvc_walk
 from dvc.utils import relpath
-from dvc.utils.compat import open
 
 logger = logging.getLogger(__name__)
 
@@ -21,13 +19,13 @@ class DvcIgnore(object):
 
 
 class DvcIgnorePatterns(DvcIgnore):
-    def __init__(self, ignore_file_path):
+    def __init__(self, ignore_file_path, tree):
         assert os.path.isabs(ignore_file_path)
 
         self.ignore_file_path = ignore_file_path
         self.dirname = os.path.normpath(os.path.dirname(ignore_file_path))
 
-        with open(ignore_file_path, encoding="utf-8") as fobj:
+        with tree.open(ignore_file_path, encoding="utf-8") as fobj:
             self.ignore_spec = PathSpec.from_lines(GitWildMatchPattern, fobj)
 
     def __call__(self, root, dirs, files):
@@ -74,17 +72,18 @@ class DvcIgnoreDirs(DvcIgnore):
 
 
 class DvcIgnoreFilter(object):
-    def __init__(self, root_dir):
+    def __init__(self, root_dir, tree):
+        self.tree = tree
         self.ignores = {DvcIgnoreDirs([".git", ".hg", ".dvc"])}
         self._update(root_dir)
-        for root, dirs, _ in dvc_walk(root_dir, self):
+        for root, dirs, _ in self.tree.walk(root_dir, dvcignore=self):
             for d in dirs:
                 self._update(os.path.join(root, d))
 
     def _update(self, dirname):
         ignore_file_path = os.path.join(dirname, DvcIgnore.DVCIGNORE_FILE)
-        if os.path.exists(ignore_file_path):
-            self.ignores.add(DvcIgnorePatterns(ignore_file_path))
+        if self.tree.exists(ignore_file_path):
+            self.ignores.add(DvcIgnorePatterns(ignore_file_path, self.tree))
 
     def __call__(self, root, dirs, files):
         for ignore in self.ignores:
diff --git a/dvc/repo/__init__.py b/dvc/repo/__init__.py
index 19d862a24..e0003b808 100644
--- a/dvc/repo/__init__.py
+++ b/dvc/repo/__init__.py
@@ -496,7 +496,7 @@ class Repo(object):
 
     @cached_property
     def dvcignore(self):
-        return DvcIgnoreFilter(self.root_dir)
+        return DvcIgnoreFilter(self.root_dir, self.tree)
 
     def close(self):
         self.scm.close()


Test patch:
diff --git a/tests/func/test_ignore.py b/tests/func/test_ignore.py
index 6e38c2b02..b09a06a78 100644
--- a/tests/func/test_ignore.py
+++ b/tests/func/test_ignore.py
@@ -1,3 +1,4 @@
+import itertools
 import os
 import shutil
 
@@ -8,6 +9,7 @@ from dvc.ignore import DvcIgnore
 from dvc.ignore import DvcIgnoreDirs
 from dvc.ignore import DvcIgnoreFilter
 from dvc.ignore import DvcIgnorePatterns
+from dvc.scm.tree import WorkingTree
 from dvc.utils.compat import cast_bytes
 from dvc.utils.fs import get_mtime_and_size
 from tests.basic_env import TestDvc
@@ -148,7 +150,37 @@ def test_ignore_collecting_dvcignores(repo_dir, dname):
     )
     repo_dir.create(ignore_file, repo_dir.FOO)
 
-    assert DvcIgnoreFilter(repo_dir.root_dir).ignores == {
+    assert DvcIgnoreFilter(
+        repo_dir.root_dir, WorkingTree(repo_dir.root_dir)
+    ).ignores == {
         DvcIgnoreDirs([".git", ".hg", ".dvc"]),
-        DvcIgnorePatterns(top_ignore_file),
+        DvcIgnorePatterns(top_ignore_file, WorkingTree(repo_dir.root_dir)),
     }
+
+
+def test_ignore_on_branch(git, dvc_repo, repo_dir):
+    dvc_repo.add(repo_dir.DATA_DIR)
+    dvc_repo.scm.commit("add data dir")
+
+    branch_name = "branch_one"
+    dvc_repo.scm.checkout(branch_name, create_new=True)
+
+    repo_dir.create(DvcIgnore.DVCIGNORE_FILE, to_posixpath(repo_dir.DATA_SUB))
+    dvc_repo.scm.add([DvcIgnore.DVCIGNORE_FILE])
+    git.index.commit("add ignore")
+
+    dvc_repo.scm.checkout("master")
+
+    git_tree = dvc_repo.scm.get_tree(branch_name)
+    branch_data_files = set(
+        itertools.chain.from_iterable(
+            [
+                files
+                for _, _, files in dvc_repo.tree.walk(
+                    repo_dir.DATA_DIR,
+                    dvcignore=DvcIgnoreFilter(repo_dir.root_dir, git_tree),
+                )
+            ]
+        )
+    )
+    assert branch_data_files == {"data"}
diff --git a/tests/func/test_tree.py b/tests/func/test_tree.py
index 8b351f757..c2ae316c2 100644
--- a/tests/func/test_tree.py
+++ b/tests/func/test_tree.py
@@ -108,8 +108,8 @@ class AssertWalkEqualMixin(object):
 
 class TestWalkInNoSCM(AssertWalkEqualMixin, TestDir):
     def test(self):
-        dvcignore = DvcIgnoreFilter(self.root_dir)
         tree = WorkingTree(self._root_dir)
+        dvcignore = DvcIgnoreFilter(self.root_dir, tree)
         self.assertWalkEqual(
             tree.walk(self._root_dir, dvcignore=dvcignore),
             [
@@ -128,8 +128,8 @@ class TestWalkInNoSCM(AssertWalkEqualMixin, TestDir):
         )
 
     def test_subdir(self):
-        dvcignore = DvcIgnoreFilter(self.root_dir)
         tree = WorkingTree(self._root_dir)
+        dvcignore = DvcIgnoreFilter(self.root_dir, tree)
         self.assertWalkEqual(
             tree.walk(join("data_dir", "data_sub_dir"), dvcignore=dvcignore),
             [
@@ -145,7 +145,7 @@ class TestWalkInNoSCM(AssertWalkEqualMixin, TestDir):
 class TestWalkInGit(AssertWalkEqualMixin, TestGit):
     def test_nobranch(self):
         tree = WorkingTree(self._root_dir)
-        dvcignore = DvcIgnoreFilter(self._root_dir)
+        dvcignore = DvcIgnoreFilter(self._root_dir, tree)
         self.assertWalkEqual(
             tree.walk(".", dvcignore=dvcignore),
             [
diff --git a/tests/unit/test_ignore.py b/tests/unit/test_ignore.py
index 0eed6adc2..373f07424 100644
--- a/tests/unit/test_ignore.py
+++ b/tests/unit/test_ignore.py
@@ -1,20 +1,18 @@
 import os
 
 import pytest
+from mock import MagicMock
 from mock import mock_open
 from mock import patch
 
-import dvc
 from dvc.ignore import DvcIgnoreDirs
 from dvc.ignore import DvcIgnorePatterns
 
 
 def mock_dvcignore(dvcignore_path, patterns):
-
-    with patch.object(
-        dvc.ignore, "open", mock_open(read_data="\n".join(patterns))
-    ):
-        ignore_patterns = DvcIgnorePatterns(dvcignore_path)
+    tree = MagicMock()
+    with patch.object(tree, "open", mock_open(read_data="\n".join(patterns))):
+        ignore_patterns = DvcIgnorePatterns(dvcignore_path, tree)
 
     return ignore_patterns
 
diff --git a/tests/unit/utils/test_fs.py b/tests/unit/utils/test_fs.py
index 2cf460037..e5a18ba9c 100644
--- a/tests/unit/utils/test_fs.py
+++ b/tests/unit/utils/test_fs.py
@@ -7,6 +7,7 @@ from mock import patch
 import dvc
 from dvc.ignore import DvcIgnoreFilter
 from dvc.path_info import PathInfo
+from dvc.scm.tree import WorkingTree
 from dvc.system import System
 from dvc.utils import relpath
 from dvc.utils.compat import str
@@ -20,7 +21,7 @@ from tests.utils import spy
 
 class TestMtimeAndSize(TestDir):
     def test(self):
-        dvcignore = DvcIgnoreFilter(self.root_dir)
+        dvcignore = DvcIgnoreFilter(self.root_dir, WorkingTree(self.root_dir))
         file_time, file_size = get_mtime_and_size(self.DATA, dvcignore)
         dir_time, dir_size = get_mtime_and_size(self.DATA_DIR, dvcignore)
 
@@ -124,7 +125,9 @@ def test_get_inode(repo_dir):
 def test_path_object_and_str_are_valid_types_get_mtime_and_size(
     path, repo_dir
 ):
-    dvcignore = DvcIgnoreFilter(repo_dir.root_dir)
+    dvcignore = DvcIgnoreFilter(
+        repo_dir.root_dir, WorkingTree(repo_dir.root_dir)
+    )
     time, size = get_mtime_and_size(path, dvcignore)
     object_time, object_size = get_mtime_and_size(PathInfo(path), dvcignore)
     assert time == object_time