Always advance MergeTip during updateRepo

Multiple batch update operations expect to see the results of
previous updates. In the case of inspecting the current MergeTip,
subsequent updateRepo calls expect to see the tip advancing. However,
this was not always done in updateRepo; in the rebase and cherry-pick
cases we were mistakenly delaying this until updateChange.

This resulted in a serious bug during RebaseIfNecessary, where commits
after the first one were all rebased onto the original branch tip
rather than following previous commits in the series. Worse, this case
was untested, so add a test for that.

Change-Id: Ib4f0a5667a8d2b7d37ca75b81368b24ef79374fb

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

@@ -21,7 +21,10 @@ import com.google.gerrit.acceptance.TestProjectInput;
 import com.google.gerrit.extensions.client.InheritableBoolean;
 import com.google.gerrit.extensions.client.SubmitType;
 
+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 class SubmitByRebaseIfNecessaryIT extends AbstractSubmit {
@@ -71,6 +74,43 @@ public class SubmitByRebaseIfNecessaryIT extends AbstractSubmit {
     assertPersonEquals(admin.getIdent(), head.getCommitterIdent());
   }
 
+
+  @Test
+  public void submitWithRebaseMultipleChanges() throws Exception {
+    RevCommit initialHead = getRemoteHead();
+    PushOneCommit.Result change1 =
+        createChange("Change 1", "a.txt", "content");
+    submit(change1.getChangeId());
+
+    testRepo.reset(initialHead);
+    PushOneCommit.Result change2 =
+        createChange("Change 2", "b.txt", "other content");
+    assertThat(change2.getCommit().getParent(0))
+        .isNotEqualTo(change1.getCommit());
+    PushOneCommit.Result change3 =
+        createChange("Change 3", "c.txt", "third content");
+    approve(change2.getChangeId());
+    submit(change3.getChangeId());
+
+    assertRebase(testRepo, false);
+    assertApproved(change2.getChangeId());
+    assertApproved(change3.getChangeId());
+
+    RevCommit head = parse(getRemoteHead());
+    assertThat(head.getShortMessage()).isEqualTo("Change 3");
+    assertThat(head).isNotEqualTo(change3.getCommit());
+    assertCurrentRevision(change3.getChangeId(), 2, head);
+
+    RevCommit parent = parse(head.getParent(0));
+    assertThat(parent.getShortMessage()).isEqualTo("Change 2");
+    assertThat(parent).isNotEqualTo(change2.getCommit());
+    assertCurrentRevision(change2.getChangeId(), 2, parent);
+
+    RevCommit grandparent = parse(parent.getParent(0));
+    assertThat(grandparent).isEqualTo(change1.getCommit());
+    assertCurrentRevision(change1.getChangeId(), 1, grandparent);
+  }
+
   @Test
   @TestProjectInput(useContentMerge = InheritableBoolean.TRUE)
   public void submitWithContentMerge() throws Exception {
@@ -113,4 +153,13 @@ public class SubmitByRebaseIfNecessaryIT extends AbstractSubmit {
     assertCurrentRevision(change2.getChangeId(), 1, change2.getCommitId());
     assertNoSubmitter(change2.getChangeId(), 1);
   }
+
+  private RevCommit parse(ObjectId id) throws Exception {
+    try (Repository repo = repoManager.openRepository(project);
+        RevWalk rw = new RevWalk(repo)) {
+      RevCommit c = rw.parseCommit(id);
+      rw.parseBody(c);
+      return c;
+    }
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java

@@ -170,6 +170,12 @@ public class RebaseChangeOp extends BatchUpdate.Op {
     return rebasedCommit;
   }
 
+  public PatchSet.Id getPatchSetId() {
+    checkState(rebasedPatchSetId != null,
+        "getPatchSetId() only valid after updateRepo");
+    return rebasedPatchSetId;
+  }
+
   public PatchSet getPatchSet() {
     checkState(rebasedPatchSet != null,
         "getPatchSet() only valid after executing update");

gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java

@@ -147,6 +147,10 @@ public class CherryPick extends SubmitStrategy {
         newCommit = args.mergeUtil.createCherryPickFromCommit(
             args.repo, args.inserter, mergeTip.getCurrentTip(), toMerge,
             committer, cherryPickCmtMsg, args.rw);
+        newCommit.copyFrom(toMerge);
+        newCommit.setStatusCode(CommitMergeStatus.CLEAN_PICK);
+        mergeTip.moveTipTo(newCommit, newCommit);
+        args.commits.put(newCommit);
         ctx.addRefUpdate(
             new ReceiveCommand(ObjectId.zeroId(), newCommit, psId.toRefName()));
         patchSetInfo =
@@ -186,12 +190,8 @@ public class CherryPick extends SubmitStrategy {
       }
       args.db.patchSetApprovals().insert(approvals);
 
-      newCommit.copyFrom(toMerge);
-      newCommit.setStatusCode(CommitMergeStatus.CLEAN_PICK);
       newCommit.setControl(
           args.changeControlFactory.controlFor(toMerge.change(), args.caller));
-      mergeTip.moveTipTo(newCommit, newCommit);
-      args.commits.put(newCommit);
     }
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java

@@ -36,8 +36,6 @@ import com.google.gerrit.server.project.InvalidChangeOperationException;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gwtorm.server.OrmException;
 
-import org.eclipse.jgit.lib.ObjectId;
-
 import java.io.IOException;
 import java.util.Collection;
 import java.util.Collections;
@@ -123,6 +121,7 @@ public class RebaseIfNecessary extends SubmitStrategy {
     private final CodeReviewCommit toMerge;
 
     private RebaseChangeOp rebaseOp;
+    private CodeReviewCommit newCommit;
 
     private RebaseOneOp(MergeTip mergeTip, CodeReviewCommit toMerge) {
       this.mergeTip = mergeTip;
@@ -161,6 +160,13 @@ public class RebaseIfNecessary extends SubmitStrategy {
         throw new IntegrationException(
             "Cannot rebase " + toMerge.name() + ": " + e.getMessage(), e);
       }
+      newCommit = args.rw.parseCommit(rebaseOp.getRebasedCommit());
+      newCommit.copyFrom(toMerge);
+      newCommit.setStatusCode(CommitMergeStatus.CLEAN_REBASE);
+      newCommit.setPatchsetId(rebaseOp.getPatchSetId());
+      mergeTip.moveTipTo(newCommit, newCommit);
+      args.commits.put(mergeTip.getCurrentTip());
+      acceptMergeTip(mergeTip);
     }
 
     @Override
@@ -172,29 +178,19 @@ public class RebaseIfNecessary extends SubmitStrategy {
       }
 
       rebaseOp.updateChange(ctx);
-      PatchSet newPatchSet = rebaseOp.getPatchSet();
+      PatchSet.Id newPatchSetId = rebaseOp.getPatchSetId();
       List<PatchSetApproval> approvals = Lists.newArrayList();
       for (PatchSetApproval a : args.approvalsUtil.byPatchSet(ctx.getDb(),
           toMerge.getControl(), toMerge.getPatchsetId())) {
-        approvals.add(new PatchSetApproval(newPatchSet.getId(), a));
+        approvals.add(new PatchSetApproval(newPatchSetId, a));
       }
       args.db.patchSetApprovals().insert(approvals);
 
-      // TODO(dborowitz): Make RevWalk available via BatchUpdate.
-      CodeReviewCommit newTip = args.rw.parseCommit(
-          ObjectId.fromString(newPatchSet.getRevision().get()));
-      mergeTip.moveTipTo(newTip, newTip);
       toMerge.change().setCurrentPatchSet(
           args.patchSetInfoFactory.get(args.rw, mergeTip.getCurrentTip(),
-              newPatchSet.getId()));
+              newPatchSetId));
-      mergeTip.getCurrentTip().copyFrom(toMerge);
+      newCommit.setControl(
-      mergeTip.getCurrentTip().setControl(
           args.changeControlFactory.controlFor(toMerge.change(), args.caller));
-      mergeTip.getCurrentTip().setPatchsetId(newPatchSet.getId());
-      mergeTip.getCurrentTip().setStatusCode(
-          CommitMergeStatus.CLEAN_REBASE);
-      args.commits.put(mergeTip.getCurrentTip());
-      acceptMergeTip(mergeTip);
     }
 
     @Override