Fix GitBackend.checkout

recipe_engine/fetch.py

 # Copyright 2016 The LUCI Authors. All rights reserved.
 # Use of this source code is governed under the Apache License, Version 2.0
 # that can be found in the LICENSE file.
 
 import base64
 import httplib
 import json
 import logging
 import os
+import re
 import shutil
 import sys
 import tarfile
 import tempfile
 
 # Add third party paths.
 from . import env
 from . import requests_ssl
 from . import util
 from .requests_ssl import requests
@@ skipping 92 matching lines @@
         subcommand = (args[0]) if args else ('')
         is_remote = subcommand in self._REMOTE_SUBCOMMANDS
         raise GitError(is_remote, 'Git "%s" failed: %s' % (
             subcommand, e.message,))
 
     def _execute(self, *args):
       """Runs a raw command. Separate so it's easily mockable."""
       logging.info('Running: %s', args)
       return subprocess42.check_output(args)
 
-
   @property
   def repo_type(self):
     return package_pb2.DepSpec.GIT
 
   @staticmethod
   def branch_spec(branch):
     return 'origin/%s' % branch
 
   @util.exponential_retry(condition=GitError.is_remote_error)
-  def checkout(self, repo, revision, checkout_dir, allow_fetch):
-    logging.info('Freshening repository %s in %s', repo, checkout_dir)
+  def checkout(self, repo_url, revision, checkout_dir, allow_fetch):
+    logging.info('Freshening repository %s in %s', repo_url, checkout_dir)
 
-    git = self.Git()
-    if not os.path.isdir(checkout_dir):
-      if not allow_fetch:
-        raise FetchNotAllowedError(
-            'need to clone %s but fetch not allowed' % repo)
-      git('clone', '-q', repo, checkout_dir)
-    elif not os.path.isdir(os.path.join(checkout_dir, '.git')):
-      raise UncleanFilesystemError(
-          '%s exists but is not a git repo' % checkout_dir)
+    if revision.startswith('origin/'):
+      # Some clients pass "origin/master" when they actually mean "master".
+      # Strip "origin/"
+      # TODO(nodir): raise an exception http://crbug.com/696704

[iannucci, 2017/02/27 20:42:03]

Let's use syntax `BUG(http://crbug.com/696704): ...` and also point at this file
in that bug too.

We really need a way to keep track of those cleanup bugs :(. I feel like they
just all get lost now.

Honestly, restarting all the masters using remote_run isn't that bad and could
probably be done tonight.


I would also make sure, when making this an exception, to assert that revision
starts with "refs/". These are the only refs guaranteed by git to be absolute
(basically "refs/" is the same thing as starting a path with "/" in posix). All
other refs try to pick the 'longest match' suffix out of the refs that the git
server knows about. It's stupid.

[nodir, 2017/02/27 21:23:31]

Done
I need to get other work done today and my work is blocked on this. Sorry, I
cannot wait for master restart.
Done

+      revision = revision[len('origin/'):]
+
+    is_commit = re.match('^[a-z0-9]{40}$', revision)
 
     git = self.Git(checkout_dir=checkout_dir)
-    git('config', 'remote.origin.url', repo)
-    try:
-      git('rev-parse', '-q', '--verify', '%s^{commit}' % revision)
-    except GitError as e:
-      logging.warning('Revision %s is not available: %s', revision, e)
+    if not os.path.isdir(checkout_dir):
+      os.makedirs(checkout_dir)
+    else:
+      if not os.path.isdir(os.path.join(checkout_dir, '.git')):
+        raise UncleanFilesystemError(
+            '%s exists but is not a git repo' % checkout_dir)
 
-      # Revision does not exist. If we can't fetch, then we fail here.
-      if not allow_fetch:
-        raise FetchNotAllowedError(
-            'need to fetch %s but fetch not allowed' % repo)
-      git('fetch')
+      if is_commit:
+        # Avoid doing sever roundtrips if we already have the commit.
+        # Note: rev-parse is only safe if revision is a commit. If the revision
+        # is a ref, they may differ locally and remotely.
+        try:
+          git('rev-parse', '-q', '--verify', '%s^{commit}' % revision)
+        except GitError as e:
+          logging.warning('Revision %s is not available: %s', revision, e)

[iannucci, 2017/02/27 20:42:03]

Revision %s needs to be fetched: %s

So that way the logs aren't confusing (it IS available... just not locally).

[nodir, 2017/02/27 21:23:31]

done

+        else:
+          git('checkout', '-q', '-f', revision)
+          return
 
-    git('reset', '-q', '--hard', revision)
+    if not allow_fetch:
+      # If revision is a commit hash, it is not present in the existing repo, so
+      # we have to fetch.
+      # If revision is a ref, what we have locally may be stale, so we have to
+      # fetch.
+      # We have to fetch anyway.
+      raise FetchNotAllowedError(
+          'need to fetch %s but fetch not allowed' % repo_url)
+
+    git('init')  # Safe to call on an existing git repo.
+
+    # Typically we cannot fetch a commit, so we assume that remote master branch
+    # contains the requested commit. We will checkout the commit afterwards.
+    # Do not name the git remote to minimize repo state and to avoid making it
+    # work with invalid value of "origin/master".
+    # Do not map the fetched commit to anything to avoid checking out possibly
+    # stale refs.
+    git('fetch', repo_url, 'refs/heads/master' if is_commit else revision)
+
+    # FETCH_HEAD points to the fetched commit.
+
+    git('checkout', '-q', '-f', revision if is_commit else 'FETCH_HEAD')
 
   @util.exponential_retry(condition=GitError.is_remote_error)
   def updates(self, repo, revision, checkout_dir, allow_fetch,
               other_revision, paths):
     self.checkout(repo, revision, checkout_dir, allow_fetch)
 
     git = self.Git(checkout_dir=checkout_dir)
     if allow_fetch:
       git('fetch')
 
@@ skipping 162 matching lines @@
     logging.info('fetching %s', url)
 
     resp = requests.get(url)
     if resp.status_code != httplib.OK:
       raise GitilesFetchError(resp.status_code, resp.text)
 
     if not resp.text.startswith(cls._GERRIT_XSRF_HEADER):
       raise GitilesFetchError(resp.status_code, 'Missing XSRF header')
 
     return json.loads(resp.text[len(cls._GERRIT_XSRF_HEADER):])