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 <sokcevic@google.com>
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
diff --git a/gerrit_util.py b/gerrit_util.py
index d91312a..aa8a396 100644
--- a/gerrit_util.py
+++ b/gerrit_util.py
@@ -926,6 +926,12 @@
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 6f40737..91be147 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -145,6 +145,10 @@
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 @@
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 @@
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 @@
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 @@
# 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 @@
git_push_metadata = {
'gerrit_host': self._GetGerritHost(),
- 'title': title or '<untitled>',
+ 'title': options.title or '<untitled>',
'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 cf788f7..2c06c98 100755
--- a/tests/git_cl_test.py
+++ b/tests/git_cl_test.py
@@ -226,6 +226,70 @@
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)))
