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)))