Completing implementation of GitCheckout in depot_tools.

Tested with:
* Unit tests have been added and they work.
* I also tested end-to-end using a skiabot-test repository in https://skia.googlesource.com/ (it is hidden). CLs I tested with are:
** https://codereview.chromium.org/22797006/ : Add whitespace in file1
** https://codereview.chromium.org/22815013/ : Remove whitespace from file1
** https://codereview.chromium.org/22867025/ : Add new files in directories
** https://codereview.chromium.org/22901018/ : Edit file in directory and delete file in directory
** https://codereview.chromium.org/22918014/ : Add, Delete and Modify 3 files
** https://codereview.chromium.org/23360004/ : Add new files in new directories


Note:
* When committing GitCheckout uses the --author to specify the original author. The author flag takes in 'Firstname Lastname <email_addr>' but we do not know the Firstname and LastName of the original author, which is why the code here parses out the username from the email address and uses it.
  Eg: For email address xyz@example.com it passes in --author 'xyz <xyz@example.com>'
* An example of the changes required in a project to use GitCheckout instead of SvnCheckout is in https://codereview.chromium.org/22859063/


Created to fix the following feature requests-
https://code.google.com/p/chromium/issues/detail?id=261619 : Update the Chrome commit queue to push to src.git.
https://code.google.com/p/skia/issues/detail?id=1593 : Add Git support to the Commit Queue.

Review URL: https://chromiumcodereview.appspot.com/22794015

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@221392 0039d316-1c4b-4281-b951-d872f2087c98

apply_issue.py

@@ -144,7 +144,7 @@ def main():
   if scm_type == 'svn':
     scm_obj = checkout.SvnCheckout(full_dir, None, None, None, None)
   elif scm_type == 'git':
-    scm_obj = checkout.GitCheckoutBase(full_dir, None, None)
+    scm_obj = checkout.GitCheckout(full_dir, None, None)
   elif scm_type == None:
     scm_obj = checkout.RawCheckout(full_dir, None, None)
   else:

checkout.py

@@ -550,16 +550,21 @@ class SvnCheckout(CheckoutBase, SvnMixIn):
     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 +572,25 @@ class GitCheckoutBase(CheckoutBase):
     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,12 +601,32 @@ class GitCheckoutBase(CheckoutBase):
       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_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 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.
 
@@ -597,8 +637,9 @@ class GitCheckoutBase(CheckoutBase):
     # 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:
@@ -667,20 +708,42 @@ class GitCheckoutBase(CheckoutBase):
     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()
+    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),
+         '--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()
+    # 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)
@@ -727,13 +790,7 @@ class GitCheckoutBase(CheckoutBase):
 
   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)
 

tests/checkout_test.py

@@ -32,6 +32,8 @@ BAD_PATCH = ''.join(
 
 
 class FakeRepos(fake_repos.FakeReposBase):
+  TEST_GIT_REPO = 'repo_1'
+
   def populateSvn(self):
     """Creates a few revisions of changes files."""
     subprocess2.check_call(
@@ -39,11 +41,63 @@ class FakeRepos(fake_repos.FakeReposBase):
          '--non-interactive', '--no-auth-cache',
          '--username', self.USERS[0][0], '--password', self.USERS[0][1]])
     assert os.path.isdir(os.path.join(self.svn_checkout, '.svn'))
-    self._commit_svn(self._tree_1())
-    self._commit_svn(self._tree_2())
+    self._commit_svn(self._svn_tree_1())
+    self._commit_svn(self._svn_tree_2())
+
+  def populateGit(self):
+    """Creates a few revisions of changes files."""
+    self._commit_git(self.TEST_GIT_REPO, self._git_tree())
+    # Fix for the remote rejected error. For more details see:
+    # http://stackoverflow.com/questions/2816369/git-push-error-remote
+    subprocess2.check_output(
+        ['git', '--git-dir',
+         os.path.join(self.git_root, self.TEST_GIT_REPO, '.git'),
+         'config', '--bool', 'core.bare', 'true'])
+
+    assert os.path.isdir(
+        os.path.join(self.git_root, self.TEST_GIT_REPO, '.git'))
 
   @staticmethod
-  def _tree_1():
+  def _git_tree():
+    fs = {}
+    fs['origin'] = 'git@1'
+    fs['extra'] = 'dummy\n'  # new
+    fs['codereview.settings'] = (
+        '# Test data\n'
+        'bar: pouet\n')
+    fs['chrome/file.cc'] = (
+        'a\n'
+        'bb\n'
+        'ccc\n'
+        'dd\n'
+        'e\n'
+        'ff\n'
+        'ggg\n'
+        'hh\n'
+        'i\n'
+        'jj\n'
+        'kkk\n'
+        'll\n'
+        'm\n'
+        'nn\n'
+        'ooo\n'
+        'pp\n'
+        'q\n')
+    fs['chromeos/views/DOMui_menu_widget.h'] = (
+      '// Copyright (c) 2010\n'
+      '// Use of this source code\n'
+      '// found in the LICENSE file.\n'
+      '\n'
+      '#ifndef DOM\n'
+      '#define DOM\n'
+      '#pragma once\n'
+      '\n'
+      '#include <string>\n'
+      '#endif\n')
+    return fs
+
+  @staticmethod
+  def _svn_tree_1():
     fs = {}
     fs['trunk/origin'] = 'svn@1'
     fs['trunk/codereview.settings'] = (
@@ -70,8 +124,8 @@ class FakeRepos(fake_repos.FakeReposBase):
     return fs
 
   @classmethod
-  def _tree_2(cls):
-    fs = cls._tree_1()
+  def _svn_tree_2(cls):
+    fs = cls._svn_tree_1()
     fs['trunk/origin'] = 'svn@2\n'
     fs['trunk/extra'] = 'dummy\n'
     fs['trunk/bin_file'] = '\x00'
@@ -88,9 +142,6 @@ class FakeRepos(fake_repos.FakeReposBase):
       '#endif\n')
     return fs
 
-  def populateGit(self):
-    raise NotImplementedError()
-
 
 # pylint: disable=R0201
 class BaseTest(fake_repos.FakeReposTestBase):
@@ -127,70 +178,10 @@ class BaseTest(fake_repos.FakeReposTestBase):
     ])
 
   def get_trunk(self, modified):
-    tree = {}
-    subroot = 'trunk/'
-    for k, v in self.FAKE_REPOS.svn_revs[-1].iteritems():
-      if k.startswith(subroot):
-        f = k[len(subroot):]
-        assert f not in tree
-        tree[f] = v
-
-    if modified:
-      content_lines = tree['chrome/file.cc'].splitlines(True)
-      tree['chrome/file.cc'] = ''.join(
-          content_lines[0:5] + ['FOO!\n'] + content_lines[5:])
-      del tree['extra']
-      tree['new_dir/subdir/new_file'] = 'A new file\nshould exist.\n'
-    return tree
-
-  def _check_base(self, co, root, git, expected):
-    read_only = isinstance(co, checkout.ReadOnlyCheckout)
-    self.assertEquals(not read_only, bool(expected))
-    self.assertEquals(read_only, self.is_read_only)
-    if not read_only:
-      self.FAKE_REPOS.svn_dirty = True
-
-    self.assertEquals(root, co.project_path)
-    self.assertEquals(self.previous_log['revision'], co.prepare(None))
-    self.assertEquals('pouet', co.get_settings('bar'))
-    self.assertTree(self.get_trunk(False), root)
-    patches = self.get_patches()
-    co.apply_patch(patches)
-    self.assertEquals(
-        ['bin_file', 'chrome/file.cc', 'new_dir/subdir/new_file', 'extra'],
-        patches.filenames)
-
-    if git:
-      # Hackish to verify _branches() internal function.
-      # pylint: disable=W0212
-      self.assertEquals(
-          (['master', 'working_branch'], 'working_branch'),
-          co._branches())
-
-    # Verify that the patch is applied even for read only checkout.
-    self.assertTree(self.get_trunk(True), root)
-    fake_author = self.FAKE_REPOS.USERS[1][0]
-    revision = co.commit(u'msg', fake_author)
-    # Nothing changed.
-    self.assertTree(self.get_trunk(True), root)
-
-    if read_only:
-      self.assertEquals('FAKE', revision)
-      self.assertEquals(self.previous_log['revision'], co.prepare(None))
-      # Changes should be reverted now.
-      self.assertTree(self.get_trunk(False), root)
-      expected = self.previous_log
-    else:
-      self.assertEquals(self.previous_log['revision'] + 1, revision)
-      self.assertEquals(self.previous_log['revision'] + 1, co.prepare(None))
-      self.assertTree(self.get_trunk(True), root)
-      expected = expected.copy()
-      expected['msg'] = 'msg'
-      expected['revision'] = self.previous_log['revision'] + 1
-      expected.setdefault('author', fake_author)
+    raise NotImplementedError()
 
-    actual = self._log()
-    self.assertEquals(expected, actual)
+  def _check_base(self, co, root, expected):
+    raise NotImplementedError()
 
   def _check_exception(self, co, err_msg):
     co.prepare(None)
@@ -285,9 +276,205 @@ class SvnBaseTest(BaseTest):
         data['revprops'].append((prop.attrib['name'], prop.text))
     return data
 
+  def _check_base(self, co, root, expected):
+    read_only = isinstance(co, checkout.ReadOnlyCheckout)
+    self.assertEquals(not read_only, bool(expected))
+    self.assertEquals(read_only, self.is_read_only)
+    if not read_only:
+      self.FAKE_REPOS.svn_dirty = True
+
+    self.assertEquals(root, co.project_path)
+    self.assertEquals(self.previous_log['revision'], co.prepare(None))
+    self.assertEquals('pouet', co.get_settings('bar'))
+    self.assertTree(self.get_trunk(False), root)
+    patches = self.get_patches()
+    co.apply_patch(patches)
+    self.assertEquals(
+        ['bin_file', 'chrome/file.cc', 'new_dir/subdir/new_file', 'extra'],
+        patches.filenames)
+
+    # Verify that the patch is applied even for read only checkout.
+    self.assertTree(self.get_trunk(True), root)
+    fake_author = self.FAKE_REPOS.USERS[1][0]
+    revision = co.commit(u'msg', fake_author)
+    # Nothing changed.
+    self.assertTree(self.get_trunk(True), root)
+
+    if read_only:
+      self.assertEquals('FAKE', revision)
+      self.assertEquals(self.previous_log['revision'], co.prepare(None))
+      # Changes should be reverted now.
+      self.assertTree(self.get_trunk(False), root)
+      expected = self.previous_log
+    else:
+      self.assertEquals(self.previous_log['revision'] + 1, revision)
+      self.assertEquals(self.previous_log['revision'] + 1, co.prepare(None))
+      self.assertTree(self.get_trunk(True), root)
+      expected = expected.copy()
+      expected['msg'] = 'msg'
+      expected['revision'] = self.previous_log['revision'] + 1
+      expected.setdefault('author', fake_author)
+
+    actual = self._log()
+    self.assertEquals(expected, actual)
+
   def _test_prepare(self, co):
     self.assertEquals(1, co.prepare(1))
 
+  def get_trunk(self, modified):
+    tree = {}
+    subroot = 'trunk/'
+    for k, v in self.FAKE_REPOS.svn_revs[-1].iteritems():
+      if k.startswith(subroot):
+        f = k[len(subroot):]
+        assert f not in tree
+        tree[f] = v
+
+    if modified:
+      content_lines = tree['chrome/file.cc'].splitlines(True)
+      tree['chrome/file.cc'] = ''.join(
+          content_lines[0:5] + ['FOO!\n'] + content_lines[5:])
+      del tree['extra']
+      tree['new_dir/subdir/new_file'] = 'A new file\nshould exist.\n'
+    return tree
+
+
+class GitBaseTest(BaseTest):
+  def setUp(self):
+    super(GitBaseTest, self).setUp()
+    self.enabled = self.FAKE_REPOS.set_up_git()
+    self.assertTrue(self.enabled)
+    self.previous_log = self._log()
+
+  # pylint: disable=W0221
+  def _log(self, log_from_local_repo=False):
+    if log_from_local_repo:
+      repo_root = os.path.join(self.root_dir, self.name)
+    else:
+      repo_root = os.path.join(self.FAKE_REPOS.git_root,
+                               self.FAKE_REPOS.TEST_GIT_REPO)
+    out = subprocess2.check_output(
+        ['git',
+         '--git-dir',
+         os.path.join(repo_root, '.git'),
+         'log', '--pretty=format:"%H%x09%ae%x09%ad%x09%s"',
+         '--max-count=1']).strip('"')
+    if out and len(out.split()) != 0:
+      revision = out.split()[0]
+    else:
+      return {'revision': 0}
+
+    return {
+        'revision': revision,
+        'author': out.split()[1],
+        'msg': out.split()[-1],
+    }
+
+  def _check_base(self, co, root, expected):
+    read_only = isinstance(co, checkout.ReadOnlyCheckout)
+    self.assertEquals(read_only, self.is_read_only)
+    if not read_only:
+      self.FAKE_REPOS.git_dirty = True
+
+    self.assertEquals(root, co.project_path)
+    self.assertEquals(self.previous_log['revision'], co.prepare(None))
+    self.assertEquals('pouet', co.get_settings('bar'))
+    self.assertTree(self.get_trunk(False), root)
+    patches = self.get_patches()
+    co.apply_patch(patches)
+    self.assertEquals(
+        ['bin_file', 'chrome/file.cc', 'new_dir/subdir/new_file', 'extra'],
+        patches.filenames)
+
+    # Hackish to verify _branches() internal function.
+    # pylint: disable=W0212
+    self.assertEquals(
+        (['master', 'working_branch'], 'working_branch'),
+        co._branches())
+
+    # Verify that the patch is applied even for read only checkout.
+    self.assertTree(self.get_trunk(True), root)
+    fake_author = self.FAKE_REPOS.USERS[1][0]
+    revision = co.commit(u'msg', fake_author)
+    # Nothing changed.
+    self.assertTree(self.get_trunk(True), root)
+
+    if read_only:
+      self.assertEquals('FAKE', revision)
+      self.assertEquals(self.previous_log['revision'], co.prepare(None))
+      # Changes should be reverted now.
+      self.assertTree(self.get_trunk(False), root)
+      expected = self.previous_log
+    else:
+      self.assertEquals(self._log()['revision'], revision)
+      self.assertEquals(self._log()['revision'], co.prepare(None))
+      self.assertTree(self.get_trunk(True), root)
+      expected = self._log()
+
+    actual = self._log(log_from_local_repo=True)
+    self.assertEquals(expected, actual)
+
+  def get_trunk(self, modified):
+    tree = {}
+    for k, v in self.FAKE_REPOS.git_hashes[
+        self.FAKE_REPOS.TEST_GIT_REPO][1][1].iteritems():
+      assert k not in tree
+      tree[k] = v 
+
+    if modified:
+      content_lines = tree['chrome/file.cc'].splitlines(True)
+      tree['chrome/file.cc'] = ''.join(
+          content_lines[0:5] + ['FOO!\n'] + content_lines[5:])
+      tree['bin_file'] = '\x00'
+      del tree['extra']
+      tree['new_dir/subdir/new_file'] = 'A new file\nshould exist.\n'
+    return tree
+
+  def _test_prepare(self, co):
+    print co.prepare(None)
+
+
+class GitCheckout(GitBaseTest):
+  def _get_co(self, post_processors):
+    self.assertNotEqual(False, post_processors)
+    return checkout.GitCheckout(
+      root_dir=self.root_dir,
+      project_name=self.name,
+      remote_branch='master',
+      git_url=os.path.join(self.FAKE_REPOS.git_root,
+                           self.FAKE_REPOS.TEST_GIT_REPO),
+      commit_user=self.usr,
+      post_processors=post_processors)
+
+  def testAll(self):
+    root = os.path.join(self.root_dir, self.name)
+    self._check_base(self._get_co(None), root, None)
+
+  def testException(self):
+    self._check_exception(
+        self._get_co(None),
+        'While running git apply --index -p1;\n  fatal: corrupt patch at line '
+        '12\n')
+
+  def testProcess(self):
+    self._test_process(self._get_co)
+
+  def _testPrepare(self):
+    self._test_prepare(self._get_co(None))
+
+  def testMove(self):
+    co = self._get_co(None)
+    self._check_move(co)
+    out = subprocess2.check_output(
+        ['git', 'diff', 'HEAD~', '--name-status'], cwd=co.project_path)
+    out = sorted(out.splitlines())
+    expected = sorted(
+      [
+        'A\tchromeos/views/webui_menu_widget.h',
+        'D\tchromeos/views/DOMui_menu_widget.h',
+      ])
+    self.assertEquals(expected, out)
+
 
 class SvnCheckout(SvnBaseTest):
   def _get_co(self, post_processors):
@@ -302,7 +489,7 @@ class SvnCheckout(SvnBaseTest):
         'revprops': [('realauthor', self.FAKE_REPOS.USERS[1][0])]
     }
     root = os.path.join(self.root_dir, self.name)
-    self._check_base(self._get_co(None), root, False, expected)
+    self._check_base(self._get_co(None), root, expected)
 
   def testException(self):
     self._check_exception(
@@ -357,7 +544,7 @@ class SvnCheckout(SvnBaseTest):
         'revprops': [('commit-bot', 'user1@example.com')],
     }
     root = os.path.join(self.root_dir, self.name)
-    self._check_base(self._get_co(None), root, False, expected)
+    self._check_base(self._get_co(None), root, expected)
 
   def testWithRevPropsSupportNotCommitBot(self):
     # Add the hook that will commit in a way that removes the race condition.
@@ -372,7 +559,7 @@ class SvnCheckout(SvnBaseTest):
     expected = {
         'author': self.FAKE_REPOS.USERS[1][0],
     }
-    self._check_base(co, root, False, expected)
+    self._check_base(co, root, expected)
 
   def testAutoProps(self):
     co = self._get_co(None)
@@ -512,7 +699,7 @@ class ReadOnlyCheckout(SvnBaseTest):
 
   def testAll(self):
     root = os.path.join(self.root_dir, self.name)
-    self._check_base(self._get_co(None), root, False, None)
+    self._check_base(self._get_co(None), root, None)
 
   def testException(self):
     self._check_exception(