Fix GitCheckout.prepare to be sane and use git-cache

checkout.py

 # coding=utf8
 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 """Manages a project checkout.
 
 Includes support for svn, git-svn and git.
 """
 
 import ConfigParser
@@ skipping 548 matching lines @@
       commit_user, post_processors=None, base_ref=None):
     super(GitCheckout, self).__init__(root_dir, project_name, post_processors)
     self.base_ref = base_ref
     self.git_url = git_url
     self.commit_user = commit_user
     self.remote_branch = remote_branch
     # The working branch where patches will be applied. It will track the
     # remote branch.
     self.working_branch = 'working_branch'
     # There is no reason to not hardcode origin.
-    self.remote = 'origin'
-    # There is no reason to not hardcode master.
-    self.master_branch = 'master'
+    self.pull_remote = 'origin'
+    self.push_remote = 'upstream'
 
   def prepare(self, revision):
     """Resets the git repository in a clean state.
 
     Checks it out if not present and deletes the working branch.
     """
+    assert self.git_url
     assert self.remote_branch
-    assert self.git_url
+
+    self._check_call_git(
+        ['cache', 'populate', self.git_url], timeout=FETCH_TIMEOUT, cwd=None)
+    cache_path = self._check_output_git(
+        ['cache', 'exists', self.git_url], cwd=None).strip()
 
     if not os.path.isdir(self.project_path):
       # Clone the repo if the directory is not present.
-      logging.info(
-          'Checking out %s in %s', self.project_name, self.project_path)
       self._check_call_git(
-          ['clone', self.git_url, '-b', self.remote_branch, self.project_path],
+          ['clone', '--shared', cache_path, self.project_path],
           cwd=None, timeout=FETCH_TIMEOUT)
-    else:
-      # Throw away all uncommitted changes in the existing checkout.
-      self._check_call_git(['checkout', self.remote_branch])
-      self._check_call_git(
-          ['reset', '--hard', '--quiet',
-           '%s/%s' % (self.remote, self.remote_branch)])
+      self._call_git(
+          ['config', 'remote.%s.url' % self.push_remote, self.git_url],
+          cwd=self.project_path)
 
-    if revision:
-      try:
-        # Look if the commit hash already exist. If so, we can skip a
-        # 'git fetch' call.
-        revision = self._check_output_git(['rev-parse', revision])
-      except subprocess.CalledProcessError:
-        self._check_call_git(
-            ['fetch', self.remote, self.remote_branch, '--quiet'])
-        revision = self._check_output_git(['rev-parse', revision])
-      self._check_call_git(['checkout', '--force', '--quiet', revision])
-    else:
-      branches, active = self._branches()
-      if active != self.master_branch:
-        self._check_call_git(
-            ['checkout', '--force', '--quiet', self.master_branch])
-      self._sync_remote_branch()
+    if not revision:
+      revision = self.remote_branch
 
-      if self.working_branch in branches:
-        self._call_git(['branch', '-D', self.working_branch])
+    if not re.match(r'[0-9a-f]{40}$', revision):

[iannucci, 2014/03/08 10:15:06]

add a (?i) to make this case insensitive

What about short-hashes?

[agable, 2014/03/11 17:23:30]

Done
Our systems shouldn't be passing around short hashes. Short hashes are solely
for ease-of-use for developers, and should only be used in display contexts.

+      self._check_call_git(['fetch', self.pull_remote, revision])
+      revision = self.pull_remote + '/' + revision
+
+    self._check_call_git(['checkout', '--force', '--quiet', revision])
+    self._call_git(['clean', '-fdx'])
+
+    branches, _ = self._branches()
+    if self.working_branch in branches:
+      self._call_git(['branch', '-D', self.working_branch])
+
     return self._get_head_commit_hash()
 
-  def _sync_remote_branch(self):
-    """Syncs the remote branch."""
-    # We do a 'git pull origin master:refs/remotes/origin/master' instead of
-    # 'git pull origin master' because from the manpage for git-pull: 
-    #   A parameter <ref> without a colon is equivalent to <ref>: when
-    #   pulling/fetching, so it merges <ref> into the current branch without
-    #   storing the remote branch anywhere locally.
-    remote_tracked_path = 'refs/remotes/%s/%s' % (
-        self.remote, self.remote_branch)
-    self._check_call_git(
-        ['pull', self.remote,
-         '%s:%s' % (self.remote_branch, remote_tracked_path),
-         '--quiet'])
-
   def _get_head_commit_hash(self):
     """Gets the current revision (in unicode) from the local branch."""
     return unicode(self._check_output_git(['rev-parse', 'HEAD']).strip())
 
-  def apply_patch(self, patches, post_processors=None, verbose=False, 
+  def apply_patch(self, patches, post_processors=None, verbose=False,
                   name=None, email=None):
     """Applies a patch on 'working_branch' and switches to it.
 
     Also commits the changes on the local branch.
 
     Ignores svn properties and raise an exception on unexpected ones.
     """
     post_processors = post_processors or self.post_processors or []
     # It this throws, the checkout is corrupted. Maybe worth deleting it and
     # trying again?
@@ skipping 68 matching lines @@
     # index.
     cmd = ['commit', '-m', 'Committed patch']
     if name and email:
       cmd = ['-c', 'user.email=%s' % email, '-c', 'user.name=%s' % name] + cmd
     if verbose:
       cmd.append('--verbose')
     self._check_call_git(cmd)
     if self.base_ref:
       base_ref = self.base_ref
     else:
-      base_ref = '%s/%s' % (self.remote,
-                            self.remote_branch or self.master_branch)
+      base_ref = '%s/%s' % (self.pull_remote, self.remote_branch)
     found_files = self._check_output_git(
         ['diff', base_ref,
          '--name-only']).splitlines(False)
     assert sorted(patches.filenames) == sorted(found_files), (
         sorted(patches.filenames), sorted(found_files))
 
   def commit(self, commit_message, user):
     """Commits, updates the commit message and pushes."""
     assert self.commit_user
     assert isinstance(commit_message, unicode)
     current_branch = self._check_output_git(
         ['rev-parse', '--abbrev-ref', 'HEAD']).strip()
     assert current_branch == self.working_branch
- 
+
     commit_cmd = ['commit', '--amend', '-m', commit_message]
     if user and user != self.commit_user:
       # We do not have the first or last name of the user, grab the username
       # from the email and call it the original author's name.
       # TODO(rmistry): Do not need the below if user is already in
       #                "Name <email>" format.
       name = user.split('@')[0]
       commit_cmd.extend(['--author', '%s <%s>' % (name, user)])
     self._check_call_git(commit_cmd)
 
     # Push to the remote repository.
     self._check_call_git(
-        ['push', 'origin', '%s:%s' % (self.working_branch, self.remote_branch),
+        ['push', self.push_remote,
+          '%s:%s' % (self.working_branch, self.remote_branch),
          '--force', '--quiet'])
     # Get the revision after the push.
     revision = self._get_head_commit_hash()
-    # Switch back to the remote_branch and sync it.
-    self._check_call_git(['checkout', self.remote_branch])
-    self._sync_remote_branch()
+    # Switch back to the remote_branch.
+    self._check_call_git(['fetch', self.pull_remote, self.remote_branch])
+    self._check_call_git(['checkout', '--force', '--quiet',
+                          '%s/%s' % (self.pull_remote, self.remote_branch)])
     # Delete the working branch since we are done with it.
     self._check_call_git(['branch', '-D', self.working_branch])
 
     return revision
 
   def _check_call_git(self, args, **kwargs):
     kwargs.setdefault('cwd', self.project_path)
     kwargs.setdefault('stdout', self.VOID)
     kwargs.setdefault('timeout', GLOBAL_TIMEOUT)
     return subprocess2.check_call_out(['git'] + args, **kwargs)
@@ skipping 19 matching lines @@
     for l in out:
       if l.startswith('*'):
         active = l[2:]
         break
     return branches, active
 
   def revisions(self, rev1, rev2):
     """Returns the number of actual commits between both hash."""
     self._fetch_remote()
 
-    rev2 = rev2 or '%s/%s' % (self.remote, self.remote_branch)
+    rev2 = rev2 or '%s/%s' % (self.pull_remote, self.remote_branch)
     # Revision range is ]rev1, rev2] and ordering matters.
     try:
       out = self._check_output_git(
           ['log', '--format="%H"' , '%s..%s' % (rev1, rev2)])
     except subprocess.CalledProcessError:
       return None
     return len(out.splitlines())
 
   def _fetch_remote(self):
     """Fetches the remote without rebasing."""
     # git fetch is always verbose even with -q, so redirect its output.
-    self._check_output_git(['fetch', self.remote, self.remote_branch],
+    self._check_output_git(['fetch', self.pull_remote, self.remote_branch],
                            timeout=FETCH_TIMEOUT)
 
 
 class ReadOnlyCheckout(object):
   """Converts a checkout into a read-only one."""
   def __init__(self, checkout, post_processors=None):
     super(ReadOnlyCheckout, self).__init__()
     self.checkout = checkout
     self.post_processors = (post_processors or []) + (
         self.checkout.post_processors or [])
@@ skipping 17 matching lines @@
   def revisions(self, rev1, rev2):
     return self.checkout.revisions(rev1, rev2)
 
   @property
   def project_name(self):
     return self.checkout.project_name
 
   @property
   def project_path(self):
     return self.checkout.project_path