PatchListLoader: Respect change.cacheAutomerge setting

When change.cacheAutomerge was false, we were still creating a new
inserter in PatchListLoader and passing it into AutoMerger, we were
just never flushing it. Unfortunately, when AutoMerger completely
ignores the caller's inserter to use its own InMemoryInserter, it
doesn't make the objects it created available to the caller in any
way.

The fix for this is in two parts. First, teach PatchListLoader to
create an InMemoryInserter rather than a real ObjectInserter when
cacheAutomerge is false. Second, teach AutoMerger to reuse an existing
InMemoryInserter if that's what the caller gave it.

This makes cacheAutomerge = false work with PatchListCache in offline
programs like reindex, but it is still not appropriate for running a
live server. In particular, it will take a bit of work to make
PatchScriptFactory get access to the in-memory objects that are
currently discarded before returning from the PatchListLoader.

Change-Id: I7eae7291e1f0195e55d66975e8431fce4bc86c4c

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

@@ -31,6 +31,7 @@ import org.eclipse.jgit.transport.PackParser;
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Set;
@@ -108,9 +109,16 @@ public class InMemoryInserter extends ObjectInserter {
     }
 
     @Override
-    public Collection<ObjectId> resolve(AbbreviatedObjectId id) {
+    public Collection<ObjectId> resolve(AbbreviatedObjectId id)
-      // This method should be unused by ChangeRebuilder.
+        throws IOException {
-      throw new UnsupportedOperationException();
+      Set<ObjectId> result = new HashSet<>();
+      for (ObjectId insId : inserted.keySet()) {
+        if (id.prefixCompare(insId) == 0) {
+          result.add(insId);
+        }
+      }
+      result.addAll(reader.resolve(id));
+      return result;
     }
 
     @Override

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

@@ -57,6 +57,10 @@ import java.util.Map;
 public class AutoMerger {
   private static final Logger log = LoggerFactory.getLogger(AutoMerger.class);
 
+  static boolean cacheAutomerge(Config cfg) {
+    return cfg.getBoolean("change", null, "cacheAutomerge", true);
+  }
+
   private final PersonIdent gerritIdent;
   private final boolean save;
 
@@ -64,7 +68,7 @@ public class AutoMerger {
   AutoMerger(
       @GerritServerConfig Config cfg,
       @GerritPersonIdent PersonIdent gerritIdent) {
-    save = cfg.getBoolean("change", null, "cacheAutomerge", true);
+    save = cacheAutomerge(cfg);
     this.gerritIdent = gerritIdent;
   }
 
@@ -79,7 +83,11 @@ public class AutoMerger {
       throws IOException {
     checkArgument(rw.getObjectReader().getCreatedFromInserter() == ins);
     InMemoryInserter tmpIns = null;
-    if (!save) {
+    if (ins instanceof InMemoryInserter) {
+      // Caller gave us an in-memory inserter, so ensure anything we write from
+      // this method is visible to them.
+      tmpIns = (InMemoryInserter) ins;
+    } else if (!save) {
       // If we don't plan on saving results, use a fully in-memory inserter.
       // Using just a non-flushing wrapper is not sufficient, since in
       // particular DfsInserter might try to write to storage after exceeding an
@@ -105,7 +113,7 @@ public class AutoMerger {
     ResolveMerger m = (ResolveMerger) mergeStrategy.newMerger(repo, true);
     DirCache dc = DirCache.newInCore();
     m.setDirCache(dc);
-    m.setObjectInserter(save ? new NonFlushingWrapper(ins) : tmpIns);
+    m.setObjectInserter(tmpIns == null ? new NonFlushingWrapper(ins) : tmpIns);
 
     boolean couldMerge;
     try {
@@ -236,6 +244,7 @@ public class AutoMerger {
     }
 
     checkArgument(tmpIns == null);
+    checkArgument(!(ins instanceof InMemoryInserter));
     ObjectId commitId = ins.insert(cb);
     ins.flush();
 

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

@@ -27,6 +27,7 @@ import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.server.config.ConfigUtil;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.git.InMemoryInserter;
 import com.google.gerrit.server.git.MergeUtil;
 import com.google.inject.assistedinject.Assisted;
 import com.google.inject.assistedinject.AssistedInject;
@@ -84,6 +85,7 @@ public class PatchListLoader implements Callable<PatchList> {
   private final PatchListKey key;
   private final Project.NameKey project;
   private final long timeoutMillis;
+  private final boolean save;
 
   @AssistedInject
   PatchListLoader(GitRepositoryManager mgr,
@@ -104,6 +106,7 @@ public class PatchListLoader implements Callable<PatchList> {
         ConfigUtil.getTimeUnit(cfg, "cache", PatchListCacheImpl.FILE_NAME,
             "timeout", TimeUnit.MILLISECONDS.convert(5, TimeUnit.SECONDS),
             TimeUnit.MILLISECONDS);
+    save = AutoMerger.cacheAutomerge(cfg);
   }
 
   @Override
@@ -131,33 +134,39 @@ public class PatchListLoader implements Callable<PatchList> {
     }
   }
 
-  private PatchList readPatchList(final PatchListKey key, final Repository repo)
+  private ObjectInserter newInserter(Repository repo) {
+    return save
+        ? repo.newObjectInserter()
+        : new InMemoryInserter(repo);
+  }
+
+  private PatchList readPatchList(PatchListKey key, Repository repo)
       throws IOException, PatchListNotAvailableException {
-    final RawTextComparator cmp = comparatorFor(key.getWhitespace());
+    RawTextComparator cmp = comparatorFor(key.getWhitespace());
-    try (ObjectInserter ins = repo.newObjectInserter();
+    try (ObjectInserter ins = newInserter(repo);
         ObjectReader reader = ins.newReader();
         RevWalk rw = new RevWalk(reader);
         DiffFormatter df = new DiffFormatter(DisabledOutputStream.INSTANCE)) {
-      final RevCommit b = rw.parseCommit(key.getNewId());
+      RevCommit b = rw.parseCommit(key.getNewId());
-      final RevObject a = aFor(key, repo, rw, ins, b);
+      RevObject a = aFor(key, repo, rw, ins, b);
 
       if (a == null) {
         // TODO(sop) Remove this case.
         // This is a merge commit, compared to its ancestor.
         //
-        final PatchListEntry[] entries = new PatchListEntry[1];
+        PatchListEntry[] entries = new PatchListEntry[1];
         entries[0] = newCommitMessage(cmp, reader, null, b);
         return new PatchList(a, b, true, entries);
       }
 
-      final boolean againstParent =
+      boolean againstParent =
           b.getParentCount() > 0 && b.getParent(0).equals(a);
 
       RevCommit aCommit = a instanceof RevCommit ? (RevCommit) a : null;
       RevTree aTree = rw.parseTree(a);
       RevTree bTree = b.getTree();
 
-      df.setRepository(repo);
+      df.setReader(reader, repo.getConfig());
       df.setDiffComparator(cmp);
       df.setDetectRenames(true);
       List<DiffEntry> diffEntries = df.scan(aTree, bTree);
@@ -191,9 +200,9 @@ public class PatchListLoader implements Callable<PatchList> {
 
           FileHeader fh = toFileHeader(key, df, e);
           long oldSize =
-              getFileSize(repo, reader, e.getOldMode(), e.getOldPath(), aTree);
+              getFileSize(reader, e.getOldMode(), e.getOldPath(), aTree);
           long newSize =
-              getFileSize(repo, reader, e.getNewMode(), e.getNewPath(), bTree);
+              getFileSize(reader, e.getNewMode(), e.getNewPath(), bTree);
           entries.add(newEntry(aTree, fh, newSize, newSize - oldSize));
         }
       }
@@ -202,14 +211,14 @@ public class PatchListLoader implements Callable<PatchList> {
     }
   }
 
-  private static long getFileSize(Repository repo, ObjectReader reader,
+  private static long getFileSize(ObjectReader reader,
       FileMode mode, String path, RevTree t) throws IOException {
     if (!isBlob(mode)) {
       return 0;
     }
     try (TreeWalk tw = TreeWalk.forPath(reader, path, t)) {
       return tw != null
-          ? repo.open(tw.getObjectId(0), OBJ_BLOB).getSize()
+          ? reader.open(tw.getObjectId(0), OBJ_BLOB).getSize()
           : 0;
     }
   }
@@ -324,7 +333,7 @@ public class PatchListLoader implements Callable<PatchList> {
 
     switch (b.getParentCount()) {
       case 0:
-        return rw.parseAny(emptyTree(repo));
+        return rw.parseAny(emptyTree(ins));
       case 1: {
         RevCommit r = b.getParent(0);
         rw.parseBody(r);
@@ -343,11 +352,9 @@ public class PatchListLoader implements Callable<PatchList> {
     }
   }
 
-  private static ObjectId emptyTree(final Repository repo) throws IOException {
+  private static ObjectId emptyTree(ObjectInserter ins) throws IOException {
-    try (ObjectInserter oi = repo.newObjectInserter()) {
+    ObjectId id = ins.insert(Constants.OBJ_TREE, new byte[] {});
-      ObjectId id = oi.insert(Constants.OBJ_TREE, new byte[] {});
+    ins.flush();
-      oi.flush();
     return id;
   }
 }
-}