From eb9031212cda1f960dbdfa76c913bba6d19c035d Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Wed, 1 Jun 2022 22:56:17 -0700 Subject: Limit the number of changes that can be submitted together When chaining changes together, the sequence of commits to navigate was previously unbound, causing the potential explosion to millions of changes. The explosion could have also been accidental and caused by the push of a change with a non-existent branch, which would have resulted in the full scan of the repository for changes. Introduce a new Gerrit configuration change.maxSubmittableAtOnce with a safe default of 32767, which would allow any use case that would have also worked before this change. Navigating over 32767 changes up to potentially a huge number of commits would have generated a significant CPU and memory overload and still not resulted in a submittable chain of changes anyway. Release-Notes: Limit the number of changes that can be submitted at once Bug: Issue 16322 Change-Id: Id71aed2341f72708778395359bb6e4d4c270401c --- Documentation/config-gerrit.txt | 8 +++++ .../submit/LocalMergeSuperSetComputation.java | 35 ++++++++++++++++++---- .../server/change/SubmittedTogetherIT.java | 22 ++++++++++++++ 3 files changed, 59 insertions(+), 6 deletions(-) diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 9885e71e22..f5b42c6589 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -1213,6 +1213,14 @@ performance improvements by reducing the number of refs in the repo. + Default is true. +[[change.maxSubmittableAtOnce]]change.maxSubmittableAtOnce:: ++ +Maximum number of changes that can be chained together in the same repository +to be submitted at once. ++ +Default is 32767. + + [[change.showAssigneeInChangesTable]]change.showAssigneeInChangesTable:: + Show assignee field in changes table. If set to false, assignees will diff --git a/java/com/google/gerrit/server/submit/LocalMergeSuperSetComputation.java b/java/com/google/gerrit/server/submit/LocalMergeSuperSetComputation.java index 9efb976a65..60b0c5b01a 100644 --- a/java/com/google/gerrit/server/submit/LocalMergeSuperSetComputation.java +++ b/java/com/google/gerrit/server/submit/LocalMergeSuperSetComputation.java @@ -30,6 +30,7 @@ import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.permissions.ChangePermission; import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; @@ -53,6 +54,7 @@ import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; +import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.revwalk.RevCommit; @@ -65,6 +67,8 @@ import org.eclipse.jgit.revwalk.RevSort; public class LocalMergeSuperSetComputation implements MergeSuperSetComputation { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + public static final int MAX_SUBMITTABLE_CHANGES_AT_ONCE_DEFAULT = 1024; + public static class Module extends AbstractModule { @Override protected void configure() { @@ -90,17 +94,22 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation { private final Map> queryCache; private final Map> heads; private final ProjectCache projectCache; + private final int maxSubmittableChangesAtOnce; @Inject LocalMergeSuperSetComputation( PermissionBackend permissionBackend, Provider queryProvider, - ProjectCache projectCache) { + ProjectCache projectCache, + @GerritServerConfig Config gerritConfig) { this.projectCache = projectCache; this.permissionBackend = permissionBackend; this.queryProvider = queryProvider; this.queryCache = new HashMap<>(); this.heads = new HashMap<>(); + this.maxSubmittableChangesAtOnce = + gerritConfig.getInt( + "change", "maxSubmittableAtOnce", MAX_SUBMITTABLE_CHANGES_AT_ONCE_DEFAULT); } @Override @@ -146,10 +155,12 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation { } Set visibleHashes = - walkChangesByHashes(visibleCommits, Collections.emptySet(), or, b); + walkChangesByHashes( + visibleCommits, Collections.emptySet(), or, b, maxSubmittableChangesAtOnce); Iterables.addAll(visibleChanges, byCommitsOnBranchNotMerged(or, db, b, visibleHashes)); - Set nonVisibleHashes = walkChangesByHashes(nonVisibleCommits, visibleHashes, or, b); + Set nonVisibleHashes = + walkChangesByHashes(nonVisibleCommits, visibleHashes, or, b, maxSubmittableChangesAtOnce); Iterables.addAll(nonVisibleChanges, byCommitsOnBranchNotMerged(or, db, b, nonVisibleHashes)); } @@ -229,7 +240,11 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation { } private Set walkChangesByHashes( - Collection sourceCommits, Set ignoreHashes, OpenRepo or, Branch.NameKey b) + Collection sourceCommits, + Set ignoreHashes, + OpenRepo or, + Branch.NameKey b, + int limit) throws IOException { Set destHashes = new HashSet<>(); or.rw.reset(); @@ -239,7 +254,11 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation { if (ignoreHashes.contains(name)) { continue; } - destHashes.add(name); + if (destHashes.size() < limit) { + destHashes.add(name); + } else { + break; + } or.rw.markStart(c); } for (RevCommit c : or.rw) { @@ -247,7 +266,11 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation { if (ignoreHashes.contains(name)) { continue; } - destHashes.add(name); + if (destHashes.size() < limit) { + destHashes.add(name); + } else { + break; + } } return destHashes; diff --git a/javatests/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java b/javatests/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java index 304a1e48a4..0067e04cea 100644 --- a/javatests/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java +++ b/javatests/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java @@ -19,7 +19,9 @@ import static com.google.gerrit.acceptance.GitUtil.pushHead; import static com.google.gerrit.extensions.api.changes.SubmittedTogetherOption.NON_VISIBLE_CHANGES; import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.GerritConfig; import com.google.gerrit.acceptance.GitUtil; +import com.google.gerrit.acceptance.Sandboxed; import com.google.gerrit.acceptance.TestProjectInput; import com.google.gerrit.extensions.api.changes.SubmittedTogetherInfo; import com.google.gerrit.extensions.client.ChangeStatus; @@ -177,6 +179,26 @@ public class SubmittedTogetherIT extends AbstractDaemonTest { } } + @Test + @Sandboxed + @GerritConfig(name = "change.maxSubmittableAtOnce", value = "2") + public void submittedTogetherWithMaxChangesLimit() throws Exception { + String targetRef = "refs/for/master"; + + commitBuilder().add("a.txt", "1").message("subject: 1").create(); + pushHead(testRepo, targetRef, false); + + RevCommit c2_1 = commitBuilder().add("b.txt", "2").message("subject: 2").create(); + String id2 = getChangeId(c2_1); + pushHead(testRepo, targetRef, false); + + RevCommit c3_1 = commitBuilder().add("b.txt", "3").message("subject: 3").create(); + String id3 = getChangeId(c3_1); + pushHead(testRepo, targetRef, false); + + assertSubmittedTogether(id3, id3, id2); + } + @Test public void respectTopicsOnAncestors() throws Exception { RevCommit initialHead = getRemoteHead(); -- cgit v1.2.3