Merge pull request #5623 from cjerdonek/vcs-fix-install-git-ref

Fix #5624: installing a Git ref for installs other than the first

news/5624.bugfix

@@ -0,0 +1 @@
+Allow a Git ref to be installed over an existing installation.

src/pip/_internal/vcs/__init__.py

@@ -295,7 +295,7 @@ def switch(self, dest, url, rev_options):
         """
         raise NotImplementedError
 
-    def update(self, dest, rev_options):
+    def update(self, dest, url, rev_options):
         """
         Update an already-existing repo to the given ``rev_options``.
 
@@ -345,7 +345,7 @@ def obtain(self, dest):
                         self.repo_name,
                         rev_display,
                     )
-                    self.update(dest, rev_options)
+                    self.update(dest, url, rev_options)
                 else:
                     logger.info('Skipping because already up-to-date.')
                 return

src/pip/_internal/vcs/bazaar.py

@@ -62,7 +62,7 @@ def fetch_new(self, dest, url, rev_options):
     def switch(self, dest, url, rev_options):
         self.run_command(['switch', url], cwd=dest)
 
-    def update(self, dest, rev_options):
+    def update(self, dest, url, rev_options):
         cmd_args = ['pull', '-q'] + rev_options.to_args()
         self.run_command(cmd_args, cwd=dest)
 

src/pip/_internal/vcs/git.py

@@ -117,11 +117,10 @@ def get_revision_sha(self, dest, 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.
-
-        Returns a new RevOptions object for the SHA1 of the branch or tag
-        if found.
+    def resolve_revision(self, dest, url, rev_options):
+        """
+        Resolve a revision to a new RevOptions object with the SHA1 of the
+        branch, tag, or ref if found.
 
         Args:
           rev_options: a RevOptions object.
@@ -139,6 +138,19 @@ def check_rev_options(self, dest, rev_options):
                 "Did not find branch or tag '%s', assuming revision or ref.",
                 rev,
             )
+
+        if not rev.startswith('refs/'):
+            return rev_options
+
+        # If it looks like a ref, we have to fetch it explicitly.
+        self.run_command(
+            ['fetch', '-q', url] + rev_options.to_args(),
+            cwd=dest,
+        )
+        # Change the revision to the SHA of the ref we fetched
+        sha = self.get_revision(dest, rev='FETCH_HEAD')
+        rev_options = rev_options.make_new(sha)
+
         return rev_options
 
     def is_commit_id_equal(self, dest, name):
@@ -164,20 +176,12 @@ def fetch_new(self, dest, url, rev_options):
 
         if rev_options.rev:
             # Then a specific revision was requested.
-            rev_options = self.check_rev_options(dest, rev_options)
+            rev_options = self.resolve_revision(dest, url, 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):
-                rev = rev_options.rev
-                # Only fetch the revision if it's a ref
-                if rev.startswith('refs/'):
-                    self.run_command(
-                        ['fetch', '-q', url] + rev_options.to_args(),
-                        cwd=dest,
-                    )
-                    # Change the revision to the SHA of the ref we fetched
-                    rev = 'FETCH_HEAD'
-                self.run_command(['checkout', '-q', rev], cwd=dest)
+                cmd_args = ['checkout', '-q'] + rev_options.to_args()
+                self.run_command(cmd_args, cwd=dest)
 
         #: repo may contain submodules
         self.update_submodules(dest)
@@ -189,15 +193,15 @@ def switch(self, dest, url, rev_options):
 
         self.update_submodules(dest)
 
-    def update(self, dest, rev_options):
+    def update(self, dest, url, rev_options):
         # First fetch changes from the default remote
         if self.get_git_version() >= parse_version('1.9.0'):
             # fetch tags in addition to everything else
             self.run_command(['fetch', '-q', '--tags'], cwd=dest)
         else:
             self.run_command(['fetch', '-q'], cwd=dest)
         # Then reset to wanted revision (maybe even origin/master)
-        rev_options = self.check_rev_options(dest, rev_options)
+        rev_options = self.resolve_revision(dest, url, rev_options)
         cmd_args = ['reset', '--hard', '-q'] + rev_options.to_args()
         self.run_command(cmd_args, cwd=dest)
         #: update submodules
@@ -218,9 +222,11 @@ def get_url(self, location):
         url = found_remote.split(' ')[1]
         return url.strip()
 
-    def get_revision(self, location):
+    def get_revision(self, location, rev=None):
+        if rev is None:
+            rev = 'HEAD'
         current_rev = self.run_command(
-            ['rev-parse', 'HEAD'], show_stdout=False, cwd=location,
+            ['rev-parse', rev], show_stdout=False, cwd=location,
         )
         return current_rev.strip()
 

src/pip/_internal/vcs/mercurial.py

@@ -59,7 +59,7 @@ def switch(self, dest, url, rev_options):
             cmd_args = ['update', '-q'] + rev_options.to_args()
             self.run_command(cmd_args, cwd=dest)
 
-    def update(self, dest, rev_options):
+    def update(self, dest, url, rev_options):
         self.run_command(['pull', '-q'], cwd=dest)
         cmd_args = ['update', '-q'] + rev_options.to_args()
         self.run_command(cmd_args, cwd=dest)

src/pip/_internal/vcs/subversion.py

@@ -57,7 +57,7 @@ def switch(self, dest, url, rev_options):
         cmd_args = ['switch'] + rev_options.to_args() + [url, dest]
         self.run_command(cmd_args)
 
-    def update(self, dest, rev_options):
+    def update(self, dest, url, rev_options):
         cmd_args = ['update'] + rev_options.to_args() + [dest]
         self.run_command(cmd_args)
 

tests/functional/test_install_vcs.py

@@ -180,23 +180,52 @@ def test_git_with_tag_name_as_revision(script):
     assert '0.1' in version.stdout
 
 
-@pytest.mark.network
-def test_git_with_ref_as_revision(script):
+def _add_ref(script, path, ref):
     """
-    Git backend should be able to install from a ref
+    Add a new ref to a repository at the given path.
     """
-    version_pkg_path = _create_test_package(script)
-    script.run(
-        'git', 'update-ref', 'refs/foo/bar', 'HEAD',
-        expect_stderr=True,
-        cwd=version_pkg_path,
-    )
-    _change_test_package_version(script, version_pkg_path)
-    script.pip(
-        'install', '-e', '%s@refs/foo/bar#egg=version_pkg' %
-        ('git+file://' + version_pkg_path.abspath.replace('\\', '/')),
-        expect_stderr=True
-    )
+    script.run('git', 'update-ref', ref, 'HEAD', expect_stderr=True, cwd=path)
+
+
+def make_version_pkg_url(path, revision=None):
+    path = path.abspath.replace('\\', '/')
+    url_rev = '' if revision is None else '@' + revision
+    url = 'git+file://{}{}#egg=version_pkg'.format(path, url_rev)
+
+    return url
+
+
+def test_git_install_ref(script):
+    """
+    The Git backend should be able to install a ref with the first install.
+    """
+    package_path = _create_test_package(script)
+    _add_ref(script, package_path, 'refs/foo/bar')
+    _change_test_package_version(script, package_path)
+
+    package_url = make_version_pkg_url(package_path, revision='refs/foo/bar')
+    script.pip('install', '-e', package_url, expect_stderr=True)
+    version = script.run('version_pkg')
+    assert '0.1' in version.stdout
+
+
+def test_git_install_then_install_ref(script):
+    """
+    The Git backend should be able to install a ref after a package has
+    already been installed.
+    """
+    package_path = _create_test_package(script)
+    _add_ref(script, package_path, 'refs/foo/bar')
+    _change_test_package_version(script, package_path)
+
+    package_url = make_version_pkg_url(package_path)
+    script.pip('install', '-e', package_url, expect_stderr=True)
+    version = script.run('version_pkg')
+    assert 'some different version' in version.stdout
+
+    # Now install the ref.
+    package_url = make_version_pkg_url(package_path, revision='refs/foo/bar')
+    script.pip('install', '-e', package_url, expect_stderr=True)
     version = script.run('version_pkg')
     assert '0.1' in version.stdout
 

tests/unit/test_vcs.py

@@ -108,37 +108,40 @@ def test_git_get_src_requirements(git, dist):
 
 
 @patch('pip._internal.vcs.git.Git.get_revision_sha')
-def test_git_check_rev_options_ref_exists(get_sha_mock):
+def test_git_resolve_revision_rev_exists(get_sha_mock):
     get_sha_mock.return_value = '123456'
     git = Git()
     rev_options = git.make_rev_options('develop')
 
-    new_options = git.check_rev_options('.', rev_options)
+    url = 'git+https://git.example.com'
+    new_options = git.resolve_revision('.', url, rev_options)
     assert new_options.rev == '123456'
 
 
 @patch('pip._internal.vcs.git.Git.get_revision_sha')
-def test_git_check_rev_options_ref_not_found(get_sha_mock):
+def test_git_resolve_revision_rev_not_found(get_sha_mock):
     get_sha_mock.return_value = None
     git = Git()
     rev_options = git.make_rev_options('develop')
 
-    new_options = git.check_rev_options('.', rev_options)
+    url = 'git+https://git.example.com'
+    new_options = git.resolve_revision('.', url, rev_options)
     assert new_options.rev == 'develop'
 
 
 @patch('pip._internal.vcs.git.Git.get_revision_sha')
-def test_git_check_rev_options_not_found_warning(get_sha_mock, caplog):
+def test_git_resolve_revision_not_found_warning(get_sha_mock, caplog):
     get_sha_mock.return_value = None
     git = Git()
 
+    url = 'git+https://git.example.com'
     sha = 40 * 'a'
     rev_options = git.make_rev_options(sha)
-    new_options = git.check_rev_options('.', rev_options)
+    new_options = git.resolve_revision('.', url, rev_options)
     assert new_options.rev == sha
 
     rev_options = git.make_rev_options(sha[:6])
-    new_options = git.check_rev_options('.', rev_options)
+    new_options = git.resolve_revision('.', url, rev_options)
     assert new_options.rev == 'aaaaaa'
 
     # Check that a warning got logged only for the abbreviated hash.