diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/CodeReviewCommit.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/CodeReviewCommit.java index 5584718b0b..fac0f7f895 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/CodeReviewCommit.java +++ b/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. + *

+ * Operates only on these commits and does not take ancestry into account. + *

+ * Use this in preference to the default order, which comes from {@link + * AnyObjectId} and only orders on SHA-1. + */ + public static final Ordering ORDER = Ordering.natural() + .onResultOf(new Function() { + @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. - *

- * Only valid if {@link #patchsetId} is not null. - */ - int originalOrder; - /** * The result status for this commit. *

@@ -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; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index b859dc209f..6c2092b85b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/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(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java index 4a6c8def10..6c3e6dd67b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java +++ b/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, - final List toSort) throws MergeException { - final Collection heads; + public List reduceToMinimalMerge(MergeSorter mergeSorter, + Collection toSort) throws MergeException { + List result = new ArrayList<>(); try { - heads = mergeSorter.sort(toSort); + result.addAll(mergeSorter.sort(toSort)); } catch (IOException e) { throw new MergeException("Branch head sorting failed", e); } - - toSort.clear(); - toSort.addAll(heads); - Collections.sort(toSort, new Comparator() { - @Override - public int compare(final CodeReviewCommit a, final CodeReviewCommit b) { - return a.originalOrder - b.originalOrder; - } - }); + Collections.sort(result, CodeReviewCommit.ORDER); + return result; } public PatchSetApproval getSubmitter(CodeReviewCommit c) { 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 2d2678f233..35e0689428 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 @@ -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 toMerge) throws MergeException { - while (!toMerge.isEmpty()) { - CodeReviewCommit n = toMerge.remove(0); + Collection toMerge) throws MergeException { + List sorted = CodeReviewCommit.ORDER.sortedCopy(toMerge); + while (!sorted.isEmpty()) { + CodeReviewCommit n = sorted.remove(0); try { if (mergeTip == null) { 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 a646372f82..9e37e1103d 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 @@ -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 toMerge) throws MergeException { - args.mergeUtil.reduceToMinimalMerge(args.mergeSorter, toMerge); + Collection toMerge) throws MergeException { + List 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()) { - CodeReviewCommit n = toMerge.remove(0); + while (!sorted.isEmpty()) { + CodeReviewCommit n = sorted.remove(0); n.setStatusCode(CommitMergeStatus.NOT_FAST_FORWARD); } 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 63a994324a..5bf69e936f 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 @@ -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 toMerge) throws MergeException { - args.mergeUtil.reduceToMinimalMerge(args.mergeSorter, toMerge); + Collection toMerge) throws MergeException { + List 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 = 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 cd5e9a376c..1c172d05cc 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 @@ -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 toMerge) throws MergeException { - args.mergeUtil.reduceToMinimalMerge(args.mergeSorter, toMerge); + Collection toMerge) throws MergeException { + List 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( 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 eb0284e015..211cb2739d 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 @@ -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 toMerge) throws MergeException { + Collection toMerge) throws MergeException { CodeReviewCommit newMergeTip = mergeTip; - sort(toMerge); + List sorted = sort(toMerge); - while (!toMerge.isEmpty()) { - CodeReviewCommit n = toMerge.remove(0); + while (!sorted.isEmpty()) { + 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 toSort) throws MergeException { + private List sort(Collection toSort) + throws MergeException { try { - List sorted = new RebaseSorter( + List result = new RebaseSorter( args.rw, args.alreadyAccepted, args.canMergeFlag).sort(toSort); - toSort.clear(); - toSort.addAll(sorted); + Collections.sort(result, CodeReviewCommit.ORDER); + return result; } catch (IOException e) { throw new MergeException("Commit sorting failed", e); } 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 930359d9e8..e38e749920 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 @@ -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 toMerge) throws MergeException { + Collection toMerge) throws MergeException { refLogIdent = null; return _run(mergeTip, toMerge); } - /** @see #run(CodeReviewCommit, List) */ + /** @see #run(CodeReviewCommit, Collection) */ protected abstract CodeReviewCommit _run(CodeReviewCommit mergeTip, - List toMerge) throws MergeException; + Collection toMerge) throws MergeException; /** * Checks whether the given commit can be merged. @@ -140,7 +141,7 @@ public abstract class SubmitStrategy { * the destination branch. *

* 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. */