opendev/gerrit
Code Issues Proposed changes

SubmitStrategy: Fix ordering of implicit and explicit operations

Browse Source 
When multiple changes are merged at the same time by a single submit
operation, including one or more changes that are merged implicitly,
the change-merged events are emitted in the wrong order for the
strategies:

- Fast-Forward Only
- Always Merge
- Merge If Necessary

The events are emitted first for the explicitly merged change, and
then for the implicitly merged changes. This order does not correspond
with the order in which the commits appear in the repository.

Fix the creation of the list of operations to first add the implicit
operations, and then the explicit ones.

Update the tests to assert for the correct order. Also extend the
tests to include multiple implicitly merged changes.

Bug: Issue 4199
Change-Id: I07b6520bd71dcff9cdcab417482c3770c6d9108d
This commit is contained in:
David Pursehouse 2016-06-16 11:44:21 +09:00
parent 450332bd94
commit 4ba8b9035b
5 changed files with 64 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

23
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java
View File

@ -62,31 +62,36 @@ public class SubmitByFastForwardIT extends AbstractSubmit {
}
@Test
public void submitTwoChangesWithFastForward() throws Exception {
public void submitMultipleChangesWithFastForward() throws Exception {
RevCommit initialHead = getRemoteHead();
PushOneCommit.Result change = createChange();
PushOneCommit.Result change2 = createChange();
PushOneCommit.Result change3 = createChange();
String id1 = change.getChangeId();
String id2 = change2.getChangeId();
String id3 = change3.getChangeId();
approve(id1);
submit(id2);
approve(id2);
submit(id3);
RevCommit updatedHead = getRemoteHead();
assertThat(updatedHead.getId()).isEqualTo(change2.getCommit());
assertThat(updatedHead.getParent(0).getId()).isEqualTo(change.getCommit());
assertThat(updatedHead.getId()).isEqualTo(change3.getCommit());
assertThat(updatedHead.getParent(0).getId()).isEqualTo(change2.getCommit());
assertSubmitter(change.getChangeId(), 1);
assertSubmitter(change2.getChangeId(), 1);
assertSubmitter(change3.getChangeId(), 1);
assertPersonEquals(admin.getIdent(), updatedHead.getAuthorIdent());
assertPersonEquals(admin.getIdent(), updatedHead.getCommitterIdent());
assertSubmittedTogether(id1, id2, id1);
assertSubmittedTogether(id2, id2, id1);
assertSubmittedTogether(id1, id3, id2, id1);
assertSubmittedTogether(id2, id3, id2, id1);
assertSubmittedTogether(id3, id3, id2, id1);
assertRefUpdatedEvents(initialHead, updatedHead);
//TODO(dpursehouse) why are change-merged events in reverse order?
assertChangeMergedEvents(change2.getChangeId(), updatedHead.name(),
change.getChangeId(), updatedHead.name());
assertChangeMergedEvents(id1, updatedHead.name(),
id2, updatedHead.name(),
id3, updatedHead.name());
}
@Test

17
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeAlwaysIT.java
View File

@ -64,20 +64,23 @@ public class SubmitByMergeAlwaysIT extends AbstractSubmitByMerge {
assertThat(headAfterFirstSubmit.getParent(0).getId()).isEqualTo(
initialHead.getId());
// Submit two changes at the same time
// Submit three changes at the same time
PushOneCommit.Result change2 = createChange("Change 2", "c", "c");
PushOneCommit.Result change3 = createChange("Change 3", "d", "d");
PushOneCommit.Result change4 = createChange("Change 4", "e", "e");
approve(change2.getChangeId());
submit(change3.getChangeId());
approve(change3.getChangeId());
submit(change4.getChangeId());
// Submitting change 3 should result in change 2 also being submitted
// Submitting change 4 should result in changes 2 and 3 also being submitted
assertMerged(change2.getChangeId());
assertMerged(change3.getChangeId());
// The remote head should now be a merge of the new head after
// the previous submit, and "Change 3".
// the previous submit, and "Change 4".
RevCommit headAfterSecondSubmit = getRemoteLog().get(0);
assertThat(headAfterSecondSubmit.getParent(1).getShortMessage()).isEqualTo(
change3.getCommit().getShortMessage());
change4.getCommit().getShortMessage());
assertThat(headAfterSecondSubmit.getParent(0).getShortMessage()).isEqualTo(
headAfterFirstSubmit.getShortMessage());
assertThat(headAfterSecondSubmit.getParent(0).getId()).isEqualTo(
@ -88,9 +91,9 @@ public class SubmitByMergeAlwaysIT extends AbstractSubmitByMerge {
assertRefUpdatedEvents(initialHead, headAfterFirstSubmit,
headAfterFirstSubmit, headAfterSecondSubmit);
//TODO(dpursehouse) why are change-merged events in reverse order?
assertChangeMergedEvents(change.getChangeId(), headAfterFirstSubmit.name(),
change2.getChangeId(), headAfterSecondSubmit.name(),
change3.getChangeId(), headAfterSecondSubmit.name(),
change2.getChangeId(), headAfterSecondSubmit.name());
change4.getChangeId(), headAfterSecondSubmit.name());
}
}

14
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java
View File

@ -56,7 +56,8 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge {
testRepo.reset(initialHead);
PushOneCommit.Result change3 = createChange("Change 3", "d", "d");
PushOneCommit.Result change4 = createChange("Change 4", "d", "d");
PushOneCommit.Result change4 = createChange("Change 4", "e", "e");
PushOneCommit.Result change5 = createChange("Change 5", "f", "f");
// Change 2 is a fast-forward, no need to merge.
submit(change2.getChangeId());
@ -69,13 +70,14 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge {
assertPersonEquals(admin.getIdent(), headAfterFirstSubmit.getAuthorIdent());
assertPersonEquals(admin.getIdent(), headAfterFirstSubmit.getCommitterIdent());
// We need to merge changes 3 and 4.
// We need to merge changes 3, 4 and 5.
approve(change3.getChangeId());
submit(change4.getChangeId());
approve(change4.getChangeId());
submit(change5.getChangeId());
RevCommit headAfterSecondSubmit = getRemoteLog().get(0);
assertThat(headAfterSecondSubmit.getParent(1).getShortMessage()).isEqualTo(
change4.getCommit().getShortMessage());
change5.getCommit().getShortMessage());
assertThat(headAfterSecondSubmit.getParent(0).getShortMessage()).isEqualTo(
change2.getCommit().getShortMessage());
@ -89,10 +91,10 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge {
// and three change-merged events.
assertRefUpdatedEvents(initialHead, headAfterFirstSubmit,
headAfterFirstSubmit, headAfterSecondSubmit);
//TODO(dpursehouse) why are change-merged events in reverse order?
assertChangeMergedEvents(change2.getChangeId(), headAfterFirstSubmit.name(),
change3.getChangeId(), headAfterSecondSubmit.name(),
change4.getChangeId(), headAfterSecondSubmit.name(),
change3.getChangeId(), headAfterSecondSubmit.name());
change5.getChangeId(), headAfterSecondSubmit.name());
}
@Test

29
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java
View File

@ -113,32 +113,41 @@ public class SubmitByRebaseIfNecessaryIT extends AbstractSubmit {
.isNotEqualTo(change1.getCommit());
PushOneCommit.Result change3 =
createChange("Change 3", "c.txt", "third content");
PushOneCommit.Result change4 =
createChange("Change 4", "d.txt", "fourth content");
approve(change2.getChangeId());
submit(change3.getChangeId());
approve(change3.getChangeId());
submit(change4.getChangeId());
assertRebase(testRepo, false);
assertApproved(change2.getChangeId());
assertApproved(change3.getChangeId());
assertApproved(change4.getChangeId());
RevCommit headAfterSecondSubmit = parse(getRemoteHead());
assertThat(headAfterSecondSubmit.getShortMessage()).isEqualTo("Change 3");
assertThat(headAfterSecondSubmit).isNotEqualTo(change3.getCommit());
assertCurrentRevision(change3.getChangeId(), 2, headAfterSecondSubmit);
assertThat(headAfterSecondSubmit.getShortMessage()).isEqualTo("Change 4");
assertThat(headAfterSecondSubmit).isNotEqualTo(change4.getCommit());
assertCurrentRevision(change4.getChangeId(), 2, headAfterSecondSubmit);
RevCommit parent = parse(headAfterSecondSubmit.getParent(0));
assertThat(parent.getShortMessage()).isEqualTo("Change 2");
assertThat(parent).isNotEqualTo(change2.getCommit());
assertCurrentRevision(change2.getChangeId(), 2, parent);
assertThat(parent.getShortMessage()).isEqualTo("Change 3");
assertThat(parent).isNotEqualTo(change3.getCommit());
assertCurrentRevision(change3.getChangeId(), 2, parent);
RevCommit grandparent = parse(parent.getParent(0));
assertThat(grandparent).isEqualTo(change1.getCommit());
assertCurrentRevision(change1.getChangeId(), 1, grandparent);
assertThat(grandparent).isNotEqualTo(change2.getCommit());
assertCurrentRevision(change2.getChangeId(), 2, grandparent);
RevCommit greatgrandparent = parse(grandparent.getParent(0));
assertThat(greatgrandparent).isEqualTo(change1.getCommit());
assertCurrentRevision(change1.getChangeId(), 1, greatgrandparent);
assertRefUpdatedEvents(initialHead, headAfterFirstSubmit,
headAfterFirstSubmit, headAfterSecondSubmit);
assertChangeMergedEvents(change1.getChangeId(), headAfterFirstSubmit.name(),
change2.getChangeId(), headAfterSecondSubmit.name(),
change3.getChangeId(), headAfterSecondSubmit.name());
change3.getChangeId(), headAfterSecondSubmit.name(),
change4.getChangeId(), headAfterSecondSubmit.name());
}
@Test

16
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java
View File

@ -56,7 +56,9 @@ import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevFlag;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Set;
@ -220,15 +222,23 @@ public abstract class SubmitStrategy {
throws IntegrationException {
List<SubmitStrategyOp> ops = buildOps(toMerge);
Set<CodeReviewCommit> added = Sets.newHashSetWithExpectedSize(ops.size());
for (SubmitStrategyOp op : ops) {
bu.addOp(op.getId(), op);
added.add(op.getCommit());
}
// Fill in ops for any implicitly merged changes.
for (CodeReviewCommit c : Sets.difference(toMerge, added)) {
// First add ops for any implicitly merged changes.
List<CodeReviewCommit> difference =
new ArrayList<>(Sets.difference(toMerge, added));
Collections.reverse(difference);
for (CodeReviewCommit c : difference) {
bu.addOp(c.change().getId(), new ImplicitIntegrateOp(args, c));
}
// Then ops for explicitly merged changes
for (SubmitStrategyOp op : ops) {
bu.addOp(op.getId(), op);
}
}
protected abstract List<SubmitStrategyOp> buildOps(