From f736cabba58b374aeb96d7512813a39f30567b5a Mon Sep 17 00:00:00 2001 From: Josip Sokcevic Date: Tue, 20 Oct 2020 23:41:38 +0000 Subject: [PATCH] Add retry on git push failure This adds retry if target is old default branch. This may happen only if user didn't fetch remote for extended period of time and old default branch no longer exists. R=ehmaldonado@chromium.org Change-Id: I81981049ee006f68a073718180b454c230d055e9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2473757 Commit-Queue: Josip Sokcevic Reviewed-by: Edward Lesmes --- gerrit_util.py | 6 ++++ git_cl.py | 77 ++++++++++++++++++++++++++++++-------------- tests/git_cl_test.py | 64 ++++++++++++++++++++++++++++++++++++ 3 files changed, 123 insertions(+), 24 deletions(-) diff --git a/gerrit_util.py b/gerrit_util.py index d91312a05..aa8a39617 100644 --- a/gerrit_util.py +++ b/gerrit_util.py @@ -926,6 +926,12 @@ def GetGerritBranch(host, project, branch): raise GerritError(200, 'Unable to get gerrit branch') +def GetProjectHead(host, project): + conn = CreateHttpConn(host, + '/projects/%s/HEAD' % urllib.parse.quote(project, '')) + return ReadHttpJsonResponse(conn, accept_statuses=[200]) + + def GetAccountDetails(host, account_id='self'): """Returns details of the account. diff --git a/git_cl.py b/git_cl.py index 6f40737de..91be1474f 100755 --- a/git_cl.py +++ b/git_cl.py @@ -145,6 +145,10 @@ assert len(_KNOWN_GERRIT_TO_SHORT_URLS) == len( set(_KNOWN_GERRIT_TO_SHORT_URLS.values())), 'must have unique values' +class GitPushError(Exception): + pass + + def DieWithError(message, change_desc=None): if change_desc: SaveDescriptionBackup(change_desc) @@ -2080,8 +2084,7 @@ class Changelist(object): gclient_utils.rmtree(git_info_dir) - def _RunGitPushWithTraces( - self, change_desc, refspec, refspec_opts, git_push_metadata): + def _RunGitPushWithTraces(self, refspec, refspec_opts, git_push_metadata): """Run git push and collect the traces resulting from the execution.""" # Create a temporary directory to store traces in. Traces will be compressed # and stored in a 'traces' dir inside depot_tools. @@ -2111,20 +2114,19 @@ class Changelist(object): push_stdout = push_stdout.decode('utf-8', 'replace') except subprocess2.CalledProcessError as e: push_returncode = e.returncode - DieWithError('Failed to create a change. Please examine output above ' - 'for the reason of the failure.\n' - 'Hint: run command below to diagnose common Git/Gerrit ' - 'credential problems:\n' - ' git cl creds-check\n' - '\n' - 'If git-cl is not working correctly, file a bug under the ' - 'Infra>SDK component including the files below.\n' - 'Review the files before upload, since they might contain ' - 'sensitive information.\n' - 'Set the Restrict-View-Google label so that they are not ' - 'publicly accessible.\n' - + TRACES_MESSAGE % {'trace_name': trace_name}, - change_desc) + raise GitPushError( + 'Failed to create a change. Please examine output above for the ' + 'reason of the failure.\n' + 'Hint: run command below to diagnose common Git/Gerrit ' + 'credential problems:\n' + ' git cl creds-check\n' + '\n' + 'If git-cl is not working correctly, file a bug under the Infra>SDK ' + 'component including the files below.\n' + 'Review the files before upload, since they might contain sensitive ' + 'information.\n' + 'Set the Restrict-View-Google label so that they are not publicly ' + 'accessible.\n' + TRACES_MESSAGE % {'trace_name': trace_name}) finally: execution_time = time_time() - before_push metrics.collector.add_repeated('sub_commands', { @@ -2143,8 +2145,32 @@ class Changelist(object): return push_stdout - def CMDUploadChange( - self, options, git_diff_args, custom_cl_base, change_desc): + def CMDUploadChange(self, options, git_diff_args, custom_cl_base, + change_desc): + """Upload the current branch to Gerrit, retry if new remote HEAD is + found. options and change_desc may be mutated.""" + try: + return self._CMDUploadChange(options, git_diff_args, custom_cl_base, + change_desc) + except GitPushError as e: + remote, remote_branch = self.GetRemoteBranch() + should_retry = remote_branch == DEFAULT_OLD_BRANCH and \ + gerrit_util.GetProjectHead( + self._gerrit_host, self._GetGerritProject()) == 'refs/heads/main' + if not should_retry: + DieWithError(str(e), change_desc) + + print("WARNING: Detected HEAD change in upstream, fetching remote state") + RunGit(['fetch', remote]) + options.edit_description = False + options.force = True + try: + self._CMDUploadChange(options, git_diff_args, custom_cl_base, change_desc) + except GitPushError as e: + DieWithError(str(e), change_desc) + + def _CMDUploadChange(self, options, git_diff_args, custom_cl_base, + change_desc): """Upload the current branch to Gerrit.""" remote, remote_branch = self.GetRemoteBranch() branch = GetTargetRef(remote, remote_branch, options.target_branch) @@ -2242,10 +2268,13 @@ class Changelist(object): # TODO(tandrii): options.message should be posted as a comment # if --send-mail is set on non-initial upload as Rietveld used to do it. - title = self._GetTitleForUpload(options) - if title: + # Set options.title in case user was prompted in _GetTitleForUpload and + # _CMDUploadChange needs to be called again. + options.title = self._GetTitleForUpload(options) + if options.title: # Punctuation and whitespace in |title| must be percent-encoded. - refspec_opts.append('m=' + gerrit_util.PercentEncodeForGitRef(title)) + refspec_opts.append( + 'm=' + gerrit_util.PercentEncodeForGitRef(options.title)) if options.private: refspec_opts.append('private') @@ -2298,12 +2327,12 @@ class Changelist(object): git_push_metadata = { 'gerrit_host': self._GetGerritHost(), - 'title': title or '', + 'title': options.title or '', 'change_id': change_id, 'description': change_desc.description, } - push_stdout = self._RunGitPushWithTraces( - change_desc, refspec, refspec_opts, git_push_metadata) + push_stdout = self._RunGitPushWithTraces(refspec, refspec_opts, + git_push_metadata) if options.squash: regex = re.compile(r'remote:\s+https?://[\w\-\.\+\/#]*/(\d+)\s.*') diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index cf788f7f3..2c06c98c6 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -226,6 +226,70 @@ class TestGitClBasic(unittest.TestCase): actual = set(git_cl.get_cl_statuses(changes, True)) self.assertEqual(set(zip(changes, statuses)), actual) + def test_upload_to_non_default_branch_no_retry(self): + m = mock.patch('git_cl.Changelist._CMDUploadChange', + side_effect=[git_cl.GitPushError(), None]).start() + mock.patch('git_cl.Changelist.GetRemoteBranch', + return_value=('foo', 'bar')).start() + mock.patch('git_cl.Changelist._GetGerritProject', + return_value='foo').start() + mock.patch('git_cl.gerrit_util.GetProjectHead', + return_value='refs/heads/main').start() + + cl = git_cl.Changelist() + options = optparse.Values() + with self.assertRaises(SystemExitMock): + cl.CMDUploadChange(options, [], 'foo', git_cl.ChangeDescription('bar')) + + # ensure upload is called once + self.assertEqual(len(m.mock_calls), 1) + sys.exit.assert_called_once_with(1) + # option not set as retry didn't happen + self.assertFalse(hasattr(options, 'force')) + self.assertFalse(hasattr(options, 'edit_description')) + + def test_upload_to_old_default_still_active(self): + m = mock.patch('git_cl.Changelist._CMDUploadChange', + side_effect=[git_cl.GitPushError(), None]).start() + mock.patch('git_cl.Changelist.GetRemoteBranch', + return_value=('foo', git_cl.DEFAULT_OLD_BRANCH)).start() + mock.patch('git_cl.Changelist._GetGerritProject', + return_value='foo').start() + mock.patch('git_cl.gerrit_util.GetProjectHead', + return_value='refs/heads/old_default').start() + + cl = git_cl.Changelist() + options = optparse.Values() + with self.assertRaises(SystemExitMock): + cl.CMDUploadChange(options, [], 'foo', git_cl.ChangeDescription('bar')) + + # ensure upload is called once + self.assertEqual(len(m.mock_calls), 1) + sys.exit.assert_called_once_with(1) + # option not set as retry didn't happen + self.assertFalse(hasattr(options, 'force')) + self.assertFalse(hasattr(options, 'edit_description')) + + def test_upload_to_old_default_retry_on_failure(self): + m = mock.patch('git_cl.Changelist._CMDUploadChange', + side_effect=[git_cl.GitPushError(), None]).start() + mock.patch('git_cl.Changelist.GetRemoteBranch', + return_value=('foo', git_cl.DEFAULT_OLD_BRANCH)).start() + mock.patch('git_cl.Changelist._GetGerritProject', + return_value='foo').start() + mock.patch('git_cl.gerrit_util.GetProjectHead', + return_value='refs/heads/main').start() + mock.patch('git_cl.RunGit').start() + + cl = git_cl.Changelist() + options = optparse.Values() + cl.CMDUploadChange(options, [], 'foo', git_cl.ChangeDescription('bar')) + # ensure upload is called twice + self.assertEqual(len(m.mock_calls), 2) + # option overrides on retry + self.assertEqual(options.force, True) + self.assertEqual(options.edit_description, False) + def test_get_cl_statuses_no_changes(self): self.assertEqual([], list(git_cl.get_cl_statuses([], True)))