opendev/gerrit
Code Issues Proposed changes

Do not modify fast forward commit for submodules

Browse Source 
When superproject has a merge submit strategy and submodule
subscriptions, do not use FastForwardOp even a change can be
fast-forwarded.

Submodule in Fast Forward submit strategy is supported by create a
separate gitlink commit.

Change-Id: I3822977cbf32acbaca01612ba411c495cfcc5722
This commit is contained in:
Zhen Chen
2016-08-25 11:20:32 -07:00
parent 4c93a559b6
commit b8983698a5
9 changed files with 118 additions and 78 deletions
Download Patch File Download Diff File Expand all files Collapse all files

24
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractSubmoduleSubscription.java
View File

@@ -29,6 +29,7 @@ import com.google.gerrit.server.git.ProjectConfig;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevObject;
import org.eclipse.jgit.revwalk.RevTree;
@@ -75,8 +76,9 @@ public abstract class AbstractSubmoduleSubscription extends AbstractDaemonTest {
}
protected TestRepository<?> createProjectWithPush(String name,
@Nullable Project.NameKey parent, SubmitType submitType) throws Exception {
Project.NameKey project = createProject(name, parent, submitType);
@Nullable Project.NameKey parent, boolean createEmptyCommit,
SubmitType submitType) throws Exception {
Project.NameKey project = createProject(name, parent, createEmptyCommit, submitType);
grant(Permission.PUSH, project, "refs/heads/*");
grant(Permission.SUBMIT, project, "refs/for/refs/heads/*");
return cloneProject(project);
@@ -84,12 +86,17 @@ public abstract class AbstractSubmoduleSubscription extends AbstractDaemonTest {
protected TestRepository<?> createProjectWithPush(String name,
@Nullable Project.NameKey parent) throws Exception {
return createProjectWithPush(name, parent, getSubmitType());
return createProjectWithPush(name, parent, true, getSubmitType());
}
protected TestRepository<?> createProjectWithPush(String name,
boolean createEmptyCommit) throws Exception {
return createProjectWithPush(name, null, createEmptyCommit, getSubmitType());
}
protected TestRepository<?> createProjectWithPush(String name)
throws Exception {
return createProjectWithPush(name, null);
return createProjectWithPush(name, null, true, getSubmitType());
}
private static AtomicInteger contentCounter = new AtomicInteger(0);
@@ -305,8 +312,13 @@ public abstract class AbstractSubmoduleSubscription extends AbstractDaemonTest {
String submodule) throws Exception {
submodule = name(submodule);
ObjectId commitId = repo.git().fetch().setRemote("origin").call()
.getAdvertisedRef("refs/heads/" + branch).getObjectId();
Ref branchTip = repo.git().fetch().setRemote("origin").call()
.getAdvertisedRef("refs/heads/" + branch);
if (branchTip == null) {
return false;
}
ObjectId commitId = branchTip.getObjectId();
RevWalk rw = repo.getRevWalk();
RevCommit c = rw.parseCommit(commitId);

50
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java
View File

@@ -257,6 +257,56 @@ public class SubmoduleSubscriptionsWholeTopicMergeIT
.isEqualTo(superPreviousId);
}
@Test
public void testDoNotUseFastForward() throws Exception {
TestRepository<?> superRepo = createProjectWithPush("super-project", false);
TestRepository<?> sub = createProjectWithPush("sub", false);
allowMatchingSubmoduleSubscription("sub", "refs/heads/master",
"super-project", "refs/heads/master");
createSubmoduleSubscription(superRepo, "master", "sub", "master");
ObjectId subId =
pushChangeTo(sub, "refs/for/master", "some message", "same-topic");
ObjectId superId =
pushChangeTo(superRepo, "refs/for/master", "some message", "same-topic");
String subChangeId = getChangeId(sub, subId).get();
approve(subChangeId);
approve(getChangeId(superRepo, superId).get());
gApi.changes().id(subChangeId).current().submit();
expectToHaveSubmoduleState(superRepo, "master", "sub", sub, "master");
RevCommit superHead = getRemoteHead(name("super-project"), "master");
assertThat(superHead.getShortMessage()).contains("some message");
assertThat(superHead.getId()).isNotEqualTo(superId);
}
@Test
public void testUseFastForwardWhenNoSubmodule() throws Exception {
TestRepository<?> superRepo = createProjectWithPush("super-project", false);
TestRepository<?> sub = createProjectWithPush("sub", false);
ObjectId subId =
pushChangeTo(sub, "refs/for/master", "some message", "same-topic");
ObjectId superId =
pushChangeTo(superRepo, "refs/for/master", "some message", "same-topic");
String subChangeId = getChangeId(sub, subId).get();
approve(subChangeId);
approve(getChangeId(superRepo, superId).get());
gApi.changes().id(subChangeId).current().submit();
RevCommit superHead = getRemoteHead(name("super-project"), "master");
assertThat(superHead.getShortMessage()).isEqualTo("some message");
assertThat(superHead.getId()).isEqualTo(superId);
}
@Test
public void testDifferentPaths() throws Exception {
TestRepository<?> superRepo = createProjectWithPush("super-project");

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

@@ -550,6 +550,10 @@ public class MergeOp implements AutoCloseable {
submitting.submitType(), ob.oldTip, submoduleOp, dryrun);
strategies.add(strategy);
strategy.addOps(or.getUpdate(), commitsToSubmit);
if (submitting.submitType().equals(SubmitType.FAST_FORWARD_ONLY) &&
submoduleOp.hasSubscription(branch)) {
submoduleOp.addOp(or.getUpdate(), branch);
}
} else {
// no open change for this branch
// add submodule triggered op into BatchUpdate

18
gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java
View File

@@ -354,12 +354,16 @@ public class SubmoduleOp {
}
CodeReviewCommit currentCommit;
Ref r = or.repo.exactRef(subscriber.get());
if (r == null) {
throw new SubmoduleException(
"The branch was probably deleted from the subscriber repository");
if (branchTips.containsKey(subscriber)) {
currentCommit = branchTips.get(subscriber);
} else {
Ref r = or.repo.exactRef(subscriber.get());
if (r == null) {
throw new SubmoduleException(
"The branch was probably deleted from the subscriber repository");
}
currentCommit = or.rw.parseCommit(r.getObjectId());
}
currentCommit = or.rw.parseCommit(r.getObjectId());
StringBuilder msgbuf = new StringBuilder("");
PersonIdent author = null;
@@ -436,7 +440,9 @@ public class SubmoduleOp {
commit.setAuthor(currentCommit.getAuthorIdent());
commit.setCommitter(myIdent);
ObjectId id = or.ins.insert(commit);
return or.rw.parseCommit(id);
CodeReviewCommit newCommit = or.rw.parseCommit(id);
newCommit.copyFrom(currentCommit);
return newCommit;
}
private RevCommit updateSubmodule(DirCache dc, DirCacheEditor ed,

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

@@ -48,14 +48,14 @@ public class CherryPick extends SubmitStrategy {
@Override
public List<SubmitStrategyOp> buildOps(
Collection<CodeReviewCommit> toMerge) {
Collection<CodeReviewCommit> toMerge) throws IntegrationException {
List<CodeReviewCommit> sorted = CodeReviewCommit.ORDER.sortedCopy(toMerge);
List<SubmitStrategyOp> ops = new ArrayList<>(sorted.size());
boolean first = true;
while (!sorted.isEmpty()) {
CodeReviewCommit n = sorted.remove(0);
if (first && args.mergeTip.getInitialTip() == null) {
ops.add(new CherryPickUnbornRootOp(n));
ops.add(new FastForwardOp(args, n));
} else if (n.getParentCount() == 0) {
ops.add(new CherryPickRootOp(n));
} else if (n.getParentCount() == 1) {
@@ -68,21 +68,6 @@ public class CherryPick extends SubmitStrategy {
return ops;
}
private class CherryPickUnbornRootOp extends SubmitStrategyOp {
private CherryPickUnbornRootOp(CodeReviewCommit toMerge) {
super(CherryPick.this.args, toMerge);
}
@Override
protected void updateRepoImpl(RepoContext ctx) throws IntegrationException {
// The branch is unborn. Take fast-forward resolution to create the
// branch.
CodeReviewCommit newCommit = amendGitlink(toMerge);
args.mergeTip.moveTipTo(newCommit, toMerge);
newCommit.setStatusCode(CommitMergeStatus.CLEAN_MERGE);
}
}
private class CherryPickRootOp extends SubmitStrategyOp {
private CherryPickRootOp(CodeReviewCommit toMerge) {
super(CherryPick.this.args, toMerge);
@@ -191,8 +176,9 @@ public class CherryPick extends SubmitStrategy {
// different first parent. So instead behave as though MERGE_IF_NECESSARY
// was configured.
MergeTip mergeTip = args.mergeTip;
if (args.rw.isMergedInto(mergeTip.getCurrentTip(), toMerge)) {
mergeTip.moveTipTo(amendGitlink(toMerge), toMerge);
if (args.rw.isMergedInto(mergeTip.getCurrentTip(), toMerge) &&
!args.submoduleOp.hasSubscription(args.destBranch)) {
mergeTip.moveTipTo(toMerge, toMerge);
} else {
PersonIdent myIdent = new PersonIdent(args.serverIdent, ctx.getWhen());
CodeReviewCommit result = args.mergeUtil.mergeOneCommit(myIdent,
@@ -200,9 +186,9 @@ public class CherryPick extends SubmitStrategy {
mergeTip.getCurrentTip(), toMerge);
result = amendGitlink(result);
mergeTip.moveTipTo(result, toMerge);
args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag,
mergeTip.getCurrentTip(), args.alreadyAccepted);
}
args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag,
mergeTip.getCurrentTip(), args.alreadyAccepted);
}
}

2
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/FastForwardOp.java
View File

@@ -25,6 +25,6 @@ class FastForwardOp extends SubmitStrategyOp {
@Override
protected void updateRepoImpl(RepoContext ctx) throws IntegrationException {
args.mergeTip.moveTipTo(amendGitlink(toMerge), toMerge);
args.mergeTip.moveTipTo(toMerge, toMerge);
}
}

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

@@ -32,11 +32,15 @@ public class MergeIfNecessary extends SubmitStrategy {
List<CodeReviewCommit> sorted =
args.mergeUtil.reduceToMinimalMerge(args.mergeSorter, toMerge);
List<SubmitStrategyOp> ops = new ArrayList<>(sorted.size());
CodeReviewCommit firstFastForward = args.mergeUtil.getFirstFastForward(
if (args.mergeTip.getInitialTip() == null || !args.submoduleOp
.hasSubscription(args.destBranch)) {
CodeReviewCommit firstFastForward = args.mergeUtil.getFirstFastForward(
args.mergeTip.getInitialTip(), args.rw, sorted);
if (firstFastForward != null &&
!firstFastForward.equals(args.mergeTip.getInitialTip())) {
ops.add(new FastForwardOp(args, firstFastForward));
if (firstFastForward != null &&
!firstFastForward.equals(args.mergeTip.getInitialTip())) {
ops.add(new FastForwardOp(args, firstFastForward));
}
}
// For every other commit do a pair-wise merge.

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

@@ -63,7 +63,7 @@ public class RebaseIfNecessary extends SubmitStrategy {
while (!sorted.isEmpty()) {
CodeReviewCommit n = sorted.remove(0);
if (first && args.mergeTip.getInitialTip() == null) {
ops.add(new RebaseUnbornRootOp(n));
ops.add(new FastForwardOp(args, n));
} else if (n.getParentCount() == 0) {
ops.add(new RebaseRootOp(n));
} else if (n.getParentCount() == 1) {
@@ -76,22 +76,6 @@ public class RebaseIfNecessary extends SubmitStrategy {
return ops;
}
private class RebaseUnbornRootOp extends SubmitStrategyOp {
private RebaseUnbornRootOp(CodeReviewCommit toMerge) {
super(RebaseIfNecessary.this.args, toMerge);
}
@Override
public void updateRepoImpl(RepoContext ctx) throws IntegrationException {
// The branch is unborn. Take fast-forward resolution to create the
// branch.
toMerge.setStatusCode(CommitMergeStatus.CLEAN_MERGE);
CodeReviewCommit newCommit = amendGitlink(toMerge);
args.mergeTip.moveTipTo(newCommit, toMerge);
acceptMergeTip(args.mergeTip);
}
}
private class RebaseRootOp extends SubmitStrategyOp {
private RebaseRootOp(CodeReviewCommit toMerge) {
super(RebaseIfNecessary.this.args, toMerge);
@@ -120,9 +104,11 @@ public class RebaseIfNecessary extends SubmitStrategy {
// TODO(dborowitz): args.rw is needed because it's a CodeReviewRevWalk.
// When hoisting BatchUpdate into MergeOp, we will need to teach
// BatchUpdate how to produce CodeReviewRevWalks.
if (args.mergeUtil.canFastForward(args.mergeSorter,
args.mergeTip.getCurrentTip(), args.rw, toMerge)) {
if (args.mergeUtil
.canFastForward(args.mergeSorter, args.mergeTip.getCurrentTip(),
args.rw, toMerge)) {
args.mergeTip.moveTipTo(amendGitlink(toMerge), toMerge);
toMerge.setStatusCode(CommitMergeStatus.CLEAN_MERGE);
acceptMergeTip(args.mergeTip);
return;
}
@@ -193,9 +179,9 @@ public class RebaseIfNecessary extends SubmitStrategy {
// first parent. So instead behave as though MERGE_IF_NECESSARY was
// configured.
MergeTip mergeTip = args.mergeTip;
if (args.rw.isMergedInto(mergeTip.getCurrentTip(), toMerge)) {
mergeTip.moveTipTo(amendGitlink(toMerge), toMerge);
acceptMergeTip(mergeTip);
if (args.rw.isMergedInto(mergeTip.getCurrentTip(), toMerge) &&
!args.submoduleOp.hasSubscription(args.destBranch)) {
mergeTip.moveTipTo(toMerge, toMerge);
} else {
CodeReviewCommit newTip = args.mergeUtil.mergeOneCommit(
args.serverIdent, args.serverIdent, args.repo, args.rw,

28
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
View File

@@ -564,26 +564,18 @@ abstract class SubmitStrategyOp extends BatchUpdate.Op {
*/
protected CodeReviewCommit amendGitlink(CodeReviewCommit commit)
throws IntegrationException {
CodeReviewCommit newCommit = commit;
// Modify the commit with gitlink update
if (args.submoduleOp.hasSubscription(args.destBranch)) {
try {
newCommit =
args.submoduleOp.composeGitlinksCommit(args.destBranch, commit);
newCommit.copyFrom(commit);
if (commit.equals(toMerge)) {
newCommit.setPatchsetId(ChangeUtil.nextPatchSetId(
args.repo, toMerge.change().currentPatchSetId()));
args.commits.put(newCommit);
}
} catch (SubmoduleException | IOException e) {
throw new IntegrationException(
"cannot update gitlink for the commit at branch: "
+ args.destBranch);
}
if (!args.submoduleOp.hasSubscription(args.destBranch)) {
return commit;
}
return newCommit;
// Modify the commit with gitlink update
try {
return args.submoduleOp.composeGitlinksCommit(args.destBranch, commit);
} catch (SubmoduleException | IOException e) {
throw new IntegrationException(
"cannot update gitlink for the commit at branch: "
+ args.destBranch);
}
}
protected final void logDebug(String msg, Object... args) {