erepo: pull each time

Fixes #3062.

dvc/external_repo.py

@@ -1,9 +1,11 @@
+import logging
 import os
 import tempfile
 from contextlib import contextmanager
 from distutils.dir_util import copy_tree
+import threading
 
-from funcy import retry, suppress, memoize, cached_property
+from funcy import retry, suppress, wrap_with, cached_property
 
 from dvc.compat import fspath
 from dvc.repo import Repo
@@ -16,6 +18,9 @@ from dvc.utils.fs import remove, fs_copy
 from dvc.scm import SCM
 
 
+logger = logging.getLogger(__name__)
+
+
 @contextmanager
 def external_repo(url, rev=None):
     path = _cached_clone(url, rev)
@@ -40,8 +45,8 @@ def external_repo(url, rev=None):
 
 def clean_repos():
     # Outside code should not see cache while we are removing
-    repo_paths = list(_cached_clone.memory.values())
-    _cached_clone.memory.clear()
+    repo_paths = list(REPOS_CACHE.values())
+    REPOS_CACHE.clear()
 
     for path in repo_paths:
         _remove(path)
@@ -137,19 +142,28 @@ class ExternalGitRepo:
             raise PathMissingError(path, self.url)
 
 
-@memoize
+REPOS_CACHE = {}
+
+
+@wrap_with(threading.Lock())
 def _cached_clone(url, rev):
     """Clone an external git repo to a temporary directory.
 
     Returns the path to a local temporary directory with the specified
     revision checked out.
     """
+    if (url, rev) in REPOS_CACHE:
+        path = REPOS_CACHE[url, rev]
+        _git_pull(path, rev)
+        return path
+
     new_path = tempfile.mkdtemp("dvc-erepo")
 
-    if url in _cached_clone.memory:
+    if url in REPOS_CACHE:
         # Copy and an existing clean clone
         # This one unlike shutil.copytree() works with an existing dir
-        copy_tree(_cached_clone.memory[url], new_path)
+        copy_tree(REPOS_CACHE[url], new_path)
+        _git_pull(new_path, None)
     else:
         # Create a new clone
         _clone_repo(url, new_path)
@@ -157,21 +171,37 @@ def _cached_clone(url, rev):
         # Save clean clone dir so that we will have access to a default branch
         clean_clone_path = tempfile.mkdtemp("dvc-erepo")
         copy_tree(new_path, clean_clone_path)
-        _cached_clone.memory[url] = clean_clone_path
+        REPOS_CACHE[url] = clean_clone_path
 
     # Check out the specified revision
     if rev is not None:
         _git_checkout(new_path, rev)
 
+    REPOS_CACHE[url, rev] = new_path
     return new_path
 
 
-def _git_checkout(repo_path, revision):
+def _git_checkout(repo_path, rev):
     from dvc.scm import Git
 
     git = Git(repo_path)
     try:
-        git.checkout(revision)
+        git.checkout(rev)
+    finally:
+        git.close()
+
+
+def _git_pull(repo_path, rev):
+    import git
+
+    # Do not try to pull in a detached mode
+    if rev and git.Repo.re_hexsha_only.search(rev):
+        return
+
+    git = git.Repo(repo_path)
+    try:
+        msg = git.git.pull()
+        logger.debug("external repo: git pull: {}", msg)
     finally:
         git.close()
 

tests/func/test_external_repo.py

@@ -23,3 +23,16 @@ def test_external_repo(erepo_dir):
                 assert fd.read() == "branch"
 
         assert mock.call_count == 1
+
+
+def test_source_change(erepo_dir):
+    url = fspath(erepo_dir)
+    with external_repo(url) as repo:
+        old_rev = repo.scm.get_rev()
+
+    erepo_dir.scm_gen("file", "text", commit="a change")
+
+    with external_repo(url) as repo:
+        new_rev = repo.scm.get_rev()
+
+    assert old_rev != new_rev

tests/func/test_status.py

@@ -4,7 +4,6 @@ from mock import patch
 
 from dvc.main import main
 from dvc.compat import fspath
-from dvc.external_repo import clean_repos
 from tests.basic_env import TestDvc
 
 
@@ -33,19 +32,12 @@ def test_status_non_dvc_repo_import(tmp_dir, dvc, git_dir):
 
     dvc.imp(fspath(git_dir), "file", "file", rev="branch")
 
-    status = dvc.status(["file.dvc"])
-
-    assert status == {}
-
-    # Caching in external repos doesn't see upstream updates within single
-    # cli call, so we need to clean the caches to see the changes.
-    clean_repos()
+    assert dvc.status(["file.dvc"]) == {}
 
     with git_dir.branch("branch", new=False):
         git_dir.scm_gen("file", "second version", commit="update file")
 
     status, = dvc.status(["file.dvc"])["file.dvc"]
-
     assert status == {
         "changed deps": {"file ({})".format(git_dir): "update available"}
     }
@@ -68,12 +60,7 @@ def test_status_before_and_after_dvc_init(tmp_dir, dvc, git_dir):
 
     assert old_rev != new_rev
 
-    # Caching in external repos doesn't see upstream updates within single
-    # cli call, so we need to clean the caches to see the changes.
-    clean_repos()
-
     status, = dvc.status(["file.dvc"])["file.dvc"]
-
     assert status == {
         "changed deps": {
             "file ({})".format(fspath(git_dir)): "update available"

tests/func/test_update.py

@@ -3,7 +3,6 @@ import os
 
 from dvc.stage import Stage
 from dvc.compat import fspath
-from dvc.external_repo import clean_repos
 
 
 @pytest.mark.parametrize("cached", [True, False])
@@ -16,43 +15,21 @@ def test_update_import(tmp_dir, dvc, erepo_dir, cached):
 
     stage = dvc.imp(fspath(erepo_dir), "version", "version", rev="branch")
 
-    imported = tmp_dir / "version"
-    assert imported.is_file()
-    assert imported.read_text() == "branch"
-    assert stage.deps[0].def_repo == {
-        "url": fspath(erepo_dir),
-        "rev": "branch",
-        "rev_lock": old_rev,
-    }
+    assert (tmp_dir / "version").read_text() == "branch"
+    assert stage.deps[0].def_repo["rev_lock"] == old_rev
 
+    # Update version file
     with erepo_dir.branch("branch", new=False), erepo_dir.chdir():
         gen("version", "updated", "update version content")
         new_rev = erepo_dir.scm.get_rev()
 
     assert old_rev != new_rev
 
-    # Caching in external repos doesn't see upstream updates within single
-    # cli call, so we need to clean the caches to see the changes.
-    clean_repos()
-
-    status, = dvc.status([stage.path])["version.dvc"]
-    changed_dep, = list(status["changed deps"].items())
-    assert changed_dep[0].startswith("version ")
-    assert changed_dep[1] == "update available"
-
     dvc.update(stage.path)
-
-    assert dvc.status([stage.path]) == {}
-
-    assert imported.is_file()
-    assert imported.read_text() == "updated"
+    assert (tmp_dir / "version").read_text() == "updated"
 
     stage = Stage.load(dvc, stage.path)
-    assert stage.deps[0].def_repo == {
-        "url": fspath(erepo_dir),
-        "rev": "branch",
-        "rev_lock": new_rev,
-    }
+    assert stage.deps[0].def_repo["rev_lock"] == new_rev
 
 
 def test_update_import_after_remote_updates_to_dvc(tmp_dir, dvc, erepo_dir):
@@ -82,10 +59,6 @@ def test_update_import_after_remote_updates_to_dvc(tmp_dir, dvc, erepo_dir):
 
     assert old_rev != new_rev
 
-    # Caching in external repos doesn't see upstream updates within single
-    # cli call, so we need to clean the caches to see the changes.
-    clean_repos()
-
     status, = dvc.status([stage.path])["version.dvc"]
     changed_dep, = list(status["changed deps"].items())
     assert changed_dep[0].startswith("version ")
@@ -122,10 +95,6 @@ def test_update_before_and_after_dvc_init(tmp_dir, dvc, git_dir):
 
     assert old_rev != new_rev
 
-    # Caching in external repos doesn't see upstream updates within single
-    # cli call, so we need to clean the caches to see the changes.
-    clean_repos()
-
     assert dvc.status([stage.path]) == {
         "file.dvc": [
             {