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..f8f17b19acf6810dd01212f7db3d91cfc39e7fe4 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
@@ -38,11 +39,6 @@ class Backend(object):
     """Returns repo type (see package_pb2.DepSpec)."""
     raise NotImplementedError()
 
-  @staticmethod
-  def branch_spec(branch):
-    """Returns branch spec for given branch suitable for given git backend."""
-    raise NotImplementedError()
-
   def checkout(self, repo, revision, checkout_dir, allow_fetch):
     """Checks out given |repo| at |revision| to |checkout_dir|.
 
@@ -119,53 +115,85 @@ class GitBackend(Backend):
       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):
+    """Checks out given |repo| at |revision| to |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)
+    Network operations are performed only if |allow_fetch| is True.
+
+    Returns the commit hash, if any.
+    """
+    logging.info('Freshening repository %s in %s', repo_url, checkout_dir)
+
+    if revision == 'origin/master':
+      # Some clients pass 'origin/master' when they actually mean
+      # 'refs/heads/master'.
+      # TODO(nodir): remove this if statement BUG(http://crbug.com/696704).
+      revision = 'refs/heads/master'

[Paweł Hajdan Jr., 2017/02/28 08:44:33]

Consider a logging.warning call in this case.

+    assert re.match('^([a-z0-9]{40}|refs/.+)$', revision), revision
+
+    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)
+
+      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 needs to be fetched: %s', revision, e)
+        else:
+          git('checkout', '-q', '-f', revision)
+          return 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)
 
-      # 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')
+    git('init')  # Safe to call on an existing git repo.
 
-    git('reset', '-q', '--hard', revision)
+    # Typically we cannot fetch a commit, so we assume that remote master branch

[Paweł Hajdan Jr., 2017/02/28 08:44:33]

recipes.cfg has a "branch" field, so that assumption might not be true.

How about we fetch everything, or at the very least add a TODO for even more
precise handling of branches?

+    # 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')
+    return git('rev-parse', 'HEAD').strip()
 
   @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')
+    assert re.match('^[a-z0-9]{40}$', revision), revision
+    # Since we are asked for revision..other_revision, revision must be among
+    # ancestors of other_revision and by fetching other_revision we ensure that
+    # both revisions will be present in the repo.
+    other_revision = self.checkout(
+        repo, other_revision, checkout_dir, allow_fetch)
 
+    git = self.Git(checkout_dir=checkout_dir)
     args = [
         'rev-list',
         '--reverse',
@@ -211,10 +239,6 @@ class GitilesBackend(Backend):
   def repo_type(self):
     return package_pb2.DepSpec.GITILES
 
-  @staticmethod
-  def branch_spec(branch):
-    return branch
-
   def checkout(self, repo, revision, checkout_dir, allow_fetch):
     requests_ssl.check_requests_ssl()
     logging.info('Freshening repository %s in %s', repo, checkout_dir)