Minor cleanup of SubmitStrategy and subclasses

Remove unnecessary finals. Clean up some Javadocs. Reflow some lines. Change-Id: Ia05cff877d2016898f0e646a798f4d01e8e472a1
2015-01-12 13:42:23 -08:00
parent cf4ff534ff
commit e6e0378e76
6 changed files with 104 additions and 127 deletions
--- 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 @@ public class CherryPick extends SubmitStrategy {
  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 @@ public class CherryPick extends SubmitStrategy {

  @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 @@ public class CherryPick extends SubmitStrategy {
            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 @@ public class CherryPick extends SubmitStrategy {

    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 @@ public class CherryPick extends SubmitStrategy {
      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 @@ public class CherryPick extends SubmitStrategy {
          .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 @@ public class CherryPick extends SubmitStrategy {

  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 @@ public class CherryPick extends SubmitStrategy {
  }

  @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);
  }
--- 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 com.google.gerrit.server.git.MergeException;
 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 @@ public class FastForwardOnly extends SubmitStrategy {
  }

  @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);
  }
--- 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 com.google.gerrit.server.git.MergeException;
 import java.util.List;

 public class MergeAlways extends SubmitStrategy {
-
-  MergeAlways(final SubmitStrategy.Arguments args) {
+  MergeAlways(SubmitStrategy.Arguments args) {
    super(args);
  }

@@ -37,13 +36,12 @@ public class MergeAlways extends SubmitStrategy {
      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 @@ public class MergeAlways extends SubmitStrategy {
  }

  @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);
  }
--- 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 @@ import java.util.List;

 public class MergeIfNecessary extends SubmitStrategy {

-  MergeIfNecessary(final SubmitStrategy.Arguments args) {
+  MergeIfNecessary(SubmitStrategy.Arguments args) {
    super(args);
  }

@@ -42,13 +42,12 @@ public class MergeIfNecessary extends SubmitStrategy {

    // 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 @@ public class MergeIfNecessary extends SubmitStrategy {
  }

  @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(
--- 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 @@ public class RebaseIfNecessary extends SubmitStrategy {
  }

  @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 @@ public class RebaseIfNecessary extends SubmitStrategy {

        } 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 @@ public class RebaseIfNecessary extends SubmitStrategy {
            // 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 @@ public class RebaseIfNecessary extends SubmitStrategy {
                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 @@ public class RebaseIfNecessary extends SubmitStrategy {
    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 @@ public class RebaseIfNecessary extends SubmitStrategy {
  }

  @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);
--- 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.Map;
 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 @@ public abstract class SubmitStrategy {
    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 @@ public abstract class SubmitStrategy {

  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 @@ public abstract class SubmitStrategy {
      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 @@ public abstract class SubmitStrategy {
  /**
   * 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 @@ public abstract class SubmitStrategy {
  }

  /**
-   * 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();
    }
  }
 }