Don't expose RepoView#getRepository()

For the purposes of fusing BatchUpdateOp#updateChange and updateRepo
into a single batch update, implementations of updateChange need to be
able to observe the ref updates from updateRepo before they are
executed. If we expose the repository directly to updateRepo, it's too
easy for them use it in a wrong way that won't see the results of
updateRepo. That is why we limited the operations that are available
through RepoView.

Finish this refactoring by not exposing the Repository to any op phases,
including updateRepo.

The repo still needs to be directly accessed by the BatchUpdate
internals so it can actually do the writes, which is fine as it doesn't
escape this package.

Change-Id: I0959aa2d2bce346a9a6d493c23a38301b43d1e5d

gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdate.java

@@ -322,11 +322,6 @@ public abstract class BatchUpdate implements AutoCloseable {
         : Optional.empty();
   }
 
-  protected Repository getRepository() throws IOException {
-    initRepository();
-    return repoView.getRepository();
-  }
-
   protected RevWalk getRevWalk() throws IOException {
     initRepository();
     return repoView.getRevWalk();

gerrit-server/src/main/java/com/google/gerrit/server/update/Context.java

@@ -24,7 +24,6 @@ import com.google.gerrit.server.IdentifiedUser;
 import java.io.IOException;
 import java.sql.Timestamp;
 import java.util.TimeZone;
-import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevWalk;
 
 /**
@@ -41,15 +40,14 @@ public interface Context {
   Project.NameKey getProject();
 
   /**
-   * Get an open repository instance for this project.
+   * Get a read-only view of the open repository for this project.
    *
-   * <p>Will be opened lazily if necessary; callers should not close the repo. In some phases of the
-   * update, the repository might be read-only; see {@link BatchUpdateOp} for details.
+   * <p>Will be opened lazily if necessary.
    *
    * @return repository instance.
    * @throws IOException if an error occurred opening the repo.
    */
-  Repository getRepository() throws IOException;
+  RepoView getRepoView() throws IOException;
 
   /**
    * Get a walk for this project.

gerrit-server/src/main/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java

@@ -140,19 +140,15 @@ class NoteDbBatchUpdate extends BatchUpdate {
   }
 
   class ContextImpl implements Context {
-    private Repository repoWrapper;
-
     @Override
-    public Repository getRepository() throws IOException {
-      if (repoWrapper == null) {
-        repoWrapper = new ReadOnlyRepository(NoteDbBatchUpdate.this.getRepository());
-      }
-      return repoWrapper;
+    public RepoView getRepoView() throws IOException {
+      initRepository();
+      return NoteDbBatchUpdate.this.repoView;
     }
 
     @Override
     public RevWalk getRevWalk() throws IOException {
-      return NoteDbBatchUpdate.this.getRevWalk();
+      return getRepoView().getRevWalk();
     }
 
     @Override
@@ -187,14 +183,9 @@ class NoteDbBatchUpdate extends BatchUpdate {
   }
 
   private class RepoContextImpl extends ContextImpl implements RepoContext {
-    @Override
-    public Repository getRepository() throws IOException {
-      return NoteDbBatchUpdate.this.getRepository();
-    }
-
     @Override
     public ObjectInserter getInserter() throws IOException {
-      return NoteDbBatchUpdate.this.getObjectInserter();
+      return getRepoView().getInserter();
     }
 
     @Override
@@ -370,7 +361,8 @@ class NoteDbBatchUpdate extends BatchUpdate {
     logDebug("Executing change ops");
     Map<Change.Id, ChangeResult> result =
         Maps.newLinkedHashMapWithExpectedSize(ops.keySet().size());
-    Repository repo = getRepository();
+    initRepository();
+    Repository repo = repoView.getRepository();
     // TODO(dborowitz): Teach NoteDbUpdateManager to allow reusing the same inserter and batch ref
     // update as in executeUpdateRepo.
     try (ObjectInserter ins = repo.newObjectInserter();

gerrit-server/src/main/java/com/google/gerrit/server/update/RepoContext.java

@@ -31,8 +31,7 @@ public interface RepoContext extends Context {
   /**
    * Add a command to the pending list of commands.
    *
-   * <p>Callers should use this method instead of writing directly to the repository returned by
-   * {@link #getRepository()}.
+   * <p>This method is the only way of updating refs in the repository from a {@link BatchUpdateOp}.
    *
    * @param cmd ref update command.
    * @throws IOException if an error occurred opening the repo.

gerrit-server/src/main/java/com/google/gerrit/server/update/RepoView.java

@@ -17,15 +17,33 @@ package com.google.gerrit.server.update;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 
+import com.google.common.collect.Maps;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
 import java.util.Optional;
+import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
+import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevWalk;
 
+/**
+ * Restricted view of a {@link Repository} for use by {@link BatchUpdateOp} implementations.
+ *
+ * <p>This class serves two purposes in the context of {@link BatchUpdate}. First, the subset of
+ * normal Repository functionality is purely read-only, which prevents implementors from modifying
+ * the repository outside of {@link BatchUpdateOp#updateRepo}. Write operations can only be
+ * performed by calling methods on {@link RepoContext}.
+ *
+ * <p>Second, the read methods take into account any pending operations on the repository that
+ * implementations have staged using the write methods on {@link RepoContext}. Callers do not have
+ * to worry about whether operations have been performed yet, and the implementation details may
+ * differ between ReviewDb and NoteDb, but callers just don't need to care.
+ */
 public class RepoView {
   private final Repository repo;
   private final RevWalk rw;
@@ -54,25 +72,112 @@ public class RepoView {
     closeRepo = false;
   }
 
+  /**
+   * Get this repo's configuration.
+   *
+   * <p>This is the storage-level config you would get with {@link Repository#getConfig()}, not, for
+   * example, the Gerrit-level project config.
+   *
+   * @return a defensive copy of the config; modifications have no effect on the underlying config.
+   */
+  public Config getConfig() {
+    return new Config(repo.getConfig());
+  }
+
+  /**
+   * Get an open revwalk on the repo.
+   *
+   * <p>Guaranteed to be able to read back any objects inserted in the repository via {@link
+   * RepoContext#getInserter()}, even if objects have not been flushed to the underlying repo. In
+   * particular this includes any object returned by {@link #getRef(String)}, even taking into
+   * account not-yet-executed commands.
+   *
+   * @return revwalk.
+   */
   public RevWalk getRevWalk() {
     return rw;
   }
 
-  public ObjectInserter getInserter() {
-    return inserter;
-  }
-
-  public ChainedReceiveCommands getCommands() {
-    return commands;
-  }
-
+  /**
+   * Read a single ref from the repo.
+   *
+   * <p>Takes into account any ref update commands added during the course of the update using
+   * {@link RepoContext#addRefUpdate}, even if they have not yet been executed on the underlying
+   * repo.
+   *
+   * <p>The results of individual ref lookups are cached: calling this method multiple times with
+   * the same ref name will return the same result (unless a command was added in the meantime). The
+   * repo is not reread.
+   *
+   * @param name exact ref name.
+   * @return the value of the ref, if present.
+   * @throws IOException if an error occurred.
+   */
   public Optional<ObjectId> getRef(String name) throws IOException {
     return getCommands().get(name);
   }
 
-  // TODO(dborowitz): Remove this so callers can't do arbitrary stuff.
-  Repository getRepository() {
-    return repo;
+  /**
+   * Look up refs by prefix.
+   *
+   * <p>Takes into account any ref update commands added during the course of the update using
+   * {@link RepoContext#addRefUpdate}, even if they have not yet been executed on the underlying
+   * repo.
+   *
+   * <p>For any ref that has previously been accessed with {@link #getRef(String)}, the value in the
+   * result map will be that same cached value. Any refs that have <em>not</em> been previously
+   * accessed are re-scanned from the repo on each call.
+   *
+   * @param prefix ref prefix; must end in '/' or else be empty.
+   * @return a map of ref suffixes to SHA-1s. The refs are all under {@code prefix} and have the
+   *     prefix stripped; this matches the behavior of {@link
+   *     org.eclipse.jgit.lib.RefDatabase#getRefs(String)}.
+   * @throws IOException if an error occurred.
+   */
+  public Map<String, ObjectId> getRefs(String prefix) throws IOException {
+    Map<String, ObjectId> result =
+        new HashMap<>(
+            Maps.transformValues(repo.getRefDatabase().getRefs(prefix), Ref::getObjectId));
+
+    // First, overwrite any cached reads from the underlying RepoRefCache. If any of these differ,
+    // it's because a ref was updated after the RepoRefCache read it. It feels a little odd to
+    // prefer the *old* value in this case, but it would be weirder to be inconsistent with getRef.
+    //
+    // Mostly this doesn't matter. If the caller was intending to write to the ref, they lost a
+    // race, and they will get a lock failure. If they just want to read, well, the JGit interface
+    // doesn't currently guarantee that any snapshot of multiple refs is consistent, so they were
+    // probably out of luck anyway.
+    commands
+        .getRepoRefCache()
+        .getCachedRefs()
+        .forEach((k, v) -> updateRefIfPrefixMatches(result, prefix, k, v));
+
+    // Second, overwrite with any pending commands.
+    commands
+        .getCommands()
+        .values()
+        .forEach(
+            c ->
+                updateRefIfPrefixMatches(result, prefix, c.getRefName(), toOptional(c.getNewId())));
+
+    return result;
+  }
+
+  private static Optional<ObjectId> toOptional(ObjectId id) {
+    return id.equals(ObjectId.zeroId()) ? Optional.empty() : Optional.of(id);
+  }
+
+  private static void updateRefIfPrefixMatches(
+      Map<String, ObjectId> map, String prefix, String fullRefName, Optional<ObjectId> maybeId) {
+    if (!fullRefName.startsWith(prefix)) {
+      return;
+    }
+    String suffix = fullRefName.substring(prefix.length());
+    if (maybeId.isPresent()) {
+      map.put(suffix, maybeId.get());
+    } else {
+      map.remove(suffix);
+    }
   }
 
   // Not AutoCloseable so callers can't improperly close it. Plus it's never managed with a try
@@ -84,4 +189,16 @@ public class RepoView {
       repo.close();
     }
   }
+
+  Repository getRepository() {
+    return repo;
+  }
+
+  ObjectInserter getInserter() {
+    return inserter;
+  }
+
+  ChainedReceiveCommands getCommands() {
+    return commands;
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java

@@ -111,19 +111,15 @@ class ReviewDbBatchUpdate extends BatchUpdate {
   }
 
   class ContextImpl implements Context {
-    private Repository repoWrapper;
-
     @Override
-    public Repository getRepository() throws IOException {
-      if (repoWrapper == null) {
-        repoWrapper = new ReadOnlyRepository(ReviewDbBatchUpdate.this.getRepository());
-      }
-      return repoWrapper;
+    public RepoView getRepoView() throws IOException {
+      initRepository();
+      return ReviewDbBatchUpdate.this.repoView;
     }
 
     @Override
     public RevWalk getRevWalk() throws IOException {
-      return ReviewDbBatchUpdate.this.getRevWalk();
+      return getRepoView().getRevWalk();
     }
 
     @Override
@@ -158,14 +154,9 @@ class ReviewDbBatchUpdate extends BatchUpdate {
   }
 
   private class RepoContextImpl extends ContextImpl implements RepoContext {
-    @Override
-    public Repository getRepository() throws IOException {
-      return ReviewDbBatchUpdate.this.getRepository();
-    }
-
     @Override
     public ObjectInserter getInserter() throws IOException {
-      return ReviewDbBatchUpdate.this.getObjectInserter();
+      return getRepoView().getInserter();
     }
 
     @Override
@@ -200,11 +191,6 @@ class ReviewDbBatchUpdate extends BatchUpdate {
       return dbWrapper;
     }
 
-    @Override
-    public Repository getRepository() {
-      return threadLocalRepo;
-    }
-
     @Override
     public RevWalk getRevWalk() {
       return threadLocalRevWalk;
@@ -534,9 +520,10 @@ class ReviewDbBatchUpdate extends BatchUpdate {
     // updates on the change repo first.
     logDebug("Executing NoteDb updates for {} changes", tasks.size());
     try {
-      BatchRefUpdate changeRefUpdate = getRepository().getRefDatabase().newBatchUpdate();
+      initRepository();
+      BatchRefUpdate changeRefUpdate = repoView.getRepository().getRefDatabase().newBatchUpdate();
       boolean hasAllUsersCommands = false;
-      try (ObjectInserter ins = getRepository().newObjectInserter()) {
+      try (ObjectInserter ins = repoView.getRepository().newObjectInserter()) {
         int objs = 0;
         for (ChangeTask task : tasks) {
           if (task.noteDbResult == null) {
@@ -643,7 +630,8 @@ class ReviewDbBatchUpdate extends BatchUpdate {
     public Void call() throws Exception {
       taskId = id.toString() + "-" + Thread.currentThread().getId();
       if (Thread.currentThread() == mainThread) {
-        Repository repo = getRepository();
+        initRepository();
+        Repository repo = repoView.getRepository();
         try (RevWalk rw = new RevWalk(repo)) {
           call(ReviewDbBatchUpdate.this.db, repo, rw);
         }
@@ -802,10 +790,10 @@ class ReviewDbBatchUpdate extends BatchUpdate {
           updateManagerFactory
               .create(ctx.getProject())
               .setChangeRepo(
-                  ctx.getRepository(),
-                  ctx.getRevWalk(),
+                  ctx.threadLocalRepo,
+                  ctx.threadLocalRevWalk,
                   null,
-                  new ChainedReceiveCommands(ctx.getRepository()));
+                  new ChainedReceiveCommands(ctx.threadLocalRepo));
       if (ctx.getUser().isIdentifiedUser()) {
         updateManager.setRefLogIdent(
             ctx.getUser().asIdentifiedUser().newRefLogIdent(ctx.getWhen(), tz));