Start converting CherryPick to use BatchUpdate

Start conservatively, only implementing the single-parent case, using
one BatchUpdate per change to be merged. All commits are created
outside of the batch, and the helper methods in MergeUtil still
aggressively flush objects. Ref and DB updates go through the
BatchUpdate.

Change-Id: Ifa2aab363abcdd38549399b3acf18db7eb43a57e

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

@@ -307,7 +307,7 @@ public class PatchSetInserter {
 
     @Override
     public void updateRepo(RepoContext ctx) throws IOException {
-      ctx.getBatchRefUpdate().addCommand(new ReceiveCommand(ObjectId.zeroId(),
+      ctx.addRefUpdate(new ReceiveCommand(ObjectId.zeroId(),
           commit, patchSet.getRefName(), ReceiveCommand.Type.CREATE));
     }
 

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

@@ -111,6 +111,10 @@ public class BatchUpdate implements AutoCloseable {
       }
       return batchRefUpdate;
     }
+
+    public void addRefUpdate(ReceiveCommand cmd) throws IOException {
+      getBatchRefUpdate().addCommand(cmd);
+    }
   }
 
   public class ChangeContext extends Context {
@@ -210,7 +214,6 @@ public class BatchUpdate implements AutoCloseable {
       closeRepo = true;
       inserter = repo.newObjectInserter();
       revWalk = new RevWalk(inserter.newReader());
-      batchRefUpdate = repo.getRefDatabase().newBatchUpdate();
     }
   }
 
@@ -229,12 +232,6 @@ public class BatchUpdate implements AutoCloseable {
     return inserter;
   }
 
-  public BatchUpdate addRefUpdate(ReceiveCommand cmd) throws IOException {
-    initRepository();
-    batchRefUpdate.addCommand(cmd);
-    return this;
-  }
-
   public BatchUpdate addOp(ChangeControl ctl, Op op) {
     Change.Id id = ctl.getChange().getId();
     ChangeControl old = changeControls.get(id);

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

@@ -16,6 +16,7 @@ package com.google.gerrit.server.git.strategy;
 
 import com.google.common.collect.Lists;
 import com.google.gerrit.common.TimeUtil;
+import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetAncestor;
@@ -23,7 +24,9 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ChangeUtil;
-import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
+import com.google.gerrit.server.git.BatchUpdate;
+import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
+import com.google.gerrit.server.git.BatchUpdate.RepoContext;
 import com.google.gerrit.server.git.CodeReviewCommit;
 import com.google.gerrit.server.git.CommitMergeStatus;
 import com.google.gerrit.server.git.GroupCollector;
@@ -31,16 +34,18 @@ import com.google.gerrit.server.git.MergeConflictException;
 import com.google.gerrit.server.git.MergeException;
 import com.google.gerrit.server.git.MergeIdenticalTreeException;
 import com.google.gerrit.server.git.MergeTip;
+import com.google.gerrit.server.git.UpdateException;
 import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gwtorm.server.OrmException;
 
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.PersonIdent;
-import org.eclipse.jgit.lib.RefUpdate;
 import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.transport.ReceiveCommand;
 
 import java.io.IOException;
+import java.sql.Timestamp;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -50,16 +55,12 @@ import java.util.Map;
 
 public class CherryPick extends SubmitStrategy {
   private final PatchSetInfoFactory patchSetInfoFactory;
-  private final GitReferenceUpdated gitRefUpdated;
   private final Map<Change.Id, CodeReviewCommit> newCommits;
 
   CherryPick(SubmitStrategy.Arguments args,
-      PatchSetInfoFactory patchSetInfoFactory,
+      PatchSetInfoFactory patchSetInfoFactory) {
-      GitReferenceUpdated gitRefUpdated) {
     super(args);
-
     this.patchSetInfoFactory = patchSetInfoFactory;
-    this.gitRefUpdated = gitRefUpdated;
     this.newCommits = new HashMap<>();
   }
 
@@ -70,20 +71,27 @@ public class CherryPick extends SubmitStrategy {
     List<CodeReviewCommit> sorted = CodeReviewCommit.ORDER.sortedCopy(toMerge);
     while (!sorted.isEmpty()) {
       CodeReviewCommit n = sorted.remove(0);
-      try {
+      Timestamp now = TimeUtil.nowTs();
+      try (BatchUpdate u = args.newBatchUpdate(now)) {
+        // TODO(dborowitz): This won't work when mergeTip is updated only at the
+        // end of the batch.
         if (mergeTip.getCurrentTip() == null) {
           cherryPickUnbornRoot(n, mergeTip);
         } else if (n.getParentCount() == 0) {
           cherryPickRootOntoBranch(n);
         } else if (n.getParentCount() == 1) {
-          cherryPickOne(n, mergeTip);
+          u.addOp(n.getControl(), new CherryPickOneOp(mergeTip, n));
         } else {
-          cherryPickMultipleParents(n, mergeTip);
+          cherryPickMultipleParents(n, mergeTip, now);
         }
-      } catch (NoSuchChangeException | IOException | OrmException e) {
+        u.execute();
+      } catch (IOException | UpdateException | RestApiException e) {
         throw new MergeException("Cannot merge " + n.name(), e);
       }
     }
+    // TODO(dborowitz): When BatchUpdate is hoisted out of CherryPick,
+    // SubmitStrategy should probably no longer return MergeTip, instead just
+    // mutating a single shared MergeTip passed in from the caller.
     return mergeTip;
   }
 
@@ -99,29 +107,8 @@ public class CherryPick extends SubmitStrategy {
     n.setStatusCode(CommitMergeStatus.CANNOT_CHERRY_PICK_ROOT);
   }
 
-  private void cherryPickOne(CodeReviewCommit n, MergeTip mergeTip)
+  private void cherryPickMultipleParents(CodeReviewCommit n, MergeTip mergeTip,
-      throws NoSuchChangeException, OrmException, IOException {
+      Timestamp when) throws IOException, MergeException {
-    // If there is only one parent, a cherry-pick can be done by taking the
-    // delta relative to that one parent and redoing that on the current merge
-    // tip.
-    //
-    // Keep going in the case of a single merge failure; the goal is to
-    // cherry-pick as many commits as possible.
-    try {
-      CodeReviewCommit merge =
-          writeCherryPickCommit(mergeTip.getCurrentTip(), n);
-      mergeTip.moveTipTo(merge, merge);
-      newCommits.put(mergeTip.getCurrentTip().getPatchsetId()
-          .getParentKey(), mergeTip.getCurrentTip());
-    } catch (MergeConflictException mce) {
-      n.setStatusCode(CommitMergeStatus.PATH_CONFLICT);
-    } catch (MergeIdenticalTreeException mie) {
-      n.setStatusCode(CommitMergeStatus.ALREADY_MERGED);
-    }
-  }
-
-  private void cherryPickMultipleParents(CodeReviewCommit n, MergeTip mergeTip)
-      throws IOException, MergeException {
     // There are multiple parents, so this is a merge commit. We don't want
     // to cherry-pick the merge as clients can't easily rebase their history
     // with that merge present and replaced by an equivalent merge with a
@@ -131,7 +118,7 @@ public class CherryPick extends SubmitStrategy {
       if (args.rw.isMergedInto(mergeTip.getCurrentTip(), n)) {
         mergeTip.moveTipTo(n, n);
       } else {
-        PersonIdent myIdent = args.serverIdent.get();
+        PersonIdent myIdent = new PersonIdent(args.serverIdent.get(), when);
         CodeReviewCommit result = args.mergeUtil.mergeOneCommit(myIdent,
             myIdent, args.repo, args.rw, args.inserter,
             args.canMergeFlag, args.destBranch, mergeTip.getCurrentTip(), n);
@@ -146,70 +133,80 @@ public class CherryPick extends SubmitStrategy {
     }
   }
 
-  private CodeReviewCommit writeCherryPickCommit(CodeReviewCommit mergeTip,
+  private class CherryPickOneOp extends BatchUpdate.Op {
-      CodeReviewCommit n) throws IOException, OrmException,
+    private final MergeTip mergeTip;
-      NoSuchChangeException, MergeConflictException,
+    private final CodeReviewCommit toMerge;
-      MergeIdenticalTreeException {
 
-    args.rw.parseBody(n);
+    private PatchSet.Id psId;
+    private CodeReviewCommit newCommit;
 
-    String cherryPickCmtMsg = args.mergeUtil.createCherryPickCommitMessage(n);
+    private CherryPickOneOp(MergeTip mergeTip, CodeReviewCommit n) {
+      this.mergeTip = mergeTip;
+      this.toMerge = n;
+    }
+
+    @Override
+    public void updateRepo(RepoContext ctx) throws IOException {
+      // If there is only one parent, a cherry-pick can be done by taking the
+      // delta relative to that one parent and redoing that on the current merge
+      // tip.
+      args.rw.parseBody(toMerge);
+      psId = ChangeUtil.nextPatchSetId(
+          args.repo, toMerge.change().currentPatchSetId());
+      String cherryPickCmtMsg =
+          args.mergeUtil.createCherryPickCommitMessage(toMerge);
 
       PersonIdent committer = args.caller.newCommitterIdent(
-        TimeUtil.nowTs(), args.serverIdent.get().getTimeZone());
+          ctx.getWhen(), args.serverIdent.get().getTimeZone());
-    CodeReviewCommit newCommit = args.mergeUtil.createCherryPickFromCommit(
+      try {
-        args.repo, args.inserter, mergeTip, n, committer, cherryPickCmtMsg,
+        newCommit = args.mergeUtil.createCherryPickFromCommit(
-        args.rw);
+            args.repo, args.inserter, mergeTip.getCurrentTip(), toMerge,
+            committer, cherryPickCmtMsg, args.rw);
+        ctx.addRefUpdate(
+            new ReceiveCommand(ObjectId.zeroId(), newCommit, psId.toRefName()));
+      } catch (MergeConflictException mce) {
+        // Keep going in the case of a single merge failure; the goal is to
+        // cherry-pick as many commits as possible.
+        toMerge.setStatusCode(CommitMergeStatus.PATH_CONFLICT);
+      } catch (MergeIdenticalTreeException mie) {
+        toMerge.setStatusCode(CommitMergeStatus.ALREADY_MERGED);
+      }
+    }
 
-    PatchSet.Id id =
+    @Override
-        ChangeUtil.nextPatchSetId(args.repo, n.change().currentPatchSetId());
+    public void updateChange(ChangeContext ctx) throws OrmException,
-    PatchSet ps = new PatchSet(id);
+         NoSuchChangeException {
-    ps.setCreatedOn(TimeUtil.nowTs());
+      if (newCommit == null) {
+        // Merge conflict; don't update change.
+        return;
+      }
+      PatchSet ps = new PatchSet(psId);
+      ps.setCreatedOn(ctx.getWhen());
       ps.setUploader(args.caller.getAccountId());
       ps.setRevision(new RevId(newCommit.getId().getName()));
 
-    RefUpdate ru;
+      Change c = toMerge.change();
-
+      ps.setGroups(GroupCollector.getCurrentGroups(args.db, c));
-    args.db.changes().beginTransaction(n.change().getId());
-    try {
-      insertAncestors(args.db, ps.getId(), newCommit);
-      ps.setGroups(GroupCollector.getCurrentGroups(args.db, n.change()));
       args.db.patchSets().insert(Collections.singleton(ps));
-      n.change()
+      insertAncestors(args.db, ps.getId(), newCommit);
-          .setCurrentPatchSet(patchSetInfoFactory.get(newCommit, ps.getId()));
+      c.setCurrentPatchSet(patchSetInfoFactory.get(newCommit, ps.getId()));
-      args.db.changes().update(Collections.singletonList(n.change()));
+      args.db.changes().update(Collections.singletonList(c));
 
       List<PatchSetApproval> approvals = Lists.newArrayList();
       for (PatchSetApproval a : args.approvalsUtil.byPatchSet(
-          args.db, n.getControl(), n.getPatchsetId())) {
+          args.db, toMerge.getControl(), toMerge.getPatchsetId())) {
         approvals.add(new PatchSetApproval(ps.getId(), a));
       }
       args.db.patchSetApprovals().insert(approvals);
 
-      ru = args.repo.updateRef(ps.getRefName());
+      newCommit.copyFrom(toMerge);
-      ru.setExpectedOldObjectId(ObjectId.zeroId());
-      ru.setNewObjectId(newCommit);
-      ru.disableRefLog();
-      if (ru.update(args.rw) != RefUpdate.Result.NEW) {
-        throw new IOException(String.format(
-            "Failed to create ref %s in %s: %s", ps.getRefName(), n.change()
-                .getDest().getParentKey().get(), ru.getResult()));
-      }
-
-      args.db.commit();
-    } finally {
-      args.db.rollback();
-    }
-
-    gitRefUpdated.fire(n.change().getProject(), ru);
-
-    newCommit.copyFrom(n);
       newCommit.setStatusCode(CommitMergeStatus.CLEAN_PICK);
       newCommit.setControl(
-        args.changeControlFactory.controlFor(n.change(), args.caller));
+          args.changeControlFactory.controlFor(toMerge.change(), args.caller));
-    newCommits.put(newCommit.getPatchsetId().getParentKey(), newCommit);
+      mergeTip.moveTipTo(newCommit, newCommit);
+      newCommits.put(c.getId(), newCommit);
       setRefLogIdent();
-    return newCommit;
+    }
   }
 
   private static void insertAncestors(ReviewDb db, PatchSet.Id id,

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

@@ -22,6 +22,7 @@ import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.git.BatchUpdate;
 import com.google.gerrit.server.git.CodeReviewCommit;
 import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
 import com.google.gerrit.server.git.MergeException;
@@ -39,6 +40,7 @@ import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevFlag;
 
+import java.sql.Timestamp;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Map;
@@ -55,6 +57,7 @@ public abstract class SubmitStrategy {
     protected final IdentifiedUser.GenericFactory identifiedUserFactory;
     protected final Provider<PersonIdent> serverIdent;
     protected final ReviewDb db;
+    protected final BatchUpdate.Factory batchUpdateFactory;
     protected final ChangeControl.GenericFactory changeControlFactory;
 
     protected final Repository repo;
@@ -71,6 +74,7 @@ public abstract class SubmitStrategy {
 
     Arguments(IdentifiedUser.GenericFactory identifiedUserFactory,
         Provider<PersonIdent> serverIdent, ReviewDb db,
+        BatchUpdate.Factory batchUpdateFactory,
         ChangeControl.GenericFactory changeControlFactory, Repository repo,
         CodeReviewRevWalk rw, ObjectInserter inserter, RevFlag canMergeFlag,
         Set<RevCommit> alreadyAccepted, Branch.NameKey destBranch,
@@ -79,6 +83,7 @@ public abstract class SubmitStrategy {
       this.identifiedUserFactory = identifiedUserFactory;
       this.serverIdent = serverIdent;
       this.db = db;
+      this.batchUpdateFactory = batchUpdateFactory;
       this.changeControlFactory = changeControlFactory;
 
       this.repo = repo;
@@ -93,6 +98,11 @@ public abstract class SubmitStrategy {
       this.mergeSorter = new MergeSorter(rw, alreadyAccepted, canMergeFlag);
       this.caller = caller;
     }
+
+    BatchUpdate newBatchUpdate(Timestamp when) {
+      return batchUpdateFactory.create(db, destBranch.getParentKey(), when)
+          .setRepository(repo, rw, inserter);
+    }
   }
 
   protected final Arguments args;

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

@@ -21,7 +21,7 @@ import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.change.RebaseChange;
-import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
+import com.google.gerrit.server.git.BatchUpdate;
 import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
 import com.google.gerrit.server.git.MergeException;
 import com.google.gerrit.server.git.MergeUtil;
@@ -53,9 +53,9 @@ public class SubmitStrategyFactory {
 
   private final IdentifiedUser.GenericFactory identifiedUserFactory;
   private final Provider<PersonIdent> myIdent;
+  private final BatchUpdate.Factory batchUpdateFactory;
   private final ChangeControl.GenericFactory changeControlFactory;
   private final PatchSetInfoFactory patchSetInfoFactory;
-  private final GitReferenceUpdated gitRefUpdated;
   private final RebaseChange rebaseChange;
   private final ProjectCache projectCache;
   private final ApprovalsUtil approvalsUtil;
@@ -66,18 +66,19 @@ public class SubmitStrategyFactory {
   SubmitStrategyFactory(
       final IdentifiedUser.GenericFactory identifiedUserFactory,
       @GerritPersonIdent Provider<PersonIdent> myIdent,
+      final BatchUpdate.Factory batchUpdateFactory,
       final ChangeControl.GenericFactory changeControlFactory,
       final PatchSetInfoFactory patchSetInfoFactory,
-      final GitReferenceUpdated gitRefUpdated, final RebaseChange rebaseChange,
+      final RebaseChange rebaseChange,
       final ProjectCache projectCache,
       final ApprovalsUtil approvalsUtil,
       final MergeUtil.Factory mergeUtilFactory,
       final ChangeIndexer indexer) {
     this.identifiedUserFactory = identifiedUserFactory;
     this.myIdent = myIdent;
+    this.batchUpdateFactory = batchUpdateFactory;
     this.changeControlFactory = changeControlFactory;
     this.patchSetInfoFactory = patchSetInfoFactory;
-    this.gitRefUpdated = gitRefUpdated;
     this.rebaseChange = rebaseChange;
     this.projectCache = projectCache;
     this.approvalsUtil = approvalsUtil;
@@ -92,12 +93,13 @@ public class SubmitStrategyFactory {
       throws MergeException, NoSuchProjectException {
     ProjectState project = getProject(destBranch);
     SubmitStrategy.Arguments args = new SubmitStrategy.Arguments(
-        identifiedUserFactory, myIdent, db, changeControlFactory, repo, rw,
+        identifiedUserFactory, myIdent, db, batchUpdateFactory,
-        inserter, canMergeFlag, alreadyAccepted, destBranch,approvalsUtil,
+        changeControlFactory, repo, rw, inserter, canMergeFlag, alreadyAccepted,
-        mergeUtilFactory.create(project), indexer, caller);
+        destBranch,approvalsUtil, mergeUtilFactory.create(project), indexer,
+        caller);
     switch (submitType) {
       case CHERRY_PICK:
-        return new CherryPick(args, patchSetInfoFactory, gitRefUpdated);
+        return new CherryPick(args, patchSetInfoFactory);
       case FAST_FORWARD_ONLY:
         return new FastForwardOnly(args);
       case MERGE_ALWAYS: