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

Fix get_branch() to work if the current branch shares a name with a tag #5912

Merged
merged 4 commits into from
Dec 21, 2018
Merged
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
7 changes: 6 additions & 1 deletion src/pip/_internal/utils/misc.py
Original file line number Diff line number Diff line change
Expand Up @@ -653,6 +653,7 @@ def call_subprocess(
show_stdout=True, # type: bool
cwd=None, # type: Optional[str]
on_returncode='raise', # type: str
extra_ok_returncodes=None, # type: Optional[Iterable[int]]
command_desc=None, # type: Optional[str]
extra_environ=None, # type: Optional[Mapping[str, Any]]
unset_environ=None, # type: Optional[Iterable[str]]
Expand All @@ -661,9 +662,13 @@ def call_subprocess(
# type: (...) -> Optional[Text]
"""
Args:
extra_ok_returncodes: an iterable of integer return codes that are
acceptable, in addition to 0. Defaults to None, which means [].
unset_environ: an iterable of environment variable names to unset
prior to calling subprocess.Popen().
"""
if extra_ok_returncodes is None:
extra_ok_returncodes = []
if unset_environ is None:
unset_environ = []
# This function's handling of subprocess output is confusing and I
Expand Down Expand Up @@ -740,7 +745,7 @@ def call_subprocess(
spinner.finish("error")
else:
spinner.finish("done")
if proc.returncode:
if proc.returncode and proc.returncode not in extra_ok_returncodes:
if on_returncode == 'raise':
if (logger.getEffectiveLevel() > std_logging.DEBUG and
not show_stdout):
Expand Down
9 changes: 6 additions & 3 deletions src/pip/_internal/vcs/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

if MYPY_CHECK_RUNNING:
from typing import ( # noqa: F401
Dict, Optional, Tuple, List, Type, Any, Mapping, Text
Any, Dict, Iterable, List, Mapping, Optional, Text, Tuple, Type
Copy link
Member

Choose a reason for hiding this comment

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

This has to be automated. I'll have to check how to do this.

)
from pip._internal.utils.ui import SpinnerInterface # noqa: F401

Expand Down Expand Up @@ -467,6 +467,7 @@ def run_command(
show_stdout=True, # type: bool
cwd=None, # type: Optional[str]
on_returncode='raise', # type: str
extra_ok_returncodes=None, # type: Optional[Iterable[int]]
command_desc=None, # type: Optional[str]
extra_environ=None, # type: Optional[Mapping[str, Any]]
spinner=None # type: Optional[SpinnerInterface]
Expand All @@ -480,8 +481,10 @@ def run_command(
cmd = [self.name] + cmd
try:
return call_subprocess(cmd, show_stdout, cwd,
on_returncode,
command_desc, extra_environ,
on_returncode=on_returncode,
extra_ok_returncodes=extra_ok_returncodes,
command_desc=command_desc,
extra_environ=extra_environ,
unset_environ=self.unset_environ,
spinner=spinner)
except OSError as e:
Expand Down
22 changes: 14 additions & 8 deletions src/pip/_internal/vcs/git.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,19 +79,25 @@ def get_git_version(self):
version = '.'.join(version.split('.')[:3])
return parse_version(version)

def get_branch(self, location):
def get_current_branch(self, location):
"""
Return the current branch, or None if HEAD isn't at a branch
(e.g. detached HEAD).
"""
args = ['rev-parse', '--abbrev-ref', 'HEAD']
output = self.run_command(args, show_stdout=False, cwd=location)
branch = output.strip()
# git-symbolic-ref exits with empty stdout if "HEAD" is a detached
# HEAD rather than a symbolic ref. In addition, the -q causes the
# command to exit with status code 1 instead of 128 in this case
# and to suppress the message to stderr.
args = ['symbolic-ref', '-q', 'HEAD']
output = self.run_command(
args, extra_ok_returncodes=(1, ), show_stdout=False, cwd=location,
)
ref = output.strip()

if branch == 'HEAD':
return None
if ref.startswith('refs/heads/'):
return ref[len('refs/heads/'):]

return branch
return None

def export(self, location):
"""Export the Git repository at the url to the destination location"""
Expand Down Expand Up @@ -210,7 +216,7 @@ def fetch_new(self, dest, url, rev_options):
if not self.is_commit_id_equal(dest, rev_options.rev):
cmd_args = ['checkout', '-q'] + rev_options.to_args()
self.run_command(cmd_args, cwd=dest)
elif self.get_branch(dest) != branch_name:
elif self.get_current_branch(dest) != branch_name:
# Then a specific branch was requested, and that branch
# is not yet checked out.
track_branch = 'origin/{}'.format(branch_name)
Expand Down
45 changes: 36 additions & 9 deletions tests/functional/test_vcs_git.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,16 @@ def get_head_sha(script, dest):
return sha


def checkout_ref(script, repo_dir, ref):
script.run('git', 'checkout', ref, cwd=repo_dir, expect_stderr=True)


def checkout_new_branch(script, repo_dir, branch):
script.run(
'git', 'checkout', '-b', branch, cwd=repo_dir, expect_stderr=True,
)


def do_commit(script, dest):
_git_commit(script, dest, message='test commit', args=['--allow-empty'])
return get_head_sha(script, dest)
Expand Down Expand Up @@ -83,26 +93,43 @@ def test_get_remote_url(script, tmpdir):
assert remote_url == source_url


def test_get_branch(script):
def test_get_current_branch(script):
repo_dir = str(script.scratch_path)

script.run('git', 'init', cwd=repo_dir)
sha = do_commit(script, repo_dir)

git = Git()
assert git.get_branch(repo_dir) == 'master'
assert git.get_current_branch(repo_dir) == 'master'

# Switch to a branch with the same SHA as "master" but whose name
# is alphabetically after.
script.run(
'git', 'checkout', '-b', 'release', cwd=repo_dir,
expect_stderr=True,
)
assert git.get_branch(repo_dir) == 'release'
checkout_new_branch(script, repo_dir, 'release')
assert git.get_current_branch(repo_dir) == 'release'

# Also test the detached HEAD case.
script.run('git', 'checkout', sha, cwd=repo_dir, expect_stderr=True)
assert git.get_branch(repo_dir) is None
checkout_ref(script, repo_dir, sha)
assert git.get_current_branch(repo_dir) is None


def test_get_current_branch__branch_and_tag_same_name(script, tmpdir):
"""
Check calling get_current_branch() from a branch or tag when the branch
and tag have the same name.
"""
repo_dir = str(tmpdir)
script.run('git', 'init', cwd=repo_dir)
do_commit(script, repo_dir)
checkout_new_branch(script, repo_dir, 'dev')
# Create a tag with the same name as the branch.
script.run('git', 'tag', 'dev', cwd=repo_dir)

git = Git()
assert git.get_current_branch(repo_dir) == 'dev'

# Now try with the tag checked out.
checkout_ref(script, repo_dir, 'refs/tags/dev')
assert git.get_current_branch(repo_dir) is None


def test_get_revision_sha(script):
Expand Down