Use assisted injection for SubmitStrategy.Arguments

Admittedly an Arguments.Factory seems a bit excessively Java-y, but it
has several distinct advantages:

 - There is one clear place to add additional dependencies
   (SubmitStrategy.java), rather than having to add it to
   SubmitStrategy.Arguments plus SubmitStrategyFactory and injecting
   manually.
 - As a result, it is easy to move the additional arguments from the
   constructors of SubmitStrategy subclasses to the base Arguments
   class.
 - The factory interface makes it easy to see at a glance which
   arguments are specific to a single merge operation.
 - Guice gives us free null checks.

Change-Id: I2029cdc9a7ac2dc0fcca5b622b7d892e8f416313

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

@@ -86,6 +86,7 @@ import com.google.gerrit.server.git.NotesBranchUtil;
 import com.google.gerrit.server.git.ReceivePackInitializer;
 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;
@@ -175,6 +176,7 @@ public class GerritGlobalModule extends FactoryModule {
     install(PatchListCacheImpl.module());
     install(ProjectCacheImpl.module());
     install(SectionSortCache.module());
+    install(SubmitStrategy.module());
     install(TagCache.module());
 
     install(new AccessControlModule());

gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java

@@ -33,7 +33,6 @@ import com.google.gerrit.server.git.IntegrationException;
 import com.google.gerrit.server.git.MergeIdenticalTreeException;
 import com.google.gerrit.server.git.MergeTip;
 import com.google.gerrit.server.git.UpdateException;
-import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gwtorm.server.OrmException;
 
@@ -49,13 +48,10 @@ import java.util.List;
 import java.util.Map;
 
 public class CherryPick extends SubmitStrategy {
-  private final PatchSetInfoFactory patchSetInfoFactory;
   private final Map<Change.Id, CodeReviewCommit> newCommits;
 
-  CherryPick(SubmitStrategy.Arguments args,
+  CherryPick(SubmitStrategy.Arguments args) {
-      PatchSetInfoFactory patchSetInfoFactory) {
     super(args);
-    this.patchSetInfoFactory = patchSetInfoFactory;
     this.newCommits = new HashMap<>();
   }
 
@@ -158,7 +154,7 @@ public class CherryPick extends SubmitStrategy {
         ctx.addRefUpdate(
             new ReceiveCommand(ObjectId.zeroId(), newCommit, psId.toRefName()));
         patchSetInfo =
-            patchSetInfoFactory.get(ctx.getRevWalk(), newCommit, psId);
+            args.patchSetInfoFactory.get(ctx.getRevWalk(), newCommit, psId);
       } catch (MergeConflictException mce) {
         // Keep going in the case of a single merge failure; the goal is to
         // cherry-pick as many commits as possible.

gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java

@@ -32,7 +32,6 @@ import com.google.gerrit.server.git.MergeTip;
 import com.google.gerrit.server.git.RebaseSorter;
 import com.google.gerrit.server.git.UpdateException;
 import com.google.gerrit.server.git.validators.CommitValidators;
-import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.project.InvalidChangeOperationException;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gwtorm.server.OrmException;
@@ -47,16 +46,10 @@ import java.util.List;
 import java.util.Map;
 
 public class RebaseIfNecessary extends SubmitStrategy {
-  private final PatchSetInfoFactory patchSetInfoFactory;
-  private final RebaseChangeOp.Factory rebaseFactory;
   private final Map<Change.Id, CodeReviewCommit> newCommits;
 
-  RebaseIfNecessary(SubmitStrategy.Arguments args,
+  RebaseIfNecessary(SubmitStrategy.Arguments args) {
-      PatchSetInfoFactory patchSetInfoFactory,
-      RebaseChangeOp.Factory rebaseFactory) {
     super(args);
-    this.patchSetInfoFactory = patchSetInfoFactory;
-    this.rebaseFactory = rebaseFactory;
     this.newCommits = new HashMap<>();
   }
 
@@ -158,7 +151,7 @@ public class RebaseIfNecessary extends SubmitStrategy {
         return;
       }
 
-      rebaseOp = rebaseFactory.create(
+      rebaseOp = args.rebaseFactory.create(
             toMerge.getControl(),
             // Racy read of patch set is ok; see comments in RebaseChangeOp.
             args.db.patchSets().get(toMerge.getPatchsetId()),
@@ -199,7 +192,7 @@ public class RebaseIfNecessary extends SubmitStrategy {
           ObjectId.fromString(newPatchSet.getRevision().get()));
       mergeTip.moveTipTo(newTip, newTip);
       toMerge.change().setCurrentPatchSet(
-          patchSetInfoFactory.get(args.rw, mergeTip.getCurrentTip(),
+          args.patchSetInfoFactory.get(args.rw, mergeTip.getCurrentTip(),
               newPatchSet.getId()));
       mergeTip.getCurrentTip().copyFrom(toMerge);
       mergeTip.getCurrentTip().setControl(

gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java

@@ -17,11 +17,14 @@ package com.google.gerrit.server.git.strategy;
 import static com.google.common.base.Preconditions.checkNotNull;
 
 import com.google.gerrit.extensions.client.SubmitType;
+import com.google.gerrit.extensions.config.FactoryModule;
 import com.google.gerrit.reviewdb.client.Branch;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
+import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.change.RebaseChangeOp;
 import com.google.gerrit.server.git.BatchUpdate;
 import com.google.gerrit.server.git.CodeReviewCommit;
 import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
@@ -29,7 +32,13 @@ import com.google.gerrit.server.git.IntegrationException;
 import com.google.gerrit.server.git.MergeSorter;
 import com.google.gerrit.server.git.MergeTip;
 import com.google.gerrit.server.git.MergeUtil;
+import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.project.ProjectState;
+import com.google.inject.Module;
+import com.google.inject.assistedinject.Assisted;
+import com.google.inject.assistedinject.AssistedInject;
 
 import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.PersonIdent;
@@ -50,46 +59,87 @@ import java.util.Set;
  * commits should be merged.
  */
 public abstract class SubmitStrategy {
+  public static Module module() {
+    return new FactoryModule() {
+      @Override
+      protected void configure() {
+        factory(SubmitStrategy.Arguments.Factory.class);
+      }
+    };
+  }
+
   static class Arguments {
-    protected final PersonIdent serverIdent;
+    interface Factory {
-    protected final ReviewDb db;
+      Arguments create(
-    protected final BatchUpdate.Factory batchUpdateFactory;
+          Branch.NameKey destBranch,
-    protected final ChangeControl.GenericFactory changeControlFactory;
+          CodeReviewRevWalk rw,
+          IdentifiedUser caller,
+          ObjectInserter inserter,
+          Repository repo,
+          RevFlag canMergeFlag,
+          ReviewDb db,
+          Set<RevCommit> alreadyAccepted);
+    }
 
-    protected final Repository repo;
+    final ApprovalsUtil approvalsUtil;
-    protected final CodeReviewRevWalk rw;
+    final BatchUpdate.Factory batchUpdateFactory;
-    protected final ObjectInserter inserter;
+    final ChangeControl.GenericFactory changeControlFactory;
-    protected final RevFlag canMergeFlag;
+    final PatchSetInfoFactory patchSetInfoFactory;
-    protected final Set<RevCommit> alreadyAccepted;
+    final ProjectCache projectCache;
-    protected final Branch.NameKey destBranch;
+    final PersonIdent serverIdent;
-    protected final ApprovalsUtil approvalsUtil;
+    final RebaseChangeOp.Factory rebaseFactory;
-    protected final MergeUtil mergeUtil;
-    protected final MergeSorter mergeSorter;
-    protected final IdentifiedUser caller;
 
-    Arguments(PersonIdent serverIdent, ReviewDb db,
+    final Branch.NameKey destBranch;
+    final CodeReviewRevWalk rw;
+    final IdentifiedUser caller;
+    final ObjectInserter inserter;
+    final Repository repo;
+    final RevFlag canMergeFlag;
+    final ReviewDb db;
+    final Set<RevCommit> alreadyAccepted;
+
+    final ProjectState project;
+    final MergeSorter mergeSorter;
+    final MergeUtil mergeUtil;
+
+    @AssistedInject
+    Arguments(
+        ApprovalsUtil approvalsUtil,
         BatchUpdate.Factory batchUpdateFactory,
-        ChangeControl.GenericFactory changeControlFactory, Repository repo,
+        ChangeControl.GenericFactory changeControlFactory,
-        CodeReviewRevWalk rw, ObjectInserter inserter, RevFlag canMergeFlag,
+        MergeUtil.Factory mergeUtilFactory,
-        Set<RevCommit> alreadyAccepted, Branch.NameKey destBranch,
+        PatchSetInfoFactory patchSetInfoFactory,
-        ApprovalsUtil approvalsUtil, MergeUtil mergeUtil,
+        @GerritPersonIdent PersonIdent serverIdent,
-        IdentifiedUser caller) {
+        ProjectCache projectCache,
-      this.serverIdent = checkNotNull(serverIdent);
+        RebaseChangeOp.Factory rebaseFactory,
-      this.db = checkNotNull(db);
+        @Assisted Branch.NameKey destBranch,
-      this.batchUpdateFactory = checkNotNull(batchUpdateFactory);
+        @Assisted CodeReviewRevWalk rw,
-      this.changeControlFactory = checkNotNull(changeControlFactory);
+        @Assisted IdentifiedUser caller,
+        @Assisted ObjectInserter inserter,
+        @Assisted Repository repo,
+        @Assisted RevFlag canMergeFlag,
+        @Assisted ReviewDb db,
+        @Assisted Set<RevCommit> alreadyAccepted) {
+      this.approvalsUtil = approvalsUtil;
+      this.batchUpdateFactory = batchUpdateFactory;
+      this.changeControlFactory = changeControlFactory;
+      this.patchSetInfoFactory = patchSetInfoFactory;
+      this.projectCache = projectCache;
+      this.rebaseFactory = rebaseFactory;
 
-      this.repo = checkNotNull(repo);
+      this.serverIdent = serverIdent;
-      this.rw = checkNotNull(rw);
+      this.destBranch = destBranch;
-      this.inserter = checkNotNull(inserter);
+      this.rw = rw;
-      this.canMergeFlag = checkNotNull(canMergeFlag);
+      this.caller = caller;
-      this.alreadyAccepted = checkNotNull(alreadyAccepted);
+      this.inserter = inserter;
-      this.destBranch = checkNotNull(destBranch);
+      this.repo = repo;
-      this.approvalsUtil = checkNotNull(approvalsUtil);
+      this.canMergeFlag = canMergeFlag;
-      this.mergeUtil = checkNotNull(mergeUtil);
+      this.db = db;
-      this.caller = checkNotNull(caller);
+      this.alreadyAccepted = alreadyAccepted;
 
+      this.project = projectCache.get(destBranch.getParentKey());
       this.mergeSorter = new MergeSorter(rw, alreadyAccepted, canMergeFlag);
+      this.mergeUtil = mergeUtilFactory.create(project);
     }
 
     BatchUpdate newBatchUpdate(Timestamp when) {

gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java

@@ -17,25 +17,14 @@ package com.google.gerrit.server.git.strategy;
 import com.google.gerrit.extensions.client.SubmitType;
 import com.google.gerrit.reviewdb.client.Branch;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.ApprovalsUtil;
-import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.change.RebaseChangeOp;
-import com.google.gerrit.server.git.BatchUpdate;
 import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
 import com.google.gerrit.server.git.IntegrationException;
-import com.google.gerrit.server.git.MergeUtil;
-import com.google.gerrit.server.patch.PatchSetInfoFactory;
-import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.NoSuchProjectException;
-import com.google.gerrit.server.project.ProjectCache;
-import com.google.gerrit.server.project.ProjectState;
 import com.google.inject.Inject;
-import com.google.inject.Provider;
 import com.google.inject.Singleton;
 
 import org.eclipse.jgit.lib.ObjectInserter;
-import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevFlag;
@@ -50,33 +39,11 @@ public class SubmitStrategyFactory {
   private static final Logger log = LoggerFactory
       .getLogger(SubmitStrategyFactory.class);
 
-  private final Provider<PersonIdent> myIdent;
+  private final SubmitStrategy.Arguments.Factory argsFactory;
-  private final BatchUpdate.Factory batchUpdateFactory;
-  private final ChangeControl.GenericFactory changeControlFactory;
-  private final PatchSetInfoFactory patchSetInfoFactory;
-  private final RebaseChangeOp.Factory rebaseFactory;
-  private final ProjectCache projectCache;
-  private final ApprovalsUtil approvalsUtil;
-  private final MergeUtil.Factory mergeUtilFactory;
 
   @Inject
-  SubmitStrategyFactory(
+  SubmitStrategyFactory(SubmitStrategy.Arguments.Factory argsFactory) {
-      @GerritPersonIdent Provider<PersonIdent> myIdent,
+    this.argsFactory = argsFactory;
-      BatchUpdate.Factory batchUpdateFactory,
-      ChangeControl.GenericFactory changeControlFactory,
-      PatchSetInfoFactory patchSetInfoFactory,
-      RebaseChangeOp.Factory rebaseFactory,
-      ProjectCache projectCache,
-      ApprovalsUtil approvalsUtil,
-      MergeUtil.Factory mergeUtilFactory) {
-    this.myIdent = myIdent;
-    this.batchUpdateFactory = batchUpdateFactory;
-    this.changeControlFactory = changeControlFactory;
-    this.patchSetInfoFactory = patchSetInfoFactory;
-    this.rebaseFactory = rebaseFactory;
-    this.projectCache = projectCache;
-    this.approvalsUtil = approvalsUtil;
-    this.mergeUtilFactory = mergeUtilFactory;
   }
 
   public SubmitStrategy create(SubmitType submitType, ReviewDb db,
@@ -84,14 +51,14 @@ public class SubmitStrategyFactory {
       RevFlag canMergeFlag, Set<RevCommit> alreadyAccepted,
       Branch.NameKey destBranch, IdentifiedUser caller)
       throws IntegrationException, NoSuchProjectException {
-    ProjectState project = getProject(destBranch);
+    SubmitStrategy.Arguments args = argsFactory.create(destBranch, rw, caller,
-    SubmitStrategy.Arguments args = new SubmitStrategy.Arguments(
+        inserter, repo, canMergeFlag, db, alreadyAccepted);
-        myIdent.get(), db, batchUpdateFactory, changeControlFactory,
+    if (args.project == null) {
-        repo, rw, inserter, canMergeFlag, alreadyAccepted,
+      throw new NoSuchProjectException(destBranch.getParentKey());
-        destBranch,approvalsUtil, mergeUtilFactory.create(project), caller);
+    }
     switch (submitType) {
       case CHERRY_PICK:
-        return new CherryPick(args, patchSetInfoFactory);
+        return new CherryPick(args);
       case FAST_FORWARD_ONLY:
         return new FastForwardOnly(args);
       case MERGE_ALWAYS:
@@ -99,20 +66,11 @@ public class SubmitStrategyFactory {
       case MERGE_IF_NECESSARY:
         return new MergeIfNecessary(args);
       case REBASE_IF_NECESSARY:
-        return new RebaseIfNecessary(args, patchSetInfoFactory, rebaseFactory);
+        return new RebaseIfNecessary(args);
       default:
         String errorMsg = "No submit strategy for: " + submitType;
         log.error(errorMsg);
         throw new IntegrationException(errorMsg);
     }
   }
-
-  private ProjectState getProject(Branch.NameKey branch)
-      throws NoSuchProjectException {
-    ProjectState p = projectCache.get(branch.getParentKey());
-    if (p == null) {
-      throw new NoSuchProjectException(branch.getParentKey());
-    }
-    return p;
-  }
 }