From b2495aa2308b3d8f91e7ea2075a3cc349a2c9e74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20Rowlands=20=28=EB=B3=80=EA=B8=B0=ED=98=B8=29?= Date: Tue, 27 Oct 2020 22:39:01 +0900 Subject: [PATCH] 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 | 23 ++++++++--- tests/func/test_add.py | 89 +++++++++++++++++++----------------------- 2 files changed, 58 insertions(+), 54 deletions(-) diff --git a/dvc/utils/__init__.py b/dvc/utils/__init__.py index fc324211d..fbf75637b 100644 --- a/dvc/utils/__init__.py +++ b/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) diff --git a/tests/func/test_add.py b/tests/func/test_add.py index 9b3003f59..ff35e012f 100644 --- a/tests/func/test_add.py +++ b/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"))