diff --git a/news/9822.bugfix.rst b/news/9822.bugfix.rst new file mode 100644 index 00000000000..8a692c3ff29 --- /dev/null +++ b/news/9822.bugfix.rst @@ -0,0 +1,3 @@ +Fix :ref:`pip freeze` to output packages :ref:`installed from git ` +in the correct ``git+protocol://git.example.com/MyProject#egg=MyProject`` format +rather than the old and no longer supported ``git+git@`` format. diff --git a/src/pip/_internal/operations/freeze.py b/src/pip/_internal/operations/freeze.py index f34a9d4be7e..3cda5c8c90e 100644 --- a/src/pip/_internal/operations/freeze.py +++ b/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( diff --git a/src/pip/_internal/vcs/__init__.py b/src/pip/_internal/vcs/__init__.py index 30025d632a9..b6beddbe6d2 100644 --- a/src/pip/_internal/vcs/__init__.py +++ b/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, diff --git a/src/pip/_internal/vcs/git.py b/src/pip/_internal/vcs/git.py index 9f24ccdf5ee..3c46c250ec0 100644 --- a/src/pip/_internal/vcs/git.py +++ b/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 @@ -322,7 +336,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): @@ -440,5 +486,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) diff --git a/src/pip/_internal/vcs/versioncontrol.py b/src/pip/_internal/vcs/versioncontrol.py index 97977b5799c..d06c81032b0 100644 --- a/src/pip/_internal/vcs/versioncontrol.py +++ b/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: """ diff --git a/tests/functional/test_freeze.py b/tests/functional/test_freeze.py index 858e43931db..0af29dd0cb2 100644 --- a/tests/functional/test_freeze.py +++ b/tests/functional/test_freeze.py @@ -409,7 +409,6 @@ 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( @@ -417,19 +416,31 @@ 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) # 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) diff --git a/tests/unit/test_vcs.py b/tests/unit/test_vcs.py index f86d04d7425..305c45fd857 100644 --- a/tests/unit/test_vcs.py +++ b/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,10 +109,14 @@ 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): @@ -119,26 +124,83 @@ def test_should_add_vcs_url_prefix(vcs_cls, remote_url, expected): 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')