From 0afa6e6a0945dc17db72b35d5830b78a886703b4 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 6 Jan 2016 16:57:47 -0500 Subject: [PATCH] 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 --- .../server/config/GerritGlobalModule.java | 2 + .../server/git/strategy/CherryPick.java | 8 +- .../git/strategy/RebaseIfNecessary.java | 13 +- .../server/git/strategy/SubmitStrategy.java | 116 +++++++++++++----- .../git/strategy/SubmitStrategyFactory.java | 62 ++-------- 5 files changed, 100 insertions(+), 101 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java index 564e0fb53e..8bcbdac162 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/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()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java index 03c3eb1d95..63ab65cb53 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java +++ b/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 newCommits; - CherryPick(SubmitStrategy.Arguments args, - PatchSetInfoFactory patchSetInfoFactory) { + CherryPick(SubmitStrategy.Arguments args) { 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. diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java index b611274341..be5fa57bd4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java +++ b/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 newCommits; - RebaseIfNecessary(SubmitStrategy.Arguments args, - PatchSetInfoFactory patchSetInfoFactory, - RebaseChangeOp.Factory rebaseFactory) { + RebaseIfNecessary(SubmitStrategy.Arguments args) { 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( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java index 560d55f545..b826ae0da8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java +++ b/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; - protected final ReviewDb db; - protected final BatchUpdate.Factory batchUpdateFactory; - protected final ChangeControl.GenericFactory changeControlFactory; + interface Factory { + Arguments create( + Branch.NameKey destBranch, + CodeReviewRevWalk rw, + IdentifiedUser caller, + ObjectInserter inserter, + Repository repo, + RevFlag canMergeFlag, + ReviewDb db, + Set alreadyAccepted); + } - protected final Repository repo; - protected final CodeReviewRevWalk rw; - protected final ObjectInserter inserter; - protected final RevFlag canMergeFlag; - protected final Set alreadyAccepted; - protected final Branch.NameKey destBranch; - protected final ApprovalsUtil approvalsUtil; - protected final MergeUtil mergeUtil; - protected final MergeSorter mergeSorter; - protected final IdentifiedUser caller; + final ApprovalsUtil approvalsUtil; + final BatchUpdate.Factory batchUpdateFactory; + final ChangeControl.GenericFactory changeControlFactory; + final PatchSetInfoFactory patchSetInfoFactory; + final ProjectCache projectCache; + final PersonIdent serverIdent; + final RebaseChangeOp.Factory rebaseFactory; - 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 alreadyAccepted; + + final ProjectState project; + final MergeSorter mergeSorter; + final MergeUtil mergeUtil; + + @AssistedInject + Arguments( + ApprovalsUtil approvalsUtil, BatchUpdate.Factory batchUpdateFactory, - ChangeControl.GenericFactory changeControlFactory, Repository repo, - CodeReviewRevWalk rw, ObjectInserter inserter, RevFlag canMergeFlag, - Set alreadyAccepted, Branch.NameKey destBranch, - ApprovalsUtil approvalsUtil, MergeUtil mergeUtil, - IdentifiedUser caller) { - this.serverIdent = checkNotNull(serverIdent); - this.db = checkNotNull(db); - this.batchUpdateFactory = checkNotNull(batchUpdateFactory); - this.changeControlFactory = checkNotNull(changeControlFactory); + ChangeControl.GenericFactory changeControlFactory, + MergeUtil.Factory mergeUtilFactory, + PatchSetInfoFactory patchSetInfoFactory, + @GerritPersonIdent PersonIdent serverIdent, + ProjectCache projectCache, + RebaseChangeOp.Factory rebaseFactory, + @Assisted Branch.NameKey destBranch, + @Assisted CodeReviewRevWalk rw, + @Assisted IdentifiedUser caller, + @Assisted ObjectInserter inserter, + @Assisted Repository repo, + @Assisted RevFlag canMergeFlag, + @Assisted ReviewDb db, + @Assisted Set alreadyAccepted) { + this.approvalsUtil = approvalsUtil; + this.batchUpdateFactory = batchUpdateFactory; + this.changeControlFactory = changeControlFactory; + this.patchSetInfoFactory = patchSetInfoFactory; + this.projectCache = projectCache; + this.rebaseFactory = rebaseFactory; - this.repo = checkNotNull(repo); - this.rw = checkNotNull(rw); - this.inserter = checkNotNull(inserter); - this.canMergeFlag = checkNotNull(canMergeFlag); - this.alreadyAccepted = checkNotNull(alreadyAccepted); - this.destBranch = checkNotNull(destBranch); - this.approvalsUtil = checkNotNull(approvalsUtil); - this.mergeUtil = checkNotNull(mergeUtil); - this.caller = checkNotNull(caller); + this.serverIdent = serverIdent; + this.destBranch = destBranch; + this.rw = rw; + this.caller = caller; + this.inserter = inserter; + this.repo = repo; + this.canMergeFlag = canMergeFlag; + this.db = db; + 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) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java index 4eeed6725a..0480a96e1a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java +++ b/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 myIdent; - 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; + private final SubmitStrategy.Arguments.Factory argsFactory; @Inject - SubmitStrategyFactory( - @GerritPersonIdent Provider myIdent, - 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; + SubmitStrategyFactory(SubmitStrategy.Arguments.Factory argsFactory) { + this.argsFactory = argsFactory; } public SubmitStrategy create(SubmitType submitType, ReviewDb db, @@ -84,14 +51,14 @@ public class SubmitStrategyFactory { RevFlag canMergeFlag, Set alreadyAccepted, Branch.NameKey destBranch, IdentifiedUser caller) throws IntegrationException, NoSuchProjectException { - ProjectState project = getProject(destBranch); - SubmitStrategy.Arguments args = new SubmitStrategy.Arguments( - myIdent.get(), db, batchUpdateFactory, changeControlFactory, - repo, rw, inserter, canMergeFlag, alreadyAccepted, - destBranch,approvalsUtil, mergeUtilFactory.create(project), caller); + SubmitStrategy.Arguments args = argsFactory.create(destBranch, rw, caller, + inserter, repo, canMergeFlag, db, alreadyAccepted); + if (args.project == null) { + throw new NoSuchProjectException(destBranch.getParentKey()); + } 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; - } }