Merge changes I8e67dc88,Ib7cdcd97

* changes:
  Don't fuse code and meta ref updates if it won't be atomic
  TestNotesMigration: Initialize from env during constructor

Documentation/dev-note-db.txt

@@ -32,8 +32,8 @@ same repository as code changes.
 - Storing the rest of account data is a work in progress.
 - Storing group data is a work in progress.
 
-To match the current configuration of `googlesource.com`, paste the following
+To use a configuration similar to the current state of `googlesource.com`, paste
-config snippet in your `gerrit.config`:
+the following config snippet in your `gerrit.config`:
 
 ----
 [noteDb "changes"]
@@ -46,6 +46,9 @@ config snippet in your `gerrit.config`:
 This is the configuration that will be produced if you enable experimental
 NoteDb support on a new site with `init`.
 
+`googlesource.com` additionally uses `fuseUpdates = true`, because its repo
+backend supports atomic multi-ref transactions. Native JGit doesn't, so setting
+this option on a dev server would fail.
 
 For an example NoteDb change, poke around at this one:
 ----
@@ -99,6 +102,11 @@ previous options, unless otherwise noted.
   implementation of the `rebuild-note-db` program does not do this. +
   In this phase, it would be possible to delete the Changes tables out from
   under a running server with no effect.
+- `noteDb.changes.fuseUpdates=true`: Code and meta updates within a single
+  repository are fused into a single atomic `BatchRefUpdate`, so they either
+  all succeed or all fail. This mode is only possible on a backend that
+  supports atomic ref updates, which notably excludes the default file repository
+  backend.
 
 [[migration]]
 == Migration

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -348,7 +348,6 @@ public abstract class AbstractDaemonTest {
     }
 
     server.getTestInjector().injectMembers(this);
-    notesMigration.setFromEnv();
     Transport.register(inProcessProtocol);
     toClose = Collections.synchronizedList(new ArrayList<Repository>());
     admin = accounts.admin();
@@ -528,6 +527,7 @@ public abstract class AbstractDaemonTest {
       server.stop();
       server = null;
     }
+    notesMigration.resetFromEnv();
   }
 
   protected TestRepository<?>.CommitBuilder commitBuilder() throws Exception {

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java

@@ -896,7 +896,14 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
 
     PushResult pr =
         GitUtil.pushHead(testRepo, "refs/for/foo%base=" + rBase.getCommit().name(), false, false);
+
+    if (notesMigration.fuseUpdates()) {
+      // InMemoryRepository's atomic BatchRefUpdate implementation doesn't update the progress
+      // monitor. That's fine, we just care that there was at least one new change and no errors.
+      assertThat(pr.getMessages()).contains("changes: new: 1, done");
+    } else {
       assertThat(pr.getMessages()).contains("changes: new: 1, refs: 1, done");
+    }
 
     assertTwoChangesWithSameRevision(r);
   }

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbOnlyIT.java

@@ -31,7 +31,6 @@ import com.google.gerrit.server.update.BatchUpdate;
 import com.google.gerrit.server.update.BatchUpdateOp;
 import com.google.gerrit.server.update.ChangeContext;
 import com.google.gerrit.server.update.RepoContext;
-import com.google.gerrit.testutil.NoteDbMode;
 import com.google.inject.Inject;
 import java.io.IOException;
 import java.util.EnumSet;
@@ -48,11 +47,12 @@ public class NoteDbOnlyIT extends AbstractDaemonTest {
 
   @Before
   public void setUp() throws Exception {
-    assume().that(NoteDbMode.get()).isEqualTo(NoteDbMode.DISABLE_CHANGE_REVIEW_DB);
+    assume().that(notesMigration.disableChangeReviewDb()).isTrue();
   }
 
   @Test
   public void updateChangeFailureRollsBackRefUpdate() throws Exception {
+    assume().that(notesMigration.fuseUpdates()).isTrue();
     PushOneCommit.Result r = createChange();
     Change.Id id = r.getChange().getId();
 

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ConfigNotesMigration.java

@@ -48,6 +48,7 @@ public class ConfigNotesMigration extends NotesMigration {
 
   // All of these names must be reflected in the allowed set in checkConfig.
   private static final String DISABLE_REVIEW_DB = "disableReviewDb";
+  private static final String FUSE_UPDATES = "fuseUpdates";
   private static final String PRIMARY_STORAGE = "primaryStorage";
   private static final String READ = "read";
   private static final String SEQUENCE = "sequence";
@@ -77,6 +78,8 @@ public class ConfigNotesMigration extends NotesMigration {
     cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), SEQUENCE, true);
     cfg.setString(SECTION_NOTE_DB, CHANGES.key(), PRIMARY_STORAGE, PrimaryStorage.NOTE_DB.name());
     cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), DISABLE_REVIEW_DB, true);
+    // TODO(dborowitz): Set to true when FileRepository supports it.
+    cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), FUSE_UPDATES, false);
     return cfg;
   }
 
@@ -85,6 +88,7 @@ public class ConfigNotesMigration extends NotesMigration {
   private final boolean readChangeSequence;
   private final PrimaryStorage changePrimaryStorage;
   private final boolean disableChangeReviewDb;
+  private final boolean fuseUpdates;
 
   @Inject
   public ConfigNotesMigration(@GerritServerConfig Config cfg) {
@@ -103,6 +107,7 @@ public class ConfigNotesMigration extends NotesMigration {
         cfg.getEnum(SECTION_NOTE_DB, CHANGES.key(), PRIMARY_STORAGE, PrimaryStorage.REVIEW_DB);
     disableChangeReviewDb =
         cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), DISABLE_REVIEW_DB, false);
+    fuseUpdates = cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), FUSE_UPDATES, false);
 
     checkArgument(
         !(disableChangeReviewDb && changePrimaryStorage != PrimaryStorage.NOTE_DB),
@@ -133,4 +138,9 @@ public class ConfigNotesMigration extends NotesMigration {
   public boolean disableChangeReviewDb() {
     return disableChangeReviewDb;
   }
+
+  @Override
+  public boolean fuseUpdates() {
+    return fuseUpdates;
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java

@@ -82,6 +82,19 @@ public abstract class NotesMigration {
    */
   public abstract boolean disableChangeReviewDb();
 
+  /**
+   * Fuse meta ref updates in the same batch as code updates.
+   *
+   * <p>When set, each {@link com.google.gerrit.server.update.BatchUpdate} results in a single
+   * {@link org.eclipse.jgit.lib.BatchRefUpdate} to update both code and meta refs atomically.
+   * Setting this option with a repository backend that does not support atomic multi-ref
+   * transactions ({@link org.eclipse.jgit.lib.RefDatabase#performsAtomicTransactions()}) is a
+   * configuration error, and all updates will fail at runtime.
+   *
+   * <p>Has no effect if {@link #disableChangeReviewDb()} is false.
+   */
+  public abstract boolean fuseUpdates();
+
   /**
    * Whether to fail when reading any data from NoteDb.
    *

gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteRef.java

@@ -170,6 +170,7 @@ public class DeleteRef {
   private void deleteMultipleRefs(Repository r)
       throws OrmException, IOException, ResourceConflictException {
     BatchRefUpdate batchUpdate = r.getRefDatabase().newBatchUpdate();
+    batchUpdate.setAtomic(false);
     List<String> refs =
         prefix == null
             ? refsToDelete

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

@@ -94,7 +94,8 @@ public abstract class BatchUpdate implements AutoCloseable {
       @Override
       public void configure() {
         factory(ReviewDbBatchUpdate.AssistedFactory.class);
-        factory(NoteDbBatchUpdate.AssistedFactory.class);
+        factory(FusedNoteDbBatchUpdate.AssistedFactory.class);
+        factory(UnfusedNoteDbBatchUpdate.AssistedFactory.class);
       }
     };
   }
@@ -103,22 +104,28 @@ public abstract class BatchUpdate implements AutoCloseable {
   public static class Factory {
     private final NotesMigration migration;
     private final ReviewDbBatchUpdate.AssistedFactory reviewDbBatchUpdateFactory;
-    private final NoteDbBatchUpdate.AssistedFactory noteDbBatchUpdateFactory;
+    private final FusedNoteDbBatchUpdate.AssistedFactory fusedNoteDbBatchUpdateFactory;
+    private final UnfusedNoteDbBatchUpdate.AssistedFactory unfusedNoteDbBatchUpdateFactory;
 
     @Inject
     Factory(
         NotesMigration migration,
         ReviewDbBatchUpdate.AssistedFactory reviewDbBatchUpdateFactory,
-        NoteDbBatchUpdate.AssistedFactory noteDbBatchUpdateFactory) {
+        FusedNoteDbBatchUpdate.AssistedFactory fusedNoteDbBatchUpdateFactory,
+        UnfusedNoteDbBatchUpdate.AssistedFactory unfusedNoteDbBatchUpdateFactory) {
       this.migration = migration;
       this.reviewDbBatchUpdateFactory = reviewDbBatchUpdateFactory;
-      this.noteDbBatchUpdateFactory = noteDbBatchUpdateFactory;
+      this.fusedNoteDbBatchUpdateFactory = fusedNoteDbBatchUpdateFactory;
+      this.unfusedNoteDbBatchUpdateFactory = unfusedNoteDbBatchUpdateFactory;
     }
 
     public BatchUpdate create(
         ReviewDb db, Project.NameKey project, CurrentUser user, Timestamp when) {
       if (migration.disableChangeReviewDb()) {
-        return noteDbBatchUpdateFactory.create(db, project, user, when);
+        if (migration.fuseUpdates()) {
+          return fusedNoteDbBatchUpdateFactory.create(db, project, user, when);
+        }
+        return unfusedNoteDbBatchUpdateFactory.create(db, project, user, when);
       }
       return reviewDbBatchUpdateFactory.create(db, project, user, when);
     }
@@ -138,9 +145,15 @@ public abstract class BatchUpdate implements AutoCloseable {
       // copy them into an ImmutableList so there is no chance the callee can pollute the input
       // collection.
       if (migration.disableChangeReviewDb()) {
-        ImmutableList<NoteDbBatchUpdate> noteDbUpdates =
+        if (migration.fuseUpdates()) {
+          ImmutableList<FusedNoteDbBatchUpdate> noteDbUpdates =
               (ImmutableList) ImmutableList.copyOf(updates);
-        NoteDbBatchUpdate.execute(noteDbUpdates, listener, requestId, dryRun);
+          FusedNoteDbBatchUpdate.execute(noteDbUpdates, listener, requestId, dryRun);
+        } else {
+          ImmutableList<UnfusedNoteDbBatchUpdate> noteDbUpdates =
+              (ImmutableList) ImmutableList.copyOf(updates);
+          UnfusedNoteDbBatchUpdate.execute(noteDbUpdates, listener, requestId, dryRun);
+        }
       } else {
         ImmutableList<ReviewDbBatchUpdate> reviewDbUpdates =
             (ImmutableList) ImmutableList.copyOf(updates);

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

@@ -16,6 +16,7 @@ package com.google.gerrit.server.update;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
 import static java.util.Comparator.comparing;
 
 import com.google.common.base.Throwables;
@@ -51,23 +52,25 @@ import java.util.TimeZone;
 import java.util.TreeMap;
 import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.transport.ReceiveCommand;
 
 /**
- * {@link BatchUpdate} implementation that only supports NoteDb.
+ * {@link BatchUpdate} implementation using only NoteDb that updates code refs and meta refs in a
+ * single {@link org.eclipse.jgit.lib.BatchRefUpdate}.
  *
  * <p>Used when {@code noteDb.changes.disableReviewDb=true}, at which point ReviewDb is not
  * consulted during updates.
  */
-class NoteDbBatchUpdate extends BatchUpdate {
+class FusedNoteDbBatchUpdate extends BatchUpdate {
   interface AssistedFactory {
-    NoteDbBatchUpdate create(
+    FusedNoteDbBatchUpdate create(
         ReviewDb db, Project.NameKey project, CurrentUser user, Timestamp when);
   }
 
   static void execute(
-      ImmutableList<NoteDbBatchUpdate> updates,
+      ImmutableList<FusedNoteDbBatchUpdate> updates,
       BatchUpdateListener listener,
       @Nullable RequestId requestId,
       boolean dryrun)
@@ -84,11 +87,11 @@ class NoteDbBatchUpdate extends BatchUpdate {
       try {
         switch (order) {
           case REPO_BEFORE_DB:
-            for (NoteDbBatchUpdate u : updates) {
+            for (FusedNoteDbBatchUpdate u : updates) {
               u.executeUpdateRepo();
             }
             listener.afterUpdateRepos();
-            for (NoteDbBatchUpdate u : updates) {
+            for (FusedNoteDbBatchUpdate u : updates) {
               handles.add(u.executeChangeOps(dryrun));
             }
             for (ChangesHandle h : handles) {
@@ -109,10 +112,10 @@ class NoteDbBatchUpdate extends BatchUpdate {
             // TODO(dborowitz): This may still result in multiple updates to All-Users, but that's
             // currently not a big deal because multi-change batches generally aren't affecting
             // drafts anyway.
-            for (NoteDbBatchUpdate u : updates) {
+            for (FusedNoteDbBatchUpdate u : updates) {
               handles.add(u.executeChangeOps(dryrun));
             }
-            for (NoteDbBatchUpdate u : updates) {
+            for (FusedNoteDbBatchUpdate u : updates) {
               u.executeUpdateRepo();
             }
             for (ChangesHandle h : handles) {
@@ -147,7 +150,7 @@ class NoteDbBatchUpdate extends BatchUpdate {
               u -> u.gitRefUpdated.fire(u.project, u.batchRefUpdate, u.getAccount().orElse(null)));
 
       if (!dryrun) {
-        for (NoteDbBatchUpdate u : updates) {
+        for (FusedNoteDbBatchUpdate u : updates) {
           u.executePostOps();
         }
       }
@@ -159,7 +162,7 @@ class NoteDbBatchUpdate extends BatchUpdate {
   class ContextImpl implements Context {
     @Override
     public RepoView getRepoView() throws IOException {
-      return NoteDbBatchUpdate.this.getRepoView();
+      return FusedNoteDbBatchUpdate.this.getRepoView();
     }
 
     @Override
@@ -268,7 +271,7 @@ class NoteDbBatchUpdate extends BatchUpdate {
   private final ReviewDb db;
 
   @Inject
-  NoteDbBatchUpdate(
+  FusedNoteDbBatchUpdate(
       GitRepositoryManager repoManager,
       @GerritPersonIdent PersonIdent serverIdent,
       ChangeNotes.Factory changeNotesFactory,
@@ -351,7 +354,7 @@ class NoteDbBatchUpdate extends BatchUpdate {
     }
 
     void execute() throws OrmException, IOException {
-      NoteDbBatchUpdate.this.batchRefUpdate = manager.execute(dryrun);
+      FusedNoteDbBatchUpdate.this.batchRefUpdate = manager.execute(dryrun);
     }
 
     List<CheckedFuture<?, IOException>> startIndexFutures() {
@@ -382,16 +385,19 @@ class NoteDbBatchUpdate extends BatchUpdate {
   private ChangesHandle executeChangeOps(boolean dryrun) throws Exception {
     logDebug("Executing change ops");
     initRepository();
+    Repository repo = repoView.getRepository();
+    checkState(
+        repo.getRefDatabase().performsAtomicTransactions(),
+        "cannot use noteDb.changes.fuseUpdates=true with a repository that does not support atomic"
+            + " batch ref updates: %s",
+        repo);
 
     ChangesHandle handle =
         new ChangesHandle(
             updateManagerFactory
                 .create(project)
                 .setChangeRepo(
-                    repoView.getRepository(),
+                    repo, repoView.getRevWalk(), repoView.getInserter(), repoView.getCommands()),
-                    repoView.getRevWalk(),
-                    repoView.getInserter(),
-                    repoView.getCommands()),
             dryrun);
     if (user.isIdentifiedUser()) {
       handle.manager.setRefLogIdent(user.asIdentifiedUser().newRefLogIdent(when, tz));

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

@@ -0,0 +1,458 @@
+// Copyright (C) 2017 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.update;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
+import static java.util.Comparator.comparing;
+import static java.util.stream.Collectors.toList;
+
+import com.google.common.base.Throwables;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Maps;
+import com.google.common.util.concurrent.CheckedFuture;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.GerritPersonIdent;
+import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.index.change.ChangeIndexer;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.notedb.ChangeUpdate;
+import com.google.gerrit.server.notedb.NoteDbUpdateManager;
+import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.util.RequestId;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.assistedinject.Assisted;
+import java.io.IOException;
+import java.sql.Timestamp;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.TimeZone;
+import java.util.TreeMap;
+import org.eclipse.jgit.lib.NullProgressMonitor;
+import org.eclipse.jgit.lib.ObjectInserter;
+import org.eclipse.jgit.lib.ObjectReader;
+import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.transport.ReceiveCommand;
+
+/**
+ * {@link BatchUpdate} implementation that only supports NoteDb.
+ *
+ * <p>Used when {@code noteDb.changes.disableReviewDb=true}, at which point ReviewDb is not
+ * consulted during updates.
+ */
+class UnfusedNoteDbBatchUpdate extends BatchUpdate {
+  interface AssistedFactory {
+    UnfusedNoteDbBatchUpdate create(
+        ReviewDb db, Project.NameKey project, CurrentUser user, Timestamp when);
+  }
+
+  static void execute(
+      ImmutableList<UnfusedNoteDbBatchUpdate> updates,
+      BatchUpdateListener listener,
+      @Nullable RequestId requestId,
+      boolean dryrun)
+      throws UpdateException, RestApiException {
+    if (updates.isEmpty()) {
+      return;
+    }
+    setRequestIds(updates, requestId);
+
+    try {
+      Order order = getOrder(updates, listener);
+      // TODO(dborowitz): Fuse implementations to use a single BatchRefUpdate between phases. Note
+      // that we may still need to respect the order, since op implementations may make assumptions
+      // about the order in which their methods are called.
+      switch (order) {
+        case REPO_BEFORE_DB:
+          for (UnfusedNoteDbBatchUpdate u : updates) {
+            u.executeUpdateRepo();
+          }
+          listener.afterUpdateRepos();
+          for (UnfusedNoteDbBatchUpdate u : updates) {
+            u.executeRefUpdates(dryrun);
+          }
+          listener.afterUpdateRefs();
+          for (UnfusedNoteDbBatchUpdate u : updates) {
+            u.reindexChanges(u.executeChangeOps(dryrun), dryrun);
+          }
+          listener.afterUpdateChanges();
+          break;
+        case DB_BEFORE_REPO:
+          for (UnfusedNoteDbBatchUpdate u : updates) {
+            u.reindexChanges(u.executeChangeOps(dryrun), dryrun);
+          }
+          for (UnfusedNoteDbBatchUpdate u : updates) {
+            u.executeUpdateRepo();
+          }
+          for (UnfusedNoteDbBatchUpdate u : updates) {
+            u.executeRefUpdates(dryrun);
+          }
+          break;
+        default:
+          throw new IllegalStateException("invalid execution order: " + order);
+      }
+
+      ChangeIndexer.allAsList(
+              updates.stream().flatMap(u -> u.indexFutures.stream()).collect(toList()))
+          .get();
+
+      // Fire ref update events only after all mutations are finished, since callers may assume a
+      // patch set ref being created means the change was created, or a branch advancing meaning
+      // some changes were closed.
+      updates
+          .stream()
+          .filter(u -> u.batchRefUpdate != null)
+          .forEach(
+              u -> u.gitRefUpdated.fire(u.project, u.batchRefUpdate, u.getAccount().orElse(null)));
+
+      if (!dryrun) {
+        for (UnfusedNoteDbBatchUpdate u : updates) {
+          u.executePostOps();
+        }
+      }
+    } catch (Exception e) {
+      wrapAndThrowException(e);
+    }
+  }
+
+  class ContextImpl implements Context {
+    @Override
+    public RepoView getRepoView() throws IOException {
+      return UnfusedNoteDbBatchUpdate.this.getRepoView();
+    }
+
+    @Override
+    public RevWalk getRevWalk() throws IOException {
+      return getRepoView().getRevWalk();
+    }
+
+    @Override
+    public Project.NameKey getProject() {
+      return project;
+    }
+
+    @Override
+    public Timestamp getWhen() {
+      return when;
+    }
+
+    @Override
+    public TimeZone getTimeZone() {
+      return tz;
+    }
+
+    @Override
+    public ReviewDb getDb() {
+      return db;
+    }
+
+    @Override
+    public CurrentUser getUser() {
+      return user;
+    }
+
+    @Override
+    public Order getOrder() {
+      return order;
+    }
+  }
+
+  private class RepoContextImpl extends ContextImpl implements RepoContext {
+    @Override
+    public ObjectInserter getInserter() throws IOException {
+      return getRepoView().getInserterWrapper();
+    }
+
+    @Override
+    public void addRefUpdate(ReceiveCommand cmd) throws IOException {
+      getRepoView().getCommands().add(cmd);
+    }
+  }
+
+  private class ChangeContextImpl extends ContextImpl implements ChangeContext {
+    private final ChangeControl ctl;
+    private final Map<PatchSet.Id, ChangeUpdate> updates;
+
+    private boolean deleted;
+
+    protected ChangeContextImpl(ChangeControl ctl) {
+      this.ctl = checkNotNull(ctl);
+      updates = new TreeMap<>(comparing(PatchSet.Id::get));
+    }
+
+    @Override
+    public ChangeUpdate getUpdate(PatchSet.Id psId) {
+      ChangeUpdate u = updates.get(psId);
+      if (u == null) {
+        u = changeUpdateFactory.create(ctl, when);
+        if (newChanges.containsKey(ctl.getId())) {
+          u.setAllowWriteToNewRef(true);
+        }
+        u.setPatchSetId(psId);
+        updates.put(psId, u);
+      }
+      return u;
+    }
+
+    @Override
+    public ChangeControl getControl() {
+      return ctl;
+    }
+
+    @Override
+    public void dontBumpLastUpdatedOn() {
+      // Do nothing; NoteDb effectively updates timestamp if and only if a commit was written to the
+      // change meta ref.
+    }
+
+    @Override
+    public void deleteChange() {
+      deleted = true;
+    }
+  }
+
+  /** Per-change result status from {@link #executeChangeOps}. */
+  private enum ChangeResult {
+    SKIPPED,
+    UPSERTED,
+    DELETED;
+  }
+
+  private final ChangeNotes.Factory changeNotesFactory;
+  private final ChangeControl.GenericFactory changeControlFactory;
+  private final ChangeUpdate.Factory changeUpdateFactory;
+  private final NoteDbUpdateManager.Factory updateManagerFactory;
+  private final ChangeIndexer indexer;
+  private final GitReferenceUpdated gitRefUpdated;
+  private final ReviewDb db;
+
+  private List<CheckedFuture<?, IOException>> indexFutures;
+
+  @Inject
+  UnfusedNoteDbBatchUpdate(
+      GitRepositoryManager repoManager,
+      @GerritPersonIdent PersonIdent serverIdent,
+      ChangeNotes.Factory changeNotesFactory,
+      ChangeControl.GenericFactory changeControlFactory,
+      ChangeUpdate.Factory changeUpdateFactory,
+      NoteDbUpdateManager.Factory updateManagerFactory,
+      ChangeIndexer indexer,
+      GitReferenceUpdated gitRefUpdated,
+      @Assisted ReviewDb db,
+      @Assisted Project.NameKey project,
+      @Assisted CurrentUser user,
+      @Assisted Timestamp when) {
+    super(repoManager, serverIdent, project, user, when);
+    checkArgument(!db.changesTablesEnabled(), "expected Change tables to be disabled on %s", db);
+    this.changeNotesFactory = changeNotesFactory;
+    this.changeControlFactory = changeControlFactory;
+    this.changeUpdateFactory = changeUpdateFactory;
+    this.updateManagerFactory = updateManagerFactory;
+    this.indexer = indexer;
+    this.gitRefUpdated = gitRefUpdated;
+    this.db = db;
+    this.indexFutures = new ArrayList<>();
+  }
+
+  @Override
+  public void execute(BatchUpdateListener listener) throws UpdateException, RestApiException {
+    execute(ImmutableList.of(this), listener, requestId, false);
+  }
+
+  @Override
+  protected Context newContext() {
+    return new ContextImpl();
+  }
+
+  private void executeUpdateRepo() throws UpdateException, RestApiException {
+    try {
+      logDebug("Executing updateRepo on {} ops", ops.size());
+      RepoContextImpl ctx = new RepoContextImpl();
+      for (BatchUpdateOp op : ops.values()) {
+        op.updateRepo(ctx);
+      }
+
+      logDebug("Executing updateRepo on {} RepoOnlyOps", repoOnlyOps.size());
+      for (RepoOnlyOp op : repoOnlyOps) {
+        op.updateRepo(ctx);
+      }
+
+      if (onSubmitValidators != null && !getRefUpdates().isEmpty()) {
+        // Validation of refs has to take place here and not at the beginning of executeRefUpdates.
+        // Otherwise, failing validation in a second BatchUpdate object will happen *after* the
+        // first update's executeRefUpdates has finished, hence after first repo's refs have been
+        // updated, which is too late.
+        onSubmitValidators.validate(
+            project, ctx.getRevWalk().getObjectReader(), repoView.getCommands());
+      }
+
+      // TODO(dborowitz): Don't flush when fusing phases.
+      if (repoView != null) {
+        logDebug("Flushing inserter");
+        repoView.getInserter().flush();
+      } else {
+        logDebug("No objects to flush");
+      }
+    } catch (Exception e) {
+      Throwables.throwIfInstanceOf(e, RestApiException.class);
+      throw new UpdateException(e);
+    }
+  }
+
+  // TODO(dborowitz): Don't execute non-change ref updates separately when fusing phases.
+  private void executeRefUpdates(boolean dryrun) throws IOException, RestApiException {
+    if (getRefUpdates().isEmpty()) {
+      logDebug("No ref updates to execute");
+      return;
+    }
+    // May not be opened if the caller added ref updates but no new objects.
+    initRepository();
+    batchRefUpdate = repoView.getRepository().getRefDatabase().newBatchUpdate();
+    repoView.getCommands().addTo(batchRefUpdate);
+    logDebug("Executing batch of {} ref updates", batchRefUpdate.getCommands().size());
+    if (dryrun) {
+      return;
+    }
+
+    // Force BatchRefUpdate to read newly referenced objects using a new RevWalk, rather than one
+    // that might have access to unflushed objects.
+    try (RevWalk updateRw = new RevWalk(repoView.getRepository())) {
+      batchRefUpdate.execute(updateRw, NullProgressMonitor.INSTANCE);
+    }
+    boolean ok = true;
+    for (ReceiveCommand cmd : batchRefUpdate.getCommands()) {
+      if (cmd.getResult() != ReceiveCommand.Result.OK) {
+        ok = false;
+        break;
+      }
+    }
+    if (!ok) {
+      throw new RestApiException("BatchRefUpdate failed: " + batchRefUpdate);
+    }
+  }
+
+  private Map<Change.Id, ChangeResult> executeChangeOps(boolean dryrun) throws Exception {
+    logDebug("Executing change ops");
+    Map<Change.Id, ChangeResult> result =
+        Maps.newLinkedHashMapWithExpectedSize(ops.keySet().size());
+    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();
+        ObjectReader reader = ins.newReader();
+        RevWalk rw = new RevWalk(reader);
+        NoteDbUpdateManager updateManager =
+            updateManagerFactory
+                .create(project)
+                .setChangeRepo(repo, rw, ins, new ChainedReceiveCommands(repo))) {
+      if (user.isIdentifiedUser()) {
+        updateManager.setRefLogIdent(user.asIdentifiedUser().newRefLogIdent(when, tz));
+      }
+      for (Map.Entry<Change.Id, Collection<BatchUpdateOp>> e : ops.asMap().entrySet()) {
+        Change.Id id = e.getKey();
+        ChangeContextImpl ctx = newChangeContext(id);
+        boolean dirty = false;
+        logDebug("Applying {} ops for change {}", e.getValue().size(), id);
+        for (BatchUpdateOp op : e.getValue()) {
+          dirty |= op.updateChange(ctx);
+        }
+        if (!dirty) {
+          logDebug("No ops reported dirty, short-circuiting");
+          result.put(id, ChangeResult.SKIPPED);
+          continue;
+        }
+        for (ChangeUpdate u : ctx.updates.values()) {
+          updateManager.add(u);
+        }
+        if (ctx.deleted) {
+          logDebug("Change {} was deleted", id);
+          updateManager.deleteChange(id);
+          result.put(id, ChangeResult.DELETED);
+        } else {
+          result.put(id, ChangeResult.UPSERTED);
+        }
+      }
+
+      if (!dryrun) {
+        logDebug("Executing NoteDb updates");
+        updateManager.execute();
+      }
+    }
+    return result;
+  }
+
+  private ChangeContextImpl newChangeContext(Change.Id id) throws OrmException {
+    logDebug("Opening change {} for update", id);
+    Change c = newChanges.get(id);
+    boolean isNew = c != null;
+    if (!isNew) {
+      // Pass a synthetic change into ChangeNotes.Factory, which will take care of checking for
+      // existence and populating columns from the parsed notes state.
+      // TODO(dborowitz): This dance made more sense when using Reviewdb; consider a nicer way.
+      c = ChangeNotes.Factory.newNoteDbOnlyChange(project, id);
+    } else {
+      logDebug("Change {} is new", id);
+    }
+    ChangeNotes notes = changeNotesFactory.createForBatchUpdate(c, !isNew);
+    ChangeControl ctl = changeControlFactory.controlFor(notes, user);
+    return new ChangeContextImpl(ctl);
+  }
+
+  private void reindexChanges(Map<Change.Id, ChangeResult> updateResults, boolean dryrun) {
+    if (dryrun) {
+      return;
+    }
+    logDebug("Reindexing {} changes", updateResults.size());
+    for (Map.Entry<Change.Id, ChangeResult> e : updateResults.entrySet()) {
+      Change.Id id = e.getKey();
+      switch (e.getValue()) {
+        case UPSERTED:
+          indexFutures.add(indexer.indexAsync(project, id));
+          break;
+        case DELETED:
+          indexFutures.add(indexer.deleteAsync(id));
+          break;
+        case SKIPPED:
+          break;
+        default:
+          throw new IllegalStateException("unexpected result: " + e.getValue());
+      }
+    }
+  }
+
+  private void executePostOps() throws Exception {
+    ContextImpl ctx = new ContextImpl();
+    for (BatchUpdateOp op : ops.values()) {
+      op.postUpdate(ctx);
+    }
+
+    for (RepoOnlyOp op : repoOnlyOps) {
+      op.postUpdate(ctx);
+    }
+  }
+}

gerrit-server/src/test/java/com/google/gerrit/server/update/BatchUpdateTest.java

@@ -31,11 +31,13 @@ import com.google.gerrit.server.util.ThreadLocalRequestContext;
 import com.google.gerrit.testutil.InMemoryDatabase;
 import com.google.gerrit.testutil.InMemoryModule;
 import com.google.gerrit.testutil.InMemoryRepositoryManager;
+import com.google.gwtorm.server.SchemaFactory;
 import com.google.inject.Guice;
 import com.google.inject.Inject;
 import com.google.inject.Injector;
 import com.google.inject.Provider;
 import com.google.inject.util.Providers;
+
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.revwalk.RevCommit;
@@ -45,19 +47,16 @@ import org.junit.Test;
 
 public class BatchUpdateTest {
   @Inject private AccountManager accountManager;
-
   @Inject private IdentifiedUser.GenericFactory userFactory;
-
+  @Inject private SchemaFactory<ReviewDb> schemaFactory;
-  @Inject private InMemoryDatabase schemaFactory;
-
   @Inject private InMemoryRepositoryManager repoManager;
-
   @Inject private SchemaCreator schemaCreator;
-
   @Inject private ThreadLocalRequestContext requestContext;
-
   @Inject private BatchUpdate.Factory batchUpdateFactory;
 
+  // Only for use in setting up/tearing down injector; other users should use schemaFactory.
+  @Inject private InMemoryDatabase inMemoryDatabase;
+
   private LifecycleManager lifecycle;
   private ReviewDb db;
   private TestRepository<InMemoryRepository> repo;
@@ -72,8 +71,10 @@ public class BatchUpdateTest {
     lifecycle.add(injector);
     lifecycle.start();
 
+    try (ReviewDb underlyingDb = inMemoryDatabase.getDatabase().open()) {
+      schemaCreator.create(underlyingDb);
+    }
     db = schemaFactory.open();
-    schemaCreator.create(db);
     Account.Id userId = accountManager.authenticate(AuthRequest.forUser("user")).getAccountId();
     user = userFactory.create(userId);
 
@@ -108,7 +109,7 @@ public class BatchUpdateTest {
     if (db != null) {
       db.close();
     }
-    InMemoryDatabase.drop(schemaFactory);
+    InMemoryDatabase.drop(inMemoryDatabase);
   }
 
   @Test

gerrit-server/src/test/java/com/google/gerrit/testutil/GerritServerTests.java

@@ -49,8 +49,10 @@ public class GerritServerTests extends GerritBaseTests {
       };
 
   public void beforeTest() throws Exception {
-    notesMigration = new TestNotesMigration().setFromEnv();
+    notesMigration = new TestNotesMigration();
   }
 
-  public void afterTest() {}
+  public void afterTest() {
+    notesMigration.resetFromEnv();
+  }
 }

gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryRepositoryManager.java

@@ -19,6 +19,8 @@ import com.google.common.collect.Sets;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.RepositoryCaseMismatchException;
+import com.google.gerrit.server.notedb.NotesMigration;
+import com.google.inject.Inject;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.SortedSet;
@@ -30,7 +32,7 @@ import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 /** Repository manager that uses in-memory repositories. */
 public class InMemoryRepositoryManager implements GitRepositoryManager {
   public static InMemoryRepository newRepository(Project.NameKey name) {
-    return new Repo(name);
+    return new Repo(new TestNotesMigration(), name);
   }
 
   public static class Description extends DfsRepositoryDescription {
@@ -49,11 +51,15 @@ public class InMemoryRepositoryManager implements GitRepositoryManager {
   public static class Repo extends InMemoryRepository {
     private String description;
 
-    private Repo(Project.NameKey name) {
+    private Repo(NotesMigration notesMigration, Project.NameKey name) {
       super(new Description(name));
-      // TODO(dborowitz): Allow atomic transactions when this is supported:
+      // Normally, mimic the behavior of JGit FileRepository, the standard Gerrit repository
-      // https://git.eclipse.org/r/#/c/61841/2/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/InMemoryRepository.java@313
+      // backend, and don't support atomic ref updates. The exception is when we're testing with
-      setPerformsAtomicTransactions(false);
+      // fused ref updates, which requires atomic ref updates to function.
+      //
+      // TODO(dborowitz): Change to match the behavior of JGit FileRepository after fixing
+      // https://bugs.eclipse.org/bugs/show_bug.cgi?id=515678
+      setPerformsAtomicTransactions(notesMigration.fuseUpdates());
     }
 
     @Override
@@ -72,7 +78,18 @@ public class InMemoryRepositoryManager implements GitRepositoryManager {
     }
   }
 
-  private Map<String, Repo> repos = new HashMap<>();
+  private final NotesMigration notesMigration;
+  private final Map<String, Repo> repos;
+
+  public InMemoryRepositoryManager() {
+    this(new TestNotesMigration());
+  }
+
+  @Inject
+  InMemoryRepositoryManager(NotesMigration notesMigration) {
+    this.notesMigration = notesMigration;
+    this.repos = new HashMap<>();
+  }
 
   @Override
   public synchronized Repo openRepository(Project.NameKey name) throws RepositoryNotFoundException {
@@ -89,7 +106,7 @@ public class InMemoryRepositoryManager implements GitRepositoryManager {
         throw new RepositoryCaseMismatchException(name);
       }
     } catch (RepositoryNotFoundException e) {
-      repo = new Repo(name);
+      repo = new Repo(notesMigration, name);
       repos.put(normalize(name), repo);
     }
     return repo;

gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbMode.java

@@ -35,6 +35,9 @@ public enum NoteDbMode {
   /** All change tables are entirely disabled. */
   DISABLE_CHANGE_REVIEW_DB(true),
 
+  /** All change tables are entirely disabled, and code/meta ref updates are fused. */
+  FUSED(true),
+
   /**
    * Run tests with NoteDb disabled, then convert ReviewDb to NoteDb and check that the results
    * match.

gerrit-server/src/test/java/com/google/gerrit/testutil/TestNotesMigration.java

@@ -27,9 +27,12 @@ public class TestNotesMigration extends NotesMigration {
   private volatile boolean writeChanges;
   private volatile PrimaryStorage changePrimaryStorage = PrimaryStorage.REVIEW_DB;
   private volatile boolean disableChangeReviewDb;
+  private volatile boolean fuseUpdates;
   private volatile boolean failOnLoad;
 
-  public TestNotesMigration() {}
+  public TestNotesMigration() {
+    resetFromEnv();
+  }
 
   @Override
   public boolean readChanges() {
@@ -53,6 +56,11 @@ public class TestNotesMigration extends NotesMigration {
     return disableChangeReviewDb;
   }
 
+  @Override
+  public boolean fuseUpdates() {
+    return fuseUpdates;
+  }
+
   // Increase visbility from superclass, as tests may want to check whether
   // NoteDb data is written in specific migration scenarios.
   @Override
@@ -85,6 +93,11 @@ public class TestNotesMigration extends NotesMigration {
     return this;
   }
 
+  public TestNotesMigration setFuseUpdates(boolean fuseUpdates) {
+    this.fuseUpdates = fuseUpdates;
+    return this;
+  }
+
   public TestNotesMigration setFailOnLoad(boolean failOnLoad) {
     this.failOnLoad = failOnLoad;
     return this;
@@ -94,31 +107,42 @@ public class TestNotesMigration extends NotesMigration {
     return setReadChanges(enabled).setWriteChanges(enabled);
   }
 
-  public TestNotesMigration setFromEnv() {
+  public TestNotesMigration resetFromEnv() {
     switch (NoteDbMode.get()) {
       case READ_WRITE:
         setWriteChanges(true);
         setReadChanges(true);
         setChangePrimaryStorage(PrimaryStorage.REVIEW_DB);
         setDisableChangeReviewDb(false);
+        setFuseUpdates(false);
         break;
       case WRITE:
         setWriteChanges(true);
         setReadChanges(false);
         setChangePrimaryStorage(PrimaryStorage.REVIEW_DB);
         setDisableChangeReviewDb(false);
+        setFuseUpdates(false);
         break;
       case PRIMARY:
         setWriteChanges(true);
         setReadChanges(true);
         setChangePrimaryStorage(PrimaryStorage.NOTE_DB);
         setDisableChangeReviewDb(false);
+        setFuseUpdates(false);
         break;
       case DISABLE_CHANGE_REVIEW_DB:
         setWriteChanges(true);
         setReadChanges(true);
         setChangePrimaryStorage(PrimaryStorage.NOTE_DB);
         setDisableChangeReviewDb(true);
+        setFuseUpdates(false);
+        break;
+      case FUSED:
+        setWriteChanges(true);
+        setReadChanges(true);
+        setChangePrimaryStorage(PrimaryStorage.NOTE_DB);
+        setDisableChangeReviewDb(true);
+        setFuseUpdates(true);
         break;
       case CHECK:
       case OFF:
@@ -127,6 +151,7 @@ public class TestNotesMigration extends NotesMigration {
         setReadChanges(false);
         setChangePrimaryStorage(PrimaryStorage.REVIEW_DB);
         setDisableChangeReviewDb(false);
+        setFuseUpdates(false);
         break;
     }
     return this;
@@ -137,6 +162,7 @@ public class TestNotesMigration extends NotesMigration {
     setReadChanges(other.readChanges());
     setChangePrimaryStorage(other.changePrimaryStorage());
     setDisableChangeReviewDb(other.disableChangeReviewDb());
+    setFuseUpdates(other.fuseUpdates());
     return this;
   }
 }