checkout/remove: properly handle broken symlinks

Fixes #1856

Signed-off-by: Ruslan Kuprieiev <[email protected]>

dvc/remote/local.py

@@ -129,7 +129,7 @@ class RemoteLOCAL(RemoteBase):
     def exists(self, path_info):
         assert not isinstance(path_info, list)
         assert path_info["scheme"] == "local"
-        return os.path.exists(path_info["path"])
+        return os.path.lexists(path_info["path"])
 
     def changed_cache(self, md5):
         cache = self.get(md5)
@@ -316,7 +316,7 @@ class RemoteLOCAL(RemoteBase):
         cache = self.get(checksum)
 
         if not self.is_dir_cache(cache):
-            if os.path.exists(path):
+            if self.exists(path_info):
                 self.safe_remove(path_info, force=force)
 
             self.link(cache, path)
@@ -347,7 +347,7 @@ class RemoteLOCAL(RemoteBase):
             entry_checksum_info = {self.PARAM_CHECKSUM: m}
 
             if self.changed(entry_info, entry_checksum_info):
-                if os.path.exists(p):
+                if self.exists(entry_info):
                     self.safe_remove(entry_info, force=force)
 
                 self.link(c, p)

dvc/utils/__init__.py

@@ -10,6 +10,7 @@ import sys
 import stat
 import math
 import json
+import errno
 import shutil
 import hashlib
 import nanotime
@@ -156,22 +157,30 @@ def move(src, dst):
     shutil.move(src, dst)
 
 
-def remove(path):
-    if not os.path.exists(path):
-        return
-
-    logger.debug("Removing '{}'".format(os.path.relpath(path)))
-
-    def _chmod(func, p, excinfo):
+def _chmod(func, p, excinfo):
+    try:
         perm = os.stat(p).st_mode
         perm |= stat.S_IWRITE
         os.chmod(p, perm)
-        func(p)
+    except OSError as exc:
+        # NOTE: broken symlink case.
+        if exc.errno != errno.ENOENT:
+            raise
 
-    if os.path.isfile(path):
-        _chmod(os.unlink, path, None)
-    else:
-        shutil.rmtree(path, onerror=_chmod)
+    func(p)
+
+
+def remove(path):
+    logger.debug("Removing '{}'".format(os.path.relpath(path)))
+
+    try:
+        if os.path.isdir(path):
+            shutil.rmtree(path, onerror=_chmod)
+        else:
+            _chmod(os.unlink, path, None)
+    except OSError as exc:
+        if exc.errno != errno.ENOENT:
+            raise
 
 
 def to_chunks(l, jobs):

dvc/utils/fs.py

@@ -1,7 +1,8 @@
 from __future__ import unicode_literals
 
-import nanotime
 import os
+import errno
+import nanotime
 import logging
 
 from dvc.exceptions import DvcException
@@ -26,7 +27,13 @@ def get_mtime_and_size(path):
         for root, dirs, files in os.walk(str(path)):
             for name in dirs + files:
                 entry = os.path.join(root, name)
-                stat = os.stat(entry)
+                try:
+                    stat = os.stat(entry)
+                except OSError as exc:
+                    # NOTE: broken symlink case.
+                    if exc.errno != errno.ENOENT:
+                        raise
+                    continue
                 size += stat.st_size
                 entry_mtime = stat.st_mtime
                 if entry_mtime > mtime:

tests/requirements.txt

@@ -13,3 +13,4 @@ collective.checkdocs
 black==19.3b0 ; python_version >= '3.6'
 flake8
 flake8-docstrings
+jaraco.windows==3.9.2

tests/test_checkout.py

@@ -517,3 +517,52 @@ class TestCheckoutRecursiveNotDirectory(TestDvc):
 
         with self.assertRaises(TargetNotDirectoryError):
             self.dvc.checkout(target=self.FOO, recursive=True)
+
+
+class TestCheckoutMovedCacheDirWithSymlinks(TestDvc):
+    def test(self):
+        ret = main(["config", "cache.type", "symlink"])
+        self.assertEqual(ret, 0)
+
+        ret = main(["add", self.FOO])
+        self.assertEqual(ret, 0)
+
+        ret = main(["add", self.DATA_DIR])
+        self.assertEqual(ret, 0)
+
+        if os.name == "nt":
+            from jaraco.windows.filesystem import readlink
+        else:
+            readlink = os.readlink
+
+        self.assertTrue(System.is_symlink(self.FOO))
+        old_foo_link = readlink(self.FOO)
+
+        self.assertTrue(System.is_symlink(self.DATA))
+        old_data_link = readlink(self.DATA)
+
+        old_cache_dir = self.dvc.cache.local.cache_dir
+        new_cache_dir = old_cache_dir + "_new"
+        os.rename(old_cache_dir, new_cache_dir)
+
+        ret = main(["cache", "dir", new_cache_dir])
+        self.assertEqual(ret, 0)
+
+        ret = main(["checkout", "-f"])
+        self.assertEqual(ret, 0)
+
+        self.assertTrue(System.is_symlink(self.FOO))
+        new_foo_link = readlink(self.FOO)
+
+        self.assertTrue(System.is_symlink(self.DATA))
+        new_data_link = readlink(self.DATA)
+
+        self.assertEqual(
+            os.path.relpath(old_foo_link, old_cache_dir),
+            os.path.relpath(new_foo_link, new_cache_dir),
+        )
+
+        self.assertEqual(
+            os.path.relpath(old_data_link, old_cache_dir),
+            os.path.relpath(new_data_link, new_cache_dir),
+        )

tests/test_remove.py

@@ -1,6 +1,8 @@
 import os
+import shutil
 
 from dvc.main import main
+from dvc.system import System
 from dvc.stage import Stage, StageFileDoesNotExistError
 from dvc.exceptions import DvcException
 
@@ -33,6 +35,24 @@ class TestRemoveNonExistentFile(TestDvc):
             self.dvc.remove("non_existent_dvc_file")
 
 
+class TestRemoveBrokenSymlink(TestDvc):
+    def test(self):
+        ret = main(["config", "cache.type", "symlink"])
+        self.assertEqual(ret, 0)
+
+        ret = main(["add", self.FOO])
+        self.assertEqual(ret, 0)
+
+        shutil.rmtree(self.dvc.cache.local.cache_dir)
+
+        self.assertTrue(System.is_symlink(self.FOO))
+
+        ret = main(["remove", self.FOO + ".dvc"])
+        self.assertEqual(ret, 0)
+
+        self.assertFalse(os.path.lexists(self.FOO))
+
+
 class TestRemoveDirectory(TestDvc):
     def test(self):
         stages = self.dvc.add(self.DATA_DIR)