Inline edit: Strip trailing blank lines from commit messages

Trim the input commit message to prevent trailing blank lines. If
blank lines are present, it causes problems when the commit is
merged and then cherry-picked with the -x option (from the command
line).

Trim it both on the client and on the server.  If we do it only on
the client, it will still be possible to update the commit message
with trailing blank lines by a direct call to the REST API.

Bug: Issue 3616
Change-Id: I1a3df4668d7ba6f395c5e782e2962652ead577d8

Documentation/user-inline-edit.txt

@@ -60,6 +60,8 @@ screen editor for that file.
 To save edits, click the 'Save' button or press `CTRL-S`.  To return to the
 change screen, click the 'Close' button.
 
+Note that when editing the commit message, trailing blank lines will be stripped.
+
 image::images/inline-edit-full-screen-editor.png[width=800, link="images/inline-edit-full-screen-editor.png"]
 
 If there are unsaved edits when the 'Close' button is pressed, a dialog will

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/edit/ChangeEditIT.java

@@ -296,7 +296,7 @@ public class ChangeEditIT extends AbstractDaemonTest {
     Optional<ChangeEdit> edit = editUtil.byChange(change);
     assertThat(edit.get().getEditCommit().getParentCount()).isEqualTo(0);
 
-    String msg = String.format("New commit message\n\nChange-Id: %s",
+    String msg = String.format("New commit message\n\nChange-Id: %s\n",
         change.getKey());
     assertThat(modifier.modifyMessage(edit.get(), msg))
         .isEqualTo(RefUpdate.Result.FORCED);
@@ -310,17 +310,10 @@ public class ChangeEditIT extends AbstractDaemonTest {
         .isEqualTo(RefUpdate.Result.NEW);
     Optional<ChangeEdit> edit = editUtil.byChange(change);
 
-    try {
-      modifier.modifyMessage(
-          edit.get(),
-          edit.get().getEditCommit().getFullMessage());
-      fail("UnchangedCommitMessageException expected");
-    } catch (UnchangedCommitMessageException ex) {
-      assertThat(ex.getMessage()).isEqualTo(
-          "New commit message cannot be same as existing commit message");
-    }
+    assertUnchangedMessage(edit, edit.get().getEditCommit().getFullMessage());
+    assertUnchangedMessage(edit, edit.get().getEditCommit().getFullMessage() + "\n\n");
 
-    String msg = String.format("New commit message\n\nChange-Id: %s",
+    String msg = String.format("New commit message\n\nChange-Id: %s\n",
         change.getKey());
     assertThat(modifier.modifyMessage(edit.get(), msg)).isEqualTo(
         RefUpdate.Result.FORCED);
@@ -342,7 +335,7 @@ public class ChangeEditIT extends AbstractDaemonTest {
         .isEqualTo(SC_NOT_FOUND);
     EditMessage.Input in = new EditMessage.Input();
     in.message = String.format("New commit message\n\n" +
-        CONTENT_NEW2_STR + "\n\nChange-Id: %s",
+        CONTENT_NEW2_STR + "\n\nChange-Id: %s\n",
         change.getKey());
     assertThat(adminSession.put(urlEditMessage(), in).getStatusCode())
         .isEqualTo(SC_NO_CONTENT);
@@ -352,7 +345,7 @@ public class ChangeEditIT extends AbstractDaemonTest {
     Optional<ChangeEdit> edit = editUtil.byChange(change);
     assertThat(edit.get().getEditCommit().getFullMessage())
         .isEqualTo(in.message);
-    in.message = String.format("New commit message2\n\nChange-Id: %s",
+    in.message = String.format("New commit message2\n\nChange-Id: %s\n",
         change.getKey());
     assertThat(adminSession.put(urlEditMessage(), in).getStatusCode())
         .isEqualTo(SC_NO_CONTENT);
@@ -672,6 +665,19 @@ public class ChangeEditIT extends AbstractDaemonTest {
     assertThat(approvals.get(0).value).isEqualTo(1);
   }
 
+  private void assertUnchangedMessage(Optional<ChangeEdit> edit, String message)
+      throws Exception {
+    try {
+      modifier.modifyMessage(
+          edit.get(),
+          message);
+      fail("UnchangedCommitMessageException expected");
+    } catch (UnchangedCommitMessageException ex) {
+      assertThat(ex.getMessage()).isEqualTo(
+          "New commit message cannot be same as existing commit message");
+    }
+  }
+
   private String newChange(Git git, PersonIdent ident) throws Exception {
     PushOneCommit push =
         pushFactory.create(db, ident, PushOneCommit.SUBJECT, FILE_NAME,

gerrit-gwtui/src/main/java/com/google/gerrit/client/editor/EditScreen.java

@@ -453,6 +453,13 @@ public class EditScreen extends Screen {
         if (!cm.isClean(generation)) {
           close.setEnabled(false);
           String text = cm.getValue();
+          if (Patch.COMMIT_MSG.equals(path)) {
+            String trimmed = text.trim() + "\r";
+            if (!trimmed.equals(text)) {
+              text = trimmed;
+              cm.setValue(text);
+            }
+          }
           final int g = cm.changeGeneration(false);
           ChangeEditApi.put(revision.getParentKey().get(), path, text,
               new GerritCallback<VoidResult>() {

gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java

@@ -215,6 +215,7 @@ public class ChangeEditModifier {
   public RefUpdate.Result modifyMessage(ChangeEdit edit, String msg)
       throws AuthException, InvalidChangeOperationException, IOException,
       UnchangedCommitMessageException {
+    msg = msg.trim() + "\n";
     checkState(!Strings.isNullOrEmpty(msg), "message cannot be null");
     if (!currentUser.get().isIdentifiedUser()) {
       throw new AuthException("Authentication required");