MergeSuperSet: Cache open repositories

completeChangeSetWithoutTopic was trying to be nice by grouping
changes by project to cut down on opening repositories multiple times.
However, it would still open repos multiple times across multiple
iterations of the main completeChangeSet loop.

Reuse the existing MergeOpRepoManager, since this code is already
called in the MergeOp path where we have one available. As it happens,
the options (particularly the sort) that are already set in
MergeOpRepoManager are more or less what we need here.

Change-Id: Icdc78daab3290f5a39dba0e5a162d51ec93ca00a

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

@@ -412,7 +412,8 @@ public class MergeOp implements AutoCloseable {
 
     logDebug("Beginning integration of {}", change);
     try {
-      ChangeSet cs = mergeSuperSet.completeChangeSet(db, change, caller);
+      ChangeSet cs = mergeSuperSet.setMergeOpRepoManager(orm)
+          .completeChangeSet(db, change, caller);
       checkState(cs.ids().contains(change.getId()),
           "change %s missing from %s", change.getId(), cs);
       if (cs.furtherHiddenChanges()) {

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

@@ -14,13 +14,12 @@
 
 package com.google.gerrit.server.git;
 
+import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 
 import com.google.common.base.Strings;
-import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
-import com.google.common.collect.Multimap;
 import com.google.gerrit.common.data.SubmitTypeRecord;
 import com.google.gerrit.extensions.client.SubmitType;
 import com.google.gerrit.reviewdb.client.Branch;
@@ -31,8 +30,10 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.change.Submit;
 import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.git.MergeOpRepoManager.OpenRepo;
 import com.google.gerrit.server.index.change.ChangeField;
 import com.google.gerrit.server.project.NoSuchChangeException;
+import com.google.gerrit.server.project.NoSuchProjectException;
 import com.google.gerrit.server.project.SubmitRuleEvaluator;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.query.change.InternalChangeQuery;
@@ -40,15 +41,11 @@ import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 
-import org.eclipse.jgit.errors.IncorrectObjectTypeException;
-import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Ref;
-import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevSort;
-import org.eclipse.jgit.revwalk.RevWalk;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -82,41 +79,47 @@ public class MergeSuperSet {
 
   private final ChangeData.Factory changeDataFactory;
   private final Provider<InternalChangeQuery> queryProvider;
-  private final GitRepositoryManager repoManager;
+  private final Provider<MergeOpRepoManager> repoManagerProvider;
   private final Config cfg;
 
+  private MergeOpRepoManager orm;
+  private boolean closeOrm;
+
   @Inject
   MergeSuperSet(@GerritServerConfig Config cfg,
       ChangeData.Factory changeDataFactory,
       Provider<InternalChangeQuery> queryProvider,
-      GitRepositoryManager repoManager) {
+      Provider<MergeOpRepoManager> repoManagerProvider) {
     this.cfg = cfg;
     this.changeDataFactory = changeDataFactory;
     this.queryProvider = queryProvider;
-    this.repoManager = repoManager;
+    this.repoManagerProvider = repoManagerProvider;
   }
 
-  public ChangeSet completeChangeSet(ReviewDb db, Change change, CurrentUser user)
+  public MergeSuperSet setMergeOpRepoManager(MergeOpRepoManager orm) {
-      throws MissingObjectException, IncorrectObjectTypeException, IOException,
+    checkState(this.orm == null);
-      OrmException {
+    this.orm = checkNotNull(orm);
-    ChangeData cd =
+    closeOrm = false;
-        changeDataFactory.create(db, change.getProject(), change.getId());
+    return this;
-    cd.changeControl(user);
-    ChangeSet cs = new ChangeSet(cd, cd.changeControl().isVisible(db, cd));
-    if (Submit.wholeTopicEnabled(cfg)) {
-      return completeChangeSetIncludingTopics(db, cs, user);
-    }
-    return completeChangeSetWithoutTopic(db, cs, user);
   }
 
-  private static ImmutableListMultimap<Project.NameKey, ChangeData>
+  public ChangeSet completeChangeSet(ReviewDb db, Change change,
-      byProject(Iterable<ChangeData> changes) throws OrmException {
+      CurrentUser user) throws IOException, OrmException {
-    ImmutableListMultimap.Builder<Project.NameKey, ChangeData> builder =
+    try {
-        new ImmutableListMultimap.Builder<>();
+      ChangeData cd =
-    for (ChangeData cd : changes) {
+          changeDataFactory.create(db, change.getProject(), change.getId());
-      builder.put(cd.change().getProject(), cd);
+      cd.changeControl(user);
+      ChangeSet cs = new ChangeSet(cd, cd.changeControl().isVisible(db, cd));
+      if (Submit.wholeTopicEnabled(cfg)) {
+        return completeChangeSetIncludingTopics(db, cs, user);
+      }
+      return completeChangeSetWithoutTopic(db, cs, user);
+    } finally {
+      if (closeOrm && orm != null) {
+        orm.close();
+        orm = null;
+      }
     }
-    return builder.build();
   }
 
   private SubmitType submitType(ChangeData cd, PatchSet ps, boolean visible)
@@ -146,87 +149,78 @@ public class MergeSuperSet {
     return str.type;
   }
 
-  private ChangeSet completeChangeSetWithoutTopic(ReviewDb db, ChangeSet changes,
+  private ChangeSet completeChangeSetWithoutTopic(ReviewDb db,
-      CurrentUser user) throws MissingObjectException,
+      ChangeSet changes, CurrentUser user) throws IOException, OrmException {
-      IncorrectObjectTypeException, IOException, OrmException {
     List<ChangeData> visibleChanges = new ArrayList<>();
     List<ChangeData> nonVisibleChanges = new ArrayList<>();
 
-    Multimap<Project.NameKey, ChangeData> pc =
+    for (ChangeData cd :
-        byProject(
+        Iterables.concat(changes.changes(), changes.nonVisibleChanges())) {
-            Iterables.concat(changes.changes(), changes.nonVisibleChanges()));
+      checkState(cd.hasChangeControl(),
-    for (Project.NameKey project : pc.keySet()) {
+          "completeChangeSet forgot to set changeControl for current user"
-      try (Repository repo = repoManager.openRepository(project);
+          + " at ChangeData creation time");
-           RevWalk rw = CodeReviewCommit.newRevWalk(repo)) {
+      OpenRepo or = getRepo(cd.change().getProject());
-        for (ChangeData cd : pc.get(project)) {
+      boolean visible = changes.ids().contains(cd.getId());
-          checkState(cd.hasChangeControl(),
+      if (visible && !cd.changeControl().isVisible(db, cd)) {
-              "completeChangeSet forgot to set changeControl for current user"
+        // We thought the change was visible, but it isn't.
-              + " at ChangeData creation time");
+        // This can happen if the ACL changes during the
-          boolean visible = changes.ids().contains(cd.getId());
+        // completeChangeSet computation, for example.
-          if (visible && !cd.changeControl().isVisible(db, cd)) {
+        visible = false;
-            // We thought the change was visible, but it isn't.
+      }
-            // This can happen if the ACL changes during the
+      List<ChangeData> dest = visible ? visibleChanges : nonVisibleChanges;
-            // completeChangeSet computation, for example.
-            visible = false;
-          }
-          List<ChangeData> dest = visible ? visibleChanges : nonVisibleChanges;
 
-          // Pick a revision to use for traversal.  If any of the patch sets
+      // Pick a revision to use for traversal.  If any of the patch sets
-          // is visible, we use the most recent one.  Otherwise, use the current
+      // is visible, we use the most recent one.  Otherwise, use the current
-          // patch set.
+      // patch set.
-          PatchSet ps = cd.currentPatchSet();
+      PatchSet ps = cd.currentPatchSet();
-          boolean visiblePatchSet = visible;
+      boolean visiblePatchSet = visible;
-          if (!cd.changeControl().isPatchVisible(ps, cd)) {
+      if (!cd.changeControl().isPatchVisible(ps, cd)) {
-            Iterable<PatchSet> visiblePatchSets = cd.visiblePatchSets();
+        Iterable<PatchSet> visiblePatchSets = cd.visiblePatchSets();
-            if (Iterables.isEmpty(visiblePatchSets)) {
+        if (Iterables.isEmpty(visiblePatchSets)) {
-              visiblePatchSet = false;
+          visiblePatchSet = false;
-            } else {
+        } else {
-              ps = Iterables.getLast(visiblePatchSets);
+          ps = Iterables.getLast(visiblePatchSets);
-            }
+        }
-          }
+      }
 
-          if (submitType(cd, ps, visiblePatchSet) == SubmitType.CHERRY_PICK) {
+      if (submitType(cd, ps, visiblePatchSet) == SubmitType.CHERRY_PICK) {
-            dest.add(cd);
+        dest.add(cd);
-            continue;
+        continue;
-          }
+      }
 
-          // Get the underlying git commit object
+      // Get the underlying git commit object
-          String objIdStr = ps.getRevision().get();
+      String objIdStr = ps.getRevision().get();
-          RevCommit commit = rw.parseCommit(ObjectId.fromString(objIdStr));
+      RevCommit commit = or.rw.parseCommit(ObjectId.fromString(objIdStr));
 
-          // Collect unmerged ancestors
+      // Collect unmerged ancestors
-          Branch.NameKey destBranch = cd.change().getDest();
+      Branch.NameKey destBranch = cd.change().getDest();
-          repo.getRefDatabase().refresh();
+      Ref ref = or.repo.getRefDatabase().getRef(destBranch.get());
-          Ref ref = repo.getRefDatabase().getRef(destBranch.get());
 
-          rw.reset();
+      or.rw.reset();
-          rw.sort(RevSort.TOPO);
+      or.rw.markStart(commit);
-          rw.markStart(commit);
+      if (ref != null) {
-          if (ref != null) {
+        RevCommit head = or.rw.parseCommit(ref.getObjectId());
-            RevCommit head = rw.parseCommit(ref.getObjectId());
+        or.rw.markUninteresting(head);
-            rw.markUninteresting(head);
+      }
-          }
 
-          List<String> hashes = new ArrayList<>();
+      List<String> hashes = new ArrayList<>();
-          // Always include the input, even if merged. This allows
+      // Always include the input, even if merged. This allows
-          // SubmitStrategyOp to correct the situation later, assuming it gets
+      // SubmitStrategyOp to correct the situation later, assuming it gets
-          // returned by byCommitsOnBranchNotMerged below.
+      // returned by byCommitsOnBranchNotMerged below.
-          hashes.add(objIdStr);
+      hashes.add(objIdStr);
-          for (RevCommit c : rw) {
+      for (RevCommit c : or.rw) {
-            if (!c.equals(commit)) {
+        if (!c.equals(commit)) {
-              hashes.add(c.name());
+          hashes.add(c.name());
-            }
+        }
-          }
+      }
 
-          if (!hashes.isEmpty()) {
+      if (!hashes.isEmpty()) {
-            Iterable<ChangeData> destChanges = query()
+        Iterable<ChangeData> destChanges = query()
-                .byCommitsOnBranchNotMerged(
+            .byCommitsOnBranchNotMerged(
-                  repo, db, cd.change().getDest(), hashes);
+              or.repo, db, cd.change().getDest(), hashes);
-            for (ChangeData chd : destChanges) {
+        for (ChangeData chd : destChanges) {
-              chd.changeControl(user);
+          chd.changeControl(user);
-              dest.add(chd);
+          dest.add(chd);
-            }
-          }
         }
       }
     }
@@ -234,6 +228,20 @@ public class MergeSuperSet {
     return new ChangeSet(visibleChanges, nonVisibleChanges);
   }
 
+  private OpenRepo getRepo(Project.NameKey project) throws IOException {
+    if (orm == null) {
+      orm = repoManagerProvider.get();
+      closeOrm = true;
+    }
+    try {
+      OpenRepo or = orm.openRepo(project);
+      checkState(or.rw.hasRevSort(RevSort.TOPO));
+      return or;
+    } catch (NoSuchProjectException e) {
+      throw new IOException(e);
+    }
+  }
+
   /**
    * Completes {@code cs} with any additional changes from its topics
    * <p>
@@ -296,8 +304,7 @@ public class MergeSuperSet {
 
   private ChangeSet completeChangeSetIncludingTopics(
       ReviewDb db, ChangeSet changes, CurrentUser user)
-      throws MissingObjectException, IncorrectObjectTypeException, IOException,
+      throws IOException, OrmException {
-      OrmException {
     Set<String> topicsSeen = new HashSet<>();
     Set<String> visibleTopicsSeen = new HashSet<>();
     int oldSeen;