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 4e4eeeb521..45da3d8ba7 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 @@ -34,6 +34,7 @@ import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.TestAccount; import com.google.gerrit.acceptance.TestProjectInput; +import com.google.gerrit.common.TimeUtil; import com.google.gerrit.common.data.Permission; import com.google.gerrit.extensions.api.changes.SubmitInput; import com.google.gerrit.extensions.api.projects.BranchInput; @@ -56,13 +57,17 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.ApprovalsUtil; +import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.change.RevisionResource; import com.google.gerrit.server.change.Submit; +import com.google.gerrit.server.git.BatchUpdate; +import com.google.gerrit.server.git.BatchUpdate.ChangeContext; import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.project.Util; import com.google.gerrit.testutil.ConfigSuite; import com.google.gerrit.testutil.TestTimeUtil; +import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import org.eclipse.jgit.diff.DiffFormatter; @@ -99,6 +104,12 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { @Inject private Submit submitHandler; + @Inject + private IdentifiedUser.GenericFactory userFactory; + + @Inject + private BatchUpdate.Factory updateFactory; + private String systemTimeZone; @Before @@ -571,6 +582,90 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { assertThat(log).contains(mergeReview.getCommit()); } + @Test + public void submitChangeWithCommitThatWasAlreadyMerged() throws Exception { + // create and submit a change + PushOneCommit.Result change = createChange(); + submit(change.getChangeId()); + RevCommit headAfterSubmit = getRemoteHead(); + + // set the status of the change back to NEW to simulate a failed submit that + // merged the commit but failed to update the change status + setChangeStatusToNew(change); + + // submitting the change again should detect that the commit was already + // merged and just fix the change status to be MERGED + submit(change.getChangeId()); + assertThat(getRemoteHead()).isEqualTo(headAfterSubmit); + } + + @Test + public void submitChangesWithCommitsThatWereAlreadyMerged() throws Exception { + // create and submit 2 changes + PushOneCommit.Result change1 = createChange(); + PushOneCommit.Result change2 = createChange(); + approve(change1.getChangeId()); + if (getSubmitType() == SubmitType.CHERRY_PICK) { + submit(change1.getChangeId()); + } + submit(change2.getChangeId()); + assertMerged(change1.getChangeId()); + RevCommit headAfterSubmit = getRemoteHead(); + + // set the status of the changes back to NEW to simulate a failed submit that + // merged the commits but failed to update the change status + setChangeStatusToNew(change1, change2); + + // submitting the changes again should detect that the commits were already + // merged and just fix the change status to be MERGED + submit(change1.getChangeId()); + submit(change2.getChangeId()); + assertThat(getRemoteHead()).isEqualTo(headAfterSubmit); + } + + @Test + public void submitTopicWithCommitsThatWereAlreadyMerged() throws Exception { + assume().that(isSubmitWholeTopicEnabled()).isTrue(); + + // create and submit 2 changes with the same topic + String topic = name("topic"); + PushOneCommit.Result change1 = createChange("refs/for/master/" + topic); + PushOneCommit.Result change2 = createChange("refs/for/master/" + topic); + approve(change1.getChangeId()); + submit(change2.getChangeId()); + assertMerged(change1.getChangeId()); + RevCommit headAfterSubmit = getRemoteHead(); + + // set the status of the second change back to NEW to simulate a failed + // submit that merged the commits but failed to update the change status of + // some changes in the topic + setChangeStatusToNew(change2); + + // submitting the topic again should detect that the commits were already + // merged and just fix the change status to be MERGED + submit(change2.getChangeId()); + assertThat(getRemoteHead()).isEqualTo(headAfterSubmit); + } + + private void setChangeStatusToNew(PushOneCommit.Result... changes) + throws Exception { + for (PushOneCommit.Result change : changes) { + try (BatchUpdate bu = updateFactory.create(db, project, + userFactory.create(admin.id), TimeUtil.nowTs())) { + bu.addOp(change.getChange().getId(), new BatchUpdate.Op() { + @Override + public boolean updateChange(ChangeContext ctx) throws OrmException { + ctx.getChange().setStatus(Change.Status.NEW); + ctx.getUpdate(ctx.getChange().currentPatchSetId()) + .setStatus(Change.Status.NEW); + return true; + } + }); + bu.execute(); + } + } + } + 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 fa447656d6..4df342a5e0 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 @@ -496,7 +496,8 @@ public class MergeUtil { } try { - return mergeTip == null || rw.isMergedInto(mergeTip, toMerge); + return mergeTip == null || rw.isMergedInto(mergeTip, toMerge) + || rw.isMergedInto(toMerge, mergeTip); } catch (IOException e) { throw new IntegrationException("Cannot fast-forward test during merge", e); }