Merge pull request #5587 from cjerdonek/vcs-move-obtain-to-base-class

Implement VersionControl.obtain() in the base class.

src/pip/_internal/vcs/__init__.py

@@ -232,6 +232,24 @@ class VersionControl(object):
         url = urllib_parse.urlunsplit((scheme, netloc, path, query, ''))
         return url, rev
 
+    def get_url_rev_args(self, url):
+        """
+        Return the URL and RevOptions "extra arguments" to use in obtain(),
+        as a tuple (url, extra_args).
+        """
+        return url, []
+
+    def get_url_rev_options(self):
+        """
+        Return the URL and RevOptions object to use in obtain(), as a tuple
+        (url, rev_options).
+        """
+        url, rev = self.get_url_rev()
+        url, extra_args = self.get_url_rev_args(url)
+        rev_options = self.make_rev_options(rev, extra_args=extra_args)
+
+        return url, rev_options
+
     def get_info(self, location):
         """
         Returns (url, revision), where both are strings
@@ -253,13 +271,6 @@ class VersionControl(object):
         """
         return (self.normalize_url(url1) == self.normalize_url(url2))
 
-    def obtain(self, dest):
-        """
-        Called when installing or updating an editable package, takes the
-        source path of the checkout.
-        """
-        raise NotImplementedError
-
     def fetch_new(self, dest, url, rev_options):
         """
         Fetch a revision from a repository, in the case that this is the
@@ -299,18 +310,19 @@ class VersionControl(object):
         """
         raise NotImplementedError
 
-    def check_destination(self, dest, url, rev_options):
+    def obtain(self, dest):
         """
-        Prepare a location to receive a checkout/clone.
-
-        Return True if the location is ready for (and requires) a
-        checkout/clone, False otherwise.
+        Install or update in editable mode the package represented by this
+        VersionControl object.
 
         Args:
-          rev_options: a RevOptions object.
+          dest: the repository directory in which to install or update.
         """
+        url, rev_options = self.get_url_rev_options()
+
         if not os.path.exists(dest):
-            return True
+            self.fetch_new(dest, url, rev_options)
+            return
 
         rev_display = rev_options.to_display()
         if os.path.exists(os.path.join(dest, self.dirname)):
@@ -332,7 +344,7 @@ class VersionControl(object):
                     self.update(dest, rev_options)
                 else:
                     logger.info('Skipping because already up-to-date.')
-                return False
+                return
 
             logger.warning(
                 '%s %s in %s exists with URL %s',
@@ -365,7 +377,8 @@ class VersionControl(object):
         if response == 'w':
             logger.warning('Deleting %s', display_path(dest))
             rmtree(dest)
-            return True
+            self.fetch_new(dest, url, rev_options)
+            return
 
         if response == 'b':
             dest_dir = backup_dir(dest)
@@ -373,8 +386,10 @@ class VersionControl(object):
                 'Backing up %s to %s', display_path(dest), dest_dir,
             )
             shutil.move(dest, dest_dir)
-            return True
+            self.fetch_new(dest, url, rev_options)
+            return
 
+        # Do nothing if the response is "i".
         if response == 's':
             logger.info(
                 'Switching %s %s to %s%s',
@@ -385,10 +400,6 @@ class VersionControl(object):
             )
             self.switch(dest, url, rev_options)
 
-        # Do nothing if the response is "i".
-
-        return False
-
     def unpack(self, location):
         """
         Clean up current location and download the url repository
@@ -410,7 +421,8 @@ class VersionControl(object):
     def get_url(self, location):
         """
         Return the url used at location
-        Used in get_info or check_destination
+
+        This is used in get_info() and obtain().
         """
         raise NotImplementedError
 

src/pip/_internal/vcs/bazaar.py

@@ -66,12 +66,6 @@ class Bazaar(VersionControl):
         cmd_args = ['pull', '-q'] + rev_options.to_args()
         self.run_command(cmd_args, cwd=dest)
 
-    def obtain(self, dest):
-        url, rev = self.get_url_rev()
-        rev_options = self.make_rev_options(rev)
-        if self.check_destination(dest, url, rev_options):
-            self.fetch_new(dest, url, rev_options)
-
     def get_url_rev(self):
         # hotfix the URL scheme after removing bzr+ from bzr+ssh:// readd it
         url, rev = super(Bazaar, self).get_url_rev()

src/pip/_internal/vcs/git.py

@@ -203,12 +203,6 @@ class Git(VersionControl):
         #: update submodules
         self.update_submodules(dest)
 
-    def obtain(self, dest):
-        url, rev = self.get_url_rev()
-        rev_options = self.make_rev_options(rev)
-        if self.check_destination(dest, url, rev_options):
-            self.fetch_new(dest, url, rev_options)
-
     def get_url(self, location):
         """Return URL of the first remote encountered."""
         remotes = self.run_command(

src/pip/_internal/vcs/mercurial.py

@@ -64,12 +64,6 @@ class Mercurial(VersionControl):
         cmd_args = ['update', '-q'] + rev_options.to_args()
         self.run_command(cmd_args, cwd=dest)
 
-    def obtain(self, dest):
-        url, rev = self.get_url_rev()
-        rev_options = self.make_rev_options(rev)
-        if self.check_destination(dest, url, rev_options):
-            self.fetch_new(dest, url, rev_options)
-
     def get_url(self, location):
         url = self.run_command(
             ['showconfig', 'paths.default'],

src/pip/_internal/vcs/subversion.py

@@ -61,9 +61,8 @@ class Subversion(VersionControl):
 
     def export(self, location):
         """Export the svn repository at the url to the destination location"""
-        url, rev = self.get_url_rev()
-        rev_options = get_rev_options(self, url, rev)
-        url = remove_auth_from_url(url)
+        url, rev_options = self.get_url_rev_options()
+
         logger.info('Exporting svn repository %s to %s', url, location)
         with indent_log():
             if os.path.exists(location):
@@ -92,13 +91,6 @@ class Subversion(VersionControl):
         cmd_args = ['update'] + rev_options.to_args() + [dest]
         self.run_command(cmd_args)
 
-    def obtain(self, dest):
-        url, rev = self.get_url_rev()
-        rev_options = get_rev_options(self, url, rev)
-        url = remove_auth_from_url(url)
-        if self.check_destination(dest, url, rev_options):
-            self.fetch_new(dest, url, rev_options)
-
     def get_location(self, dist, dependency_links):
         for url in dependency_links:
             egg_fragment = Link(url).egg_fragment
@@ -147,6 +139,12 @@ class Subversion(VersionControl):
             url = 'svn+' + url
         return url, rev
 
+    def get_url_rev_args(self, url):
+        extra_args = get_rev_options_args(url)
+        url = remove_auth_from_url(url)
+
+        return url, extra_args
+
     def get_url(self, location):
         # In cases where the source is in a subdirectory, not alongside
         # setup.py we have to look up in the location until we find a real
@@ -225,9 +223,9 @@ class Subversion(VersionControl):
         return False
 
 
-def get_rev_options(vcs, url, rev):
+def get_rev_options_args(url):
     """
-    Return a RevOptions object.
+    Return the extra arguments to pass to RevOptions.
     """
     r = urllib_parse.urlsplit(url)
     if hasattr(r, 'username'):
@@ -250,7 +248,7 @@ def get_rev_options(vcs, url, rev):
     if password:
         extra_args += ['--password', password]
 
-    return vcs.make_rev_options(rev, extra_args=extra_args)
+    return extra_args
 
 
 vcs.register(Subversion)

tests/unit/test_vcs.py

@@ -177,3 +177,38 @@ def test_bazaar_simple_urls():
 def test_get_git_version():
     git_version = Git().get_git_version()
     assert git_version >= parse_version('1.0.0')
+
+
+# The non-SVN backends all have the same get_url_rev_args() implementation,
+# so test with Git as a representative.
+@pytest.mark.parametrize('url, expected', [
+    # Test a basic case.
+    ('git+https://git.example.com/MyProject#egg=MyProject',
+     ('git+https://git.example.com/MyProject#egg=MyProject', [])),
+    # Test with username and password.
+    ('git+https://user:pass@git.example.com/MyProject#egg=MyProject',
+     ('git+https://user:pass@git.example.com/MyProject#egg=MyProject', [])),
+])
+def test_git__get_url_rev_args(url, expected):
+    """
+    Test Git.get_url_rev_args().
+    """
+    actual = Git().get_url_rev_args(url)
+    assert actual == expected
+
+
+@pytest.mark.parametrize('url, expected', [
+    # Test a basic case.
+    ('svn+https://svn.example.com/MyProject#egg=MyProject',
+     ('svn+https://svn.example.com/MyProject#egg=MyProject', [])),
+    # Test with username and password.
+    ('svn+https://user:pass@svn.example.com/MyProject#egg=MyProject',
+     ('svn+https://svn.example.com/MyProject#egg=MyProject',
+      ['--username', 'user', '--password', 'pass'])),
+])
+def test_subversion__get_url_rev_args(url, expected):
+    """
+    Test Subversion.get_url_rev_args().
+    """
+    actual = Subversion().get_url_rev_args(url)
+    assert actual == expected