Explicitly order CodeReviewCommits when merging

The commits passed in to SubmitStrategy#run were coming straight
out of ChangeAccess#submitted, which should generally use primary
key (numeric change ID) order; these CodeReviewCommits also had
their originalOrder fields set based on this order. SubmitStrategy
implementations were implicitly relying on the order of the List
argument, which results in commits sharing no ancestry being merged in
change ID order. However, merge-based strategies were also re-sorting
the list via MergeUtil#reduceToMinimalMerge, which used the
originalOrder to re-sort the results after reducing the merge.

Instead of these multiple subtle layers of implicit and explicit
ordering, change the SubmitStrategy interface to take Collections, and
make implementations responsible for ordering the commits that are
passed in. This results in a small amount of repeated code, to sort
the inputs, but this is made manageable by adding an explicit Ordering
to CodeReviewCommit. The advantage is that we no longer have some
implementations sorting and some not, so the semantics of the
SubmitStrategy class are slightly more straightforward.

Change-Id: Ia6b0166cdd8b8b331989bf948de3b28b0480d249

gerrit-server/src/main/java/com/google/gerrit/server/git/CodeReviewCommit.java

@@ -14,6 +14,8 @@
 
 package com.google.gerrit.server.git;
 
+import com.google.common.base.Function;
+import com.google.common.collect.Ordering;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.server.notedb.ChangeNotes;
@@ -29,6 +31,24 @@ import java.util.List;
 
 /** Extended commit entity with code review specific metadata. */
 public class CodeReviewCommit extends RevCommit {
+  /**
+   * Default ordering when merging multiple topologically-equivalent commits.
+   * <p>
+   * Operates only on these commits and does not take ancestry into account.
+   * <p>
+   * Use this in preference to the default order, which comes from {@link
+   * AnyObjectId} and only orders on SHA-1.
+   */
+  public static final Ordering<CodeReviewCommit> ORDER = Ordering.natural()
+      .onResultOf(new Function<CodeReviewCommit, Integer>() {
+        @Override
+        public Integer apply(CodeReviewCommit in) {
+          return in.getPatchsetId() != null
+              ? in.getPatchsetId().getParentKey().get()
+              : null;
+        }
+      }).nullsFirst();
+
   public static RevWalk newRevWalk(Repository repo) {
     return new RevWalk(repo) {
       @Override
@@ -76,13 +96,6 @@ public class CodeReviewCommit extends RevCommit {
   /** Change control for the change owner. */
   private ChangeControl control;
 
-  /**
-   * Ordinal position of this commit within the submit queue.
-   * <p>
-   * Only valid if {@link #patchsetId} is not null.
-   */
-  int originalOrder;
-
   /**
    * The result status for this commit.
    * <p>
@@ -120,7 +133,6 @@ public class CodeReviewCommit extends RevCommit {
   public void copyFrom(final CodeReviewCommit src) {
     control = src.control;
     patchsetId = src.patchsetId;
-    originalOrder = src.originalOrder;
     statusCode = src.statusCode;
     missing = src.missing;
   }

gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java

@@ -496,7 +496,6 @@ public class MergeOp {
       tips.add(r.getObjectId());
     }
 
-    int commitOrder = 0;
     for (Change chg : submitted) {
       ChangeControl ctl;
       try {
@@ -568,7 +567,6 @@ public class MergeOp {
 
       commit.setControl(ctl);
       commit.setPatchsetId(ps.getId());
-      commit.originalOrder = commitOrder++;
       commits.put(changeId, commit);
 
       MergeValidators mergeValidators = mergeValidatorsFactory.create();

gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java

@@ -72,7 +72,6 @@ import java.sql.Timestamp;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.Comparator;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashSet;
@@ -153,23 +152,16 @@ public class MergeUtil {
     return mergeTip;
   }
 
-  public void reduceToMinimalMerge(final MergeSorter mergeSorter,
+  public List<CodeReviewCommit> reduceToMinimalMerge(MergeSorter mergeSorter,
-      final List<CodeReviewCommit> toSort) throws MergeException {
+      Collection<CodeReviewCommit> toSort) throws MergeException {
-    final Collection<CodeReviewCommit> heads;
+    List<CodeReviewCommit> result = new ArrayList<>();
     try {
-      heads = mergeSorter.sort(toSort);
+      result.addAll(mergeSorter.sort(toSort));
     } catch (IOException e) {
       throw new MergeException("Branch head sorting failed", e);
     }
-
+    Collections.sort(result, CodeReviewCommit.ORDER);
-    toSort.clear();
+    return result;
-    toSort.addAll(heads);
-    Collections.sort(toSort, new Comparator<CodeReviewCommit>() {
-      @Override
-      public int compare(final CodeReviewCommit a, final CodeReviewCommit b) {
-        return a.originalOrder - b.originalOrder;
-      }
-    });
   }
 
   public PatchSetApproval getSubmitter(CodeReviewCommit c) {

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

@@ -41,6 +41,7 @@ import org.eclipse.jgit.revwalk.RevCommit;
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
@@ -63,9 +64,10 @@ public class CherryPick extends SubmitStrategy {
 
   @Override
   protected CodeReviewCommit _run(CodeReviewCommit mergeTip,
-      List<CodeReviewCommit> toMerge) throws MergeException {
+      Collection<CodeReviewCommit> toMerge) throws MergeException {
-    while (!toMerge.isEmpty()) {
+    List<CodeReviewCommit> sorted = CodeReviewCommit.ORDER.sortedCopy(toMerge);
-      CodeReviewCommit n = toMerge.remove(0);
+    while (!sorted.isEmpty()) {
+      CodeReviewCommit n = sorted.remove(0);
 
       try {
         if (mergeTip == null) {

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

@@ -19,6 +19,7 @@ import com.google.gerrit.server.git.CodeReviewCommit;
 import com.google.gerrit.server.git.CommitMergeStatus;
 import com.google.gerrit.server.git.MergeException;
 
+import java.util.Collection;
 import java.util.List;
 
 public class FastForwardOnly extends SubmitStrategy {
@@ -28,13 +29,14 @@ public class FastForwardOnly extends SubmitStrategy {
 
   @Override
   protected CodeReviewCommit _run(CodeReviewCommit mergeTip,
-      List<CodeReviewCommit> toMerge) throws MergeException {
+      Collection<CodeReviewCommit> toMerge) throws MergeException {
-    args.mergeUtil.reduceToMinimalMerge(args.mergeSorter, toMerge);
+    List<CodeReviewCommit> sorted = args.mergeUtil.reduceToMinimalMerge(
+        args.mergeSorter, toMerge);
     CodeReviewCommit newMergeTip =
-        args.mergeUtil.getFirstFastForward(mergeTip, args.rw, toMerge);
+        args.mergeUtil.getFirstFastForward(mergeTip, args.rw, sorted);
 
-    while (!toMerge.isEmpty()) {
+    while (!sorted.isEmpty()) {
-      CodeReviewCommit n = toMerge.remove(0);
+      CodeReviewCommit n = sorted.remove(0);
       n.setStatusCode(CommitMergeStatus.NOT_FAST_FORWARD);
     }
 

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

@@ -18,6 +18,7 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.server.git.CodeReviewCommit;
 import com.google.gerrit.server.git.MergeException;
 
+import java.util.Collection;
 import java.util.List;
 
 public class MergeAlways extends SubmitStrategy {
@@ -27,18 +28,19 @@ public class MergeAlways extends SubmitStrategy {
 
   @Override
   protected CodeReviewCommit _run(CodeReviewCommit mergeTip,
-      List<CodeReviewCommit> toMerge) throws MergeException {
+      Collection<CodeReviewCommit> toMerge) throws MergeException {
-    args.mergeUtil.reduceToMinimalMerge(args.mergeSorter, toMerge);
+    List<CodeReviewCommit> sorted = args.mergeUtil.reduceToMinimalMerge(
+        args.mergeSorter, toMerge);
 
     if (mergeTip == null) {
       // The branch is unborn. Take a fast-forward resolution to
       // create the branch.
-      mergeTip = toMerge.remove(0);
+      mergeTip = sorted.remove(0);
     }
-    while (!toMerge.isEmpty()) {
+    while (!sorted.isEmpty()) {
       mergeTip = args.mergeUtil.mergeOneCommit(args.serverIdent.get(),
           args.repo, args.rw, args.inserter, args.canMergeFlag, args.destBranch,
-          mergeTip, toMerge.remove(0));
+          mergeTip, sorted.remove(0));
     }
 
     PatchSetApproval submitApproval =

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

@@ -18,33 +18,34 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.server.git.CodeReviewCommit;
 import com.google.gerrit.server.git.MergeException;
 
+import java.util.Collection;
 import java.util.List;
 
 public class MergeIfNecessary extends SubmitStrategy {
-
   MergeIfNecessary(SubmitStrategy.Arguments args) {
     super(args);
   }
 
   @Override
   protected CodeReviewCommit _run(CodeReviewCommit mergeTip,
-      List<CodeReviewCommit> toMerge) throws MergeException {
+      Collection<CodeReviewCommit> toMerge) throws MergeException {
-    args.mergeUtil.reduceToMinimalMerge(args.mergeSorter, toMerge);
+    List<CodeReviewCommit> sorted = args.mergeUtil.reduceToMinimalMerge(
+        args.mergeSorter, toMerge);
 
     if (mergeTip == null) {
       // The branch is unborn. Take a fast-forward resolution to
       // create the branch.
-      mergeTip = toMerge.remove(0);
+      mergeTip = sorted.remove(0);
     }
 
     mergeTip =
-        args.mergeUtil.getFirstFastForward(mergeTip, args.rw, toMerge);
+        args.mergeUtil.getFirstFastForward(mergeTip, args.rw, sorted);
 
     // For every other commit do a pair-wise merge.
-    while (!toMerge.isEmpty()) {
+    while (!sorted.isEmpty()) {
       mergeTip = args.mergeUtil.mergeOneCommit(args.serverIdent.get(),
           args.repo, args.rw, args.inserter, args.canMergeFlag, args.destBranch,
-          mergeTip, toMerge.remove(0));
+          mergeTip, sorted.remove(0));
     }
 
     PatchSetApproval submitApproval = args.mergeUtil.markCleanMerges(

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

@@ -34,6 +34,8 @@ import com.google.gwtorm.server.OrmException;
 import org.eclipse.jgit.lib.ObjectId;
 
 import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -54,12 +56,12 @@ public class RebaseIfNecessary extends SubmitStrategy {
 
   @Override
   protected CodeReviewCommit _run(CodeReviewCommit mergeTip,
-      List<CodeReviewCommit> toMerge) throws MergeException {
+      Collection<CodeReviewCommit> toMerge) throws MergeException {
     CodeReviewCommit newMergeTip = mergeTip;
-    sort(toMerge);
+    List<CodeReviewCommit> sorted = sort(toMerge);
 
-    while (!toMerge.isEmpty()) {
+    while (!sorted.isEmpty()) {
-      CodeReviewCommit n = toMerge.remove(0);
+      CodeReviewCommit n = sorted.remove(0);
 
       if (newMergeTip == null) {
         // The branch is unborn. Take a fast-forward resolution to
@@ -144,12 +146,13 @@ public class RebaseIfNecessary extends SubmitStrategy {
     return newMergeTip;
   }
 
-  private void sort(List<CodeReviewCommit> toSort) throws MergeException {
+  private List<CodeReviewCommit> sort(Collection<CodeReviewCommit> toSort)
+      throws MergeException {
     try {
-      List<CodeReviewCommit> sorted = new RebaseSorter(
+      List<CodeReviewCommit> result = new RebaseSorter(
           args.rw, args.alreadyAccepted, args.canMergeFlag).sort(toSort);
-      toSort.clear();
+      Collections.sort(result, CodeReviewCommit.ORDER);
-      toSort.addAll(sorted);
+      return result;
     } catch (IOException e) {
       throw new MergeException("Commit sorting failed", e);
     }

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

@@ -37,8 +37,8 @@ import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevFlag;
 import org.eclipse.jgit.revwalk.RevWalk;
 
+import java.util.Collection;
 import java.util.Collections;
-import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
@@ -106,19 +106,20 @@ public abstract class SubmitStrategy {
    *
    * @param mergeTip the merge tip.
    * @param toMerge the list of submitted commits that should be merged using
-   *        this submit strategy.
+   *        this submit strategy. Implementations are responsible for ordering
+   *        of commits, and should not modify the input in place.
    * @return the new merge tip.
    * @throws MergeException
    */
   public CodeReviewCommit run(CodeReviewCommit mergeTip,
-      List<CodeReviewCommit> toMerge) throws MergeException {
+      Collection<CodeReviewCommit> toMerge) throws MergeException {
     refLogIdent = null;
     return _run(mergeTip, toMerge);
   }
 
-  /** @see #run(CodeReviewCommit, List) */
+  /** @see #run(CodeReviewCommit, Collection) */
   protected abstract CodeReviewCommit _run(CodeReviewCommit mergeTip,
-      List<CodeReviewCommit> toMerge) throws MergeException;
+      Collection<CodeReviewCommit> toMerge) throws MergeException;
 
   /**
    * Checks whether the given commit can be merged.
@@ -140,7 +141,7 @@ public abstract class SubmitStrategy {
    * the destination branch.
    * <p>
    * The reflog identity may only be set during {@link #run(CodeReviewCommit,
-   * List)}, and this method is invalid to call beforehand.
+   * Collection)}, and this method is invalid to call beforehand.
    *
    * @return the ref log identity, which may be {@code null}.
    */
@@ -155,7 +156,8 @@ public abstract class SubmitStrategy {
    * By default this method returns an empty map, but subclasses may override
    * this method to provide any newly created commits.
    *
-   * This method may only be called after {@link #run(CodeReviewCommit, List)}.
+   * This method may only be called after {@link #run(CodeReviewCommit,
+   * Collection)}.
    *
    * @return new commits created for changes that were merged.
    */