Fix pip freeze to use modern format for git repos (pypa#9822)

Pip dropped support for `git+ssh@` style requirements (see pypa#7554)
in favour of `git+ssh://` but didn't propagate the change to
 `pip freeze` which resultantly returns invalid requirements.
Fix this behaviour.

Fixes pypa#9625.

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/vcs/git.py

@@ -1,5 +1,6 @@
 import logging
 import os.path
+import pathlib
 import re
 import urllib.parse
 import urllib.request
@@ -322,7 +323,36 @@ 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. SPC 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()
+        # SPC shorthand. e.g. git@example.com:foo/bar.git
+        # Should add an ssh:// prefix and replace the ':' with a '/'.
+        return "ssh://" + url.replace(":", "/")
 
     @classmethod
     def has_commit(cls, location, rev):
@@ -440,5 +470,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)

tests/functional/test_freeze.py

@@ -409,26 +409,36 @@ 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. Otherwise it is assumed to
+    # be an ssh:// remote. This is a side effect and not intentional behaviour.
+    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 = textwrap.dedent(
+        """
+            ...-e git+ssh://{remote}@...#egg=version_pkg
+            ...
+        """
+    ).format(remote=other_remote.replace(":", "/")).strip()
     _check_output(result.stdout, expected)
     # when there are more than one origin, priority is given to the
     # remote named origin
@@ -439,7 +449,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
 
@@ -108,37 +109,80 @@ 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
+
+
+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')