diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 6bcf8512fb..a3a813ed92 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -3541,6 +3541,15 @@ number of changes for review by mistake. + Default is zero, no limit. +[[receive.maxBatchCommits]]receive.maxBatchCommits:: ++ +The maximum number of commits that Gerrit allows to be pushed in a batch +directly to a branch when link:user-upload.html#bypass_review[bypassing review]. +This limit can be bypassed if a user link:user-upload.html#skip_validation[skips +validation]. ++ +Default is 10000. + [[receive.maxObjectSizeLimit]]receive.maxObjectSizeLimit:: + Maximum allowed Git object size that 'receive-pack' will accept. diff --git a/Documentation/error-messages.txt b/Documentation/error-messages.txt index 2632254fe4..ca8dc75251 100644 --- a/Documentation/error-messages.txt +++ b/Documentation/error-messages.txt @@ -32,6 +32,7 @@ occurring and what can be done to solve it. * link:error-prohibited-by-gerrit.html[prohibited by Gerrit] * link:error-project-not-found.html[Project not found: ...] * link:error-same-change-id-in-multiple-changes.html[same Change-Id in multiple changes] +* link:error-too-many-commits.html[too many commits] * link:error-upload-denied.html[Upload denied for project \'...'] * link:error-not-allowed-to-upload-merges.html[you are not allowed to upload merges] diff --git a/Documentation/error-too-many-commits.txt b/Documentation/error-too-many-commits.txt new file mode 100644 index 0000000000..3e16220f23 --- /dev/null +++ b/Documentation/error-too-many-commits.txt @@ -0,0 +1,20 @@ += too many commits + +This error occurs when a push directly to a branch +link:user-upload.html#bypass_review[bypassing review] contains more commits than +the server is able to validate in a single batch. + +The recommended way to avoid this message is to use the +link:user-upload.html#skip_validation[`skip-validation` push option]. Depending +on the number of commits, it may also be feasible to split the push into smaller +batches. + +The actual limit is controlled by a +link:config-gerrit.html#receive.maxBatchCommits[server config option]. + +GERRIT +------ +Part of link:error-messages.html[Gerrit Error Messages] + +SEARCHBOX +--------- diff --git a/Documentation/user-upload.txt b/Documentation/user-upload.txt index 47e08c5180..556a8bbe35 100644 --- a/Documentation/user-upload.txt +++ b/Documentation/user-upload.txt @@ -549,6 +549,11 @@ This option only applies when pushing directly to a branch bypassing review. Validation also occurs when pushing new changes for review, and that type of validation cannot be skipped. +The `skip-validation` option is always required when pushing +link:error-too-many-commits.html[more than a certain number of commits]. This is +the recommended approach when pushing lots of old history, since some validators +would require rewriting history in order to make them pass. + [[auto_merge]] === Auto-Merge during Push diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java index 6face43fae..0dc1389664 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java @@ -23,10 +23,12 @@ import static com.google.gerrit.acceptance.GitUtil.pushHead; import static com.google.gerrit.acceptance.PushOneCommit.FILE_NAME; import static com.google.gerrit.common.FooterConstants.CHANGE_ID; import static com.google.gerrit.extensions.common.EditInfoSubject.assertThat; +import static com.google.gerrit.server.git.receive.ReceiveConstants.PUSH_OPTION_SKIP_VALIDATION; import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS; import static com.google.gerrit.server.project.Util.category; import static com.google.gerrit.server.project.Util.value; import static java.util.concurrent.TimeUnit.SECONDS; +import static java.util.stream.Collectors.joining; import static java.util.stream.Collectors.toList; import com.google.common.collect.ImmutableList; @@ -35,6 +37,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Streams; import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.GerritConfig; import com.google.gerrit.acceptance.GitUtil; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.TestAccount; @@ -62,10 +65,12 @@ import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.git.receive.ReceiveConstants; +import com.google.gerrit.server.group.SystemGroupBackend; import com.google.gerrit.server.mail.Address; import com.google.gerrit.server.project.Util; import com.google.gerrit.server.query.change.ChangeData; @@ -1766,6 +1771,29 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { assertThat(getPublishedComments(r.getChangeId())).isEmpty(); } + @GerritConfig(name = "receive.maxBatchCommits", value = "2") + @Test + public void maxBatchCommits() throws Exception { + List commits = new ArrayList<>(); + commits.addAll(initChanges(2)); + String master = "refs/heads/master"; + assertPushOk(pushHead(testRepo, master), master); + + commits.addAll(initChanges(3)); + assertPushRejected(pushHead(testRepo, master), master, "too many commits"); + + grantSkipValidation(project, master, SystemGroupBackend.REGISTERED_USERS); + PushResult r = + pushHead(testRepo, master, false, false, ImmutableList.of(PUSH_OPTION_SKIP_VALIDATION)); + assertPushOk(r, master); + + // No open changes; branch was advanced. + String q = commits.stream().map(ObjectId::name).collect(joining(" OR commit:", "commit:", "")); + assertThat(gApi.changes().query(q).get()).isEmpty(); + assertThat(gApi.projects().name(project.get()).branch(master).get().revision) + .isEqualTo(Iterables.getLast(commits).name()); + } + private DraftInput newDraft(String path, int line, String message) { DraftInput d = new DraftInput(); d.path = path; @@ -1839,11 +1867,21 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { } private List createChanges(int n, String refsFor) throws Exception { - return createChanges(n, refsFor, ImmutableList.of()); + return createChanges(n, refsFor, ImmutableList.of()); } private List createChanges(int n, String refsFor, List footerLines) throws Exception { + List commits = initChanges(n, footerLines); + assertPushOk(pushHead(testRepo, refsFor, false), refsFor); + return commits; + } + + private List initChanges(int n) throws Exception { + return initChanges(n, ImmutableList.of()); + } + + private List initChanges(int n, List footerLines) throws Exception { List commits = new ArrayList<>(n); for (int i = 1; i <= n; i++) { String msg = "Change " + i; @@ -1863,7 +1901,6 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { testRepo.getRevWalk().parseBody(c); commits.add(c); } - assertPushOk(pushHead(testRepo, refsFor, false), refsFor); return commits; } @@ -1934,4 +1971,15 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { config.getProject().setCreateNewChangeForAllNotInTarget(InheritableBoolean.TRUE); saveProjectConfig(project, config); } + + private void grantSkipValidation(Project.NameKey project, String ref, AccountGroup.UUID groupUuid) + throws Exception { + // See SKIP_VALIDATION implementation in default permission backend. + ProjectConfig config = projectCache.checkedGet(project).getConfig(); + Util.allow(config, Permission.FORGE_AUTHOR, groupUuid, ref); + Util.allow(config, Permission.FORGE_COMMITTER, groupUuid, ref); + Util.allow(config, Permission.FORGE_SERVER, groupUuid, ref); + Util.allow(config, Permission.PUSH_MERGE, groupUuid, "refs/for/" + ref); + saveProjectConfig(project, config); + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index 6a04174f49..e11cdf7095 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -2710,9 +2710,22 @@ class ReceiveCommits { ListMultimap existing = changeRefsById(); walk.markStart((RevCommit) parsedObject); markHeadsAsUninteresting(walk, cmd.getRefName()); - int i = 0; + int limit = receiveConfig.maxBatchCommits; + int n = 0; for (RevCommit c; (c = walk.next()) != null; ) { - i++; + if (++n > limit) { + logDebug("Number of new commits exceeds limit of {}", limit); + addMessage( + "Cannot push more than " + + limit + + " commits to " + + branch.get() + + " without " + + PUSH_OPTION_SKIP_VALIDATION + + " option"); + reject(cmd, "too many commits"); + return; + } if (existing.keySet().contains(c)) { continue; } else if (!validCommit(walk, perm, branch, cmd, c)) { @@ -2725,7 +2738,7 @@ class ReceiveCommits { missingFullName = false; } } - logDebug("Validated {} new commits", i); + logDebug("Validated {} new commits", n); } catch (IOException err) { cmd.setResult(REJECTED_MISSING_OBJECT); logError("Invalid pack upload; one or more objects weren't sent", err); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveConfig.java index 7be6dccec6..30d5071f43 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveConfig.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveConfig.java @@ -28,6 +28,7 @@ class ReceiveConfig { final boolean checkMagicRefs; final boolean checkReferencedObjectsAreReachable; final boolean allowDrafts; + final int maxBatchCommits; private final int systemMaxBatchChanges; private final AccountLimits.Factory limitsFactory; @@ -37,6 +38,7 @@ class ReceiveConfig { checkReferencedObjectsAreReachable = config.getBoolean("receive", null, "checkReferencedObjectsAreReachable", true); allowDrafts = config.getBoolean("change", null, "allowDrafts", true); + maxBatchCommits = config.getInt("receive", null, "maxBatchCommits", 10000); systemMaxBatchChanges = config.getInt("receive", "maxBatchChanges", 0); this.limitsFactory = limitsFactory; }