Merge changes I9a7060e3,I8d80186c

* changes:
  Refactor CommitValidators to reduce repetition
  Add test for pushing initial review in empty repo

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

@@ -33,6 +33,7 @@ import com.google.gerrit.acceptance.GerritConfig;
 import com.google.gerrit.acceptance.GitUtil;
 import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.TestProjectInput;
 import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.extensions.api.changes.NotifyHandling;
@@ -128,6 +129,27 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
     r.assertChange(Change.Status.NEW, null);
   }
 
+  @Test
+  @TestProjectInput(createEmptyCommit = false)
+  public void pushInitialCommitForMasterBranch() throws Exception {
+    RevCommit c =
+        testRepo.commit().message("Initial commit").insertChangeId().create();
+    String id = GitUtil.getChangeId(testRepo, c).get();
+    testRepo.reset(c);
+
+    String r = "refs/for/master";
+    PushResult pr = pushHead(testRepo, r, false);
+    assertPushOk(pr, r);
+
+    ChangeInfo change = gApi.changes().id(id).info();
+    assertThat(change.branch).isEqualTo("master");
+    assertThat(change.status).isEqualTo(ChangeStatus.NEW);
+
+    try (Repository repo = repoManager.openRepository(project)) {
+      assertThat(repo.resolve("master")).isNull();
+    }
+  }
+
   @Test
   public void output() throws Exception {
     String url = canonicalWebUrl.get();

gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/BatchGitModule.java

@@ -19,7 +19,6 @@ import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
 import com.google.gerrit.extensions.registration.DynamicSet;
 import com.google.gerrit.server.git.GitModule;
 import com.google.gerrit.server.git.validators.CommitValidationListener;
-import com.google.gerrit.server.git.validators.CommitValidators;
 
 /** Module for batch programs that need git access. */
 public class BatchGitModule extends FactoryModule {
@@ -27,7 +26,6 @@ public class BatchGitModule extends FactoryModule {
   protected void configure() {
     DynamicSet.setOf(binder(), GitReferenceUpdatedListener.class);
     DynamicSet.setOf(binder(), CommitValidationListener.class);
-    factory(CommitValidators.Factory.class);
     install(new GitModule());
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java

@@ -41,7 +41,6 @@ import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.events.CommitReceivedEvent;
 import com.google.gerrit.server.extensions.events.CommentAdded;
 import com.google.gerrit.server.extensions.events.RevisionCreated;
-import com.google.gerrit.server.git.BanCommit;
 import com.google.gerrit.server.git.BatchUpdate;
 import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
 import com.google.gerrit.server.git.BatchUpdate.Context;
@@ -66,7 +65,6 @@ import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;
 
 import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.notes.NoteMap;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.transport.ReceiveCommand;
 import org.eclipse.jgit.util.ChangeIdUtil;
@@ -468,9 +466,6 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
     try {
       RefControl refControl = projectControlFactory
           .controlFor(ctx.getProject(), ctx.getUser()).controlForRef(refName);
-      CommitValidators cv = commitValidatorsFactory.create(
-          refControl, new NoSshInfo(), ctx.getRepository());
-
       String refName = psId.toRefName();
       CommitReceivedEvent event = new CommitReceivedEvent(
           new ReceiveCommand(
@@ -481,19 +476,10 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
           change.getDest().get(),
           commit,
           ctx.getIdentifiedUser());
-
+      commitValidatorsFactory
-      switch (validatePolicy) {
+          .create(
-      case RECEIVE_COMMITS:
+              validatePolicy, refControl, new NoSshInfo(), ctx.getRepository())
-        NoteMap rejectCommits = BanCommit.loadRejectCommitsMap(
+          .validate(event);
-            ctx.getRepository(), ctx.getRevWalk());
-        cv.validateForReceiveCommits(event, rejectCommits);
-        break;
-      case GERRIT:
-        cv.validateForGerritCommits(event);
-        break;
-      case NONE:
-        break;
-      }
     } catch (CommitValidationException e) {
       throw new ResourceConflictException(e.getFullMessage());
     } catch (NoSuchProjectException e) {

gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java

@@ -35,7 +35,6 @@ import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.ReviewerSet;
 import com.google.gerrit.server.events.CommitReceivedEvent;
 import com.google.gerrit.server.extensions.events.RevisionCreated;
-import com.google.gerrit.server.git.BanCommit;
 import com.google.gerrit.server.git.BatchUpdate;
 import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
 import com.google.gerrit.server.git.BatchUpdate.Context;
@@ -52,7 +51,6 @@ import com.google.inject.assistedinject.Assisted;
 import com.google.inject.assistedinject.AssistedInject;
 
 import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.notes.NoteMap;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.transport.ReceiveCommand;
 import org.slf4j.Logger;
@@ -275,12 +273,12 @@ public class PatchSetInserter extends BatchUpdate.Op {
   private void validate(RepoContext ctx)
       throws AuthException, ResourceConflictException, IOException,
       OrmException {
-    CommitValidators cv = commitValidatorsFactory.create(
-        origCtl.getRefControl(), new NoSshInfo(), ctx.getRepository());
-
     if (!origCtl.canAddPatchSet(ctx.getDb())) {
       throw new AuthException("cannot add patch set");
     }
+    if (validatePolicy == CommitValidators.Policy.NONE) {
+      return;
+    }
 
     String refName = getPatchSetId().toRefName();
     CommitReceivedEvent event = new CommitReceivedEvent(
@@ -293,18 +291,11 @@ public class PatchSetInserter extends BatchUpdate.Op {
         commit, ctx.getIdentifiedUser());
 
     try {
-      switch (validatePolicy) {
+      commitValidatorsFactory
-      case RECEIVE_COMMITS:
+          .create(
-        NoteMap rejectCommits = BanCommit.loadRejectCommitsMap(
+              validatePolicy, origCtl.getRefControl(), new NoSshInfo(),
-            ctx.getRepository(), ctx.getRevWalk());
+              ctx.getRepository())
-        cv.validateForReceiveCommits(event, rejectCommits);
+          .validate(event);
-        break;
-      case GERRIT:
-        cv.validateForGerritCommits(event);
-        break;
-      case NONE:
-        break;
-      }
     } catch (CommitValidationException e) {
       throw new ResourceConflictException(e.getFullMessage());
     }

gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java

@@ -116,7 +116,6 @@ import com.google.gerrit.server.git.TagCache;
 import com.google.gerrit.server.git.TransferConfig;
 import com.google.gerrit.server.git.strategy.SubmitStrategy;
 import com.google.gerrit.server.git.validators.CommitValidationListener;
-import com.google.gerrit.server.git.validators.CommitValidators;
 import com.google.gerrit.server.git.validators.MergeValidationListener;
 import com.google.gerrit.server.git.validators.MergeValidators;
 import com.google.gerrit.server.git.validators.MergeValidators.ProjectConfigValidator;
@@ -372,7 +371,6 @@ public class GerritGlobalModule extends FactoryModule {
 
     bind(AnonymousUser.class);
 
-    factory(CommitValidators.Factory.class);
     factory(RefOperationValidators.Factory.class);
     factory(MergeValidators.Factory.class);
     factory(ProjectConfigValidator.Factory.class);

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

@@ -2588,12 +2588,11 @@ public class ReceiveCommits {
     rw.parseBody(c);
     CommitReceivedEvent receiveEvent =
         new CommitReceivedEvent(cmd, project, ctl.getRefName(), c, user);
-    CommitValidators commitValidators =
+    CommitValidators commitValidators = commitValidatorsFactory.create(
-        commitValidatorsFactory.create(ctl, sshInfo, repo);
+        CommitValidators.Policy.RECEIVE_COMMITS, ctl, sshInfo, repo);
 
     try {
-      messages.addAll(commitValidators.validateForReceiveCommits(
+      messages.addAll(commitValidators.validate(receiveEvent));
-          receiveEvent, rejectCommits));
     } catch (CommitValidationException e) {
       logDebug("Commit validation failed on {}", c.name());
       messages.addAll(e.getMessages());

gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java

@@ -16,8 +16,10 @@ package com.google.gerrit.server.git.validators;
 
 import static com.google.gerrit.reviewdb.client.Change.CHANGE_ID_PATTERN;
 import static com.google.gerrit.reviewdb.client.RefNames.REFS_CONFIG;
+import static com.google.gerrit.server.git.ReceiveCommits.NEW_PATCHSET;
 
 import com.google.common.base.CharMatcher;
+import com.google.common.collect.ImmutableList;
 import com.google.gerrit.common.FooterConstants;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.PageLinks;
@@ -31,15 +33,15 @@ import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.config.CanonicalWebUrl;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.events.CommitReceivedEvent;
+import com.google.gerrit.server.git.BanCommit;
 import com.google.gerrit.server.git.ProjectConfig;
-import com.google.gerrit.server.git.ReceiveCommits;
 import com.google.gerrit.server.git.ValidationError;
 import com.google.gerrit.server.project.ProjectControl;
 import com.google.gerrit.server.project.RefControl;
 import com.google.gerrit.server.ssh.SshInfo;
 import com.google.gerrit.server.util.MagicBranch;
 import com.google.inject.Inject;
-import com.google.inject.assistedinject.Assisted;
+import com.google.inject.Singleton;
 
 import com.jcraft.jsch.HostKey;
 
@@ -51,6 +53,7 @@ import org.eclipse.jgit.notes.NoteMap;
 import org.eclipse.jgit.revwalk.FooterKey;
 import org.eclipse.jgit.revwalk.FooterLine;
 import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.util.SystemReader;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -68,107 +71,99 @@ public class CommitValidators {
       .getLogger(CommitValidators.class);
 
   public enum Policy {
-    /** Use {@link #validateForGerritCommits}. */
+    /** Use {@link Factory#forGerritCommits}. */
     GERRIT,
 
-    /** Use {@link #validateForReceiveCommits}. */
+    /** Use {@link Factory#forReceiveCommits}. */
     RECEIVE_COMMITS,
 
     /** Do not validate commits. */
     NONE
   }
 
-  public interface Factory {
+  @Singleton
-    CommitValidators create(RefControl refControl, SshInfo sshInfo,
+  public static class Factory {
-        Repository repo);
+    private final PersonIdent gerritIdent;
-  }
+    private final String canonicalWebUrl;
+    private final DynamicSet<CommitValidationListener> pluginValidators;
+    private final AllUsersName allUsers;
+    private final String installCommitMsgHookCommand;
 
-  private final PersonIdent gerritIdent;
+    @Inject
-  private final RefControl refControl;
+    Factory(@GerritPersonIdent PersonIdent gerritIdent,
-  private final String canonicalWebUrl;
+        @CanonicalWebUrl @Nullable String canonicalWebUrl,
-  private final String installCommitMsgHookCommand;
+        @GerritServerConfig Config cfg,
-  private final SshInfo sshInfo;
+        DynamicSet<CommitValidationListener> pluginValidators,
-  private final Repository repo;
+        AllUsersName allUsers) {
-  private final DynamicSet<CommitValidationListener> commitValidationListeners;
+      this.gerritIdent = gerritIdent;
-  private final AllUsersName allUsers;
+      this.canonicalWebUrl = canonicalWebUrl;
-
+      this.pluginValidators = pluginValidators;
-  @Inject
+      this.allUsers = allUsers;
-  CommitValidators(@GerritPersonIdent PersonIdent gerritIdent,
+      this.installCommitMsgHookCommand = cfg != null
-      @CanonicalWebUrl @Nullable String canonicalWebUrl,
+          ? cfg.getString("gerrit", null, "installCommitMsgHookCommand") : null;
-      @GerritServerConfig Config config,
-      DynamicSet<CommitValidationListener> commitValidationListeners,
-      AllUsersName allUsers,
-      @Assisted SshInfo sshInfo,
-      @Assisted Repository repo,
-      @Assisted RefControl refControl) {
-    this.gerritIdent = gerritIdent;
-    this.canonicalWebUrl = canonicalWebUrl;
-    this.installCommitMsgHookCommand =
-        config.getString("gerrit", null, "installCommitMsgHookCommand");
-    this.commitValidationListeners = commitValidationListeners;
-    this.allUsers = allUsers;
-    this.sshInfo = sshInfo;
-    this.repo = repo;
-    this.refControl = refControl;
-  }
-
-  public List<CommitValidationMessage> validateForReceiveCommits(
-      CommitReceivedEvent receiveEvent, NoteMap rejectCommits)
-      throws CommitValidationException {
-
-    List<CommitValidationListener> validators = new LinkedList<>();
-
-    validators.add(new UploadMergesPermissionValidator(refControl));
-    validators.add(new AmendedGerritMergeCommitValidationListener(
-        refControl, gerritIdent));
-    validators.add(new AuthorUploaderValidator(refControl, canonicalWebUrl));
-    validators.add(new CommitterUploaderValidator(refControl, canonicalWebUrl));
-    validators.add(new SignedOffByValidator(refControl));
-    if (MagicBranch.isMagicBranch(receiveEvent.command.getRefName())
-        || ReceiveCommits.NEW_PATCHSET.matcher(
-            receiveEvent.command.getRefName()).matches()) {
-      validators.add(new ChangeIdValidator(refControl, canonicalWebUrl,
-          installCommitMsgHookCommand, sshInfo));
     }
-    validators.add(new ConfigValidator(refControl, repo, allUsers));
-    validators.add(new BannedCommitsValidator(rejectCommits));
-    validators.add(new PluginCommitValidationListener(commitValidationListeners));
 
-    List<CommitValidationMessage> messages = new LinkedList<>();
+    public CommitValidators create(Policy policy, RefControl refControl,
-
+        SshInfo sshInfo, Repository repo) throws IOException {
-    try {
+      switch (policy) {
-      for (CommitValidationListener commitValidator : validators) {
+        case RECEIVE_COMMITS:
-        messages.addAll(commitValidator.onCommitReceived(receiveEvent));
+          return forReceiveCommits(refControl, sshInfo, repo);
+        case GERRIT:
+          return forGerritCommits(refControl, sshInfo, repo);
+        case NONE:
+          return none();
+        default:
+          throw new IllegalArgumentException("unspported policy: " + policy);
       }
-    } catch (CommitValidationException e) {
-      // Keep the old messages (and their order) in case of an exception
-      messages.addAll(e.getMessages());
-      throw new CommitValidationException(e.getMessage(), messages);
     }
-    return messages;
+
+    private CommitValidators forReceiveCommits(RefControl refControl,
+        SshInfo sshInfo, Repository repo) throws IOException {
+      try (RevWalk rw = new RevWalk(repo)) {
+        NoteMap rejectCommits = BanCommit.loadRejectCommitsMap(repo, rw);
+        return new CommitValidators(ImmutableList.of(
+            new UploadMergesPermissionValidator(refControl),
+            new AmendedGerritMergeCommitValidationListener(
+                refControl, gerritIdent),
+            new AuthorUploaderValidator(refControl, canonicalWebUrl),
+            new CommitterUploaderValidator(refControl, canonicalWebUrl),
+            new SignedOffByValidator(refControl),
+            new ChangeIdValidator(refControl, canonicalWebUrl,
+                installCommitMsgHookCommand, sshInfo),
+            new ConfigValidator(refControl, repo, allUsers),
+            new BannedCommitsValidator(rejectCommits),
+            new PluginCommitValidationListener(pluginValidators)));
+      }
+    }
+
+    private CommitValidators forGerritCommits(RefControl refControl,
+        SshInfo sshInfo, Repository repo) {
+      return new CommitValidators(ImmutableList.of(
+          new UploadMergesPermissionValidator(refControl),
+          new AmendedGerritMergeCommitValidationListener(
+              refControl, gerritIdent),
+          new AuthorUploaderValidator(refControl, canonicalWebUrl),
+          new SignedOffByValidator(refControl),
+          new ChangeIdValidator(refControl, canonicalWebUrl,
+                installCommitMsgHookCommand, sshInfo),
+          new ConfigValidator(refControl, repo, allUsers),
+          new PluginCommitValidationListener(pluginValidators)));
+    }
+
+    private CommitValidators none() {
+      return new CommitValidators(ImmutableList.<CommitValidationListener>of());
+    }
   }
 
-  public List<CommitValidationMessage> validateForGerritCommits(
+  private final List<CommitValidationListener> validators;
+
+  CommitValidators(List<CommitValidationListener> validators) {
+    this.validators = validators;
+  }
+
+  public List<CommitValidationMessage> validate(
       CommitReceivedEvent receiveEvent) throws CommitValidationException {
-
-    List<CommitValidationListener> validators = new LinkedList<>();
-
-    validators.add(new UploadMergesPermissionValidator(refControl));
-    validators.add(new AmendedGerritMergeCommitValidationListener(
-        refControl, gerritIdent));
-    validators.add(new AuthorUploaderValidator(refControl, canonicalWebUrl));
-    validators.add(new SignedOffByValidator(refControl));
-    if (MagicBranch.isMagicBranch(receiveEvent.command.getRefName())
-        || ReceiveCommits.NEW_PATCHSET.matcher(
-            receiveEvent.command.getRefName()).matches()) {
-      validators.add(new ChangeIdValidator(refControl, canonicalWebUrl,
-          installCommitMsgHookCommand, sshInfo));
-    }
-    validators.add(new ConfigValidator(refControl, repo, allUsers));
-    validators.add(new PluginCommitValidationListener(commitValidationListeners));
-
     List<CommitValidationMessage> messages = new LinkedList<>();
-
     try {
       for (CommitValidationListener commitValidator : validators) {
         messages.addAll(commitValidator.onCommitReceived(receiveEvent));
@@ -221,6 +216,9 @@ public class CommitValidators {
     @Override
     public List<CommitValidationMessage> onCommitReceived(
         CommitReceivedEvent receiveEvent) throws CommitValidationException {
+      if (!shouldValidateChangeId(receiveEvent)) {
+        return Collections.emptyList();
+      }
       RevCommit commit = receiveEvent.commit;
       List<CommitValidationMessage> messages = new LinkedList<>();
       List<String> idList = commit.getFooterLines(FooterConstants.CHANGE_ID);
@@ -255,6 +253,11 @@ public class CommitValidators {
       return Collections.emptyList();
     }
 
+    private static boolean shouldValidateChangeId(CommitReceivedEvent event) {
+      return MagicBranch.isMagicBranch(event.command.getRefName())
+          || NEW_PATCHSET.matcher(event.command.getRefName()).matches();
+    }
+
     private CommitValidationMessage getMissingChangeIdErrorMsg(
         final String errMsg, final RevCommit c) {
       StringBuilder sb = new StringBuilder();