Allow percent encoding in patch set titles.

Bug: https://bugs.chromium.org/p/chromium/issues/detail?id=663787
Bug: Issue 8728
Change-Id: I62a251661a468a5df0418cd8e6988d12ecac0448
(cherry picked from commit 83010b57ea)

Documentation/user-upload.txt

@@ -313,12 +313,20 @@ A comment message can be applied to the change by using the `message` (or `m`)
 option:
 
 ----
-  git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/experimental%m=This_is_a_rebase_on_master
+  git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/experimental%m=This_is_a_rebase_on_master%21
 ----
 
 [NOTE]
-git push refs parameter does not allow spaces.  Use the '_' character instead,
+git push refs parameter does not allow spaces. Use the '_' or '+' character
-it will then be applied as "This is a rebase on master".
+to represent spaces, and percent-encoding to represent other special chars.
+The above example will thus be applied as "This is a rebase on master!"
+
+To avoid confusion in parsing the git ref, at least the following characters
+must be percent-encoded: " %^@.~-+_:/!". Note that some of the reserved
+characters (like tilde) are not escaped in the standard URL encoding rules,
+so a language-provided function (e.g. encodeURIComponent(), in javascript)
+might not suffice. To be safest, you might consider percent-encoding all
+non-alphanumeric characters (and all multibyte UTF-8 code points).
 
 [[publish-comments]]
 ==== Publish Draft Comments

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java

@@ -691,7 +691,9 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
     PushOneCommit push =
         pushFactory.create(
             db, admin.getIdent(), testRepo, PushOneCommit.SUBJECT, "a.txt", "content");
-    PushOneCommit.Result r = push.to("refs/for/master/%m=my_test_message");
+    // %2C is comma; the value below tests that percent decoding happens after splitting.
+    // All three ways of representing space ("%20", "+", and "_" are also exercised.
+    PushOneCommit.Result r = push.to("refs/for/master/%m=my_test%20+_message%2Cm=");
     r.assertOkStatus();
 
     push =
@@ -713,11 +715,53 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
       if (ri.isCurrent) {
         assertThat(ri.description).isEqualTo("new test message");
       } else {
-        assertThat(ri.description).isEqualTo("my test message");
+        assertThat(ri.description).isEqualTo("my test   message,m=");
       }
     }
   }
 
+  @Test
+  public void pushForMasterWithPercentEncodedMessage() throws Exception {
+    // Exercise percent-encoding of UTF-8, underscores, and patterns reserved by git-rev-parse.
+    PushOneCommit.Result r =
+        pushTo(
+            "refs/for/master/%m="
+                + "Punctu%2E%2e%2Eation%7E%2D%40%7Bu%7D%20%7C%20%28%E2%95%AF%C2%B0%E2%96%A1%C2%B0"
+                + "%EF%BC%89%E2%95%AF%EF%B8%B5%20%E2%94%BB%E2%94%81%E2%94%BB%20%5E%5F%5E");
+    r.assertOkStatus();
+    r.assertChange(Change.Status.NEW, null);
+    ChangeInfo ci = get(r.getChangeId(), MESSAGES, ALL_REVISIONS);
+    Collection<ChangeMessageInfo> changeMessages = ci.messages;
+    assertThat(changeMessages).hasSize(1);
+    for (ChangeMessageInfo cm : changeMessages) {
+      assertThat(cm.message)
+          .isEqualTo("Uploaded patch set 1.\nPunctu...ation~-@{u} | (╯°□°）╯︵ ┻━┻ ^_^");
+    }
+    Collection<RevisionInfo> revisions = ci.revisions.values();
+    assertThat(revisions).hasSize(1);
+    for (RevisionInfo ri : revisions) {
+      assertThat(ri.description).isEqualTo("Punctu...ation~-@{u} | (╯°□°）╯︵ ┻━┻ ^_^");
+    }
+  }
+
+  @Test
+  public void pushForMasterWithInvalidPercentEncodedMessage() throws Exception {
+    PushOneCommit.Result r = pushTo("refs/for/master/%m=not_percent_decodable_%%oops%20");
+    r.assertOkStatus();
+    r.assertChange(Change.Status.NEW, null);
+    ChangeInfo ci = get(r.getChangeId(), MESSAGES, ALL_REVISIONS);
+    Collection<ChangeMessageInfo> changeMessages = ci.messages;
+    assertThat(changeMessages).hasSize(1);
+    for (ChangeMessageInfo cm : changeMessages) {
+      assertThat(cm.message).isEqualTo("Uploaded patch set 1.\nnot percent decodable %%oops%20");
+    }
+    Collection<RevisionInfo> revisions = ci.revisions.values();
+    assertThat(revisions).hasSize(1);
+    for (RevisionInfo ri : revisions) {
+      assertThat(ri.description).isEqualTo("not percent decodable %%oops%20");
+    }
+  }
+
   @Test
   public void pushForMasterWithApprovals() throws Exception {
     PushOneCommit.Result r = pushTo("refs/for/master/%l=Code-Review");

gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java

@@ -27,6 +27,7 @@ import static com.google.gerrit.server.git.receive.ReceiveConstants.PUSH_OPTION_
 import static com.google.gerrit.server.git.receive.ReceiveConstants.SAME_CHANGE_ID_IN_MULTIPLE_CHANGES;
 import static com.google.gerrit.server.git.validators.CommitValidators.NEW_PATCHSET_PATTERN;
 import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromFooters;
+import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.Comparator.comparingInt;
 import static java.util.stream.Collectors.joining;
 import static java.util.stream.Collectors.toList;
@@ -157,6 +158,8 @@ import com.google.inject.assistedinject.Assisted;
 import com.google.inject.util.Providers;
 import java.io.IOException;
 import java.io.StringWriter;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -1274,8 +1277,19 @@ class ReceiveCommits {
       usage = "Comment message to apply to the review"
     )
     void addMessage(String token) {
-      // git push does not allow spaces in refs.
+      // Many characters have special meaning in the context of a git ref.
+      //
+      // Clients can use underscores to represent spaces.
       message = token.replace("_", " ");
+      try {
+        // Other characters can be represented using percent-encoding.
+        message = URLDecoder.decode(message, UTF_8.name());
+      } catch (IllegalArgumentException e) {
+        // Ignore decoding errors; leave message as percent-encoded.
+      } catch (UnsupportedEncodingException e) {
+        // This shouldn't happen; surely URLDecoder recognizes UTF-8.
+        throw new IllegalStateException(e);
+      }
     }
 
     @Option(