Merge branch 'stable-2.13' into stable-2.14

* stable-2.13:
  CreateChange: Fix appending Signed-off-by line after Change-Id
  CreateChange: Only insert Change-Id if there isn't already one
  CreateChangeIT: Disable "Insert Signed-off-by" after test

Change-Id: Id0ec1fa964c1afd746f8e4fdc7bcd6f681677a62

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java

@@ -105,9 +105,32 @@ public class CreateChangeIT extends AbstractDaemonTest {
         "invalid Change-Id line format in commit message footer");
   }
 
+  @Test
+  public void createEmptyChange_InvalidSubject() throws Exception {
+    ChangeInput ci = newChangeInput(ChangeStatus.NEW);
+    ci.subject = "Change-Id: I1234000000000000000000000000000000000000";
+    assertCreateFails(
+        ci,
+        ResourceConflictException.class,
+        "missing subject; Change-Id must be in commit message footer");
+  }
+
   @Test
   public void createNewChange() throws Exception {
-    assertCreateSucceeds(newChangeInput(ChangeStatus.NEW));
+    ChangeInfo info = assertCreateSucceeds(newChangeInput(ChangeStatus.NEW));
+    assertThat(info.revisions.get(info.currentRevision).commit.message)
+        .contains("Change-Id: " + info.changeId);
+  }
+
+  @Test
+  public void createNewChangeWithChangeId() throws Exception {
+    ChangeInput ci = newChangeInput(ChangeStatus.NEW);
+    String changeId = "I1234000000000000000000000000000000000000";
+    String changeIdLine = "Change-Id: " + changeId;
+    ci.subject = "Subject\n\n" + changeIdLine;
+    ChangeInfo info = assertCreateSucceeds(ci);
+    assertThat(info.changeId).isEqualTo(changeId);
+    assertThat(info.revisions.get(info.currentRevision).commit.message).contains(changeIdLine);
   }
 
   @Test
@@ -135,13 +158,38 @@ public class CreateChangeIT extends AbstractDaemonTest {
 
   @Test
   public void createNewChangeSignedOffByFooter() throws Exception {
-    setSignedOffByFooter();
+    setSignedOffByFooter(true);
-    ChangeInfo info = assertCreateSucceeds(newChangeInput(ChangeStatus.NEW));
+    try {
-    String message = info.revisions.get(info.currentRevision).commit.message;
+      ChangeInfo info = assertCreateSucceeds(newChangeInput(ChangeStatus.NEW));
-    assertThat(message)
+      String message = info.revisions.get(info.currentRevision).commit.message;
-        .contains(
+      assertThat(message)
-            String.format(
+          .contains(
-                "%sAdministrator <%s>", SIGNED_OFF_BY_TAG, admin.getIdent().getEmailAddress()));
+              String.format(
+                  "%sAdministrator <%s>", SIGNED_OFF_BY_TAG, admin.getIdent().getEmailAddress()));
+    } finally {
+      setSignedOffByFooter(false);
+    }
+  }
+
+  @Test
+  public void createNewChangeSignedOffByFooterWithChangeId() throws Exception {
+    setSignedOffByFooter(true);
+    try {
+      ChangeInput ci = newChangeInput(ChangeStatus.NEW);
+      String changeId = "I1234000000000000000000000000000000000000";
+      String changeIdLine = "Change-Id: " + changeId;
+      ci.subject = "Subject\n\n" + changeIdLine;
+      ChangeInfo info = assertCreateSucceeds(ci);
+      assertThat(info.changeId).isEqualTo(changeId);
+      String message = info.revisions.get(info.currentRevision).commit.message;
+      assertThat(message).contains(changeIdLine);
+      assertThat(message)
+          .contains(
+              String.format(
+                  "%sAdministrator <%s>", SIGNED_OFF_BY_TAG, admin.getIdent().getEmailAddress()));
+    } finally {
+      setSignedOffByFooter(false);
+    }
   }
 
   @Test
@@ -302,7 +350,7 @@ public class CreateChangeIT extends AbstractDaemonTest {
     ChangeInfo out = gApi.changes().create(in).get();
     assertThat(out.project).isEqualTo(in.project);
     assertThat(out.branch).isEqualTo(in.branch);
-    assertThat(out.subject).isEqualTo(in.subject);
+    assertThat(out.subject).isEqualTo(in.subject.split("\n")[0]);
     assertThat(out.topic).isEqualTo(in.topic);
     assertThat(out.status).isEqualTo(in.status);
     assertThat(out.revisions).hasSize(1);
@@ -328,17 +376,21 @@ public class CreateChangeIT extends AbstractDaemonTest {
   }
 
   // TODO(davido): Expose setting of account preferences in the API
-  private void setSignedOffByFooter() throws Exception {
+  private void setSignedOffByFooter(boolean value) throws Exception {
     RestResponse r = adminRestSession.get("/accounts/" + admin.email + "/preferences");
     r.assertOK();
     GeneralPreferencesInfo i = newGson().fromJson(r.getReader(), GeneralPreferencesInfo.class);
-    i.signedOffBy = true;
+    i.signedOffBy = value;
 
     r = adminRestSession.put("/accounts/" + admin.email + "/preferences", i);
     r.assertOK();
     GeneralPreferencesInfo o = newGson().fromJson(r.getReader(), GeneralPreferencesInfo.class);
 
-    assertThat(o.signedOffBy).isTrue();
+    if (value) {
+      assertThat(o.signedOffBy).isTrue();
+    } else {
+      assertThat(o.signedOffBy).isNull();
+    }
   }
 
   private ChangeInput newMergeChangeInput(String targetBranch, String sourceRef, String strategy) {

gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.change;
 
 import static org.eclipse.jgit.lib.Constants.SIGNED_OFF_BY_TAG;
 
+import com.google.common.base.Joiner;
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Strings;
 import com.google.common.collect.Iterables;
@@ -227,13 +228,22 @@ public class CreateChange implements RestModifyView<TopLevelResource, ChangeInpu
       AccountState account = accountCache.get(me.getAccountId());
       GeneralPreferencesInfo info = account.getAccount().getGeneralPreferencesInfo();
 
-      ObjectId treeId = mergeTip == null ? emptyTreeId(oi) : mergeTip.getTree();
+      // Add a Change-Id line if there isn't already one
-      ObjectId id = ChangeIdUtil.computeChangeId(treeId, mergeTip, author, author, input.subject);
+      String commitMessage = input.subject;
-      String commitMessage = ChangeIdUtil.insertId(input.subject, id);
+      if (ChangeIdUtil.indexOfChangeId(commitMessage, "\n") == -1) {
+        ObjectId treeId = mergeTip == null ? emptyTreeId(oi) : mergeTip.getTree();
+        ObjectId id = ChangeIdUtil.computeChangeId(treeId, mergeTip, author, author, commitMessage);
+        commitMessage = ChangeIdUtil.insertId(commitMessage, id);
+      }
+
       if (Boolean.TRUE.equals(info.signedOffBy)) {
-        commitMessage +=
+        commitMessage =
-            String.format(
+            Joiner.on("\n")
-                "%s%s", SIGNED_OFF_BY_TAG, account.getAccount().getNameEmail(anonymousCowardName));
+                .join(
+                    commitMessage.trim(),
+                    String.format(
+                        "%s%s",
+                        SIGNED_OFF_BY_TAG, account.getAccount().getNameEmail(anonymousCowardName)));
       }
 
       RevCommit c;