Completing implementation of GitCheckout in depot_tools

checkout.py

Index: checkout.py
===================================================================
--- checkout.py	(revision 220562)
+++ checkout.py	(working copy)
@@ -131,7 +131,8 @@
     """
     raise NotImplementedError()
 
-  def apply_patch(self, patches, post_processors=None, verbose=False):
+  def apply_patch(self, patches, post_processors=None, verbose=False,
+                  revert=False):
     """Applies a patch and returns the list of modified files.
 
     This function should throw patch.UnsupportedPatchFormat or
@@ -165,7 +166,8 @@
     """Stubbed out."""
     pass
 
-  def apply_patch(self, patches, post_processors=None, verbose=False):
+  def apply_patch(self, patches, post_processors=None, verbose=False,
+                  unused_revert=False):
     """Ignores svn properties."""
     post_processors = post_processors or self.post_processors or []
     for p in patches:
@@ -349,7 +351,8 @@
           (self.project_name, self.project_path))
     return self._revert(revision)
 
-  def apply_patch(self, patches, post_processors=None, verbose=False):
+  def apply_patch(self, patches, post_processors=None, verbose=False,
+                  unused_revert=False):

[M-A Ruel, 2013/09/02 20:35:09]

Keep revert=False for compatibility.

Why not:
assert not revert, 'Implement revert=True'

[rmistry, 2013/09/03 12:40:36]

Yes will do in a separate CL, see the comment below.

     post_processors = post_processors or self.post_processors or []
     for p in patches:
       stdout = []
@@ -550,16 +553,21 @@
     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.
-    self.remote = 'origin'
+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'
 
   def prepare(self, revision):
     """Resets the git repository in a clean state.
@@ -567,10 +575,25 @@
     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' %
+          (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(
@@ -581,13 +604,33 @@
       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 apply_patch(self, patches, post_processors=None, verbose=False):
-    """Applies a patch on 'working_branch' and switch to it.
+  def _sync_remote_branch(self):
+    """Sync 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.
+    self._check_call_git(
+        ['pull', self.remote,
+         '%s:refs/remotes/%s/%s' % (self.remote_branch, self.remote,

[M-A Ruel, 2013/09/02 20:35:09]

I think it'd be more readable if you made it a named variable, so the cmd list
fits a single line.

[rmistry, 2013/09/03 12:40:36]

Done.

+                                    self.remote_branch),
+         '--quiet'])
 
+  def _get_revision(self):
+    """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,
+                  revert=False):
+    """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.
@@ -597,8 +640,9 @@
     # 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:
@@ -631,6 +675,8 @@
             # No need to do anything special with p.is_new or if not
             # p.diff_hunks. git apply manages all that already.
             cmd = ['apply', '--index', '-p%s' % p.patchlevel]
+            if revert:

[M-A Ruel, 2013/09/02 20:35:09]

You mean "reversed"? Is that really needed?

[rmistry, 2013/09/03 12:40:36]

Context behind the revert parameters:
I am also working on adding one-click revert functionality to depot_tools,
rietveld and CQ. I figured I would go ahead and add revert/reversed
functionality to this CL, but it should really be in a separate CL. I reverted
the reverts.

When I do send out that CL (probably after this one is submitted) I will
incorporate your comments there.

+              cmd.append('-R')
             if verbose:
               cmd.append('--verbose')
             stdout.append(self._check_output_git(cmd, stdin=p.get(True)))
@@ -667,21 +713,38 @@
     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.
-
-    Subclass needs to dcommit or push.
-    """
+    """Commits, updates the commit message and pushes."""
     assert isinstance(commit_message, unicode)
-    self._check_call_git(['commit', '--amend', '-m', commit_message])
-    return self._check_output_git(['rev-parse', 'HEAD']).strip()
+ 
+    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.
+      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),
+         '--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)
@@ -727,13 +790,7 @@
 
   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)
 
@@ -752,9 +809,10 @@
   def get_settings(self, key):
     return self.checkout.get_settings(key)
 
-  def apply_patch(self, patches, post_processors=None, verbose=False):
+  def apply_patch(self, patches, post_processors=None, verbose=False,
+                  revert=False):
     return self.checkout.apply_patch(
-        patches, post_processors or self.post_processors, verbose)
+        patches, post_processors or self.post_processors, verbose, revert)
 
   def commit(self, message, user):  # pylint: disable=R0201
     logging.info('Would have committed for %s with message: %s' % (