Reset the fetch config in the bots before each run.

If a ref is incorrectly set, it will affect later runs.
For example, [1] caused a bad refspec to be added to the fetch config,
(+refs/branch-heads/refs/branch-heads/6.7:refs/branch-heads/refs/branch-heads/6.7)
which affects all subsequent runs in the same bot: [2], [3].

This makes bot_update clear the fetch config before each run, so it is more
robust against these kind of failures.

[1] https://logs.chromium.org/v/?s=v8%2Fbuildbucket%2Fcr-buildbucket.appspot.com%2F8942033142534034848%2F%2B%2Fsteps%2Fbot_update%2F0%2Fstdout
[2] https://logs.chromium.org/v/?s=v8%2Fbuildbucket%2Fcr-buildbucket.appspot.com%2F8941307871443322944%2F%2B%2Fsteps%2Fbot_update%2F0%2Fstdout
[3] https://logs.chromium.org/v/?s=v8%2Fbuildbucket%2Fcr-buildbucket.appspot.com%2F8941333453153279680%2F%2B%2Fsteps%2Fbot_update%2F0%2Fstdout

Bug: 862547
Change-Id: I2f849c604656e81ebd7377465d287226b8bdea1a
Reviewed-on: https://chromium-review.googlesource.com/1135645
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Aaron Gable <agable@chromium.org>
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>

gclient_scm.py

@@ -305,7 +305,7 @@ class GitWrapper(SCMWrapper):
 
   def _FetchAndReset(self, revision, file_list, options):
     """Equivalent to git fetch; git reset."""
-    self._UpdateBranchHeads(options, fetch=False)
+    self._SetFetchConfig(options)
 
     self._Fetch(options, prune=True, quiet=options.verbose)
     self._Scrub(revision, options)
@@ -462,7 +462,7 @@ class GitWrapper(SCMWrapper):
       return self._Capture(['rev-parse', '--verify', 'HEAD'])
 
     if not managed:
-      self._UpdateBranchHeads(options, fetch=False)
+      self._SetFetchConfig(options)
       self.Print('________ unmanaged solution; skipping %s' % self.relpath)
       return self._Capture(['rev-parse', '--verify', 'HEAD'])
 
@@ -547,6 +547,9 @@ class GitWrapper(SCMWrapper):
       else:
         raise gclient_utils.Error('Invalid Upstream: %s' % upstream_branch)
 
+    self._SetFetchConfig(options)
+    self._Fetch(options, prune=options.force)
+
     if not scm.GIT.IsValidRevision(self.checkout_path, revision, sha_only=True):
       # Update the remotes first so we have all the refs.
       remote_output = scm.GIT.Capture(['remote'] + verbose + ['update'],
@@ -554,8 +557,6 @@ class GitWrapper(SCMWrapper):
       if verbose:
         self.Print(remote_output)
 
-    self._UpdateBranchHeads(options, fetch=True)
-
     revision = self._AutoFetchRef(options, revision)
 
     # This is a big hammer, debatable if it should even be here...
@@ -941,7 +942,8 @@ class GitWrapper(SCMWrapper):
       gclient_utils.rmtree(tmp_dir)
       if template_dir:
         gclient_utils.rmtree(template_dir)
-    self._UpdateBranchHeads(options, fetch=True)
+    self._SetFetchConfig(options)
+    self._Fetch(options, prune=options.force)
     revision = self._AutoFetchRef(options, revision)
     remote_ref = scm.GIT.RefToRemoteRef(revision, self.remote)
     self._Checkout(options, ''.join(remote_ref or revision), quiet=True)
@@ -1204,24 +1206,30 @@ class GitWrapper(SCMWrapper):
     # Return the revision that was fetched; this will be stored in 'FETCH_HEAD'
     return self._Capture(['rev-parse', '--verify', 'FETCH_HEAD'])
 
-  def _UpdateBranchHeads(self, options, fetch=False):
+  def _SetFetchConfig(self, options):
     """Adds, and optionally fetches, "branch-heads" and "tags" refspecs
     if requested."""
-    need_fetch = fetch
+    if options.reset:
+      try:
+        self._Run(['config', '--unset-all', 'remote.%s.fetch' % self.remote],
+                  options)
+        self._Run(['config', 'remote.%s.fetch' % self.remote,
+                   '+refs/heads/*:refs/remotes/%s/*' % self.remote], options)
+      except subprocess2.CalledProcessError as e:
+        # If exit code was 5, it means we attempted to unset a config that
+        # didn't exist. Ignore it.
+        if e.returncode != 5:
+          raise
     if hasattr(options, 'with_branch_heads') and options.with_branch_heads:
       config_cmd = ['config', 'remote.%s.fetch' % self.remote,
                     '+refs/branch-heads/*:refs/remotes/branch-heads/*',
                     '^\\+refs/branch-heads/\\*:.*$']
       self._Run(config_cmd, options)
-      need_fetch = True
     if hasattr(options, 'with_tags') and options.with_tags:
       config_cmd = ['config', 'remote.%s.fetch' % self.remote,
                     '+refs/tags/*:refs/tags/*',
                     '^\\+refs/tags/\\*:.*$']
       self._Run(config_cmd, options)
-      need_fetch = True
-    if fetch and need_fetch:
-      self._Fetch(options, prune=options.force)
 
   def _AutoFetchRef(self, options, revision):
     """Attempts to fetch |revision| if not available in local repo.

git_cache.py

@@ -311,10 +311,13 @@ class Mirror(object):
     self.print('running "git %s" in "%s"' % (' '.join(cmd), cwd))
     gclient_utils.CheckCallAndFilter([self.git_exe] + cmd, **kwargs)
 
-  def config(self, cwd=None):
+  def config(self, cwd=None, reset_fetch_config=False):
     if cwd is None:
       cwd = self.mirror_path
 
+    if reset_fetch_config:
+      self.RunGit(['config', '--unset-all', 'remote.origin.fetch'], cwd=cwd)
+
     # Don't run git-gc in a daemon.  Bad things can happen if it gets killed.
     try:
       self.RunGit(['config', 'gc.autodetach', '0'], cwd=cwd)
@@ -508,8 +511,8 @@ class Mirror(object):
             'Shallow fetch requested, but repo cache already exists.')
     return tempdir
 
-  def _fetch(self, rundir, verbose, depth):
-    self.config(rundir)
+  def _fetch(self, rundir, verbose, depth, reset_fetch_config):
+    self.config(rundir, reset_fetch_config)
     v = []
     d = []
     if verbose:
@@ -531,7 +534,8 @@ class Mirror(object):
         logging.warn('Fetch of %s failed' % spec)
 
   def populate(self, depth=None, shallow=False, bootstrap=False,
-               verbose=False, ignore_lock=False, lock_timeout=0):
+               verbose=False, ignore_lock=False, lock_timeout=0,
+               reset_fetch_config=False):
     assert self.GetCachePath()
     if shallow and not depth:
       depth = 10000
@@ -545,14 +549,14 @@ class Mirror(object):
     try:
       tempdir = self._ensure_bootstrapped(depth, bootstrap)
       rundir = tempdir or self.mirror_path
-      self._fetch(rundir, verbose, depth)
+      self._fetch(rundir, verbose, depth, reset_fetch_config)
     except ClobberNeeded:
       # This is a major failure, we need to clean and force a bootstrap.
       gclient_utils.rmtree(rundir)
       self.print(GIT_CACHE_CORRUPT_MESSAGE)
       tempdir = self._ensure_bootstrapped(depth, bootstrap, force=True)
       assert tempdir
-      self._fetch(tempdir, verbose, depth)
+      self._fetch(tempdir, verbose, depth, reset_fetch_config)
     finally:
       if tempdir:
         if os.path.exists(self.mirror_path):
@@ -693,6 +697,8 @@ def CMDpopulate(parser, args):
   parser.add_option('--ignore_locks', '--ignore-locks',
                     action='store_true',
                     help='Don\'t try to lock repository')
+  parser.add_option('--reset-fetch-config', action='store_true', default=False,
+                    help='Reset the fetch config before populating the cache.')
 
   options, args = parser.parse_args(args)
   if not len(args) == 1:
@@ -706,6 +712,7 @@ def CMDpopulate(parser, args):
       'bootstrap': not options.no_bootstrap,
       'ignore_lock': options.ignore_locks,
       'lock_timeout': options.timeout,
+      'reset_fetch_config': options.reset_fetch_config,
   }
   if options.depth:
     kwargs['depth'] = options.depth

recipes/recipe_modules/bot_update/resources/bot_update.py

@@ -354,8 +354,9 @@ def gclient_sync(
   os.close(fd)
 
   args = ['sync', '--verbose', '--reset', '--force',
-         '--ignore_locks', '--output-json', gclient_output_file,
-         '--nohooks', '--noprehooks', '--delete_unversioned_trees']
+          '--ignore_locks', '--output-json', gclient_output_file,
+          '--nohooks', '--noprehooks', '--delete_unversioned_trees',
+          '--reset-fetch-config']
   if with_branch_heads:
     args += ['--with_branch_heads']
   if with_tags:
@@ -653,7 +654,7 @@ def _git_checkout(sln, sln_dir, revisions, refs, git_cache_dir, cleanup_dir):
   name = sln['name']
   url = sln['url']
   populate_cmd = (['cache', 'populate', '--ignore_locks', '-v',
-                   '--cache-dir', git_cache_dir, url])
+                   '--cache-dir', git_cache_dir, url, '--reset-fetch-config'])
   for ref in refs:
     populate_cmd.extend(['--ref', ref])
 

tests/gclient_scm_test.py

@@ -449,6 +449,23 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
     self.assert_(gclient_scm.os.path.isfile(file_path))
     sys.stdout.close()
 
+  def testUpdateResetUnsetsFetchConfig(self):
+    if not self.enabled:
+      return
+    options = self.Options()
+    options.reset = True
+
+    scm = gclient_scm.GitWrapper(self.url, self.root_dir,
+                                 self.relpath)
+    scm._Run(['config', 'remote.origin.fetch',
+              '+refs/heads/bad/ref:refs/remotes/origin/bad/ref'], options)
+
+    file_list = []
+    scm.update(options, (), file_list)
+    self.assertEquals(scm.revinfo(options, (), None),
+                      '069c602044c5388d2d15c3f875b057c852003458')
+    sys.stdout.close()
+
   def testUpdateResetDeleteUnversionedTrees(self):
     if not self.enabled:
       return