Refactor CommitValidators to reduce repetition

Use a manual factory. Move the banned commit map loading into the
factory method, throwing IOException, which we couldn't do with an
assisted factory. At this point CommitValidators itself is just a
thin wrapper around the list of CommitValidationListeners.

Since the BanCommit stuff is now in the factory, we can move the
switch on the Policy into the factory method as well, which greatly
reduces complexity at call sites. Callers are still responsible for
constructing their own CommitReceivedEvents, so there is still some
code there.

Change-Id: I9a7060e3a9c43f92f34da66e958ab5d1cbdaf116

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 RefControl refControl;
     private final String canonicalWebUrl;
-  private final String installCommitMsgHookCommand;
+    private final DynamicSet<CommitValidationListener> pluginValidators;
-  private final SshInfo sshInfo;
-  private final Repository repo;
-  private final DynamicSet<CommitValidationListener> commitValidationListeners;
     private final AllUsersName allUsers;
+    private final String installCommitMsgHookCommand;
 
     @Inject
-  CommitValidators(@GerritPersonIdent PersonIdent gerritIdent,
+    Factory(@GerritPersonIdent PersonIdent gerritIdent,
         @CanonicalWebUrl @Nullable String canonicalWebUrl,
-      @GerritServerConfig Config config,
+        @GerritServerConfig Config cfg,
-      DynamicSet<CommitValidationListener> commitValidationListeners,
+        DynamicSet<CommitValidationListener> pluginValidators,
-      AllUsersName allUsers,
+        AllUsersName allUsers) {
-      @Assisted SshInfo sshInfo,
-      @Assisted Repository repo,
-      @Assisted RefControl refControl) {
       this.gerritIdent = gerritIdent;
       this.canonicalWebUrl = canonicalWebUrl;
-    this.installCommitMsgHookCommand =
+      this.pluginValidators = pluginValidators;
-        config.getString("gerrit", null, "installCommitMsgHookCommand");
-    this.commitValidationListeners = commitValidationListeners;
       this.allUsers = allUsers;
-    this.sshInfo = sshInfo;
+      this.installCommitMsgHookCommand = cfg != null
-    this.repo = repo;
+          ? cfg.getString("gerrit", null, "installCommitMsgHookCommand") : null;
-    this.refControl = refControl;
     }
 
-  public List<CommitValidationMessage> validateForReceiveCommits(
+    public CommitValidators create(Policy policy, RefControl refControl,
-      CommitReceivedEvent receiveEvent, NoteMap rejectCommits)
+        SshInfo sshInfo, Repository repo) throws IOException {
-      throws CommitValidationException {
+      switch (policy) {
-
+        case RECEIVE_COMMITS:
-    List<CommitValidationListener> validators = new LinkedList<>();
+          return forReceiveCommits(refControl, sshInfo, repo);
-
+        case GERRIT:
-    validators.add(new UploadMergesPermissionValidator(refControl));
+          return forGerritCommits(refControl, sshInfo, repo);
-    validators.add(new AmendedGerritMergeCommitValidationListener(
+        case NONE:
-        refControl, gerritIdent));
+          return none();
-    validators.add(new AuthorUploaderValidator(refControl, canonicalWebUrl));
+        default:
-    validators.add(new CommitterUploaderValidator(refControl, canonicalWebUrl));
+          throw new IllegalArgumentException("unspported policy: " + policy);
-    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<>();
-
-    try {
-      for (CommitValidationListener commitValidator : validators) {
-        messages.addAll(commitValidator.onCommitReceived(receiveEvent));
-      }
-    } 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;
     }
 
-  public List<CommitValidationMessage> validateForGerritCommits(
+    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());
+    }
+  }
+
+  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();