Merge pull request #4676 from cjerdonek/issue-4675-fix-check-version

Address issue #4675: fix Git.check_version()

Author: xavfernandez

docs/reference/pip_install.rst

@@ -374,6 +374,10 @@ Passing branch names, a commit hash or a tag name is possible like so::
     [-e] git://git.example.com/[email protected]#egg=MyProject
     [-e] git://git.example.com/MyProject.git@da39a3ee5e6b4b0d3255bfef95601890afd80709#egg=MyProject
 
+When passing a commit hash, specifying a full hash is preferable to a partial
+hash because a full hash allows pip to operate more efficiently (e.g. by
+making fewer network calls).
+
 Mercurial
 ~~~~~~~~~
 

news/4675.bugfix

@@ -0,0 +1,3 @@
+Fix an issue where ``pip install -e`` on a Git url would fail to update if
+a branch or tag name is specified that happens to match the prefix of the
+current ``HEAD`` commit hash.

src/pip/_internal/vcs/__init__.py

@@ -279,13 +279,13 @@ def update(self, dest, rev_options):
         """
         raise NotImplementedError
 
-    def check_version(self, dest, rev_options):
+    def is_commit_id_equal(self, dest, name):
         """
-        Return True if the version is identical to what exists and
-        doesn't need to be updated.
+        Return whether the id of the current commit equals the given name.
 
         Args:
-          rev_options: a RevOptions object.
+          dest: the repository directory.
+          name: a string name.
         """
         raise NotImplementedError
 
@@ -313,7 +313,7 @@ def check_destination(self, dest, url, rev_options):
                         display_path(dest),
                         url,
                     )
-                    if not self.check_version(dest, rev_options):
+                    if not self.is_commit_id_equal(dest, rev_options.rev):
                         logger.info(
                             'Updating %s %s%s',
                             display_path(dest),
@@ -405,8 +405,7 @@ def get_url(self, location):
 
     def get_revision(self, location):
         """
-        Return the current revision of the files at location
-        Used in get_info
+        Return the current commit id of the files at the given location.
         """
         raise NotImplementedError
 

src/pip/_internal/vcs/bazaar.py

@@ -104,7 +104,7 @@ def get_src_requirement(self, dist, location):
         current_rev = self.get_revision(location)
         return '%s@%s#egg=%s' % (repo, current_rev, egg_project_name)
 
-    def check_version(self, dest, rev_options):
+    def is_commit_id_equal(self, dest, name):
         """Always assume the versions don't match"""
         return False
 

src/pip/_internal/vcs/git.py

@@ -119,17 +119,19 @@ def check_rev_options(self, dest, rev_options):
 
         return rev_options
 
-    def check_version(self, dest, rev_options):
+    def is_commit_id_equal(self, dest, name):
         """
-        Compare the current sha to the ref. ref may be a branch or tag name,
-        but current rev will always point to a sha. This means that a branch
-        or tag will never compare as True. So this ultimately only matches
-        against exact shas.
+        Return whether the current commit hash equals the given name.
 
         Args:
-          rev_options: a RevOptions object.
+          dest: the repository directory.
+          name: a string name.
         """
-        return self.get_revision(dest).startswith(rev_options.arg_rev)
+        if not name:
+            # Then avoid an unnecessary subprocess call.
+            return False
+
+        return self.get_revision(dest) == name
 
     def switch(self, dest, url, rev_options):
         self.run_command(['config', 'remote.origin.url', url], cwd=dest)
@@ -164,10 +166,11 @@ def obtain(self, dest):
 
             if rev:
                 rev_options = self.check_rev_options(dest, rev_options)
-                # Only do a checkout if rev_options differs from HEAD
-                if not self.check_version(dest, rev_options):
+                # Only do a checkout if the current commit id doesn't match
+                # the requested revision.
+                if not self.is_commit_id_equal(dest, rev_options.rev):
                     cmd_args = ['fetch', '-q', url] + rev_options.to_args()
-                    self.run_command(cmd_args, cwd=dest,)
+                    self.run_command(cmd_args, cwd=dest)
                     self.run_command(
                         ['checkout', '-q', 'FETCH_HEAD'],
                         cwd=dest,

src/pip/_internal/vcs/mercurial.py

@@ -97,7 +97,7 @@ def get_src_requirement(self, dist, location):
         current_rev_hash = self.get_revision_hash(location)
         return '%s@%s#egg=%s' % (repo, current_rev_hash, egg_project_name)
 
-    def check_version(self, dest, rev_options):
+    def is_commit_id_equal(self, dest, name):
         """Always assume the versions don't match"""
         return False
 

src/pip/_internal/vcs/subversion.py

@@ -217,7 +217,7 @@ def get_src_requirement(self, dist, location):
         rev = self.get_revision(location)
         return 'svn+%s@%s#egg=%s' % (repo, rev, egg_project_name)
 
-    def check_version(self, dest, rev_options):
+    def is_commit_id_equal(self, dest, name):
         """Always assume the versions don't match"""
         return False
 

tests/functional/test_install_vcs_git.py

@@ -58,24 +58,24 @@ def test_get_short_refs_should_ignore_no_branch(script):
     assert result['branch0.1'] == commit, result
 
 
-def call_check_version(vcs, path, rev):
-    rev_options = vcs.make_rev_options(rev)
-    return vcs.check_version(path, rev_options)
-
-
 @pytest.mark.network
-def test_check_version(script):
+def test_is_commit_id_equal(script):
+    """
+    Test Git.is_commit_id_equal().
+    """
     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()
-    assert call_check_version(git, version_pkg_path, commit)
-    assert call_check_version(git, version_pkg_path, commit[:7])
-    assert not call_check_version(git, version_pkg_path, 'branch0.1')
-    assert not call_check_version(git, version_pkg_path, 'abc123')
+    assert git.is_commit_id_equal(version_pkg_path, commit)
+    assert not git.is_commit_id_equal(version_pkg_path, commit[:7])
+    assert not git.is_commit_id_equal(version_pkg_path, 'branch0.1')
+    assert not git.is_commit_id_equal(version_pkg_path, 'abc123')
+    # Also check passing a None value.
+    assert not git.is_commit_id_equal(version_pkg_path, None)
 
 
 @patch('pip._internal.vcs.git.Git.get_short_refs')

tests/unit/test_vcs.py

@@ -118,15 +118,19 @@ def test_git_get_src_requirements(git, dist):
     ])
 
 
-@pytest.mark.parametrize('ref,result', (
+@pytest.mark.parametrize('rev_name,result', (
     ('5547fa909e83df8bd743d3978d6667497983a4b7', True),
-    ('5547fa909', True),
+    ('5547fa909', False),
+    ('5678', False),
     ('abc123', False),
     ('foo', False),
+    (None, False),
 ))
-def test_git_check_version(git, ref, result):
-    rev_options = git.make_rev_options(ref)
-    assert git.check_version('foo', rev_options) is result
+def test_git_is_commit_id_equal(git, rev_name, result):
+    """
+    Test Git.is_commit_id_equal().
+    """
+    assert git.is_commit_id_equal('/path', rev_name) is result
 
 
 def test_translate_egg_surname():