Merge "Recover from merges that failed after updateRepo"

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<? extends RestApiException> 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 {

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);
+    }
+  }
 }

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);
+    }
+  }
 }

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);
+    }
+  }
 }

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)) {

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<RevisionResource, SubmitInput>,
     }
   }
 
+  /**
+   * 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<ReviewDb> dbProvider;
   private final GitRepositoryManager repoManager;
   private final ChangeData.Factory changeDataFactory;
@@ -185,7 +202,7 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
 
     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());
     }
 

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) {

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<String> 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()) {

gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java

@@ -672,13 +672,15 @@ public class MergeUtil {
 
   public Set<Change.Id> findUnmergedChanges(Set<Change.Id> expected,
       CodeReviewRevWalk rw, RevFlag canMergeFlag, CodeReviewCommit oldTip,
-      CodeReviewCommit mergeTip) throws IntegrationException {
+      CodeReviewCommit mergeTip, Iterable<Change.Id> alreadyMerged)
+      throws IntegrationException {
     if (mergeTip == null) {
       return expected;
     }
 
     try {
       Set<Change.Id> 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<CodeReviewCommit> 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;
+  }
 }

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());

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<SubmitStrategy> strategies;
   private final CommitStatus commits;
+  private final boolean failAfterRefUpdates;
 
-  public SubmitStrategyListener(Collection<SubmitStrategy> strategies,
+  public SubmitStrategyListener(SubmitInput input,
-      CommitStatus commits) {
+      Collection<SubmitStrategy> 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();
+      List<Change.Id> alreadyMerged = checkCommitStatus();
-      findUnmergedChanges();
+      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<Change.Id> alreadyMerged)
       throws ResourceConflictException, IntegrationException {
     for (SubmitStrategy strategy : strategies) {
       if (strategy instanceof CherryPick) {
@@ -57,7 +74,7 @@ public class SubmitStrategyListener extends BatchUpdate.Listener {
       Set<Change.Id> 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<Change.Id> checkCommitStatus() throws ResourceConflictException {
+    List<Change.Id> 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

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<Ref> refs = ctx.getRepository().getRefDatabase()
+        .getRefs(id.toRefPrefix()).values();
+    List<CodeReviewCommit> 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<CodeReviewCommit, PatchSet.Id>() {
+            @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());
+    PatchSet.Id newPsId;
-    if (newPatchSet == null) {
+
-      checkState(oldPsId.equals(newPsId),
+    if (alreadyMerged != null) {
-          "patch set advanced from %s to %s but updateChangeImpl did not return"
+      alreadyMerged.setControl(ctx.getControl());
-          + " new patch set instance", oldPsId, newPsId);
+      mergedPatchSet = getOrCreateAlreadyMergedPatchSet(ctx);
-      // Ok to use stale notes to get the old patch set, which didn't change
+      newPsId = mergedPatchSet.getId();
-      // 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();
+      PatchSet newPatchSet = updateChangeImpl(ctx);
-      checkState(!n.equals(oldPsId) && n.equals(newPsId),
+      newPsId = checkNotNull(ctx.getChange().currentPatchSetId());
-          "current patch was %s and is now %s, but updateChangeImpl returned"
+      if (newPatchSet == null) {
-          + " new patch set instance at %s", oldPsId, newPsId, n);
+        checkState(oldPsId.equals(newPsId),
-      mergedPatchSet = newPatchSet;
+            "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
+      mergeResultRev = alreadyMerged == null
-          ? args.mergeTip.getMergeResults().get(commit) : 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<String> 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();