Merge "Make SubmoduleOp#createSubmoduleCommitMsg() less wasteful"

Documentation/config-gerrit.txt

@@ -4590,6 +4590,21 @@ This allows to enable the superproject subscription mechanism.
 +
 By default this is true.
 
+[[submodule.maxCombinedCommitMessageSize]]submodule.maxCombinedCommitMessageSize::
++
+This allows to limit the length of the commit message for a submodule.
++
+By default this is 262144 (256 KiB).
++
+Common unit suffixes of k, m, or g are supported.
+
+[[submodule.maxCommitMessages]]submodule.maxCommitMessages::
++
+This allows to limit the number of commit messages that should be combined when creating
+a commit message for a submodule.
++
+By default this is 1000.
+
 [[user]]
 === Section user
 

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractSubmoduleSubscription.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.acceptance.git;
 
 import static com.google.common.truth.Truth.assertThat;
+import static java.util.stream.Collectors.toList;
 
 import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
@@ -25,7 +26,9 @@ import com.google.gerrit.extensions.client.SubmitType;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.server.git.MetaDataUpdate;
 import com.google.gerrit.server.git.ProjectConfig;
+import java.util.List;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.StreamSupport;
 import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectId;
@@ -141,6 +144,31 @@ public abstract class AbstractSubmoduleSubscription extends AbstractDaemonTest {
     return pushChangeTo(repo, "refs/heads/" + branch, "some change", "");
   }
 
+  protected ObjectId pushChangesTo(TestRepository<?> repo, String branch, int numChanges)
+      throws Exception {
+    for (int i = 0; i < numChanges; i++) {
+      repo.branch("HEAD")
+          .commit()
+          .insertChangeId()
+          .message("Message " + i)
+          .add(name("file"), "content" + i)
+          .create();
+    }
+    String remoteBranch = "refs/heads/" + branch;
+    Iterable<PushResult> res =
+        repo.git()
+            .push()
+            .setRemote("origin")
+            .setRefSpecs(new RefSpec("HEAD:" + remoteBranch))
+            .call();
+    List<Status> status =
+        StreamSupport.stream(res.spliterator(), false)
+            .map(r -> r.getRemoteUpdate(remoteBranch).getStatus())
+            .collect(toList());
+    assertThat(status).containsExactly(Status.OK);
+    return Iterables.getLast(res).getRemoteUpdate(remoteBranch).getNewObjectId();
+  }
+
   protected void allowSubmoduleSubscription(
       String submodule, String subBranch, String superproject, String superBranch, boolean match)
       throws Exception {

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsIT.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.acceptance.git;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.TruthJUnit.assume;
 
 import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.GerritConfig;
@@ -499,4 +500,42 @@ public class SubmoduleSubscriptionsIT extends AbstractSubmoduleSubscription {
 
     expectToHaveSubmoduleState(superRepo, "master", "nested/subscribed-to-project", subHEAD);
   }
+
+  @Test
+  @GerritConfig(name = "submodule.verboseSuperprojectUpdate", value = "SUBJECT_ONLY")
+  @GerritConfig(name = "submodule.maxCommitMessages", value = "1")
+  public void submoduleSubjectCommitMessageCountLimit() throws Exception {
+    testSubmoduleSubjectCommitMessageAndExpectTruncation();
+  }
+
+  @Test
+  @GerritConfig(name = "submodule.verboseSuperprojectUpdate", value = "SUBJECT_ONLY")
+  @GerritConfig(name = "submodule.maxCombinedCommitMessageSize", value = "175")
+  public void submoduleSubjectCommitMessageSizeLimit() throws Exception {
+    assume().that(isSubmitWholeTopicEnabled()).isFalse();
+    testSubmoduleSubjectCommitMessageAndExpectTruncation();
+  }
+
+  private void testSubmoduleSubjectCommitMessageAndExpectTruncation() throws Exception {
+    TestRepository<?> superRepo = createProjectWithPush("super-project");
+    TestRepository<?> subRepo = createProjectWithPush("subscribed-to-project");
+    allowMatchingSubmoduleSubscription(
+        "subscribed-to-project", "refs/heads/master", "super-project", "refs/heads/master");
+
+    pushChangeTo(subRepo, "master");
+    createSubmoduleSubscription(superRepo, "master", "subscribed-to-project", "master");
+    // The first update doesn't include the rev log, so we ignore it
+    pushChangeTo(subRepo, "master");
+
+    // Next, we push two commits at once. Since maxCommitMessages=1, we expect to see only the first
+    // message plus ellipsis to mark truncation.
+    ObjectId subHEAD = pushChangesTo(subRepo, "master", 2);
+    RevCommit subCommitMsg = subRepo.getRevWalk().parseCommit(subHEAD);
+    expectToHaveCommitMessage(
+        superRepo,
+        "master",
+        String.format(
+            "Update git submodules\n\n* Update %s from branch 'master'\n  - %s\n\n[...]",
+            name("subscribed-to-project"), subCommitMsg.getShortMessage()));
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java

@@ -45,10 +45,12 @@ import java.util.Collections;
 import java.util.Deque;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.Iterator;
 import java.util.LinkedHashSet;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
+import org.apache.commons.lang.StringUtils;
 import org.eclipse.jgit.dircache.DirCache;
 import org.eclipse.jgit.dircache.DirCacheBuilder;
 import org.eclipse.jgit.dircache.DirCacheEditor;
@@ -135,6 +137,8 @@ public class SubmoduleOp {
   private final BatchUpdate.Factory batchUpdateFactory;
   private final VerboseSuperprojectUpdate verboseSuperProject;
   private final boolean enableSuperProjectSubscriptions;
+  private final long maxCombinedCommitMessageSize;
+  private final long maxCommitMessages;
   private final MergeOpRepoManager orm;
   private final Map<Branch.NameKey, GitModules> branchGitModules;
 
@@ -170,6 +174,9 @@ public class SubmoduleOp {
         cfg.getEnum("submodule", null, "verboseSuperprojectUpdate", VerboseSuperprojectUpdate.TRUE);
     this.enableSuperProjectSubscriptions =
         cfg.getBoolean("submodule", "enableSuperProjectSubscriptions", true);
+    this.maxCombinedCommitMessageSize =
+        cfg.getLong("submodule", "maxCombinedCommitMessageSize", 256 << 10);
+    this.maxCommitMessages = cfg.getLong("submodule", "maxCommitMessages", 1000);
     this.orm = orm;
     this.updatedBranches = updatedBranches;
     this.targets = MultimapBuilder.hashKeys().hashSetValues().build();
@@ -547,8 +554,11 @@ public class SubmoduleOp {
       RevCommit newCommit,
       RevCommit oldCommit)
       throws SubmoduleException {
-    msgbuf.append("* Update " + s.getPath());
-    msgbuf.append(" from branch '" + s.getSubmodule().getShortName() + "'");
+    msgbuf.append("* Update ");
+    msgbuf.append(s.getPath());
+    msgbuf.append(" from branch '");
+    msgbuf.append(s.getSubmodule().getShortName());
+    msgbuf.append("'");
 
     // newly created submodule gitlink, do not append whole history
     if (oldCommit == null) {
@@ -559,13 +569,27 @@ public class SubmoduleOp {
       subOr.rw.resetRetain(subOr.canMergeFlag);
       subOr.rw.markStart(newCommit);
       subOr.rw.markUninteresting(oldCommit);
-      for (RevCommit c : subOr.rw) {
+      int numMessages = 0;
+      for (Iterator<RevCommit> iter = subOr.rw.iterator(); iter.hasNext(); ) {
+        RevCommit c = iter.next();
         subOr.rw.parseBody(c);
-        if (verboseSuperProject == VerboseSuperprojectUpdate.SUBJECT_ONLY) {
-          msgbuf.append("\n  - " + c.getShortMessage());
-        } else if (verboseSuperProject == VerboseSuperprojectUpdate.TRUE) {
-          msgbuf.append("\n  - " + c.getFullMessage().replace("\n", "\n    "));
+
+        String message =
+            verboseSuperProject == VerboseSuperprojectUpdate.SUBJECT_ONLY
+                ? c.getShortMessage()
+                : StringUtils.replace(c.getFullMessage(), "\n", "\n    ");
+
+        String bullet = "\n  - ";
+        String ellipsis = "\n\n[...]";
+        int newSize = msgbuf.length() + bullet.length() + message.length();
+        if (++numMessages > maxCommitMessages
+            || newSize > maxCombinedCommitMessageSize
+            || iter.hasNext() && (newSize + ellipsis.length()) > maxCombinedCommitMessageSize) {
+          msgbuf.append(ellipsis);
+          break;
         }
+        msgbuf.append(bullet);
+        msgbuf.append(message);
       }
     } catch (IOException e) {
       throw new SubmoduleException(