Merge branch 'stable-2.16' into stable-3.0

* stable-2.16:
  Generate Change-Ids randomly instead of computing them from commit content and timestamp

Change-Id: I666ee4c2c2f10bdee0189fb376072232a221a633

7 files changed, 90 insertions, 38 deletions

diff --git a/java/com/google/gerrit/server/change/ChangeInserter.java b/java/com/google/gerrit/server/change/ChangeInserter.java
index 9f9ec1f04a..a3b5db0e4e 100644
--- a/java/com/google/gerrit/server/change/ChangeInserter.java
+++ b/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -67,6 +67,7 @@ import com.google.gerrit.server.update.ChangeContext;
 import com.google.gerrit.server.update.Context;
 import com.google.gerrit.server.update.InsertChangeOp;
 import com.google.gerrit.server.update.RepoContext;
+import com.google.gerrit.server.util.CommitMessageUtil;
 import com.google.gerrit.server.util.RequestScopePropagator;
 import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;
@@ -83,7 +84,6 @@ import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.transport.ReceiveCommand;
-import org.eclipse.jgit.util.ChangeIdUtil;
 
 public class ChangeInserter implements InsertChangeOp {
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@@ -203,16 +203,9 @@ public class ChangeInserter implements InsertChangeOp {
     if (!idList.isEmpty()) {
       return new Change.Key(idList.get(idList.size() - 1).trim());
     }
-    ObjectId changeId =
-        ChangeIdUtil.computeChangeId(
-            commit.getTree(),
-            commit,
-            commit.getAuthorIdent(),
-            commit.getCommitterIdent(),
-            commit.getShortMessage());
-    StringBuilder changeIdStr = new StringBuilder();
-    changeIdStr.append("I").append(ObjectId.toString(changeId));
-    return new Change.Key(changeIdStr.toString());
+    // A Change-Id is generated for the review, but not appended to the commit message.
+    // This can happen if requireChangeId is false.
+    return CommitMessageUtil.generateKey();
   }
 
   public PatchSet.Id getPatchSetId() {
diff --git a/java/com/google/gerrit/server/git/meta/VersionedMetaData.java b/java/com/google/gerrit/server/git/meta/VersionedMetaData.java
index f88144e4ac..5702fb4d64 100644
--- a/java/com/google/gerrit/server/git/meta/VersionedMetaData.java
+++ b/java/com/google/gerrit/server/git/meta/VersionedMetaData.java
@@ -22,6 +22,7 @@ import com.google.gerrit.git.LockFailureException;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.server.logging.TraceContext;
 import com.google.gerrit.server.logging.TraceContext.TraceTimer;
+import com.google.gerrit.server.util.CommitMessageUtil;
 import java.io.BufferedReader;
 import java.io.File;
 import java.io.IOException;
@@ -326,14 +327,8 @@ public abstract class VersionedMetaData {
         }
 
         if (update.insertChangeId()) {
-          ObjectId id =
-              ChangeIdUtil.computeChangeId(
-                  res,
-                  getRevision(),
-                  commit.getAuthor(),
-                  commit.getCommitter(),
-                  commit.getMessage());
-          commit.setMessage(ChangeIdUtil.insertId(commit.getMessage(), id));
+          commit.setMessage(
+              ChangeIdUtil.insertId(commit.getMessage(), CommitMessageUtil.generateChangeId()));
         }
 
         src = rw.parseCommit(inserter.insert(commit));
diff --git a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
index c1fd0baff5..132310d048 100644
--- a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
+++ b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
@@ -58,6 +58,7 @@ import com.google.gerrit.server.submit.IntegrationException;
 import com.google.gerrit.server.submit.MergeIdenticalTreeException;
 import com.google.gerrit.server.update.BatchUpdate;
 import com.google.gerrit.server.update.UpdateException;
+import com.google.gerrit.server.util.CommitMessageUtil;
 import com.google.gerrit.server.util.time.TimeUtil;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -191,14 +192,8 @@ public class CherryPickChange {
       Timestamp now = TimeUtil.nowTs();
       PersonIdent committerIdent = identifiedUser.newCommitterIdent(now, serverTimeZone);
 
-      final ObjectId computedChangeId =
-          ChangeIdUtil.computeChangeId(
-              commitToCherryPick.getTree(),
-              baseCommit,
-              commitToCherryPick.getAuthorIdent(),
-              committerIdent,
-              input.message);
-      String commitMessage = ChangeIdUtil.insertId(input.message, computedChangeId).trim() + '\n';
+      final ObjectId generatedChangeId = CommitMessageUtil.generateChangeId();
+      String commitMessage = ChangeIdUtil.insertId(input.message, generatedChangeId).trim() + '\n';
 
       CodeReviewCommit cherryPickCommit;
       ProjectState projectState = projectCache.checkedGet(dest.getParentKey());
@@ -233,7 +228,7 @@ public class CherryPickChange {
           final String idStr = idList.get(idList.size() - 1).trim();
           changeKey = new Change.Key(idStr);
         } else {
-          changeKey = new Change.Key("I" + computedChangeId.name());
+          changeKey = new Change.Key("I" + generatedChangeId.name());
         }
 
         Branch.NameKey newDest = new Branch.NameKey(project, destRef.getName());
diff --git a/java/com/google/gerrit/server/restapi/change/CreateChange.java b/java/com/google/gerrit/server/restapi/change/CreateChange.java
index 0d1a001387..d22bd0cd5f 100644
--- a/java/com/google/gerrit/server/restapi/change/CreateChange.java
+++ b/java/com/google/gerrit/server/restapi/change/CreateChange.java
@@ -70,6 +70,7 @@ import com.google.gerrit.server.update.BatchUpdate;
 import com.google.gerrit.server.update.RetryHelper;
 import com.google.gerrit.server.update.RetryingRestCollectionModifyView;
 import com.google.gerrit.server.update.UpdateException;
+import com.google.gerrit.server.util.CommitMessageUtil;
 import com.google.gerrit.server.util.time.TimeUtil;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -413,7 +414,7 @@ public class CreateChange
     String commitMessage = subject;
     if (ChangeIdUtil.indexOfChangeId(commitMessage, "\n") == -1) {
       ObjectId treeId = mergeTip == null ? emptyTreeId(objectInserter) : mergeTip.getTree();
-      ObjectId id = ChangeIdUtil.computeChangeId(treeId, mergeTip, author, author, commitMessage);
+      ObjectId id = CommitMessageUtil.generateChangeId();
       commitMessage = ChangeIdUtil.insertId(commitMessage, id);
     }
 
diff --git a/java/com/google/gerrit/server/restapi/change/Revert.java b/java/com/google/gerrit/server/restapi/change/Revert.java
index 3fe6770529..dd51e7f28b 100644
--- a/java/com/google/gerrit/server/restapi/change/Revert.java
+++ b/java/com/google/gerrit/server/restapi/change/Revert.java
@@ -63,6 +63,7 @@ import com.google.gerrit.server.update.Context;
 import com.google.gerrit.server.update.RetryHelper;
 import com.google.gerrit.server.update.RetryingRestModifyView;
 import com.google.gerrit.server.update.UpdateException;
+import com.google.gerrit.server.util.CommitMessageUtil;
 import com.google.gerrit.server.util.time.TimeUtil;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -201,14 +202,8 @@ public class Revert extends RetryingRestModifyView<ChangeResource, RevertInput,
                 patch.getRevision().get());
       }
 
-      ObjectId computedChangeId =
-          ChangeIdUtil.computeChangeId(
-              parentToCommitToRevert.getTree(),
-              commitToRevert,
-              authorIdent,
-              committerIdent,
-              message);
-      revertCommitBuilder.setMessage(ChangeIdUtil.insertId(message, computedChangeId, true));
+      ObjectId generatedChangeId = CommitMessageUtil.generateChangeId();
+      revertCommitBuilder.setMessage(ChangeIdUtil.insertId(message, generatedChangeId, true));
 
       Change.Id changeId = new Change.Id(seq.nextChangeId());
       ObjectId id = oi.insert(revertCommitBuilder);
@@ -240,7 +235,7 @@ public class Revert extends RetryingRestModifyView<ChangeResource, RevertInput,
         bu.setNotify(notify);
         bu.insertChange(ins);
         bu.addOp(changeId, new NotifyOp(changeToRevert, ins));
-        bu.addOp(changeToRevert.getId(), new PostRevertedMessageOp(computedChangeId));
+        bu.addOp(changeToRevert.getId(), new PostRevertedMessageOp(generatedChangeId));
         bu.execute();
       }
       return changeId;
diff --git a/java/com/google/gerrit/server/util/CommitMessageUtil.java b/java/com/google/gerrit/server/util/CommitMessageUtil.java
index e984f46f99..4ad226b7fd 100644
--- a/java/com/google/gerrit/server/util/CommitMessageUtil.java
+++ b/java/com/google/gerrit/server/util/CommitMessageUtil.java
@@ -14,12 +14,29 @@
 
 package com.google.gerrit.server.util;
 
+import static java.nio.charset.StandardCharsets.UTF_8;
+
 import com.google.common.base.Strings;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.reviewdb.client.Change;
+import java.security.NoSuchAlgorithmException;
+import java.security.SecureRandom;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectInserter;
 
 /** Utility functions to manipulate commit messages. */
 public class CommitMessageUtil {
+  private static final SecureRandom rng;
+
+  static {
+    try {
+      rng = SecureRandom.getInstance("SHA1PRNG");
+    } catch (NoSuchAlgorithmException e) {
+      throw new RuntimeException("Cannot create RNG for Change-Id generator", e);
+    }
+  }
 
   private CommitMessageUtil() {}
 
@@ -42,4 +59,18 @@ public class CommitMessageUtil {
     trimmed = trimmed + "\n";
     return trimmed;
   }
+
+  public static ObjectId generateChangeId() {
+    byte[] rand = new byte[Constants.OBJECT_ID_STRING_LENGTH];
+    rng.nextBytes(rand);
+    String randomString = new String(rand, UTF_8);
+
+    try (ObjectInserter f = new ObjectInserter.Formatter()) {
+      return f.idFor(Constants.OBJ_COMMIT, Constants.encode(randomString));
+    }
+  }
+
+  public static Change.Key generateKey() {
+    return new Change.Key("I" + generateChangeId().name());
+  }
 }
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
index 10428c56dc..bc675e539c 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
@@ -52,6 +52,12 @@ import com.google.inject.Inject;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Repository;
@@ -485,6 +491,42 @@ public class CreateChangeIT extends AbstractDaemonTest {
   }
 
   @Test
+  public void sha1sOfTwoNewChangesDiffer() throws Exception {
+    ChangeInput changeInput = newChangeInput(ChangeStatus.NEW);
+    ChangeInfo info1 = assertCreateSucceeds(changeInput);
+    ChangeInfo info2 = assertCreateSucceeds(changeInput);
+    assertThat(info1.currentRevision).isNotEqualTo(info2.currentRevision);
+    assertThat(info1.changeId).isNotEqualTo(info2.changeId);
+  }
+
+  @Test
+  public void sha1sOfTwoNewChangesDifferIfCreatedConcurrently() throws Exception {
+    ExecutorService executor = Executors.newFixedThreadPool(2);
+    try {
+      for (int i = 0; i < 10; i++) {
+        ChangeInput changeInput = newChangeInput(ChangeStatus.NEW);
+
+        CyclicBarrier sync = new CyclicBarrier(2);
+        Callable<ChangeInfo> createChange =
+            () -> {
+              requestScopeOperations.setApiUser(admin.id());
+              sync.await();
+              return assertCreateSucceeds(changeInput);
+            };
+
+        Future<ChangeInfo> changeInfo1 = executor.submit(createChange);
+        Future<ChangeInfo> changeInfo2 = executor.submit(createChange);
+        assertThat(changeInfo1.get().currentRevision)
+            .isNotEqualTo(changeInfo2.get().currentRevision);
+        assertThat(changeInfo1.get().changeId).isNotEqualTo(changeInfo2.get().changeId);
+      }
+    } finally {
+      executor.shutdown();
+      executor.awaitTermination(5, TimeUnit.SECONDS);
+    }
+  }
+
+  @Test
   public void createChangeOnExistingBranchNotPermitted() throws Exception {
     createBranch(new Branch.NameKey(project, "foo"));
     blockRead("refs/heads/*");