add: fix issue when adding already tracked symlinked files (#4778)

* LocalOutput: paths symlinked outside of git can't be gitignored

* utils: contains_symlink_up_to should only check parent dirs on add

* tests: update test_add symlink cases

* add: forbid adding symlinked dirs or files inside symlinked dirs

* update tests

* add troubleshooting link

dvc/utils/__init__.py

@@ -353,7 +353,9 @@ def resolve_paths(repo, out):
     from urllib.parse import urlparse
 
     from ..dvcfile import DVC_FILE_SUFFIX
+    from ..exceptions import DvcException
     from ..path_info import PathInfo
+    from ..system import System
     from .fs import contains_symlink_up_to
 
     abspath = PathInfo(os.path.abspath(out))
@@ -364,15 +366,24 @@ def resolve_paths(repo, out):
     if os.name == "nt" and scheme == abspath.drive[0].lower():
         # urlparse interprets windows drive letters as URL scheme
         scheme = ""
-    if (
-        not scheme
-        and abspath.isin_or_eq(repo.root_dir)
-        and not contains_symlink_up_to(abspath, repo.root_dir)
+
+    if scheme or not abspath.isin_or_eq(repo.root_dir):
+        wdir = os.getcwd()
+    elif contains_symlink_up_to(dirname, repo.root_dir) or (
+        os.path.isdir(abspath) and System.is_symlink(abspath)
     ):
+        msg = (
+            "Cannot add files inside symlinked directories to DVC. "
+            "See {} for more information."
+        ).format(
+            format_link(
+                "https://dvc.org/doc/user-guide/troubleshooting#add-symlink"
+            )
+        )
+        raise DvcException(msg)
+    else:
         wdir = dirname
         out = base
-    else:
-        wdir = os.getcwd()
 
     path = os.path.join(wdir, base + DVC_FILE_SUFFIX)
 

tests/func/test_add.py

@@ -1,7 +1,6 @@
 import filecmp
 import logging
 import os
-import posixpath
 import shutil
 import time
 
@@ -422,52 +421,6 @@ def test_should_collect_dir_cache_only_once(mocker, tmp_dir, dvc):
     assert get_dir_hash_counter.mock.call_count == 1
 
 
-class SymlinkAddTestBase(TestDvc):
-    def _get_data_dir(self):
-        raise NotImplementedError
-
-    def _prepare_external_data(self):
-        data_dir = self._get_data_dir()
-
-        data_file_name = "data_file"
-        external_data_path = os.path.join(data_dir, data_file_name)
-        with open(external_data_path, "w+") as f:
-            f.write("data")
-
-        link_name = "data_link"
-        System.symlink(data_dir, link_name)
-        return data_file_name, link_name
-
-    def _test(self):
-        data_file_name, link_name = self._prepare_external_data()
-
-        ret = main(["add", os.path.join(link_name, data_file_name)])
-        self.assertEqual(0, ret)
-
-        stage_file = data_file_name + DVC_FILE_SUFFIX
-        self.assertTrue(os.path.exists(stage_file))
-
-        d = load_yaml(stage_file)
-        relative_data_path = posixpath.join(link_name, data_file_name)
-        self.assertEqual(relative_data_path, d["outs"][0]["path"])
-
-
-class TestShouldAddDataFromExternalSymlink(SymlinkAddTestBase):
-    def _get_data_dir(self):
-        return self.mkdtemp()
-
-    def test(self):
-        self._test()
-
-
-class TestShouldAddDataFromInternalSymlink(SymlinkAddTestBase):
-    def _get_data_dir(self):
-        return self.DATA_DIR
-
-    def test(self):
-        self._test()
-
-
 class TestShouldPlaceStageInDataDirIfRepositoryBelowSymlink(TestDvc):
     def test(self):
         def is_symlink_true_below_dvc_root(path):
@@ -809,13 +762,15 @@ def test_add_pipeline_file(tmp_dir, dvc, run_copy):
         dvc.add(PIPELINE_FILE)
 
 
-def test_add_symlink(tmp_dir, dvc):
+def test_add_symlink_file(tmp_dir, dvc):
     tmp_dir.gen({"dir": {"bar": "bar"}})
 
     (tmp_dir / "dir" / "foo").symlink_to(os.path.join(".", "bar"))
 
     dvc.add(os.path.join("dir", "foo"))
 
+    assert not (tmp_dir / "foo.dvc").exists()
+    assert (tmp_dir / "dir" / "foo.dvc").exists()
     assert not (tmp_dir / "dir" / "foo").is_symlink()
     assert not (tmp_dir / "dir" / "bar").is_symlink()
     assert (tmp_dir / "dir" / "foo").read_text() == "bar"
@@ -827,3 +782,41 @@ def test_add_symlink(tmp_dir, dvc):
     assert not (
         tmp_dir / ".dvc" / "cache" / "37" / "b51d194a7513e45b56f6524f2d51f2"
     ).is_symlink()
+
+    # Test that subsequent add succeeds
+    # See https://github.com/iterative/dvc/issues/4654
+    dvc.add(os.path.join("dir", "foo"))
+
+
+@pytest.mark.parametrize("external", [True, False])
+def test_add_symlink_dir(make_tmp_dir, tmp_dir, dvc, external):
+    if external:
+        data_dir = make_tmp_dir("data")
+        data_dir.gen({"foo": "foo"})
+        target = os.fspath(data_dir)
+    else:
+        tmp_dir.gen({"data": {"foo": "foo"}})
+        target = os.path.join(".", "data")
+
+    tmp_dir.gen({"data": {"foo": "foo"}})
+
+    (tmp_dir / "dir").symlink_to(target)
+
+    with pytest.raises(DvcException):
+        dvc.add("dir")
+
+
+@pytest.mark.parametrize("external", [True, False])
+def test_add_file_in_symlink_dir(make_tmp_dir, tmp_dir, dvc, external):
+    if external:
+        data_dir = make_tmp_dir("data")
+        data_dir.gen({"dir": {"foo": "foo"}})
+        target = os.fspath(data_dir / "dir")
+    else:
+        tmp_dir.gen({"data": {"foo": "foo"}})
+        target = os.path.join(".", "data")
+
+    (tmp_dir / "dir").symlink_to(target)
+
+    with pytest.raises(DvcException):
+        dvc.add(os.path.join("dir", "foo"))