Merge pull request #9822 from bwoodsend/fix-pip-freeze-git

news/9822.bugfix.rst

@@ -0,0 +1,3 @@
+Fix :ref:`pip freeze` to output packages :ref:`installed from git <vcs support>`
+in the correct ``git+protocol://git.example.com/MyProject#egg=MyProject`` format
+rather than the old and no longer supported ``git+git@`` format.

src/pip/_internal/operations/freeze.py

@@ -176,7 +176,7 @@ def get_requirement_info(dist):
 
     location = os.path.normcase(os.path.abspath(dist.location))
 
-    from pip._internal.vcs import RemoteNotFoundError, vcs
+    from pip._internal.vcs import RemoteNotFoundError, RemoteNotValidError, vcs
     vcs_backend = vcs.get_backend_for_dir(location)
 
     if vcs_backend is None:
@@ -200,6 +200,14 @@ def get_requirement_info(dist):
             )
         ]
         return (location, True, comments)
+    except RemoteNotValidError as ex:
+        req = dist.as_requirement()
+        comments = [
+            f"# Editable {type(vcs_backend).__name__} install ({req}) with "
+            f"either a deleted local remote or invalid URI:",
+            f"# '{ex.url}'",
+        ]
+        return (location, True, comments)
 
     except BadCommand:
         logger.warning(

src/pip/_internal/vcs/__init__.py

@@ -8,6 +8,7 @@
 import pip._internal.vcs.subversion  # noqa: F401
 from pip._internal.vcs.versioncontrol import (  # noqa: F401
     RemoteNotFoundError,
+    RemoteNotValidError,
     is_url,
     make_vcs_requirement_url,
     vcs,

src/pip/_internal/vcs/git.py

@@ -1,5 +1,6 @@
 import logging
 import os.path
+import pathlib
 import re
 import urllib.parse
 import urllib.request
@@ -14,6 +15,7 @@
 from pip._internal.vcs.versioncontrol import (
     AuthInfo,
     RemoteNotFoundError,
+    RemoteNotValidError,
     RevOptions,
     VersionControl,
     find_path_to_setup_from_repo_root,
@@ -29,6 +31,18 @@
 
 HASH_REGEX = re.compile('^[a-fA-F0-9]{40}$')
 
+# SCP (Secure copy protocol) shorthand. e.g. 'git@example.com:foo/bar.git'
+SCP_REGEX = re.compile(r"""^
+    # Optional user, e.g. 'git@'
+    (\w+@)?
+    # Server, e.g. 'github.com'.
+    ([^/:]+):
+    # The server-side path. e.g. 'user/project.git'. Must start with an
+    # alphanumeric character so as not to be confusable with a Windows paths
+    # like 'C:/foo/bar' or 'C:\foo\bar'.
+    (\w[^:]*)
+$""", re.VERBOSE)
+
 
 def looks_like_hash(sha):
     # type: (str) -> bool
@@ -328,7 +342,39 @@ def get_remote_url(cls, location):
                 found_remote = remote
                 break
         url = found_remote.split(' ')[1]
-        return url.strip()
+        return cls._git_remote_to_pip_url(url.strip())
+
+    @staticmethod
+    def _git_remote_to_pip_url(url):
+        # type: (str) -> str
+        """
+        Convert a remote url from what git uses to what pip accepts.
+
+        There are 3 legal forms **url** may take:
+
+            1. A fully qualified url: ssh://git@example.com/foo/bar.git
+            2. A local project.git folder: /path/to/bare/repository.git
+            3. SCP shorthand for form 1: git@example.com:foo/bar.git
+
+        Form 1 is output as-is. Form 2 must be converted to URI and form 3 must
+        be converted to form 1.
+
+        See the corresponding test test_git_remote_url_to_pip() for examples of
+        sample inputs/outputs.
+        """
+        if re.match(r"\w+://", url):
+            # This is already valid. Pass it though as-is.
+            return url
+        if os.path.exists(url):
+            # A local bare remote (git clone --mirror).
+            # Needs a file:// prefix.
+            return pathlib.PurePath(url).as_uri()
+        scp_match = SCP_REGEX.match(url)
+        if scp_match:
+            # Add an ssh:// prefix and replace the ':' with a '/'.
+            return scp_match.expand(r"ssh://\1\2/\3")
+        # Otherwise, bail out.
+        raise RemoteNotValidError(url)
 
     @classmethod
     def has_commit(cls, location, rev):
@@ -446,5 +492,12 @@ def get_repository_root(cls, location):
             return None
         return os.path.normpath(r.rstrip('\r\n'))
 
+    @staticmethod
+    def should_add_vcs_url_prefix(repo_url):
+        # type: (str) -> bool
+        """In either https or ssh form, requirements must be prefixed with git+.
+        """
+        return True
+
 
 vcs.register(Git)

src/pip/_internal/vcs/versioncontrol.py

@@ -100,6 +100,12 @@ class RemoteNotFoundError(Exception):
     pass
 
 
+class RemoteNotValidError(Exception):
+    def __init__(self, url: str):
+        super().__init__(url)
+        self.url = url
+
+
 class RevOptions:
 
     """

tests/functional/test_freeze.py

@@ -409,27 +409,38 @@ def test_freeze_git_remote(script, tmpdir):
         expect_stderr=True,
     )
     origin_remote = pkg_version
-    other_remote = pkg_version + '-other'
     # check frozen remote after clone
     result = script.pip('freeze', expect_stderr=True)
     expected = textwrap.dedent(
         """
             ...-e git+{remote}@...#egg=version_pkg
             ...
         """
-    ).format(remote=origin_remote).strip()
+    ).format(remote=path_to_url(origin_remote)).strip()
     _check_output(result.stdout, expected)
     # check frozen remote when there is no remote named origin
-    script.run('git', 'remote', 'remove', 'origin', cwd=repo_dir)
-    script.run('git', 'remote', 'add', 'other', other_remote, cwd=repo_dir)
+    script.run('git', 'remote', 'rename', 'origin', 'other', cwd=repo_dir)
     result = script.pip('freeze', expect_stderr=True)
     expected = textwrap.dedent(
         """
             ...-e git+{remote}@...#egg=version_pkg
             ...
         """
-    ).format(remote=other_remote).strip()
+    ).format(remote=path_to_url(origin_remote)).strip()
     _check_output(result.stdout, expected)
+    # When the remote is a local path, it must exist.
+    # If it doesn't, it gets flagged as invalid.
+    other_remote = pkg_version + '-other'
+    script.run('git', 'remote', 'set-url', 'other', other_remote, cwd=repo_dir)
+    result = script.pip('freeze', expect_stderr=True)
+    expected = os.path.normcase(textwrap.dedent(
+        f"""
+            ...# Editable Git...(version-pkg...)...
+            # '{other_remote}'
+            -e {repo_dir}...
+        """
+    ).strip())
+    _check_output(os.path.normcase(result.stdout), expected)
     # when there are more than one origin, priority is given to the
     # remote named origin
     script.run('git', 'remote', 'add', 'origin', origin_remote, cwd=repo_dir)
@@ -439,7 +450,7 @@ def test_freeze_git_remote(script, tmpdir):
             ...-e git+{remote}@...#egg=version_pkg
             ...
         """
-    ).format(remote=origin_remote).strip()
+    ).format(remote=path_to_url(origin_remote)).strip()
     _check_output(result.stdout, expected)
 
 

tests/unit/test_vcs.py

@@ -1,4 +1,5 @@
 import os
+import pathlib
 from unittest import TestCase
 from unittest.mock import patch
 
@@ -9,7 +10,7 @@
 from pip._internal.utils.misc import hide_url, hide_value
 from pip._internal.vcs import 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.git import Git, RemoteNotValidError, looks_like_hash
 from pip._internal.vcs.mercurial import Mercurial
 from pip._internal.vcs.subversion import Subversion
 from pip._internal.vcs.versioncontrol import RevOptions, VersionControl
@@ -108,37 +109,98 @@ def test_looks_like_hash(sha, expected):
 
 
 @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),
+    # Mercurial is one of the subclasses using the base class implementation.
+    # `hg://` isn't a real prefix but it tests the default behaviour.
+    (Mercurial, 'hg://user@example.com/MyProject', False),
+    (Mercurial, 'http://example.com/MyProject', True),
+    # The Git subclasses should return true in all cases.
+    (Git, 'git://example.com/MyProject', True),
     (Git, 'http://example.com/MyProject', True),
-    # Subversion is the only subclass overriding the base class implementation.
+    # Subversion also overrides 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
 
 
+@pytest.mark.parametrize("url, target", [
+    # A fully qualified remote url. No changes needed.
+    ("ssh://bob@server/foo/bar.git", "ssh://bob@server/foo/bar.git"),
+    ("git://bob@server/foo/bar.git", "git://bob@server/foo/bar.git"),
+    # User is optional and does not need a default.
+    ("ssh://server/foo/bar.git", "ssh://server/foo/bar.git"),
+    # The common scp shorthand for ssh remotes. Pip won't recognise these as
+    # git remotes until they have a 'ssh://' prefix and the ':' in the middle
+    # is gone.
+    ("git@example.com:foo/bar.git", "ssh://git@example.com/foo/bar.git"),
+    ("example.com:foo.git", "ssh://example.com/foo.git"),
+    # Http(s) remote names are already complete and should remain unchanged.
+    ("https://example.com/foo", "https://example.com/foo"),
+    ("http://example.com/foo/bar.git", "http://example.com/foo/bar.git"),
+    ("https://bob@example.com/foo", "https://bob@example.com/foo"),
+ ])
+def test_git_remote_url_to_pip(url, target):
+    assert Git._git_remote_to_pip_url(url) == target
+
+
+@pytest.mark.parametrize("url, platform", [
+    # Windows paths with the ':' drive prefix look dangerously close to SCP.
+    ("c:/piffle/wiffle/waffle/poffle.git", "nt"),
+    (r"c:\faffle\waffle\woffle\piffle.git", "nt"),
+    # Unix paths less so but test them anyway.
+    ("/muffle/fuffle/pufffle/fluffle.git", "posix"),
+])
+def test_paths_are_not_mistaken_for_scp_shorthand(url, platform):
+    # File paths should not be mistaken for SCP shorthand. If they do then
+    # 'c:/piffle/wiffle' would end up as 'ssh://c/piffle/wiffle'.
+    from pip._internal.vcs.git import SCP_REGEX
+    assert not SCP_REGEX.match(url)
+
+    if platform == os.name:
+        with pytest.raises(RemoteNotValidError):
+            Git._git_remote_to_pip_url(url)
+
+
+def test_git_remote_local_path(tmpdir):
+    path = pathlib.Path(tmpdir, "project.git")
+    path.mkdir()
+    # Path must exist to be recognised as a local git remote.
+    assert Git._git_remote_to_pip_url(str(path)) == path.as_uri()
+
+
 @patch('pip._internal.vcs.git.Git.get_remote_url')
 @patch('pip._internal.vcs.git.Git.get_revision')
 @patch('pip._internal.vcs.git.Git.get_subdirectory')
+@pytest.mark.parametrize(
+    "git_url, target_url_prefix",
+    [
+        (
+            "https://github.com/pypa/pip-test-package",
+            "git+https://github.com/pypa/pip-test-package",
+        ),
+        (
+            "git@github.com:pypa/pip-test-package",
+            "git+ssh://git@github.com/pypa/pip-test-package",
+        ),
+    ],
+    ids=["https", "ssh"],
+)
 @pytest.mark.network
 def test_git_get_src_requirements(
-    mock_get_subdirectory, mock_get_revision, mock_get_remote_url
+    mock_get_subdirectory, mock_get_revision, mock_get_remote_url,
+        git_url, target_url_prefix,
 ):
-    git_url = 'https://github.com/pypa/pip-test-package'
     sha = '5547fa909e83df8bd743d3978d6667497983a4b7'
 
-    mock_get_remote_url.return_value = git_url
+    mock_get_remote_url.return_value = Git._git_remote_to_pip_url(git_url)
     mock_get_revision.return_value = sha
     mock_get_subdirectory.return_value = None
 
     ret = Git.get_src_requirement('.', 'pip-test-package')
 
-    assert ret == (
-        'git+https://github.com/pypa/pip-test-package'
-        '@5547fa909e83df8bd743d3978d6667497983a4b7#egg=pip_test_package'
-    )
+    target = f"{target_url_prefix}@{sha}#egg=pip_test_package"
+    assert ret == target
 
 
 @patch('pip._internal.vcs.git.Git.get_revision_sha')