Repository: iterative/dvc

Issue:
Misleading warning (Removing before checkout)
During a manual update of a file, I've faced the following message:
>WARNING: data 'foo' exists. Removing before checkout.

It puzzled me, I thought it would overwrite my file, since checkout restores the file based on current DVC file.
I don't know why the file was replaced by the same file. It took a long time (huge file on a HDD).

@efiop provided me a script that reproduces the message:
```
#!/bin/bash     
                
set -e          
set -x          
                
rm -rf myrepo   
mkdir myrepo    
cd myrepo       
                
git init        
dvc init        
                
echo foo > foo  
dvc add foo     
                
echo bar > bar  
dvc add bar     
                
echo bar > foo  
dvc add foo     
cat foo         
dvc status
```

I think a better message and avoiding re-copying the file would be great improvements.

Discussion:
pared: @shcheklein 
I've been thinking what should happen when dealing with protected cache.
We have 2 options:
- we have protected link, in such case we probably should not remove it.
- the link was unprotected, so now it is copy of file in cache, and should probably be removed as 
`dvc add` shows intention of re-adding file under dvc control, so it should be handled as "new" file.

What do you think? 
shcheklein: @pared so, I'm not sure the "remove" term is correct here. From the user perspective they should see the same set of files. The only difference is that for some of them we don't copy/link them from the cache again. Can we just do checkout and protecting as two different steps? Protection/linking can be run afterwards for files that are clearly not protected/linked.

And only if it's needed during that phase we can write a message (not sure about WARNING) that we are replacing the file with a link (effectively removing in the workspace). It should be clear that we are not completely removing it. It's just in certain cases we are replacing it with a link.
pared: @shcheklein 
> The only difference is that for some of them we don't copy/link them from the cache again. Can we just do checkout and protecting as two different steps?

I think we are drifting a bit from original problem. In this PR, we are supposed to prevent unnecessary removing and checking out file that already exists. The special case we have to handle is, `what should happen when we have identical file, but it is unprotected?`

I don't think discussion if we could protect after checkout is scope of this issue, since logic related to `protect` is embedded inside `RemoteLOCAL._do_link` and changing that would require some other changes. 

What I think we could do, is to try to detect if particular file we are supposed to checkout is protected, and if it is not, don't remove it, but protect it. In that case, such file would not behave as in `normal checkout` case.
Why? Because in normal checkout, when we use some linking from cache, file is linked to its counterpart in cache. In case described above (unprotected file) this file is no longer a link, its copy of file from cache. So, protecting it will make it `protected` but it will not be a link from cache, which it should be in this case.
shcheklein: @pared  I think I was fine with your initial logic (detect unprotected files and restore an appropriate link - even if requires removing it from the workspace). My concern that this ticket is not only about optimization but also about a misleading warning in two different cases. Would be great to use a separate message in this case - like protecting the file, or relinking the file.

Btw, should the same apply to the unprotected DVC project, when we have a copy of the file not a link? Should we go and restore the link in this case as part of the `dvc checkout`? It feels reasonable to me.  @efiop can you give more insights if we are doing this already and what do you think about this?

Then it becomes a more general check, not only for the sake of protecting files, but saving some space for example. It can be useful, when we change cache type. We are "drifting" a bit again, but I think it's fine if it gives more insights or leads us to a better implementation :)

efiop: @shcheklein

> Btw, should the same apply to the unprotected DVC project, when we have a copy of the file not a link? Should we go and restore the link in this case as part of the dvc checkout? It feels reasonable to me. @efiop can you give more insights if we are doing this already and what do you think about this?

It was actually part of the reason why this change wasn't implemented earlier. Currently we have a naive approach where checkout simply re-does the link, but after this patch it won't. So, as you've said earlier, there is indeed more to this ticket if we want to solve it properly. This patch will make dvc not remove duplicates, which would be an issue. So I wouldn't merge this as is.
pared: @efiop 
Is my understanding of this issue right?
Problem: We don't want to remove already existing file on checkout, if we know that it is already under dvc control.

Possible cases:
- We have unprotected repo -> no need to checkout, maybe inform user that file already exists
- Protected repo, user wants to checkout protected file -> same as above
- protected repo, unprotected file -> we should relink file from cache, so it has same properties as newly added file

efiop: @pared Yes, correct. And the main thing that we need to add to your current patch is link type detection, it looks like, so we could correctly decide if we need to relink the file.
pared: @efiop 
Ok, so we are on the same page.

So I guess, in 3rd case, I should detect whether already existing file is of same link type as cache:
- if it is, we don't have to checkout
- if its not, we need to relink the file
efiop: A little summary to be clear:

> We have unprotected repo -> no need to checkout, maybe inform user that file already exists

Need to check if link type matches, if it is a copy but we have links, relink with a link to avoid dublication. Also need to verify that the file is no longer protected.

> Protected repo, user wants to checkout protected file -> same as above

Same as before, need to check link types to avoid duplication. Need to verify that the file is protected.

> protected repo, unprotected file -> we should relink file from cache, so it has same properties as newly added file

And this one becomes a regular case for 2).


pared: @efiop, ok, my previous response was wrong. Ill work on adjusting this change to handle link types.
efiop: @pared Check PR formatting.
pared: @mroutis thanks for pointing this one out. Ill look into it
pared: Case pointed out by @mroutis  is a little bit different than one descirbed in original issue, though it boils down to similar question that has been stated by original issue. 

@mroutis file is not checked out, because `remote/base.checkout` performs its own checks on existing path info, and if it doesn't detect any changes, skips `_checkout` invocation which would handle link issues.

I think `checkout` should also take into consideration cache protection and link type and "reset" existing file.

For example, following test passes, and it should not:
```
def test_should_relink_on_unprotected(dvc_repo, repo_dir):
    dvc_repo.cache.local.cache_types = ["hardlink"]
    dvc_repo.add(repo_dir.FOO)

    dvc_repo.cache.local.cache_types = ["copy"]
    dvc_repo.checkout(repo_dir.FOO + ".dvc")

    assert System.is_hardlink(repo_dir.FOO)
```

I think behavior for both `checkout` and `add` in case of existing file should be unified and should reset currently set protection level and link type. 
efiop: @pared we can have a separate ticket for `checkout`, we've discussed it previously and that `if not self.changed(): return` in RemoteBASE.checkout is the one that is causing that behaviour. With `dvc add` it is indeed a little different and more explicit, and this PR handles that.
pared: @efiop I rolled back checkout change, and changed test a bit
efiop: @pared your merge/rebase situation is a mess. Please stick to one or another.
pared: Need to test for directories and check performance.
Ideally, `_checkout_dir` should reuse `_checkout_file` for each entry. Need to check whether performance impact of such solution will not be too much.
pared: Test for re-adding directory:
```
#!/bin/bash

rm -rf repo
mkdir repo

cd repo

mkdir data
for i in {1..100000}
do
	echo $i>>data/$i
done

set -e 
set -x

git init >> /dev/null && dvc init -q
dvc config cache.type hardlink

dvc add data -q

dvc unprotect data/1

time dvc add data
```

as we can see I unprotect one file and `time` adding whole directory again. 
If I replace logic implemented for [each entry](https://github.com/iterative/dvc/blob/41351a2df2f36ed04846d5e3d976d6d99b71a151/dvc/remote/base.py#L705) with new implementation of `_checkout_file`, the time of second add doubles comparing to current implementation.
- For 10K files: 9s -> 20s
- For 100K files: 50s -> 95s
 
Worth noting, that it might be partially due to fact that `stderr` is flooded with warnings `Removing before checkout`. 

 I guess we could cut some of this time if we would be checking `cache` link type against `entry_info` link type to avoid relinking for each `entry_info`.

The same test performed for `cache.type=copy` yielded about same execution time.



efiop: @pared so this patch currently doesn't work for directories, right? 

> Worth noting, that it might be partially due to fact that stderr is flooded with warnings Removing before checkout.

So try removing those and see if that helps, should be pretty trivial :slightly_smiling_face: 
pared: @efiop Seems like logging is a big part of the additional time. For 100K files, just removing this warning reduced additional time by 50% (instead of 95 s we had about 70) Still, there is some overhead for relinking. Ill try to implement simple check that would avoid relinking.
pared: @efiop 
I pushed unfinished change that would solve original issue and let us reuse `_checkout_file` inside `_checkout_dir`. Test of re-adding 100K element dir, when one file has been unprotected shows that the overhead is 2 seconds (for whole operation taking 55 seconds). It seems to me that it should be the way to go. Though I believe we should implement some kind of mechanism that would allow us to cache link type and pass the information that it is the "working" one, because currently, in case of reflink, we will be repeatedly perform link type test.
pared: @efiop @Suor Here is short summary of last few days of disussion and comments for code.

I believe to solve original problem for files added "through" adding directory, I need to either:
- duplicate most of the code of `_checkout_file` in `_checkout_dir`

or

- reuse `_checkout_file` in `_checkout_dir`

I think that proper solution is the second one, but in order to make it work properly I need to:
- implement mechanism for caching "working" link type 
- add flag `save_link` in `_checkout_file` to prevent excessive growth of state db when adding directory
- add flag `make_copy` to `RemoteLOCAL._unprotect_file`


efiop: @pared that sounds good.
Suor: This gets or of control with all those flags. We would stop and rethink this.
pared: Maybe we should finish this up for single file and then rethink and implement it for dir?
efiop: @Suor Could you clarify? The only flag added that I see is `allow_copy`.
efiop: Thanks!

Patch:
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()))
-
-            # 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)
+            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)
 
         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


Test patch:
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)