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
diff --git a/Documentation/user-inline-edit.txt b/Documentation/user-inline-edit.txt index 5ad6b39f..49821af 100644 --- a/Documentation/user-inline-edit.txt +++ b/Documentation/user-inline-edit.txt
@@ -60,6 +60,8 @@ 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
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/edit/ChangeEditIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/edit/ChangeEditIT.java index 8a02831..4299896 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/edit/ChangeEditIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/edit/ChangeEditIT.java
@@ -296,7 +296,7 @@ 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 @@ .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 @@ .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 @@ 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 @@ 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,
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/editor/EditScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/editor/EditScreen.java index dd36657..efdbe44 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/editor/EditScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/editor/EditScreen.java
@@ -453,6 +453,13 @@ 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>() {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java index 67de914..8f73793 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java
@@ -215,6 +215,7 @@ 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");