diff --git a/dvc/cache/base.py b/dvc/cache/base.py index 294fea640..4ec3683d2 100644 --- a/dvc/cache/base.py +++ b/dvc/cache/base.py @@ -221,7 +221,7 @@ class CloudCache: path_info ): # Default relink procedure involves unneeded copy - self.tree.unprotect(path_info) + self.unprotect(path_info) else: self.tree.remove(path_info) self.link(cache_info, path_info) @@ -281,7 +281,9 @@ class CloudCache: from_info = PathInfo(tmp) to_info = self.tree.path_info / tmp_fname("") - self.tree.upload(from_info, to_info, no_progress_bar=True) + self.tree.upload( + from_info, to_info, no_progress_bar=True, file_mode=self.CACHE_MODE + ) hash_info = self.tree.get_file_hash(to_info) hash_info.value += self.tree.CHECKSUM_DIR_SUFFIX @@ -373,6 +375,15 @@ class CloudCache: def hash_to_path(self, hash_): return self.tree.hash_to_path_info(hash_) + def protect(self, path_info): # pylint: disable=unused-argument + pass + + def is_protected(self, path_info): # pylint: disable=unused-argument + return False + + def unprotect(self, path_info): # pylint: disable=unused-argument + pass + def changed_cache_file(self, hash_info): """Compare the given hash with the (corresponding) actual one. @@ -387,7 +398,7 @@ class CloudCache: """ # Prefer string path over PathInfo when possible due to performance cache_info = self.hash_to_path(hash_info.value) - if self.tree.is_protected(cache_info): + if self.is_protected(cache_info): logger.trace( "Assuming '%s' is unchanged since it is read-only", cache_info ) @@ -408,7 +419,7 @@ class CloudCache: if actual.value.split(".")[0] == hash_info.value.split(".")[0]: # making cache file read-only so we don't need to check it # next time - self.tree.protect(cache_info) + self.protect(cache_info) return False if self.tree.exists(cache_info): diff --git a/dvc/cache/local.py b/dvc/cache/local.py index e2c0199e2..14d76cd89 100644 --- a/dvc/cache/local.py +++ b/dvc/cache/local.py @@ -2,13 +2,14 @@ import logging import os from funcy import cached_property +from shortuuid import uuid from dvc.hash_info import HashInfo from dvc.path_info import PathInfo from dvc.progress import Tqdm -from ..tree.local import LocalTree -from ..utils.fs import walk_files +from ..utils import relpath +from ..utils.fs import copyfile, remove, walk_files from .base import CloudCache logger = logging.getLogger(__name__) @@ -16,7 +17,7 @@ logger = logging.getLogger(__name__) class LocalCache(CloudCache): DEFAULT_CACHE_TYPES = ["reflink", "copy"] - CACHE_MODE = LocalTree.CACHE_MODE + CACHE_MODE = 0o444 UNPACKED_DIR_SUFFIX = ".unpacked" def __init__(self, tree): @@ -89,3 +90,56 @@ class LocalCache(CloudCache): info = self.tree.hash_to_path_info(hash_) path_info = info.with_name(info.name + self.UNPACKED_DIR_SUFFIX) self.tree.remove(path_info) + + def _unprotect_file(self, path): + if self.tree.is_symlink(path) or self.tree.is_hardlink(path): + logger.debug("Unprotecting '%s'", path) + tmp = os.path.join(os.path.dirname(path), "." + uuid()) + + # The operations order is important here - if some application + # would access the file during the process of copyfile then it + # would get only the part of file. So, at first, the file should be + # copied with the temporary name, and then original file should be + # replaced by new. + copyfile(path, tmp, name="Unprotecting '{}'".format(relpath(path))) + remove(path) + os.rename(tmp, path) + + else: + logger.debug( + "Skipping copying for '%s', since it is not " + "a symlink or a hardlink.", + path, + ) + + os.chmod(path, self.tree.file_mode) + + def _unprotect_dir(self, path): + for fname in self.tree.walk_files(path): + self._unprotect_file(fname) + + def unprotect(self, path_info): + if not os.path.exists(path_info): + from dvc.exceptions import DvcException + + raise DvcException( + f"can't unprotect non-existing data '{path_info}'" + ) + + if os.path.isdir(path_info): + self._unprotect_dir(path_info) + else: + self._unprotect_file(path_info) + + def protect(self, path_info): + self.tree.chmod(path_info, self.CACHE_MODE) + + def is_protected(self, path_info): + import stat + + try: + mode = os.stat(path_info).st_mode + except FileNotFoundError: + return False + + return stat.S_IMODE(mode) == self.CACHE_MODE diff --git a/dvc/output/base.py b/dvc/output/base.py index 8303a6606..420fc6516 100644 --- a/dvc/output/base.py +++ b/dvc/output/base.py @@ -430,7 +430,7 @@ class BaseOutput: def unprotect(self): if self.exists: - self.tree.unprotect(self.path_info) + self.cache.unprotect(self.path_info) def get_dir_cache(self, **kwargs): if not self.is_dir_checksum: diff --git a/dvc/remote/base.py b/dvc/remote/base.py index f44dc2302..eb2d2f86b 100644 --- a/dvc/remote/base.py +++ b/dvc/remote/base.py @@ -309,15 +309,24 @@ class Remote: ) if download: + # NOTE: don't set CACHE_MODE if we don't trust the remote, so that + # cache could verify these files by-hand later. + if self.tree.verify: + mode = self.tree.file_mode + else: + mode = cache.CACHE_MODE func = partial( _log_exceptions(self.tree.download, "download"), dir_mode=cache.tree.dir_mode, - file_mode=cache.tree.file_mode, + file_mode=mode, ) status = STATUS_DELETED desc = "Downloading" else: - func = _log_exceptions(self.tree.upload, "upload") + func = partial( + _log_exceptions(self.tree.upload, "upload"), + file_mode=self.cache.CACHE_MODE, + ) status = STATUS_NEW desc = "Uploading" diff --git a/dvc/repo/__init__.py b/dvc/repo/__init__.py index aee323509..4b64fd0cd 100644 --- a/dvc/repo/__init__.py +++ b/dvc/repo/__init__.py @@ -267,7 +267,7 @@ class Repo: return Repo(root_dir) def unprotect(self, target): - return self.cache.local.tree.unprotect(PathInfo(target)) + return self.cache.local.unprotect(PathInfo(target)) def _ignore(self): flist = [ diff --git a/dvc/tree/base.py b/dvc/tree/base.py index 705a4a509..311b9263a 100644 --- a/dvc/tree/base.py +++ b/dvc/tree/base.py @@ -60,7 +60,6 @@ class BaseTree: TRAVERSE_THRESHOLD_SIZE = 500000 CAN_TRAVERSE = True - CACHE_MODE: Optional[int] = None SHARED_MODE_MAP = {None: (None, None), "group": (None, None)} PARAM_CHECKSUM: ClassVar[Optional[str]] = None @@ -251,19 +250,8 @@ class BaseTree: def reflink(self, from_info, to_info): raise RemoteActionNotImplemented("reflink", self.scheme) - @staticmethod - def protect(path_info): - pass - - def is_protected(self, path_info): - return False - # pylint: enable=unused-argument - @staticmethod - def unprotect(path_info): - pass - @classmethod def is_dir_hash(cls, hash_): if not hash_: diff --git a/dvc/tree/local.py b/dvc/tree/local.py index e5506963d..f8aa4e069 100644 --- a/dvc/tree/local.py +++ b/dvc/tree/local.py @@ -5,14 +5,12 @@ import stat from typing import Any, Dict from funcy import cached_property -from shortuuid import uuid -from dvc.exceptions import DvcException from dvc.hash_info import HashInfo from dvc.path_info import PathInfo from dvc.scheme import Schemes from dvc.system import System -from dvc.utils import file_md5, is_exec, relpath, tmp_fname +from dvc.utils import file_md5, is_exec, tmp_fname from dvc.utils.fs import copy_fobj_to_file, copyfile, makedirs, move, remove from .base import BaseTree @@ -27,7 +25,6 @@ class LocalTree(BaseTree): PARAM_PATH = "path" TRAVERSE_PREFIX_LEN = 2 - CACHE_MODE = 0o444 SHARED_MODE_MAP: Dict[Any, Any] = { None: (0o644, 0o755), "group": (0o664, 0o775), @@ -253,55 +250,6 @@ class LocalTree(BaseTree): if actual != mode: raise - def _unprotect_file(self, path): - if System.is_symlink(path) or System.is_hardlink(path): - logger.debug("Unprotecting '%s'", path) - tmp = os.path.join(os.path.dirname(path), "." + uuid()) - - # The operations order is important here - if some application - # would access the file during the process of copyfile then it - # would get only the part of file. So, at first, the file should be - # copied with the temporary name, and then original file should be - # replaced by new. - copyfile(path, tmp, name="Unprotecting '{}'".format(relpath(path))) - remove(path) - os.rename(tmp, path) - - else: - logger.debug( - "Skipping copying for '%s', since it is not " - "a symlink or a hardlink.", - path, - ) - - os.chmod(path, self.file_mode) - - def _unprotect_dir(self, path): - for fname in self.walk_files(path): - self._unprotect_file(fname) - - def unprotect(self, path_info): - if not os.path.exists(path_info): - raise DvcException( - f"can't unprotect non-existing data '{path_info}'" - ) - - if os.path.isdir(path_info): - self._unprotect_dir(path_info) - else: - self._unprotect_file(path_info) - - def protect(self, path_info): - self.chmod(path_info, self.CACHE_MODE) - - def is_protected(self, path_info): - try: - mode = os.stat(path_info).st_mode - except FileNotFoundError: - return False - - return stat.S_IMODE(mode) == self.CACHE_MODE - def get_file_hash(self, path_info): hash_info = HashInfo(self.PARAM_CHECKSUM, file_md5(path_info)[0],) @@ -332,8 +280,6 @@ class LocalTree(BaseTree): if file_mode is not None: self.chmod(tmp_file, file_mode) - else: - self.protect(tmp_file) os.replace(tmp_file, to_info) @staticmethod diff --git a/tests/func/test_add.py b/tests/func/test_add.py index b3ff50688..554efd8b2 100644 --- a/tests/func/test_add.py +++ b/tests/func/test_add.py @@ -714,7 +714,7 @@ def test_readding_dir_should_not_unprotect_all(tmp_dir, dvc, mocker): dvc.add("dir") tmp_dir.gen("dir/new_file", "new_file_content") - unprotect_spy = mocker.spy(LocalTree, "unprotect") + unprotect_spy = mocker.spy(dvc.cache.local, "unprotect") dvc.add("dir") assert not unprotect_spy.mock.called diff --git a/tests/unit/remote/test_local.py b/tests/unit/cache/test_local.py similarity index 77% rename from tests/unit/remote/test_local.py rename to tests/unit/cache/test_local.py index 5846c1ddf..d50354293 100644 --- a/tests/unit/remote/test_local.py +++ b/tests/unit/cache/test_local.py @@ -36,7 +36,8 @@ def test_status_download_optimization(mocker, dvc): @pytest.mark.parametrize("link_name", ["hardlink", "symlink"]) def test_is_protected(tmp_dir, dvc, link_name): - tree = LocalTree(dvc, {}) + cache = dvc.cache.local + tree = cache.tree link_method = getattr(tree, link_name) (tmp_dir / "foo").write_text("foo") @@ -46,47 +47,45 @@ def test_is_protected(tmp_dir, dvc, link_name): link_method(foo, link) - assert not tree.is_protected(foo) - assert not tree.is_protected(link) + assert not cache.is_protected(foo) + assert not cache.is_protected(link) - tree.protect(foo) + cache.protect(foo) - assert tree.is_protected(foo) - assert tree.is_protected(link) + assert cache.is_protected(foo) + assert cache.is_protected(link) - tree.unprotect(link) + cache.unprotect(link) - assert not tree.is_protected(link) + assert not cache.is_protected(link) if os.name == "nt" and link_name == "hardlink": # NOTE: NTFS doesn't allow deleting read-only files, which forces us to # set write perms on the link, which propagates to the source. - assert not tree.is_protected(foo) + assert not cache.is_protected(foo) else: - assert tree.is_protected(foo) + assert cache.is_protected(foo) @pytest.mark.parametrize("err", [errno.EPERM, errno.EACCES]) -def test_protect_ignore_errors(tmp_dir, mocker, err): +def test_protect_ignore_errors(tmp_dir, dvc, mocker, err): tmp_dir.gen("foo", "foo") foo = PathInfo("foo") - tree = LocalTree(None, {}) - tree.protect(foo) + dvc.cache.local.protect(foo) mock_chmod = mocker.patch( "os.chmod", side_effect=OSError(err, "something") ) - tree.protect(foo) + dvc.cache.local.protect(foo) assert mock_chmod.called -def test_protect_ignore_erofs(tmp_dir, mocker): +def test_protect_ignore_erofs(tmp_dir, dvc, mocker): tmp_dir.gen("foo", "foo") foo = PathInfo("foo") - tree = LocalTree(None, {}) mock_chmod = mocker.patch( "os.chmod", side_effect=OSError(errno.EROFS, "read-only fs") ) - tree.protect(foo) + dvc.cache.local.protect(foo) assert mock_chmod.called diff --git a/tests/unit/tree/test_tree.py b/tests/unit/tree/test_tree.py index de6a0ad23..b3e8373e7 100644 --- a/tests/unit/tree/test_tree.py +++ b/tests/unit/tree/test_tree.py @@ -53,11 +53,15 @@ def test_get_cloud_tree_validate(tmp_dir, dvc): @pytest.mark.skipif(os.name == "nt", reason="Not supported for Windows.") @pytest.mark.parametrize( - "mode, expected", [(None, LocalTree.CACHE_MODE), (0o777, 0o777)], + "mode, expected", [(None, None), (0o777, 0o777)], ) def test_upload_file_mode(tmp_dir, mode, expected): tmp_dir.gen("src", "foo") src = PathInfo(tmp_dir / "src") + + if not expected: + expected = stat.S_IMODE((tmp_dir / "src").stat().st_mode) + dest = PathInfo(tmp_dir / "dest") tree = LocalTree(None, {"url": os.fspath(tmp_dir)}) tree.upload(src, dest, file_mode=mode)