git cl upload for Gerrit: use push options instead of refspec.

This removes limitation of no special chars in patchset titles.

BUG=chromium:663787,chromium:707963,gerrit:5184
R=sergiyb@chromium.org,agable@chromium.org
TEST=uploaded this CL using depot_tools with this patch :)

Change-Id: I5d684d0a0aa286a45ff99cca6d57aefa8436cd0f
Reviewed-on: https://chromium-review.googlesource.com/468926
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>
Reviewed-by: Sergiy Byelozyorov <sergiyb@google.com>

git_cl.py

@@ -2815,7 +2815,6 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
 
     # This may be None; default fallback value is determined in logic below.
     title = options.title
-    automatic_title = False
 
     if options.squash:
       self._GerritCommitMsgHookCheck(offer_removal=not options.force)
@@ -2830,8 +2829,6 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
           default_title = RunGit(['show', '-s', '--format=%s', 'HEAD']).strip()
           title = ask_for_data(
               'Title for patchset [%s]: ' % default_title) or default_title
-          if title == default_title:
-            automatic_title = True
         change_id = self._GetChangeDetail()['change_id']
         while True:
           footer_change_ids = git_footers.get_footer_change_id(message)
@@ -2880,7 +2877,6 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
         # On first upload, patchset title is always this string, while
         # --title flag gets converted to first line of message.
         title = 'Initial upload'
-        automatic_title = True
         if not change_desc.description:
           DieWithError("Description is empty. Aborting...")
         message = change_desc.description
@@ -2932,29 +2928,22 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
 
     # Extra options that can be specified at push time. Doc:
     # https://gerrit-review.googlesource.com/Documentation/user-upload.html
-    refspec_opts = []
+    # This can be done either through use of refspec OR by using --push-option
+    # as documented here: https://git-scm.com/docs/git-push#git-push--o .
+    push_opts = []
     if change_desc.get_reviewers(tbr_only=True):
       print('Adding self-LGTM (Code-Review +1) because of TBRs')
-      refspec_opts.append('l=Code-Review+1')
+      push_opts.append('l=Code-Review+1')
 
     if title:
-      if not re.match(r'^[\w ]+$', title):
-        title = re.sub(r'[^\w ]', '', title)
-        if not automatic_title:
-          print('WARNING: Patchset title may only contain alphanumeric chars '
-                'and spaces. You can edit it in the UI. '
-                'See https://crbug.com/663787.\n'
-                'Cleaned up title: %s' % title)
-      # Per doc, spaces must be converted to underscores, and Gerrit will do the
-      # reverse on its side.
-      refspec_opts.append('m=' + title.replace(' ', '_'))
+      push_opts.append('m=' + title)
 
     if options.send_mail:
       if not change_desc.get_reviewers():
         DieWithError('Must specify reviewers to send email.', change_desc)
-      refspec_opts.append('notify=ALL')
+      push_opts.append('notify=ALL')
     else:
-      refspec_opts.append('notify=NONE')
+      push_opts.append('notify=NONE')
 
     reviewers = change_desc.get_reviewers()
     if reviewers:
@@ -2962,28 +2951,24 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
       # side for real (b/34702620).
       def clean_invisible_chars(email):
         return email.decode('unicode_escape').encode('ascii', 'ignore')
-      refspec_opts.extend('r=' + clean_invisible_chars(email).strip()
+      push_opts.extend('r=' + clean_invisible_chars(email).strip()
                        for email in reviewers)
 
     if options.private:
-      refspec_opts.append('draft')
+      push_opts.append('draft')
 
     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)
-
-    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_opts.append('topic=%s' % options.topic)
 
+    push_cmd = ['git', 'push']
+    for opt in sorted(push_opts):  # Sort to make tests deterministic.
+      push_cmd += ['-o', opt]
+    push_cmd += [self.GetRemoteUrl(), '%s:refs/for/%s' % (ref_to_push, branch)]
     try:
       push_stdout = gclient_utils.CheckCallAndFilter(
-          ['git', 'push', self.GetRemoteUrl(), refspec],
-          print_stdout=True,
+          push_cmd, print_stdout=True,
           # Flush after every line: useful for seeing progress when running as
           # recipe.
           filter_fn=lambda _: sys.stdout.flush())

tests/git_cl_test.py

@@ -7,6 +7,7 @@
 
 import contextlib
 import datetime
+import itertools
 import json
 import logging
 import os
@@ -1415,13 +1416,14 @@ class TestGitCl(TestCase):
   def _gerrit_upload_calls(cls, description, reviewers, squash,
                            squash_mode='default',
                            expected_upstream_ref='origin/refs/heads/master',
-                           ref_suffix='', title=None, notify=False,
+                           push_opts=None, title=None, notify=False,
                            post_amend_description=None, issue=None, cc=None,
                            git_mirror=None):
     if post_amend_description is None:
       post_amend_description = description
     calls = []
     cc = cc or []
+    push_opts = push_opts or []
 
     if squash_mode == 'default':
       calls.extend([
@@ -1443,14 +1445,14 @@ class TestGitCl(TestCase):
                    'fake_ancestor_sha..HEAD'],),
                   description)]
       if squash:
-        title = 'Initial_upload'
+        title = 'Initial upload'
     else:
       if not title:
         calls += [
             ((['git', 'show', '-s', '--format=%s', 'HEAD'],), ''),
             (('ask_for_data', 'Title for patchset []: '), 'User input'),
         ]
-        title = 'User_input'
+        title = 'User input'
     if not git_footers.get_footer_change_id(description) and not squash:
       calls += [
           # DownloadGerritHook(False)
@@ -1497,20 +1499,10 @@ class TestGitCl(TestCase):
         ]
 
     if title:
-      if ref_suffix:
-        ref_suffix += ',m=' + title
-      else:
-        ref_suffix = '%m=' + title
-
-    notify_suffix = 'notify=%s' % ('ALL' if notify else 'NONE')
-    if ref_suffix:
-      ref_suffix += ',' + notify_suffix
-    else:
-      ref_suffix = '%' + notify_suffix
+      push_opts += ['m=' + title]
 
-    if reviewers:
-      ref_suffix += ',' + ','.join('r=%s' % email
-                                   for email in sorted(reviewers))
+    push_opts += ['notify=%s' % ('ALL' if notify else 'NONE')]
+    push_opts += ['r=%s' % email for email in sorted(reviewers or [])]
 
     if git_mirror is None:
       calls += [
@@ -1529,9 +1521,10 @@ class TestGitCl(TestCase):
       ]
 
     calls += [
-        ((['git', 'push',
-           'https://chromium.googlesource.com/yyy/zzz',
-           ref_to_push + ':refs/for/refs/heads/master' + ref_suffix],),
+        ((['git', 'push'] +
+          list(itertools.chain(*(['-o', opt] for opt in sorted(push_opts)))) +
+          ['https://chromium.googlesource.com/yyy/zzz',
+           ref_to_push + ':refs/for/refs/heads/master'],),
          ('remote:\n'
           'remote: Processing changes: (\)\n'
           'remote: Processing changes: (|)\n'
@@ -1572,7 +1565,7 @@ class TestGitCl(TestCase):
       squash=True,
       squash_mode=None,
       expected_upstream_ref='origin/refs/heads/master',
-      ref_suffix='',
+      push_opts=None,
       title=None,
       notify=False,
       post_amend_description=None,
@@ -1619,7 +1612,7 @@ class TestGitCl(TestCase):
           description, reviewers, squash,
           squash_mode=squash_mode,
           expected_upstream_ref=expected_upstream_ref,
-          ref_suffix=ref_suffix, title=title, notify=notify,
+          push_opts=push_opts, title=title, notify=notify,
           post_amend_description=post_amend_description,
           issue=issue, cc=cc, git_mirror=git_mirror)
     # Uncomment when debugging.
@@ -1652,20 +1645,6 @@ class TestGitCl(TestCase):
         squash=False,
         squash_mode='override_nosquash')
 
-  def test_gerrit_patch_bad_chars(self):
-    self.mock(git_cl.sys, 'stdout', StringIO.StringIO())
-    self._run_gerrit_upload_test(
-        ['-f', '-t', 'Don\'t put bad cha,.rs'],
-        'desc\n\nBUG=\n\nChange-Id: I123456789',
-        squash=False,
-        squash_mode='override_nosquash',
-        title='Dont_put_bad_chars')
-    self.assertIn(
-        'WARNING: Patchset title may only contain alphanumeric chars '
-        'and spaces. You can edit it in the UI. See https://crbug.com/663787.\n'
-        'Cleaned up title: Dont put bad chars\n',
-        git_cl.sys.stdout.getvalue())
-
   def test_gerrit_reviewers_cmd_line(self):
     self._run_gerrit_upload_test(
         ['-r', 'foo@example.com', '--send-mail'],
@@ -1684,7 +1663,7 @@ class TestGitCl(TestCase):
         ['reviewer@example.com', 'another@example.com'],
         squash=False,
         squash_mode='override_nosquash',
-        ref_suffix='%l=Code-Review+1',
+        push_opts=['l=Code-Review+1'],
         cc=['more@example.com', 'people@example.com'])
 
   def test_gerrit_upload_squash_first_is_default(self):