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
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 5584718..fac0f7f 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 @@
/** 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
@@ -77,13 +97,6 @@
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>
* Only valid if {@link #patchsetId} is not null.
@@ -120,7 +133,6 @@
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 b859dc2..6c2092b 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 @@
tips.add(r.getObjectId());
}
- int commitOrder = 0;
for (Change chg : submitted) {
ChangeControl ctl;
try {
@@ -568,7 +567,6 @@
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 4a6c8de..6c3e6dd 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.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 @@
return mergeTip;
}
- public void reduceToMinimalMerge(final MergeSorter mergeSorter,
- final List<CodeReviewCommit> toSort) throws MergeException {
- final Collection<CodeReviewCommit> heads;
+ public List<CodeReviewCommit> reduceToMinimalMerge(MergeSorter mergeSorter,
+ Collection<CodeReviewCommit> toSort) throws MergeException {
+ 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);
}
-
- toSort.clear();
- toSort.addAll(heads);
- Collections.sort(toSort, new Comparator<CodeReviewCommit>() {
- @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 2d2678f..35e0689 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 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 @@
@Override
protected CodeReviewCommit _run(CodeReviewCommit mergeTip,
- List<CodeReviewCommit> toMerge) throws MergeException {
- while (!toMerge.isEmpty()) {
- CodeReviewCommit n = toMerge.remove(0);
+ Collection<CodeReviewCommit> toMerge) throws MergeException {
+ List<CodeReviewCommit> 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 a646372..9e37e11 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.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 @@
@Override
protected CodeReviewCommit _run(CodeReviewCommit mergeTip,
- List<CodeReviewCommit> toMerge) throws MergeException {
- args.mergeUtil.reduceToMinimalMerge(args.mergeSorter, toMerge);
+ Collection<CodeReviewCommit> toMerge) throws MergeException {
+ 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()) {
- 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 63a9943..5bf69e9 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.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 @@
@Override
protected CodeReviewCommit _run(CodeReviewCommit mergeTip,
- List<CodeReviewCommit> toMerge) throws MergeException {
- args.mergeUtil.reduceToMinimalMerge(args.mergeSorter, toMerge);
+ Collection<CodeReviewCommit> toMerge) throws MergeException {
+ 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 =
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 cd5e9a3..1c172d0 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.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 {
- args.mergeUtil.reduceToMinimalMerge(args.mergeSorter, toMerge);
+ Collection<CodeReviewCommit> toMerge) throws MergeException {
+ 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(
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 eb0284e..211cb27 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 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 @@
@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()) {
- 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 @@
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();
- 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 930359d..e38e749 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.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 @@
*
* @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 @@
* 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 @@
* 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.
*/
