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 11 matching lines @@
 class FetchNotAllowedError(FetchError):
   pass
 
 
 class Backend(object):
   @property
   def repo_type(self):
     """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|.
 
     Network operations are performed only if |allow_fetch| is True.
     """
     raise NotImplementedError()
 
   def updates(self, repo, revision, checkout_dir, allow_fetch,
               other_revision, paths):
     """Returns a list of revisions between |revision| and |other_revision|.
@@ skipping 56 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_url, revision, checkout_dir, allow_fetch):
+    """Checks out given |repo| at |revision| to |checkout_dir|.
 
-  @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)
+    Network operations are performed only if |allow_fetch| is True.
 
-    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)
+    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)
 
-      # 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 needs to be fetched: %s', revision, e)
+        else:
+          git('checkout', '-q', '-f', revision)
+          return revision
 
-    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

[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)
+
+    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)
-    if allow_fetch:
-      git('fetch')
-
     args = [
         'rev-list',
         '--reverse',
         '%s..%s' % (revision, other_revision),
     ]
     if paths:
       args.extend(['--'] + paths)
     return filter(bool, git(*args).strip().split('\n'))
 
   def commit_metadata(self, repo, revision, checkout_dir, allow_fetch):
@@ skipping 25 matching lines @@
 class GitilesBackend(Backend):
   """GitilesBackend uses a repo served by Gitiles."""
 
   # Header at the beginning of Gerrit/Gitiles JSON API responses.
   _GERRIT_XSRF_HEADER = ')]}\'\n'
 
   @property
   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)
 
     # TODO(phajdan.jr): implement caching.
     if not allow_fetch:
       raise FetchNotAllowedError(
           'need to download %s from gitiles but fetch not allowed' % repo)
 
     revision = self._resolve_revision(repo, revision)
@@ skipping 103 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):])