Defer object flushing when merging

Mergers in the most recent JGit know how to read previously-inserted
objects back, so a series of merges does not require aggressively
flushing the objects. Instead, flush objects in MergeOp just before
updating the relevant refs.

Change-Id: Ia85c5114e86739b4a480bea65f3413fe47cf572d

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

@@ -113,87 +113,89 @@ public class CherryPickChange {
 
     Project.NameKey project = change.getProject();
     IdentifiedUser identifiedUser = (IdentifiedUser) currentUser.get();
-    final Repository git;
+    Repository git = null;
+    ObjectInserter oi = null;
+    RevWalk revWalk = null;
+
     try {
       git = gitManager.openRepository(project);
+      oi = git.newObjectInserter();
+      revWalk = new RevWalk(oi.newReader());
+
+      Ref destRef = git.getRef(destinationBranch);
+      if (destRef == null) {
+        throw new InvalidChangeOperationException("Branch "
+            + destinationBranch + " does not exist.");
+      }
+
+      final RevCommit mergeTip = revWalk.parseCommit(destRef.getObjectId());
+
+      RevCommit commitToCherryPick =
+          revWalk.parseCommit(ObjectId.fromString(patch.getRevision().get()));
+
+      PersonIdent committerIdent =
+          identifiedUser.newCommitterIdent(TimeUtil.nowTs(),
+              serverTimeZone);
+
+      final ObjectId computedChangeId =
+          ChangeIdUtil
+              .computeChangeId(commitToCherryPick.getTree(), mergeTip,
+                  commitToCherryPick.getAuthorIdent(), committerIdent, message);
+      String commitMessage =
+          ChangeIdUtil.insertId(message, computedChangeId).trim() + '\n';
+
+      RevCommit cherryPickCommit;
+      ProjectState projectState = refControl.getProjectControl().getProjectState();
+      cherryPickCommit =
+          mergeUtilFactory.create(projectState).createCherryPickFromCommit(git, oi, mergeTip,
+              commitToCherryPick, committerIdent, commitMessage, revWalk);
+      oi.flush();
+
+      Change.Key changeKey;
+      final List<String> idList = cherryPickCommit.getFooterLines(
+          FooterConstants.CHANGE_ID);
+      if (!idList.isEmpty()) {
+        final String idStr = idList.get(idList.size() - 1).trim();
+        changeKey = new Change.Key(idStr);
+      } else {
+        changeKey = new Change.Key("I" + computedChangeId.name());
+      }
+
+      Branch.NameKey newDest =
+          new Branch.NameKey(change.getProject(), destRef.getName());
+      List<ChangeData> destChanges = queryProvider.get()
+          .setLimit(2)
+          .byBranchKey(newDest, changeKey);
+      if (destChanges.size() > 1) {
+        throw new InvalidChangeOperationException("Several changes with key "
+            + changeKey + " reside on the same branch. "
+            + "Cannot create a new patch set.");
+      } else if (destChanges.size() == 1) {
+        // The change key exists on the destination branch. The cherry pick
+        // will be added as a new patch set.
+        return insertPatchSet(git, revWalk, destChanges.get(0).change(),
+            cherryPickCommit, refControl, identifiedUser);
+      } else {
+        // Change key not found on destination branch. We can create a new
+        // change.
+        return createNewChange(git, revWalk, changeKey, project,
+            patch.getId(), destRef, cherryPickCommit, refControl,
+            identifiedUser, change.getTopic());
+      }
+    } catch (MergeIdenticalTreeException | MergeConflictException e) {
+      throw new MergeException("Cherry pick failed: " + e.getMessage());
     } catch (RepositoryNotFoundException e) {
       throw new NoSuchChangeException(change.getId(), e);
-    }
+    } finally {
-
+      if (revWalk != null) {
-    try {
-      RevWalk revWalk = new RevWalk(git);
-      try {
-        Ref destRef = git.getRef(destinationBranch);
-        if (destRef == null) {
-          throw new InvalidChangeOperationException("Branch "
-              + destinationBranch + " does not exist.");
-        }
-
-        final RevCommit mergeTip = revWalk.parseCommit(destRef.getObjectId());
-
-        RevCommit commitToCherryPick =
-            revWalk.parseCommit(ObjectId.fromString(patch.getRevision().get()));
-
-        PersonIdent committerIdent =
-            identifiedUser.newCommitterIdent(TimeUtil.nowTs(),
-                serverTimeZone);
-
-        final ObjectId computedChangeId =
-            ChangeIdUtil
-                .computeChangeId(commitToCherryPick.getTree(), mergeTip,
-                    commitToCherryPick.getAuthorIdent(), committerIdent, message);
-        String commitMessage =
-            ChangeIdUtil.insertId(message, computedChangeId).trim() + '\n';
-
-        RevCommit cherryPickCommit;
-        ObjectInserter oi = git.newObjectInserter();
-        try {
-          ProjectState projectState = refControl.getProjectControl().getProjectState();
-          cherryPickCommit =
-              mergeUtilFactory.create(projectState).createCherryPickFromCommit(git, oi, mergeTip,
-                  commitToCherryPick, committerIdent, commitMessage, revWalk);
-        } catch (MergeIdenticalTreeException | MergeConflictException e) {
-          throw new MergeException("Cherry pick failed: " + e.getMessage());
-        } finally {
-          oi.release();
-        }
-
-        Change.Key changeKey;
-        final List<String> idList = cherryPickCommit.getFooterLines(
-            FooterConstants.CHANGE_ID);
-        if (!idList.isEmpty()) {
-          final String idStr = idList.get(idList.size() - 1).trim();
-          changeKey = new Change.Key(idStr);
-        } else {
-          changeKey = new Change.Key("I" + computedChangeId.name());
-        }
-
-        Branch.NameKey newDest =
-            new Branch.NameKey(change.getProject(), destRef.getName());
-        List<ChangeData> destChanges = queryProvider.get()
-            .setLimit(2)
-            .byBranchKey(newDest, changeKey);
-        if (destChanges.size() > 1) {
-          throw new InvalidChangeOperationException("Several changes with key "
-              + changeKey + " reside on the same branch. "
-              + "Cannot create a new patch set.");
-        } else if (destChanges.size() == 1) {
-          // The change key exists on the destination branch. The cherry pick
-          // will be added as a new patch set.
-          return insertPatchSet(git, revWalk, destChanges.get(0).change(),
-              cherryPickCommit, refControl, identifiedUser);
-        } else {
-          // Change key not found on destination branch. We can create a new
-          // change.
-          return createNewChange(git, revWalk, changeKey, project,
-              patch.getId(), destRef, cherryPickCommit, refControl,
-              identifiedUser, change.getTopic());
-        }
-      } finally {
         revWalk.release();
       }
-    } finally {
+      if (oi != null) {
-      git.close();
+        oi.release();
+      }
+      if (git != null) {
+        git.close();
+      }
     }
   }
 

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

@@ -23,6 +23,7 @@ import com.google.gerrit.server.project.ChangeControl;
 
 import org.eclipse.jgit.lib.AnyObjectId;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectReader;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
@@ -50,12 +51,11 @@ public class CodeReviewCommit extends RevCommit {
       }).nullsFirst();
 
   public static RevWalk newRevWalk(Repository repo) {
-    return new RevWalk(repo) {
+    return new CodeReviewRevWalk(repo);
-      @Override
+  }
-      protected RevCommit createCommit(AnyObjectId id) {
+
-        return new CodeReviewCommit(id);
+  public static RevWalk newRevWalk(ObjectReader reader) {
-      }
+    return new CodeReviewRevWalk(reader);
-    };
   }
 
   static CodeReviewCommit revisionGone(ChangeControl ctl) {
@@ -85,6 +85,21 @@ public class CodeReviewCommit extends RevCommit {
     return r;
   }
 
+  private static class CodeReviewRevWalk extends RevWalk {
+    private CodeReviewRevWalk(Repository repo) {
+      super(repo);
+    }
+
+    private CodeReviewRevWalk(ObjectReader reader) {
+      super(reader);
+    }
+
+    @Override
+    protected RevCommit createCommit(AnyObjectId id) {
+      return new CodeReviewCommit(id);
+    }
+  }
+
   /**
    * Unique key of the PatchSet entity from the code review system.
    * <p>

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

@@ -430,13 +430,11 @@ public class MergeOp {
       String m = "Error opening repository \"" + name.get() + '"';
       throw new MergeException(m, err);
     }
-
+    inserter = repo.newObjectInserter();
-    rw = CodeReviewCommit.newRevWalk(repo);
+    rw = CodeReviewCommit.newRevWalk(inserter.newReader());
     rw.sort(RevSort.TOPO);
     rw.sort(RevSort.COMMIT_TIME_DESC, true);
     canMergeFlag = rw.newFlag("CAN_MERGE");
-
-    inserter = repo.newObjectInserter();
   }
 
   private RefUpdate openBranch()
@@ -674,6 +672,11 @@ public class MergeOp {
             + destProject.getProject().getName(), e);
       }
     }
+    try {
+      inserter.flush();
+    } catch (IOException e) {
+      throw new MergeException("Cannot flush merge results", e);
+    }
 
     branchUpdate.setRefLogIdent(refLogIdent);
     branchUpdate.setForceUpdate(false);

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

@@ -43,7 +43,6 @@ import org.eclipse.jgit.errors.LargeObjectException;
 import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.errors.NoMergeBaseException;
 import org.eclipse.jgit.errors.NoMergeBaseException.MergeBaseFailureReason;
-import org.eclipse.jgit.lib.AnyObjectId;
 import org.eclipse.jgit.lib.CommitBuilder;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.Constants;
@@ -67,7 +66,6 @@ import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.io.InputStream;
-import java.io.UnsupportedEncodingException;
 import java.sql.Timestamp;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -79,6 +77,12 @@ import java.util.List;
 import java.util.Set;
 import java.util.TimeZone;
 
+/**
+ * Utilities for various kinds of merges and cherry-picks.
+ * <p>
+ * <b>Note:</b> Unless otherwise noted, the methods in this class do not flush
+ * the {@link ObjectInserter}s passed in after performing a merge.
+ */
 public class MergeUtil {
   private static final Logger log = LoggerFactory.getLogger(MergeUtil.class);
   private static final String R_HEADS_MASTER =
@@ -177,7 +181,7 @@ public class MergeUtil {
     final ThreeWayMerger m = newThreeWayMerger(repo, inserter);
 
     m.setBase(originalCommit.getParent(0));
-    if (m.merge(mergeTip, originalCommit)) {
+    if (m.merge(false, mergeTip, originalCommit)) {
       ObjectId tree = m.getResultTreeId();
       if (tree.equals(mergeTip.getTree())) {
         throw new MergeIdenticalTreeException("identical tree");
@@ -189,7 +193,7 @@ public class MergeUtil {
       mergeCommit.setAuthor(originalCommit.getAuthorIdent());
       mergeCommit.setCommitter(cherryPickCommitterIdent);
       mergeCommit.setMessage(commitMsg);
-      return rw.parseCommit(commit(inserter, mergeCommit));
+      return rw.parseCommit(inserter.insert(mergeCommit));
     } else {
       throw new MergeConflictException("merge conflict");
     }
@@ -393,7 +397,7 @@ public class MergeUtil {
 
     ThreeWayMerger m = newThreeWayMerger(repo, createDryRunInserter(repo));
     try {
-      return m.merge(new AnyObjectId[] {mergeTip, toMerge});
+      return m.merge(false, mergeTip, toMerge);
     } catch (LargeObjectException e) {
       log.warn("Cannot merge due to LargeObjectException: " + toMerge.name());
       return false;
@@ -442,7 +446,7 @@ public class MergeUtil {
       try {
         ThreeWayMerger m = newThreeWayMerger(repo, createDryRunInserter(repo));
         m.setBase(toMerge.getParent(0));
-        return m.merge(mergeTip, toMerge);
+        return m.merge(false, mergeTip, toMerge);
       } catch (IOException e) {
         throw new MergeException("Cannot merge " + toMerge.name(), e);
       }
@@ -494,7 +498,7 @@ public class MergeUtil {
       throws MergeException {
     final ThreeWayMerger m = newThreeWayMerger(repo, inserter);
     try {
-      if (m.merge(new AnyObjectId[] {mergeTip, n})) {
+      if (m.merge(false, mergeTip, n)) {
         return writeMergeCommit(myIdent, rw, inserter, canMergeFlag, destBranch,
             mergeTip, m.getResultTreeId(), n);
       } else {
@@ -582,7 +586,7 @@ public class MergeUtil {
     mergeCommit.setMessage(msgbuf.toString());
 
     CodeReviewCommit mergeResult =
-        (CodeReviewCommit) rw.parseCommit(commit(inserter, mergeCommit));
+        (CodeReviewCommit) rw.parseCommit(inserter.insert(mergeCommit));
     mergeResult.setControl(n.getControl());
     return mergeResult;
   }
@@ -656,31 +660,10 @@ public class MergeUtil {
     Merger m = strategy.newMerger(repo, true);
     checkArgument(m instanceof ThreeWayMerger,
         "merge strategy %s does not support three-way merging", strategyName);
-    m.setObjectInserter(new ObjectInserter.Filter() {
+    m.setObjectInserter(inserter);
-      @Override
-      protected ObjectInserter delegate() {
-        return inserter;
-      }
-
-      @Override
-      public void flush() {
-      }
-
-      @Override
-      public void release() {
-      }
-    });
     return (ThreeWayMerger) m;
   }
 
-  public ObjectId commit(final ObjectInserter inserter,
-      final CommitBuilder mergeCommit) throws IOException,
-      UnsupportedEncodingException {
-    ObjectId id = inserter.insert(mergeCommit);
-    inserter.flush();
-    return id;
-  }
-
   public PatchSetApproval markCleanMerges(final RevWalk rw,
       final RevFlag canMergeFlag, final CodeReviewCommit mergeTip,
       final Set<RevCommit> alreadyAccepted) throws MergeException {

gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java

@@ -275,24 +275,11 @@ public class PatchListLoader extends CacheLoader<PatchListKey, PatchList> {
     try {
       DirCache dc = DirCache.newInCore();
       m.setDirCache(dc);
-      m.setObjectInserter(new ObjectInserter.Filter() {
+      m.setObjectInserter(ins);
-        @Override
-        protected ObjectInserter delegate() {
-          return ins;
-        }
-
-        @Override
-        public void flush() {
-        }
-
-        @Override
-        public void release() {
-        }
-      });
 
       boolean couldMerge;
       try {
-        couldMerge = m.merge(b.getParents());
+        couldMerge = m.merge(false, b.getParents());
       } catch (IOException e) {
         // It is not safe to continue further down in this method as throwing
         // an exception most likely means that the merge tree was not created