Allow punctuation in patch set titles by means of percent encoding.

Bug: 663787
Change-Id: Ie7e2fca42db0c840d713dc6edba5dc0c2b65223d
Reviewed-on: https://chromium-review.googlesource.com/676086Reviewed-by: Aaron Gable <agable@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Nick Carter <nick@chromium.org>

gerrit_util.py

@@ -830,6 +830,26 @@ def GetAccountDetails(host, account_id='self'):
   return ReadHttpJsonResponse(conn)
 
 
+def PercentEncodeForGitRef(original):
+  """Apply percent-encoding for strings sent to gerrit via git ref metadata.
+
+  The encoding used is based on but stricter than URL encoding (Section 2.1
+  of RFC 3986). The only non-escaped characters are alphanumerics, and
+  'SPACE' (U+0020) can be represented as 'LOW LINE' (U+005F) or
+  'PLUS SIGN' (U+002B).
+
+  For more information, see the Gerrit docs here:
+
+  https://gerrit-review.googlesource.com/Documentation/user-upload.html#message
+  """
+  safe = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789 '
+  encoded = ''.join(c if c in safe else '%%%02X' % ord(c) for c in original)
+
+  # spaces are not allowed in git refs; gerrit will interpret either '_' or
+  # '+' (or '%20') as space. Use '_' since that has been supported the longest.
+  return encoded.replace(' ', '_')
+
+
 @contextlib.contextmanager
 def tempdir():
   tdir = None

git_cl.py

@@ -2901,7 +2901,6 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
 
     # This may be None; default fallback value is determined in logic below.
     title = options.title
-    automatic_title = False
 
     # Extract bug number from branch name.
     bug = options.bug
@@ -2931,8 +2930,6 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
             else:
               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)
@@ -2982,7 +2979,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...")
         change_ids = git_footers.get_footer_change_id(change_desc.description)
@@ -3066,16 +3062,8 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
     # if --send-mail is set on non-initial upload as Rietveld used to do it.
 
     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(' ', '_'))
+      # Punctuation and whitespace in |title| must be percent-encoded.
+      refspec_opts.append('m=' + gerrit_util.PercentEncodeForGitRef(title))
 
     if options.private:
       refspec_opts.append('private')

tests/git_cl_test.py

@@ -1755,19 +1755,14 @@ class TestGitCl(TestCase):
         squash=False,
         squash_mode='override_nosquash')
 
-  def test_gerrit_patchset_title_bad_chars(self):
+  def test_gerrit_patchset_title_special_chars(self):
     self.mock(git_cl.sys, 'stdout', StringIO.StringIO())
     self._run_gerrit_upload_test(
-        ['-f', '-t', 'Don\'t put bad cha,.rs'],
+        ['-f', '-t', 'We\'ll escape ^_ ^ special chars...@{u}'],
         '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())
+        title='We%27ll_escape_%5E%5F_%5E_special_chars%2E%2E%2E%40%7Bu%7D')
 
   def test_gerrit_reviewers_cmd_line(self):
     self._run_gerrit_upload_test(