Repository: iterative/dvc

Issue:
Bug: `dvc add` fails with a modified file (or directory) at the end of a list of files
Using DVC 0.71.0 on RHEL6, installed via `conda`, configured for system-wide hard links (not sure if it's relevant):

```
$ dvc --version
0.71.0

$ cat ~/.config/dvc/config
[cache]
protected = true
type = hardlink
```

I have come across what appears to be a bug, where attempting to `dvc add` a directory previously under DVC version control whose contents have changed, results in an error, but only when adding along with a list of files, and doesn't occur if the command is repeated (i.e. after all the other files have been added).  I have reproduced this in several project directories and via the following minimal example (courtesy of @mroutis):

```
dvc init --no-scm
mkdir data
echo "foo" > data/foo
dvc add data
echo "bar" > bar.txt
dvc unprotect data
echo "change" > data/foo
dvc add bar.txt data
```

This results in the following output:

```
WARNING: Output 'data' of 'data.dvc' changed because it is 'modified'
100%|██████████|Add                                       2.00/2.00 [00:02<00:00,  1.08s/file]
ERROR: file/directory 'data' is specified as an output in more than one stage: data.dvc
    data.dvc
```

But if the command is re-run:

```
$ dvc add bar.txt data
Stage is cached, skipping.
100% Add                                                  1.00/1.00 [00:01<00:00,  1.96s/file]
```

So it appears `dvc` is somehow mishandling the list of files.  Of course, the expected behavior is that it will add the directory successfully on the first try.

Thanks for any effort to track down the source of the bug.






Discussion:
efiop: @pared Looks like you've forgotten to methion the ticket that this is closing :)
efiop: Also, let's add a test. :slightly_smiling_face: 
efiop: From discussion in https://github.com/iterative/dvc/issues/2886 it seems like only `dvc add` for multiple targets is affected, right? Other things like import/run/etc are not? If so, looks like we only need to add a `_reset()` call in `add()`.
pared: @efiop, that is actually not true, as I am inspecting the code, `run` seems to be affected too,
situation is the same: we run `check_modified_graph`, and if we call `stages` before, it will fail.
Example:
```
def test_run_update(tmp_dir, dvc):
    dvc.run(outs=["file"], cmd="echo content > file")
    dvc.stages
    dvc.run(outs=["file"], cmd="echo new_content > file")
```
pared: So to summarize: the problem occurs when we are re-`creating` stage and then running `check_modified_graph` each case of this methods combination is vulnerable to cached `stages`.
efiop: @pared Looks like there is a miscommunication here. You are showing tests that are not really replicating what happens in the original, issue, right?
```
repo.stages
repo.add(...)
```
is not the same as what we get in that error, where we `repo.add(["foo", "bar"])`. I understand that you could trigger that behaviour with something synthetic like calling `repo.stages` before `repo.run/add/imp`, but I'm trying to understand what happened in the original issue. Could you please explain what happens in the original issue, please?
efiop: Ok, so what happens is we have
```
for target in targets:
    stages = _create_stages...
    repo.check_modified_graph(stages)
```
and when `data.dvc` already exists and you pass `["bar", "data"]` as `targets`, dvc collects `repo.stages`(incuding pre-existing `data.dvc`) in `check_modified_graph` when processing `bar` and on the next run, when processing `data`, it sees that it already has `data.dvc` in `repo.stages` and also `stages` contains `data.dvc`, and so it detects the collision. And if `data` is specified as the first entry in `targets`, that bug doesn't happen, because `repo.stages` gets collected after `_create_stages` when `data.dvc` doesn't exist(because it gets deleted if it already exists) and so `repo.stages` don't contain `data.dvc` and `check_modified_graph(["data.dvc"])` doesn't detect any issues.

And so this particular issue doesn't happen on `run/imp/etc` because they don't accept list of targets.
pared: > You are showing tests that are not really replicating what happens in the original, issue, right?

I believe I do. Yes, you are right, that they do not accept targets, but it is still possible to replicate same 
problem (OverlappingOutputPaths) even without having multiple targets. The original cause is that we cache the `repo.stages` and don't invalidate them when we are deleting stage file upon `Stae.create`. This is the root of the problem and needs to be fixed, because now, simply calling `repo.stages` somewhere in code makes `repo.check_modified_graph` unpredictible.
efiop: @pared Ah, i see. I guess the miscommunication is that I'm talking about CLI and in it you won't be able to replicate the issue anywhere except in `dvc add target1 target2`, right? But your point of invalidating stages when something gets deleted/overwritten is great, that would indeed make the API more robust. Wonder if the current solution could be improved to not mixup the context or if it is inevitable. Probably it is caused by the `create` method doing too much stuff.
efiop: @pared The reason I'm trying to clarify that your tests are not strictly the same as the original issue is because your test cases could be solved by resetting in `locked` decorator before grabbing the lock, which would be an elegant solution for it. And the original issue won't be solved by that, because there are other things going on, as we are talking about method internals.
pared: @efiop, ah that is right, for the original issue resetting in locked decorator will not solve the problem.
It seems to me that the most "universal" solution is the one I proposed originally, that is resetting the `repo` when unlinking the stage file. It takes care of both situations, at the cost of entangling `Repo` with `Stage`. Maybe we should reconsider the amount of responsibility that `Stage` has? I would need to take a deeper look at it to propose some alternatives.

efiop: @pared Or maybe we could combine `reset()` in `locked` with `reset` inside `add()`? 🙂 That would not spread the context and would solve both issues. What do you think?
pared: @efiop I think you are right about resetting during add. Basically any call for `Stage.create` invalidates current `repo.stages`. Because, well, we are creating a new stage.

Patch:
diff --git a/dvc/repo/__init__.py b/dvc/repo/__init__.py
index 36dc6b073..6205e24e5 100644
--- a/dvc/repo/__init__.py
+++ b/dvc/repo/__init__.py
@@ -24,6 +24,7 @@ def locked(f):
     @wraps(f)
     def wrapper(repo, *args, **kwargs):
         with repo.lock, repo.state:
+            repo._reset()
             ret = f(repo, *args, **kwargs)
             # Our graph cache is no longer valid after we release the repo.lock
             repo._reset()
diff --git a/dvc/repo/add.py b/dvc/repo/add.py
index f216d6dde..b0a0fa586 100644
--- a/dvc/repo/add.py
+++ b/dvc/repo/add.py
@@ -126,6 +126,7 @@ def _create_stages(repo, targets, fname, pbar=None):
         stage = Stage.create(
             repo, outs=[out], accompany_outs=True, fname=fname
         )
+        repo._reset()
 
         if not stage:
             if pbar is not None:


Test patch:
diff --git a/tests/func/test_add.py b/tests/func/test_add.py
index 13b83c631..95407ee7d 100644
--- a/tests/func/test_add.py
+++ b/tests/func/test_add.py
@@ -655,3 +655,10 @@ def test_add_from_data_dir(tmp_dir, scm, dvc):
         "tracked output: 'dir'.\n"
         "To include '{out}' in 'dir', run 'dvc commit dir.dvc'"
     ).format(out=os.path.join("dir", "file2"))
+
+
+def test_not_raises_on_re_add(tmp_dir, dvc):
+    tmp_dir.dvc_gen("file", "file content")
+
+    tmp_dir.gen({"file2": "file2 content", "file": "modified file"})
+    dvc.add(["file2", "file"])
diff --git a/tests/unit/repo/test_repo.py b/tests/unit/repo/test_repo.py
index d544da793..b6ee260de 100644
--- a/tests/unit/repo/test_repo.py
+++ b/tests/unit/repo/test_repo.py
@@ -2,6 +2,8 @@ import os
 
 import pytest
 
+from dvc.repo import locked
+
 
 def test_is_dvc_internal(dvc):
     assert dvc.is_dvc_internal(os.path.join("path", "to", ".dvc", "file"))
@@ -49,3 +51,18 @@ def test_used_cache(tmp_dir, dvc, path):
             used_cache._items == expected._items
             and used_cache.external == expected.external
         )
+
+
+def test_locked(mocker):
+    repo = mocker.MagicMock()
+    repo.method = locked(repo.method)
+
+    args = {}
+    kwargs = {}
+    repo.method(repo, args, kwargs)
+
+    assert repo.method_calls == [
+        mocker.call._reset(),
+        mocker.call.method(repo, args, kwargs),
+        mocker.call._reset(),
+    ]