From b14a972cd057a5fcf6dfbee5da475e30c1f36bec Mon Sep 17 00:00:00 2001
From: Ruslan Kuprieiev <kupruser@gmail.com>
Date: Sat, 7 May 2022 00:03:47 +0300
Subject: [PATCH] repofs: use / as root_marker

This makes dvcfs/repofs much easier to work with and allows it to support
isabs/abspath/etc correctly, as previously any full path (with root_marker "")
would be impossible to distinguish from a relative one.

This also detaches external workspaces from the main repo workspace in
DvcFileSystem, which was necessary to be able to distinguish dvcfs abspaths
from local external output paths. Note that currently we still have limited
support for those (e.g. get/api supports them, but import doesn't) and this
change keeps it that way.
---
 dvc/api.py                      |  3 ++-
 dvc/fs/base.py                  |  5 ++++-
 dvc/fs/dvc.py                   | 37 +++++++++++++++++++++------------
 dvc/fs/git.py                   |  6 ++++++
 dvc/fs/path.py                  |  6 +++++-
 dvc/fs/repo.py                  | 27 +++++++++++++-----------
 dvc/repo/__init__.py            | 14 +++++++++++--
 dvc/repo/collect.py             |  8 +++----
 dvc/repo/diff.py                |  3 ++-
 dvc/repo/get.py                 | 16 ++++++++++++--
 dvc/repo/ls.py                  |  4 +++-
 dvc/repo/metrics/show.py        | 10 ++++++---
 dvc/repo/plots/__init__.py      |  5 ++++-
 tests/unit/fs/test_dvc.py       |  1 +
 tests/unit/fs/test_repo.py      | 24 ++++++++++-----------
 tests/unit/fs/test_repo_info.py |  7 -------
 16 files changed, 115 insertions(+), 61 deletions(-)

diff --git a/dvc/api.py b/dvc/api.py
index 9db246ad3..d668d3b6c 100644
--- a/dvc/api.py
+++ b/dvc/api.py
@@ -19,8 +19,9 @@ def get_url(path, repo=None, rev=None, remote=None):
     directory in the remote storage.
     """
     with Repo.open(repo, rev=rev, subrepos=True, uninitialized=True) as _repo:
+        fs_path = _repo.repo_fs.from_os_path(path)
         with reraise(FileNotFoundError, PathMissingError(path, repo)):
-            info = _repo.repo_fs.info(path)
+            info = _repo.repo_fs.info(fs_path)
 
         dvc_info = info.get("dvc_info")
         if not dvc_info:
diff --git a/dvc/fs/base.py b/dvc/fs/base.py
index edea3c8bd..c733f39ee 100644
--- a/dvc/fs/base.py
+++ b/dvc/fs/base.py
@@ -83,7 +83,10 @@ class FileSystem:
     def path(self):
         from .path import Path
 
-        return Path(self.sep)
+        def _getcwd():
+            return self.fs.root_marker
+
+        return Path(self.sep, getcwd=_getcwd)
 
     @classmethod
     def _strip_protocol(cls, path: str) -> str:
diff --git a/dvc/fs/dvc.py b/dvc/fs/dvc.py
index e7218e3ec..c6000952f 100644
--- a/dvc/fs/dvc.py
+++ b/dvc/fs/dvc.py
@@ -1,5 +1,4 @@
 import logging
-import os
 import threading
 import typing
 
@@ -22,31 +21,43 @@ class _DvcFileSystem(AbstractFileSystem):  # pylint:disable=abstract-method
         repo: DVC repo.
     """
 
-    def __init__(self, **kwargs):
+    root_marker = "/"
+
+    def __init__(self, workspace=None, **kwargs):
         super().__init__(**kwargs)
         self.repo = kwargs["repo"]
+        self.workspace = workspace or "repo"
+
+    @cached_property
+    def path(self):
+        from .path import Path
+
+        def _getcwd():
+            return self.root_marker
+
+        return Path(self.sep, getcwd=_getcwd)
 
     @property
     def config(self):
         raise NotImplementedError
 
     def _get_key(self, path):
-        from dvc.fs.local import LocalFileSystem
+        if self.workspace != "repo":
+            from . import get_cloud_fs
 
-        from . import get_cloud_fs
-
-        cls, kwargs, fs_path = get_cloud_fs(None, url=path)
-
-        if cls != LocalFileSystem or os.path.isabs(path):
+            cls, kwargs, fs_path = get_cloud_fs(None, url=path)
             fs = cls(**kwargs)
-            return (cls.scheme, *fs.path.parts(fs_path))
+            return (self.workspace, *fs.path.parts(fs_path))
 
-        fs_key = "repo"
-        key = path.split(self.sep)
-        if key == ["."] or key == [""]:
+        path = self.path.abspath(path)
+        if path == self.root_marker:
+            return (self.workspace,)
+
+        key = self.path.relparts(path, self.root_marker)
+        if key == (".") or key == (""):
             key = ()
 
-        return (fs_key, *key)
+        return (self.workspace, *key)
 
     def _get_fs_path(self, path: "AnyPath", remote=None):
         from dvc.config import NoRemoteError
diff --git a/dvc/fs/git.py b/dvc/fs/git.py
index c3ff051f1..4eab7b098 100644
--- a/dvc/fs/git.py
+++ b/dvc/fs/git.py
@@ -40,6 +40,12 @@ class GitFileSystem(FileSystem):  # pylint:disable=abstract-method
             }
         )
 
+    @cached_property
+    def path(self):
+        from .path import Path
+
+        return Path(self.sep, getcwd=os.getcwd)
+
     @wrap_prop(threading.Lock())
     @cached_property
     def fs(self) -> "FsspecGitFileSystem":
diff --git a/dvc/fs/path.py b/dvc/fs/path.py
index eaf20f416..9e734eac9 100644
--- a/dvc/fs/path.py
+++ b/dvc/fs/path.py
@@ -105,7 +105,11 @@ class Path:
         return self.isin_or_eq(left, right) or self.isin(right, left)
 
     def relpath(self, path, start):
-        return self.flavour.relpath(path, start=start)
+        if not start:
+            start = "."
+        return self.flavour.relpath(
+            self.abspath(path), start=self.abspath(start)
+        )
 
     def relparts(self, path, base):
         return self.parts(self.relpath(path, base))
diff --git a/dvc/fs/repo.py b/dvc/fs/repo.py
index a5da9a1fe..8d1977e4f 100644
--- a/dvc/fs/repo.py
+++ b/dvc/fs/repo.py
@@ -73,6 +73,8 @@ class _RepoFileSystem(AbstractFileSystem):  # pylint:disable=abstract-method
         kwargs: Additional keyword arguments passed to the `DvcFileSystem()`.
     """
 
+    root_marker = "/"
+
     PARAM_REPO_URL = "repo_url"
     PARAM_REPO_ROOT = "repo_root"
     PARAM_REV = "rev"
@@ -103,7 +105,10 @@ class _RepoFileSystem(AbstractFileSystem):  # pylint:disable=abstract-method
         else:
             self.repo_factory = repo_factory
 
-        self.path = Path(self.sep)
+        def _getcwd():
+            return self.root_marker
+
+        self.path = Path(self.sep, getcwd=_getcwd)
         self.repo = repo
         self.hash_jobs = repo.fs.hash_jobs
         self._traverse_subrepos = subrepos
@@ -258,17 +263,7 @@ class _RepoFileSystem(AbstractFileSystem):  # pylint:disable=abstract-method
         """
         Returns a pair of fss based on repo the path falls in, using prefix.
         """
-        from dvc.utils import as_posix
-
-        if os.path.isabs(path):
-            if self.repo.fs.path.isin_or_eq(path, self.repo.root_dir):
-                path = self.repo.fs.path.relpath(path, self.repo.root_dir)
-            else:
-                return None, None, self.repo.dvcfs, path
-
-        path = as_posix(path)
-
-        parts = self.path.parts(path)
+        parts = self.path.relparts(path, self.root_marker)
         if parts and parts[0] == os.curdir:
             parts = parts[1:]
 
@@ -461,3 +456,11 @@ class RepoFileSystem(FileSystem):
     @property
     def config(self):
         return self.fs.config
+
+    def from_os_path(self, path):
+        from dvc.utils import as_posix
+
+        if os.path.isabs(path):
+            path = os.path.relpath(path, self.repo.root_dir)
+
+        return as_posix(path)
diff --git a/dvc/repo/__init__.py b/dvc/repo/__init__.py
index e807d8c01..7491df609 100644
--- a/dvc/repo/__init__.py
+++ b/dvc/repo/__init__.py
@@ -491,12 +491,22 @@ class Repo:
     @contextmanager
     def open_by_relpath(self, path, remote=None, mode="r", encoding=None):
         """Opens a specified resource as a file descriptor"""
+        from dvc.fs.dvc import DvcFileSystem
         from dvc.fs.repo import RepoFileSystem
 
-        fs = RepoFileSystem(repo=self, subrepos=True)
+        if os.path.isabs(path):
+            fs = DvcFileSystem(repo=self, workspace="local")
+            fs_path = path
+        else:
+            fs = RepoFileSystem(repo=self, subrepos=True)
+            fs_path = fs.from_os_path(path)
+
         try:
             with fs.open(
-                path, mode=mode, encoding=encoding, remote=remote
+                fs_path,
+                mode=mode,
+                encoding=encoding,
+                remote=remote,
             ) as fobj:
                 yield fobj
         except FileNotFoundError as exc:
diff --git a/dvc/repo/collect.py b/dvc/repo/collect.py
index a73b1b6b2..bc74dfd32 100644
--- a/dvc/repo/collect.py
+++ b/dvc/repo/collect.py
@@ -1,5 +1,4 @@
 import logging
-import os
 from typing import TYPE_CHECKING, Callable, Iterable, List, Tuple
 
 from dvc.types import AnyPath
@@ -34,8 +33,8 @@ def _collect_paths(
     from dvc.fs.repo import RepoFileSystem
     from dvc.utils import relpath
 
-    fs_paths = [os.path.abspath(target) for target in targets]
     fs = RepoFileSystem(repo=repo)
+    fs_paths = [fs.from_os_path(target) for target in targets]
 
     target_paths: StrPaths = []
     for fs_path in fs_paths:
@@ -60,10 +59,11 @@ def _filter_duplicates(
     fs_res_paths = fs_paths
 
     for out in outs:
-        if out.fs_path in fs_paths:
+        fs_path = out.repo.repo_fs.from_os_path(out.fs_path)
+        if fs_path in fs_paths:
             res_outs.append(out)
             # MUTATING THE SAME LIST!!
-            fs_res_paths.remove(out.fs_path)
+            fs_res_paths.remove(fs_path)
 
     return res_outs, fs_res_paths
 
diff --git a/dvc/repo/diff.py b/dvc/repo/diff.py
index 44aa3dd3e..176b05903 100644
--- a/dvc/repo/diff.py
+++ b/dvc/repo/diff.py
@@ -186,8 +186,9 @@ def _dir_output_paths(fs, fs_path, obj, targets=None):
 
 def _filter_missing(repo_fs, paths):
     for path in paths:
+        fs_path = repo_fs.from_os_path(path)
         try:
-            info = repo_fs.info(path)
+            info = repo_fs.info(fs_path)
             dvc_info = info.get("dvc_info")
             if (
                 dvc_info
diff --git a/dvc/repo/get.py b/dvc/repo/get.py
index 55a4f218a..6373f429c 100644
--- a/dvc/repo/get.py
+++ b/dvc/repo/get.py
@@ -50,14 +50,26 @@ def get(url, path, out=None, rev=None, jobs=None):
         with external_repo(
             url=url, rev=rev, cache_dir=tmp_dir, cache_types=cache_types
         ) as repo:
-            fs = repo.repo_fs
+
+            if os.path.isabs(path):
+                from dvc.fs.dvc import DvcFileSystem
+
+                fs = DvcFileSystem(repo=repo, workspace="local")
+                fs_path = path
+            else:
+                fs = repo.repo_fs
+                fs_path = fs.from_os_path(path)
+
             with FsspecCallback.as_tqdm_callback(
                 total=-1,
                 desc=f"Downloading {fs.path.name(path)}",
                 unit="files",
             ) as cb:
                 fs.get(
-                    path, os.path.abspath(out), batch_size=jobs, callback=cb
+                    fs_path,
+                    os.path.abspath(out),
+                    batch_size=jobs,
+                    callback=cb,
                 )
     finally:
         remove(tmp_dir)
diff --git a/dvc/repo/ls.py b/dvc/repo/ls.py
index 55c41030a..c27c4a6d0 100644
--- a/dvc/repo/ls.py
+++ b/dvc/repo/ls.py
@@ -45,8 +45,10 @@ def ls(url, path=None, rev=None, recursive=None, dvc_only=False):
 
 
 def _ls(fs, path, recursive=None, dvc_only=False):
+    fs_path = fs.from_os_path(path)
+
     try:
-        fs_path = fs.info(path)["name"]
+        fs_path = fs.info(fs_path)["name"]
     except FileNotFoundError:
         return {}
 
diff --git a/dvc/repo/metrics/show.py b/dvc/repo/metrics/show.py
index 697048b98..d57df8f3b 100644
--- a/dvc/repo/metrics/show.py
+++ b/dvc/repo/metrics/show.py
@@ -24,11 +24,11 @@ def _to_fs_paths(metrics: List[Output]) -> StrPaths:
     result = []
     for out in metrics:
         if out.metric:
-            result.append(out.fs_path)
+            result.append(out.repo.repo_fs.from_os_path(out.fs_path))
         elif out.live:
             fs_path = summary_fs_path(out)
             if fs_path:
-                result.append(fs_path)
+                result.append(out.repo.repo_fs.from_os_path(fs_path))
     return result
 
 
@@ -78,12 +78,16 @@ def _read_metric(path, fs, rev, **kwargs):
 def _read_metrics(repo, metrics, rev, onerror=None):
     fs = RepoFileSystem(repo=repo)
 
+    relpath = ""
+    if repo.root_dir != repo.fs.path.getcwd():
+        relpath = repo.fs.path.relpath(repo.root_dir, repo.fs.path.getcwd())
+
     res = {}
     for metric in metrics:
         if not fs.isfile(metric):
             continue
 
-        res[os.path.relpath(metric, os.getcwd())] = _read_metric(
+        res[os.path.join(relpath, *fs.path.parts(metric))] = _read_metric(
             metric, fs, rev, onerror=onerror
         )
 
diff --git a/dvc/repo/plots/__init__.py b/dvc/repo/plots/__init__.py
index 7e3118f4d..f99c71166 100644
--- a/dvc/repo/plots/__init__.py
+++ b/dvc/repo/plots/__init__.py
@@ -251,7 +251,10 @@ def _collect_plots(
         recursive=recursive,
     )
 
-    result = {plot.fs_path: _plot_props(plot) for plot in plots}
+    result = {
+        repo.repo_fs.from_os_path(plot.fs_path): _plot_props(plot)
+        for plot in plots
+    }
     result.update({fs_path: {} for fs_path in fs_paths})
     return result
 
diff --git a/tests/unit/fs/test_dvc.py b/tests/unit/fs/test_dvc.py
index 45fda5e63..04e68a405 100644
--- a/tests/unit/fs/test_dvc.py
+++ b/tests/unit/fs/test_dvc.py
@@ -15,6 +15,7 @@ from dvc.utils.fs import remove
     [
         ("", ("repo",)),
         (".", ("repo",)),
+        ("/", ("repo",)),
         ("foo", ("repo", "foo")),
         ("dir/foo", ("repo", "dir", "foo")),
     ],
diff --git a/tests/unit/fs/test_repo.py b/tests/unit/fs/test_repo.py
index 70b9e8464..83e5f605a 100644
--- a/tests/unit/fs/test_repo.py
+++ b/tests/unit/fs/test_repo.py
@@ -474,16 +474,16 @@ def test_repo_fs_no_subrepos(tmp_dir, dvc, scm):
     dvc._reset()
     fs = RepoFileSystem(repo=dvc)
     expected = [
-        ".dvcignore",
-        ".gitignore",
-        "lorem",
-        "lorem.dvc",
-        "dir",
-        "dir/repo.txt",
+        "/.dvcignore",
+        "/.gitignore",
+        "/lorem",
+        "/lorem.dvc",
+        "/dir",
+        "/dir/repo.txt",
     ]
 
     actual = []
-    for root, dirs, files in fs.walk("", dvcfiles=True):
+    for root, dirs, files in fs.walk("/", dvcfiles=True):
         for entry in dirs + files:
             actual.append(posixpath.join(root, entry))
 
@@ -657,9 +657,9 @@ def test_walk_nested_subrepos(tmp_dir, dvc, scm, traverse_subrepos):
 
         if traverse_subrepos or repo_dir == tmp_dir:
             repo_dir_path = (
-                repo_dir.relative_to(tmp_dir).as_posix()
+                "/" + repo_dir.relative_to(tmp_dir).as_posix()
                 if repo_dir != tmp_dir
-                else ""
+                else "/"
             )
             expected[repo_dir_path] = set(
                 scm_files.keys() | dvc_files.keys() | extras
@@ -675,13 +675,13 @@ def test_walk_nested_subrepos(tmp_dir, dvc, scm, traverse_subrepos):
 
     if traverse_subrepos:
         # update subrepos
-        expected[""].update(["subrepo1", "subrepo2"])
-        expected["subrepo1"].add("subrepo3")
+        expected["/"].update(["subrepo1", "subrepo2"])
+        expected["/subrepo1"].add("subrepo3")
 
     actual = {}
     fs = RepoFileSystem(repo=dvc)
     for root, dirs, files in fs.walk(
-        "", ignore_subrepos=not traverse_subrepos
+        "/", ignore_subrepos=not traverse_subrepos
     ):
         actual[root] = set(dirs + files)
     assert expected == actual
diff --git a/tests/unit/fs/test_repo_info.py b/tests/unit/fs/test_repo_info.py
index f50589aac..5f93d1edf 100644
--- a/tests/unit/fs/test_repo_info.py
+++ b/tests/unit/fs/test_repo_info.py
@@ -137,10 +137,3 @@ def test_info_on_subrepos(make_tmp_dir, tmp_dir, dvc, scm, repo_fs):
         assert info["repo"].root_dir == str(
             subrepo
         ), f"repo root didn't match for {path}"
-
-    # supports external outputs on top-level DVC repo
-    external_dir = make_tmp_dir("external-output")
-    external_dir.gen("bar", "bar")
-    dvc.add(str(external_dir / "bar"), external=True)
-    info = repo_fs.info((external_dir / "bar").fs_path)
-    assert info["repo"].root_dir == str(tmp_dir)