remote: base: don't checkout existing file

[pared]

• Have you followed the guidelines in our
  Contributing document?

• Does your PR affect documented changes or does it add new functionality
  that should be documented? If yes, have you created a PR for
  dvc.org documenting it or at
  least opened an issue for it? If so, please add a link to it.

---

Fixes #2016

[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]

Btw, this function would work perfectly fine for non-local remotes too, it is just that _is_same_link_as_cache would return True as all they support currently is copy.

[efiop]

Looks good! A few more comments down below.

[efiop]

LGTM.

[efiop]

Thanks!