Cache ChangeKind computations

We want to be able to evaluate copyAllScoresOnTrivialRebase and
copyAllScoresIfNoCodeChange at submit rule evaluation time based on
the current value for each label, not what it happened to be at the
time a new patch set is created. This computation is potentially
expensive, so cache it.

Keys contain the SHA-1 of a patch set, and the merge strategy name,
since the triviality of a rebase may depend on the strategy used.

Since this functionality is not yet implemented, don't persist any
data in memory or on disk yet.

Change-Id: Ica7a7a52ecc317c710bdac3de41bd98c46465b05

gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java

@@ -27,7 +27,7 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.PatchSetApproval.LabelId;
 import com.google.gerrit.reviewdb.client.PatchSetInfo;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.change.PatchSetInserter.ChangeKind;
+import com.google.gerrit.server.change.ChangeKind;
 import com.google.gerrit.server.util.TimeUtil;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;

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

@@ -0,0 +1,27 @@
+// Copyright (C) 2013 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.change;
+
+/** Operation performed by a change relative to its parent. */
+public enum ChangeKind {
+  /** Nontrivial content changes. */
+  REWORK,
+
+  /** Conflict-free merge between the new parent and the prior patch set. */
+  TRIVIAL_REBASE,
+
+  /** Same tree and same parents. */
+  NO_CODE_CHANGE;
+}

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

@@ -0,0 +1,195 @@
+// Copyright (C) 2013 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.change;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+import com.google.common.base.Objects;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+import com.google.gerrit.server.cache.CacheModule;
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.git.MergeUtil;
+import com.google.gerrit.server.project.ProjectState;
+import com.google.inject.Inject;
+import com.google.inject.Module;
+import com.google.inject.Singleton;
+import com.google.inject.name.Named;
+
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.merge.ThreeWayMerger;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.io.Serializable;
+import java.util.concurrent.ExecutionException;
+
+/**
+ * Cache of {@link ChangeKind} per commit.
+ * <p>
+ * This is immutable conditioned on the merge strategy (unless the JGit strategy
+ * implementation changes, which might invalidate old entries).
+ */
+public class ChangeKindCache {
+  private static final Logger log =
+      LoggerFactory.getLogger(ChangeKindCache.class);
+
+  private static final String ID_CACHE = "change_kind";
+
+  public static Module module() {
+    return new CacheModule() {
+      @Override
+      protected void configure() {
+        cache(ID_CACHE,
+            Key.class,
+            ChangeKind.class)
+          .maximumWeight(0)
+          .loader(Loader.class);
+      }
+    };
+  }
+
+  public static class Key implements Serializable {
+    private static final long serialVersionUID = 1L;
+
+    private final ObjectId prior;
+    private final ObjectId next;
+    private final String strategyName;
+    private transient Repository repo;
+
+    private Key(ObjectId prior, ObjectId next, String strategyName,
+        Repository repo) {
+      this.prior = prior.copy();
+      this.next = next.copy();
+      this.strategyName = strategyName;
+      this.repo = repo;
+    }
+
+    public ObjectId getPrior() {
+      return prior;
+    }
+
+    public ObjectId getNext() {
+      return next;
+    }
+
+    public String getStrategyName() {
+      return strategyName;
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      if (o instanceof Key) {
+        Key k = (Key) o;
+        return Objects.equal(prior, k.prior)
+            && Objects.equal(next, k.next)
+            && Objects.equal(strategyName, k.strategyName);
+      }
+      return false;
+    }
+
+    @Override
+    public int hashCode() {
+      return Objects.hashCode(prior, next, strategyName);
+    }
+  }
+
+  @Singleton
+  private static class Loader extends CacheLoader<Key, ChangeKind> {
+    @Override
+    public ChangeKind load(Key key) throws IOException {
+      RevWalk walk = new RevWalk(key.repo);
+      try {
+        RevCommit prior = walk.parseCommit(key.prior);
+        walk.parseBody(prior);
+        RevCommit next = walk.parseCommit(key.next);
+        walk.parseBody(next);
+
+        if (!next.getFullMessage().equals(prior.getFullMessage())) {
+          if (next.getTree() == prior.getTree() && isSameParents(prior, next)) {
+            return ChangeKind.NO_CODE_CHANGE;
+          } else {
+            return ChangeKind.REWORK;
+          }
+        }
+
+        if (prior.getParentCount() != 1 || next.getParentCount() != 1) {
+          // Trivial rebases done by machine only work well on 1 parent.
+          return ChangeKind.REWORK;
+        }
+
+        if (next.getTree() == prior.getTree() &&
+           isSameParents(prior, next)) {
+          return ChangeKind.TRIVIAL_REBASE;
+        }
+
+        // A trivial rebase can be detected by looking for the next commit
+        // having the same tree as would exist when the prior commit is
+        // cherry-picked onto the next commit's new first parent.
+        ThreeWayMerger merger = MergeUtil.newThreeWayMerger(
+            key.repo, MergeUtil.createDryRunInserter(), key.strategyName);
+        merger.setBase(prior.getParent(0));
+        if (merger.merge(next.getParent(0), prior)
+            && merger.getResultTreeId().equals(next.getTree())) {
+          return ChangeKind.TRIVIAL_REBASE;
+        } else {
+          return ChangeKind.REWORK;
+        }
+      } finally {
+        key.repo = null;
+        walk.release();
+      }
+    }
+
+    private static boolean isSameParents(RevCommit prior, RevCommit next) {
+      if (prior.getParentCount() != next.getParentCount()) {
+        return false;
+      } else if (prior.getParentCount() == 0) {
+        return true;
+      }
+      return prior.getParent(0).equals(next.getParent(0));
+    }
+  }
+
+  private final LoadingCache<Key, ChangeKind> cache;
+  private final boolean useRecursiveMerge;
+
+  @Inject
+  ChangeKindCache(
+      @GerritServerConfig Config serverConfig,
+      @Named(ID_CACHE) LoadingCache<Key, ChangeKind> cache) {
+    this.cache = cache;
+    this.useRecursiveMerge = MergeUtil.useRecursiveMerge(serverConfig);
+  }
+
+  public ChangeKind getChangeKind(ProjectState project, Repository repo,
+      ObjectId prior, ObjectId next) {
+    checkNotNull(next, "next");
+    String strategyName = MergeUtil.mergeStrategyName(
+        project.isUseContentMerge(), useRecursiveMerge);
+    try {
+      return cache.get(new Key(prior, next, strategyName, repo));
+    } catch (ExecutionException e) {
+      log.warn("Cannot check trivial rebase of new patch set " + next.name()
+          + " in " + project.getProject().getName(), e);
+      return ChangeKind.REWORK;
+    }
+  }
+}

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

@@ -33,7 +33,6 @@ import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.events.CommitReceivedEvent;
 import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
-import com.google.gerrit.server.git.MergeUtil;
 import com.google.gerrit.server.git.validators.CommitValidationException;
 import com.google.gerrit.server.git.validators.CommitValidators;
 import com.google.gerrit.server.mail.ReplacePatchSetSender;
@@ -52,7 +51,6 @@ import com.google.inject.assistedinject.Assisted;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.RefUpdate;
 import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.merge.ThreeWayMerger;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.transport.ReceiveCommand;
@@ -82,10 +80,6 @@ public class PatchSetInserter {
     GERRIT, RECEIVE_COMMITS, NONE
   }
 
-  public static enum ChangeKind {
-    REWORK, TRIVIAL_REBASE, NO_CODE_CHANGE
-  }
-
   private final ChangeHooks hooks;
   private final PatchSetInfoFactory patchSetInfoFactory;
   private final ReviewDb db;
@@ -94,8 +88,8 @@ public class PatchSetInserter {
   private final CommitValidators.Factory commitValidatorsFactory;
   private final MergeabilityChecker mergeabilityChecker;
   private final ReplacePatchSetSender.Factory replacePatchSetFactory;
-  private final MergeUtil.Factory mergeUtilFactory;
   private final ApprovalsUtil approvalsUtil;
+  private final ChangeKindCache changeKindCache;
 
   private final Repository git;
   private final RevWalk revWalk;
@@ -120,8 +114,8 @@ public class PatchSetInserter {
       CommitValidators.Factory commitValidatorsFactory,
       MergeabilityChecker mergeabilityChecker,
       ReplacePatchSetSender.Factory replacePatchSetFactory,
-      MergeUtil.Factory mergeUtilFactory,
       ApprovalsUtil approvalsUtil,
+      ChangeKindCache changeKindCache,
       @Assisted Repository git,
       @Assisted RevWalk revWalk,
       @Assisted RefControl refControl,
@@ -136,8 +130,8 @@ public class PatchSetInserter {
     this.commitValidatorsFactory = commitValidatorsFactory;
     this.mergeabilityChecker = mergeabilityChecker;
     this.replacePatchSetFactory = replacePatchSetFactory;
-    this.mergeUtilFactory = mergeUtilFactory;
     this.approvalsUtil = approvalsUtil;
+    this.changeKindCache = changeKindCache;
 
     this.git = git;
     this.revWalk = revWalk;
@@ -279,8 +273,8 @@ public class PatchSetInserter {
         RevCommit priorCommit = revWalk.parseCommit(priorCommitId);
         ProjectState projectState =
             refControl.getProjectControl().getProjectState();
-        ChangeKind changeKind =
-            getChangeKind(mergeUtilFactory, projectState, git, priorCommit, commit);
+        ChangeKind changeKind = changeKindCache.getChangeKind(
+            projectState, git, priorCommit, commit);
 
         approvalsUtil.copyLabels(db, refControl.getProjectControl()
             .getLabelTypes(), currentPatchSetId, patchSet, changeKind);
@@ -362,56 +356,6 @@ public class PatchSetInserter {
     }
   }
 
-  public static ChangeKind getChangeKind(MergeUtil.Factory mergeUtilFactory, ProjectState project,
-      Repository git, RevCommit prior, RevCommit next) {
-    if (!next.getFullMessage().equals(prior.getFullMessage())) {
-      if (next.getTree() == prior.getTree() && isSameParents(prior, next)) {
-        return ChangeKind.NO_CODE_CHANGE;
-      } else {
-        return ChangeKind.REWORK;
-      }
-    }
-
-    if (prior.getParentCount() != 1 || next.getParentCount() != 1) {
-      // Trivial rebases done by machine only work well on 1 parent.
-      return ChangeKind.REWORK;
-    }
-
-    if (next.getTree() == prior.getTree() &&
-       isSameParents(prior, next)) {
-      return ChangeKind.TRIVIAL_REBASE;
-    }
-
-    // A trivial rebase can be detected by looking for the next commit
-    // having the same tree as would exist when the prior commit is
-    // cherry-picked onto the next commit's new first parent.
-    try {
-      MergeUtil mergeUtil = mergeUtilFactory.create(project);
-      ThreeWayMerger merger =
-          mergeUtil.newThreeWayMerger(git, mergeUtil.createDryRunInserter());
-      merger.setBase(prior.getParent(0));
-      if (merger.merge(next.getParent(0), prior)
-          && merger.getResultTreeId().equals(next.getTree())) {
-        return ChangeKind.TRIVIAL_REBASE;
-      } else {
-        return ChangeKind.REWORK;
-      }
-    } catch (IOException err) {
-      log.warn("Cannot check trivial rebase of new patch set " + next.name()
-          + " in " + project.getProject().getName(), err);
-      return ChangeKind.REWORK;
-    }
-  }
-
-  private static boolean isSameParents(RevCommit prior, RevCommit next) {
-    if (prior.getParentCount() != next.getParentCount()) {
-      return false;
-    } else if (prior.getParentCount() == 0) {
-      return true;
-    }
-    return prior.getParent(0).equals(next.getParent(0));
-  }
-
   public class ChangeModifiedException extends InvalidChangeOperationException {
     private static final long serialVersionUID = 1L;
 

gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java

@@ -68,6 +68,7 @@ import com.google.gerrit.server.auth.AuthBackend;
 import com.google.gerrit.server.auth.UniversalAuthBackend;
 import com.google.gerrit.server.avatar.AvatarProvider;
 import com.google.gerrit.server.cache.CacheRemovalListener;
+import com.google.gerrit.server.change.ChangeKindCache;
 import com.google.gerrit.server.change.MergeabilityChecker;
 import com.google.gerrit.server.documentation.QueryDocumentationExecutor;
 import com.google.gerrit.server.events.EventFactory;
@@ -150,6 +151,7 @@ public class GerritGlobalModule extends FactoryModule {
     install(AccountByEmailCacheImpl.module());
     install(AccountCacheImpl.module());
     install(ChangeCache.module());
+    install(ChangeKindCache.module());
     install(ConflictsCacheImpl.module());
     install(GroupCacheImpl.module());
     install(GroupIncludeCacheImpl.module());

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

@@ -14,6 +14,8 @@
 
 package com.google.gerrit.server.git;
 
+import static com.google.common.base.Preconditions.checkArgument;
+
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.reviewdb.client.Account;
@@ -44,6 +46,7 @@ import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.merge.MergeStrategy;
+import org.eclipse.jgit.merge.Merger;
 import org.eclipse.jgit.merge.ThreeWayMerger;
 import org.eclipse.jgit.revwalk.FooterKey;
 import org.eclipse.jgit.revwalk.FooterLine;
@@ -72,6 +75,10 @@ import java.util.TimeZone;
 public class MergeUtil {
   private static final Logger log = LoggerFactory.getLogger(MergeUtil.class);
 
+  public static boolean useRecursiveMerge(Config cfg) {
+    return cfg.getBoolean("core", null, "useRecursiveMerge", false);
+  }
+
   public static interface Factory {
     MergeUtil create(ProjectState project);
     MergeUtil create(ProjectState project, boolean useContentMerge);
@@ -112,8 +119,7 @@ public class MergeUtil {
     this.urlProvider = urlProvider;
     this.project = project;
     this.useContentMerge = useContentMerge;
-    this.useRecursiveMerge =
-        serverConfig.getBoolean("core", null, "useRecursiveMerge", false);
+    this.useRecursiveMerge = useRecursiveMerge(serverConfig);
   }
 
   public CodeReviewCommit getFirstFastForward(
@@ -483,7 +489,7 @@ public class MergeUtil {
     }
   }
 
-  public ObjectInserter createDryRunInserter() {
+  public static ObjectInserter createDryRunInserter() {
     return new ObjectInserter() {
       @Override
       public ObjectId insert(int objectType, long length, InputStream in)
@@ -624,21 +630,35 @@ public class MergeUtil {
 
   public ThreeWayMerger newThreeWayMerger(final Repository repo,
       final ObjectInserter inserter) {
-    ThreeWayMerger m;
+    return newThreeWayMerger(repo, inserter,
+        mergeStrategyName(useContentMerge, useRecursiveMerge));
+  }
+
+  public static String mergeStrategyName(boolean useContentMerge,
+      boolean useRecursiveMerge) {
     if (useContentMerge) {
       // Settings for this project allow us to try and automatically resolve
       // conflicts within files if needed. Use either the old resolve merger or
       // new recursive merger, and instruct to operate in core.
       if (useRecursiveMerge) {
-        m = MergeStrategy.RECURSIVE.newMerger(repo, true);
+        return MergeStrategy.RECURSIVE.getName();
       } else {
-        m = MergeStrategy.RESOLVE.newMerger(repo, true);
+        return MergeStrategy.RESOLVE.getName();
       }
     } else {
       // No auto conflict resolving allowed. If any of the
       // affected files was modified, merge will fail.
-      m = MergeStrategy.SIMPLE_TWO_WAY_IN_CORE.newMerger(repo);
+      return MergeStrategy.SIMPLE_TWO_WAY_IN_CORE.getName();
     }
+  }
+
+  public static ThreeWayMerger newThreeWayMerger(Repository repo,
+      final ObjectInserter inserter, String strategyName) {
+    MergeStrategy strategy = MergeStrategy.get(strategyName);
+    checkArgument(strategy != null, "invalid merge strategy: %s", strategyName);
+    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() {
       @Override
       protected ObjectInserter delegate() {
@@ -653,7 +673,7 @@ public class MergeUtil {
       public void release() {
       }
     });
-    return m;
+    return (ThreeWayMerger) m;
   }
 
   public ObjectId commit(final ObjectInserter inserter,

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

@@ -69,10 +69,10 @@ import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.account.AccountResolver;
 import com.google.gerrit.server.change.ChangeInserter;
+import com.google.gerrit.server.change.ChangeKind;
+import com.google.gerrit.server.change.ChangeKindCache;
 import com.google.gerrit.server.change.ChangesCollection;
 import com.google.gerrit.server.change.MergeabilityChecker;
-import com.google.gerrit.server.change.PatchSetInserter;
-import com.google.gerrit.server.change.PatchSetInserter.ChangeKind;
 import com.google.gerrit.server.change.RevisionResource;
 import com.google.gerrit.server.change.Submit;
 import com.google.gerrit.server.config.AllProjectsName;
@@ -279,6 +279,7 @@ public class ReceiveCommits {
   private final SshInfo sshInfo;
   private final AllProjectsName allProjectsName;
   private final ReceiveConfig receiveConfig;
+  private final ChangeKindCache changeKindCache;
 
   private final ProjectControl projectControl;
   private final Project project;
@@ -302,7 +303,6 @@ public class ReceiveCommits {
   private final SubmoduleOp.Factory subOpFactory;
   private final Provider<Submit> submitProvider;
   private final MergeQueue mergeQueue;
-  private final MergeUtil.Factory mergeUtilFactory;
 
   private final List<CommitValidationMessage> messages = new ArrayList<CommitValidationMessage>();
   private ListMultimap<Error, String> errors = LinkedListMultimap.create();
@@ -348,7 +348,7 @@ public class ReceiveCommits {
       final SubmoduleOp.Factory subOpFactory,
       final Provider<Submit> submitProvider,
       final MergeQueue mergeQueue,
-      final MergeUtil.Factory mergeUtilFactory) throws IOException {
+      final ChangeKindCache changeKindCache) throws IOException {
     this.currentUser = (IdentifiedUser) projectControl.getCurrentUser();
     this.db = db;
     this.schemaFactory = schemaFactory;
@@ -377,6 +377,7 @@ public class ReceiveCommits {
     this.sshInfo = sshInfo;
     this.allProjectsName = allProjectsName;
     this.receiveConfig = config;
+    this.changeKindCache = changeKindCache;
 
     this.projectControl = projectControl;
     this.labelTypes = projectControl.getLabelTypes();
@@ -388,7 +389,6 @@ public class ReceiveCommits {
     this.subOpFactory = subOpFactory;
     this.submitProvider = submitProvider;
     this.mergeQueue = mergeQueue;
-    this.mergeUtilFactory = mergeUtilFactory;
 
     this.messageSender = new ReceivePackMessageSender();
 
@@ -1781,9 +1781,8 @@ public class ReceiveCommits {
         }
       }
 
-      changeKind =
-          PatchSetInserter.getChangeKind(mergeUtilFactory,
-              projectControl.getProjectState(), repo, priorCommit, newCommit);
+      changeKind = changeKindCache.getChangeKind(
+          projectControl.getProjectState(), repo, priorCommit, newCommit);
 
       PatchSet.Id id =
           ChangeUtil.nextPatchSetId(allRefs, change.currentPatchSetId());