diff --git a/dvc/remote/base.py b/dvc/remote/base.py index 8eecd0bd1..207b94703 100644 --- a/dvc/remote/base.py +++ b/dvc/remote/base.py @@ -680,20 +680,45 @@ class RemoteBASE(object): self.remove(path_info) def _checkout_file( - self, path_info, checksum, force, progress_callback=None + self, + path_info, + checksum, + force, + progress_callback=None, + save_link=True, ): - cache_info = self.checksum_to_path_info(checksum) - if self.exists(path_info): - msg = "data '{}' exists. Removing before checkout." - logger.warning(msg.format(str(path_info))) + # NOTE: In case if path_info is already cached and path_info's + # link type matches cache link type, we would like to avoid + # relinking. + if self.changed( + path_info, {self.PARAM_CHECKSUM: checksum} + ) or not self._link_matches(path_info): self.safe_remove(path_info, force=force) - self.link(cache_info, path_info) - self.state.save_link(path_info) - self.state.save(path_info, checksum) + cache_info = self.checksum_to_path_info(checksum) + self.link(cache_info, path_info) + + if save_link: + self.state.save_link(path_info) + + self.state.save(path_info, checksum) + else: + # NOTE: performing (un)protection costs us +/- the same as checking + # if path_info is protected. Instead of implementing logic, + # just (un)protect according to self.protected. + if self.protected: + self.protect(path_info) + else: + # NOTE dont allow copy, because we checked before that link + # type matches cache, and we don't want data duplication + self.unprotect(path_info, allow_copy=False) + if progress_callback: progress_callback(str(path_info)) + def _link_matches(self, path_info): + return True + def makedirs(self, path_info): raise NotImplementedError @@ -712,17 +737,14 @@ class RemoteBASE(object): for entry in dir_info: relative_path = entry[self.PARAM_RELPATH] entry_checksum = entry[self.PARAM_CHECKSUM] - entry_cache_info = self.checksum_to_path_info(entry_checksum) entry_info = path_info / relative_path - - entry_checksum_info = {self.PARAM_CHECKSUM: entry_checksum} - if self.changed(entry_info, entry_checksum_info): - if self.exists(entry_info): - self.safe_remove(entry_info, force=force) - self.link(entry_cache_info, entry_info) - self.state.save(entry_info, entry_checksum) - if progress_callback: - progress_callback(str(entry_info)) + self._checkout_file( + entry_info, + entry_checksum, + force, + progress_callback, + save_link=False, + ) self._remove_redundant_files(path_info, dir_info, force) @@ -803,7 +825,7 @@ class RemoteBASE(object): return 1 @staticmethod - def unprotect(path_info): + def unprotect(path_info, allow_copy=True): pass def _get_unpacked_dir_names(self, checksums): diff --git a/dvc/remote/local/__init__.py b/dvc/remote/local/__init__.py index 168c1f9c0..43e679dbb 100644 --- a/dvc/remote/local/__init__.py +++ b/dvc/remote/local/__init__.py @@ -78,6 +78,7 @@ class RemoteLOCAL(RemoteBASE): self.cache_types = types else: self.cache_types = copy(self.DEFAULT_CACHE_TYPES) + self.cache_type_confirmed = False # A clunky way to detect cache dir storagepath = config.get(Config.SECTION_LOCAL_STORAGEPATH, None) @@ -188,6 +189,7 @@ class RemoteLOCAL(RemoteBASE): link_method = self._get_link_method(link_types[0]) try: self._do_link(from_info, to_info, link_method) + self.cache_type_confirmed = True return except DvcException as exc: @@ -471,19 +473,22 @@ class RemoteLOCAL(RemoteBASE): logger.warning(msg) @staticmethod - def _unprotect_file(path): + def _unprotect_file(path, allow_copy=True): if System.is_symlink(path) or System.is_hardlink(path): - logger.debug("Unprotecting '{}'".format(path)) - tmp = os.path.join(os.path.dirname(path), "." + str(uuid())) + if allow_copy: + logger.debug("Unprotecting '{}'".format(path)) + tmp = os.path.join(os.path.dirname(path), "." + str(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) + # 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( @@ -493,11 +498,11 @@ class RemoteLOCAL(RemoteBASE): os.chmod(path, os.stat(path).st_mode | stat.S_IWRITE) - def _unprotect_dir(self, path): + def _unprotect_dir(self, path, allow_copy=True): for fname in walk_files(path, self.repo.dvcignore): - RemoteLOCAL._unprotect_file(fname) + self._unprotect_file(fname, allow_copy) - def unprotect(self, path_info): + def unprotect(self, path_info, allow_copy=True): path = path_info.fspath if not os.path.exists(path): raise DvcException( @@ -505,9 +510,9 @@ class RemoteLOCAL(RemoteBASE): ) if os.path.isdir(path): - self._unprotect_dir(path) + self._unprotect_dir(path, allow_copy) else: - RemoteLOCAL._unprotect_file(path) + self._unprotect_file(path, allow_copy) @staticmethod def protect(path_info): @@ -581,3 +586,36 @@ class RemoteLOCAL(RemoteBASE): if self.is_dir_checksum(c): unpacked.add(c + self.UNPACKED_DIR_SUFFIX) return unpacked + + def _get_cache_type(self, path_info): + if self.cache_type_confirmed: + return self.cache_types[0] + + workspace_file = path_info.with_name("." + uuid()) + test_cache_file = self.path_info / ".cache_type_test_file" + if not self.exists(test_cache_file): + with open(fspath_py35(test_cache_file), "wb") as fobj: + fobj.write(bytes(1)) + try: + self.link(test_cache_file, workspace_file) + finally: + self.remove(workspace_file) + self.remove(test_cache_file) + + self.cache_type_confirmed = True + return self.cache_types[0] + + def _link_matches(self, path_info): + is_hardlink = System.is_hardlink(path_info) + is_symlink = System.is_symlink(path_info) + is_copy_or_reflink = not is_hardlink and not is_symlink + + cache_type = self._get_cache_type(path_info) + + if cache_type == "symlink": + return is_symlink + + if cache_type == "hardlink": + return is_hardlink + + return is_copy_or_reflink diff --git a/tests/func/test_add.py b/tests/func/test_add.py index 6e7516c49..c11e0a3b6 100644 --- a/tests/func/test_add.py +++ b/tests/func/test_add.py @@ -553,3 +553,81 @@ def test_readding_dir_should_not_unprotect_all(dvc_repo, repo_dir): assert not unprotect_spy.mock.called assert System.is_symlink(new_file) + + +def test_should_not_checkout_when_adding_cached_copy(repo_dir, dvc_repo): + dvc_repo.cache.local.cache_types = ["copy"] + + dvc_repo.add(repo_dir.FOO) + dvc_repo.add(repo_dir.BAR) + + shutil.copy(repo_dir.BAR, repo_dir.FOO) + + copy_spy = spy(shutil.copyfile) + + RemoteLOCAL.CACHE_TYPE_MAP["copy"] = copy_spy + dvc_repo.add(repo_dir.FOO) + + assert copy_spy.mock.call_count == 0 + + +@pytest.mark.parametrize( + "link,new_link,link_test_func", + [ + ("hardlink", "copy", lambda path: not System.is_hardlink(path)), + ("symlink", "copy", lambda path: not System.is_symlink(path)), + ("copy", "hardlink", System.is_hardlink), + ("copy", "symlink", System.is_symlink), + ], +) +def test_should_relink_on_repeated_add( + link, new_link, link_test_func, repo_dir, dvc_repo +): + dvc_repo.cache.local.cache_types = [link] + + dvc_repo.add(repo_dir.FOO) + dvc_repo.add(repo_dir.BAR) + + os.remove(repo_dir.FOO) + RemoteLOCAL.CACHE_TYPE_MAP[link](repo_dir.BAR, repo_dir.FOO) + + dvc_repo.cache.local.cache_types = [new_link] + + dvc_repo.add(repo_dir.FOO) + + assert link_test_func(repo_dir.FOO) + + +@pytest.mark.parametrize( + "link, link_func", + [("hardlink", System.hardlink), ("symlink", System.symlink)], +) +def test_should_relink_single_file_in_dir(link, link_func, dvc_repo, repo_dir): + dvc_repo.cache.local.cache_types = [link] + + dvc_repo.add(repo_dir.DATA_DIR) + + # NOTE status triggers unpacked dir creation for hardlink case + dvc_repo.status() + + dvc_repo.unprotect(repo_dir.DATA_SUB) + + link_spy = spy(link_func) + RemoteLOCAL.CACHE_TYPE_MAP[link] = link_spy + dvc_repo.add(repo_dir.DATA_DIR) + + assert link_spy.mock.call_count == 1 + + +@pytest.mark.parametrize("link", ["hardlink", "symlink", "copy"]) +def test_should_protect_on_repeated_add(link, dvc_repo, repo_dir): + dvc_repo.cache.local.cache_types = [link] + dvc_repo.cache.local.protected = True + + dvc_repo.add(repo_dir.FOO) + + dvc_repo.unprotect(repo_dir.FOO) + + dvc_repo.add(repo_dir.FOO) + + assert not os.access(repo_dir.FOO, os.W_OK)