Make sure _UpdateMirrorIfNotContains fetches the specified revision

_UpdateMirrorIfNotContains populates the mirror if a given rev
isn't found in the mirror, but if this rev is invalid, it's never
fetched at all. Check that we've actually fetched it after
populating the mirror.

Bug: 407864212
Change-Id: Id66695636e749c4f7372aa522ab03ec4ec8feb52
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6441962
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>
Commit-Queue: Gavin Mak <gavinmak@google.com>

gclient_scm.py

@@ -1330,9 +1330,11 @@ class GitWrapper(SCMWrapper):
         return git_cache.Mirror(url, **mirror_kwargs)
 
     def _UpdateMirrorIfNotContains(self, mirror, options, rev_type, revision):
-        """Update a git mirror by fetching the latest commits from the remote,
-    unless mirror already contains revision whose type is sha1 hash.
-    """
+        """Update a git mirror unless it already contains a hash revision.
+
+        This raises an error if a hash revision isn't present even after
+        fetching from the remote.
+        """
         if rev_type == 'hash' and mirror.contains_revision(revision):
             if options.verbose:
                 self.Print('skipping mirror update, it has rev=%s already' %
@@ -1349,6 +1351,10 @@ class GitWrapper(SCMWrapper):
                         depth=depth,
                         lock_timeout=getattr(options, 'lock_timeout', 0))
 
+        # Make sure we've actually fetched the revision we want.
+        if rev_type == 'hash' and not mirror.contains_revision(revision):
+            raise gclient_utils.Error(f'Failed to fetch {revision}.')
+
     def _Clone(self, revision, url, options):
         """Clone a git repository from the given URL.
 

tests/gclient_scm_test.py

@@ -502,8 +502,8 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
                                              self.relpath)
         file_list = []
         git_wrapper.update(options, (), file_list)
-        self.assert_(gclient_scm.os.path.isdir(dir_path))
-        self.assert_(gclient_scm.os.path.isfile(file_path))
+        self.assertTrue(gclient_scm.os.path.isdir(dir_path))
+        self.assertTrue(gclient_scm.os.path.isfile(file_path))
         sys.stdout.close()
 
     def testUpdateResetUnsetsFetchConfig(self):
@@ -545,8 +545,8 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
                                              self.relpath)
         file_list = []
         git_wrapper.update(options, (), file_list)
-        self.assert_(not gclient_scm.os.path.isdir(dir_path))
-        self.assert_(gclient_scm.os.path.isfile(file_path))
+        self.assertTrue(not gclient_scm.os.path.isdir(dir_path))
+        self.assertTrue(gclient_scm.os.path.isfile(file_path))
         sys.stdout.close()
 
     def testUpdateUnstagedConflict(self):
@@ -596,8 +596,8 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
         with open(file_path, 'w'):
             pass
         git_wrapper.update(options, (), [])
-        self.assertRegexpMatches(sys.stdout.getvalue(),
-                                 r'breaking lock.*\.git[/|\\]index\.lock')
+        self.assertRegex(sys.stdout.getvalue(),
+                         r'breaking lock.*\.git[/|\\]index\.lock')
         self.assertFalse(os.path.exists(file_path))
         sys.stdout.close()
 
@@ -1298,12 +1298,14 @@ class GerritChangesTest(fake_repos.FakeReposTestBase):
                          self.gitrevparse(self.root_dir))
 
     def testCanCloneGerritChangeMirror(self):
-        self.setUpMirror()
-        self.testCanCloneGerritChange()
+        with mock.patch('git_cache.Mirror.contains_revision',
+                        side_effect=lambda r: r == 'refs/changes/35/1235/1'):
+            self.testCanCloneGerritChange()
 
     def testCanSyncToGerritChangeMirror(self):
-        self.setUpMirror()
-        self.testCanSyncToGerritChange()
+        with mock.patch('git_cache.Mirror.contains_revision',
+                        side_effect=lambda r: r == 'refs/changes/35/1235/1'):
+            self.testCanSyncToGerritChange()
 
     def testMirrorPushUrl(self):
         self.setUpMirror()