Clean up cache_dir handling in gclient/git_cache/bot_update.

This CL makes a couple changes:
  * The goal here is to be able to specify git cache entirely from the
    environment variable $GIT_CACHE_PATH and not require special treatment from
    bot_update. Eventually this will be specified at the swarming task level
    instead of in the recipe (i.e. "cached git" will eventually be an
    implementation detail of git on the bots and completely transparent to
    all other software).
  * Removal of the general --cache-dir option from gclient. This option was
    error-prone; it doesn't actually make sense to configure this on
    a per-invocation basis. The sole exception was `gclient config`, which
    now allows this option to be set directly.
  * Consolidation of GitWrapper.cache_dir and GitWrapper._GetMirror; previously
    these two things could disagree, leading to weird intermediate states. Now
    they're the same value.

R=agable@chromium.org, ehmaldonado@chromium.org, tandrii@chromium.org

Bug: 823434
Change-Id: I14adc7619b5fc10768ce32be2651c6215ba94aff
Reviewed-on: https://chromium-review.googlesource.com/1105473Reviewed-by: Aaron Gable <agable@chromium.org>
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Robbie Iannucci <iannucci@chromium.org>

gclient.py

@@ -110,6 +110,11 @@ import subprocess2
 import setup_color
 
 
+# Singleton object to represent an unset cache_dir (as opposed to a disabled
+# one, e.g. if a spec explicitly says `cache_dir = None`.)
+UNSET_CACHE_DIR = object()
+
+
 class GNException(Exception):
   pass
 
@@ -1252,9 +1257,13 @@ solutions = [
     "custom_vars": %(custom_vars)r,
   },
 ]
+""")
+
+  DEFAULT_CLIENT_CACHE_DIR_TEXT = ("""\
 cache_dir = %(cache_dir)r
 """)
 
+
   DEFAULT_SNAPSHOT_FILE_TEXT = ("""\
 # Snapshot generated with gclient revinfo --snapshot
 solutions = %(solution_list)s
@@ -1349,13 +1358,13 @@ it or fix the checkout.
     else:
       self._enforced_cpu = tuple(set(self._enforced_cpu).union(target_cpu))
 
-    cache_dir = config_dict.get('cache_dir', self._options.cache_dir)
-    if cache_dir:
-      cache_dir = os.path.join(self.root_dir, cache_dir)
-      cache_dir = os.path.abspath(cache_dir)
+    cache_dir = config_dict.get('cache_dir', UNSET_CACHE_DIR)
+    if cache_dir is not UNSET_CACHE_DIR:
+      if cache_dir:
+        cache_dir = os.path.join(self.root_dir, cache_dir)
+        cache_dir = os.path.abspath(cache_dir)
 
-    gclient_scm.GitWrapper.cache_dir = cache_dir
-    git_cache.Mirror.SetCachePath(cache_dir)
+      git_cache.Mirror.SetCachePath(cache_dir)
 
     if not target_os and config_dict.get('target_os_only', False):
       raise gclient_utils.Error('Can\'t use target_os_only if target_os is '
@@ -1427,15 +1436,22 @@ it or fix the checkout.
     return client
 
   def SetDefaultConfig(self, solution_name, deps_file, solution_url,
-                       managed=True, cache_dir=None, custom_vars=None):
-    self.SetConfig(self.DEFAULT_CLIENT_FILE_TEXT % {
+                       managed=True, cache_dir=UNSET_CACHE_DIR,
+                       custom_vars=None):
+    text = self.DEFAULT_CLIENT_FILE_TEXT
+    format_dict = {
       'solution_name': solution_name,
       'solution_url': solution_url,
       'deps_file': deps_file,
       'managed': managed,
-      'cache_dir': cache_dir,
       'custom_vars': custom_vars or {},
-    })
+    }
+
+    if cache_dir is not UNSET_CACHE_DIR:
+      text += self.DEFAULT_CLIENT_CACHE_DIR_TEXT
+      format_dict['cache_dir'] = cache_dir
+
+    self.SetConfig(text % format_dict)
 
   def _SaveEntries(self):
     """Creates a .gclient_entries file to record the list of unique checkouts.
@@ -2337,6 +2353,11 @@ def CMDconfig(parser, args):
                          'to have the main solution untouched by gclient '
                          '(gclient will check out unmanaged dependencies but '
                          'will never sync them)')
+  parser.add_option('--cache-dir', default=UNSET_CACHE_DIR,
+                    help='Cache all git repos into this dir and do shared '
+                         'clones from the cache, instead of cloning directly '
+                         'from the remote. Pass "None" to disable cache, even '
+                         'if globally enabled due to $GIT_CACHE_PATH.')
   parser.add_option('--custom-var', action='append', dest='custom_vars',
                     default=[],
                     help='overrides variables; key=value syntax')
@@ -2348,6 +2369,10 @@ def CMDconfig(parser, args):
       (not options.spec and not args)):
     parser.error('Inconsistent arguments. Use either --spec or one or 2 args')
 
+  if (options.cache_dir is not UNSET_CACHE_DIR
+      and options.cache_dir.lower() == 'none'):
+    options.cache_dir = None
+
   custom_vars = {}
   for arg in options.custom_vars:
     kv = arg.split('=', 1)
@@ -2845,12 +2870,6 @@ class OptionParser(optparse.OptionParser):
         '--spec',
         help='create a gclient file containing the provided string. Due to '
             'Cygwin/Python brokenness, it can\'t contain any newlines.')
-    self.add_option(
-        '--cache-dir',
-        help='(git only) Cache all git repos into this dir and do '
-             'shared clones from the cache, instead of cloning '
-             'directly from the remote. (experimental)',
-        default=os.environ.get('GCLIENT_CACHE_DIR'))
     self.add_option(
         '--no-nag-max', default=False, action='store_true',
         help='Ignored for backwards compatibility.')

gclient_scm.py

@@ -138,7 +138,7 @@ class SCMWrapper(object):
     return log.splitlines()[0].split(' ', 1)[1]
 
   def GetCacheMirror(self):
-    if (getattr(self, 'cache_dir', None)):
+    if getattr(self, 'cache_dir', None):
       url, _ = gclient_utils.SplitUrlRevision(self.url)
       return git_cache.Mirror(url)
     return None
@@ -210,7 +210,12 @@ class GitWrapper(SCMWrapper):
   name = 'git'
   remote = 'origin'
 
-  cache_dir = None
+  @property
+  def cache_dir(self):
+    try:
+      return git_cache.Mirror.GetCachePath()
+    except RuntimeError:
+      return None
 
   def __init__(self, url=None, *args, **kwargs):
     """Removes 'git+' fake prefix from git URL."""
@@ -832,7 +837,7 @@ class GitWrapper(SCMWrapper):
 
   def _GetMirror(self, url, options):
     """Get a git_cache.Mirror object for the argument url."""
-    if not git_cache.Mirror.GetCachePath():
+    if not self.cache_dir:
       return None
     mirror_kwargs = {
         'print_func': self.filter,

git_cache.py

@@ -195,6 +195,11 @@ class Mirror(object):
     os.path.dirname(os.path.abspath(__file__)), 'gsutil.py')
   cachepath_lock = threading.Lock()
 
+  UNSET_CACHEPATH = object()
+
+  # Used for tests
+  _GIT_CONFIG_LOCATION = []
+
   @staticmethod
   def parse_fetch_spec(spec):
     """Parses and canonicalizes a fetch spec.
@@ -272,14 +277,18 @@ class Mirror(object):
       if not hasattr(cls, 'cachepath'):
         try:
           cachepath = subprocess.check_output(
-              [cls.git_exe, 'config', '--global', 'cache.cachepath']).strip()
+              [cls.git_exe, 'config'] +
+              cls._GIT_CONFIG_LOCATION +
+              ['cache.cachepath']).strip()
         except subprocess.CalledProcessError:
-          cachepath = None
-        if not cachepath:
-          raise RuntimeError(
-              'No global cache.cachepath git configuration found.')
+          cachepath = os.environ.get('GIT_CACHE_PATH', cls.UNSET_CACHEPATH)
         setattr(cls, 'cachepath', cachepath)
-      return getattr(cls, 'cachepath')
+
+      ret = getattr(cls, 'cachepath')
+      if ret is cls.UNSET_CACHEPATH:
+        raise RuntimeError('No cache.cachepath git configuration or '
+                           '$GIT_CACHE_PATH is set.')
+      return ret
 
   def Rename(self, src, dst):
     # This is somehow racy on Windows.
@@ -795,7 +804,10 @@ class OptionParser(optparse.OptionParser):
   def __init__(self, *args, **kwargs):
     optparse.OptionParser.__init__(self, *args, prog='git cache', **kwargs)
     self.add_option('-c', '--cache-dir',
-                    help='Path to the directory containing the cache')
+                    help=(
+                      'Path to the directory containing the caches. Normally '
+                      'deduced from git config cache.cachepath or '
+                      '$GIT_CACHE_PATH.'))
     self.add_option('-v', '--verbose', action='count', default=1,
                     help='Increase verbosity (can be passed multiple times)')
     self.add_option('-q', '--quiet', action='store_true',

tests/gclient_smoketest.py

@@ -189,10 +189,10 @@ class GClientSmoke(GClientSmokeBase):
           '    },\n'
           '    "custom_vars": {},\n'
           '  },\n'
-          ']\n'
-          'cache_dir = None\n') % self.git_base)
+          ']\n' % self.git_base))
 
-    test(['config', self.git_base + 'repo_1', '--name', 'src'],
+    test(['config', self.git_base + 'repo_1', '--name', 'src',
+          '--cache-dir', 'none'],
          ('solutions = [\n'
           '  { "name"        : "src",\n'
           '    "url"         : "%srepo_1",\n'
@@ -205,7 +205,8 @@ class GClientSmoke(GClientSmokeBase):
           ']\n'
           'cache_dir = None\n') % self.git_base)
 
-    test(['config', 'https://example.com/foo', 'faa'],
+    test(['config', 'https://example.com/foo', 'faa',
+          '--cache-dir', 'something'],
          'solutions = [\n'
          '  { "name"        : "foo",\n'
          '    "url"         : "https://example.com/foo",\n'
@@ -216,7 +217,7 @@ class GClientSmoke(GClientSmokeBase):
          '    "custom_vars": {},\n'
          '  },\n'
          ']\n'
-         'cache_dir = None\n')
+         'cache_dir = \'something\'\n')
 
     test(['config', 'https://example.com/foo', '--deps', 'blah'],
          'solutions = [\n'
@@ -228,8 +229,7 @@ class GClientSmoke(GClientSmokeBase):
          '    },\n'
          '    "custom_vars": {},\n'
          '  },\n'
-         ']\n'
-         'cache_dir = None\n')
+         ']\n')
 
     test(['config', self.git_base + 'src/',
           '--custom-var', 'bool_var=True',
@@ -243,8 +243,7 @@ class GClientSmoke(GClientSmokeBase):
           '    },\n'
           '    "custom_vars": {\'bool_var\': True, \'str_var\': \'abc\'},\n'
           '  },\n'
-          ']\n'
-          'cache_dir = None\n') % self.git_base)
+          ']\n') % self.git_base)
 
     test(['config', '--spec', '["blah blah"]'], '["blah blah"]')
 

tests/git_cache_test.py

@@ -51,6 +51,78 @@ class GitCacheTest(unittest.TestCase):
       mirror = git_cache.Mirror('test://phony.example.biz', refs=fetch_specs)
       self.assertItemsEqual(mirror.fetch_specs, expected)
 
+
+class GitCacheDirTest(unittest.TestCase):
+  def setUp(self):
+    try:
+      delattr(git_cache.Mirror, 'cachepath')
+    except AttributeError:
+      pass
+    super(GitCacheDirTest, self).setUp()
+
+  def tearDown(self):
+    try:
+      delattr(git_cache.Mirror, 'cachepath')
+    except AttributeError:
+      pass
+    super(GitCacheDirTest, self).tearDown()
+
+  def test_git_config_read(self):
+    (fd, tmpFile) = tempfile.mkstemp()
+    old = git_cache.Mirror._GIT_CONFIG_LOCATION
+    try:
+      try:
+        os.write(fd, '[cache]\n  cachepath="hello world"\n')
+      finally:
+        os.close(fd)
+
+      git_cache.Mirror._GIT_CONFIG_LOCATION = ['-f', tmpFile]
+
+      self.assertEqual(git_cache.Mirror.GetCachePath(), 'hello world')
+    finally:
+      git_cache.Mirror._GIT_CONFIG_LOCATION = old
+      os.remove(tmpFile)
+
+  def test_environ_read(self):
+    path = os.environ.get('GIT_CACHE_PATH')
+    config = os.environ.get('GIT_CONFIG')
+    try:
+      os.environ['GIT_CACHE_PATH'] = 'hello world'
+      os.environ['GIT_CONFIG'] = 'disabled'
+
+      self.assertEqual(git_cache.Mirror.GetCachePath(), 'hello world')
+    finally:
+      for name, val in zip(('GIT_CACHE_PATH', 'GIT_CONFIG'), (path, config)):
+        if val is None:
+          os.environ.pop(name, None)
+        else:
+          os.environ[name] = val
+
+  def test_manual_set(self):
+    git_cache.Mirror.SetCachePath('hello world')
+    self.assertEqual(git_cache.Mirror.GetCachePath(), 'hello world')
+
+  def test_unconfigured(self):
+    path = os.environ.get('GIT_CACHE_PATH')
+    config = os.environ.get('GIT_CONFIG')
+    try:
+      os.environ.pop('GIT_CACHE_PATH', None)
+      os.environ['GIT_CONFIG'] = 'disabled'
+
+      with self.assertRaisesRegexp(RuntimeError, 'cache\.cachepath'):
+        git_cache.Mirror.GetCachePath()
+
+      # negatively cached value still raises
+      with self.assertRaisesRegexp(RuntimeError, 'cache\.cachepath'):
+        git_cache.Mirror.GetCachePath()
+    finally:
+      for name, val in zip(('GIT_CACHE_PATH', 'GIT_CONFIG'), (path, config)):
+        if val is None:
+          os.environ.pop(name, None)
+        else:
+          os.environ[name] = val
+
+
 if __name__ == '__main__':
   sys.exit(coverage_utils.covered_main((
     os.path.join(DEPOT_TOOLS_ROOT, 'git_cache.py')