get/import/url: handle existing output directory (#2610)

* get/import/url: handle existing output directory

Currently if you run

```
dvc get https://github.com/iterative/example-get-started model.pkl --out .
```

it will nuke your cwd. To fix that we need to properly handle existing
directories, same as usual unix cli utilities do that.

* travis: specify python version when installing from chocolatey

Fixes #2609

dvc/repo/get.py

@@ -13,15 +13,14 @@ from dvc.exceptions import (
 from dvc.path_info import PathInfo
 from dvc.external_repo import external_repo
 from dvc.state import StateNoop
-from dvc.utils import remove
-from dvc.utils.compat import urlparse
+from dvc.utils import remove, resolve_output
 
 logger = logging.getLogger(__name__)
 
 
 @staticmethod
 def get(url, path, out=None, rev=None):
-    out = out or os.path.basename(urlparse(path).path)
+    out = resolve_output(path, out)
 
     if Stage.is_valid_filename(out):
         raise GetDVCFileError()

dvc/repo/get_url.py

@@ -3,12 +3,12 @@ import os
 import dvc.output as output
 import dvc.dependency as dependency
 
-from dvc.utils.compat import urlparse
+from dvc.utils import resolve_output
 
 
 @staticmethod
 def get_url(url, out=None):
-    out = out or os.path.basename(urlparse(url).path)
+    out = resolve_output(url, out)
 
     if os.path.exists(url):
         url = os.path.abspath(url)

dvc/repo/imp_url.py

@@ -1,4 +1,4 @@
-from dvc.utils.compat import pathlib
+from dvc.utils import resolve_output
 from dvc.repo.scm_context import scm_context
 
 from . import locked as locked_repo
@@ -9,7 +9,7 @@ from . import locked as locked_repo
 def imp_url(self, url, out=None, fname=None, erepo=None, locked=True):
     from dvc.stage import Stage
 
-    out = out or pathlib.PurePath(url).name
+    out = resolve_output(url, out)
 
     stage = Stage.create(
         self, cmd=None, deps=[url], outs=[out], fname=fname, erepo=erepo

dvc/utils/__init__.py

@@ -460,3 +460,14 @@ def relpath(path, start=os.curdir):
     ):
         return path
     return os.path.relpath(path, start)
+
+
+def resolve_output(inp, out):
+    from dvc.utils.compat import urlparse
+
+    name = os.path.basename(urlparse(inp).path)
+    if not out:
+        return name
+    if os.path.isdir(out):
+        return os.path.join(out, name)
+    return out

scripts/ci/before_install.sh

@@ -48,7 +48,7 @@ if [[ "$TRAVIS_OS_NAME" == "windows" ]]; then
         choco install python2
         echo 'PATH="/c/Python27:/c/Python27/Scripts:$PATH"' >> env.sh
     else
-        choco install python
+        choco install python --version 3.7.4
         echo 'PATH="/c/Python37:/c/Python37/Scripts:$PATH"' >> env.sh
     fi
 elif [[ "$TRAVIS_OS_NAME" == "osx" ]]; then

tests/func/test_get.py

@@ -1,3 +1,5 @@
+from __future__ import unicode_literals
+
 import os
 import filecmp
 
@@ -5,6 +7,7 @@ import pytest
 
 from dvc.exceptions import UrlNotDvcRepoError, GetDVCFileError
 from dvc.repo import Repo
+from dvc.utils import makedirs
 
 from tests.utils import trees_equal
 
@@ -51,3 +54,17 @@ def test_get_from_non_dvc_repo(git_erepo):
 def test_get_a_dvc_file(repo_dir, erepo):
     with pytest.raises(GetDVCFileError):
         Repo.get(erepo.root_dir, "some_file.dvc")
+
+
+@pytest.mark.parametrize("dname", [".", "dir", "dir/subdir"])
+def test_get_to_dir(dname, repo_dir, erepo):
+    src = erepo.FOO
+
+    makedirs(dname, exist_ok=True)
+
+    Repo.get(erepo.root_dir, src, dname)
+
+    dst = os.path.join(dname, os.path.basename(src))
+
+    assert os.path.isdir(dname)
+    assert filecmp.cmp(repo_dir.FOO, dst, shallow=False)

tests/func/test_get_url.py

@@ -1,7 +1,12 @@
+from __future__ import unicode_literals
+
 import os
 import filecmp
 
+import pytest
+
 from dvc.repo import Repo
+from dvc.utils import makedirs
 
 
 def test_get_file(repo_dir):
@@ -13,3 +18,17 @@ def test_get_file(repo_dir):
     assert os.path.exists(dst)
     assert os.path.isfile(dst)
     assert filecmp.cmp(repo_dir.FOO, dst, shallow=False)
+
+
+@pytest.mark.parametrize("dname", [".", "dir", "dir/subdir"])
+def test_get_url_to_dir(dname, repo_dir):
+    src = repo_dir.DATA
+
+    makedirs(dname, exist_ok=True)
+
+    Repo.get_url(src, dname)
+
+    dst = os.path.join(dname, os.path.basename(src))
+
+    assert os.path.isdir(dname)
+    assert filecmp.cmp(repo_dir.DATA, dst, shallow=False)

tests/func/test_import.py

@@ -1,3 +1,5 @@
+from __future__ import unicode_literals
+
 import os
 import filecmp
 
@@ -6,6 +8,7 @@ from mock import patch
 from tests.utils import trees_equal
 
 from dvc.stage import Stage
+from dvc.utils import makedirs
 from dvc.exceptions import DownloadError
 
 
@@ -80,3 +83,18 @@ def test_download_error_pulling_imported_stage(dvc_repo, erepo):
         "dvc.remote.RemoteLOCAL._download", side_effect=Exception
     ), pytest.raises(DownloadError):
         dvc_repo.pull(["foo_imported.dvc"])
+
+
+@pytest.mark.parametrize("dname", [".", "dir", "dir/subdir"])
+def test_import_to_dir(dname, repo_dir, dvc_repo, erepo):
+    src = erepo.FOO
+
+    makedirs(dname, exist_ok=True)
+
+    stage = dvc_repo.imp(erepo.root_dir, src, dname)
+
+    dst = os.path.join(dname, os.path.basename(src))
+
+    assert stage.outs[0].fspath == os.path.abspath(dst)
+    assert os.path.isdir(dname)
+    assert filecmp.cmp(repo_dir.FOO, dst, shallow=False)

tests/func/test_import_url.py

@@ -1,12 +1,17 @@
-import dvc
-
-from dvc.utils.compat import str
+from __future__ import unicode_literals
 
 import os
 from uuid import uuid4
 
-from dvc.main import main
+import pytest
+import filecmp
 from mock import patch
+
+import dvc
+from dvc.main import main
+from dvc.utils import makedirs
+from dvc.utils.compat import str
+
 from tests.basic_env import TestDvc
 from tests.utils import spy
 
@@ -86,3 +91,18 @@ class TestImportFilename(TestDvc):
         ret = main(["import-url", "--file", path, self.external_source])
         self.assertEqual(0, ret)
         self.assertTrue(os.path.exists(path))
+
+
+@pytest.mark.parametrize("dname", [".", "dir", "dir/subdir"])
+def test_import_url_to_dir(dname, repo_dir, dvc_repo):
+    src = repo_dir.DATA
+
+    makedirs(dname, exist_ok=True)
+
+    stage = dvc_repo.imp_url(src, dname)
+
+    dst = os.path.join(dname, os.path.basename(src))
+
+    assert stage.outs[0].fspath == os.path.abspath(dst)
+    assert os.path.isdir(dname)
+    assert filecmp.cmp(repo_dir.DATA, dst, shallow=False)