Notice merged commits even if they appear on a different branch
Backport for stable-2.12 of:
Notice merged changes even if they appear on a different branch
Idcaec3f0db9e67b8a8390ddd73c0aca95a654b0b
Check for merge changes contains an optimization which is overeager: It
assumes that all changes that are reachable from any branch are not part
of the set of new changes being applied. This speeds up the walk to
determine the set of changes to be examined, but produces an incorrect
[too small] set.
This results in "internal error: change is new" errors when trying to
submit a change to one branch that is already part of another branch.
For RebaseIfNecessary:
Do not mark child of merge candidates as uninteresting
If a merge candidate is parent of one of the already accepted refs,
the merge candidate will disappear during the sorting in
RebaseSorter#sort if it's child is marked as uninteresting. Only mark
tip of target branch as uninteresting.
Bug: Issue 4158
Inspired-by: Stefan Beller <sbeller@google.com>
Change-Id: I483de5bdb2740bb1a7d3dcb71a15865574231647
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
index bee9091..213f62c 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
@@ -56,6 +56,7 @@
import com.google.gerrit.server.events.ChangeMergedEvent;
import com.google.gerrit.server.events.Event;
import com.google.gerrit.server.events.RefUpdatedEvent;
+import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.testutil.ConfigSuite;
import com.google.gson.reflect.TypeToken;
@@ -164,6 +165,39 @@
assertSubmitter(change2);
}
+ @Test
+ public void submitChangeWhenParentOfOtherBranchTip() throws Exception {
+ // Chain of two commits
+ // Push both to topic-branch
+ // Push the first commit for review and submit
+ //
+ // C2 -- tip of topic branch
+ // |
+ // C1 -- pushed for review
+ // |
+ // C0 -- Master
+ //
+ ProjectConfig config = projectCache.checkedGet(project).getConfig();
+ config.getProject().setCreateNewChangeForAllNotInTarget(
+ InheritableBoolean.TRUE);
+ saveProjectConfig(project, config);
+
+ PushOneCommit push1 = pushFactory.create(db, admin.getIdent(), testRepo,
+ PushOneCommit.SUBJECT, "a.txt", "content");
+ PushOneCommit.Result c1 = push1.to("refs/heads/topic");
+ c1.assertOkStatus();
+ PushOneCommit push2 = pushFactory.create(db, admin.getIdent(), testRepo,
+ PushOneCommit.SUBJECT, "b.txt", "anotherContent");
+ PushOneCommit.Result c2 = push2.to("refs/heads/topic");
+ c2.assertOkStatus();
+
+ PushOneCommit.Result change1 = push1.to("refs/for/master");
+ change1.assertOkStatus();
+
+ approve(change1.getChangeId());
+ submit(change1.getChangeId());
+ }
+
private void assertSubmitter(PushOneCommit.Result change) throws Exception {
ChangeInfo info = get(change.getChangeId(), ListChangesOption.MESSAGES);
assertThat(info.messages).isNotNull();
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 436147c..9124c62 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
@@ -657,7 +657,10 @@
rw.sort(RevSort.REVERSE, true);
rw.markStart(mergeTip);
for (RevCommit c : alreadyAccepted) {
- rw.markUninteresting(c);
+ // If branch was not created by this submit.
+ if (c != mergeTip) {
+ rw.markUninteresting(c);
+ }
}
CodeReviewCommit c;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/RebaseSorter.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/RebaseSorter.java
index 16031ac..be181fa 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/RebaseSorter.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/RebaseSorter.java
@@ -31,13 +31,13 @@
public class RebaseSorter {
private final CodeReviewRevWalk rw;
private final RevFlag canMergeFlag;
- private final Set<RevCommit> accepted;
+ private final RevCommit initialTip;
- public RebaseSorter(CodeReviewRevWalk rw, Set<RevCommit> alreadyAccepted,
+ public RebaseSorter(CodeReviewRevWalk rw, RevCommit initialTip,
RevFlag canMergeFlag) {
this.rw = rw;
this.canMergeFlag = canMergeFlag;
- this.accepted = alreadyAccepted;
+ this.initialTip = initialTip;
}
public List<CodeReviewCommit> sort(Collection<CodeReviewCommit> incoming)
@@ -49,11 +49,8 @@
rw.resetRetain(canMergeFlag);
rw.markStart(n);
- for (RevCommit c : accepted) {
- // n also tip of directly pushed branch => n remains 'interesting' here
- if (!c.equals(n)) {
- rw.markUninteresting(c);
- }
+ if (initialTip != null) {
+ rw.markUninteresting(initialTip);
}
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 61b1b00..e2f869f 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
@@ -14,6 +14,7 @@
package com.google.gerrit.server.git.strategy;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.restapi.MergeConflictException;
@@ -40,6 +41,7 @@
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.transport.ReceiveCommand;
import java.io.IOException;
@@ -238,8 +240,10 @@
toMerge);
mergeTip.moveTipTo(result, toMerge);
}
+ RevCommit initialTip = mergeTip.getInitialTip();
args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag,
- mergeTip.getCurrentTip(), args.alreadyAccepted);
+ mergeTip.getCurrentTip(), initialTip == null
+ ? ImmutableSet.<RevCommit> of() : ImmutableSet.of(initialTip));
setRefLogIdent();
}
}
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 1709659..69943c7 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
@@ -14,11 +14,14 @@
package com.google.gerrit.server.git.strategy;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.CommitMergeStatus;
import com.google.gerrit.server.git.IntegrationException;
import com.google.gerrit.server.git.MergeTip;
+import org.eclipse.jgit.revwalk.RevCommit;
+
import java.util.Collection;
import java.util.List;
@@ -42,8 +45,10 @@
n.setStatusCode(CommitMergeStatus.NOT_FAST_FORWARD);
}
+ RevCommit initialTip = mergeTip.getInitialTip();
args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag,
- newMergeTipCommit, args.alreadyAccepted);
+ mergeTip.getCurrentTip(), initialTip == null
+ ? ImmutableSet.<RevCommit> of() : ImmutableSet.of(initialTip));
setRefLogIdent();
return mergeTip;
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 9f5f521..bd10970 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
@@ -14,11 +14,13 @@
package com.google.gerrit.server.git.strategy;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.IntegrationException;
import com.google.gerrit.server.git.MergeTip;
import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.revwalk.RevCommit;
import java.util.Collection;
import java.util.List;
@@ -53,8 +55,10 @@
mergeTip.moveTipTo(newTip, mergedFrom);
}
+ RevCommit initialTip = mergeTip.getInitialTip();
args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag,
- mergeTip.getCurrentTip(), args.alreadyAccepted);
+ mergeTip.getCurrentTip(), initialTip == null
+ ? ImmutableSet.<RevCommit> of() : ImmutableSet.of(initialTip));
setRefLogIdent();
return mergeTip;
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 4ebe461..46a62ea 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
@@ -14,11 +14,13 @@
package com.google.gerrit.server.git.strategy;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.IntegrationException;
import com.google.gerrit.server.git.MergeTip;
import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.revwalk.RevCommit;
import java.util.Collection;
import java.util.List;
@@ -58,9 +60,10 @@
args.destBranch, branchTip, mergedFrom);
mergeTip.moveTipTo(branchTip, mergedFrom);
}
-
+ RevCommit initialTip = mergeTip.getInitialTip();
args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag, branchTip,
- args.alreadyAccepted);
+ initialTip == null ? ImmutableSet.<RevCommit> of()
+ : ImmutableSet.of(initialTip));
setRefLogIdent();
return mergeTip;
}
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 f9a7e6e..4413e26 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
@@ -14,6 +14,7 @@
package com.google.gerrit.server.git.strategy;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.restapi.MergeConflictException;
@@ -36,6 +37,7 @@
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.revwalk.RevCommit;
import java.io.IOException;
import java.util.Collection;
@@ -67,7 +69,7 @@
protected MergeTip _run(final CodeReviewCommit branchTip,
final Collection<CodeReviewCommit> toMerge) throws IntegrationException {
MergeTip mergeTip = new MergeTip(branchTip, toMerge);
- List<CodeReviewCommit> sorted = sort(toMerge);
+ List<CodeReviewCommit> sorted = sort(toMerge, branchTip);
for (CodeReviewCommit c : sorted) {
if (c.getParentCount() > 1) {
@@ -155,8 +157,10 @@
args.repo, args.rw, args.inserter, args.canMergeFlag,
args.destBranch, mergeTip.getCurrentTip(), n), n);
}
+ RevCommit initialTip = mergeTip.getInitialTip();
args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag,
- mergeTip.getCurrentTip(), args.alreadyAccepted);
+ mergeTip.getCurrentTip(), initialTip == null ?
+ ImmutableSet.<RevCommit>of() : ImmutableSet.of(initialTip));
setRefLogIdent();
} catch (IOException e) {
throw new IntegrationException("Cannot merge " + n.name(), e);
@@ -169,11 +173,11 @@
return mergeTip;
}
- private List<CodeReviewCommit> sort(Collection<CodeReviewCommit> toSort)
- throws IntegrationException {
+ private List<CodeReviewCommit> sort(Collection<CodeReviewCommit> toSort,
+ RevCommit initialTip) throws IntegrationException {
try {
return new RebaseSorter(
- args.rw, args.alreadyAccepted, args.canMergeFlag).sort(toSort);
+ args.rw, initialTip, args.canMergeFlag).sort(toSort);
} catch (IOException e) {
throw new IntegrationException("Commit sorting failed", e);
}
