Revert "Add --push-options in git cl upload"

This reverts commit e2c65d7b75cb358c0b75c9668b075f821d03e0aa.

Reason for revert: https://crbug.com/1192942

Original change's description:
> Add --push-options in git cl upload
>
> --push-option parameter is passed to git push as is.
>
> R=​ehmaldonado@google.com
>
> Bug: 1184393
> Change-Id: Id1f7da1f0c8e8a23144b547d50d817fe8d4efeb1
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2786080
> Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
> Commit-Queue: Josip Sokcevic <sokcevic@google.com>

Bug: 1184393
Change-Id: Idf5d79fda3c5f917b1de5ca396837f14a401ffb7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2791749
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Josip Sokcevic <sokcevic@google.com>
Reviewed-by: Gavin Mak <gavinmak@google.com>

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)

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(