From 3facbc60330a3ca88a73961a3b9ac7f2b03082f1 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Thu, 22 Dec 2016 11:03:15 +0100 Subject: [PATCH] Fix submitting changes with commits that were already merged Due to bugs it can happen that a change gets into an inconsistent state where the commit of the last patch set is already merged into the destination branch, but the change status is still NEW. In this case the inconsistency should get resolved by submitting the change again. The submit detects that the commit is already merged and then only the change status is updated to MERGED. This already worked for all cases except for Fast Forward Only if several changes got merged, but stayed open. Then submitting the first change again was not possible since the MergeUtil.canFastForward claimed that the change was not mergeable. Change-Id: I73641f02e58298baf6f13da27d37500a44ad6d2b Signed-off-by: Edwin Kempin --- .../rest/change/AbstractSubmit.java | 95 +++++++++++++++++++ .../google/gerrit/server/git/MergeUtil.java | 3 +- 2 files changed, 97 insertions(+), 1 deletion(-) 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); }