diff --git a/git_cl.py b/git_cl.py index b67200570..886baee85 100755 --- a/git_cl.py +++ b/git_cl.py @@ -964,6 +964,7 @@ def _is_git_numberer_enabled(remote_url, remote_ref): 'chromium/src', 'external/webrtc', 'v8/v8', + 'infra/experimental', ] assert remote_ref and remote_ref.startswith('refs/'), remote_ref @@ -4687,6 +4688,11 @@ def CMDdcommit(parser, args): return 1 +# Two special branches used by git cl land. +MERGE_BRANCH = 'git-cl-commit' +CHERRY_PICK_BRANCH = 'git-cl-cherry-pick' + + @subcommand.usage('[upstream branch to apply against]') def CMDland(parser, args): """Commits the current changelist via git. @@ -4840,9 +4846,7 @@ def CMDland(parser, args): # 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. - MERGE_BRANCH = 'git-cl-commit' - CHERRY_PICK_BRANCH = 'git-cl-cherry-pick' - # Delete the branches if they exist. + # 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) @@ -4883,24 +4887,13 @@ def CMDland(parser, args): git_numberer_enabled = _is_git_numberer_enabled( RunGit(['config', 'remote.%s.url' % remote]).strip(), branch) - if git_numberer_enabled: - # TODO(tandrii): maybe do autorebase + retry on failure - # http://crbug.com/682934, but better just use Gerrit :) - logging.debug('Adding git number footers') - parent_msg = RunGit(['show', '-s', '--format=%B', merge_base]).strip() - commit_desc.update_with_git_number_footers(merge_base, parent_msg, - branch) - # Ensure timestamps are monotonically increasing. - timestamp = max(1 + _get_committer_timestamp(merge_base), - _get_committer_timestamp('HEAD')) - _git_amend_head(commit_desc.description, timestamp) - change_desc = ChangeDescription(commit_desc.description) - - retcode, output = RunGitWithCode( - ['push', '--porcelain', pushurl, 'HEAD:%s' % branch]) + retcode = PushToGitWithAutoRebase( + pushurl, branch, commit_desc.description, git_numberer_enabled) if retcode == 0: revision = RunGit(['rev-parse', 'HEAD']).strip() - logging.debug(output) + 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' @@ -4911,6 +4904,7 @@ def CMDland(parser, args): # 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.') @@ -4943,6 +4937,75 @@ def CMDland(parser, args): return 0 +def PushToGitWithAutoRebase(remote, branch, original_description, + git_numberer_enabled, max_attempts=3): + """Pushes current HEAD commit on top of remote's branch. + + Attempts to fetch and autorebase on push failures. + Adds git number footers on the fly. + + Returns integer code from last command. + """ + cherry = RunGit(['rev-parse', 'HEAD']).strip() + code = 0 + attempts_left = max_attempts + while attempts_left: + attempts_left -= 1 + print('Attempt %d of %d' % (max_attempts - attempts_left, max_attempts)) + + # Fetch remote/branch into local cherry_pick_branch, overriding the latter. + # If fetch fails, retry. + print('Fetching %s/%s...' % (remote, branch)) + code, out = RunGitWithCode( + ['retry', 'fetch', remote, + '+%s:refs/heads/%s' % (branch, CHERRY_PICK_BRANCH)]) + if code: + print('Fetch failed with exit code %d.' % code) + print(out.strip()) + continue + + print('Cherry-picking commit on top of latest %s' % branch) + RunGitWithCode(['checkout', 'refs/heads/%s' % CHERRY_PICK_BRANCH], + suppress_stderr=True) + parent_hash = RunGit(['rev-parse', 'HEAD']).strip() + code, out = RunGitWithCode(['cherry-pick', cherry]) + if code: + print('Your patch doesn\'t apply cleanly to \'%s\' HEAD @ %s, ' + 'the following files have merge conflicts:' % + (branch, parent_hash)) + print(RunGit(['diff', '--name-status', '--diff-filter=U']).strip()) + print('Please rebase your patch and try again.') + RunGitWithCode(['cherry-pick', '--abort']) + break + + commit_desc = ChangeDescription(original_description) + if git_numberer_enabled: + logging.debug('Adding git number footers') + parent_msg = RunGit(['show', '-s', '--format=%B', parent_hash]).strip() + commit_desc.update_with_git_number_footers(parent_hash, parent_msg, + branch) + # Ensure timestamps are monotonically increasing. + timestamp = max(1 + _get_committer_timestamp(parent_hash), + _get_committer_timestamp('HEAD')) + _git_amend_head(commit_desc.description, timestamp) + + code, out = RunGitWithCode( + ['push', '--porcelain', remote, 'HEAD:%s' % branch]) + print(out) + if code == 0: + break + if IsFatalPushFailure(out): + print('Fatal push error. Make sure your .netrc credentials and git ' + 'user.email are correct and you have push access to the repo.') + break + return code + + +def IsFatalPushFailure(push_stdout): + """True if retrying push won't help.""" + return '(prohibited by Gerrit)' in push_stdout + + @subcommand.usage('') def CMDpatch(parser, args): """Patches in a code review.""" diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 74099d4b7..068729d14 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -957,7 +957,13 @@ class TestGitCl(TestCase): 'desc\n\nBUG=500658\nBUG=proj:1234', []) - def _land_rietveld_common(self, debug=False): + 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. @@ -965,6 +971,14 @@ class TestGitCl(TestCase): 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( @@ -1023,76 +1037,126 @@ class TestGitCl(TestCase): ((['git', 'config', 'branch.feature.merge'],), 'refs/heads/master'), ((['git', 'config', 'branch.feature.remote'],), 'origin'), ((['git', 'config', '--get', 'remote.origin.url'],), - 'https://chromium.googlesource.com/infra/infra'), + 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. - def test_land_rietveld(self): - self._land_rietveld_common(debug=False) self.calls += [ - ((['git', 'config', 'remote.origin.url'],), - 'https://chromium.googlesource.com/infra/infra'), - (('_is_git_numberer_enabled', - 'https://chromium.googlesource.com/infra/infra', - 'refs/heads/master'), - False), - ((['git', 'push', '--porcelain', 'origin', 'HEAD:refs/heads/master'],), - ''), - ((['git', 'rev-parse', 'HEAD'],), 'fake_sha_rebased'), + ((['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/+/fake_sha_rebased'],), + 'https://chromium.googlesource.com/infra/infra/+/sha1_committed'],), ''), ((['add_comment', 123, 'Committed patchset #2 (id:20001) manually as ' - 'fake_sha_rebased (presubmit successful).'],), ''), + 'sha1_committed (presubmit successful).'],), ''), ] git_cl.main(['land']) - def test_land_rietveld_git_numberer(self): - self._land_rietveld_common(debug=False) - - # 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) - + 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', 'config', 'remote.origin.url'],), - 'https://chromium.googlesource.com/chromium/src'), - (('_is_git_numberer_enabled', - 'https://chromium.googlesource.com/chromium/src', - 'refs/heads/master'), - True), - ((['git', 'show', '-s', '--format=%B', 'fake_ancestor_sha'],), - 'This is parent commit.\n' - '\n' - 'Cr-Commit-Position: refs/heads/master@{#543}\n' - 'Cr-Branched-From: refs/svn/2014@{#2208}'), - ((['git', 'show', '-s', '--format=%ct', 'fake_ancestor_sha'],), - '1480022355'), # Committer's unix timestamp. - ((['git', 'show', '-s', '--format=%ct', 'HEAD'],), - '1480024000'), - - ((['git_amend_head committer timestamp', 1480024000],), None), - ((['git', 'commit', '--amend', '-m', - '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}'],), ''), - - ((['git', 'push', '--porcelain', 'origin', 'HEAD:refs/heads/master'],), - ''), - ((['git', 'rev-parse', 'HEAD'],), 'fake_sha_rebased'), + ((['git', '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/infra/infra/+/'), + 'https://chromium.googlesource.com/chromium/src/+/'), ((['update_description', 123, 'Issue: 123\n\nR=john@chromium.org\n' '\n' @@ -1100,27 +1164,25 @@ class TestGitCl(TestCase): 'Cr-Commit-Position: refs/heads/master@{#544}\n' 'Cr-Branched-From: refs/svn/2014@{#2208}\n' 'Committed: ' - 'https://chromium.googlesource.com/infra/infra/+/fake_sha_rebased'],), + 'https://chromium.googlesource.com/chromium/src/+/sha1_committed'],), ''), ((['add_comment', 123, 'Committed patchset #2 (id:20001) manually as ' - 'fake_sha_rebased (presubmit successful).'],), ''), + 'sha1_committed (presubmit successful).'],), ''), ] git_cl.main(['land']) def test_land_rietveld_git_numberer_bad_parent(self): - self._land_rietveld_common(debug=False) + 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 += [ - ((['git', 'config', 'remote.origin.url'],), - 'https://chromium.googlesource.com/v8/v8'), - (('_is_git_numberer_enabled', - 'https://chromium.googlesource.com/v8/v8', 'refs/heads/master'), - True), - - ((['git', 'show', '-s', '--format=%B', 'fake_ancestor_sha'],), - 'This is parent commit with no footer.'), - + # 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'])