Merge changes from topic "skip-validation"

* changes:
  Add limit to number of commits that ReceiveCommits will validate
  Rename bypass-review to skip-validation and document it
  ReceiveCommits: Update caller fullName after everything else

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.

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]
 

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
+---------

Documentation/project-configuration.txt

@@ -224,6 +224,7 @@ maximum Git object size limit that is actually used on the project.
 The defined maximum Git object size limit is inherited by any child
 project.
 
+[[require-signed-off-by]]
 === Require Signed-off-by
 
 The `Require Signed-off-by in commit message` option defines whether a

Documentation/user-upload.txt

@@ -517,6 +517,44 @@ grant nothing at all.  This ensures that accidental pushes don't
 make undesired changes to the public repository.
 
 
+[[skip_validation]]
+=== Skip Validation
+
+Even when a user has permission to push directly to a branch
+link:#bypass_review[bypassing review], by default Gerrit will still validate any
+new commits, for example to check author/committer identities, and run
+link:config-validation.html#new-commit-validation[validation plugins]. This
+behavior can be bypassed with a push option:
+
+----
+git push -o skip-validation HEAD:master
+----
+
+Using the `skip-validation` option requires the user to have a specific set
+of permissions, *in addition* to those permissions already required to bypass
+review:
+
+* link:access-control.html#category_forge_author[Forge Author]
+* link:access-control.html#category_forge_committer[Forge Committer]
+* link:access-control.html#category_forge_server[Forge Server]
+* link:access-control.html#category_push_merge[Push Merge Commits]
+
+Plus these additional requirements on the project:
+
+* Project must not link:project-configuration.html#require-signed-off-by[require
+Signed-off-by].
+* Project must not have `refs/meta/reject-commits`.
+
+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
 

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<RevCommit> 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<RevCommit> createChanges(int n, String refsFor) throws Exception {
-    return createChanges(n, refsFor, ImmutableList.<String>of());
+    return createChanges(n, refsFor, ImmutableList.of());
   }
 
   private List<RevCommit> createChanges(int n, String refsFor, List<String> footerLines)
       throws Exception {
+    List<RevCommit> commits = initChanges(n, footerLines);
+    assertPushOk(pushHead(testRepo, refsFor, false), refsFor);
+    return commits;
+  }
+
+  private List<RevCommit> initChanges(int n) throws Exception {
+    return initChanges(n, ImmutableList.of());
+  }
+
+  private List<RevCommit> initChanges(int n, List<String> footerLines) throws Exception {
     List<RevCommit> 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);
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java

@@ -23,6 +23,7 @@ import static com.google.gerrit.server.change.HashtagsUtil.cleanupHashtag;
 import static com.google.gerrit.server.git.MultiProgressMonitor.UNKNOWN;
 import static com.google.gerrit.server.git.receive.ReceiveConstants.COMMAND_REJECTION_MESSAGE_FOOTER;
 import static com.google.gerrit.server.git.receive.ReceiveConstants.ONLY_OWNER_CAN_MODIFY_WIP;
+import static com.google.gerrit.server.git.receive.ReceiveConstants.PUSH_OPTION_SKIP_VALIDATION;
 import static com.google.gerrit.server.git.receive.ReceiveConstants.SAME_CHANGE_ID_IN_MULTIPLE_CHANGES;
 import static com.google.gerrit.server.git.validators.CommitValidators.NEW_PATCHSET_PATTERN;
 import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromFooters;
@@ -198,7 +199,6 @@ import org.slf4j.LoggerFactory;
 /** Receives change upload using the Git receive-pack protocol. */
 class ReceiveCommits {
   private static final Logger log = LoggerFactory.getLogger(ReceiveCommits.class);
-  private static final String BYPASS_REVIEW = "bypass-review";
 
   private enum ReceiveError {
     CONFIG_UPDATE(
@@ -362,6 +362,7 @@ class ReceiveCommits {
   // Other settings populated during processing.
   private MagicBranchInput magicBranch;
   private boolean newChangeForAllNotInTarget;
+  private String setFullNameTo;
 
   // Handles for outputting back over the wire to the end user.
   private Task newProgress;
@@ -587,6 +588,9 @@ class ReceiveCommits {
       }
     }
 
+    // Update account info with details discovered during commit walking.
+    updateAccountInfo();
+
     closeProgress.end();
     commandProgress.end();
     progress.end();
@@ -2681,11 +2685,11 @@ class ReceiveCommits {
     if (!RefNames.REFS_CONFIG.equals(cmd.getRefName())
         && !(MagicBranch.isMagicBranch(cmd.getRefName())
             || NEW_PATCHSET_PATTERN.matcher(cmd.getRefName()).matches())
-        && pushOptions.containsKey(BYPASS_REVIEW)) {
+        && pushOptions.containsKey(PUSH_OPTION_SKIP_VALIDATION)) {
       try {
-        perm.check(RefPermission.BYPASS_REVIEW);
+        perm.check(RefPermission.SKIP_VALIDATION);
         if (!Iterables.isEmpty(rejectCommits)) {
-          throw new AuthException("reject-commits prevents " + BYPASS_REVIEW);
+          throw new AuthException("reject-commits prevents " + PUSH_OPTION_SKIP_VALIDATION);
         }
         logDebug("Short-circuiting new commit validation");
       } catch (AuthException denied) {
@@ -2694,7 +2698,7 @@ class ReceiveCommits {
       return;
     }
 
-    boolean defaultName = Strings.isNullOrEmpty(user.getAccount().getFullName());
+    boolean missingFullName = Strings.isNullOrEmpty(user.getAccount().getFullName());
     RevWalk walk = rp.getRevWalk();
     walk.reset();
     walk.sort(RevSort.NONE);
@@ -2706,39 +2710,35 @@ class ReceiveCommits {
       ListMultimap<ObjectId, Ref> 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)) {
           break;
         }
 
-        if (defaultName && user.hasEmailAddress(c.getCommitterIdent().getEmailAddress())) {
-          try {
-            String committerName = c.getCommitterIdent().getName();
-            Account account =
-                accountsUpdate
-                    .create()
-                    .update(
-                        user.getAccountId(),
-                        a -> {
-                          if (Strings.isNullOrEmpty(a.getFullName())) {
-                            a.setFullName(committerName);
-                          }
-                        });
-            if (account != null && Strings.isNullOrEmpty(account.getFullName())) {
-              user.getAccount().setFullName(account.getFullName());
-            }
-          } catch (IOException | ConfigInvalidException e) {
-            logWarn("Cannot default full_name", e);
-          } finally {
-            defaultName = false;
+        if (missingFullName && user.hasEmailAddress(c.getCommitterIdent().getEmailAddress())) {
+          logDebug("Will update full name of caller");
+          setFullNameTo = c.getCommitterIdent().getName();
+          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);
@@ -2880,6 +2880,30 @@ class ReceiveCommits {
     }
   }
 
+  private void updateAccountInfo() {
+    if (setFullNameTo == null) {
+      return;
+    }
+    logDebug("Updating full name of caller");
+    try {
+      Account account =
+          accountsUpdate
+              .create()
+              .update(
+                  user.getAccountId(),
+                  a -> {
+                    if (Strings.isNullOrEmpty(a.getFullName())) {
+                      a.setFullName(setFullNameTo);
+                    }
+                  });
+      if (account != null) {
+        user.getAccount().setFullName(account.getFullName());
+      }
+    } catch (IOException | ConfigInvalidException e) {
+      logWarn("Failed to update full name of caller", e);
+    }
+  }
+
   private Map<Change.Key, ChangeNotes> openChangesByBranch(Branch.NameKey branch)
       throws OrmException {
     Map<Change.Key, ChangeNotes> r = new HashMap<>();

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;
   }

gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveConstants.java

@@ -17,6 +17,8 @@ package com.google.gerrit.server.git.receive;
 import com.google.common.annotations.VisibleForTesting;
 
 public final class ReceiveConstants {
+  public static final String PUSH_OPTION_SKIP_VALIDATION = "skip-validation";
+
   @VisibleForTesting
   public static final String ONLY_OWNER_CAN_MODIFY_WIP =
       "only change owner can modify Work-in-Progress";

gerrit-server/src/main/java/com/google/gerrit/server/permissions/RefPermission.java

@@ -29,7 +29,7 @@ public enum RefPermission {
   FORGE_COMMITTER(Permission.FORGE_COMMITTER),
   FORGE_SERVER(Permission.FORGE_SERVER),
   MERGE,
-  BYPASS_REVIEW,
+  SKIP_VALIDATION,
 
   /** Create a change to code review a commit. */
   CREATE_CHANGE,

gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java

@@ -609,7 +609,7 @@ public class RefControl {
         case UPDATE_BY_SUBMIT:
           return projectControl.controlForRef("refs/for/" + getRefName()).canSubmit(true);
 
-        case BYPASS_REVIEW:
+        case SKIP_VALIDATION:
           return canForgeAuthor()
               && canForgeCommitter()
               && canForgeGerritServerIdentity()