Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect all registered VCS and choose inner-most #7593

Merged
merged 11 commits into from
Feb 8, 2020
1 change: 1 addition & 0 deletions news/3988.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Correctly freeze a VCS editable package when it is nested inside another VCS repository.
27 changes: 16 additions & 11 deletions src/pip/_internal/vcs/git.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
from pip._vendor.six.moves.urllib import parse as urllib_parse
from pip._vendor.six.moves.urllib import request as urllib_request

from pip._internal.exceptions import BadCommand
from pip._internal.exceptions import BadCommand, InstallationError
from pip._internal.utils.misc import display_path, hide_url
from pip._internal.utils.subprocess import make_command
from pip._internal.utils.temp_dir import TempDirectory
Expand Down Expand Up @@ -370,20 +370,25 @@ def update_submodules(cls, location):
)

@classmethod
def controls_location(cls, location):
if super(Git, cls).controls_location(location):
return True
def get_repository_root(cls, location):
loc = super(Git, cls).get_repository_root(location)
if loc:
return loc
try:
r = cls.run_command(['rev-parse'],
cwd=location,
show_stdout=False,
on_returncode='ignore',
log_failed_cmd=False)
return not r
r = cls.run_command(
['rev-parse', '--show-toplevel'],
cwd=location,
show_stdout=False,
on_returncode='raise',
log_failed_cmd=False,
)
except BadCommand:
logger.debug("could not determine if %s is under git control "
"because git is not available", location)
return False
return None
except InstallationError:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems weird, I'd say call_subprocess should not raise InstallationError which is too high-level.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is definitely not right, but I’d say this belongs to another (refactoring) PR. Note that the Mercurial implementation already catches InstallationError right now.

return None
return os.path.normpath(r.rstrip('\r\n'))


vcs.register(Git)
24 changes: 15 additions & 9 deletions src/pip/_internal/vcs/mercurial.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,19 +137,25 @@ def get_subdirectory(cls, location):
return find_path_to_setup_from_repo_root(location, repo_root)

@classmethod
def controls_location(cls, location):
if super(Mercurial, cls).controls_location(location):
return True
def get_repository_root(cls, location):
loc = super(Mercurial, cls).get_repository_root(location)
if loc:
return loc
try:
cls.run_command(
['identify'],
r = cls.run_command(
['root'],
cwd=location,
show_stdout=False,
on_returncode='raise',
log_failed_cmd=False)
return True
except (BadCommand, InstallationError):
return False
log_failed_cmd=False,
)
except BadCommand:
logger.debug("could not determine if %s is under hg control "
"because hg is not available", location)
return None
except InstallationError:
return None
return os.path.normpath(r.rstrip('\r\n'))


vcs.register(Mercurial)
38 changes: 27 additions & 11 deletions src/pip/_internal/vcs/versioncontrol.py
Original file line number Diff line number Diff line change
Expand Up @@ -232,12 +232,24 @@ def get_backend_for_dir(self, location):
Return a VersionControl object if a repository of that type is found
at the given directory.
"""
vcs_backends = {}
for vcs_backend in self._registry.values():
if vcs_backend.controls_location(location):
logger.debug('Determine that %s uses VCS: %s',
location, vcs_backend.name)
return vcs_backend
return None
repo_path = vcs_backend.get_repository_root(location)
if not repo_path:
continue
logger.debug('Determine that %s uses VCS: %s',
location, vcs_backend.name)
vcs_backends[repo_path] = vcs_backend

if not vcs_backends:
return None

# Choose the VCS in the inner-most directory. Since all repository
# roots found here would be either ``location``` or one of its
uranusjr marked this conversation as resolved.
Show resolved Hide resolved
# parents, the longest path should have the most path components,
# i.e. the backend representing the inner-most repository.
inner_most_repo_path = max(vcs_backends, key=len)
return vcs_backends[inner_most_repo_path]

def get_backend_for_scheme(self, scheme):
# type: (str) -> Optional[VersionControl]
Expand Down Expand Up @@ -687,14 +699,18 @@ def is_repository_directory(cls, path):
return os.path.exists(os.path.join(path, cls.dirname))

@classmethod
def controls_location(cls, location):
# type: (str) -> bool
def get_repository_root(cls, location):
# type: (str) -> Optional[str]
"""
Check if a location is controlled by the vcs.
Return the "root" (top-level) directory controlled by the vcs,
or ``None`` if the directory is not in any.
uranusjr marked this conversation as resolved.
Show resolved Hide resolved

It is meant to be overridden to implement smarter detection
mechanisms for specific vcs.

This can do more than is_repository_directory() alone. For example,
the Git override checks that Git is actually available.
This can do more than is_repository_directory() alone. For
example, the Git override checks that Git is actually available.
"""
return cls.is_repository_directory(location)
if cls.is_repository_directory(location):
return location
return None
34 changes: 34 additions & 0 deletions tests/functional/test_freeze.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
_create_test_package,
_create_test_package_with_srcdir,
_git_commit,
_vcs_add,
need_bzr,
need_mercurial,
need_svn,
Expand Down Expand Up @@ -495,6 +496,39 @@ def test_freeze_bazaar_clone(script, tmpdir):
_check_output(result.stdout, expected)


@need_mercurial
@pytest.mark.git
@pytest.mark.parametrize(
"outer_vcs, inner_vcs",
[("hg", "git"), ("git", "hg")],
)
def test_freeze_nested_vcs(script, outer_vcs, inner_vcs):
"""Test VCS can be correctly freezed when resides inside another VCS repo.
"""
# Create Python package.
pkg_path = _create_test_package(script, vcs=inner_vcs)

# Create outer repo to clone into.
root_path = script.scratch_path.joinpath("test_freeze_nested_vcs")
root_path.mkdir()
root_path.joinpath(".hgignore").write_text("src")
root_path.joinpath(".gitignore").write_text("src")
_vcs_add(script, root_path, outer_vcs)

# Clone Python package into inner directory and install it.
src_path = root_path.joinpath("src")
src_path.mkdir()
script.run(inner_vcs, "clone", pkg_path, src_path, expect_stderr=True)
script.pip("install", "-e", src_path, expect_stderr=True)

# Check the freeze output recognizes the inner VCS.
result = script.pip("freeze", expect_stderr=True)
_check_output(
result.stdout,
"...-e {}+...#egg=version_pkg\n...".format(inner_vcs),
)


# used by the test_freeze_with_requirement_* tests below
_freeze_req_opts = textwrap.dedent("""\
# Unchanged requirements below this line
Expand Down
12 changes: 12 additions & 0 deletions tests/functional/test_vcs_git.py
Original file line number Diff line number Diff line change
Expand Up @@ -238,3 +238,15 @@ def test_is_immutable_rev_checkout(script):
assert not Git().is_immutable_rev_checkout(
"git+https://g.c/o/r@master", version_pkg_path
)


def test_get_repository_root(script):
version_pkg_path = _create_test_package(script)
tests_path = version_pkg_path.joinpath("tests")
tests_path.mkdir()

root1 = Git.get_repository_root(version_pkg_path)
assert os.path.normcase(root1) == os.path.normcase(version_pkg_path)

root2 = Git.get_repository_root(version_pkg_path.joinpath("tests"))
assert os.path.normcase(root2) == os.path.normcase(version_pkg_path)
17 changes: 17 additions & 0 deletions tests/functional/test_vcs_mercurial.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import os

from pip._internal.vcs.mercurial import Mercurial
from tests.lib import _create_test_package, need_mercurial


@need_mercurial
def test_get_repository_root(script):
version_pkg_path = _create_test_package(script, vcs="hg")
tests_path = version_pkg_path.joinpath("tests")
tests_path.mkdir()

root1 = Mercurial.get_repository_root(version_pkg_path)
assert os.path.normcase(root1) == os.path.normcase(version_pkg_path)

root2 = Mercurial.get_repository_root(version_pkg_path.joinpath("tests"))
assert os.path.normcase(root2) == os.path.normcase(version_pkg_path)