Minor cleanup of SubmitStrategy and subclasses Remove unnecessary finals. Clean up some Javadocs. Reflow some lines. Change-Id: Ia05cff877d2016898f0e646a798f4d01e8e472a1
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 17cd06a..2d2678f 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
@@ -51,9 +51,9 @@ private final GitReferenceUpdated gitRefUpdated; private final Map<Change.Id, CodeReviewCommit> newCommits; - CherryPick(final SubmitStrategy.Arguments args, - final PatchSetInfoFactory patchSetInfoFactory, - final GitReferenceUpdated gitRefUpdated) { + CherryPick(SubmitStrategy.Arguments args, + PatchSetInfoFactory patchSetInfoFactory, + GitReferenceUpdated gitRefUpdated) { super(args); this.patchSetInfoFactory = patchSetInfoFactory; @@ -63,9 +63,9 @@ @Override protected CodeReviewCommit _run(CodeReviewCommit mergeTip, - final List<CodeReviewCommit> toMerge) throws MergeException { + List<CodeReviewCommit> toMerge) throws MergeException { while (!toMerge.isEmpty()) { - final CodeReviewCommit n = toMerge.remove(0); + CodeReviewCommit n = toMerge.remove(0); try { if (mergeTip == null) { @@ -107,16 +107,13 @@ if (args.rw.isMergedInto(mergeTip, n)) { mergeTip = n; } else { - mergeTip = - args.mergeUtil.mergeOneCommit(args.serverIdent.get(), args.repo, - args.rw, args.inserter, args.canMergeFlag, - args.destBranch, mergeTip, n); - } - final PatchSetApproval submitApproval = - args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag, - mergeTip, args.alreadyAccepted); + mergeTip = args.mergeUtil.mergeOneCommit(args.serverIdent.get(), + args.repo, args.rw, args.inserter, args.canMergeFlag, + args.destBranch, mergeTip, n); + } + PatchSetApproval submitApproval = args.mergeUtil.markCleanMerges( + args.rw, args.canMergeFlag, mergeTip, args.alreadyAccepted); setRefLogIdent(submitApproval); - } else { // One or more dependencies were not met. The status was // already marked on the commit so we have nothing further @@ -139,7 +136,7 @@ args.rw.parseBody(n); - final PatchSetApproval submitAudit = args.mergeUtil.getSubmitter(n); + PatchSetApproval submitAudit = args.mergeUtil.getSubmitter(n); IdentifiedUser cherryPickUser; PersonIdent serverNow = args.serverIdent.get(); @@ -154,21 +151,21 @@ cherryPickCommitterIdent = serverNow; } - final String cherryPickCmtMsg = args.mergeUtil.createCherryPickCommitMessage(n); + String cherryPickCmtMsg = args.mergeUtil.createCherryPickCommitMessage(n); - final CodeReviewCommit newCommit = + CodeReviewCommit newCommit = (CodeReviewCommit) args.mergeUtil.createCherryPickFromCommit(args.repo, args.inserter, mergeTip, n, cherryPickCommitterIdent, cherryPickCmtMsg, args.rw); PatchSet.Id id = ChangeUtil.nextPatchSetId(args.repo, n.change().currentPatchSetId()); - final PatchSet ps = new PatchSet(id); + PatchSet ps = new PatchSet(id); ps.setCreatedOn(TimeUtil.nowTs()); ps.setUploader(cherryPickUser.getAccountId()); ps.setRevision(new RevId(newCommit.getId().getName())); - final RefUpdate ru; + RefUpdate ru; args.db.changes().beginTransaction(n.change().getId()); try { @@ -178,7 +175,7 @@ .setCurrentPatchSet(patchSetInfoFactory.get(newCommit, ps.getId())); args.db.changes().update(Collections.singletonList(n.change())); - final List<PatchSetApproval> approvals = Lists.newArrayList(); + List<PatchSetApproval> approvals = Lists.newArrayList(); for (PatchSetApproval a : args.approvalsUtil.byPatchSet(args.db, n.getControl(), n.getPatchsetId())) { approvals.add(new PatchSetApproval(ps.getId(), a)); @@ -212,7 +209,7 @@ private static void insertAncestors(ReviewDb db, PatchSet.Id id, RevCommit src) throws OrmException { - final int cnt = src.getParentCount(); + int cnt = src.getParentCount(); List<PatchSetAncestor> toInsert = new ArrayList<>(cnt); for (int p = 0; p < cnt; p++) { PatchSetAncestor a; @@ -230,8 +227,8 @@ } @Override - public boolean dryRun(final CodeReviewCommit mergeTip, - final CodeReviewCommit toMerge) throws MergeException { + public boolean dryRun(CodeReviewCommit mergeTip, CodeReviewCommit toMerge) + throws MergeException { return args.mergeUtil.canCherryPick(args.mergeSorter, args.repo, mergeTip, args.rw, toMerge); }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/FastForwardOnly.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/FastForwardOnly.java index 32eeafc..a646372 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/FastForwardOnly.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/FastForwardOnly.java
@@ -22,26 +22,24 @@ import java.util.List; public class FastForwardOnly extends SubmitStrategy { - - FastForwardOnly(final SubmitStrategy.Arguments args) { + FastForwardOnly(SubmitStrategy.Arguments args) { super(args); } @Override - protected CodeReviewCommit _run(final CodeReviewCommit mergeTip, - final List<CodeReviewCommit> toMerge) throws MergeException { + protected CodeReviewCommit _run(CodeReviewCommit mergeTip, + List<CodeReviewCommit> toMerge) throws MergeException { args.mergeUtil.reduceToMinimalMerge(args.mergeSorter, toMerge); - final CodeReviewCommit newMergeTip = + CodeReviewCommit newMergeTip = args.mergeUtil.getFirstFastForward(mergeTip, args.rw, toMerge); while (!toMerge.isEmpty()) { - final CodeReviewCommit n = toMerge.remove(0); + CodeReviewCommit n = toMerge.remove(0); n.setStatusCode(CommitMergeStatus.NOT_FAST_FORWARD); } - final PatchSetApproval submitApproval = - args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag, newMergeTip, - args.alreadyAccepted); + PatchSetApproval submitApproval = args.mergeUtil.markCleanMerges(args.rw, + args.canMergeFlag, newMergeTip, args.alreadyAccepted); setRefLogIdent(submitApproval); return newMergeTip; @@ -53,8 +51,8 @@ } @Override - public boolean dryRun(final CodeReviewCommit mergeTip, - final CodeReviewCommit toMerge) throws MergeException { + public boolean dryRun(CodeReviewCommit mergeTip, + CodeReviewCommit toMerge) throws MergeException { return args.mergeUtil.canFastForward(args.mergeSorter, mergeTip, args.rw, toMerge); }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeAlways.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeAlways.java index 9023623..63a9943 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeAlways.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeAlways.java
@@ -21,8 +21,7 @@ import java.util.List; public class MergeAlways extends SubmitStrategy { - - MergeAlways(final SubmitStrategy.Arguments args) { + MergeAlways(SubmitStrategy.Arguments args) { super(args); } @@ -37,13 +36,12 @@ mergeTip = toMerge.remove(0); } while (!toMerge.isEmpty()) { - mergeTip = - args.mergeUtil.mergeOneCommit(args.serverIdent.get(), args.repo, args.rw, - args.inserter, args.canMergeFlag, args.destBranch, mergeTip, - toMerge.remove(0)); + mergeTip = args.mergeUtil.mergeOneCommit(args.serverIdent.get(), + args.repo, args.rw, args.inserter, args.canMergeFlag, args.destBranch, + mergeTip, toMerge.remove(0)); } - final PatchSetApproval submitApproval = + PatchSetApproval submitApproval = args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag, mergeTip, args.alreadyAccepted); setRefLogIdent(submitApproval); @@ -52,8 +50,8 @@ } @Override - public boolean dryRun(final CodeReviewCommit mergeTip, - final CodeReviewCommit toMerge) throws MergeException { + public boolean dryRun(CodeReviewCommit mergeTip, CodeReviewCommit toMerge) + throws MergeException { return args.mergeUtil.canMerge(args.mergeSorter, args.repo, mergeTip, toMerge); }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeIfNecessary.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeIfNecessary.java index b84586f6..cd5e9a3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeIfNecessary.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeIfNecessary.java
@@ -22,7 +22,7 @@ public class MergeIfNecessary extends SubmitStrategy { - MergeIfNecessary(final SubmitStrategy.Arguments args) { + MergeIfNecessary(SubmitStrategy.Arguments args) { super(args); } @@ -42,13 +42,12 @@ // For every other commit do a pair-wise merge. while (!toMerge.isEmpty()) { - mergeTip = - args.mergeUtil.mergeOneCommit(args.serverIdent.get(), args.repo, args.rw, - args.inserter, args.canMergeFlag, args.destBranch, mergeTip, - toMerge.remove(0)); + mergeTip = args.mergeUtil.mergeOneCommit(args.serverIdent.get(), + args.repo, args.rw, args.inserter, args.canMergeFlag, args.destBranch, + mergeTip, toMerge.remove(0)); } - final PatchSetApproval submitApproval = args.mergeUtil.markCleanMerges( + PatchSetApproval submitApproval = args.mergeUtil.markCleanMerges( args.rw, args.canMergeFlag, mergeTip, args.alreadyAccepted); setRefLogIdent(submitApproval); @@ -56,8 +55,8 @@ } @Override - public boolean dryRun(final CodeReviewCommit mergeTip, - final CodeReviewCommit toMerge) throws MergeException { + public boolean dryRun(CodeReviewCommit mergeTip, CodeReviewCommit toMerge) + throws MergeException { return args.mergeUtil.canFastForward( args.mergeSorter, mergeTip, args.rw, toMerge) || args.mergeUtil.canMerge(
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 17841f4..eb0284e 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
@@ -53,13 +53,13 @@ } @Override - protected CodeReviewCommit _run(final CodeReviewCommit mergeTip, - final List<CodeReviewCommit> toMerge) throws MergeException { + protected CodeReviewCommit _run(CodeReviewCommit mergeTip, + List<CodeReviewCommit> toMerge) throws MergeException { CodeReviewCommit newMergeTip = mergeTip; sort(toMerge); while (!toMerge.isEmpty()) { - final CodeReviewCommit n = toMerge.remove(0); + CodeReviewCommit n = toMerge.remove(0); if (newMergeTip == null) { // The branch is unborn. Take a fast-forward resolution to @@ -82,13 +82,12 @@ } else { try { - final IdentifiedUser uploader = args.identifiedUserFactory.create( + IdentifiedUser uploader = args.identifiedUserFactory.create( args.mergeUtil.getSubmitter(n).getAccountId()); - final PatchSet newPatchSet = - rebaseChange.rebase(args.repo, args.rw, args.inserter, - n.getPatchsetId(), n.change(), uploader, - newMergeTip, args.mergeUtil, args.serverIdent.get(), - false, false, ValidatePolicy.NONE); + PatchSet newPatchSet = rebaseChange.rebase(args.repo, args.rw, + args.inserter, n.getPatchsetId(), n.change(), uploader, + newMergeTip, args.mergeUtil, args.serverIdent.get(), false, + false, ValidatePolicy.NONE); List<PatchSetApproval> approvals = Lists.newArrayList(); for (PatchSetApproval a : args.approvalsUtil.byPatchSet( @@ -98,9 +97,8 @@ // rebaseChange.rebase() may already have copied some approvals, // use upsert, not insert, to avoid constraint violation on database args.db.patchSetApprovals().upsert(approvals); - newMergeTip = - (CodeReviewCommit) args.rw.parseCommit(ObjectId - .fromString(newPatchSet.getRevision().get())); + newMergeTip = (CodeReviewCommit) args.rw.parseCommit( + ObjectId.fromString(newPatchSet.getRevision().get())); n.change().setCurrentPatchSet( patchSetInfoFactory.get(newMergeTip, newPatchSet.getId())); newMergeTip.copyFrom(n); @@ -132,7 +130,7 @@ args.serverIdent.get(), args.repo, args.rw, args.inserter, args.canMergeFlag, args.destBranch, newMergeTip, n); } - final PatchSetApproval submitApproval = args.mergeUtil.markCleanMerges( + PatchSetApproval submitApproval = args.mergeUtil.markCleanMerges( args.rw, args.canMergeFlag, newMergeTip, args.alreadyAccepted); setRefLogIdent(submitApproval); } catch (IOException e) { @@ -146,11 +144,10 @@ return newMergeTip; } - private void sort(final List<CodeReviewCommit> toSort) throws MergeException { + private void sort(List<CodeReviewCommit> toSort) throws MergeException { try { - final List<CodeReviewCommit> sorted = - new RebaseSorter(args.rw, args.alreadyAccepted, args.canMergeFlag) - .sort(toSort); + List<CodeReviewCommit> sorted = new RebaseSorter( + args.rw, args.alreadyAccepted, args.canMergeFlag).sort(toSort); toSort.clear(); toSort.addAll(sorted); } catch (IOException e) { @@ -164,8 +161,8 @@ } @Override - public boolean dryRun(final CodeReviewCommit mergeTip, - final CodeReviewCommit toMerge) throws MergeException { + public boolean dryRun(CodeReviewCommit mergeTip, CodeReviewCommit toMerge) + throws MergeException { return !args.mergeUtil.hasMissingDependencies(args.mergeSorter, toMerge) && args.mergeUtil.canCherryPick(args.mergeSorter, args.repo, mergeTip, args.rw, toMerge);
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 a864b6c..930359d 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
@@ -43,14 +43,12 @@ import java.util.Set; /** - * Base class that submit strategies must extend. A submit strategy for a - * certain {@link SubmitType} defines how the submitted commits should be - * merged. + * Base class that submit strategies must extend. + * <p> + * A submit strategy for a certain {@link SubmitType} defines how the submitted + * commits should be merged. */ public abstract class SubmitStrategy { - - private PersonIdent refLogIdent; - static class Arguments { protected final IdentifiedUser.GenericFactory identifiedUserFactory; protected final Provider<PersonIdent> serverIdent; @@ -68,13 +66,13 @@ protected final ChangeIndexer indexer; protected final MergeSorter mergeSorter; - Arguments(final IdentifiedUser.GenericFactory identifiedUserFactory, - final Provider<PersonIdent> serverIdent, final ReviewDb db, - final ChangeControl.GenericFactory changeControlFactory, - final Repository repo, final RevWalk rw, final ObjectInserter inserter, - final RevFlag canMergeFlag, final Set<RevCommit> alreadyAccepted, - final Branch.NameKey destBranch, final ApprovalsUtil approvalsUtil, - final MergeUtil mergeUtil, final ChangeIndexer indexer) { + Arguments(IdentifiedUser.GenericFactory identifiedUserFactory, + Provider<PersonIdent> serverIdent, ReviewDb db, + ChangeControl.GenericFactory changeControlFactory, Repository repo, + RevWalk rw, ObjectInserter inserter, RevFlag canMergeFlag, + Set<RevCommit> alreadyAccepted, Branch.NameKey destBranch, + ApprovalsUtil approvalsUtil, MergeUtil mergeUtil, + ChangeIndexer indexer) { this.identifiedUserFactory = identifiedUserFactory; this.serverIdent = serverIdent; this.db = db; @@ -95,48 +93,41 @@ protected final Arguments args; - SubmitStrategy(final Arguments args) { + private PersonIdent refLogIdent; + + SubmitStrategy(Arguments args) { this.args = args; } /** - * Runs this submit strategy. If possible the provided commits will be merged - * with this submit strategy. + * Runs this submit strategy. + * <p> + * If possible, the provided commits will be merged with this submit strategy. * - * @param mergeTip the mergeTip + * @param mergeTip the merge tip. * @param toMerge the list of submitted commits that should be merged using - * this submit strategy - * @return the new mergeTip + * this submit strategy. + * @return the new merge tip. * @throws MergeException */ - public final CodeReviewCommit run(final CodeReviewCommit mergeTip, - final List<CodeReviewCommit> toMerge) throws MergeException { + public CodeReviewCommit run(CodeReviewCommit mergeTip, + List<CodeReviewCommit> toMerge) throws MergeException { refLogIdent = null; return _run(mergeTip, toMerge); } - /** - * Runs this submit strategy. If possible the provided commits will be merged - * with this submit strategy. - * - * @param mergeTip the mergeTip - * @param toMerge the list of submitted commits that should be merged using - * this submit strategy - * @return the new mergeTip - * @throws MergeException - */ + /** @see #run(CodeReviewCommit, List) */ protected abstract CodeReviewCommit _run(CodeReviewCommit mergeTip, List<CodeReviewCommit> toMerge) throws MergeException; /** * Checks whether the given commit can be merged. * - * Subclasses must ensure that invoking this method does neither modify the + * Implementations must ensure that invoking this method modifies neither the * git repository nor the Gerrit database. * - * @param mergeTip the mergeTip - * @param toMerge the commit for which it should be checked whether it can be - * merged or not + * @param mergeTip the merge tip. + * @param toMerge the commit that should be checked. * @return {@code true} if the given commit can be merged, otherwise * {@code false} * @throws MergeException @@ -145,14 +136,13 @@ CodeReviewCommit toMerge) throws MergeException; /** - * Returns the PersonIdent that should be used for the ref log entries when - * updating the destination branch. The ref log identity may be set after the - * {@link #run(CodeReviewCommit, List)} method finished. + * Returns the identity that should be used for reflog entries when updating + * the destination branch. + * <p> + * The reflog identity may only be set during {@link #run(CodeReviewCommit, + * List)}, and this method is invalid to call beforehand. * - * Do only call this method after the {@link #run(CodeReviewCommit, List)} - * method has been invoked. - * - * @return the ref log identity, may be {@code null} + * @return the ref log identity, which may be {@code null}. */ public final PersonIdent getRefLogIdent() { return refLogIdent; @@ -161,24 +151,23 @@ /** * Returns all commits that have been newly created for the changes that are * getting merged. + * <p> + * By default this method returns an empty map, but subclasses may override + * this method to provide any newly created commits. * - * By default this method is returning an empty map, but subclasses may - * overwrite this method to provide newly created commits. + * This method may only be called after {@link #run(CodeReviewCommit, List)}. * - * Do only call this method after the {@link #run(CodeReviewCommit, List)} - * method has been invoked. - * - * @return new commits created for changes that are getting merged + * @return new commits created for changes that were merged. */ public Map<Change.Id, CodeReviewCommit> getNewCommits() { return Collections.emptyMap(); } /** - * Returns whether a merge that failed with - * {@link Result#LOCK_FAILURE} should be retried. - * - * May be overwritten by subclasses. + * Returns whether a merge that failed with {@link Result#LOCK_FAILURE} should + * be retried. + * <p> + * May be overridden by subclasses. * * @return {@code true} if a merge that failed with * {@link Result#LOCK_FAILURE} should be retried, otherwise @@ -189,15 +178,14 @@ } /** - * Sets the ref log identity if it wasn't set yet. + * Set the ref log identity if it wasn't set yet. * * @param submitApproval the approval that submitted the patch set */ - protected final void setRefLogIdent(final PatchSetApproval submitApproval) { + protected final void setRefLogIdent(PatchSetApproval submitApproval) { if (refLogIdent == null && submitApproval != null) { - refLogIdent = - args.identifiedUserFactory.create(submitApproval.getAccountId()) - .newRefLogIdent(); + refLogIdent = args.identifiedUserFactory.create( + submitApproval.getAccountId()) .newRefLogIdent(); } } }