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/+/2786080Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Commit-Queue: Josip Sokcevic <sokcevic@google.com>

git_cl.py

@@ -2166,7 +2166,10 @@ class Changelist(object):
 
     gclient_utils.rmtree(git_info_dir)
 
-  def _RunGitPushWithTraces(self, refspec, refspec_opts, git_push_metadata):
+  def _RunGitPushWithTraces(self,
+                            refspec,
+                            push_options,
+                            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.
@@ -2186,8 +2189,12 @@ 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(
-          ['git', 'push', remote_url, refspec],
+          push_cmd,
           env=env,
           print_stdout=True,
           # Flush after every line: useful for seeing progress when running as
@@ -2217,7 +2224,7 @@ class Changelist(object):
         'command': 'git push',
         'execution_time': execution_time,
         'exit_code': push_returncode,
-        'arguments': metrics_utils.extract_known_subcommand_args(refspec_opts),
+        'arguments': metrics_utils.extract_known_subcommand_args(push_options),
       })
 
       git_push_metadata['execution_time'] = execution_time
@@ -2342,18 +2349,18 @@ class Changelist(object):
 
     # Extra options that can be specified at push time. Doc:
     # https://gerrit-review.googlesource.com/Documentation/user-upload.html
-    refspec_opts = []
+    push_options = options.push_options if options.push_options else []
 
     # 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:
-      refspec_opts.append('ready')
-      refspec_opts.append('notify=ALL')
+      push_options.append('ready')
+      push_options.append('notify=ALL')
     elif not self.GetIssue() and options.squash:
-      refspec_opts.append('wip')
+      push_options.append('wip')
     else:
-      refspec_opts.append('notify=NONE')
+      push_options.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.
@@ -2363,15 +2370,15 @@ class Changelist(object):
     options.title = self._GetTitleForUpload(options)
     if options.title:
       # Punctuation and whitespace in |title| must be percent-encoded.
-      refspec_opts.append(
+      push_options.append(
           'm=' + gerrit_util.PercentEncodeForGitRef(options.title))
 
     if options.private:
-      refspec_opts.append('private')
+      push_options.append('private')
 
     for r in sorted(reviewers):
       if r in valid_accounts:
-        refspec_opts.append('r=%s' % r)
+        push_options.append('r=%s' % r)
         reviewers.remove(r)
       else:
         # TODO(tandrii): this should probably be a hard failure.
@@ -2381,41 +2388,34 @@ 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:
-        refspec_opts.append('cc=%s' % c)
+        push_options.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
-      refspec_opts.append('topic=%s' % options.topic)
+      push_options.append('topic=%s' % options.topic)
 
     if options.enable_auto_submit:
-      refspec_opts.append('l=Auto-Submit+1')
+      push_options.append('l=Auto-Submit+1')
     if options.set_bot_commit:
-      refspec_opts.append('l=Bot-Commit+1')
+      push_options.append('l=Bot-Commit+1')
     if options.use_commit_queue:
-      refspec_opts.append('l=Commit-Queue+2')
+      push_options.append('l=Commit-Queue+2')
     elif options.cq_dry_run:
-      refspec_opts.append('l=Commit-Queue+1')
+      push_options.append('l=Commit-Queue+1')
 
     if change_desc.get_reviewers(tbr_only=True):
       score = gerrit_util.GetCodeReviewTbrScore(
           self.GetGerritHost(),
           self.GetGerritProject())
-      refspec_opts.append('l=Code-Review+%s' % score)
+      push_options.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())
-    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)
+    push_options += ['hashtag=%s' % t for t in sorted(hashtags)]
 
     git_push_metadata = {
         'gerrit_host': self.GetGerritHost(),
@@ -2423,7 +2423,10 @@ class Changelist(object):
         'change_id': change_id,
         'description': change_desc.description,
     }
-    push_stdout = self._RunGitPushWithTraces(refspec, refspec_opts,
+
+    push_stdout = self._RunGitPushWithTraces(
+        '%s:refs/for/%s' % (ref_to_push, branch),
+        push_options,
         git_push_metadata)
 
     if options.squash:
@@ -4192,6 +4195,10 @@ 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,19 +808,31 @@ 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'):
+                           labels=None,
+                           change_id=None,
+                           final_description=None,
+                           gitcookies_exists=True,
+                           force=False,
+                           edit_description=None,
+                           default_branch='master',
+                           push_opts=None):
     if post_amend_description is None:
       post_amend_description = description
     cc = cc or []
+    push_opts = push_opts if push_opts else []
 
     calls = []
 
@@ -876,17 +888,18 @@ class TestGitCl(unittest.TestCase):
       ((['git', 'rev-list', parent + '..' + ref_to_push],),'1hashPerLine\n'),
     ]
 
-    metrics_arguments = []
+
+    metrics_arguments = [arg for arg in push_opts if arg != '-o']
 
     if notify:
-      ref_suffix = '%ready,notify=ALL'
+      push_opts += ['-o', 'ready', '-o', 'notify=ALL']
       metrics_arguments += ['ready', 'notify=ALL']
     else:
       if not issue and squash:
-        ref_suffix = '%wip'
+        push_opts += ['-o', 'wip']
         metrics_arguments.append('wip')
       else:
-        ref_suffix = '%notify=NONE'
+        push_opts += ['-o', 'notify=NONE']
         metrics_arguments.append('notify=NONE')
 
     # If issue is given, then description is fetched from Gerrit instead.
@@ -901,18 +914,19 @@ class TestGitCl(unittest.TestCase):
         ]
         title = 'User input'
     if title:
-      ref_suffix += ',m=' + gerrit_util.PercentEncodeForGitRef(title)
+      push_opts += ['-o', '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):
-        ref_suffix += ',r=%s' % r
+        push_opts += ['-o', '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):
-        ref_suffix += ',cc=%s' % c
+        push_opts += ['-o', 'cc=%s' % c]
         metrics_arguments.append('cc')
       reviewers, cc = [], []
     else:
@@ -928,19 +942,19 @@ class TestGitCl(unittest.TestCase):
       ]
       for r in sorted(reviewers):
         if r != 'bad-account-or-email':
-          ref_suffix  += ',r=%s' % r
+          push_opts += ['-o', 'r=%s' % r]
           metrics_arguments.append('r')
           reviewers.remove(r)
       if issue is None:
         cc += ['joe@example.com']
       for c in sorted(cc):
-        ref_suffix += ',cc=%s' % c
+        push_opts += ['-o', 'cc=%s' % c]
         metrics_arguments.append('cc')
         if c in cc:
           cc.remove(c)
 
     for k, v in sorted((labels or {}).items()):
-      ref_suffix += ',l=%s+%d' % (k, v)
+      push_opts += ['-o', 'l=%s+%d' % (k, v)]
       metrics_arguments.append('l=%s+%d' % (k, v))
 
     if tbr:
@@ -952,10 +966,16 @@ class TestGitCl(unittest.TestCase):
       ]
 
     calls += [
-      (('time.time',), 1000,),
-      ((['git', 'push',
+        (
+            ('time.time', ),
+            1000,
+        ),
+        (
+            ([
+                'git', 'push',
                 'https://%s.googlesource.com/my/repo' % short_hostname,
-         ref_to_push + ':refs/for/refs/heads/' + default_branch + ref_suffix],),
+                ref_to_push + ':refs/for/refs/heads/' + default_branch
+            ] + push_opts, ),
             (('remote:\n'
               'remote: Processing changes: (\)\n'
               'remote: Processing changes: (|)\n'
@@ -965,22 +985,27 @@ class TestGitCl(unittest.TestCase):
               'remote: Processing changes: new: 1, done\n'
               'remote:\n'
               'remote: New Changes:\n'
-         'remote:   https://%s-review.googlesource.com/#/c/my/repo/+/123456'
+              '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',
-        {
+              ' * [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,),
+            None,
+        ),
     ]
 
     final_description = final_description or post_amend_description.strip()
@@ -1087,8 +1112,7 @@ class TestGitCl(unittest.TestCase):
       ]
     return calls
 
-  def _run_gerrit_upload_test(
-      self,
+  def _run_gerrit_upload_test(self,
                               upload_args,
                               description,
                               reviewers=None,
@@ -1112,7 +1136,8 @@ class TestGitCl(unittest.TestCase):
                               log_description=None,
                               edit_description=None,
                               fetched_description=None,
-      default_branch='master'):
+                              default_branch='master',
+                              push_opts=None):
     """Generic gerrit upload test framework."""
     if squash_mode is None:
       if '--no-squash' in upload_args:
@@ -1192,12 +1217,17 @@ 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,
@@ -1205,7 +1235,8 @@ class TestGitCl(unittest.TestCase):
           gitcookies_exists=gitcookies_exists,
           force=force,
           edit_description=edit_description,
-          default_branch=default_branch)
+          default_branch=default_branch,
+          push_opts=push_opts)
     # Uncomment when debugging.
     # print('\n'.join(map(lambda x: '%2i: %s' % x, enumerate(self.calls))))
     git_cl.main(['upload'] + upload_args)
@@ -1260,10 +1291,18 @@ 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(
-        [],
+    self._run_gerrit_upload_test([],
                                  'desc ✔\n\nBUG=\n\nChange-Id: I123456789\n',
                                  [],
                                  squash=False,