Merge "Merge branch 'stable-3.2' into stable-3.3" into stable-3.3

Documentation/dev-e2e-tests.txt

@@ -232,6 +232,17 @@ than the default, say `0.8`, will make scenarios wait somewhat less than how the
 Scenario development is often done using locally running Gerrit systems under test, which are
 sometimes dockerized.
 
+==== Number of users
+
+The `number_of_users` property can be used to scale scenario steps to run with the specified number
+of concurrent users. The value of this property remains `1` by default. For example, this sets the
+number of concurrent users to 10:
+
+* `-Dcom.google.gerrit.scenarios.number_of_users=10`
+
+This will make scenarios that support the `number_of_users` property to inject that many users
+concurrently for load testing.
+
 == How to run tests
 
 Run all tests:

e2e-tests/README

@@ -0,0 +1,53 @@
+#!/bin/bash
+#
+# Example usage only-
+# 1. Optional: replace test@mail.com below with your own, reachable locally.
+# 2. Use the '>>' operator below instead to not overwrite your known_hosts; keep '>' otherwise.
+# 3. Note that appending as proposed above may potentially repeat the same line multiple times.
+# 4. Init your local Gerrit test site then start it; you may refer to [1] below.
+# 5. Set GIT_HTTP_PASSWORD below to yours, from [2].
+# 6. Change to this directory to execute ./README (this executable file) in its own terminal.
+# 7. Install sbt if missing, based on your operating system; re-run to compile.
+# 8. Optional: add the below generated (displayed) key to your local admin user [3].
+# 9. Otherwise keep the lines below that use your existing user ssh keys for admin testing.
+# 10. This script assumes the google-sourced version of the example json file [4].
+# 11. If running that scenario locally as below reports authentication failures, [4] may be a fork.
+# 12. Uncomment any one of the below sbt commands at will; you may add some locally.
+# 13. See [5] for how to start using JAVA_OPTS below; you may leave it empty for these sbt commands.
+# 14. You can initialize an IDE sbt (Scala) project from/in this root folder; see [6].
+#
+# [1] https://gerrit-review.googlesource.com/Documentation/dev-readme.html#init
+# [2] http://localhost:8080/settings/#HTTPCredentials
+# [3] http://localhost:8080/settings/#SSHKeys
+# [4] ./src/test/resources/data/com/google/gerrit/scenarios/CloneUsingBothProtocols.json
+# [5] https://gerrit-review.googlesource.com/Documentation/dev-e2e-tests.html#_environment_properties
+# [6] https://gerrit-review.googlesource.com/Documentation/dev-e2e-tests.html#_ide_intellij
+
+# DO NOT change this (assumed) directory; force-removed *recursively* below!
+gatlingGitKeys=/tmp/ssh-keys
+
+userSshDir=$HOME/.ssh
+
+# Comment this group of lines out if willing to generate other keys as below.
+rm -f $gatlingGitKeys
+ln -s "$userSshDir" $gatlingGitKeys
+
+# Comment this group of lines out if keys already generated, as either below or above.
+#rm -fr $gatlingGitKeys
+#mkdir $gatlingGitKeys
+#ssh-keygen -m PEM -t rsa -C "test@mail.com" -f $gatlingGitKeys/id_rsa
+
+ssh-keyscan -t rsa -p 29418 localhost > "$userSshDir"/known_hosts
+cat $gatlingGitKeys/id_rsa.pub
+
+export GIT_HTTP_USERNAME="admin"
+export GIT_HTTP_PASSWORD="TODO"
+export JAVA_OPTS="\
+"
+#-Dx=y \
+
+#sbt clean
+#sbt update
+sbt compile
+#sbt "gatling:testOnly com.google.gerrit.scenarios.CloneUsingBothProtocols"
+#sbt "gatling:lastReport"

e2e-tests/src/test/resources/data/com/google/gerrit/scenarios/CreateBranch-body.json

@@ -0,0 +1,3 @@
+{
+  "revision": "master"
+}

e2e-tests/src/test/resources/data/com/google/gerrit/scenarios/CreateBranch.json

@@ -0,0 +1,6 @@
+[
+  {
+    "url": "HTTP_SCHEME://HOSTNAME:HTTP_PORT/a/projects/PROJECT/branches/",
+    "project": "PROJECT"
+  }
+]

e2e-tests/src/test/scala/com/google/gerrit/scenarios/CloneUsingBothProtocols.scala

@@ -21,9 +21,9 @@ import io.gatling.core.structure.ScenarioBuilder
 import scala.concurrent.duration._
 
 class CloneUsingBothProtocols extends GitSimulation {
-  private val data: FeederBuilder = jsonFile(resource).convert(keys).queue
+  private val data: FeederBuilder = jsonFile(resource).convert(keys).circular
   private val projectName = className
-  private val duration = 2
+  private val duration = 2 * numberOfUsers
 
   override def replaceOverride(in: String): String = {
     replaceKeyWith("_project", projectName, in)
@@ -43,7 +43,7 @@ class CloneUsingBothProtocols extends GitSimulation {
     ),
     test.inject(
       nothingFor(stepWaitTime(this) seconds),
-      constantUsersPerSec(single) during (duration seconds)
+      constantUsersPerSec(numberOfUsers) during (duration seconds)
     ).protocols(gitProtocol),
     deleteProject.test.inject(
       nothingFor(stepWaitTime(deleteProject) + duration seconds),

e2e-tests/src/test/scala/com/google/gerrit/scenarios/CreateBranch.scala

@@ -0,0 +1,56 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.scenarios
+
+import io.gatling.core.Predef.{atOnceUsers, _}
+import io.gatling.core.feeder.FeederBuilder
+import io.gatling.core.structure.ScenarioBuilder
+import io.gatling.http.Predef._
+
+import scala.concurrent.duration._
+
+class CreateBranch extends ProjectSimulation {
+  private val data: FeederBuilder = jsonFile(resource).convert(keys).circular
+  private val branchIdKey = "branchId"
+  private var counter = 0
+
+  private val test: ScenarioBuilder = scenario(uniqueName)
+      .feed(data)
+      .exec(session => {
+        counter += 1
+        session.set(branchIdKey, "branch-" + counter)
+      })
+      .exec(http(uniqueName)
+          .post("${url}${" + branchIdKey + "}")
+          .body(ElFileBody(body)).asJson)
+
+  private val createProject = new CreateProject(projectName)
+  private val deleteProject = new DeleteProject(projectName)
+
+  setUp(
+    createProject.test.inject(
+      nothingFor(stepWaitTime(createProject) seconds),
+      atOnceUsers(single)
+    ),
+    test.inject(
+      nothingFor(stepWaitTime(this) seconds),
+      atOnceUsers(numberOfUsers)
+    ),
+    deleteProject.test.inject(
+      nothingFor(stepWaitTime(deleteProject) seconds),
+      atOnceUsers(single)
+    ),
+  ).protocols(httpProtocol)
+}

e2e-tests/src/test/scala/com/google/gerrit/scenarios/CreateChange.scala

@@ -19,12 +19,14 @@ import io.gatling.core.feeder.FeederBuilder
 import io.gatling.core.structure.ScenarioBuilder
 import io.gatling.http.Predef._
 
+import scala.collection.mutable
 import scala.concurrent.duration._
 
 class CreateChange extends ProjectSimulation {
-  private val data: FeederBuilder = jsonFile(resource).convert(keys).queue
+  private val data: FeederBuilder = jsonFile(resource).convert(keys).circular
   private val numberKey = "_number"
   var number = 0
+  var numbers: mutable.Queue[Int] = mutable.Queue[Int]()
 
   override def relativeRuntimeWeight = 2
 
@@ -40,6 +42,7 @@ class CreateChange extends ProjectSimulation {
           .check(regex("\"" + numberKey + "\":(\\d+),").saveAs(numberKey)))
       .exec(session => {
         number = session(numberKey).as[Int]
+        numbers += number
         session
       })
 
@@ -54,11 +57,11 @@ class CreateChange extends ProjectSimulation {
     ),
     test.inject(
       nothingFor(stepWaitTime(this) seconds),
-      atOnceUsers(single)
+      atOnceUsers(numberOfUsers)
     ),
     deleteChange.test.inject(
       nothingFor(stepWaitTime(deleteChange) seconds),
-      atOnceUsers(single)
+      atOnceUsers(numberOfUsers)
     ),
     deleteProject.test.inject(
       nothingFor(stepWaitTime(deleteProject) seconds),

e2e-tests/src/test/scala/com/google/gerrit/scenarios/DeleteChange.scala

@@ -20,7 +20,7 @@ import io.gatling.core.structure.ScenarioBuilder
 import io.gatling.http.Predef.http
 
 class DeleteChange extends GerritSimulation {
-  private val data: FeederBuilder = jsonFile(resource).convert(keys).queue
+  private val data: FeederBuilder = jsonFile(resource).convert(keys).circular
   private var createChange: Option[CreateChange] = None
 
   override def relativeRuntimeWeight = 2
@@ -34,7 +34,7 @@ class DeleteChange extends GerritSimulation {
       .feed(data)
       .exec(session => {
         if (createChange.nonEmpty) {
-          session.set("number", createChange.get.number)
+          session.set("number", createChange.get.numbers.dequeue())
         } else {
           session
         }

e2e-tests/src/test/scala/com/google/gerrit/scenarios/GerritSimulation.scala

@@ -34,6 +34,7 @@ class GerritSimulation extends Simulation {
   protected val uniqueName: String = className + "-" + hashCode()
   protected val single = 1
 
+  val numberOfUsers: Int = replaceProperty("number_of_users", single).toInt
   val replicationDelay: Int = replaceProperty("replication_delay", 15).toInt
   private val powerFactor = replaceProperty("power_factor", 1.0).toDouble
   protected val SecondsPerWeightUnit = 2

java/com/google/gerrit/httpd/restapi/RestApiServlet.java

@@ -47,7 +47,6 @@ import static javax.servlet.http.HttpServletResponse.SC_OK;
 import static javax.servlet.http.HttpServletResponse.SC_PRECONDITION_FAILED;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.CharMatcher;
 import com.google.common.base.Joiner;
 import com.google.common.base.Splitter;
 import com.google.common.base.Strings;
@@ -140,7 +139,6 @@ import com.google.gson.Gson;
 import com.google.gson.GsonBuilder;
 import com.google.gson.JsonElement;
 import com.google.gson.JsonParseException;
-import com.google.gson.JsonPrimitive;
 import com.google.gson.stream.JsonReader;
 import com.google.gson.stream.JsonToken;
 import com.google.gson.stream.JsonWriter;
@@ -1863,10 +1861,6 @@ public class RestApiServlet extends HttpServlet {
   static long replyText(
       @Nullable HttpServletRequest req, HttpServletResponse res, boolean allowTracing, String text)
       throws IOException {
-    if ((req == null || isRead(req)) && isMaybeHTML(text)) {
-      return replyJson(
-          req, res, allowTracing, ImmutableListMultimap.of("pp", "0"), new JsonPrimitive(text));
-    }
     if (!text.endsWith("\n")) {
       text += "\n";
     }
@@ -1876,10 +1870,6 @@ public class RestApiServlet extends HttpServlet {
     return replyBinaryResult(req, res, BinaryResult.create(text).setContentType(PLAIN_TEXT));
   }
 
-  private static boolean isMaybeHTML(String text) {
-    return CharMatcher.anyOf("<&").matchesAnyOf(text);
-  }
-
   private static boolean acceptsGzip(HttpServletRequest req) {
     if (req != null) {
       String accepts = req.getHeader(HttpHeaders.ACCEPT_ENCODING);

java/com/google/gerrit/server/ChangeUtil.java

@@ -22,7 +22,10 @@ import com.google.common.io.BaseEncoding;
 import com.google.gerrit.common.FooterConstants;
 import com.google.gerrit.entities.Change;
 import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.server.config.UrlFormatter;
+import com.google.gerrit.server.util.CommitMessageUtil;
 import com.google.inject.Singleton;
 import java.io.IOException;
 import java.security.SecureRandom;
@@ -34,6 +37,8 @@ import java.util.Set;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import java.util.stream.Stream;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
@@ -109,6 +114,37 @@ public class ChangeUtil {
         id);
   }
 
+  /**
+   * Make sure that the change commit message has a correct footer.
+   *
+   * @param requireChangeId true if Change-Id is a mandatory footer for the project
+   * @param currentChangeId current Change-Id value before the commit message is updated
+   * @param newCommitMessage new commit message for the change
+   * @throws ResourceConflictException if the new commit message has a missing or invalid Change-Id
+   * @throws BadRequestException if the new commit message is null or empty
+   */
+  public static void ensureChangeIdIsCorrect(
+      boolean requireChangeId, String currentChangeId, String newCommitMessage)
+      throws ResourceConflictException, BadRequestException {
+    RevCommit revCommit =
+        RevCommit.parse(
+            Constants.encode("tree " + ObjectId.zeroId().name() + "\n\n" + newCommitMessage));
+
+    // Check that the commit message without footers is not empty
+    CommitMessageUtil.checkAndSanitizeCommitMessage(revCommit.getShortMessage());
+
+    List<String> changeIdFooters = revCommit.getFooterLines(FooterConstants.CHANGE_ID);
+    if (requireChangeId && changeIdFooters.isEmpty()) {
+      throw new ResourceConflictException("missing Change-Id footer");
+    }
+    if (!changeIdFooters.isEmpty() && !changeIdFooters.get(0).equals(currentChangeId)) {
+      throw new ResourceConflictException("wrong Change-Id footer");
+    }
+    if (changeIdFooters.size() > 1) {
+      throw new ResourceConflictException("multiple Change-Id footers");
+    }
+  }
+
   public static String status(Change c) {
     return c != null ? c.getStatus().name().toLowerCase() : "deleted";
   }

java/com/google/gerrit/server/edit/ChangeEditModifier.java

@@ -17,6 +17,7 @@ package com.google.gerrit.server.edit;
 import static com.google.gerrit.server.project.ProjectCache.illegalState;
 
 import com.google.common.base.Charsets;
+import com.google.gerrit.entities.BooleanProjectConfig;
 import com.google.gerrit.entities.Change;
 import com.google.gerrit.entities.PatchSet;
 import com.google.gerrit.entities.Project;
@@ -367,8 +368,15 @@ public class ChangeEditModifier {
     PatchSet basePatchset = modificationTarget.getBasePatchset();
     RevCommit basePatchsetCommit = NoteDbEdits.lookupCommit(repository, basePatchset.commitId());
 
+    boolean changeIdRequired =
+        projectCache
+            .get(notes.getChange().getProject())
+            .orElseThrow(illegalState(notes.getChange().getProject()))
+            .is(BooleanProjectConfig.REQUIRE_CHANGE_ID);
+    String currentChangeId = notes.getChange().getKey().get();
     String newCommitMessage =
-        createNewCommitMessage(editBehavior, commitModification, commitToModify);
+        createNewCommitMessage(
+            changeIdRequired, currentChangeId, editBehavior, commitModification, commitToModify);
     newCommitMessage = editBehavior.mergeCommitMessageIfNecessary(newCommitMessage, commitToModify);
 
     Optional<ChangeEdit> unmodifiedEdit =
@@ -464,8 +472,12 @@ public class ChangeEditModifier {
   }
 
   private String createNewCommitMessage(
-      EditBehavior editBehavior, CommitModification commitModification, RevCommit commitToModify)
+      boolean requireChangeId,
-      throws InvalidChangeOperationException, BadRequestException {
+      String currentChangeId,
+      EditBehavior editBehavior,
+      CommitModification commitModification,
+      RevCommit commitToModify)
+      throws InvalidChangeOperationException, BadRequestException, ResourceConflictException {
     if (!commitModification.newCommitMessage().isPresent()) {
       return editBehavior.getUnmodifiedCommitMessage(commitToModify);
     }
@@ -479,6 +491,8 @@ public class ChangeEditModifier {
           "New commit message cannot be same as existing commit message");
     }
 
+    ChangeUtil.ensureChangeIdIsCorrect(requireChangeId, currentChangeId, newCommitMessage);
+
     return newCommitMessage;
   }
 

java/com/google/gerrit/server/restapi/change/PutMessage.java

@@ -16,7 +16,6 @@ package com.google.gerrit.server.restapi.change;
 
 import static com.google.gerrit.server.project.ProjectCache.illegalState;
 
-import com.google.gerrit.common.FooterConstants;
 import com.google.gerrit.entities.BooleanProjectConfig;
 import com.google.gerrit.entities.PatchSet;
 import com.google.gerrit.extensions.api.changes.NotifyHandling;
@@ -51,11 +50,9 @@ import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import java.io.IOException;
 import java.sql.Timestamp;
-import java.util.List;
 import java.util.TimeZone;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.CommitBuilder;
-import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.PersonIdent;
@@ -116,14 +113,13 @@ public class PutMessage implements RestModifyView<ChangeResource, CommitMessageI
     String sanitizedCommitMessage = CommitMessageUtil.checkAndSanitizeCommitMessage(input.message);
 
     ensureCanEditCommitMessage(resource.getNotes());
-    sanitizedCommitMessage =
+    ChangeUtil.ensureChangeIdIsCorrect(
-        ensureChangeIdIsCorrect(
+        projectCache
-            projectCache
+            .get(resource.getProject())
-                .get(resource.getProject())
+            .orElseThrow(illegalState(resource.getProject()))
-                .orElseThrow(illegalState(resource.getProject()))
+            .is(BooleanProjectConfig.REQUIRE_CHANGE_ID),
-                .is(BooleanProjectConfig.REQUIRE_CHANGE_ID),
+        resource.getChange().getKey().get(),
-            resource.getChange().getKey().get(),
+        sanitizedCommitMessage);
-            sanitizedCommitMessage);
 
     try (Repository repository = repositoryManager.openRepository(resource.getProject());
         RevWalk revWalk = new RevWalk(repository);
@@ -204,33 +200,4 @@ public class PutMessage implements RestModifyView<ChangeResource, CommitMessageI
       throw new AuthException("modifying commit message not permitted", denied);
     }
   }
-
-  private String ensureChangeIdIsCorrect(
-      boolean requireChangeId, String currentChangeId, String newCommitMessage)
-      throws ResourceConflictException, BadRequestException {
-    RevCommit revCommit =
-        RevCommit.parse(
-            Constants.encode("tree " + ObjectId.zeroId().name() + "\n\n" + newCommitMessage));
-
-    // Check that the commit message without footers is not empty
-    CommitMessageUtil.checkAndSanitizeCommitMessage(revCommit.getShortMessage());
-
-    List<String> changeIdFooters = ChangeUtil.getChangeIdsFromFooter(revCommit, urlFormatter.get());
-    if (!changeIdFooters.isEmpty() && !changeIdFooters.get(0).equals(currentChangeId)) {
-      throw new ResourceConflictException("wrong Change-Id footer");
-    }
-
-    if (requireChangeId && revCommit.getFooterLines().isEmpty()) {
-      // sanitization always adds '\n' at the end.
-      newCommitMessage += "\n";
-    }
-
-    if (requireChangeId && changeIdFooters.isEmpty()) {
-      newCommitMessage += FooterConstants.CHANGE_ID.getName() + ": " + currentChangeId + "\n";
-    } else if (changeIdFooters.size() > 1) {
-      throw new ResourceConflictException("multiple Change-Id footers");
-    }
-
-    return newCommitMessage;
-  }
 }

javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -3666,16 +3666,6 @@ public class ChangeIT extends AbstractDaemonTest {
     assertThat(getCommitMessage(r.getChangeId())).isEqualTo(newMessage);
   }
 
-  @Test
-  public void changeCommitMessageWithNoChangeIdRetainsChangeID() throws Exception {
-    PushOneCommit.Result r = createChange();
-    assertThat(getCommitMessage(r.getChangeId()))
-        .isEqualTo("test commit\n\nChange-Id: " + r.getChangeId() + "\n");
-    gApi.changes().id(r.getChangeId()).setMessage("modified commit\n");
-    assertThat(getCommitMessage(r.getChangeId()))
-        .isEqualTo("modified commit\n\nChange-Id: " + r.getChangeId() + "\n");
-  }
-
   @Test
   public void changeCommitMessageNullNotAllowed() throws Exception {
     PushOneCommit.Result r = createChange();

javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java

@@ -470,7 +470,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
     gApi.changes().id(changeId).edit().modifyFile(filePath, RawInputUtil.create(fileContent));
     gApi.changes().id(changeId).edit().publish();
     String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
-    gApi.changes().id(changeId).edit().modifyCommitMessage("An unchanged patchset");
+    gApi.changes().id(changeId).edit().modifyCommitMessage(updatedCommitMessage());
     gApi.changes().id(changeId).edit().publish();
 
     DiffInfo diffInfo =
@@ -492,7 +492,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
     gApi.changes().id(changeId).edit().modifyFile(filePath, RawInputUtil.create(fileContent));
     gApi.changes().id(changeId).edit().publish();
     String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
-    gApi.changes().id(changeId).edit().modifyCommitMessage("An unchanged patchset");
+    gApi.changes().id(changeId).edit().modifyCommitMessage(updatedCommitMessage());
     gApi.changes().id(changeId).edit().publish();
 
     DiffInfo diffInfo =
@@ -510,7 +510,7 @@ public class RevisionDiffIT extends AbstractDaemonTest {
     gApi.changes().id(changeId).edit().modifyFile(filePath, RawInputUtil.create(fileContent));
     gApi.changes().id(changeId).edit().publish();
     String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
-    gApi.changes().id(changeId).edit().modifyCommitMessage("An unchanged patchset");
+    gApi.changes().id(changeId).edit().modifyCommitMessage(updatedCommitMessage());
     gApi.changes().id(changeId).edit().publish();
 
     DiffInfo diffInfo =
@@ -2704,6 +2704,10 @@ public class RevisionDiffIT extends AbstractDaemonTest {
     assertThat(e).hasMessageThat().isEqualTo("edit not allowed as base");
   }
 
+  private String updatedCommitMessage() {
+    return "An unchanged patchset\n\nChange-Id: " + changeId;
+  }
+
   private void assertDiffForNewFile(
       PushOneCommit.Result pushResult, String path, String expectedContentSideB) throws Exception {
     DiffInfo diff =

javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java

@@ -1046,7 +1046,8 @@ public class RobotCommentsIT extends AbstractDaemonTest {
 
   @Test
   public void fixDoesNotModifyCommitMessageOfChangeEdit() throws Exception {
-    String changeEditCommitMessage = "This is the commit message of the change edit.\n";
+    String changeEditCommitMessage =
+        "This is the commit message of the change edit.\n\nChange-Id: " + changeId + "\n";
     gApi.changes().id(changeId).edit().modifyCommitMessage(changeEditCommitMessage);
 
     fixReplacementInfo.path = FILE_NAME;
@@ -1065,7 +1066,8 @@ public class RobotCommentsIT extends AbstractDaemonTest {
   @Test
   public void fixOnCommitMessageCanBeApplied() throws Exception {
     // Set a dedicated commit message.
-    String originalCommitMessage = "Line 1 of commit message\nLine 2 of commit message\n";
+    String footer = "\nChange-Id: " + changeId + "\n";
+    String originalCommitMessage = "Line 1 of commit message\nLine 2 of commit message\n" + footer;
     gApi.changes().id(changeId).edit().modifyCommitMessage(originalCommitMessage);
     gApi.changes().id(changeId).edit().publish();
 
@@ -1080,13 +1082,15 @@ public class RobotCommentsIT extends AbstractDaemonTest {
     gApi.changes().id(changeId).current().applyFix(fixId);
 
     String commitMessage = gApi.changes().id(changeId).edit().getCommitMessage();
-    assertThat(commitMessage).isEqualTo("Modified line\nLine 2 of commit message\n");
+    assertThat(commitMessage).isEqualTo("Modified line\nLine 2 of commit message\n" + footer);
   }
 
   @Test
   public void fixOnHeaderPartOfCommitMessageCannotBeApplied() throws Exception {
     // Set a dedicated commit message.
-    String originalCommitMessage = "Line 1 of commit message\nLine 2 of commit message\n";
+    String footer = "Change-Id: " + changeId;
+    String originalCommitMessage =
+        "Line 1 of commit message\nLine 2 of commit message\n" + "\n" + footer + "\n";
     gApi.changes().id(changeId).edit().modifyCommitMessage(originalCommitMessage);
     gApi.changes().id(changeId).edit().publish();
 
@@ -1108,8 +1112,9 @@ public class RobotCommentsIT extends AbstractDaemonTest {
   @Test
   public void fixContainingSeveralModificationsOfCommitMessageCanBeApplied() throws Exception {
     // Set a dedicated commit message.
+    String footer = "\nChange-Id: " + changeId + "\n";
     String originalCommitMessage =
-        "Line 1 of commit message\nLine 2 of commit message\nLine 3 of commit message\n";
+        "Line 1 of commit message\nLine 2 of commit message\nLine 3 of commit message\n" + footer;
     gApi.changes().id(changeId).edit().modifyCommitMessage(originalCommitMessage);
     gApi.changes().id(changeId).edit().publish();
 
@@ -1135,13 +1140,14 @@ public class RobotCommentsIT extends AbstractDaemonTest {
 
     String commitMessage = gApi.changes().id(changeId).edit().getCommitMessage();
     assertThat(commitMessage)
-        .isEqualTo("Modified line 1\nLine 2 of commit message\nModified line 3\n");
+        .isEqualTo("Modified line 1\nLine 2 of commit message\nModified line 3\n" + footer);
   }
 
   @Test
   public void fixModifyingTheCommitMessageAndAFileCanBeApplied() throws Exception {
     // Set a dedicated commit message.
-    String originalCommitMessage = "Line 1 of commit message\nLine 2 of commit message\n";
+    String footer = "\nChange-Id: " + changeId + "\n";
+    String originalCommitMessage = "Line 1 of commit message\nLine 2 of commit message\n" + footer;
     gApi.changes().id(changeId).edit().modifyCommitMessage(originalCommitMessage);
     gApi.changes().id(changeId).edit().publish();
 
@@ -1165,7 +1171,7 @@ public class RobotCommentsIT extends AbstractDaemonTest {
     gApi.changes().id(changeId).current().applyFix(fixId);
 
     String commitMessage = gApi.changes().id(changeId).edit().getCommitMessage();
-    assertThat(commitMessage).isEqualTo("Modified line 1\nLine 2 of commit message\n");
+    assertThat(commitMessage).isEqualTo("Modified line 1\nLine 2 of commit message\n" + footer);
     Optional<BinaryResult> file = gApi.changes().id(changeId).edit().getFile(FILE_NAME2);
     BinaryResultSubject.assertThat(file)
         .value()
@@ -1176,8 +1182,9 @@ public class RobotCommentsIT extends AbstractDaemonTest {
   @Test
   public void twoFixesOnCommitMessageCanBeAppliedOneAfterTheOther() throws Exception {
     // Set a dedicated commit message.
+    String footer = "\nChange-Id: " + changeId + "\n";
     String originalCommitMessage =
-        "Line 1 of commit message\nLine 2 of commit message\nLine 3 of commit message\n";
+        "Line 1 of commit message\nLine 2 of commit message\nLine 3 of commit message\n" + footer;
     gApi.changes().id(changeId).edit().modifyCommitMessage(originalCommitMessage);
     gApi.changes().id(changeId).edit().publish();
 
@@ -1206,14 +1213,17 @@ public class RobotCommentsIT extends AbstractDaemonTest {
 
     String commitMessage = gApi.changes().id(changeId).edit().getCommitMessage();
     assertThat(commitMessage)
-        .isEqualTo("Modified line 1\nLine 2 of commit message\nModified line 3\n");
+        .isEqualTo("Modified line 1\nLine 2 of commit message\nModified line 3\n" + footer);
   }
 
   @Test
   public void twoConflictingFixesOnCommitMessageCanNotBeAppliedOneAfterTheOther() throws Exception {
     // Set a dedicated commit message.
+    String footer = "Change-Id: " + changeId;
     String originalCommitMessage =
-        "Line 1 of commit message\nLine 2 of commit message\nLine 3 of commit message\n";
+        "Line 1 of commit message\nLine 2 of commit message\nLine 3 of commit message\n\n"
+            + footer
+            + "\n";
     gApi.changes().id(changeId).edit().modifyCommitMessage(originalCommitMessage);
     gApi.changes().id(changeId).edit().publish();
 
@@ -1379,8 +1389,10 @@ public class RobotCommentsIT extends AbstractDaemonTest {
 
   @Test
   public void getFixPreviewForCommitMsg() throws Exception {
+    String footer = "Change-Id: " + changeId;
     updateCommitMessage(
-        changeId, "Commit title\n\nCommit message line 1\nLine 2\nLine 3\nLast line\n");
+        changeId,
+        "Commit title\n\nCommit message line 1\nLine 2\nLine 3\nLast line\n\n" + footer + "\n");
     FixReplacementInfo commitMsgReplacement = new FixReplacementInfo();
     commitMsgReplacement.path = Patch.COMMIT_MSG;
     // The test assumes that the first 5 lines is a header.
@@ -1420,7 +1432,11 @@ public class RobotCommentsIT extends AbstractDaemonTest {
         .isEqualTo("Commit message line 1");
     assertThat(diff).content().element(1).linesOfA().containsExactly("Line 2");
     assertThat(diff).content().element(1).linesOfB().containsExactly("New content");
-    assertThat(diff).content().element(2).commonLines().containsExactly("Line 3", "Last line", "");
+    assertThat(diff)
+        .content()
+        .element(2)
+        .commonLines()
+        .containsExactly("Line 3", "Last line", "", footer, "");
   }
 
   private void updateCommitMessage(String changeId, String newCommitMessage) throws Exception {

javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java

@@ -310,12 +310,14 @@ public class ChangeEditIT extends AbstractDaemonTest {
   @Test
   public void updateCommitMessageByEditingMagicCommitMsgFile() throws Exception {
     createEmptyEditFor(changeId);
+    String updatedCommitMsg = "Foo Bar\n\nChange-Id: " + changeId + "\n";
     gApi.changes()
         .id(changeId)
         .edit()
-        .modifyFile(Patch.COMMIT_MSG, RawInputUtil.create("Foo Bar".getBytes(UTF_8)));
+        .modifyFile(Patch.COMMIT_MSG, RawInputUtil.create(updatedCommitMsg.getBytes(UTF_8)));
     assertThat(getEdit(changeId)).isPresent();
-    ensureSameBytes(getFileContentOfEdit(changeId, Patch.COMMIT_MSG), "Foo Bar\n".getBytes(UTF_8));
+    ensureSameBytes(
+        getFileContentOfEdit(changeId, Patch.COMMIT_MSG), updatedCommitMsg.getBytes(UTF_8));
   }
 
   @Test
@@ -345,6 +347,39 @@ public class ChangeEditIT extends AbstractDaemonTest {
     assertThat(commitMessage).isEqualTo(msg);
   }
 
+  @Test
+  public void updateMessageEditChangeIdShouldThrowResourceConflictException() throws Exception {
+    createEmptyEditFor(changeId);
+    String commitMessage = gApi.changes().id(changeId).edit().getCommitMessage();
+
+    ResourceConflictException thrown =
+        assertThrows(
+            ResourceConflictException.class,
+            () ->
+                gApi.changes()
+                    .id(changeId)
+                    .edit()
+                    .modifyCommitMessage(commitMessage.replaceAll(changeId, changeId2)));
+    assertThat(thrown).hasMessageThat().isEqualTo("wrong Change-Id footer");
+  }
+
+  @Test
+  public void updateMessageEditRemoveChangeIdShouldThrowResourceConflictException()
+      throws Exception {
+    createEmptyEditFor(changeId);
+    String commitMessage = gApi.changes().id(changeId).edit().getCommitMessage();
+
+    ResourceConflictException thrown =
+        assertThrows(
+            ResourceConflictException.class,
+            () ->
+                gApi.changes()
+                    .id(changeId)
+                    .edit()
+                    .modifyCommitMessage(commitMessage.replaceAll("(Change-Id:).*", "")));
+    assertThat(thrown).hasMessageThat().isEqualTo("missing Change-Id footer");
+  }
+
   @Test
   public void updateMessageNoChange() throws Exception {
     createEmptyEditFor(changeId);