diff --git a/apply_issue.py b/apply_issue.py index d98e9ba59d..5b1807266e 100755 --- a/apply_issue.py +++ b/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: diff --git a/checkout.py b/checkout.py index d1a694b453..3d15e50654 100644 --- a/checkout.py +++ b/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 without a colon is equivalent to : when + # pulling/fetching, so it merges 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 " 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) diff --git a/tests/checkout_test.py b/tests/checkout_test.py index 46617dafe6..57c9a5d5c4 100755 --- a/tests/checkout_test.py +++ b/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 \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(