Fix GitBackend.checkout

recipe_engine/fetch.py

Index: recipe_engine/fetch.py
diff --git a/recipe_engine/fetch.py b/recipe_engine/fetch.py
index 4554f4c8e5f0af0a1a20bc8a3a71f4cabf772e86..acb57940a55e222228f61b4f4a9c54d8fe9221fb 100644
--- a/recipe_engine/fetch.py
+++ b/recipe_engine/fetch.py
@@ -7,6 +7,7 @@ import httplib
 import json
 import logging
 import os
+import re
 import shutil
 import sys
 import tarfile
@@ -119,7 +120,6 @@ class GitBackend(Backend):
       logging.info('Running: %s', args)
       return subprocess42.check_output(args)
 
-
   @property
   def repo_type(self):
     return package_pb2.DepSpec.GIT
@@ -129,33 +129,59 @@ class GitBackend(Backend):
     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)
-
-      # 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 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)
+
+      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
+
+    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('reset', '-q', '--hard', revision)
+    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,