EventFactory: Use repo instead of PatchSetAncestors for parents

The parent revisions of a patch set are just the parents of the
corresponding commit, so getting these from the repo requires parsing
only a single commit. Using the database allows us to avoid opening
the repo, but this is a negligible performance difference as repos are
generally cached.

Change-Id: I862bb10ec53ca6d9fba39fe35cbbda9102c7a77b

gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java

@@ -36,6 +36,7 @@ import com.google.gerrit.server.config.AnonymousCowardName;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.config.SitePaths;
 import com.google.gerrit.server.data.ApprovalAttribute;
+import com.google.gerrit.server.data.PatchSetAttribute;
 import com.google.gerrit.server.events.ChangeAbandonedEvent;
 import com.google.gerrit.server.events.ChangeMergedEvent;
 import com.google.gerrit.server.events.ChangeRestoredEvent;
@@ -62,6 +63,7 @@ import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.RefUpdate;
 import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevWalk;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -334,6 +336,16 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
       }
     }
 
+    private PatchSetAttribute asPatchSetAttribute(Change change,
+        PatchSet patchSet, ReviewDb db) throws OrmException {
+      try (Repository repo = repoManager.openRepository(change.getProject());
+          RevWalk revWalk = new RevWalk(repo)) {
+        return eventFactory.asPatchSetAttribute(db, revWalk, patchSet);
+      } catch (IOException e) {
+        throw new OrmException(e);
+      }
+    }
+
     /**
      * Fire the update hook
      *
@@ -381,7 +393,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
       AccountState owner = accountCache.get(change.getOwner());
 
       event.change = eventFactory.asChangeAttribute(db, change);
-      event.patchSet = eventFactory.asPatchSetAttribute(db, patchSet);
+      event.patchSet = asPatchSetAttribute(change, patchSet, db);
       event.uploader = eventFactory.asAccountAttribute(uploader.getAccount());
       fireEvent(change, event, db);
 
@@ -409,7 +421,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
       AccountState owner = accountCache.get(change.getOwner());
 
       event.change = eventFactory.asChangeAttribute(db, change);
-      event.patchSet = eventFactory.asPatchSetAttribute(db, patchSet);
+      event.patchSet = asPatchSetAttribute(change, patchSet, db);
       event.uploader = eventFactory.asAccountAttribute(uploader.getAccount());
       fireEvent(change, event, db);
 
@@ -436,7 +448,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
 
       event.change = eventFactory.asChangeAttribute(db, change);
       event.author =  eventFactory.asAccountAttribute(account);
-      event.patchSet = eventFactory.asPatchSetAttribute(db, patchSet);
+      event.patchSet = asPatchSetAttribute(change, patchSet, db);
       event.comment = comment;
 
       LabelTypes labelTypes = projectCache.get(change.getProject()).getLabelTypes();
@@ -480,7 +492,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
 
       event.change = eventFactory.asChangeAttribute(db, change);
       event.submitter = eventFactory.asAccountAttribute(account);
-      event.patchSet = eventFactory.asPatchSetAttribute(db, patchSet);
+      event.patchSet = asPatchSetAttribute(change, patchSet, db);
       event.newRev = mergeResultRev;
       fireEvent(change, event, db);
 
@@ -507,7 +519,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
 
       event.change = eventFactory.asChangeAttribute(db, change);
       event.submitter = eventFactory.asAccountAttribute(account);
-      event.patchSet = eventFactory.asPatchSetAttribute(db, patchSet);
+      event.patchSet = asPatchSetAttribute(change, patchSet, db);
       event.reason = reason;
       fireEvent(change, event, db);
 
@@ -534,7 +546,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
 
       event.change = eventFactory.asChangeAttribute(db, change);
       event.abandoner = eventFactory.asAccountAttribute(account);
-      event.patchSet = eventFactory.asPatchSetAttribute(db, patchSet);
+      event.patchSet = asPatchSetAttribute(change, patchSet, db);
       event.reason = reason;
       fireEvent(change, event, db);
 
@@ -561,7 +573,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
 
       event.change = eventFactory.asChangeAttribute(db, change);
       event.restorer = eventFactory.asAccountAttribute(account);
-      event.patchSet = eventFactory.asPatchSetAttribute(db, patchSet);
+      event.patchSet = asPatchSetAttribute(change, patchSet, db);
       event.reason = reason;
       fireEvent(change, event, db);
 
@@ -616,7 +628,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
       AccountState owner = accountCache.get(change.getOwner());
 
       event.change = eventFactory.asChangeAttribute(db, change);
-      event.patchSet = eventFactory.asPatchSetAttribute(db, patchSet);
+      event.patchSet = asPatchSetAttribute(change, patchSet, db);
       event.reviewer = eventFactory.asAccountAttribute(account);
       fireEvent(change, event, db);
 

gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java

@@ -31,7 +31,6 @@ import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.reviewdb.client.Patch;
 import com.google.gerrit.reviewdb.client.PatchLineComment;
 import com.google.gerrit.reviewdb.client.PatchSet;
-import com.google.gerrit.reviewdb.client.PatchSetAncestor;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.UserIdentity;
 import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -352,21 +351,21 @@ public class EventFactory {
     a.commitMessage = commitMessage;
   }
 
-  public void addPatchSets(ReviewDb db, ChangeAttribute ca,
+  public void addPatchSets(ReviewDb db, RevWalk revWalk, ChangeAttribute ca,
       Collection<PatchSet> ps,
       Map<PatchSet.Id, Collection<PatchSetApproval>> approvals,
       LabelTypes labelTypes) {
-    addPatchSets(db, ca, ps, approvals, false, null, labelTypes);
+    addPatchSets(db, revWalk, ca, ps, approvals, false, null, labelTypes);
   }
 
-  public void addPatchSets(ReviewDb db, ChangeAttribute ca,
+  public void addPatchSets(ReviewDb db, RevWalk revWalk, ChangeAttribute ca,
       Collection<PatchSet> ps,
       Map<PatchSet.Id, Collection<PatchSetApproval>> approvals,
       boolean includeFiles, Change change, LabelTypes labelTypes) {
     if (!ps.isEmpty()) {
       ca.patchSets = new ArrayList<>(ps.size());
       for (PatchSet p : ps) {
-        PatchSetAttribute psa = asPatchSetAttribute(db, p);
+        PatchSetAttribute psa = asPatchSetAttribute(db, revWalk, p);
         if (approvals != null) {
           addApprovals(psa, p.getId(), approvals, labelTypes);
         }
@@ -430,7 +429,8 @@ public class EventFactory {
    * @param patchSet
    * @return object suitable for serialization to JSON
    */
-  public PatchSetAttribute asPatchSetAttribute(ReviewDb db, PatchSet patchSet) {
+  public PatchSetAttribute asPatchSetAttribute(ReviewDb db, RevWalk revWalk,
+      PatchSet patchSet) {
     PatchSetAttribute p = new PatchSetAttribute();
     p.revision = patchSet.getRevision().get();
     p.number = Integer.toString(patchSet.getPatchSetId());
@@ -441,9 +441,9 @@ public class EventFactory {
     PatchSet.Id pId = patchSet.getId();
     try {
       p.parents = new ArrayList<>();
-      for (PatchSetAncestor a : db.patchSetAncestors().ancestorsOf(
+      RevCommit c = revWalk.parseCommit(ObjectId.fromString(p.revision));
-          patchSet.getId())) {
+      for (RevCommit parent : c.getParents()) {
-        p.parents.add(a.getAncestorRevision().get());
+        p.parents.add(parent.name());
       }
 
       UserIdentity author = psInfoFactory.get(db, pId).getAuthor();
@@ -466,7 +466,7 @@ public class EventFactory {
         }
       }
       p.kind = changeKindCache.getChangeKind(db, change, patchSet);
-    } catch (OrmException e) {
+    } catch (OrmException | IOException e) {
       log.error("Cannot load patch set data for " + patchSet.getId(), e);
     } catch (PatchSetInfoNotAvailableException e) {
       log.error(String.format("Cannot get authorEmail for %s.", pId), e);

gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java

@@ -194,12 +194,8 @@ public class OutputStreamQuery {
         final QueryStatsAttribute stats = new QueryStatsAttribute();
         stats.runTimeMilliseconds = TimeUtil.nowMs();
 
-        Map<Project.NameKey, Repository> repos = null;
+        Map<Project.NameKey, Repository> repos = new HashMap<>();
-        Map<Project.NameKey, RevWalk> revWalks = null;
+        Map<Project.NameKey, RevWalk> revWalks = new HashMap<>();
-        if (includeDependencies) {
-          repos = new HashMap<>();
-          revWalks = new HashMap<>();
-        }
         QueryResult results =
             queryProcessor.queryChanges(queryBuilder.parse(queryString));
         try {
@@ -266,13 +262,26 @@ public class OutputStreamQuery {
       eventFactory.addCommitMessage(c, d.commitMessage());
     }
 
+    RevWalk rw = null;
+    if (includePatchSets || includeCurrentPatchSet || includeDependencies) {
+      Project.NameKey p = d.change().getProject();
+      rw = revWalks.get(p);
+      // Cache and reuse repos and revwalks.
+      if (rw == null) {
+        Repository repo = repoManager.openRepository(p);
+        checkState(repos.put(p, repo) == null);
+        rw = new RevWalk(repo);
+        revWalks.put(p, rw);
+      }
+    }
+
     if (includePatchSets) {
       if (includeFiles) {
-        eventFactory.addPatchSets(db.get(), c, d.patchSets(),
+        eventFactory.addPatchSets(db.get(), rw, c, d.patchSets(),
           includeApprovals ? d.approvals().asMap() : null,
           includeFiles, d.change(), labelTypes);
       } else {
-        eventFactory.addPatchSets(db.get(), c, d.patchSets(),
+        eventFactory.addPatchSets(db.get(), rw, c, d.patchSets(),
             includeApprovals ? d.approvals().asMap() : null,
             labelTypes);
       }
@@ -282,7 +291,7 @@ public class OutputStreamQuery {
       PatchSet current = d.currentPatchSet();
       if (current != null) {
         c.currentPatchSet =
-            eventFactory.asPatchSetAttribute(db.get(), current);
+            eventFactory.asPatchSetAttribute(db.get(), rw, current);
         eventFactory.addApprovals(c.currentPatchSet,
             d.currentApprovals(), labelTypes);
 
@@ -304,15 +313,6 @@ public class OutputStreamQuery {
     }
 
     if (includeDependencies) {
-      Project.NameKey p = d.change().getProject();
-      RevWalk rw = revWalks.get(p);
-      // Cache and reuse repos and revwalks.
-      if (rw == null) {
-        Repository repo = repoManager.openRepository(p);
-        checkState(repos.put(p, repo) == null);
-        rw = new RevWalk(repo);
-        revWalks.put(p, rw);
-      }
       eventFactory.addDependencies(rw, c, d.change(), d.currentPatchSet());
     }