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 d817a080ff..3c318caa6b 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 @@ -219,22 +219,22 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { } protected void submit(String changeId) throws Exception { - submit(changeId, new SubmitInput(), null, null); + submit(changeId, new SubmitInput(), null, null, true); } protected void submit(String changeId, SubmitInput input) throws Exception { - submit(changeId, input, null, null); + submit(changeId, input, null, null, true); } protected void submitWithConflict(String changeId, String expectedError) throws Exception { submit(changeId, new SubmitInput(), ResourceConflictException.class, - expectedError); + expectedError, true); } - private void submit(String changeId, SubmitInput input, + protected void submit(String changeId, SubmitInput input, Class expectedExceptionType, - String expectedExceptionMsg) throws Exception { + String expectedExceptionMsg, boolean checkMergeResult) throws Exception { approve(changeId); try { gApi.changes().id(changeId).current().submit(input); @@ -258,7 +258,9 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { } ChangeInfo change = gApi.changes().id(changeId).info(); assertThat(change.status).isEqualTo(ChangeStatus.MERGED); - checkMergeResult(change); + if (checkMergeResult) { + checkMergeResult(change); + } } private void checkMergeResult(ChangeInfo change) throws Exception { diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmitByMerge.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmitByMerge.java index c390c67f4c..8ab68d5a7e 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmitByMerge.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmitByMerge.java @@ -17,11 +17,23 @@ package com.google.gerrit.acceptance.rest.change; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.TruthJUnit.assume; +import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.TestProjectInput; +import com.google.gerrit.extensions.api.changes.SubmitInput; +import com.google.gerrit.extensions.client.ChangeStatus; import com.google.gerrit.extensions.client.InheritableBoolean; +import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.extensions.common.ChangeMessageInfo; +import com.google.gerrit.extensions.restapi.ResourceConflictException; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.server.change.Submit.TestSubmitInput; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevWalk; import org.junit.Test; public abstract class AbstractSubmitByMerge extends AbstractSubmit { @@ -118,4 +130,58 @@ public abstract class AbstractSubmitByMerge extends AbstractSubmit { assertThat(head.getParent(0)).isEqualTo(change1.getCommit()); assertThat(head.getParent(1)).isEqualTo(change2.getCommit()); } + + @Test + public void repairChangeStateAfterFailure() throws Exception { + RevCommit initialHead = getRemoteHead(); + PushOneCommit.Result change = + createChange("Change 1", "a.txt", "content"); + submit(change.getChangeId()); + RevCommit afterChange1Head = getRemoteHead(); + + testRepo.reset(initialHead); + PushOneCommit.Result change2 = + createChange("Change 2", "b.txt", "other content"); + Change.Id id2 = change2.getChange().getId(); + SubmitInput failAfterRefUpdates = + new TestSubmitInput(new SubmitInput(), true); + submit(change2.getChangeId(), failAfterRefUpdates, + ResourceConflictException.class, "Failing after ref updates", true); + + // Bad: ref advanced but change wasn't updated. + PatchSet.Id psId1 = new PatchSet.Id(id2, 1); + ChangeInfo info = gApi.changes().id(id2.get()).get(); + assertThat(info.status).isEqualTo(ChangeStatus.NEW); + assertThat(info.revisions.get(info.currentRevision)._number).isEqualTo(1); + ChangeMessageInfo lastMessage = Iterables.getLast(info.messages); + + RevCommit tip; + try (Repository repo = repoManager.openRepository(project); + RevWalk rw = new RevWalk(repo)) { + ObjectId rev1 = repo.exactRef(psId1.toRefName()).getObjectId(); + assertThat(rev1).isNotNull(); + + tip = rw.parseCommit(repo.exactRef("refs/heads/master").getObjectId()); + assertThat(tip.getParentCount()).isEqualTo(2); + assertThat(tip.getParent(0)).isEqualTo(afterChange1Head); + assertThat(tip.getParent(1)).isEqualTo(change2.getCommit()); + } + + // Skip checking the merge result; in the fixup case, the newRev in + // ChangeMergedEvent won't match the current branch tip. + submit(change2.getChangeId(), new SubmitInput(), null, null, false); + + // Change status and patch set entities were updated, and branch tip stayed + // the same. + info = gApi.changes().id(id2.get()).get(); + assertThat(info.status).isEqualTo(ChangeStatus.MERGED); + assertThat(info.revisions.get(info.currentRevision)._number).isEqualTo(1); + assertThat(Iterables.getLast(info.messages).message) + .isEqualTo(lastMessage.message); + + try (Repository repo = repoManager.openRepository(project)) { + assertThat(repo.exactRef("refs/heads/master").getObjectId()) + .isEqualTo(tip); + } + } } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java index 65d2a239f6..2874215854 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java @@ -16,15 +16,25 @@ package com.google.gerrit.acceptance.rest.change; import static com.google.common.truth.Truth.assertThat; +import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.TestProjectInput; +import com.google.gerrit.extensions.api.changes.SubmitInput; import com.google.gerrit.extensions.client.ChangeStatus; import com.google.gerrit.extensions.client.InheritableBoolean; import com.google.gerrit.extensions.client.ListChangesOption; import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.extensions.common.ChangeMessageInfo; +import com.google.gerrit.extensions.restapi.ResourceConflictException; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.server.change.Submit.TestSubmitInput; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevWalk; import org.junit.Test; import java.util.List; @@ -257,4 +267,64 @@ public class SubmitByCherryPickIT extends AbstractSubmit { assertNew(change2.getChangeId()); assertNew(change3.getChangeId()); } + + @Test + public void repairChangeStateAfterFailure() throws Exception { + RevCommit initialHead = getRemoteHead(); + PushOneCommit.Result change = + createChange("Change 1", "a.txt", "content"); + submit(change.getChangeId()); + + RevCommit oldHead = getRemoteHead(); + testRepo.reset(initialHead); + PushOneCommit.Result change2 = + createChange("Change 2", "b.txt", "other content"); + Change.Id id2 = change2.getChange().getId(); + SubmitInput failAfterRefUpdates = + new TestSubmitInput(new SubmitInput(), true); + submit(change2.getChangeId(), failAfterRefUpdates, + ResourceConflictException.class, "Failing after ref updates", true); + + // Bad: ref advanced but change wasn't updated. + PatchSet.Id psId1 = new PatchSet.Id(id2, 1); + PatchSet.Id psId2 = new PatchSet.Id(id2, 2); + ChangeInfo info = gApi.changes().id(id2.get()).get(); + assertThat(info.status).isEqualTo(ChangeStatus.NEW); + assertThat(info.revisions.get(info.currentRevision)._number).isEqualTo(1); + assertThat(getPatchSet(psId2)).isNull(); + ChangeMessageInfo lastMessage = Iterables.getLast(info.messages); + + ObjectId rev2; + try (Repository repo = repoManager.openRepository(project); + RevWalk rw = new RevWalk(repo)) { + ObjectId rev1 = repo.exactRef(psId1.toRefName()).getObjectId(); + assertThat(rev1).isNotNull(); + + rev2 = repo.exactRef(psId2.toRefName()).getObjectId(); + assertThat(rev2).isNotNull(); + assertThat(rev2).isNotEqualTo(rev1); + assertThat(rw.parseCommit(rev2).getParent(0)).isEqualTo(oldHead); + + assertThat(repo.exactRef("refs/heads/master").getObjectId()) + .isEqualTo(rev2); + } + + submit(change2.getChangeId()); + + // Change status and patch set entities were updated, and branch tip stayed + // the same. + info = gApi.changes().id(id2.get()).get(); + assertThat(info.status).isEqualTo(ChangeStatus.MERGED); + assertThat(info.revisions.get(info.currentRevision)._number).isEqualTo(2); + PatchSet ps2 = getPatchSet(psId2); + assertThat(ps2).isNotNull(); + assertThat(ps2.getRevision().get()).isEqualTo(rev2.name()); + assertThat(Iterables.getLast(info.messages).message) + .isEqualTo(lastMessage.message); + + try (Repository repo = repoManager.openRepository(project)) { + assertThat(repo.exactRef("refs/heads/master").getObjectId()) + .isEqualTo(rev2); + } + } } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java index ead5419f43..c20158319c 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java @@ -16,12 +16,23 @@ package com.google.gerrit.acceptance.rest.change; import static com.google.common.truth.Truth.assertThat; +import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.PushOneCommit; +import com.google.gerrit.extensions.api.changes.SubmitInput; +import com.google.gerrit.extensions.client.ChangeStatus; import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.extensions.common.ActionInfo; +import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.extensions.common.ChangeMessageInfo; +import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.server.change.Submit.TestSubmitInput; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevWalk; import org.junit.Test; import java.util.Map; @@ -107,4 +118,44 @@ public class SubmitByFastForwardIT extends AbstractSubmit { assertThat(getRemoteHead()).isEqualTo(oldHead); assertSubmitter(change.getChangeId(), 1); } + + @Test + public void repairChangeStateAfterFailure() throws Exception { + PushOneCommit.Result change = createChange("Change 1", "a.txt", "content"); + Change.Id id = change.getChange().getId(); + SubmitInput failAfterRefUpdates = + new TestSubmitInput(new SubmitInput(), true); + submit(change.getChangeId(), failAfterRefUpdates, + ResourceConflictException.class, "Failing after ref updates", true); + + // Bad: ref advanced but change wasn't updated. + PatchSet.Id psId = new PatchSet.Id(id, 1); + ChangeInfo info = gApi.changes().id(id.get()).get(); + assertThat(info.status).isEqualTo(ChangeStatus.NEW); + assertThat(info.revisions.get(info.currentRevision)._number).isEqualTo(1); + ChangeMessageInfo lastMessage = Iterables.getLast(info.messages); + + ObjectId rev; + try (Repository repo = repoManager.openRepository(project); + RevWalk rw = new RevWalk(repo)) { + rev = repo.exactRef(psId.toRefName()).getObjectId(); + assertThat(rev).isNotNull(); + assertThat(repo.exactRef("refs/heads/master").getObjectId()) + .isEqualTo(rev); + } + + submit(change.getChangeId()); + + // Change status was updated, and branch tip stayed the same. + info = gApi.changes().id(id.get()).get(); + assertThat(info.status).isEqualTo(ChangeStatus.MERGED); + assertThat(info.revisions.get(info.currentRevision)._number).isEqualTo(1); + assertThat(Iterables.getLast(info.messages).message) + .isEqualTo(lastMessage.message); + + try (Repository repo = repoManager.openRepository(project)) { + assertThat(repo.exactRef("refs/heads/master").getObjectId()) + .isEqualTo(rev); + } + } } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java index e9d27345ba..bd4aa49623 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java @@ -16,10 +16,19 @@ package com.google.gerrit.acceptance.rest.change; import static com.google.common.truth.Truth.assertThat; +import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.TestProjectInput; +import com.google.gerrit.extensions.api.changes.SubmitInput; +import com.google.gerrit.extensions.client.ChangeStatus; import com.google.gerrit.extensions.client.InheritableBoolean; import com.google.gerrit.extensions.client.SubmitType; +import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.extensions.common.ChangeMessageInfo; +import com.google.gerrit.extensions.restapi.ResourceConflictException; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.server.change.Submit.TestSubmitInput; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Repository; @@ -154,6 +163,66 @@ public class SubmitByRebaseIfNecessaryIT extends AbstractSubmit { assertNoSubmitter(change2.getChangeId(), 1); } + @Test + public void repairChangeStateAfterFailure() throws Exception { + RevCommit initialHead = getRemoteHead(); + PushOneCommit.Result change = + createChange("Change 1", "a.txt", "content"); + submit(change.getChangeId()); + + RevCommit oldHead = getRemoteHead(); + testRepo.reset(initialHead); + PushOneCommit.Result change2 = + createChange("Change 2", "b.txt", "other content"); + Change.Id id2 = change2.getChange().getId(); + SubmitInput failAfterRefUpdates = + new TestSubmitInput(new SubmitInput(), true); + submit(change2.getChangeId(), failAfterRefUpdates, + ResourceConflictException.class, "Failing after ref updates", true); + + // Bad: ref advanced but change wasn't updated. + PatchSet.Id psId1 = new PatchSet.Id(id2, 1); + PatchSet.Id psId2 = new PatchSet.Id(id2, 2); + ChangeInfo info = gApi.changes().id(id2.get()).get(); + assertThat(info.status).isEqualTo(ChangeStatus.NEW); + assertThat(info.revisions.get(info.currentRevision)._number).isEqualTo(1); + assertThat(getPatchSet(psId2)).isNull(); + ChangeMessageInfo lastMessage = Iterables.getLast(info.messages); + + ObjectId rev2; + try (Repository repo = repoManager.openRepository(project); + RevWalk rw = new RevWalk(repo)) { + ObjectId rev1 = repo.exactRef(psId1.toRefName()).getObjectId(); + assertThat(rev1).isNotNull(); + + rev2 = repo.exactRef(psId2.toRefName()).getObjectId(); + assertThat(rev2).isNotNull(); + assertThat(rev2).isNotEqualTo(rev1); + assertThat(rw.parseCommit(rev2).getParent(0)).isEqualTo(oldHead); + + assertThat(repo.exactRef("refs/heads/master").getObjectId()) + .isEqualTo(rev2); + } + + submit(change2.getChangeId()); + + // Change status and patch set entities were updated, and branch tip stayed + // the same. + info = gApi.changes().id(id2.get()).get(); + assertThat(info.status).isEqualTo(ChangeStatus.MERGED); + assertThat(info.revisions.get(info.currentRevision)._number).isEqualTo(2); + PatchSet ps2 = getPatchSet(psId2); + assertThat(ps2).isNotNull(); + assertThat(ps2.getRevision().get()).isEqualTo(rev2.name()); + assertThat(Iterables.getLast(info.messages).message) + .isEqualTo(lastMessage.message); + + try (Repository repo = repoManager.openRepository(project)) { + assertThat(repo.exactRef("refs/heads/master").getObjectId()) + .isEqualTo(rev2); + } + } + private RevCommit parse(ObjectId id) throws Exception { try (Repository repo = repoManager.openRepository(project); RevWalk rw = new RevWalk(repo)) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java index 0c66076613..8ba80bd719 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.change; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; import com.google.common.base.Predicate; import com.google.common.base.Strings; @@ -94,6 +95,22 @@ public class Submit implements RestModifyView, } } + /** + * Subclass of {@link SubmitInput} with special bits that may be flipped for + * testing purposes only. + */ + @VisibleForTesting + public static class TestSubmitInput extends SubmitInput { + public final boolean failAfterRefUpdates; + + @SuppressWarnings("deprecation") + public TestSubmitInput(SubmitInput base, boolean failAfterRefUpdates) { + this.onBehalfOf = base.onBehalfOf; + this.waitForMerge = base.waitForMerge; + this.failAfterRefUpdates = failAfterRefUpdates; + } + } + private final Provider dbProvider; private final GitRepositoryManager repoManager; private final ChangeData.Factory changeDataFactory; @@ -185,7 +202,7 @@ public class Submit implements RestModifyView, try (MergeOp op = mergeOpProvider.get()) { ReviewDb db = dbProvider.get(); - op.merge(db, change, caller, true); + op.merge(db, change, caller, true, input); change = db.changes().get(change.getId()); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index 690b6ef40b..f81c128bf1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java @@ -38,6 +38,7 @@ import com.google.common.hash.Hashing; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.common.data.SubmitTypeRecord; +import com.google.gerrit.extensions.api.changes.SubmitInput; import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestApiException; @@ -335,6 +336,7 @@ public class MergeOp implements AutoCloseable { private CommitStatus commits; private ReviewDb db; + private SubmitInput submitInput; @Inject MergeOp(ChangeControl.GenericFactory changeControlFactory, @@ -544,7 +546,9 @@ public class MergeOp implements AutoCloseable { } public void merge(ReviewDb db, Change change, IdentifiedUser caller, - boolean checkSubmitRules) throws OrmException, RestApiException { + boolean checkSubmitRules, SubmitInput submitInput) + throws OrmException, RestApiException { + this.submitInput = submitInput; this.caller = caller; updateSubmissionId(change); this.db = db; @@ -627,7 +631,7 @@ public class MergeOp implements AutoCloseable { BatchUpdate.execute( batchUpdates(projects), - new SubmitStrategyListener(strategies, commits)); + new SubmitStrategyListener(submitInput, strategies, commits)); SubmoduleOp subOp = subOpProvider.get(); for (Branch.NameKey branch : branches) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java index 0c9ddee427..321b8fc649 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java @@ -14,6 +14,8 @@ package com.google.gerrit.server.git; +import static com.google.common.base.Preconditions.checkState; + import com.google.common.base.Strings; import com.google.common.collect.Multimap; import com.google.gerrit.common.data.SubmitTypeRecord; @@ -84,11 +86,15 @@ public class MergeSuperSet { throws MissingObjectException, IncorrectObjectTypeException, IOException, OrmException { ChangeData cd = changeDataFactory.create(db, change.getId()); + ChangeSet result; if (Submit.wholeTopicEnabled(cfg)) { - return completeChangeSetIncludingTopics(db, new ChangeSet(cd)); + result = completeChangeSetIncludingTopics(db, new ChangeSet(cd)); } else { - return completeChangeSetWithoutTopic(db, new ChangeSet(cd)); + result = completeChangeSetWithoutTopic(db, new ChangeSet(cd)); } + checkState(result.ids().contains(change.getId()), + "change %s missing from result %s", change.getId(), result); + return result; } private ChangeSet completeChangeSetWithoutTopic(ReviewDb db, ChangeSet changes) @@ -132,8 +138,13 @@ public class MergeSuperSet { } List hashes = new ArrayList<>(); + // Always include the input, even if merged. This allows + // SubmitStrategyOp to correct the situation later. + hashes.add(objIdStr); for (RevCommit c : rw) { - hashes.add(c.name()); + if (!c.equals(commit)) { + hashes.add(c.name()); + } } if (!hashes.isEmpty()) { 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 331c6b187c..df307e7c7c 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 @@ -672,13 +672,15 @@ public class MergeUtil { public Set findUnmergedChanges(Set expected, CodeReviewRevWalk rw, RevFlag canMergeFlag, CodeReviewCommit oldTip, - CodeReviewCommit mergeTip) throws IntegrationException { + CodeReviewCommit mergeTip, Iterable alreadyMerged) + throws IntegrationException { if (mergeTip == null) { return expected; } try { Set found = Sets.newHashSetWithExpectedSize(expected.size()); + Iterables.addAll(found, alreadyMerged); rw.resetRetain(canMergeFlag); rw.sort(RevSort.TOPO); rw.markStart(mergeTip); @@ -705,4 +707,16 @@ public class MergeUtil { throw new IntegrationException("Cannot check if changes were merged", e); } } + + public static CodeReviewCommit findAnyMergedInto(CodeReviewRevWalk rw, + Iterable commits, CodeReviewCommit tip) throws IOException { + for (CodeReviewCommit c : commits) { + // TODO(dborowitz): Seems like this could get expensive for many patch + // sets. Is there a more efficient implementation? + if (rw.isMergedInto(c, tip)) { + return c; + } + } + return null; + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index 358ab74e5f..f58313852e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -1840,7 +1840,7 @@ public class ReceiveCommits { RevisionResource rsrc = new RevisionResource(changes.parse(changeCtl), ps); try (MergeOp op = mergeOpProvider.get()) { op.merge(db, rsrc.getChange(), - changeCtl.getUser().asIdentifiedUser(), false); + changeCtl.getUser().asIdentifiedUser(), false, null); } addMessage(""); Change c = db.changes().get(rsrc.getChange().getId()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyListener.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyListener.java index 0023924eec..1b3d4d8d19 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyListener.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyListener.java @@ -15,38 +15,55 @@ package com.google.gerrit.server.git.strategy; import com.google.common.base.CharMatcher; +import com.google.gerrit.extensions.api.changes.SubmitInput; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.server.change.Submit.TestSubmitInput; import com.google.gerrit.server.git.BatchUpdate; import com.google.gerrit.server.git.CodeReviewCommit; import com.google.gerrit.server.git.IntegrationException; import com.google.gerrit.server.git.MergeOp.CommitStatus; +import java.util.ArrayList; import java.util.Collection; +import java.util.List; import java.util.Set; public class SubmitStrategyListener extends BatchUpdate.Listener { private final Collection strategies; private final CommitStatus commits; + private final boolean failAfterRefUpdates; - public SubmitStrategyListener(Collection strategies, - CommitStatus commits) { + public SubmitStrategyListener(SubmitInput input, + Collection strategies, CommitStatus commits) { this.strategies = strategies; this.commits = commits; + if (input instanceof TestSubmitInput) { + failAfterRefUpdates = ((TestSubmitInput) input).failAfterRefUpdates; + } else { + failAfterRefUpdates = false; + } } @Override public void afterUpdateRepos() throws ResourceConflictException { try { markCleanMerges(); - checkCommitStatus(); - findUnmergedChanges(); + List alreadyMerged = checkCommitStatus(); + findUnmergedChanges(alreadyMerged); } catch (IntegrationException e) { throw new ResourceConflictException(e.getMessage(), e); } } - private void findUnmergedChanges() + @Override + public void afterRefUpdates() throws ResourceConflictException { + if (failAfterRefUpdates) { + throw new ResourceConflictException("Failing after ref updates"); + } + } + + private void findUnmergedChanges(List alreadyMerged) throws ResourceConflictException, IntegrationException { for (SubmitStrategy strategy : strategies) { if (strategy instanceof CherryPick) { @@ -57,7 +74,7 @@ public class SubmitStrategyListener extends BatchUpdate.Listener { Set unmerged = args.mergeUtil.findUnmergedChanges( args.commits.getChangeIds(args.destBranch), args.rw, args.canMergeFlag, args.mergeTip.getInitialTip(), - args.mergeTip.getCurrentTip()); + args.mergeTip.getCurrentTip(), alreadyMerged); for (Change.Id id : unmerged) { commits.problem(id, "internal error: change not reachable from new branch tip"); @@ -74,22 +91,28 @@ public class SubmitStrategyListener extends BatchUpdate.Listener { } } - private void checkCommitStatus() throws ResourceConflictException { + private List checkCommitStatus() throws ResourceConflictException { + List alreadyMerged = + new ArrayList<>(commits.getChangeIds().size()); for (Change.Id id : commits.getChangeIds()) { CodeReviewCommit commit = commits.get(id); CommitMergeStatus s = commit != null ? commit.getStatusCode() : null; if (s == null) { commits.problem(id, "internal error: change not processed by merge strategy"); - return; + continue; } switch (s) { case CLEAN_MERGE: case CLEAN_REBASE: case CLEAN_PICK: - case ALREADY_MERGED: break; // Merge strategy accepted this change. + case ALREADY_MERGED: + // Already an ancestor of tip. + alreadyMerged.add(commit.getPatchsetId().getParentKey()); + break; + case PATH_CONFLICT: case REBASE_MERGE_CONFLICT: case MANUAL_RECURSIVE_MERGE: @@ -112,6 +135,7 @@ public class SubmitStrategyListener extends BatchUpdate.Listener { } } commits.maybeFailVerbose(); + return alreadyMerged; } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java index 221e40ea6b..e96453d5ca 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java @@ -33,6 +33,7 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.reviewdb.server.ReviewDbUtil; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.git.BatchUpdate; @@ -40,18 +41,28 @@ import com.google.gerrit.server.git.BatchUpdate.ChangeContext; import com.google.gerrit.server.git.BatchUpdate.Context; import com.google.gerrit.server.git.BatchUpdate.RepoContext; import com.google.gerrit.server.git.CodeReviewCommit; +import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk; +import com.google.gerrit.server.git.GroupCollector; import com.google.gerrit.server.git.IntegrationException; import com.google.gerrit.server.git.LabelNormalizer; +import com.google.gerrit.server.git.MergeUtil; import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.project.ProjectState; import com.google.gwtorm.server.OrmException; +import org.eclipse.jgit.errors.IncorrectObjectTypeException; +import org.eclipse.jgit.errors.MissingObjectException; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.transport.ReceiveCommand; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Objects; @@ -68,6 +79,7 @@ abstract class SubmitStrategyOp extends BatchUpdate.Op { private ObjectId mergeResultRev; private PatchSet mergedPatchSet; private Change updatedChange; + private CodeReviewCommit alreadyMerged; protected SubmitStrategyOp(SubmitStrategy.Arguments args, CodeReviewCommit toMerge) { @@ -93,18 +105,27 @@ abstract class SubmitStrategyOp extends BatchUpdate.Op { @Override public final void updateRepo(RepoContext ctx) throws Exception { + logDebug("{}#updateRepo for change {}", getClass().getSimpleName(), + toMerge.change().getId()); // Run the submit strategy implementation and record the merge tip state so // we can create the ref update. CodeReviewCommit tipBefore = args.mergeTip.getCurrentTip(); - updateRepoImpl(ctx); + alreadyMerged = getAlreadyMergedCommit(ctx); + if (alreadyMerged == null) { + updateRepoImpl(ctx); + } else { + logDebug("Already merged as {}", alreadyMerged.name()); + } CodeReviewCommit tipAfter = args.mergeTip.getCurrentTip(); if (Objects.equals(tipBefore, tipAfter)) { + logDebug("Did not move tip", getClass().getSimpleName()); return; } else if (tipAfter == null) { logDebug("No merge tip, no update to perform"); return; } + logDebug("Moved tip from {} to {}", tipBefore, tipAfter); checkProjectConfig(ctx, tipAfter); @@ -133,27 +154,97 @@ abstract class SubmitStrategyOp extends BatchUpdate.Op { } } + private CodeReviewCommit getAlreadyMergedCommit(RepoContext ctx) + throws IOException { + CodeReviewCommit tip = args.mergeTip.getInitialTip(); + if (tip == null) { + return null; + } + CodeReviewRevWalk rw = (CodeReviewRevWalk) ctx.getRevWalk(); + Change.Id id = getId(); + + Collection refs = ctx.getRepository().getRefDatabase() + .getRefs(id.toRefPrefix()).values(); + List commits = new ArrayList<>(refs.size()); + for (Ref ref : refs) { + PatchSet.Id psId = PatchSet.Id.fromRef(ref.getName()); + if (psId == null) { + continue; + } + try { + CodeReviewCommit c = rw.parseCommit(ref.getObjectId()); + c.setPatchsetId(psId); + commits.add(c); + } catch (MissingObjectException | IncorrectObjectTypeException e) { + continue; // Bogus ref, can't be merged into tip so we don't care. + } + } + Collections.sort(commits, ReviewDbUtil.intKeyOrdering().reverse() + .onResultOf( + new Function() { + @Override + public PatchSet.Id apply(CodeReviewCommit in) { + return in.getPatchsetId(); + } + })); + CodeReviewCommit result = MergeUtil.findAnyMergedInto(rw, commits, tip); + if (result == null) { + return null; + } + + // Some patch set of this change is actually merged into the target + // branch, most likely because a previous run of MergeOp failed after + // updateRepo, during updateChange. + // + // Do the best we can to clean this up: mark the change as merged and set + // the current patch set. Don't touch the dest branch at all. This can + // lead to some odd situations like another change in the set merging in + // a different patch set of this change, but that's unavoidable at this + // point. At least the change will end up in the right state. + // + // TODO(dborowitz): Consider deleting later junk patch set refs. They + // presumably don't have PatchSets pointing to them. + rw.parseBody(result); + result.add(args.canMergeFlag); + PatchSet.Id psId = result.getPatchsetId(); + result.copyFrom(toMerge); + result.setPatchsetId(psId); // Got overwriten by copyFrom. + result.setStatusCode(CommitMergeStatus.ALREADY_MERGED); + args.commits.put(result); + return result; + } + @Override public final boolean updateChange(ChangeContext ctx) throws Exception { + logDebug("{}#updateChange for change {}", getClass().getSimpleName(), + toMerge.change().getId()); toMerge.setControl(ctx.getControl()); // Update change and notes from ctx. - PatchSet newPatchSet = updateChangeImpl(ctx); PatchSet.Id oldPsId = checkNotNull(toMerge.getPatchsetId()); - PatchSet.Id newPsId = checkNotNull(ctx.getChange().currentPatchSetId()); - if (newPatchSet == null) { - checkState(oldPsId.equals(newPsId), - "patch set advanced from %s to %s but updateChangeImpl did not return" - + " new patch set instance", oldPsId, newPsId); - // Ok to use stale notes to get the old patch set, which didn't change - // during the submit strategy. - mergedPatchSet = checkNotNull( - args.psUtil.get(ctx.getDb(), ctx.getNotes(), oldPsId), - "missing old patch set %s", oldPsId); + PatchSet.Id newPsId; + + if (alreadyMerged != null) { + alreadyMerged.setControl(ctx.getControl()); + mergedPatchSet = getOrCreateAlreadyMergedPatchSet(ctx); + newPsId = mergedPatchSet.getId(); } else { - PatchSet.Id n = newPatchSet.getId(); - checkState(!n.equals(oldPsId) && n.equals(newPsId), - "current patch was %s and is now %s, but updateChangeImpl returned" - + " new patch set instance at %s", oldPsId, newPsId, n); - mergedPatchSet = newPatchSet; + PatchSet newPatchSet = updateChangeImpl(ctx); + newPsId = checkNotNull(ctx.getChange().currentPatchSetId()); + if (newPatchSet == null) { + checkState(oldPsId.equals(newPsId), + "patch set advanced from %s to %s but updateChangeImpl did not" + + " return new patch set instance", oldPsId, newPsId); + // Ok to use stale notes to get the old patch set, which didn't change + // during the submit strategy. + mergedPatchSet = checkNotNull( + args.psUtil.get(ctx.getDb(), ctx.getNotes(), oldPsId), + "missing old patch set %s", oldPsId); + } else { + PatchSet.Id n = newPatchSet.getId(); + checkState(!n.equals(oldPsId) && n.equals(newPsId), + "current patch was %s and is now %s, but updateChangeImpl returned" + + " new patch set instance at %s", oldPsId, newPsId, n); + mergedPatchSet = newPatchSet; + } } Change c = ctx.getChange(); @@ -168,8 +259,12 @@ abstract class SubmitStrategyOp extends BatchUpdate.Op { id); setApproval(ctx, args.caller); - mergeResultRev = args.mergeTip != null - ? args.mergeTip.getMergeResults().get(commit) : null; + mergeResultRev = alreadyMerged == null + ? args.mergeTip.getMergeResults().get(commit) + // Our fixup code is not smart enough to find a merge commit + // corresponding to the merge result. This results in a different + // ChangeMergedEvent in the fixup case, but we'll just live with that. + : alreadyMerged; String txt = s.getMessage(); ChangeMessage msg; @@ -197,6 +292,30 @@ abstract class SubmitStrategyOp extends BatchUpdate.Op { return true; } + private PatchSet getOrCreateAlreadyMergedPatchSet(ChangeContext ctx) + throws IOException, OrmException { + PatchSet.Id psId = alreadyMerged.getPatchsetId(); + logDebug("Fixing up already-merged patch set {}", psId); + PatchSet prevPs = args.psUtil.current(ctx.getDb(), ctx.getNotes()); + ctx.getRevWalk().parseBody(alreadyMerged); + ctx.getChange().setCurrentPatchSet(psId, + alreadyMerged.getShortMessage(), + ctx.getChange().getOriginalSubject()); + PatchSet existing = args.psUtil.get(ctx.getDb(), ctx.getNotes(), psId); + if (existing != null) { + logDebug("Patch set row exists, only updating change"); + return existing; + } + // No patch set for the already merged commit, although we know it came form + // a patch set ref. Fix up the database. Note that this uses the current + // user as the uploader, which is as good a guess as any. + List groups = prevPs != null + ? prevPs.getGroups() + : GroupCollector.getDefaultGroups(alreadyMerged); + return args.psUtil.insert(ctx.getDb(), ctx.getRevWalk(), + ctx.getUpdate(psId), psId, alreadyMerged, false, groups, null); + } + private void setApproval(ChangeContext ctx, IdentifiedUser user) throws OrmException { Change.Id id = ctx.getChange().getId();