Completing implementation of GitCheckout in depot_tools

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 532 matching lines @@
     # the order specified.
     try:
       out = self._check_output_svn(
           ['log', '-q', self.svn_url, '-r', '%s:%s' % (rev1, rev2)])
     except subprocess.CalledProcessError:
       return None
     # Ignore the '----' lines.
     return len([l for l in out.splitlines() if l.startswith('r')]) - 1
 
 
-class GitCheckoutBase(CheckoutBase):
-  """Base class for git checkout. Not to be used as-is."""
-  def __init__(self, root_dir, project_name, remote_branch,
-      post_processors=None):
-    super(GitCheckoutBase, self).__init__(
-        root_dir, project_name, post_processors)
-    # There is no reason to not hardcode it.
+class GitCheckout(CheckoutBase):
+  """Manages a git checkout."""
+  def __init__(self, root_dir, project_name, remote_branch, git_url,
+      commit_user, post_processors=None):
+    assert git_url
+    assert commit_user
+    super(GitCheckout, self).__init__(root_dir, project_name, post_processors)
+    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'
-    self.remote_branch = remote_branch
-    self.working_branch = 'working_branch'
 
   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.remote_branch
-    assert os.path.isdir(self.project_path)
-    self._check_call_git(['reset', '--hard', '--quiet'])
+
+    if not os.path.isdir(self.project_path):
+      # Clone the repo if the directory is not present.
+      logging.info('Checking out %s in %s' %

[M-A Ruel, 2013/09/03 13:10:16]

logging.info('Checking out %s in %s', self.project_name, self.project_path)

[rmistry, 2013/09/03 15:46:28]

Done.

+          (self.project_name, self.project_path))
+      self._check_call_git(
+          ['clone', self.git_url, '-b', self.remote_branch, 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)])
+
     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 != 'master':
         self._check_call_git(['checkout', '--force', '--quiet', 'master'])
-      self._check_call_git(['pull', self.remote, self.remote_branch, '--quiet'])
+      self._sync_remote_branch()
+
       if self.working_branch in branches:
         self._call_git(['branch', '-D', self.working_branch])
+    return self._get_revision()
+
+  def _sync_remote_branch(self):
+    """Sync the remote branch."""

[M-A Ruel, 2013/09/03 13:10:16]

Syncs

[rmistry, 2013/09/03 15:46:28]

Done.

+    # 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_revision(self):

[M-A Ruel, 2013/09/03 13:10:16]

maybe _get_head_commit_hash(self):
?
since 'revision' has no real meaning in git.

[rmistry, 2013/09/03 15:46:28]

Makes sense. Done.

+    """Gets the current revision from the local branch."""
+    return self._check_output_git(['rev-parse', 'HEAD']).strip()
 
   def apply_patch(self, patches, post_processors=None, verbose=False):
-    """Applies a patch on 'working_branch' and switch to it.
+    """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?
     if self.remote_branch:
       self._check_call_git(
-          ['checkout', '-b', self.working_branch,
-            '%s/%s' % (self.remote, self.remote_branch), '--quiet'])
+          ['checkout', '-b', self.working_branch, '-t', self.remote_branch,
+           '--quiet'])
+
     for index, p in enumerate(patches):
       stdout = []
       try:
         filepath = os.path.join(self.project_path, p.filename)
         if p.is_delete:
           if (not os.path.exists(filepath) and
               any(p1.source_filename == p.filename for p1 in patches[0:index])):
             # The file was already deleted if a prior patch with file rename
             # was already processed because 'git apply' did it for us.
             pass
@@ skipping 48 matching lines @@
             'While running %s;\n%s%s' % (
               ' '.join(e.cmd),
               align_stdout(stdout),
               align_stdout([getattr(e, 'stdout', '')])))
     # Once all the patches are processed and added to the index, commit the
     # index.
     cmd = ['commit', '-m', 'Committed patch']
     if verbose:
       cmd.append('--verbose')
     self._check_call_git(cmd)
-    # TODO(maruel): Weirdly enough they don't match, need to investigate.
-    #found_files = self._check_output_git(
-    #    ['diff', 'master', '--name-only']).splitlines(False)
-    #assert sorted(patches.filenames) == sorted(found_files), (
-    #    sorted(out), sorted(found_files))
+    found_files = self._check_output_git(
+        ['diff', '%s/%s' % (self.remote, self.remote_branch),
+         '--name-only']).splitlines(False)
+    assert sorted(patches.filenames) == sorted(found_files), (
+        sorted(patches.filenames), sorted(found_files))
 
   def commit(self, commit_message, user):
-    """Updates the commit message.
+    """Commits, updates the commit message and pushes."""
+    assert isinstance(commit_message, unicode)
+ 

[M-A Ruel, 2013/09/03 13:10:16]

Probably worth verifying that working_branch is the current branch.
Also please consider the scenarios where any of the commands below throws. In
practice I think the follow up prepare() will clean the mess up but just be sure
to have thought about it. Otherwise use try: <code> / finally: self.prepare();
but I don't think it'll be necessary.

[rmistry, 2013/09/03 15:46:28]

Added an assert to verify that working_branch is the current branch.
In my experiments it does look like prepare() cleans messes up because it forces
master branch to be at the latest commit hash and deletes the working_branch.

+    commit_cmd = ['commit', '--amend', '-m', commit_message]

[M-A Ruel, 2013/09/03 13:10:16]

You want commit_message.encode('utf-8') otherwise it'll throw on the first
non-ascii character. You can test this with 'é' if you want.

[rmistry, 2013/09/03 15:46:28]

I added 'é' to the commit message in this CL:
https://codereview.chromium.org/22797006/

It looks like it committed with no errors in the test git repo with the commit
message:
cac2319  Modifying file1. testing utf-8 with é by Ravi Mistry - 11 minutes ago
master

Did I test it wrong?

+    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.
+      name = user.split('@')[0]
+      commit_cmd.extend(['--author', '%s <%s>' % (name, user)])

[M-A Ruel, 2013/09/03 13:10:16]

Maybe later: try to detect if "user" is in git-compatible "Name <email>" format.

[rmistry, 2013/09/03 15:46:28]

Added a TODO here.

+    self._check_call_git(commit_cmd)
 
-    Subclass needs to dcommit or push.
-    """
-    assert isinstance(commit_message, unicode)
-    self._check_call_git(['commit', '--amend', '-m', commit_message])
-    return self._check_output_git(['rev-parse', 'HEAD']).strip()
+    # Push to the remote repository.
+    self._check_call_git(
+        ['push', 'origin', '%s:%s' % (self.working_branch, self.remote_branch),
+         '--force', '--quiet'])
+    # Get the revision after the push.
+    revision = self._get_revision()
+    # Switch back to the remote_branch and sync it.
+    self._check_call_git(['checkout', self.remote_branch])
+    self._sync_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)
 
   def _call_git(self, args, **kwargs):
     """Like check_call but doesn't throw on failure."""
     kwargs.setdefault('cwd', self.project_path)
@@ skipping 26 matching lines @@
     # 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."""
-    raise NotImplementedError()
-
-
-class GitCheckout(GitCheckoutBase):
-  """Git checkout implementation."""
-  def _fetch_remote(self):
-    # git fetch is always verbose even with -q -q so redirect its output.
+    # git fetch is always verbose even with -q, so redirect its output.
     self._check_output_git(['fetch', self.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 []) + (
@@ 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