opendev/gerrit
Code Issues Proposed changes

Notice merged commits even if they appear on a different branch

Browse Source 
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
This commit is contained in:
Sven Selberg
2016-10-31 11:18:35 +01:00
committed by David Pursehouse
parent 12898f57a6
commit a61b6eed53
8 changed files with 73 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

34
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
View File

@@ -56,6 +56,7 @@ import com.google.gerrit.server.data.RefUpdateAttribute;
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 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
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();

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

@@ -657,7 +657,10 @@ public class MergeUtil {
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;

13
gerrit-server/src/main/java/com/google/gerrit/server/git/RebaseSorter.java
View File

@@ -31,13 +31,13 @@ import java.util.Set;
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 @@ public class RebaseSorter {
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;

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

@@ -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 com.google.gwtorm.server.OrmException;
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 @@ public class CherryPick extends SubmitStrategy {
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();
}
}

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

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

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

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

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

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

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

@@ -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 com.google.gwtorm.server.OrmException;
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 @@ public class RebaseIfNecessary extends SubmitStrategy {
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 @@ public class RebaseIfNecessary extends SubmitStrategy {
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 @@ public class RebaseIfNecessary extends SubmitStrategy {
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);
}