Merge pull request #4690 from cjerdonek/simplify-git-ref-detection

Simplify code for handling Git branches and tags

src/pip/_internal/vcs/git.py

@@ -38,7 +38,7 @@ class Git(VersionControl):
     # Prevent the user's environment variables from interfering with pip:
     # https://github.com/pypa/pip/issues/1130
     unset_environ = ('GIT_DIR', 'GIT_WORK_TREE')
-    default_arg_rev = 'origin/HEAD'
+    default_arg_rev = 'HEAD'
 
     def __init__(self, url=None, *args, **kwargs):
 
@@ -89,34 +89,56 @@ def export(self, location):
                 show_stdout=False, cwd=temp_dir.path
             )
 
+    def get_revision_sha(self, dest, rev):
+        """
+        Return a commit hash for the given revision if it names a remote
+        branch or tag.  Otherwise, return None.
+
+        Args:
+          dest: the repository directory.
+          rev: the revision name.
+        """
+        # Pass rev to pre-filter the list.
+        output = self.run_command(['show-ref', rev], cwd=dest,
+                                  show_stdout=False, on_returncode='ignore')
+        refs = {}
+        for line in output.strip().splitlines():
+            try:
+                sha, ref = line.split()
+            except ValueError:
+                # Include the offending line to simplify troubleshooting if
+                # this error ever occurs.
+                raise ValueError('unexpected show-ref line: {!r}'.format(line))
+
+            refs[ref] = sha
+
+        branch_ref = 'refs/remotes/origin/{}'.format(rev)
+        tag_ref = 'refs/tags/{}'.format(rev)
+
+        return refs.get(branch_ref) or refs.get(tag_ref)
+
     def check_rev_options(self, dest, rev_options):
-        """Check the revision options before checkout to compensate that tags
-        and branches may need origin/ as a prefix.
+        """Check the revision options before checkout.
+
         Returns a new RevOptions object for the SHA1 of the branch or tag
         if found.
 
         Args:
           rev_options: a RevOptions object.
         """
-        revisions = self.get_short_refs(dest)
-
         rev = rev_options.arg_rev
-        origin_rev = 'origin/%s' % rev
-        if origin_rev in revisions:
-            # remote branch
-            return rev_options.make_new(revisions[origin_rev])
-        elif rev in revisions:
-            # a local tag or branch name
-            return rev_options.make_new(revisions[rev])
+        sha = self.get_revision_sha(dest, rev)
+
+        if sha is not None:
+            return rev_options.make_new(sha)
 
         # Do not show a warning for the common case of something that has
         # the form of a Git commit hash.
         if not looks_like_hash(rev):
             logger.warning(
-                "Did not find branch or tag '%s', assuming ref or revision.",
+                "Did not find branch or tag '%s', assuming revision or ref.",
                 rev,
             )
-
         return rev_options
 
     def is_commit_id_equal(self, dest, name):
@@ -198,50 +220,6 @@ def get_revision(self, location):
             ['rev-parse', 'HEAD'], show_stdout=False, cwd=location)
         return current_rev.strip()
 
-    def get_full_refs(self, location):
-        """Yields tuples of (commit, ref) for branches and tags"""
-        output = self.run_command(['show-ref'],
-                                  show_stdout=False, cwd=location)
-        for line in output.strip().splitlines():
-            commit, ref = line.split(' ', 1)
-            yield commit.strip(), ref.strip()
-
-    def is_ref_remote(self, ref):
-        return ref.startswith('refs/remotes/')
-
-    def is_ref_branch(self, ref):
-        return ref.startswith('refs/heads/')
-
-    def is_ref_tag(self, ref):
-        return ref.startswith('refs/tags/')
-
-    def is_ref_commit(self, ref):
-        """A ref is a commit sha if it is not anything else"""
-        return not any((
-            self.is_ref_remote(ref),
-            self.is_ref_branch(ref),
-            self.is_ref_tag(ref),
-        ))
-
-    # Should deprecate `get_refs` since it's ambiguous
-    def get_refs(self, location):
-        return self.get_short_refs(location)
-
-    def get_short_refs(self, location):
-        """Return map of named refs (branches or tags) to commit hashes."""
-        rv = {}
-        for commit, ref in self.get_full_refs(location):
-            ref_name = None
-            if self.is_ref_remote(ref):
-                ref_name = ref[len('refs/remotes/'):]
-            elif self.is_ref_branch(ref):
-                ref_name = ref[len('refs/heads/'):]
-            elif self.is_ref_tag(ref):
-                ref_name = ref[len('refs/tags/'):]
-            if ref_name is not None:
-                rv[ref_name] = commit
-        return rv
-
     def _get_subdirectory(self, location):
         """Return the relative path of setup.py to the git repo root."""
         # find the repo root

tests/functional/test_install_vcs.py

@@ -6,6 +6,57 @@
 from tests.lib.local_repos import local_checkout
 
 
+def _install_version_pkg(script, path, rev=None):
+    """
+    Install the version_pkg package, and return the version installed.
+
+    Args:
+      path: a tests.lib.path.Path object pointing to a Git repository
+        containing the package.
+      rev: an optional revision to install like a branch name or tag.
+    """
+    path = path.abspath.replace('\\', '/')
+    revision = '' if rev is None else '@{}'.format(rev)
+    url = 'git+file://{}{}#egg=version_pkg'.format(path, revision)
+    script.pip('install', '-e', url)
+    result = script.run('version_pkg')
+    version = result.stdout.strip()
+
+    return version
+
+
+def test_git_install_again_after_changes(script):
+    """
+    Test installing a repository a second time without specifying a revision,
+    and after updates to the remote repository.
+
+    This test also checks that no warning message like the following gets
+    logged on the update: "Did not find branch or tag ..., assuming ref or
+    revision."
+    """
+    version_pkg_path = _create_test_package(script)
+    version = _install_version_pkg(script, version_pkg_path)
+    assert version == '0.1'
+
+    _change_test_package_version(script, version_pkg_path)
+    version = _install_version_pkg(script, version_pkg_path)
+    assert version == 'some different version'
+
+
+def test_git_install_branch_again_after_branch_changes(script):
+    """
+    Test installing a branch again after the branch is updated in the remote
+    repository.
+    """
+    version_pkg_path = _create_test_package(script)
+    version = _install_version_pkg(script, version_pkg_path, rev='master')
+    assert version == '0.1'
+
+    _change_test_package_version(script, version_pkg_path)
+    version = _install_version_pkg(script, version_pkg_path, rev='master')
+    assert version == 'some different version'
+
+
 @pytest.mark.network
 def test_install_editable_from_git_with_https(script, tmpdir):
     """

tests/functional/test_install_vcs_git.py

@@ -9,55 +9,6 @@
 )
 
 
-@pytest.mark.network
-def test_get_short_refs_should_return_tag_name_and_commit_pair(script):
-    version_pkg_path = _create_test_package(script)
-    script.run('git', 'tag', '0.1', cwd=version_pkg_path)
-    script.run('git', 'tag', '0.2', cwd=version_pkg_path)
-    commit = script.run(
-        'git', 'rev-parse', 'HEAD',
-        cwd=version_pkg_path
-    ).stdout.strip()
-    git = Git()
-    result = git.get_short_refs(version_pkg_path)
-    assert result['0.1'] == commit, result
-    assert result['0.2'] == commit, result
-
-
-@pytest.mark.network
-def test_get_short_refs_should_return_branch_name_and_commit_pair(script):
-    version_pkg_path = _create_test_package(script)
-    script.run('git', 'branch', 'branch0.1', cwd=version_pkg_path)
-    commit = script.run(
-        'git', 'rev-parse', 'HEAD',
-        cwd=version_pkg_path
-    ).stdout.strip()
-    git = Git()
-    result = git.get_short_refs(version_pkg_path)
-    assert result['master'] == commit, result
-    assert result['branch0.1'] == commit, result
-
-
-@pytest.mark.network
-def test_get_short_refs_should_ignore_no_branch(script):
-    version_pkg_path = _create_test_package(script)
-    script.run('git', 'branch', 'branch0.1', cwd=version_pkg_path)
-    commit = script.run(
-        'git', 'rev-parse', 'HEAD',
-        cwd=version_pkg_path
-    ).stdout.strip()
-    # current branch here is "* (nobranch)"
-    script.run(
-        'git', 'checkout', commit,
-        cwd=version_pkg_path,
-        expect_stderr=True,
-    )
-    git = Git()
-    result = git.get_short_refs(version_pkg_path)
-    assert result['master'] == commit, result
-    assert result['branch0.1'] == commit, result
-
-
 @pytest.mark.network
 def test_is_commit_id_equal(script):
     """
@@ -78,39 +29,29 @@ def test_is_commit_id_equal(script):
     assert not git.is_commit_id_equal(version_pkg_path, None)
 
 
-@patch('pip._internal.vcs.git.Git.get_short_refs')
-def test_check_rev_options_should_handle_branch_name(get_refs_mock):
-    get_refs_mock.return_value = {'master': '123456', '0.1': 'abc123'}
+@patch('pip._internal.vcs.git.Git.get_revision_sha')
+def test_check_rev_options_ref_exists(get_sha_mock):
+    get_sha_mock.return_value = '123456'
     git = Git()
-    rev_options = git.make_rev_options('master')
+    rev_options = git.make_rev_options('develop')
 
     new_options = git.check_rev_options('.', rev_options)
     assert new_options.rev == '123456'
 
 
-@patch('pip._internal.vcs.git.Git.get_short_refs')
-def test_check_rev_options_should_handle_tag_name(get_refs_mock):
-    get_refs_mock.return_value = {'master': '123456', '0.1': 'abc123'}
+@patch('pip._internal.vcs.git.Git.get_revision_sha')
+def test_check_rev_options_ref_not_found(get_sha_mock):
+    get_sha_mock.return_value = None
     git = Git()
-    rev_options = git.make_rev_options('0.1')
+    rev_options = git.make_rev_options('develop')
 
     new_options = git.check_rev_options('.', rev_options)
-    assert new_options.rev == 'abc123'
-
-
-@patch('pip._internal.vcs.git.Git.get_short_refs')
-def test_check_rev_options_should_handle_ambiguous_commit(get_refs_mock):
-    get_refs_mock.return_value = {'master': '123456', '0.1': '123456'}
-    git = Git()
-    rev_options = git.make_rev_options('0.1')
-
-    new_options = git.check_rev_options('.', rev_options)
-    assert new_options.rev == '123456'
+    assert new_options.rev == 'develop'
 
 
-@patch('pip._internal.vcs.git.Git.get_short_refs')
-def test_check_rev_options_not_found_warning(get_refs_mock, caplog):
-    get_refs_mock.return_value = {}
+@patch('pip._internal.vcs.git.Git.get_revision_sha')
+def test_check_rev_options_not_found_warning(get_sha_mock, caplog):
+    get_sha_mock.return_value = None
     git = Git()
 
     sha = 40 * 'a'
@@ -126,7 +67,7 @@ def test_check_rev_options_not_found_warning(get_refs_mock, caplog):
     messages = [r.getMessage() for r in caplog.records]
     messages = [msg for msg in messages if msg.startswith('Did not find ')]
     assert messages == [
-        "Did not find branch or tag 'aaaaaa', assuming ref or revision."
+        "Did not find branch or tag 'aaaaaa', assuming revision or ref."
     ]
 
 

tests/functional/test_vcs_git.py

@@ -0,0 +1,92 @@
+"""
+Contains functional tests of the Git class.
+"""
+
+from pip._internal.utils.temp_dir import TempDirectory
+from pip._internal.vcs.git import Git
+
+
+def get_head_sha(script, dest):
+    """Return the HEAD sha."""
+    result = script.run('git', 'rev-parse', 'HEAD', cwd=dest)
+    sha = result.stdout.strip()
+
+    return sha
+
+
+def do_commit(script, dest):
+    script.run(
+        'git', 'commit', '-q', '--author', 'pip <pypa-dev@googlegroups.com>',
+        '--allow-empty', '-m', 'test commit', cwd=dest
+    )
+    return get_head_sha(script, dest)
+
+
+def add_commits(script, dest, count):
+    """Return a list of the commit hashes from oldest to newest."""
+    shas = []
+    for index in range(count):
+        sha = do_commit(script, dest)
+        shas.append(sha)
+
+    return shas
+
+
+def check_rev(repo_dir, rev, expected_sha):
+    git = Git()
+    assert git.get_revision_sha(repo_dir, rev) == expected_sha
+
+
+def test_get_revision_sha(script):
+    with TempDirectory(kind="testing") as temp:
+        repo_dir = temp.path
+        script.run('git', 'init', cwd=repo_dir)
+        shas = add_commits(script, repo_dir, count=3)
+
+        tag_sha = shas[0]
+        origin_sha = shas[1]
+        head_sha = shas[2]
+        assert head_sha == shas[-1]
+
+        origin_ref = 'refs/remotes/origin/origin-branch'
+        generic_ref = 'refs/generic-ref'
+
+        script.run(
+            'git', 'branch', 'local-branch', head_sha, cwd=repo_dir
+        )
+        script.run('git', 'tag', 'v1.0', tag_sha, cwd=repo_dir)
+        script.run('git', 'update-ref', origin_ref, origin_sha, cwd=repo_dir)
+        script.run(
+            'git', 'update-ref', 'refs/remotes/upstream/upstream-branch',
+            head_sha, cwd=repo_dir
+        )
+        script.run('git', 'update-ref', generic_ref, head_sha, cwd=repo_dir)
+
+        # Test two tags pointing to the same sha.
+        script.run('git', 'tag', 'v2.0', tag_sha, cwd=repo_dir)
+        # Test tags sharing the same suffix as another tag, both before and
+        # after the suffix alphabetically.
+        script.run('git', 'tag', 'aaa/v1.0', head_sha, cwd=repo_dir)
+        script.run('git', 'tag', 'zzz/v1.0', head_sha, cwd=repo_dir)
+
+        check_rev(repo_dir, 'v1.0', tag_sha)
+        check_rev(repo_dir, 'v2.0', tag_sha)
+        check_rev(repo_dir, 'origin-branch', origin_sha)
+
+        ignored_names = [
+            # Local branches should be ignored.
+            'local-branch',
+            # Non-origin remote branches should be ignored.
+            'upstream-branch',
+            # Generic refs should be ignored.
+            'generic-ref',
+            # Fully spelled-out refs should be ignored.
+            origin_ref,
+            generic_ref,
+            # Test passing a valid commit hash.
+            tag_sha,
+            # Test passing a non-existent name.
+            'does-not-exist',
+        ]
+        for name in ignored_names:
+            check_rev(repo_dir, name, None)