Repository: iterative/dvc
Pull request: add: do not verify hardlink if file is empty
Fixes #3390

* [x] ❗ Have you followed the guidelines in the [Contributing to DVC](https://dvc.org/doc/user-guide/contributing/core) list?

* [x] 📖 Check this box if this PR **does not** require [documentation](https://dvc.org/doc) updates, or if it does **and** you have created a separate PR in [dvc.org](https://github.com/iterative/dvc.org) with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the [dvc.org](https://github.com/iterative/dvc.org) repo.

* [x] ❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. Please review them carefully and fix those that actually improve code or fix bugs.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏


Problem statement:
add: empty files add broken when cache mode is hardlinks
## DVC Version


```
DVC version: 0.86.5+f67314.mod
Python version: 3.7.6
Platform: Darwin-18.2.0-x86_64-i386-64bit
Binary: False
Package: None
Cache: reflink - supported, hardlink - supported, symlink - supported
Filesystem type (cache directory): ('apfs', '/dev/disk1s1')
Filesystem type (workspace): ('apfs', '/dev/disk1s1')
```

## Reproduce

```
dvc config cache.type hardlink
touch foo
dvc add foo
```

outputs:

```
ERROR: no possible cache types left to try out.
```

Hints from issue discussion:
Found the root cause of this. This happens because of the optimization that we do if the file is empty (i.e. 0 bytes size). We never create a hardlink for the file with 0 bytes size and therefore it fails when we try to verify if the hardlink was created.

https://github.com/iterative/dvc/blob/a9bc65ee1f0446de766db59ad1b149de064c5360/dvc/remote/local.py#L170-L182

I think, [`is_hardlink()`](https://github.com/iterative/dvc/blob/a9bc65ee1f0446de766db59ad1b149de064c5360/dvc/remote/local.py#L196) function should be aware of this optimization as well. I'll make a fix tomorrow. 

P.S. This only failed if the file was empty (not even a newline). So:
```sh
echo > foo && dvc add foo # works because of extra-newline character added by `echo`
touch bar && dvc add bar # does not work as the file is empty
```

Also, subsequent runs would not fail because before this error is thrown out, the file gets cached (however, deletes file from workspace), i.e.
```sh
dvc init --no-scm
dvc config cache.type hardlink
touch foo && dvc add foo  # fails and deletes foo from workspace
touch foo && dvc add foo  # passes
```
@skshetry Great investigation! :pray:

Commit message: add: do not verify hardlink if file is empty

Fixes #3390

Patch to review:
diff --git a/dvc/remote/local.py b/dvc/remote/local.py
index 24725c526d..3e62e0628e 100644
--- a/dvc/remote/local.py
+++ b/dvc/remote/local.py
@@ -92,6 +92,12 @@ def already_cached(self, path_info):
 
         return not self.changed_cache(current_md5)
 
+    def _verify_link(self, path_info, link_type):
+        if link_type == "hardlink" and self.getsize(path_info) == 0:
+            return
+
+        super()._verify_link(path_info, link_type)
+
     def is_empty(self, path_info):
         path = path_info.fspath
 
diff --git a/tests/func/test_add.py b/tests/func/test_add.py
index 95407ee7d0..e788c176c3 100644
--- a/tests/func/test_add.py
+++ b/tests/func/test_add.py
@@ -7,7 +7,7 @@
 
 import colorama
 import pytest
-from mock import patch
+from mock import patch, call
 
 import dvc as dvc_module
 from dvc.cache import Cache
@@ -662,3 +662,39 @@ def test_not_raises_on_re_add(tmp_dir, dvc):
 
     tmp_dir.gen({"file2": "file2 content", "file": "modified file"})
     dvc.add(["file2", "file"])
+
+
+@pytest.mark.parametrize("link", ["hardlink", "symlink", "copy"])
+def test_add_empty_files(tmp_dir, dvc, link):
+    file = "foo"
+    dvc.cache.local.cache_types = [link]
+    stages = tmp_dir.dvc_gen(file, "")
+
+    assert (tmp_dir / file).exists()
+    assert (tmp_dir / (file + Stage.STAGE_FILE_SUFFIX)).exists()
+    assert os.path.exists(stages[0].outs[0].cache_path)
+
+
+@mock.patch(
+    "dvc.remote.local.RemoteLOCAL.is_hardlink",
+    side_effect=RemoteLOCAL.is_hardlink,
+)
+def test_add_optimization_for_hardlink_on_empty_files(m, tmp_dir, dvc):
+    dvc.cache.local.cache_types = ["hardlink"]
+    tmp_dir.gen({"foo": "", "bar": "", "lorem": "lorem", "ipsum": "ipsum"})
+    stages = dvc.add(["foo", "bar", "lorem", "ipsum"])
+
+    m.assert_called_once()
+    assert m.call_args != call(tmp_dir / "foo")
+    assert m.call_args != call(tmp_dir / "bar")
+
+    for stage in stages[:2]:
+        # hardlinks are not created for empty files
+        assert not System.is_hardlink(stage.outs[0].path_info)
+
+    for stage in stages[2:]:
+        assert System.is_hardlink(stage.outs[0].path_info)
+
+    for stage in stages:
+        assert os.path.exists(stage.path)
+        assert os.path.exists(stage.outs[0].cache_path)


Code review comments:
diff_hunk: @@ -662,3 +662,39 @@ def test_not_raises_on_re_add(tmp_dir, dvc):
 
     tmp_dir.gen({"file2": "file2 content", "file": "modified file"})
     dvc.add(["file2", "file"])
+
+
+@pytest.mark.parametrize("link", ["hardlink", "symlink", "copy"])
+def test_add_empty_files(tmp_dir, dvc, link):
+    file = "foo"
+    dvc.cache.local.cache_types = [link]
+    stages = tmp_dir.dvc_gen(file, "")
+
+    assert (tmp_dir / file).exists()
+    assert (tmp_dir / (file + Stage.STAGE_FILE_SUFFIX)).exists()
+    assert os.path.exists(stages[0].outs[0].cache_path)
+
+
+@mock.patch(
+    "dvc.remote.local.RemoteLOCAL.is_hardlink",
+    side_effect=RemoteLOCAL.is_hardlink,
+)
path: tests/func/test_add.py
text: @user1:
You can use `mocker.spy` for that. Sample usage:
`test_should_remove_outs_before_import`.

@author:
Thanks @user1. Adjusted. :fire: