brancher: only yield git revisions that contain repo_root (#3883)

* tests: add test for metrics show -T with pre-existing tags

* brancher: ignore git revisions that do not contain repo

* tests: update test to include cmd

* CleanTree: tree_root for subrepos should be the repo root, not scm root

* only force subrepo root for git trees

* fix windows shell quotes

dvc/ignore.py

@@ -115,8 +115,12 @@ class DvcIgnoreFilter:
 
 
 class CleanTree(BaseTree):
-    def __init__(self, tree):
+    def __init__(self, tree, tree_root=None):
         self.tree = tree
+        if tree_root:
+            self._tree_root = tree_root
+        else:
+            self._tree_root = self.tree.tree_root
 
     @cached_property
     def dvcignore(self):
@@ -124,7 +128,7 @@ class CleanTree(BaseTree):
 
     @property
     def tree_root(self):
-        return self.tree.tree_root
+        return self._tree_root
 
     def open(self, path, mode="r", encoding="utf-8"):
         if self.isfile(path):
@@ -146,8 +150,11 @@ class CleanTree(BaseTree):
         )
 
     def _valid_dirname(self, path):
-        dirname, basename = os.path.split(os.path.normpath(path))
-        dirs, _ = self.dvcignore(os.path.abspath(dirname), [basename], [])
+        path = os.path.abspath(path)
+        if path == self.tree_root:
+            return True
+        dirname, basename = os.path.split(path)
+        dirs, _ = self.dvcignore(dirname, [basename], [])
         if dirs:
             return True
         return False

dvc/repo/__init__.py

@@ -17,6 +17,7 @@ from dvc.exceptions import (
 from dvc.ignore import CleanTree
 from dvc.path_info import PathInfo
 from dvc.repo.tree import RepoTree
+from dvc.scm.tree import is_working_tree
 from dvc.utils.fs import path_isin
 
 from ..stage.exceptions import StageFileDoesNotExistError, StageNotFound
@@ -133,7 +134,13 @@ class Repo:
 
     @tree.setter
     def tree(self, tree):
-        self._tree = tree if isinstance(tree, CleanTree) else CleanTree(tree)
+        if is_working_tree(tree) or tree.tree_root == self.root_dir:
+            root = None
+        else:
+            root = self.root_dir
+        self._tree = (
+            tree if isinstance(tree, CleanTree) else CleanTree(tree, root)
+        )
         # Our graph cache is no longer valid, as it was based on the previous
         # tree.
         self._reset()

dvc/repo/brancher.py

@@ -48,6 +48,9 @@ def brancher(  # noqa: E302
         if revs:
             for sha, names in group_by(scm.resolve_rev, revs).items():
                 self.tree = scm.get_tree(sha)
-                yield ", ".join(names)
+                # ignore revs that don't contain repo root
+                # (i.e. revs from before a subdir=True repo was init'ed)
+                if self.tree.exists(self.root_dir):
+                    yield ", ".join(names)
     finally:
         self.tree = saved_tree

tests/func/metrics/test_show.py

@@ -1,5 +1,8 @@
+import os
+
 import pytest
 
+from dvc.repo import Repo
 from dvc.repo.metrics.show import NoMetricsError
 
 
@@ -56,3 +59,37 @@ def test_show_branch(tmp_dir, scm, dvc, run_copy_metrics):
         "working tree": {"metrics.yaml": {"foo": 1}},
         "branch": {"metrics.yaml": {"foo": 2}},
     }
+
+
+def test_show_subrepo_with_preexisting_tags(tmp_dir, scm):
+    tmp_dir.gen("foo", "foo")
+    scm.add("foo")
+    scm.commit("init")
+    scm.tag("no-metrics")
+
+    tmp_dir.gen({"subdir": {}})
+    subrepo_dir = tmp_dir / "subdir"
+    with subrepo_dir.chdir():
+        dvc = Repo.init(subdir=True)
+        scm.commit("init dvc")
+
+        dvc.run(
+            cmd="echo foo: 1 > metrics.yaml",
+            metrics=["metrics.yaml"],
+            single_stage=True,
+        )
+
+    scm.add(
+        [
+            str(subrepo_dir / "metrics.yaml"),
+            str(subrepo_dir / "metrics.yaml.dvc"),
+        ]
+    )
+    scm.commit("init metrics")
+    scm.tag("v1")
+
+    expected_path = os.path.join("subdir", "metrics.yaml")
+    assert dvc.metrics.show(all_tags=True) == {
+        "working tree": {expected_path: {"foo": 1}},
+        "v1": {expected_path: {"foo": 1}},
+    }