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

Pip freeze of editable git repos reports repo path #4760

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions news/4759.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
pip freeze now reports URL based on file system location for git
repositories with no remotes.
31 changes: 19 additions & 12 deletions src/pip/_internal/vcs/git.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from pip._vendor.six.moves.urllib import request as urllib_request

from pip._internal.compat import samefile
from pip._internal.exceptions import BadCommand
from pip._internal.exceptions import BadCommand, InstallationError
from pip._internal.utils.misc import display_path
from pip._internal.utils.temp_dir import TempDirectory
from pip._internal.vcs import VersionControl, vcs
Expand Down Expand Up @@ -203,17 +203,24 @@ def obtain(self, dest):

def get_url(self, location):
"""Return URL of the first remote encountered."""
Copy link
Member

Choose a reason for hiding this comment

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

This docstring should be updated to reflect the new behavior.

remotes = self.run_command(
['config', '--get-regexp', r'remote\..*\.url'],
show_stdout=False, cwd=location,
)
remotes = remotes.splitlines()
found_remote = remotes[0]
for remote in remotes:
if remote.startswith('remote.origin.url '):
found_remote = remote
break
url = found_remote.split(' ')[1]
try:
remotes = self.run_command(
['config', '--get-regexp', r'remote\..*\.url'],
show_stdout=False, cwd=location,
)
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.

In a previous review comment here, I made the suggestion that the "file" case not be handled simply by catching any command error. The reason is that this can result in masking various errors and bugs. It would be better if a more targeted approach can be used. Is there a different Git command that can be used that can list the remotes (even if there are 0) without erroring out with a non-zero exit status?

For example, why can't the output of git remote -v be used? That command still has exit status 0 (and so won't result in an InstallationError) even in the case of no remotes.

# If the above command fails we have no git remotes or cannot
# determine what they are. Just use the local repo path in that
# case instead.
url = 'file://' + location
Copy link
Member

Choose a reason for hiding this comment

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

See my comment here for another previous review comment, regarding escaping characters.

else:
remotes = remotes.splitlines()
found_remote = remotes[0]
for remote in remotes:
if remote.startswith('remote.origin.url '):
found_remote = remote
break
url = found_remote.split(' ')[1]
return url.strip()

def get_revision(self, location):
Expand Down
11 changes: 11 additions & 0 deletions tests/functional/test_freeze.py
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,17 @@ def test_freeze_git_remote(script, tmpdir):
"""
).format(remote=origin_remote).strip()
_check_output(result.stdout, expected)
# check when there are no remotes. Should report local file path.
script.run('git', 'remote', 'remove', 'origin', cwd=repo_dir)
script.run('git', 'remote', 'remove', 'other', cwd=repo_dir)
result = script.pip('freeze', expect_stderr=True)
expected = textwrap.dedent(
"""
...-e git+{remote}@...#egg=version_pkg
...
"""
).format(remote='file://' + os.path.normcase(repo_dir)).strip()
_check_output(result.stdout, expected)


@need_mercurial
Expand Down
15 changes: 0 additions & 15 deletions tests/functional/test_install_vcs.py
Original file line number Diff line number Diff line change
Expand Up @@ -291,21 +291,6 @@ def test_git_with_ambiguous_revs(script):
result.assert_installed('version-pkg', with_files=['.git'])


@pytest.mark.network
def test_git_works_with_editable_non_origin_repo(script):
# set up, create a git repo and install it as editable from a local
# directory path
version_pkg_path = _create_test_package(script)
script.pip('install', '-e', version_pkg_path.abspath)

# 'freeze'ing this should not fall over, but should result in stderr output
# warning
result = script.pip('freeze', expect_stderr=True)
assert "Error when trying to get requirement" in result.stderr
assert "Could not determine repository location" in result.stdout
assert "version-pkg==0.1" in result.stdout


@pytest.mark.network
def test_reinstalling_works_with_editible_non_master_branch(script):
"""
Expand Down