diff --git a/git_cl.py b/git_cl.py index e343cfc36f..4f248fcfaf 100755 --- a/git_cl.py +++ b/git_cl.py @@ -2166,10 +2166,7 @@ class Changelist(object): gclient_utils.rmtree(git_info_dir) - def _RunGitPushWithTraces(self, - refspec, - push_options, - 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. @@ -2189,12 +2186,8 @@ class Changelist(object): push_returncode = 0 remote_url = self.GetRemoteUrl() before_push = time_time() - push_cmd = ['git', 'push', remote_url, refspec] - for opt in push_options: - push_cmd.extend(['-o', opt]) - push_stdout = gclient_utils.CheckCallAndFilter( - push_cmd, + ['git', 'push', remote_url, refspec], env=env, print_stdout=True, # Flush after every line: useful for seeing progress when running as @@ -2224,7 +2217,7 @@ class Changelist(object): 'command': 'git push', 'execution_time': execution_time, 'exit_code': push_returncode, - 'arguments': metrics_utils.extract_known_subcommand_args(push_options), + 'arguments': metrics_utils.extract_known_subcommand_args(refspec_opts), }) git_push_metadata['execution_time'] = execution_time @@ -2349,18 +2342,18 @@ class Changelist(object): # Extra options that can be specified at push time. Doc: # https://gerrit-review.googlesource.com/Documentation/user-upload.html - push_options = options.push_options if options.push_options else [] + refspec_opts = [] # By default, new changes are started in WIP mode, and subsequent patchsets # don't send email. At any time, passing --send-mail will mark the change # ready and send email for that particular patch. if options.send_mail: - push_options.append('ready') - push_options.append('notify=ALL') + refspec_opts.append('ready') + refspec_opts.append('notify=ALL') elif not self.GetIssue() and options.squash: - push_options.append('wip') + refspec_opts.append('wip') else: - push_options.append('notify=NONE') + refspec_opts.append('notify=NONE') # 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. @@ -2370,15 +2363,15 @@ class Changelist(object): options.title = self._GetTitleForUpload(options) if options.title: # Punctuation and whitespace in |title| must be percent-encoded. - push_options.append( + refspec_opts.append( 'm=' + gerrit_util.PercentEncodeForGitRef(options.title)) if options.private: - push_options.append('private') + refspec_opts.append('private') for r in sorted(reviewers): if r in valid_accounts: - push_options.append('r=%s' % r) + refspec_opts.append('r=%s' % r) reviewers.remove(r) else: # TODO(tandrii): this should probably be a hard failure. @@ -2388,34 +2381,41 @@ class Changelist(object): # refspec option will be rejected if cc doesn't correspond to an # account, even though REST call to add such arbitrary cc may succeed. if c in valid_accounts: - push_options.append('cc=%s' % c) + refspec_opts.append('cc=%s' % c) cc.remove(c) if options.topic: # Documentation on Gerrit topics is here: # https://gerrit-review.googlesource.com/Documentation/user-upload.html#topic - push_options.append('topic=%s' % options.topic) + refspec_opts.append('topic=%s' % options.topic) if options.enable_auto_submit: - push_options.append('l=Auto-Submit+1') + refspec_opts.append('l=Auto-Submit+1') if options.set_bot_commit: - push_options.append('l=Bot-Commit+1') + refspec_opts.append('l=Bot-Commit+1') if options.use_commit_queue: - push_options.append('l=Commit-Queue+2') + refspec_opts.append('l=Commit-Queue+2') elif options.cq_dry_run: - push_options.append('l=Commit-Queue+1') + refspec_opts.append('l=Commit-Queue+1') if change_desc.get_reviewers(tbr_only=True): score = gerrit_util.GetCodeReviewTbrScore( self.GetGerritHost(), self.GetGerritProject()) - push_options.append('l=Code-Review+%s' % score) + refspec_opts.append('l=Code-Review+%s' % score) # Gerrit sorts hashtags, so order is not important. hashtags = {change_desc.sanitize_hash_tag(t) for t in options.hashtags} if not self.GetIssue(): hashtags.update(change_desc.get_hash_tags()) - push_options += ['hashtag=%s' % t for t in sorted(hashtags)] + refspec_opts += ['hashtag=%s' % t for t in sorted(hashtags)] + + refspec_suffix = '' + if refspec_opts: + refspec_suffix = '%' + ','.join(refspec_opts) + assert ' ' not in refspec_suffix, ( + 'spaces not allowed in refspec: "%s"' % refspec_suffix) + refspec = '%s:refs/for/%s%s' % (ref_to_push, branch, refspec_suffix) git_push_metadata = { 'gerrit_host': self.GetGerritHost(), @@ -2423,11 +2423,8 @@ class Changelist(object): 'change_id': change_id, 'description': change_desc.description, } - - push_stdout = self._RunGitPushWithTraces( - '%s:refs/for/%s' % (ref_to_push, branch), - push_options, - 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.*') @@ -4195,10 +4192,6 @@ def CMDupload(parser, args): help='Run presubmit checks in the ResultSink environment ' 'and send results to the ResultDB database.') parser.add_option('--realm', help='LUCI realm if reporting to ResultDB') - parser.add_option('-o', '--push-options', action='append', default=[], - help='Transmit the given string to the server when ' - 'performing git push (pass-through). See git-push ' - 'documentation for more details.') orig_args = args (options, args) = parser.parse_args(args) diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 27056ba62b..539e53860f 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -808,31 +808,19 @@ class TestGitCl(unittest.TestCase): return calls - def _gerrit_upload_calls(self, - description, - reviewers, - squash, + def _gerrit_upload_calls(self, description, reviewers, squash, squash_mode='default', - title=None, - notify=False, - post_amend_description=None, - issue=None, - cc=None, - custom_cl_base=None, - tbr=None, + title=None, notify=False, + post_amend_description=None, issue=None, cc=None, + custom_cl_base=None, tbr=None, short_hostname='chromium', - labels=None, - change_id=None, - final_description=None, - gitcookies_exists=True, - force=False, - edit_description=None, - default_branch='master', - push_opts=None): + labels=None, change_id=None, + final_description=None, gitcookies_exists=True, + force=False, edit_description=None, + default_branch='master'): if post_amend_description is None: post_amend_description = description cc = cc or [] - push_opts = push_opts if push_opts else [] calls = [] @@ -888,18 +876,17 @@ class TestGitCl(unittest.TestCase): ((['git', 'rev-list', parent + '..' + ref_to_push],),'1hashPerLine\n'), ] - - metrics_arguments = [arg for arg in push_opts if arg != '-o'] + metrics_arguments = [] if notify: - push_opts += ['-o', 'ready', '-o', 'notify=ALL'] + ref_suffix = '%ready,notify=ALL' metrics_arguments += ['ready', 'notify=ALL'] else: if not issue and squash: - push_opts += ['-o', 'wip'] + ref_suffix = '%wip' metrics_arguments.append('wip') else: - push_opts += ['-o', 'notify=NONE'] + ref_suffix = '%notify=NONE' metrics_arguments.append('notify=NONE') # If issue is given, then description is fetched from Gerrit instead. @@ -914,19 +901,18 @@ class TestGitCl(unittest.TestCase): ] title = 'User input' if title: - push_opts += ['-o', 'm=' + gerrit_util.PercentEncodeForGitRef(title)] + ref_suffix += ',m=' + gerrit_util.PercentEncodeForGitRef(title) metrics_arguments.append('m') if short_hostname == 'chromium': # All reviewers and ccs get into ref_suffix. for r in sorted(reviewers): - push_opts += ['-o', 'r=%s' % r] + ref_suffix += ',r=%s' % r metrics_arguments.append('r') - if issue is None: cc += ['test-more-cc@chromium.org', 'joe@example.com'] for c in sorted(cc): - push_opts += ['-o', 'cc=%s' % c] + ref_suffix += ',cc=%s' % c metrics_arguments.append('cc') reviewers, cc = [], [] else: @@ -942,19 +928,19 @@ class TestGitCl(unittest.TestCase): ] for r in sorted(reviewers): if r != 'bad-account-or-email': - push_opts += ['-o', 'r=%s' % r] + ref_suffix += ',r=%s' % r metrics_arguments.append('r') reviewers.remove(r) if issue is None: cc += ['joe@example.com'] for c in sorted(cc): - push_opts += ['-o', 'cc=%s' % c] + ref_suffix += ',cc=%s' % c metrics_arguments.append('cc') if c in cc: cc.remove(c) for k, v in sorted((labels or {}).items()): - push_opts += ['-o', 'l=%s+%d' % (k, v)] + ref_suffix += ',l=%s+%d' % (k, v) metrics_arguments.append('l=%s+%d' % (k, v)) if tbr: @@ -966,46 +952,35 @@ class TestGitCl(unittest.TestCase): ] calls += [ - ( - ('time.time', ), - 1000, - ), - ( - ([ - 'git', 'push', - 'https://%s.googlesource.com/my/repo' % short_hostname, - ref_to_push + ':refs/for/refs/heads/' + default_branch - ] + push_opts, ), - (('remote:\n' - 'remote: Processing changes: (\)\n' - 'remote: Processing changes: (|)\n' - 'remote: Processing changes: (/)\n' - 'remote: Processing changes: (-)\n' - 'remote: Processing changes: new: 1 (/)\n' - 'remote: Processing changes: new: 1, done\n' - 'remote:\n' - 'remote: New Changes:\n' - 'remote: ' - 'https://%s-review.googlesource.com/#/c/my/repo/+/123456' - ' XXX\n' - 'remote:\n' - 'To https://%s.googlesource.com/my/repo\n' - ' * [new branch] hhhh -> refs/for/refs/heads/%s\n') % - (short_hostname, short_hostname, default_branch)), - ), - ( - ('time.time', ), - 2000, - ), - ( - ('add_repeated', 'sub_commands', { - 'execution_time': 1000, - 'command': 'git push', - 'exit_code': 0, - 'arguments': sorted(metrics_arguments), - }), - None, - ), + (('time.time',), 1000,), + ((['git', 'push', + 'https://%s.googlesource.com/my/repo' % short_hostname, + ref_to_push + ':refs/for/refs/heads/' + default_branch + ref_suffix],), + (('remote:\n' + 'remote: Processing changes: (\)\n' + 'remote: Processing changes: (|)\n' + 'remote: Processing changes: (/)\n' + 'remote: Processing changes: (-)\n' + 'remote: Processing changes: new: 1 (/)\n' + 'remote: Processing changes: new: 1, done\n' + 'remote:\n' + 'remote: New Changes:\n' + 'remote: https://%s-review.googlesource.com/#/c/my/repo/+/123456' + ' XXX\n' + 'remote:\n' + 'To https://%s.googlesource.com/my/repo\n' + ' * [new branch] hhhh -> refs/for/refs/heads/%s\n' + ) % (short_hostname, short_hostname, default_branch)),), + (('time.time',), 2000,), + (('add_repeated', + 'sub_commands', + { + 'execution_time': 1000, + 'command': 'git push', + 'exit_code': 0, + 'arguments': sorted(metrics_arguments), + }), + None,), ] final_description = final_description or post_amend_description.strip() @@ -1112,32 +1087,32 @@ class TestGitCl(unittest.TestCase): ] return calls - def _run_gerrit_upload_test(self, - upload_args, - description, - reviewers=None, - squash=True, - squash_mode=None, - title=None, - notify=False, - post_amend_description=None, - issue=None, - cc=None, - fetched_status=None, - other_cl_owner=None, - custom_cl_base=None, - tbr=None, - short_hostname='chromium', - labels=None, - change_id=None, - final_description=None, - gitcookies_exists=True, - force=False, - log_description=None, - edit_description=None, - fetched_description=None, - default_branch='master', - push_opts=None): + def _run_gerrit_upload_test( + self, + upload_args, + description, + reviewers=None, + squash=True, + squash_mode=None, + title=None, + notify=False, + post_amend_description=None, + issue=None, + cc=None, + fetched_status=None, + other_cl_owner=None, + custom_cl_base=None, + tbr=None, + short_hostname='chromium', + labels=None, + change_id=None, + final_description=None, + gitcookies_exists=True, + force=False, + log_description=None, + edit_description=None, + fetched_description=None, + default_branch='master'): """Generic gerrit upload test framework.""" if squash_mode is None: if '--no-squash' in upload_args: @@ -1217,17 +1192,12 @@ class TestGitCl(unittest.TestCase): 'gclient_utils.temporary_file', TemporaryFileMock()).start() mock.patch('os.remove', return_value=True).start() self.calls += self._gerrit_upload_calls( - description, - reviewers, - squash, + description, reviewers, squash, squash_mode=squash_mode, - title=title, - notify=notify, + title=title, notify=notify, post_amend_description=post_amend_description, - issue=issue, - cc=cc, - custom_cl_base=custom_cl_base, - tbr=tbr, + issue=issue, cc=cc, + custom_cl_base=custom_cl_base, tbr=tbr, short_hostname=short_hostname, labels=labels, change_id=change_id, @@ -1235,8 +1205,7 @@ class TestGitCl(unittest.TestCase): gitcookies_exists=gitcookies_exists, force=force, edit_description=edit_description, - default_branch=default_branch, - push_opts=push_opts) + default_branch=default_branch) # Uncomment when debugging. # print('\n'.join(map(lambda x: '%2i: %s' % x, enumerate(self.calls)))) git_cl.main(['upload'] + upload_args) @@ -1291,24 +1260,16 @@ class TestGitCl(unittest.TestCase): squash_mode='override_nosquash', change_id='I123456789') - def test_gerrit_push_opts(self): - self._run_gerrit_upload_test(['-o', 'wip'], - 'desc ✔\n\nBUG=\n\nChange-Id: I123456789\n', - [], - squash=False, - squash_mode='override_nosquash', - change_id='I123456789', - push_opts=['-o', 'wip']) - def test_gerrit_no_reviewer_non_chromium_host(self): # TODO(crbug/877717): remove this test case. - self._run_gerrit_upload_test([], - 'desc ✔\n\nBUG=\n\nChange-Id: I123456789\n', - [], - squash=False, - squash_mode='override_nosquash', - short_hostname='other', - change_id='I123456789') + self._run_gerrit_upload_test( + [], + 'desc ✔\n\nBUG=\n\nChange-Id: I123456789\n', + [], + squash=False, + squash_mode='override_nosquash', + short_hostname='other', + change_id='I123456789') def test_gerrit_patchset_title_special_chars_nosquash(self): self._run_gerrit_upload_test(