Merge pull request #6130 from cjerdonek/move-get-src-requirement-to-base-class

Move get_src_requirement() to the VersionControl base class

src/pip/_internal/utils/misc.py

@@ -906,22 +906,6 @@ def enum(*sequential, **named):
     return type('Enum', (), enums)
 
 
-def make_vcs_requirement_url(repo_url, rev, project_name, subdir=None):
-    """
-    Return the URL for a VCS requirement.
-
-    Args:
-      repo_url: the remote VCS url, with any needed VCS prefix (e.g. "git+").
-      project_name: the (unescaped) project name.
-    """
-    egg_project_name = pkg_resources.to_filename(project_name)
-    req = '{}@{}#egg={}'.format(repo_url, rev, egg_project_name)
-    if subdir:
-        req += '&subdirectory={}'.format(subdir)
-
-    return req
-
-
 def split_auth_from_netloc(netloc):
     """
     Parse out and remove the auth information from a netloc.

src/pip/_internal/vcs/__init__.py

@@ -7,11 +7,12 @@ import os
 import shutil
 import sys
 
+from pip._vendor import pkg_resources
 from pip._vendor.six.moves.urllib import parse as urllib_parse
 
 from pip._internal.exceptions import BadCommand
 from pip._internal.utils.misc import (
-    display_path, backup_dir, call_subprocess, rmtree, ask_path_exists,
+    ask_path_exists, backup_dir, call_subprocess, display_path, rmtree,
 )
 from pip._internal.utils.typing import MYPY_CHECK_RUNNING
 
@@ -29,6 +30,22 @@ __all__ = ['vcs']
 logger = logging.getLogger(__name__)
 
 
+def make_vcs_requirement_url(repo_url, rev, project_name, subdir=None):
+    """
+    Return the URL for a VCS requirement.
+
+    Args:
+      repo_url: the remote VCS url, with any needed VCS prefix (e.g. "git+").
+      project_name: the (unescaped) project name.
+    """
+    egg_project_name = pkg_resources.to_filename(project_name)
+    req = '{}@{}#egg={}'.format(repo_url, rev, egg_project_name)
+    if subdir:
+        req += '&subdirectory={}'.format(subdir)
+
+    return req
+
+
 class RemoteNotFoundError(Exception):
     pass
 
@@ -186,6 +203,55 @@ class VersionControl(object):
     unset_environ = ()  # type: Tuple[str, ...]
     default_arg_rev = None  # type: Optional[str]
 
+    @classmethod
+    def should_add_vcs_url_prefix(cls, remote_url):
+        """
+        Return whether the vcs prefix (e.g. "git+") should be added to a
+        repository's remote url when used in a requirement.
+        """
+        return not remote_url.lower().startswith('{}:'.format(cls.name))
+
+    @classmethod
+    def get_subdirectory(cls, repo_dir):
+        """
+        Return the path to setup.py, relative to the repo root.
+        """
+        return None
+
+    @classmethod
+    def get_requirement_revision(cls, repo_dir):
+        """
+        Return the revision string that should be used in a requirement.
+        """
+        return cls.get_revision(repo_dir)
+
+    @classmethod
+    def get_src_requirement(cls, repo_dir, project_name):
+        """
+        Return the requirement string to use to redownload the files
+        currently at the given repository directory.
+
+        Args:
+          project_name: the (unescaped) project name.
+
+        The return value has a form similar to the following:
+
+            {repository_url}@{revision}#egg={project_name}
+        """
+        repo_url = cls.get_remote_url(repo_dir)
+        if repo_url is None:
+            return None
+
+        if cls.should_add_vcs_url_prefix(repo_url):
+            repo_url = '{}+{}'.format(cls.name, repo_url)
+
+        revision = cls.get_requirement_revision(repo_dir)
+        subdir = cls.get_subdirectory(repo_dir)
+        req = make_vcs_requirement_url(repo_url, revision, project_name,
+                                       subdir=subdir)
+
+        return req
+
     def __init__(self, url=None, *args, **kwargs):
         self.url = url
         super(VersionControl, self).__init__(*args, **kwargs)
@@ -445,16 +511,6 @@ class VersionControl(object):
             rmtree(location)
         self.obtain(location)
 
-    @classmethod
-    def get_src_requirement(cls, location, project_name):
-        """
-        Return a string representing the requirement needed to
-        redownload the files currently present in location, something
-        like:
-          {repository_url}@{revision}#egg={project_name}-{version_identifier}
-        """
-        raise NotImplementedError
-
     @classmethod
     def get_remote_url(cls, location):
         """

src/pip/_internal/vcs/bazaar.py

@@ -6,9 +6,7 @@ import os
 from pip._vendor.six.moves.urllib import parse as urllib_parse
 
 from pip._internal.download import path_to_url
-from pip._internal.utils.misc import (
-    display_path, make_vcs_requirement_url, rmtree,
-)
+from pip._internal.utils.misc import display_path, rmtree
 from pip._internal.vcs import VersionControl, vcs
 
 logger = logging.getLogger(__name__)
@@ -93,16 +91,6 @@ class Bazaar(VersionControl):
         )
         return revision.splitlines()[-1]
 
-    @classmethod
-    def get_src_requirement(cls, location, project_name):
-        repo = cls.get_remote_url(location)
-        if not repo:
-            return None
-        if not repo.lower().startswith('bzr:'):
-            repo = 'bzr+' + repo
-        current_rev = cls.get_revision(location)
-        return make_vcs_requirement_url(repo, current_rev, project_name)
-
     def is_commit_id_equal(self, dest, name):
         """Always assume the versions don't match"""
         return False

src/pip/_internal/vcs/git.py

@@ -10,9 +10,7 @@ from pip._vendor.six.moves.urllib import request as urllib_request
 
 from pip._internal.exceptions import BadCommand
 from pip._internal.utils.compat import samefile
-from pip._internal.utils.misc import (
-    display_path, make_vcs_requirement_url, redact_password_from_url,
-)
+from pip._internal.utils.misc import display_path, redact_password_from_url
 from pip._internal.utils.temp_dir import TempDirectory
 from pip._internal.vcs import RemoteNotFoundError, VersionControl, vcs
 
@@ -286,8 +284,7 @@ class Git(VersionControl):
         return current_rev.strip()
 
     @classmethod
-    def _get_subdirectory(cls, location):
-        """Return the relative path of setup.py to the git repo root."""
+    def get_subdirectory(cls, location):
         # find the repo root
         git_dir = cls.run_command(['rev-parse', '--git-dir'],
                                   show_stdout=False, cwd=location).strip()
@@ -313,18 +310,6 @@ class Git(VersionControl):
             return None
         return os.path.relpath(location, root_dir)
 
-    @classmethod
-    def get_src_requirement(cls, location, project_name):
-        repo = cls.get_remote_url(location)
-        if not repo.lower().startswith('git:'):
-            repo = 'git+' + repo
-        current_rev = cls.get_revision(location)
-        subdir = cls._get_subdirectory(location)
-        req = make_vcs_requirement_url(repo, current_rev, project_name,
-                                       subdir=subdir)
-
-        return req
-
     def get_url_rev_and_auth(self, url):
         """
         Prefixes stub URLs like 'user@hostname:user/repo.git' with 'ssh://'.

src/pip/_internal/vcs/mercurial.py

@@ -6,7 +6,7 @@ import os
 from pip._vendor.six.moves import configparser
 
 from pip._internal.download import path_to_url
-from pip._internal.utils.misc import display_path, make_vcs_requirement_url
+from pip._internal.utils.misc import display_path
 from pip._internal.utils.temp_dir import TempDirectory
 from pip._internal.vcs import VersionControl, vcs
 
@@ -75,26 +75,25 @@ class Mercurial(VersionControl):
 
     @classmethod
     def get_revision(cls, location):
+        """
+        Return the repository-local changeset revision number, as an integer.
+        """
         current_revision = cls.run_command(
             ['parents', '--template={rev}'],
             show_stdout=False, cwd=location).strip()
         return current_revision
 
     @classmethod
-    def get_revision_hash(cls, location):
+    def get_requirement_revision(cls, location):
+        """
+        Return the changeset identification hash, as a 40-character
+        hexadecimal string
+        """
         current_rev_hash = cls.run_command(
             ['parents', '--template={node}'],
             show_stdout=False, cwd=location).strip()
         return current_rev_hash
 
-    @classmethod
-    def get_src_requirement(cls, location, project_name):
-        repo = cls.get_remote_url(location)
-        if not repo.lower().startswith('hg:'):
-            repo = 'hg+' + repo
-        current_rev_hash = cls.get_revision_hash(location)
-        return make_vcs_requirement_url(repo, current_rev_hash, project_name)
-
     def is_commit_id_equal(self, dest, name):
         """Always assume the versions don't match"""
         return False

src/pip/_internal/vcs/subversion.py

@@ -6,7 +6,7 @@ import re
 
 from pip._internal.utils.logging import indent_log
 from pip._internal.utils.misc import (
-    display_path, make_vcs_requirement_url, rmtree, split_auth_from_netloc,
+    display_path, rmtree, split_auth_from_netloc,
 )
 from pip._internal.vcs import VersionControl, vcs
 
@@ -25,6 +25,10 @@ class Subversion(VersionControl):
     repo_name = 'checkout'
     schemes = ('svn', 'svn+ssh', 'svn+http', 'svn+https', 'svn+svn')
 
+    @classmethod
+    def should_add_vcs_url_prefix(cls, remote_url):
+        return True
+
     def get_base_rev_args(self, rev):
         return ['-r', rev]
 
@@ -183,15 +187,6 @@ class Subversion(VersionControl):
 
         return url, rev
 
-    @classmethod
-    def get_src_requirement(cls, location, project_name):
-        repo = cls.get_remote_url(location)
-        if repo is None:
-            return None
-        repo = 'svn+' + repo
-        rev = cls.get_revision(location)
-        return make_vcs_requirement_url(repo, rev, project_name)
-
     def is_commit_id_equal(self, dest, name):
         """Always assume the versions don't match"""
         return False

tests/unit/test_utils.py

@@ -27,10 +27,9 @@ from pip._internal.utils.glibc import check_glibc_version
 from pip._internal.utils.hashes import Hashes, MissingHashes
 from pip._internal.utils.misc import (
     call_subprocess, egg_link_path, ensure_dir, format_command_args,
-    get_installed_distributions, get_prog, make_vcs_requirement_url,
-    normalize_path, redact_netloc, redact_password_from_url,
-    remove_auth_from_url, rmtree, split_auth_from_netloc, untar_file,
-    unzip_file,
+    get_installed_distributions, get_prog, normalize_path, redact_netloc,
+    redact_password_from_url, remove_auth_from_url, rmtree,
+    split_auth_from_netloc, untar_file, unzip_file,
 )
 from pip._internal.utils.packaging import check_dist_requires_python
 from pip._internal.utils.temp_dir import AdjacentTempDirectory, TempDirectory
@@ -983,25 +982,6 @@ class TestCallSubprocess(object):
             )
 
 
-@pytest.mark.parametrize('args, expected', [
-    # Test without subdir.
-    (('git+https://example.com/pkg', 'dev', 'myproj'),
-     'git+https://example.com/pkg@dev#egg=myproj'),
-    # Test with subdir.
-    (('git+https://example.com/pkg', 'dev', 'myproj', 'sub/dir'),
-     'git+https://example.com/pkg@dev#egg=myproj&subdirectory=sub/dir'),
-    # Test with None subdir.
-    (('git+https://example.com/pkg', 'dev', 'myproj', None),
-     'git+https://example.com/pkg@dev#egg=myproj'),
-    # Test an unescaped project name.
-    (('git+https://example.com/pkg', 'dev', 'zope-interface'),
-     'git+https://example.com/pkg@dev#egg=zope_interface'),
-])
-def test_make_vcs_requirement_url(args, expected):
-    actual = make_vcs_requirement_url(*args)
-    assert actual == expected
-
-
 @pytest.mark.parametrize('netloc, expected', [
     # Test a basic case.
     ('example.com', ('example.com', (None, None))),

tests/unit/test_vcs.py

@@ -2,7 +2,9 @@ import pytest
 from mock import patch
 from pip._vendor.packaging.version import parse as parse_version
 
-from pip._internal.vcs import RevOptions, VersionControl
+from pip._internal.vcs import (
+    RevOptions, VersionControl, make_vcs_requirement_url,
+)
 from pip._internal.vcs.bazaar import Bazaar
 from pip._internal.vcs.git import Git, looks_like_hash
 from pip._internal.vcs.mercurial import Mercurial
@@ -15,6 +17,25 @@ else:
     VERBOSE_FALSE = 0
 
 
+@pytest.mark.parametrize('args, expected', [
+    # Test without subdir.
+    (('git+https://example.com/pkg', 'dev', 'myproj'),
+     'git+https://example.com/pkg@dev#egg=myproj'),
+    # Test with subdir.
+    (('git+https://example.com/pkg', 'dev', 'myproj', 'sub/dir'),
+     'git+https://example.com/pkg@dev#egg=myproj&subdirectory=sub/dir'),
+    # Test with None subdir.
+    (('git+https://example.com/pkg', 'dev', 'myproj', None),
+     'git+https://example.com/pkg@dev#egg=myproj'),
+    # Test an unescaped project name.
+    (('git+https://example.com/pkg', 'dev', 'zope-interface'),
+     'git+https://example.com/pkg@dev#egg=zope_interface'),
+])
+def test_make_vcs_requirement_url(args, expected):
+    actual = make_vcs_requirement_url(*args)
+    assert actual == expected
+
+
 def test_rev_options_repr():
     rev_options = RevOptions(Git(), 'develop')
     assert repr(rev_options) == "<RevOptions git: rev='develop'>"
@@ -79,6 +100,18 @@ def test_looks_like_hash():
     assert not looks_like_hash(39 * 'a')
 
 
+@pytest.mark.parametrize('vcs_cls, remote_url, expected', [
+    # Git is one of the subclasses using the base class implementation.
+    (Git, 'git://example.com/MyProject', False),
+    (Git, 'http://example.com/MyProject', True),
+    # Subversion is the only subclass overriding the base class implementation.
+    (Subversion, 'svn://example.com/MyProject', True),
+])
+def test_should_add_vcs_url_prefix(vcs_cls, remote_url, expected):
+    actual = vcs_cls.should_add_vcs_url_prefix(remote_url)
+    assert actual == expected
+
+
 @patch('pip._internal.vcs.git.Git.get_revision')
 @patch('pip._internal.vcs.git.Git.get_remote_url')
 @pytest.mark.network