Merge pull request #3066 from mattrobenolt/update-2

Only update VCS when things have actually changed

7 files changed, 122 insertions, 29 deletions

diff --git a/pip/vcs/__init__.py b/pip/vcs/__init__.py
index 494351c98..bb425260b 100644
--- a/pip/vcs/__init__.py
+++ b/pip/vcs/__init__.py
@@ -186,6 +186,13 @@ class VersionControl(object):
         """
         raise NotImplementedError
 
+    def check_version(self, dest, rev_options):
+        """
+        Return True if the version is identical to what exists and
+        doesn't need to be updated.
+        """
+        raise NotImplementedError
+
     def check_destination(self, dest, url, rev_options, rev_display):
         """
         Prepare a location to receive a checkout/clone.
@@ -206,13 +213,17 @@ class VersionControl(object):
                         display_path(dest),
                         url,
                     )
-                    logger.info(
-                        'Updating %s %s%s',
-                        display_path(dest),
-                        self.repo_name,
-                        rev_display,
-                    )
-                    self.update(dest, rev_options)
+                    if not self.check_version(dest, rev_options):
+                        logger.info(
+                            'Updating %s %s%s',
+                            display_path(dest),
+                            self.repo_name,
+                            rev_display,
+                        )
+                        self.update(dest, rev_options)
+                    else:
+                        logger.info(
+                            'Skipping because already up-to-date.')
                 else:
                     logger.warning(
                         '%s %s in %s exists with URL %s',
diff --git a/pip/vcs/bazaar.py b/pip/vcs/bazaar.py
index 8d26bab45..59a42ad49 100644
--- a/pip/vcs/bazaar.py
+++ b/pip/vcs/bazaar.py
@@ -128,5 +128,9 @@ class Bazaar(VersionControl):
             full_egg_name = '%s-dev_r%s' % (dist.egg_name(), current_rev)
         return '%s@%s#egg=%s' % (repo, current_rev, full_egg_name)
 
+    def check_version(self, dest, rev_options):
+        """Always assume the versions don't match"""
+        return False
+
 
 vcs.register(Bazaar)
diff --git a/pip/vcs/git.py b/pip/vcs/git.py
index 7f4bccb04..5a2415a87 100644
--- a/pip/vcs/git.py
+++ b/pip/vcs/git.py
@@ -65,7 +65,7 @@ class Git(VersionControl):
         and branches may need origin/ as a prefix.
         Returns the SHA1 of the branch or tag if found.
         """
-        revisions = self.get_refs(dest)
+        revisions = self.get_short_refs(dest)
 
         origin_rev = 'origin/%s' % rev
         if origin_rev in revisions:
@@ -80,6 +80,15 @@ class Git(VersionControl):
             )
             return rev_options
 
+    def check_version(self, dest, rev_options):
+        """
+        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 self.get_revision(dest).startswith(rev_options[0])
+
     def switch(self, dest, url, rev_options):
         self.run_command(['config', 'remote.origin.url', url], cwd=dest)
         self.run_command(['checkout', '-q'] + rev_options, cwd=dest)
@@ -115,7 +124,7 @@ class Git(VersionControl):
             if rev:
                 rev_options = self.check_rev_options(rev, dest, rev_options)
                 # Only do a checkout if rev_options differs from HEAD
-                if not self.get_revision(dest).startswith(rev_options[0]):
+                if not self.check_version(dest, rev_options):
                     self.run_command(
                         ['checkout', '-q'] + rev_options,
                         cwd=dest,
@@ -134,23 +143,48 @@ class Git(VersionControl):
             ['rev-parse', 'HEAD'], show_stdout=False, cwd=location)
         return current_rev.strip()
 
-    def get_refs(self, location):
-        """Return map of named refs (branches or tags) to commit hashes."""
+    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)
-        rv = {}
         for line in output.strip().splitlines():
             commit, ref = line.split(' ', 1)
-            ref = ref.strip()
+            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 ref.startswith('refs/remotes/'):
+            if self.is_ref_remote(ref):
                 ref_name = ref[len('refs/remotes/'):]
-            elif ref.startswith('refs/heads/'):
+            elif self.is_ref_branch(ref):
                 ref_name = ref[len('refs/heads/'):]
-            elif ref.startswith('refs/tags/'):
+            elif self.is_ref_tag(ref):
                 ref_name = ref[len('refs/tags/'):]
             if ref_name is not None:
-                rv[ref_name] = commit.strip()
+                rv[ref_name] = commit
         return rv
 
     def get_src_requirement(self, dist, location, find_tags):
@@ -161,7 +195,7 @@ class Git(VersionControl):
         if not repo:
             return None
         current_rev = self.get_revision(location)
-        refs = self.get_refs(location)
+        refs = self.get_short_refs(location)
         # refs maps names to commit hashes; we need the inverse
         # if multiple names map to a single commit, we pick the first one
         # alphabetically
diff --git a/pip/vcs/mercurial.py b/pip/vcs/mercurial.py
index b3ca4eece..feae54246 100644
--- a/pip/vcs/mercurial.py
+++ b/pip/vcs/mercurial.py
@@ -136,4 +136,8 @@ class Mercurial(VersionControl):
             full_egg_name = '%s-dev' % egg_project_name
         return '%s@%s#egg=%s' % (repo, current_rev_hash, full_egg_name)
 
+    def check_version(self, dest, rev_options):
+        """Always assume the versions don't match"""
+        return False
+
 vcs.register(Mercurial)
diff --git a/pip/vcs/subversion.py b/pip/vcs/subversion.py
index e4f3439ca..834a5bfda 100644
--- a/pip/vcs/subversion.py
+++ b/pip/vcs/subversion.py
@@ -259,6 +259,10 @@ class Subversion(VersionControl):
             full_egg_name = '%s-dev_r%s' % (egg_project_name, rev)
         return 'svn+%s@%s#egg=%s' % (repo, rev, full_egg_name)
 
+    def check_version(self, dest, rev_options):
+        """Always assume the versions don't match"""
+        return False
+
 
 def get_rev_options(url, rev):
     if rev:
diff --git a/tests/functional/test_install_vcs_git.py b/tests/functional/test_install_vcs_git.py
index 0ff787f20..d06a4d05a 100644
--- a/tests/functional/test_install_vcs_git.py
+++ b/tests/functional/test_install_vcs_git.py
@@ -12,7 +12,7 @@ from tests.lib.git_submodule_helpers import (
 
 
 @pytest.mark.network
-def test_get_refs_should_return_tag_name_and_commit_pair(script):
+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)
@@ -21,13 +21,13 @@ def test_get_refs_should_return_tag_name_and_commit_pair(script):
         cwd=version_pkg_path
     ).stdout.strip()
     git = Git()
-    result = git.get_refs(version_pkg_path)
+    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_refs_should_return_branch_name_and_commit_pair(script):
+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(
@@ -35,13 +35,13 @@ def test_get_refs_should_return_branch_name_and_commit_pair(script):
         cwd=version_pkg_path
     ).stdout.strip()
     git = Git()
-    result = git.get_refs(version_pkg_path)
+    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_refs_should_ignore_no_branch(script):
+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(
@@ -55,12 +55,27 @@ def test_get_refs_should_ignore_no_branch(script):
         expect_stderr=True,
     )
     git = Git()
-    result = git.get_refs(version_pkg_path)
+    result = git.get_short_refs(version_pkg_path)
     assert result['master'] == commit, result
     assert result['branch0.1'] == commit, result
 
 
-@patch('pip.vcs.git.Git.get_refs')
+@pytest.mark.network
+def test_check_version(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()
+    assert git.check_version(version_pkg_path, [commit])
+    assert git.check_version(version_pkg_path, [commit[:7]])
+    assert not git.check_version(version_pkg_path, ['branch0.1'])
+    assert not git.check_version(version_pkg_path, ['abc123'])
+
+
+@patch('pip.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': '123456'}
     git = Git()
@@ -69,7 +84,7 @@ def test_check_rev_options_should_handle_branch_name(get_refs_mock):
     assert result == ['123456']
 
 
-@patch('pip.vcs.git.Git.get_refs')
+@patch('pip.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': '123456'}
     git = Git()
@@ -78,7 +93,7 @@ def test_check_rev_options_should_handle_tag_name(get_refs_mock):
     assert result == ['123456']
 
 
-@patch('pip.vcs.git.Git.get_refs')
+@patch('pip.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()
diff --git a/tests/unit/test_vcs.py b/tests/unit/test_vcs.py
index 8b7e6e8ea..223cd53a9 100644
--- a/tests/unit/test_vcs.py
+++ b/tests/unit/test_vcs.py
@@ -1,3 +1,4 @@
+import pytest
 from tests.lib import pyversion
 from pip.vcs import VersionControl
 from pip.vcs.bazaar import Bazaar
@@ -10,7 +11,8 @@ else:
     VERBOSE_FALSE = 0
 
 
-def test_git_get_src_requirements():
+@pytest.fixture
+def git():
     git_url = 'http://github.com/pypa/pip-test-package'
     refs = {
         '0.1': 'a8992fc7ee17e5b9ece022417b64594423caca7c',
@@ -27,9 +29,18 @@ def test_git_get_src_requirements():
     git = Git()
     git.get_url = Mock(return_value=git_url)
     git.get_revision = Mock(return_value=sha)
-    git.get_refs = Mock(return_value=refs)
+    git.get_short_refs = Mock(return_value=refs)
+    return git
+
+
+@pytest.fixture
+def dist():
     dist = Mock()
     dist.egg_name = Mock(return_value='pip_test_package')
+    return dist
+
+
+def test_git_get_src_requirements(git, dist):
     ret = git.get_src_requirement(dist, location='.', find_tags=None)
 
     assert ret == ''.join([
@@ -39,6 +50,16 @@ def test_git_get_src_requirements():
     ])
 
 
+@pytest.mark.parametrize('ref,result', (
+    ('5547fa909e83df8bd743d3978d6667497983a4b7', True),
+    ('5547fa909', True),
+    ('abc123', False),
+    ('foo', False),
+))
+def test_git_check_version(git, ref, result):
+    assert git.check_version('foo', ref) is result
+
+
 def test_translate_egg_surname():
     vc = VersionControl()
     assert vc.translate_egg_surname("foo") == "foo"