Reject changes with Change-Id above footer

Documentation/user-change-id.txt explains:

	To be picked up by Gerrit, a Change-Id line must be in the
	footer (last paragraph) of a commit message, and may be mixed
	together with Signed-off-by, Acked-by, or other such lines.

It is easy to forget to do that, producing a commit message like

	Do something great

	Change-Id: I55ca6286e3e4f4fba5d0448333fa99fc5a404a73

	Bug: 12345

Because of the blank line between the Change-Id and other trailer
line, the Change-Id is not picked up by Gerrit.  Worse, if the
repository is configured with "Require Change-Id in commit message"
set to false, then Gerrit comes up with its own change-id for the
change.  If I amend my commit in response to review comments and push
for review again, Gerrit comes up with another change-id, so my
amended version of the change shows up as a new change.

This sequence of events can be avoided by setting "Require Change-Id"
to true, but some projects like to review changes from upstreams that
don't use Gerrit.  Fortunately, even these projects don't need changes
with a Change-Id before the footer.  Treat them as an error
unconditionally, with the same advice to the user about how to clean up
that is already shown in Require Change-Id mode:

	ERROR: Change-Id must be in message footer

	Hint: run
	  git commit --amend
	and move 'Change-Id: Ixxx..' to the bottom on a separate line

As a side benefit, treating this as its own error type also allows
simplifying the error reporting for changes with no Change-Id.

Reported-by: Paul Crowley <paulcrowley@google.com>
Change-Id: Ia5d43370e075d6fc13cd9b22031436f1138e90c0

Documentation/error-changeid-above-footer.txt

@@ -0,0 +1,31 @@
+= commit xxxxxxx: Change-Id must be in message footer
+
+With this error message, Gerrit rejects a push of a commit to a project
+if the commit message of the pushed commit contains a Change-Id line that
+is not in the footer (the last paragraph).
+
+To be picked up by Gerrit, a Change-Id must be in the last paragraph
+of a commit message. For details, see link:user-changeid.html[Change-Id Lines].
+
+You can see the commit messages for existing commits in the history
+by doing a link:http://www.kernel.org/pub/software/scm/git/docs/git-log.html[git log].
+
+
+== Change-Id is contained in the commit message but not in the last paragraph
+
+If the Change-Id is contained in the commit message but not in its
+last paragraph, you have to update the commit message and move the
+Change-Id into the last paragraph. How to update the commit message
+is explained link:error-push-fails-due-to-commit-message.html[here].
+
+To avoid confusion due to a Change-Id that was meant to be picked up by
+Gerrit not being picked up, this is an error whether or not the project
+is configured to always require a Change-Id in the commit message.
+
+
+GERRIT
+------
+Part of link:error-messages.html[Gerrit Error Messages]
+
+SEARCHBOX
+---------

Documentation/error-messages.txt

@@ -18,6 +18,7 @@ occurring and what can be done to solve it.
 * link:error-invalid-changeid-line.html[invalid Change-Id line format in commit message footer]
 * link:error-invalid-committer.html[invalid committer]
 * link:error-missing-changeid.html[missing Change-Id in commit message footer]
+* link:error-changeid-above-footer.html[Change-Id must be in commit message footer]
 * link:error-missing-subject.html[missing subject; Change-Id must be in commit message footer]
 * link:error-multiple-changeid-lines.html[multiple Change-Id lines in commit message footer]
 * link:error-no-common-ancestry.html[no common ancestry]

Documentation/error-missing-changeid.txt

@@ -3,13 +3,7 @@
 With this error message Gerrit rejects to push a commit to a project
 which is configured to always require a Change-Id in the commit
 message if the commit message of the pushed commit does not contain
-a Change-Id in the footer (the last paragraph).
-
-This error may happen for different reasons:
-
-. missing Change-Id in the commit message
-. Change-Id is contained in the commit message but not in the last
-  paragraph
+a Change-Id.
 
 You can see the commit messages for existing commits in the history
 by doing a link:http://www.kernel.org/pub/software/scm/git/docs/git-log.html[git log].
@@ -38,17 +32,6 @@ insert it into the commit message. How to update the commit message
 is explained link:error-push-fails-due-to-commit-message.html[here].
 
 
-== Change-Id is contained in the commit message but not in the last paragraph
-
-To be picked up by Gerrit, a Change-Id must be in the last paragraph
-of a commit message, for details, see link:user-changeid.html[Change-Id Lines].
-
-If the Change-Id is contained in the commit message but not in its
-last paragraph you have to update the commit message and move the
-Change-Id into the last paragraph. How to update the commit message
-is explained link:error-push-fails-due-to-commit-message.html[here].
-
-
 GERRIT
 ------
 Part of link:error-messages.html[Gerrit Error Messages]

java/com/google/gerrit/server/git/validators/CommitValidators.java

@@ -21,9 +21,7 @@ import static com.google.gerrit.reviewdb.client.RefNames.REFS_CONFIG;
 import static java.util.stream.Collectors.toList;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.Iterables;
 import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.common.FooterConstants;
 import com.google.gerrit.common.Nullable;
@@ -246,6 +244,7 @@ public class CommitValidators {
     private static final String MISSING_CHANGE_ID_MSG = "missing Change-Id in message footer";
     private static final String MISSING_SUBJECT_MSG =
         "missing subject; Change-Id must be in message footer";
+    private static final String CHANGE_ID_ABOVE_FOOTER_MSG = "Change-Id must be in message footer";
     private static final String MULTIPLE_CHANGE_ID_MSG =
         "multiple Change-Id lines in message footer";
     private static final String INVALID_CHANGE_ID_MSG =
@@ -295,8 +294,20 @@ public class CommitValidators {
             && CHANGE_ID.matcher(shortMsg.substring(CHANGE_ID_PREFIX.length()).trim()).matches()) {
           throw new CommitValidationException(MISSING_SUBJECT_MSG);
         }
+        if (commit.getFullMessage().contains("\n" + CHANGE_ID_PREFIX)) {
+          messages.add(
+              new CommitValidationMessage(
+                  CHANGE_ID_ABOVE_FOOTER_MSG
+                      + "\n"
+                      + "\n"
+                      + "Hint: run\n"
+                      + "  git commit --amend\n"
+                      + "and move 'Change-Id: Ixxx..' to the bottom on a separate line\n",
+                  Type.ERROR));
+          throw new CommitValidationException(CHANGE_ID_ABOVE_FOOTER_MSG, messages);
+        }
         if (projectState.is(BooleanProjectConfig.REQUIRE_CHANGE_ID)) {
-          messages.add(getMissingChangeIdErrorMsg(MISSING_CHANGE_ID_MSG, commit));
+          messages.add(getMissingChangeIdErrorMsg(MISSING_CHANGE_ID_MSG));
           throw new CommitValidationException(MISSING_CHANGE_ID_MSG, messages);
         }
       } else if (idList.size() > 1) {
@@ -306,7 +317,7 @@ public class CommitValidators {
         // Reject Change-Ids with wrong format and invalid placeholder ID from
         // Egit (I0000000000000000000000000000000000000000).
         if (!CHANGE_ID.matcher(v).matches() || v.matches("^I00*$")) {
-          messages.add(getMissingChangeIdErrorMsg(INVALID_CHANGE_ID_MSG, receiveEvent.commit));
+          messages.add(getMissingChangeIdErrorMsg(INVALID_CHANGE_ID_MSG));
           throw new CommitValidationException(INVALID_CHANGE_ID_MSG, messages);
         }
         if (change != null && !v.equals(change.getKey().get())) {
@@ -322,32 +333,17 @@ public class CommitValidators {
           || NEW_PATCHSET_PATTERN.matcher(event.command.getRefName()).matches();
     }
 
-    private CommitValidationMessage getMissingChangeIdErrorMsg(String errMsg, RevCommit c) {
-      StringBuilder sb = new StringBuilder();
-      sb.append(errMsg).append("\n");
-
-      boolean hinted = false;
-      if (c.getFullMessage().contains(CHANGE_ID_PREFIX)) {
-        String lastLine = Iterables.getLast(Splitter.on('\n').split(c.getFullMessage()), "");
-        if (!lastLine.contains(CHANGE_ID_PREFIX)) {
-          hinted = true;
-          sb.append("\n")
-              .append("Hint: run\n")
-              .append("  git commit --amend\n")
-              .append("and move 'Change-Id: Ixxx..' to the bottom on a separate line\n");
-        }
-      }
-
-      // Print only one hint to avoid overwhelming the user.
-      if (!hinted) {
-        sb.append("\nHint: to automatically insert a Change-Id, install the hook:\n")
-            .append(getCommitMessageHookInstallationHint())
-            .append("\n")
-            .append("and then amend the commit:\n")
-            .append("  git commit --amend --no-edit\n")
-            .append("Finally, push your changes again\n");
-      }
-      return new CommitValidationMessage(sb.toString(), Type.ERROR);
+    private CommitValidationMessage getMissingChangeIdErrorMsg(String errMsg) {
+      return new CommitValidationMessage(
+          errMsg
+              + "\n"
+              + "\nHint: to automatically insert a Change-Id, install the hook:\n"
+              + getCommitMessageHookInstallationHint()
+              + "\n"
+              + "and then amend the commit:\n"
+              + "  git commit --amend --no-edit\n"
+              + "Finally, push your changes again\n",
+          Type.ERROR);
     }
 
     private String getCommitMessageHookInstallationHint() {

javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java

@@ -1555,6 +1555,32 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
     pushForReviewOk(testRepo);
   }
 
+  @Test
+  public void pushWithChangeIdAboveFooter() throws Exception {
+    testPushWithChangeIdAboveFooter();
+  }
+
+  @Test
+  public void pushWithChangeIdAboveFooterWithCreateNewChangeForAllNotInTarget() throws Exception {
+    enableCreateNewChangeForAllNotInTarget();
+    testPushWithChangeIdAboveFooter();
+  }
+
+  private void testPushWithChangeIdAboveFooter() throws Exception {
+    RevCommit c =
+        createCommit(
+            testRepo,
+            PushOneCommit.SUBJECT
+                + "\n\n"
+                + "Change-Id: Ied70ea827f5bf968f1f6aaee6594e07c846d217a\n\n"
+                + "More text, uh oh.\n");
+    assertThat(GitUtil.getChangeId(testRepo, c)).isEmpty();
+    pushForReviewRejected(testRepo, "Change-Id must be in message footer");
+
+    setRequireChangeId(InheritableBoolean.FALSE);
+    pushForReviewRejected(testRepo, "Change-Id must be in message footer");
+  }
+
   @Test
   public void errorMessageFormat() throws Exception {
     RevCommit c = createCommit(testRepo, "Message without Change-Id");