From 2e73d43870dd9a39c62914e385d72872ad743f71 Mon Sep 17 00:00:00 2001 From: Robert Iannucci Date: Wed, 14 Mar 2018 01:10:47 -0700 Subject: [PATCH] [git_cl] Remove rietveld CMDland implementation. R=agable@chromium.org, tandrii@chromium.org Change-Id: I2bfed54a9a1bde3335340e5949eac313c3fdda32 Reviewed-on: https://chromium-review.googlesource.com/961970 Commit-Queue: Robbie Iannucci Reviewed-by: Andrii Shyshkalov --- git_cl.py | 233 ++++--------------------------------------- tests/git_cl_test.py | 228 ------------------------------------------ 2 files changed, 21 insertions(+), 440 deletions(-) diff --git a/git_cl.py b/git_cl.py index 52c9ddffb..2a302a2f1 100755 --- a/git_cl.py +++ b/git_cl.py @@ -5079,219 +5079,28 @@ def CMDland(parser, args): cl = Changelist(auth_config=auth_config) - # TODO(tandrii): refactor this into _RietveldChangelistImpl method. - if cl.IsGerrit(): - if options.message: - # This could be implemented, but it requires sending a new patch to - # Gerrit, as Gerrit unlike Rietveld versions messages with patchsets. - # Besides, Gerrit has the ability to change the commit message on submit - # automatically, thus there is no need to support this option (so far?). - parser.error('-m MESSAGE option is not supported for Gerrit.') - if options.contributor: - parser.error( - '-c CONTRIBUTOR option is not supported for Gerrit.\n' - 'Before uploading a commit to Gerrit, ensure it\'s author field is ' - 'the contributor\'s "name ". If you can\'t upload such a ' - 'commit for review, contact your repository admin and request' - '"Forge-Author" permission.') - if not cl.GetIssue(): - DieWithError('You must upload the change first to Gerrit.\n' - ' If you would rather have `git cl land` upload ' - 'automatically for you, see http://crbug.com/642759') - return cl._codereview_impl.CMDLand(options.force, options.bypass_hooks, - options.verbose) - - current = cl.GetBranch() - remote, upstream_branch = cl.FetchUpstreamTuple(cl.GetBranch()) - if remote == '.': - print() - print('Attempting to push branch %r into another local branch!' % current) - print() - print('Either reparent this branch on top of origin/master:') - print(' git reparent-branch --root') - print() - print('OR run `git rebase-update` if you think the parent branch is ') - print('already committed.') - print() - print(' Current parent: %r' % upstream_branch) - return 1 - - if not args: - # Default to merging against our best guess of the upstream branch. - args = [cl.GetUpstreamBranch()] - - if options.contributor: - if not re.match('^.*\s<\S+@\S+>$', options.contributor): - print("Please provide contributor as 'First Last '") - return 1 - - base_branch = args[0] - - if git_common.is_dirty_git_tree('land'): - return 1 - - # This rev-list syntax means "show all commits not in my branch that - # are in base_branch". - upstream_commits = RunGit(['rev-list', '^' + cl.GetBranchRef(), - base_branch]).splitlines() - if upstream_commits: - print('Base branch "%s" has %d commits ' - 'not in this branch.' % (base_branch, len(upstream_commits))) - print('Run "git merge %s" before attempting to land.' % base_branch) - return 1 - - merge_base = RunGit(['merge-base', base_branch, 'HEAD']).strip() - if not options.bypass_hooks: - author = None - if options.contributor: - author = re.search(r'\<(.*)\>', options.contributor).group(1) - hook_results = cl.RunHook( - committing=True, - may_prompt=not options.force, - verbose=options.verbose, - change=cl.GetChange(merge_base, author)) - if not hook_results.should_continue(): - return 1 - - # Check the tree status if the tree status URL is set. - status = GetTreeStatus() - if 'closed' == status: - print('The tree is closed. Please wait for it to reopen. Use ' - '"git cl land --bypass-hooks" to commit on a closed tree.') - return 1 - elif 'unknown' == status: - print('Unable to determine tree status. Please verify manually and ' - 'use "git cl land --bypass-hooks" to commit on a closed tree.') - return 1 - - change_desc = ChangeDescription(options.message) - if not change_desc.description and cl.GetIssue(): - change_desc = ChangeDescription(cl.GetDescription()) - - if not change_desc.description: - if not cl.GetIssue() and options.bypass_hooks: - change_desc = ChangeDescription(CreateDescriptionFromLog([merge_base])) - else: - print('No description set.') - print('Visit %s/edit to set it.' % (cl.GetIssueURL())) - return 1 - - # Keep a separate copy for the commit message, because the commit message - # contains the link to the Rietveld issue, while the Rietveld message contains - # the commit viewvc url. - if cl.GetIssue(): - change_desc.update_reviewers( - get_approving_reviewers(cl.GetIssueProperties()), []) - - commit_desc = ChangeDescription(change_desc.description) - if cl.GetIssue(): - # Xcode won't linkify this URL unless there is a non-whitespace character - # after it. Add a period on a new line to circumvent this. Also add a space - # before the period to make sure that Gitiles continues to correctly resolve - # the URL. - commit_desc.append_footer('Review-Url: %s .' % cl.GetIssueURL()) + if not cl.IsGerrit(): + parser.error('rietveld is not supported') + + if options.message: + # This could be implemented, but it requires sending a new patch to + # Gerrit, as Gerrit unlike Rietveld versions messages with patchsets. + # Besides, Gerrit has the ability to change the commit message on submit + # automatically, thus there is no need to support this option (so far?). + parser.error('-m MESSAGE option is not supported for Gerrit.') if options.contributor: - commit_desc.append_footer('Patch from %s.' % options.contributor) - - print('Description:') - print(commit_desc.description) - - branches = [merge_base, cl.GetBranchRef()] - if not options.force: - print_stats(branches) - - # We want to squash all this branch's commits into one commit with the proper - # description. We do this by doing a "reset --soft" to the base branch (which - # keeps the working copy the same), then landing that. - # Delete the special branches if they exist. - for branch in [MERGE_BRANCH, CHERRY_PICK_BRANCH]: - showref_cmd = ['show-ref', '--quiet', '--verify', 'refs/heads/%s' % branch] - result = RunGitWithCode(showref_cmd) - if result[0] == 0: - RunGit(['branch', '-D', branch]) - - # We might be in a directory that's present in this branch but not in the - # trunk. Move up to the top of the tree so that git commands that expect a - # valid CWD won't fail after we check out the merge branch. - rel_base_path = settings.GetRelativeRoot() - if rel_base_path: - os.chdir(rel_base_path) - - # Stuff our change into the merge branch. - # We wrap in a try...finally block so if anything goes wrong, - # we clean up the branches. - retcode = -1 - revision = None - try: - RunGit(['checkout', '-q', '-b', MERGE_BRANCH]) - RunGit(['reset', '--soft', merge_base]) - if options.contributor: - RunGit( - [ - 'commit', '--author', options.contributor, - '-m', commit_desc.description, - ]) - else: - RunGit(['commit', '-m', commit_desc.description]) - - remote, branch = cl.FetchUpstreamTuple(cl.GetBranch()) - mirror = settings.GetGitMirror(remote) - if mirror: - pushurl = mirror.url - git_numberer_enabled = _is_git_numberer_enabled(pushurl, branch) - else: - pushurl = remote # Usually, this is 'origin'. - git_numberer_enabled = _is_git_numberer_enabled( - RunGit(['config', 'remote.%s.url' % remote]).strip(), branch) - - retcode = PushToGitWithAutoRebase( - pushurl, branch, commit_desc.description, git_numberer_enabled) - if retcode == 0: - revision = RunGit(['rev-parse', 'HEAD']).strip() - if git_numberer_enabled: - change_desc = ChangeDescription( - RunGit(['show', '-s', '--format=%B', 'HEAD']).strip()) - except: # pylint: disable=bare-except - if _IS_BEING_TESTED: - logging.exception('this is likely your ACTUAL cause of test failure.\n' - + '-' * 30 + '8<' + '-' * 30) - logging.error('\n' + '-' * 30 + '8<' + '-' * 30 + '\n\n\n') - raise - finally: - # And then swap back to the original branch and clean up. - RunGit(['checkout', '-q', cl.GetBranch()]) - RunGit(['branch', '-D', MERGE_BRANCH]) - RunGit(['branch', '-D', CHERRY_PICK_BRANCH], error_ok=True) - - if not revision: - print('Failed to push. If this persists, please file a bug.') - return 1 - - if cl.GetIssue(): - viewvc_url = settings.GetViewVCUrl() - if viewvc_url and revision: - change_desc.append_footer( - 'Committed: %s%s' % (viewvc_url, revision)) - elif revision: - change_desc.append_footer('Committed: %s' % (revision,)) - print('Closing issue ' - '(you may be prompted for your codereview password)...') - cl.UpdateDescription(change_desc.description) - cl.CloseIssue() - props = cl.GetIssueProperties() - patch_num = len(props['patchsets']) - comment = "Committed patchset #%d (id:%d) manually as %s" % ( - patch_num, props['patchsets'][-1], revision) - if options.bypass_hooks: - comment += ' (tree was closed).' if GetTreeStatus() == 'closed' else '.' - else: - comment += ' (presubmit successful).' - cl.RpcServer().add_comment(cl.GetIssue(), comment) - - if os.path.isfile(POSTUPSTREAM_HOOK): - RunCommand([POSTUPSTREAM_HOOK, merge_base], error_ok=True) - - return 0 + parser.error( + '-c CONTRIBUTOR option is not supported for Gerrit.\n' + 'Before uploading a commit to Gerrit, ensure it\'s author field is ' + 'the contributor\'s "name ". If you can\'t upload such a ' + 'commit for review, contact your repository admin and request' + '"Forge-Author" permission.') + if not cl.GetIssue(): + DieWithError('You must upload the change first to Gerrit.\n' + ' If you would rather have `git cl land` upload ' + 'automatically for you, see http://crbug.com/642759') + return cl._codereview_impl.CMDLand(options.force, options.bypass_hooks, + options.verbose) def PushToGitWithAutoRebase(remote, branch, original_description, diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 78e64feb2..ee6fe3b42 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -1003,234 +1003,6 @@ class TestGitCl(TestCase): 'desc\n\nBUG=500658\nBUG=proj:1234', []) - def _land_rietveld_calls(self, repo_url, git_numberer_enabled=False, - parent_msg=None, - new_msg=None, - fail_after_parent_msg=False, - fail_cherry_pick=False, - fail_push_count=0, - debug=False): - if debug: - # Very useful due to finally clause in git cl land raising exceptions and - # shadowing real cause of failure. - self.mock(git_cl, '_IS_BEING_TESTED', True) - else: - self.mock(git_cl.sys, 'stdout', StringIO.StringIO()) - - if git_numberer_enabled: - # Special mocks to check validity of timestamp. - original_git_amend_head = git_cl._git_amend_head - def _git_amend_head_mock(msg, tstamp): - self._mocked_call(['git_amend_head committer timestamp', tstamp]) - return original_git_amend_head(msg, tstamp) - self.mock(git_cl, '_git_amend_head', _git_amend_head_mock) - - self.mock(git_cl, '_is_git_numberer_enabled', lambda url, ref: - self._mocked_call('_is_git_numberer_enabled', url, ref)) - self.mock(RietveldMock, 'update_description', staticmethod( - lambda i, d: self._mocked_call(['update_description', i, d]))) - self.mock(RietveldMock, 'add_comment', staticmethod( - lambda i, c: self._mocked_call(['add_comment', i, c]))) - self.calls = [ - ((['git', 'symbolic-ref', 'HEAD'],), 'feature'), - ((['git', 'config', 'branch.feature.rietveldissue'],), '123'), - ((['git', 'config', 'rietveld.autoupdate'],), ''), - ((['git', 'config', 'rietveld.server'],), - 'https://codereview.chromium.org'), - ((['git', 'config', 'branch.feature.merge'],), 'refs/heads/master'), - ((['git', 'config', 'branch.feature.remote'],), 'origin'), - ((['git', 'config', 'branch.feature.merge'],), 'refs/heads/master'), - ((['git', 'config', 'branch.feature.remote'],), 'origin'), - ((['git', 'rev-list', '^feature', 'refs/remotes/origin/master'],), - ''), # No commits to rebase, according to local view of origin. - ((['git', 'merge-base', 'refs/remotes/origin/master', 'HEAD'],), - 'fake_ancestor_sha'), - ] + self._git_sanity_checks('fake_ancestor_sha', 'feature') + [ - ((['git', 'rev-parse', '--show-cdup'],), ''), - ((['git', 'rev-parse', 'HEAD'],), 'fake_sha'), - ((['git', '-c', 'core.quotePath=false', 'diff', - '--name-status', '--no-renames', '-r', 'fake_ancestor_sha...', '.'],), - 'M\tfile1.cpp'), - ((['git', 'config', 'branch.feature.rietveldpatchset'],), '20001'), - ((['git', 'config', 'branch.feature.rietveldserver'],), - 'https://codereview.chromium.org'), - ((['git', 'config', 'user.email'],), 'user@e.mail'), - ((['git', 'config', 'rietveld.tree-status-url'],), CERR1), - ((['git', 'diff', '--no-ext-diff', '--stat', '-l100000', '-C50', - 'fake_ancestor_sha', 'feature'],), - # This command just prints something like this: - # file1.cpp | 53 ++++++-- - # 1 file changed, 33 insertions(+), 20 deletions(-)\n - ''), - ((['git', 'show-ref', '--quiet', '--verify', - 'refs/heads/git-cl-commit'],), - ''), # 0 return code means branch exists. - ((['git', 'branch', '-D', 'git-cl-commit'],), ''), - ((['git', 'show-ref', '--quiet', '--verify', - 'refs/heads/git-cl-cherry-pick'],), - CERR1), # This means git-cl-cherry-pick branch does not exist. - ((['git', 'rev-parse', '--show-cdup'],), ''), - ((['git', 'checkout', '-q', '-b', 'git-cl-commit'],), ''), - ((['git', 'reset', '--soft', 'fake_ancestor_sha'],), ''), - ((['git', 'commit', '-m', - 'Issue: 123\n\nR=john@chromium.org\n\n' - 'Review-Url: https://codereview.chromium.org/123 .'],), ''), - ((['git', 'config', 'branch.feature.merge'],), 'refs/heads/master'), - ((['git', 'config', 'branch.feature.remote'],), 'origin'), - ((['git', 'config', '--get', 'remote.origin.url'],), - repo_url), - - ((['git', 'config', 'remote.origin.url'],), repo_url), - (('_is_git_numberer_enabled', repo_url, 'refs/heads/master'), - git_numberer_enabled), - - # Auto-rebase init. - ((['git', 'rev-parse', 'HEAD'],), 'sha1_for_cherry_pick'), - ] - # Auto-rebase loops. - for attempt in xrange(fail_push_count + 1): - self.calls += [ - # Auto-rebase loop start. - ((['git', 'retry', 'fetch', 'origin', - '+refs/heads/master:refs/heads/git-cl-cherry-pick'],), ''), - ((['git', 'checkout', 'refs/heads/git-cl-cherry-pick'],), ''), - # NOTE: if fail_push_count>0, this sha1_of_parent should probably be - # different in each loop iteration for truly realistic test. - ((['git', 'rev-parse', 'HEAD'],), 'sha1_of_parent'), - ((['git', 'cherry-pick', 'sha1_for_cherry_pick'],), - callError(1, 'failed to cherry pick') if fail_cherry_pick - else ''), - ] - if fail_cherry_pick: - return - - if git_numberer_enabled: - assert parent_msg - self.calls += [ - ((['git', 'show', '-s', '--format=%B', 'sha1_of_parent'],), - parent_msg), - ] - if fail_after_parent_msg: - return - assert new_msg - self.calls += [ - ((['git', 'show', '-s', '--format=%ct', 'sha1_of_parent'],), - '1480022355'), # Committer's unix timestamp. - ((['git', 'show', '-s', '--format=%ct', 'HEAD'],), - '1480024000'), - ((['git_amend_head committer timestamp', 1480024000],), ''), - ((['git', 'commit', '--amend', '-m', new_msg],), ''), - ] - self.calls += [ - ((['git', 'push', '--porcelain', 'origin', 'HEAD:refs/heads/master'],), - CERR1 if attempt < fail_push_count else ''), - ] - # End of autorebase + push. - - self.calls += [ - ((['git', 'rev-parse', 'HEAD'],), 'sha1_committed'), - ] - if git_numberer_enabled: - self.calls += [ - ((['git', 'show', '-s', '--format=%B', 'HEAD'],), new_msg), - ] - - # Cleanup calls. - self.calls += [ - ((['git', 'checkout', '-q', 'feature'],), ''), - ((['git', 'branch', '-D', 'git-cl-commit'],), ''), - ((['git', 'branch', '-D', 'git-cl-cherry-pick'],), ''), - ] - - def test_land_rietveld(self): - self._land_rietveld_calls( - repo_url='https://chromium.googlesource.com/infra/infra', - git_numberer_enabled=False, - debug=False) - self.calls += [ - ((['git', 'config', 'rietveld.viewvc-url'],), - 'https://chromium.googlesource.com/infra/infra/+/'), - ((['update_description', 123, - 'Issue: 123\n\nR=john@chromium.org\n\nCommitted: ' - 'https://chromium.googlesource.com/infra/infra/+/sha1_committed'],), - ''), - ((['add_comment', 123, 'Committed patchset #2 (id:20001) manually as ' - 'sha1_committed (presubmit successful).'],), ''), - ] - git_cl.main(['land']) - - def test_land_rietveld_fail_cherry_pick(self): - self._land_rietveld_calls( - repo_url='https://chromium.googlesource.com/infra/infra', - git_numberer_enabled=False, - fail_cherry_pick=True, - debug=False) - self.calls += [ - ((['git', '-c', 'core.quotePath=false', 'diff', - '--name-status', '--diff-filter=U'],), - 'U path/file1\n' - 'U file2.cpp\n'), - ((['git', 'cherry-pick', '--abort'],), ''), - ((['git', 'checkout', '-q', 'feature'],), ''), - ((['git', 'branch', '-D', 'git-cl-commit'],), ''), - ((['git', 'branch', '-D', 'git-cl-cherry-pick'],), ''), - ] - git_cl.main(['land']) - - def test_land_rietveld_git_numberer_fail_1_push(self): - return self.test_land_rietveld_git_numberer(fail_push_count=1) - - def test_land_rietveld_git_numberer(self, fail_push_count=0): - self._land_rietveld_calls( - repo_url='https://chromium.googlesource.com/chromium/src', - git_numberer_enabled=True, - parent_msg=('This is parent commit.\n' - '\n' - 'Cr-Commit-Position: refs/heads/master@{#543}\n' - 'Cr-Branched-From: refs/svn/2014@{#2208}'), - new_msg=('Issue: 123\n\nR=john@chromium.org\n' - '\n' - 'Review-Url: https://codereview.chromium.org/123 .\n' - 'Cr-Commit-Position: refs/heads/master@{#544}\n' - 'Cr-Branched-From: refs/svn/2014@{#2208}'), - fail_push_count=fail_push_count, - debug=False) - - self.calls += [ - ((['git', 'config', 'rietveld.viewvc-url'],), - 'https://chromium.googlesource.com/chromium/src/+/'), - ((['update_description', 123, - 'Issue: 123\n\nR=john@chromium.org\n' - '\n' - 'Review-Url: https://codereview.chromium.org/123 .\n' - 'Cr-Commit-Position: refs/heads/master@{#544}\n' - 'Cr-Branched-From: refs/svn/2014@{#2208}\n' - 'Committed: ' - 'https://chromium.googlesource.com/chromium/src/+/sha1_committed'],), - ''), - ((['add_comment', 123, 'Committed patchset #2 (id:20001) manually as ' - 'sha1_committed (presubmit successful).'],), ''), - ] - git_cl.main(['land']) - - def test_land_rietveld_git_numberer_bad_parent(self): - self._land_rietveld_calls( - repo_url='https://chromium.googlesource.com/v8/v8', - git_numberer_enabled=True, - parent_msg='This is parent commit with no footer.', - fail_after_parent_msg=True, - debug=False) - self.calls += [ - # Cleanup calls to restore original state. - ((['git', 'checkout', '-q', 'feature'],), ''), - ((['git', 'branch', '-D', 'git-cl-commit'],), ''), - ((['git', 'branch', '-D', 'git-cl-cherry-pick'],), ''), - ] - with self.assertRaises(ValueError) as cm: - git_cl.main(['land']) - self.assertEqual(cm.exception.message, - 'Unable to infer commit position from footers') - def test_git_numberer_not_whitelisted_repo(self): self.calls = [] res = git_cl._is_git_numberer_enabled(