Merge changes from topics 'change-rebuilder-tests', 'note-db-update-manager'

* changes:
  NoteDb: Check change message author matches update author
  Create BatchUpdates using InternalUser where appropriate
  NoteDb: Add test for sequence of comment update operations
  Rewrite updating comments in NoteDb
  AbstractChangeUpdate: Set author/committer in superclass
  Support ChangeUpdates from InternalUser
  Start tests for ChangeRebuilder
  Rewrite ChangeRebuilder to better use NoteDbUpdateManager
  Rewrite AbstractChangeUpdate to not extend MetaDataUpdate

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

@@ -0,0 +1,7 @@
+include_defs('//gerrit-acceptance-tests/tests.defs')
+
+acceptance_tests(
+  group = 'server-notedb',
+  srcs = glob(['*IT.java']),
+  labels = ['notedb', 'server'],
+)

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

@@ -0,0 +1,126 @@
+// Copyright (C) 2016 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.acceptance.server.notedb;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.TruthJUnit.assume;
+import static com.google.gerrit.common.TimeUtil.roundToSecond;
+import static com.google.gerrit.testutil.GerritServerTests.isNoteDbTestEnabled;
+
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.notedb.ChangeRebuilder;
+import com.google.inject.Inject;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.Map;
+
+public class ChangeRebuilderIT extends AbstractDaemonTest {
+  @Inject
+  private ChangeRebuilder rebuilder;
+
+  @Before
+  public void setUp() {
+    assume().that(isNoteDbTestEnabled()).isFalse();
+    notesMigration.setAllEnabled(false);
+  }
+
+  @Test
+  public void changeFields() throws Exception {
+    PushOneCommit.Result r = createChange();
+    Change.Id id = r.getPatchSetId().getParentKey();
+    gApi.changes().id(id.get()).topic(name("a-topic"));
+    Change old = db.changes().get(id);
+
+    rebuild(id);
+
+    assertChangeEqual(old, notesFactory.create(db, project, id).getChange());
+  }
+
+  @Test
+  public void patchSets() throws Exception {
+    PushOneCommit.Result r = createChange();
+    Change.Id id = r.getPatchSetId().getParentKey();
+    r = amendChange(r.getChangeId());
+
+    PatchSet ps1 = db.patchSets().get(new PatchSet.Id(id, 1));
+    PatchSet ps2 = db.patchSets().get(new PatchSet.Id(id, 2));
+
+    rebuild(id);
+
+    ChangeNotes notes = notesFactory.create(db, project, id);
+    Map<PatchSet.Id, PatchSet> patchSets = notes.getPatchSets();
+    assertThat(patchSets.keySet()).containsExactly(ps1.getId(), ps2.getId())
+        .inOrder();
+
+    assertPatchSetEqual(ps1, patchSets.get(ps1.getId()));
+    assertPatchSetEqual(ps2, patchSets.get(ps2.getId()));
+  }
+
+  private void rebuild(Change.Id... changeIds) throws Exception {
+    notesMigration.setWriteChanges(true);
+    for (Change.Id id : changeIds) {
+      rebuilder.rebuild(db, id);
+    }
+    notesMigration.setReadChanges(true);
+  }
+
+  private static void assertChangeEqual(Change expectedReviewDb,
+      Change actualNoteDb) {
+    assertThat(actualNoteDb.getId()).isEqualTo(expectedReviewDb.getId());
+    assertThat(actualNoteDb.getKey()).isEqualTo(expectedReviewDb.getKey());
+
+    // TODO(dborowitz): actualNoteDb's timestamps should come from notedb, currently
+    // they're read from reviewdb.
+    assertThat(roundToSecond(actualNoteDb.getCreatedOn()))
+        .isEqualTo(roundToSecond(expectedReviewDb.getCreatedOn()));
+    assertThat(roundToSecond(actualNoteDb.getLastUpdatedOn()))
+        .isEqualTo(roundToSecond(expectedReviewDb.getLastUpdatedOn()));
+    assertThat(actualNoteDb.getOwner()).isEqualTo(expectedReviewDb.getOwner());
+    assertThat(actualNoteDb.getDest()).isEqualTo(expectedReviewDb.getDest());
+    assertThat(actualNoteDb.getStatus())
+        .isEqualTo(expectedReviewDb.getStatus());
+    assertThat(actualNoteDb.currentPatchSetId())
+        .isEqualTo(expectedReviewDb.currentPatchSetId());
+    assertThat(actualNoteDb.getSubject())
+        .isEqualTo(expectedReviewDb.getSubject());
+    assertThat(actualNoteDb.getTopic()).isEqualTo(expectedReviewDb.getTopic());
+    assertThat(actualNoteDb.getOriginalSubject())
+        .isEqualTo(expectedReviewDb.getOriginalSubject());
+    assertThat(actualNoteDb.getSubmissionId())
+        .isEqualTo(expectedReviewDb.getSubmissionId());
+  }
+
+  private static void assertPatchSetEqual(PatchSet expectedReviewDb,
+      PatchSet actualNoteDb) {
+    assertThat(actualNoteDb.getId()).isEqualTo(expectedReviewDb.getId());
+    assertThat(actualNoteDb.getRevision())
+        .isEqualTo(expectedReviewDb.getRevision());
+    assertThat(actualNoteDb.getUploader())
+        .isEqualTo(expectedReviewDb.getUploader());
+    assertThat(actualNoteDb.getCreatedOn())
+        .isEqualTo(roundToSecond(expectedReviewDb.getCreatedOn()));
+    assertThat(actualNoteDb.isDraft()).isEqualTo(expectedReviewDb.isDraft());
+    assertThat(actualNoteDb.getGroups())
+        .isEqualTo(expectedReviewDb.getGroups());
+    assertThat(actualNoteDb.getPushCertificate())
+        .isEqualTo(expectedReviewDb.getPushCertificate());
+  }
+}

gerrit-pgm/src/main/java/com/google/gerrit/pgm/RebuildNotedb.java

@@ -108,49 +108,43 @@ public class RebuildNotedb extends SiteProgram {
     System.out.println("Rebuilding the notedb");
     ChangeRebuilder rebuilder = sysInjector.getInstance(ChangeRebuilder.class);
 
-    Multimap<Project.NameKey, Change> changesByProject = getChangesByProject();
+    Multimap<Project.NameKey, Change.Id> changesByProject =
-    final AtomicBoolean ok = new AtomicBoolean(true);
+        getChangesByProject();
+    AtomicBoolean ok = new AtomicBoolean(true);
     Stopwatch sw = Stopwatch.createStarted();
     GitRepositoryManager repoManager =
         sysInjector.getInstance(GitRepositoryManager.class);
-    final Project.NameKey allUsersName =
+    Project.NameKey allUsersName =
         sysInjector.getInstance(AllUsersName.class);
     try (Repository allUsersRepo =
         repoManager.openMetadataRepository(allUsersName)) {
       deleteRefs(RefNames.REFS_DRAFT_COMMENTS, allUsersRepo);
       deleteRefs(RefNames.REFS_STARRED_CHANGES, allUsersRepo);
-      for (final Project.NameKey project : changesByProject.keySet()) {
+      for (Project.NameKey project : changesByProject.keySet()) {
-        try (Repository repo = repoManager.openMetadataRepository(project)) {
+        try {
-          final BatchRefUpdate bru = repo.getRefDatabase().newBatchUpdate();
-          final BatchRefUpdate bruAllUsers =
-              allUsersRepo.getRefDatabase().newBatchUpdate();
           List<ListenableFuture<?>> futures = Lists.newArrayList();
 
           // Here, we elide the project name to 50 characters to ensure that
           // the whole monitor line for a project fits on one line (<80 chars).
           final MultiProgressMonitor mpm = new MultiProgressMonitor(System.out,
               FormatUtil.elide(project.get(), 50));
-          final Task doneTask =
+          Task doneTask =
               mpm.beginSubTask("done", changesByProject.get(project).size());
-          final Task failedTask =
+          Task failedTask =
               mpm.beginSubTask("failed", MultiProgressMonitor.UNKNOWN);
 
-          for (final Change c : changesByProject.get(project)) {
+          for (Change.Id id : changesByProject.get(project)) {
-            final ListenableFuture<?> future = rebuilder.rebuildAsync(c,
+            ListenableFuture<?> future = rebuilder.rebuildAsync(id, executor);
-                executor, bru, bruAllUsers, repo, allUsersRepo);
             futures.add(future);
             future.addListener(
-                new RebuildListener(c.getId(), future, ok, doneTask, failedTask),
+                new RebuildListener(id, future, ok, doneTask, failedTask),
                 MoreExecutors.directExecutor());
           }
 
           mpm.waitFor(Futures.transformAsync(Futures.successfulAsList(futures),
               new AsyncFunction<List<?>, Void>() {
-                  @Override
+                @Override
-                public ListenableFuture<Void> apply(List<?> input)
+                public ListenableFuture<Void> apply(List<?> input) {
-                    throws Exception {
-                  execute(bru, repo);
-                  execute(bruAllUsers, allUsersRepo);
                   mpm.end();
                   return Futures.immediateFuture(null);
                 }
@@ -218,17 +212,17 @@ public class RebuildNotedb extends SiteProgram {
     }
   }
 
-  private Multimap<Project.NameKey, Change> getChangesByProject()
+  private Multimap<Project.NameKey, Change.Id> getChangesByProject()
       throws OrmException {
     // Memorize all changes so we can close the db connection and allow
     // rebuilder threads to use the full connection pool.
     SchemaFactory<ReviewDb> schemaFactory = sysInjector.getInstance(Key.get(
         new TypeLiteral<SchemaFactory<ReviewDb>>() {}));
-    Multimap<Project.NameKey, Change> changesByProject =
+    Multimap<Project.NameKey, Change.Id> changesByProject =
         ArrayListMultimap.create();
     try (ReviewDb db = schemaFactory.open()) {
       for (Change c : db.changes().all()) {
-        changesByProject.put(c.getProject(), c);
+        changesByProject.put(c.getProject(), c.getId());
       }
       return changesByProject;
     }

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

@@ -14,6 +14,8 @@
 
 package com.google.gerrit.server;
 
+import static com.google.common.base.Preconditions.checkState;
+
 import com.google.common.annotations.VisibleForTesting;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.reviewdb.client.PatchSet;
@@ -27,6 +29,7 @@ import com.google.inject.Singleton;
 
 import java.util.Collections;
 import java.util.List;
+import java.util.Objects;
 
 /**
  * Utility functions to manipulate ChangeMessages.
@@ -68,6 +71,11 @@ public class ChangeMessagesUtil {
 
   public void addChangeMessage(ReviewDb db, ChangeUpdate update,
       ChangeMessage changeMessage) throws OrmException {
+    checkState(
+        Objects.equals(changeMessage.getAuthor(),
+            update.getUser().getAccountId()),
+        "cannot store change message of %s in update of %s",
+        changeMessage.getAuthor(), update.getUser().getAccountId());
     update.setChangeMessage(changeMessage.getMessage());
     db.changeMessages().insert(Collections.singleton(changeMessage));
   }

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

@@ -288,30 +288,14 @@ public class PatchLineCommentsUtil {
     return sort(comments);
   }
 
-  public void insertComments(ReviewDb db, ChangeUpdate update,
+  public void putComments(ReviewDb db, ChangeUpdate update,
       Iterable<PatchLineComment> comments) throws OrmException {
     for (PatchLineComment c : comments) {
-      update.insertComment(c);
+      update.putComment(c);
-    }
-    db.patchComments().insert(comments);
-  }
-
-  public void upsertComments(ReviewDb db, ChangeUpdate update,
-      Iterable<PatchLineComment> comments) throws OrmException {
-    for (PatchLineComment c : comments) {
-      update.upsertComment(c);
     }
     db.patchComments().upsert(comments);
   }
 
-  public void updateComments(ReviewDb db, ChangeUpdate update,
-      Iterable<PatchLineComment> comments) throws OrmException {
-    for (PatchLineComment c : comments) {
-      update.updateComment(c);
-    }
-    db.patchComments().update(comments);
-  }
-
   public void deleteComments(ReviewDb db, ChangeUpdate update,
       Iterable<PatchLineComment> comments) throws OrmException {
     for (PatchLineComment c : comments) {

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

@@ -31,6 +31,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ChangeMessagesUtil;
 import com.google.gerrit.server.ChangeUtil;
+import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.git.BatchUpdate;
 import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
@@ -85,18 +86,19 @@ public class Abandon implements RestModifyView<ChangeResource, AbandonInput>,
     if (!control.canAbandon(dbProvider.get())) {
       throw new AuthException("abandon not permitted");
     }
-    Change change = abandon(control, input.message,
+    Change change = abandon(control, input.message);
-        control.getUser().asIdentifiedUser().getAccount());
     return json.create(ChangeJson.NO_OPTIONS).format(change);
   }
 
-  public Change abandon(ChangeControl control,
+  public Change abandon(ChangeControl control, String msgTxt)
-      final String msgTxt, final Account account)
       throws RestApiException, UpdateException {
+    CurrentUser user = control.getUser();
+    Account account = user.isIdentifiedUser()
+        ? user.asIdentifiedUser().getAccount()
+        : null;
     Op op = new Op(msgTxt, account);
     try (BatchUpdate u = batchUpdateFactory.create(dbProvider.get(),
-        control.getProject().getNameKey(), control.getUser(),
+        control.getProject().getNameKey(), user, TimeUtil.nowTs())) {
-        TimeUtil.nowTs())) {
       u.addOp(control.getId(), op).execute();
     }
     return op.change;

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

@@ -15,7 +15,7 @@
 package com.google.gerrit.server.change;
 
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
-import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.InternalUser;
 import com.google.gerrit.server.config.ChangeCleanupConfig;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.query.QueryParseException;
@@ -37,7 +37,7 @@ public class AbandonUtil {
   private static final Logger log = LoggerFactory.getLogger(AbandonUtil.class);
 
   private final ChangeCleanupConfig cfg;
-  private final IdentifiedUser.GenericFactory identifiedUserFactory;
+  private final InternalUser.Factory internalUserFactory;
   private final QueryProcessor queryProcessor;
   private final ChangeQueryBuilder queryBuilder;
   private final Abandon abandon;
@@ -45,12 +45,12 @@ public class AbandonUtil {
   @Inject
   AbandonUtil(
       ChangeCleanupConfig cfg,
-      IdentifiedUser.GenericFactory identifiedUserFactory,
+      InternalUser.Factory internalUserFactory,
       QueryProcessor queryProcessor,
       ChangeQueryBuilder queryBuilder,
       Abandon abandon) {
     this.cfg = cfg;
-    this.identifiedUserFactory = identifiedUserFactory;
+    this.internalUserFactory = internalUserFactory;
     this.queryProcessor = queryProcessor;
     this.queryBuilder = queryBuilder;
     this.abandon = abandon;
@@ -73,7 +73,7 @@ public class AbandonUtil {
       int count = 0;
       for (ChangeData cd : changesToAbandon) {
         try {
-          abandon.abandon(changeControl(cd), cfg.getAbandonMessage(), null);
+          abandon.abandon(changeControl(cd), cfg.getAbandonMessage());
           count++;
         } catch (ResourceConflictException e) {
           // Change was already merged or abandoned.
@@ -91,7 +91,6 @@ public class AbandonUtil {
   }
 
   private ChangeControl changeControl(ChangeData cd) throws OrmException {
-    return cd.changeControl(
+    return cd.changeControl(internalUserFactory.create());
-        identifiedUserFactory.create(cd.change().getOwner()));
   }
 }

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

@@ -124,7 +124,7 @@ public class CreateDraftComment implements RestModifyView<RevisionResource, Draf
       comment.setRange(in.range);
       setCommentRevId(
           comment, patchListCache, ctx.getChange(), ps);
-      plcUtil.insertComments(
+      plcUtil.putComments(
           ctx.getDb(), ctx.getUpdate(psId), Collections.singleton(comment));
       return true;
     }

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

@@ -476,7 +476,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
       // TODO(dborowitz): Currently doesn't work for PUBLISH_ALL_REVISIONS with
       // notedb.
       plcUtil.deleteComments(ctx.getDb(), u, del);
-      plcUtil.upsertComments(ctx.getDb(), u, ups);
+      plcUtil.putComments(ctx.getDb(), u, ups);
       comments.addAll(ups);
       return !del.isEmpty() || !ups.isEmpty();
     }

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

@@ -142,14 +142,14 @@ public class PutDraftComment implements RestModifyView<DraftCommentResource, Dra
             ctx.getUser().getAccountId(),
             comment.getParentUuid(), ctx.getWhen());
         setCommentRevId(comment, patchListCache, ctx.getChange(), ps);
-        plcUtil.insertComments(ctx.getDb(), update,
+        plcUtil.putComments(ctx.getDb(), update,
             Collections.singleton(update(comment, in)));
       } else {
         if (comment.getRevId() == null) {
           setCommentRevId(
               comment, patchListCache, ctx.getChange(), ps);
         }
-        plcUtil.updateComments(ctx.getDb(), update,
+        plcUtil.putComments(ctx.getDb(), update,
             Collections.singleton(update(comment, in)));
       }
       return true;

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

@@ -36,11 +36,11 @@ import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.PatchLineCommentsUtil;
 import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
-import com.google.gerrit.server.git.VersionedMetaData.BatchMetaDataUpdate;
 import com.google.gerrit.server.index.ChangeIndexer;
 import com.google.gerrit.server.notedb.ChangeDelete;
 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.notedb.NotesMigration;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.InvalidChangeOperationException;
@@ -377,6 +377,7 @@ public class BatchUpdate implements AutoCloseable {
   private final ChangeControl.GenericFactory changeControlFactory;
   private final ChangeNotes.Factory changeNotesFactory;
   private final ChangeUpdate.Factory changeUpdateFactory;
+  private final NoteDbUpdateManager.Factory updateManagerFactory;
   private final GitReferenceUpdated gitRefUpdated;
   private final NotesMigration notesMigration;
   private final PatchLineCommentsUtil plcUtil;
@@ -406,6 +407,7 @@ public class BatchUpdate implements AutoCloseable {
       ChangeControl.GenericFactory changeControlFactory,
       ChangeNotes.Factory changeNotesFactory,
       ChangeUpdate.Factory changeUpdateFactory,
+      NoteDbUpdateManager.Factory updateManagerFactory,
       GitReferenceUpdated gitRefUpdated,
       NotesMigration notesMigration,
       PatchLineCommentsUtil plcUtil,
@@ -420,6 +422,7 @@ public class BatchUpdate implements AutoCloseable {
     this.changeControlFactory = changeControlFactory;
     this.changeNotesFactory = changeNotesFactory;
     this.changeUpdateFactory = changeUpdateFactory;
+    this.updateManagerFactory = updateManagerFactory;
     this.gitRefUpdated = gitRefUpdated;
     this.notesMigration = notesMigration;
     this.plcUtil = plcUtil;
@@ -579,16 +582,12 @@ public class BatchUpdate implements AutoCloseable {
           indexFutures.add(indexer.deleteAsync(id));
         } else {
           if (notesMigration.writeChanges()) {
-            BatchMetaDataUpdate bmdu = null;
+            NoteDbUpdateManager manager =
+                updateManagerFactory.create(ctx.getProject());
             for (ChangeUpdate u : ctx.updates.values()) {
-              if (bmdu == null) {
+              manager.add(u);
-                bmdu = u.openUpdate();
-              }
-              u.writeCommit(bmdu);
-            }
-            if (bmdu != null) {
-              bmdu.commit();
             }
+            manager.execute();
           }
           indexFutures.add(indexer.indexAsync(ctx.getProject(), id));
         }

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

@@ -14,12 +14,17 @@
 
 package com.google.gerrit.server.git;
 
+import static com.google.common.base.MoreObjects.firstNonNull;
 import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.base.Preconditions.checkState;
 
 import org.eclipse.jgit.lib.BatchRefUpdate;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.transport.ReceiveCommand;
 
+import java.io.IOException;
+import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.Map;
 
@@ -33,8 +38,8 @@ import java.util.Map;
  */
 public class ChainedReceiveCommands {
   private final Map<String, ReceiveCommand> commands = new LinkedHashMap<>();
+  private final Map<String, ObjectId> oldIds = new HashMap<>();
 
-  /** @return true if no commands have been added. */
   public boolean isEmpty() {
     return commands.isEmpty();
   }
@@ -64,6 +69,43 @@ public class ChainedReceiveCommands {
         old.getOldId(), cmd.getNewId(), cmd.getRefName()));
   }
 
+  /**
+   * Get the latest value of a ref according to this sequence of commands.
+   * <p>
+   * Once the value for a ref is read once, it is cached in this instance, so
+   * that multiple callers using this instance for lookups see a single
+   * consistent snapshot.
+   *
+   * @param repo repository to read from, if result is not cached.
+   * @param refName name of the ref.
+   * @return value of the ref, taking into account commands that have already
+   *     been added to this instance. Null if the ref is deleted, matching the
+   *     behavior of {@link Repository#exactRef(String)}.
+   */
+  public ObjectId getObjectId(Repository repo, String refName)
+      throws IOException {
+    ReceiveCommand cmd = commands.get(refName);
+    if (cmd != null) {
+      return zeroToNull(cmd.getNewId());
+    }
+    ObjectId old = oldIds.get(refName);
+    if (old != null) {
+      return zeroToNull(old);
+    }
+    Ref ref = repo.exactRef(refName);
+    ObjectId id = ref != null ? ref.getObjectId() : null;
+    // Cache missing ref as zeroId to match value in commands map.
+    oldIds.put(refName, firstNonNull(id, ObjectId.zeroId()));
+    return id;
+  }
+
+  private static ObjectId zeroToNull(ObjectId id) {
+    if (id == null || id.equals(ObjectId.zeroId())) {
+      return null;
+    }
+    return id;
+  }
+
   /**
    * Add commands from this instance to a native JGit batch update.
    * <p>
@@ -74,7 +116,6 @@ public class ChainedReceiveCommands {
    * @param bru batch update
    */
   public void addTo(BatchRefUpdate bru) {
-    checkState(!isEmpty(), "no commands to add");
     for (ReceiveCommand cmd : commands.values()) {
       bru.addCommand(cmd);
     }

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

@@ -52,6 +52,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ChangeMessagesUtil;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.InternalUser;
 import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
 import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
 import com.google.gerrit.server.git.strategy.SubmitStrategy;
@@ -309,7 +310,7 @@ public class MergeOp implements AutoCloseable {
   private final ChangeMessagesUtil cmUtil;
   private final BatchUpdate.Factory batchUpdateFactory;
   private final GitRepositoryManager repoManager;
-  private final IdentifiedUser.GenericFactory identifiedUserFactory;
+  private final InternalUser.Factory internalUserFactory;
   private final MergeSuperSet mergeSuperSet;
   private final MergeValidators.Factory mergeValidatorsFactory;
   private final ProjectCache projectCache;
@@ -341,7 +342,7 @@ public class MergeOp implements AutoCloseable {
   MergeOp(ChangeMessagesUtil cmUtil,
       BatchUpdate.Factory batchUpdateFactory,
       GitRepositoryManager repoManager,
-      IdentifiedUser.GenericFactory identifiedUserFactory,
+      InternalUser.Factory internalUserFactory,
       MergeSuperSet mergeSuperSet,
       MergeValidators.Factory mergeValidatorsFactory,
       ProjectCache projectCache,
@@ -351,7 +352,7 @@ public class MergeOp implements AutoCloseable {
     this.cmUtil = cmUtil;
     this.batchUpdateFactory = batchUpdateFactory;
     this.repoManager = repoManager;
-    this.identifiedUserFactory = identifiedUserFactory;
+    this.internalUserFactory = internalUserFactory;
     this.mergeSuperSet = mergeSuperSet;
     this.mergeValidatorsFactory = mergeValidatorsFactory;
     this.projectCache = projectCache;
@@ -898,10 +899,8 @@ public class MergeOp implements AutoCloseable {
       Project.NameKey destProject) {
     try {
       for (ChangeData cd : internalChangeQuery.byProjectOpen(destProject)) {
-        //TODO: Use InternalUser instead of change owner
         try (BatchUpdate bu = batchUpdateFactory.create(db, destProject,
-            identifiedUserFactory.create(cd.change().getOwner()),
+            internalUserFactory.create(), TimeUtil.nowTs())) {
-            TimeUtil.nowTs())) {
           bu.addOp(cd.getId(), new BatchUpdate.Op() {
             @Override
             public boolean updateChange(ChangeContext ctx) throws OrmException {

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

@@ -20,49 +20,52 @@ import com.google.gerrit.reviewdb.client.Account;
 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.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.InternalUser;
 import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.git.MetaDataUpdate;
-import com.google.gerrit.server.git.VersionedMetaData;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gwtorm.server.OrmException;
 
-import org.eclipse.jgit.errors.ConfigInvalidException;
-import org.eclipse.jgit.lib.BatchRefUpdate;
 import org.eclipse.jgit.lib.CommitBuilder;
+import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.PersonIdent;
-import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
 
 import java.io.IOException;
 import java.util.Date;
 
 /** A single delta related to a specific patch-set of a change. */
-public abstract class AbstractChangeUpdate extends VersionedMetaData {
+public abstract class AbstractChangeUpdate {
   protected final NotesMigration migration;
   protected final GitRepositoryManager repoManager;
-  protected final MetaDataUpdate.User updateFactory;
   protected final ChangeControl ctl;
   protected final String anonymousCowardName;
-  protected final PersonIdent serverIdent;
   protected final Date when;
-  protected PatchSet.Id psId;
+  private final PersonIdent serverIdent;
 
-  AbstractChangeUpdate(NotesMigration migration,
+  protected PatchSet.Id psId;
+  private ObjectId result;
+
+  protected AbstractChangeUpdate(NotesMigration migration,
       GitRepositoryManager repoManager,
-      MetaDataUpdate.User updateFactory, ChangeControl ctl,
+      ChangeControl ctl,
       PersonIdent serverIdent,
       String anonymousCowardName,
       Date when) {
     this.migration = migration;
     this.repoManager = repoManager;
-    this.updateFactory = updateFactory;
     this.ctl = ctl;
     this.serverIdent = serverIdent;
     this.anonymousCowardName = anonymousCowardName;
     this.when = when;
+    checkArgument(
+        (ctl.getUser() instanceof IdentifiedUser)
+            || (ctl.getUser() instanceof InternalUser),
+        "user must be IdentifiedUser or InternalUser: %s", ctl.getUser());
   }
 
   public ChangeNotes getChangeNotes() {
@@ -77,8 +80,8 @@ public abstract class AbstractChangeUpdate extends VersionedMetaData {
     return when;
   }
 
-  public IdentifiedUser getUser() {
+  public CurrentUser getUser() {
-    return ctl.getUser().asIdentifiedUser();
+    return ctl.getUser();
   }
 
   public PatchSet.Id getPatchSetId() {
@@ -90,85 +93,15 @@ public abstract class AbstractChangeUpdate extends VersionedMetaData {
     this.psId = psId;
   }
 
-  private void load() throws IOException {
+  private PersonIdent newAuthorIdent() {
-    if (migration.writeChanges() && getRevision() == null) {
+    CurrentUser u = getUser();
-      try (Repository repo = repoManager.openMetadataRepository(getProjectName())) {
+    if (u instanceof IdentifiedUser) {
-        load(repo);
+      return ChangeNoteUtil.newIdent(u.asIdentifiedUser().getAccount(), when,
-      } catch (ConfigInvalidException e) {
+          serverIdent, anonymousCowardName);
-        throw new IOException(e);
+    } else if (u instanceof InternalUser) {
-      }
+      return serverIdent;
     }
-  }
+    throw new IllegalStateException();
-
-  public void setInserter(ObjectInserter inserter) {
-    this.inserter = inserter;
-  }
-
-  @Override
-  public BatchMetaDataUpdate openUpdate(MetaDataUpdate update) throws IOException {
-    throw new UnsupportedOperationException("use openUpdate()");
-  }
-
-  public BatchMetaDataUpdate openUpdate() throws IOException {
-    return openUpdateInBatch(null);
-  }
-
-  public BatchMetaDataUpdate openUpdateInBatch(BatchRefUpdate bru)
-      throws IOException {
-    if (migration.writeChanges()) {
-      load();
-      Project.NameKey p = getProjectName();
-      MetaDataUpdate md = updateFactory.create(
-          p, repoManager.openMetadataRepository(p), getUser(), bru);
-      md.setAllowEmpty(true);
-      return super.openUpdate(md);
-    }
-    return new BatchMetaDataUpdate() {
-      @Override
-      public void write(CommitBuilder commit) {
-        // Do nothing.
-      }
-
-      @Override
-      public void write(VersionedMetaData config, CommitBuilder commit) {
-        // Do nothing.
-      }
-
-      @Override
-      public RevCommit createRef(String refName) {
-        return null;
-      }
-
-      @Override
-      public void removeRef(String refName) {
-        // Do nothing.
-      }
-
-      @Override
-      public RevCommit commit() {
-        return null;
-      }
-
-      @Override
-      public RevCommit commitAt(ObjectId revision) {
-        return null;
-      }
-
-      @Override
-      public void close() {
-        // Do nothing.
-      }
-    };
-  }
-
-  @Override
-  public RevCommit commit(MetaDataUpdate md) throws IOException {
-    throw new UnsupportedOperationException("use commit()");
-  }
-
-  @Override
-  protected void onLoad() throws IOException, ConfigInvalidException {
-    //Do nothing; just reads the current revision.
   }
 
   protected PersonIdent newIdent(Account author, Date when) {
@@ -176,10 +109,6 @@ public abstract class AbstractChangeUpdate extends VersionedMetaData {
         anonymousCowardName);
   }
 
-  /** Writes commit to a BatchMetaDataUpdate without committing the batch. */
-  public abstract void writeCommit(BatchMetaDataUpdate batch)
-      throws OrmException, IOException;
-
   /** Whether no updates have been done. */
   public abstract boolean isEmpty();
 
@@ -188,4 +117,71 @@ public abstract class AbstractChangeUpdate extends VersionedMetaData {
    *    which is not necessarily the same as the change's project.
    */
   protected abstract Project.NameKey getProjectName();
+
+  protected abstract String getRefName();
+
+  /**
+   * Apply this update to the given inserter.
+   *
+   * @param rw walk for reading back any objects needed for the update.
+   * @param ins inserter to write to; callers should not flush.
+   * @param curr the current tip of the branch prior to this update.
+   * @return commit ID produced by inserting this update's commit, or null if
+   *     this update is a no-op and should be skipped. The zero ID is a valid
+   *     return value, and indicates the ref should be deleted.
+   * @throws OrmException if a Gerrit-level error occurred.
+   * @throws IOException if a lower-level error occurred.
+   */
+  final ObjectId apply(RevWalk rw, ObjectInserter ins, ObjectId curr)
+      throws OrmException, IOException {
+    if (isEmpty()) {
+      return null;
+    }
+    ObjectId z = ObjectId.zeroId();
+    CommitBuilder cb = applyImpl(rw, ins, curr);
+    if (cb == null) {
+      result = z;
+      return z; // Impl intends to delete the ref.
+    }
+    cb.setAuthor(newAuthorIdent());
+    cb.setCommitter(new PersonIdent(serverIdent, when));
+    if (!curr.equals(z)) {
+      cb.setParentId(curr);
+    } else {
+      cb.setParentIds(); // Ref is currently nonexistent, commit has no parents.
+    }
+    if (cb.getTreeId() == null) {
+      if (curr.equals(z)) {
+        cb.setTreeId(emptyTree(ins)); // No parent, assume empty tree.
+      } else {
+        RevCommit p = rw.parseCommit(curr);
+        cb.setTreeId(p.getTree()); // Copy tree from parent.
+      }
+    }
+    result = ins.insert(cb);
+    return result;
+  }
+
+  /**
+   * Create a commit containing the contents of this update.
+   *
+   * @param ins inserter to write to; callers should not flush.
+   * @return a new commit builder representing this commit, or null to indicate
+   *     the meta ref should be deleted as a result of this update. The parent,
+   *     author, and committer fields in the return value are always
+   *     overwritten. The tree ID may be unset by this method, which indicates
+   *     to the caller that it should be copied from the parent commit.
+   * @throws OrmException if a Gerrit-level error occurred.
+   * @throws IOException if a lower-level error occurred.
+   */
+  protected abstract CommitBuilder applyImpl(RevWalk rw, ObjectInserter ins,
+      ObjectId curr) throws OrmException, IOException;
+
+  ObjectId getResult() {
+    return result;
+  }
+
+  private static ObjectId emptyTree(ObjectInserter ins) throws IOException {
+    return ins.insert(Constants.OBJ_TREE, new byte[] {});
+  }
 }

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

@@ -14,16 +14,16 @@
 
 package com.google.gerrit.server.notedb;
 
+import static com.google.common.base.MoreObjects.firstNonNull;
 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 com.google.gerrit.server.notedb.CommentsInNotesUtil.addCommentToMap;
+import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 
-import com.google.common.collect.ListMultimap;
+import com.google.auto.value.AutoValue;
-import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.PatchLineComment;
-import com.google.gerrit.reviewdb.client.PatchLineComment.Status;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.client.RevId;
@@ -32,7 +32,6 @@ import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.config.AnonymousCowardName;
 import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.git.MetaDataUpdate;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.assistedinject.Assisted;
@@ -41,18 +40,17 @@ import com.google.inject.assistedinject.AssistedInject;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.CommitBuilder;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.notes.NoteMap;
-import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
 
 import java.io.IOException;
-import java.util.ArrayList;
 import java.util.Date;
 import java.util.HashMap;
-import java.util.List;
+import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
-import java.util.concurrent.atomic.AtomicBoolean;
 
 /**
  * A single delta to apply atomically to a change.
@@ -68,14 +66,23 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
     ChangeDraftUpdate create(ChangeControl ctl, Date when);
   }
 
+  @AutoValue
+  static abstract class Key {
+    abstract RevId revId();
+    abstract PatchLineComment.Key key();
+  }
+
+  private static Key key(PatchLineComment c) {
+    return new AutoValue_ChangeDraftUpdate_Key(c.getRevId(), c.getKey());
+  }
+
   private final AllUsersName draftsProject;
   private final Account.Id accountId;
   private final CommentsInNotesUtil commentsUtil;
-  private final ChangeNotes changeNotes;
-  private final DraftCommentNotes draftNotes;
 
-  private List<PatchLineComment> upsertComments;
+  // TODO: can go back to a list?
-  private List<PatchLineComment> deleteComments;
+  private Map<Key, PatchLineComment> put;
+  private Set<Key> delete;
 
   @AssistedInject
   private ChangeDraftUpdate(
@@ -83,187 +90,129 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
       @AnonymousCowardName String anonymousCowardName,
       GitRepositoryManager repoManager,
       NotesMigration migration,
-      MetaDataUpdate.User updateFactory,
-      DraftCommentNotes.Factory draftNotesFactory,
       AllUsersName allUsers,
       CommentsInNotesUtil commentsUtil,
       @Assisted ChangeControl ctl,
-      @Assisted Date when) throws OrmException {
+      @Assisted Date when) {
-    super(migration, repoManager, updateFactory, ctl, serverIdent,
+    super(migration, repoManager, ctl, serverIdent, anonymousCowardName, when);
-        anonymousCowardName, when);
     this.draftsProject = allUsers;
     this.commentsUtil = commentsUtil;
     checkState(ctl.getUser().isIdentifiedUser(),
         "Current user must be identified");
     IdentifiedUser user = ctl.getUser().asIdentifiedUser();
     this.accountId = user.getAccountId();
-    this.changeNotes = getChangeNotes().load();
-    this.draftNotes = draftNotesFactory.create(ctl.getId(),
-        user.getAccountId());
 
-    this.upsertComments = Lists.newArrayList();
+    this.put = new HashMap<>();
-    this.deleteComments = Lists.newArrayList();
+    this.delete = new HashSet<>();
   }
 
-  public void insertComment(PatchLineComment c) throws OrmException {
+  public void putComment(PatchLineComment c) {
     verifyComment(c);
-    checkArgument(c.getStatus() == Status.DRAFT,
+    checkArgument(c.getStatus() == PatchLineComment.Status.DRAFT,
         "Cannot insert a published comment into a ChangeDraftUpdate");
-    if (migration.readChanges()) {
+    put.put(key(c), c);
-      checkArgument(!changeNotes.containsComment(c),
-          "A comment already exists with the same key,"
-          + " so the following comment cannot be inserted: %s", c);
-    }
-    upsertComments.add(c);
   }
 
-  public void upsertComment(PatchLineComment c) {
+  public void deleteComment(PatchLineComment c) {
     verifyComment(c);
-    checkArgument(c.getStatus() == Status.DRAFT,
+    delete.add(key(c));
-        "Cannot upsert a published comment into a ChangeDraftUpdate");
-    upsertComments.add(c);
   }
 
-  public void updateComment(PatchLineComment c) throws OrmException {
+  public void deleteComment(RevId revId, PatchLineComment.Key key) {
-    verifyComment(c);
+    delete.add(new AutoValue_ChangeDraftUpdate_Key(revId, key));
-    checkArgument(c.getStatus() == Status.DRAFT,
-        "Cannot update a published comment into a ChangeDraftUpdate");
-    // Here, we check to see if this comment existed previously as a draft.
-    // However, this could cause a race condition if there is a delete and an
-    // update operation happening concurrently (or two deletes) and they both
-    // believe that the comment exists. If a delete happens first, then
-    // the update will fail. However, this is an acceptable risk since the
-    // caller wanted the comment deleted anyways, so the end result will be the
-    // same either way.
-    if (migration.readChanges()) {
-      checkArgument(draftNotes.load().containsComment(c),
-          "Cannot update this comment because it didn't exist previously");
-    }
-    upsertComments.add(c);
-  }
-
-  public void deleteComment(PatchLineComment c) throws OrmException {
-    verifyComment(c);
-    // See the comment above about potential race condition.
-    if (migration.readChanges()) {
-      checkArgument(draftNotes.load().containsComment(c),
-          "Cannot delete this comment because it didn't previously exist as a"
-          + " draft");
-    }
-    if (migration.writeChanges()) {
-      if (draftNotes.load().containsComment(c)) {
-        deleteComments.add(c);
-      }
-    }
-  }
-
-  /**
-   * Deletes a PatchLineComment from the list of drafts only if it existed
-   * previously as a draft. If it wasn't a draft previously, this is a no-op.
-   */
-  public void deleteCommentIfPresent(PatchLineComment c) throws OrmException {
-    if (draftNotes.load().containsComment(c)) {
-      verifyComment(c);
-      deleteComments.add(c);
-    }
   }
 
   private void verifyComment(PatchLineComment comment) {
-    if (migration.writeChanges()) {
-      checkArgument(comment.getRevId() != null);
-    }
     checkArgument(comment.getAuthor().equals(accountId),
         "The author for the following comment does not match the author of"
         + " this ChangeDraftUpdate (%s): %s", accountId, comment);
   }
 
   /** @return the tree id for the updated tree */
-  private ObjectId storeCommentsInNotes(AtomicBoolean removedAllComments)
+  private ObjectId storeCommentsInNotes(RevWalk rw, ObjectInserter ins,
-      throws OrmException, IOException {
+      ObjectId curr) throws ConfigInvalidException, OrmException, IOException {
-    if (isEmpty()) {
+    RevisionNoteMap rnm = getRevisionNoteMap(rw, curr);
-      return null;
+    Set<RevId> updatedRevs =
+        Sets.newHashSetWithExpectedSize(rnm.revisionNotes.size());
+    RevisionNoteBuilder.Cache cache = new RevisionNoteBuilder.Cache(rnm);
+
+    for (PatchLineComment c : put.values()) {
+      if (!delete.contains(key(c))) {
+        cache.get(c.getRevId()).putComment(c);
+      }
+    }
+    for (Key k : delete) {
+      cache.get(k.revId()).deleteComment(k.key());
     }
 
-    NoteMap noteMap = draftNotes.load().getNoteMap();
+    Map<RevId, RevisionNoteBuilder> builders = cache.getBuilders();
-    if (noteMap == null) {
-      noteMap = NoteMap.newEmptyMap();
-    }
-
-    Map<RevId, List<PatchLineComment>> allComments = new HashMap<>();
-
     boolean hasComments = false;
-    int n = deleteComments.size() + upsertComments.size();
+    for (Map.Entry<RevId, RevisionNoteBuilder> e : builders.entrySet()) {
-    Set<RevId> updatedRevs = Sets.newHashSetWithExpectedSize(n);
+      updatedRevs.add(e.getKey());
-    Set<PatchLineComment.Key> updatedKeys = Sets.newHashSetWithExpectedSize(n);
+      ObjectId id = ObjectId.fromString(e.getKey().get());
-    for (PatchLineComment c : deleteComments) {
+      byte[] data = e.getValue().build(commentsUtil);
-      allComments.put(c.getRevId(), new ArrayList<PatchLineComment>());
+      if (data.length == 0) {
-      updatedRevs.add(c.getRevId());
+        rnm.noteMap.remove(id);
-      updatedKeys.add(c.getKey());
+      } else {
-    }
-
-    for (PatchLineComment c : upsertComments) {
-      hasComments = true;
-      addCommentToMap(allComments, c);
-      updatedRevs.add(c.getRevId());
-      updatedKeys.add(c.getKey());
-    }
-
-    // Re-add old comments for updated revisions so the new note contents
-    // includes both old and new comments merged in the right order.
-    //
-    // writeCommentsToNoteMap doesn't touch notes for SHA-1s that are not
-    // mentioned in the input map, so by omitting comments for those revisions,
-    // we avoid the work of having to re-serialize identical comment data for
-    // those revisions.
-    ListMultimap<RevId, PatchLineComment> existing =
-        draftNotes.getComments();
-    for (Map.Entry<RevId, PatchLineComment> e : existing.entries()) {
-      PatchLineComment c = e.getValue();
-      if (updatedRevs.contains(c.getRevId())
-          && !updatedKeys.contains(c.getKey())) {
         hasComments = true;
-        addCommentToMap(allComments, e.getValue());
+        ObjectId dataBlob = ins.insert(OBJ_BLOB, data);
+        rnm.noteMap.set(id, dataBlob);
       }
     }
 
-    // If we touched every revision and there are no comments left, set the flag
+    // If we touched every revision and there are no comments left, tell the
-    // for the caller to delete the entire ref.
+    // caller to delete the entire ref.
-    boolean touchedAllRevs = updatedRevs.equals(existing.keySet());
+    boolean touchedAllRevs = updatedRevs.equals(rnm.revisionNotes.keySet());
     if (touchedAllRevs && !hasComments) {
-      removedAllComments.set(touchedAllRevs && !hasComments);
       return null;
     }
 
-    commentsUtil.writeCommentsToNoteMap(noteMap, allComments, inserter);
+    return rnm.noteMap.writeTree(ins);
-    return noteMap.writeTree(inserter);
   }
 
-  public RevCommit commit() throws IOException {
+  private RevisionNoteMap getRevisionNoteMap(RevWalk rw, ObjectId curr)
-    BatchMetaDataUpdate batch = openUpdate();
+      throws ConfigInvalidException, OrmException, IOException {
-    try {
+    if (migration.readChanges()) {
-      writeCommit(batch);
+      // If reading from changes is enabled, then the old DraftCommentNotes
-      return batch.commit();
+      // already parsed the revision notes. We can reuse them as long as the ref
-    } catch (OrmException e) {
+      // hasn't advanced.
-      throw new IOException(e);
+      DraftCommentNotes draftNotes =
-    } finally {
+          ctl.getNotes().load().getDraftCommentNotes();
-      batch.close();
+      if (draftNotes != null) {
+        ObjectId idFromNotes =
+            firstNonNull(draftNotes.getRevision(), ObjectId.zeroId());
+        if (idFromNotes.equals(curr)) {
+          return checkNotNull(ctl.getNotes().revisionNoteMap);
+        }
+      }
     }
+    NoteMap noteMap;
+    if (!curr.equals(ObjectId.zeroId())) {
+      noteMap = NoteMap.read(rw.getObjectReader(), rw.parseCommit(curr));
+    } else {
+      noteMap = NoteMap.newEmptyMap();
+    }
+    // Even though reading from changes might not be enabled, we need to
+    // parse any existing revision notes so we can merge them.
+    return RevisionNoteMap.parse(
+        ctl.getId(), rw.getObjectReader(), noteMap, true);
   }
 
   @Override
-  public void writeCommit(BatchMetaDataUpdate batch)
+  protected CommitBuilder applyImpl(RevWalk rw, ObjectInserter ins,
-      throws OrmException, IOException {
+      ObjectId curr) throws OrmException, IOException {
-    CommitBuilder builder = new CommitBuilder();
+    CommitBuilder cb = new CommitBuilder();
-    if (migration.writeChanges()) {
+    cb.setMessage("Update draft comments");
-      AtomicBoolean removedAllComments = new AtomicBoolean();
+    try {
-      ObjectId treeId = storeCommentsInNotes(removedAllComments);
+      ObjectId treeId = storeCommentsInNotes(rw, ins, curr);
-      if (removedAllComments.get()) {
+      if (treeId == null) {
-        batch.removeRef(getRefName());
+        return null; // Delete ref.
-      } else if (treeId != null) {
-        builder.setTreeId(treeId);
-        batch.write(builder);
       }
+      cb.setTreeId(checkNotNull(treeId));
+    } catch (ConfigInvalidException e) {
+      throw new OrmException(e);
     }
+    return cb;
   }
 
   @Override
@@ -276,21 +225,9 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
     return RefNames.refsDraftComments(accountId, ctl.getId());
   }
 
-  @Override
-  protected boolean onSave(CommitBuilder commit) throws IOException,
-      ConfigInvalidException {
-    if (isEmpty()) {
-      return false;
-    }
-    commit.setAuthor(newIdent(getUser().getAccount(), when));
-    commit.setCommitter(new PersonIdent(serverIdent, when));
-    commit.setMessage("Update draft comments");
-    return true;
-  }
-
   @Override
   public boolean isEmpty() {
-    return deleteComments.isEmpty()
+    return delete.isEmpty()
-        && upsertComments.isEmpty();
+        && put.isEmpty();
   }
 }

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

@@ -33,6 +33,8 @@ import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.ListMultimap;
+import com.google.common.collect.Multimap;
+import com.google.common.collect.Multimaps;
 import com.google.common.collect.Ordering;
 import com.google.common.primitives.Ints;
 import com.google.common.util.concurrent.AsyncFunction;
@@ -70,7 +72,6 @@ import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.notes.NoteMap;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -398,10 +399,9 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
   private ImmutableListMultimap<RevId, PatchLineComment> comments;
   private ImmutableSet<String> hashtags;
 
-  // Mutable note map state, only used by ChangeUpdate to make in-place editing
+  // Parsed note map state, used by ChangeUpdate to make in-place editing of
-  // of notes easier.
+  // notes easier.
-  NoteMap noteMap;
+  RevisionNoteMap revisionNoteMap;
-  Map<RevId, RevisionNote> revisionNotes;
 
   private final AllUsersName allUsers;
   private DraftCommentNotes draftCommentNotes;
@@ -477,7 +477,25 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
   public ImmutableListMultimap<RevId, PatchLineComment> getDraftComments(
       Account.Id author) throws OrmException {
     loadDraftComments(author);
-    return draftCommentNotes.getComments();
+    final Multimap<RevId, PatchLineComment> published = comments;
+    // Filter out any draft comments that also exist in the published map, in
+    // case the update to All-Users to delete them during the publish operation
+    // failed.
+    Multimap<RevId, PatchLineComment> filtered = Multimaps.filterEntries(
+        draftCommentNotes.getComments(),
+        new Predicate<Map.Entry<RevId, PatchLineComment>>() {
+          @Override
+          public boolean apply(Map.Entry<RevId, PatchLineComment> in) {
+            for (PatchLineComment c : published.get(in.getKey())) {
+              if (c.getKey().equals(in.getValue().getKey())) {
+                return false;
+              }
+            }
+            return true;
+          }
+        });
+    return ImmutableListMultimap.copyOf(
+        filtered);
   }
 
   /**
@@ -518,15 +536,6 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     return false;
   }
 
-  /** @return the NoteMap */
-  NoteMap getNoteMap() {
-    return noteMap;
-  }
-
-  Map<RevId, RevisionNote> getRevisionNotes() {
-    return revisionNotes;
-  }
-
   @Override
   protected String getRefName() {
     return ChangeNoteUtil.changeRefName(getChangeId());
@@ -557,8 +566,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
       changeMessagesByPatchSet = parser.buildMessagesByPatchSet();
       allChangeMessages = parser.buildAllMessages();
       comments = ImmutableListMultimap.copyOf(parser.comments);
-      noteMap = parser.noteMap;
+      revisionNoteMap = parser.revisionNoteMap;
-      revisionNotes = parser.revisionNotes;
       change.setKey(new Change.Key(parser.changeId));
       change.setDest(new Branch.NameKey(project, parser.branch));
       change.setTopic(Strings.emptyToNull(parser.topic));

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

@@ -69,7 +69,6 @@ import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectReader;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.notes.Note;
 import org.eclipse.jgit.notes.NoteMap;
 import org.eclipse.jgit.revwalk.FooterKey;
 import org.eclipse.jgit.revwalk.RevCommit;
@@ -101,7 +100,6 @@ class ChangeNotesParser implements AutoCloseable {
   final Multimap<RevId, PatchLineComment> comments;
   final TreeMap<PatchSet.Id, PatchSet> patchSets;
   final Map<PatchSet.Id, PatchSetState> patchSetStates;
-  final Map<RevId, RevisionNote> revisionNotes;
 
   String branch;
   Change.Status status;
@@ -115,7 +113,7 @@ class ChangeNotesParser implements AutoCloseable {
   String originalSubject;
   String submissionId;
   PatchSet.Id currentPatchSetId;
-  NoteMap noteMap;
+  RevisionNoteMap revisionNoteMap;
 
   private final Change.Id id;
   private final ObjectId tip;
@@ -142,7 +140,6 @@ class ChangeNotesParser implements AutoCloseable {
     comments = ArrayListMultimap.create();
     patchSets = Maps.newTreeMap(ReviewDbUtil.intKeyOrdering());
     patchSetStates = Maps.newHashMap();
-    revisionNotes = Maps.newHashMap();
   }
 
   @Override
@@ -217,7 +214,9 @@ class ChangeNotesParser implements AutoCloseable {
     }
 
     Account.Id accountId = parseIdent(commit);
-    ownerId = accountId;
+    if (accountId != null) {
+      ownerId = accountId;
+    }
 
     if (changeId == null) {
       changeId = parseChangeId(commit);
@@ -339,6 +338,10 @@ class ChangeNotesParser implements AutoCloseable {
 
   private void parsePatchSet(PatchSet.Id psId, ObjectId rev,
       Account.Id accountId, Timestamp ts) throws ConfigInvalidException {
+    if (accountId == null) {
+      throw parseException(
+          "patch set %s requires an identified user as uploader", psId.get());
+    }
     PatchSet ps = patchSets.get(psId);
     if (ps == null) {
       ps = new PatchSet(psId);
@@ -497,19 +500,18 @@ class ChangeNotesParser implements AutoCloseable {
       throws IOException, ConfigInvalidException {
     ObjectReader reader = walk.getObjectReader();
     RevCommit tipCommit = walk.parseCommit(tip);
-    noteMap = NoteMap.read(reader, tipCommit);
+    revisionNoteMap = RevisionNoteMap.parse(
+        id, reader, NoteMap.read(reader, tipCommit), false);
+    Map<RevId, RevisionNote> rns = revisionNoteMap.revisionNotes;
 
-    for (Note note : noteMap) {
+    for (Map.Entry<RevId, RevisionNote> e : rns.entrySet()) {
-      RevisionNote rn = new RevisionNote(id, reader, note.getData());
+      for (PatchLineComment plc : e.getValue().comments) {
-      RevId revId = new RevId(note.name());
+        comments.put(e.getKey(), plc);
-      revisionNotes.put(revId, rn);
-      for (PatchLineComment plc : rn.comments) {
-        comments.put(revId, plc);
       }
     }
 
     for (PatchSet ps : patchSets.values()) {
-      RevisionNote rn = revisionNotes.get(ps.getRevision());
+      RevisionNote rn = rns.get(ps.getRevision());
       if (rn != null && rn.pushCert != null) {
         ps.setPushCertificate(rn.pushCert);
       }
@@ -518,6 +520,10 @@ class ChangeNotesParser implements AutoCloseable {
 
   private void parseApproval(PatchSet.Id psId, Account.Id accountId,
       Timestamp ts, String line) throws ConfigInvalidException {
+    if (accountId == null) {
+      throw parseException(
+          "patch set %s requires an identified user as uploader", psId.get());
+    }
     if (line.startsWith("-")) {
       parseRemoveApproval(psId, accountId, line);
     } else {
@@ -665,6 +671,14 @@ class ChangeNotesParser implements AutoCloseable {
 
   private Account.Id parseIdent(RevCommit commit)
       throws ConfigInvalidException {
+    // Check if the author name/email is the same as the committer name/email,
+    // i.e. was the server ident at the time this commit was made.
+    PersonIdent a = commit.getAuthorIdent();
+    PersonIdent c = commit.getCommitterIdent();
+    if (a.getName().equals(c.getName())
+        && a.getEmailAddress().equals(c.getEmailAddress())) {
+      return null;
+    }
     return parseIdent(commit.getAuthorIdent());
   }
 

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

@@ -40,22 +40,23 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.client.StarredChange;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.InternalUser;
 import com.google.gerrit.server.PatchLineCommentsUtil;
-import com.google.gerrit.server.git.VersionedMetaData.BatchMetaDataUpdate;
+import com.google.gerrit.server.git.ChainedReceiveCommands;
+import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.patch.PatchListCache;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gwtorm.server.OrmException;
+import com.google.gwtorm.server.SchemaFactory;
 import com.google.inject.Inject;
-import com.google.inject.Provider;
+import com.google.inject.util.Providers;
 
-import org.eclipse.jgit.lib.BatchRefUpdate;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
-import org.eclipse.jgit.lib.Ref;
-import org.eclipse.jgit.lib.RefUpdate;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
@@ -78,61 +79,75 @@ public class ChangeRebuilder {
   private static final long TS_WINDOW_MS =
       TimeUnit.MILLISECONDS.convert(1, TimeUnit.SECONDS);
 
-  private final Provider<ReviewDb> dbProvider;
+  private final SchemaFactory<ReviewDb> schemaFactory;
+  private final GitRepositoryManager repoManager;
   private final ChangeControl.GenericFactory controlFactory;
   private final IdentifiedUser.GenericFactory userFactory;
+  private final InternalUser.Factory internalUserFactory;
   private final PatchListCache patchListCache;
   private final ChangeUpdate.Factory updateFactory;
   private final ChangeDraftUpdate.Factory draftUpdateFactory;
+  private final NoteDbUpdateManager.Factory updateManagerFactory;
 
   @Inject
-  ChangeRebuilder(Provider<ReviewDb> dbProvider,
+  ChangeRebuilder(SchemaFactory<ReviewDb> schemaFactory,
+      GitRepositoryManager repoManager,
       ChangeControl.GenericFactory controlFactory,
       IdentifiedUser.GenericFactory userFactory,
+      InternalUser.Factory internalUserFactory,
       PatchListCache patchListCache,
       ChangeUpdate.Factory updateFactory,
-      ChangeDraftUpdate.Factory draftUpdateFactory) {
+      ChangeDraftUpdate.Factory draftUpdateFactory,
-    this.dbProvider = dbProvider;
+      NoteDbUpdateManager.Factory updateManagerFactory) {
+    this.schemaFactory = schemaFactory;
+    this.repoManager = repoManager;
     this.controlFactory = controlFactory;
     this.userFactory = userFactory;
+    this.internalUserFactory = internalUserFactory;
     this.patchListCache = patchListCache;
     this.updateFactory = updateFactory;
     this.draftUpdateFactory = draftUpdateFactory;
+    this.updateManagerFactory = updateManagerFactory;
   }
 
-  public ListenableFuture<?> rebuildAsync(final Change change,
+  public ListenableFuture<?> rebuildAsync(final Change.Id id,
-      ListeningExecutorService executor, final BatchRefUpdate bru,
+      ListeningExecutorService executor) {
-      final BatchRefUpdate bruForDrafts, final Repository changeRepo,
-      final Repository allUsersRepo) {
     return executor.submit(new Callable<Void>() {
         @Override
       public Void call() throws Exception {
-        rebuild(change, bru, bruForDrafts, changeRepo, allUsersRepo);
+        try (ReviewDb db = schemaFactory.open()) {
+          rebuild(db, id);
+        }
         return null;
       }
     });
   }
 
-  public void rebuild(Change change, BatchRefUpdate bru,
+  public void rebuild(ReviewDb db, Change.Id changeId)
-      BatchRefUpdate bruAllUsers, Repository changeRepo,
+      throws NoSuchChangeException, IOException, OrmException {
-      Repository allUsersRepo) throws NoSuchChangeException, IOException,
+    Change change = db.changes().get(changeId);
-      OrmException {
+    if (change == null) {
-    ReviewDb db = dbProvider.get();
+      return;
-    Change.Id changeId = change.getId();
+    }
+    NoteDbUpdateManager manager =
+        updateManagerFactory.create(change.getProject());
+
     // We will rebuild all events, except for draft comments, in buckets based
-    // on author and timestamp. However, all draft comments for a given change
+    // on author and timestamp.
-    // and author will be written as one commit in the notedb.
     List<Event> events = Lists.newArrayList();
     Multimap<Account.Id, PatchLineCommentEvent> draftCommentEvents =
         ArrayListMultimap.create();
 
-    try (RevWalk rw = new RevWalk(changeRepo)) {
+    Repository changeMetaRepo = manager.getChangeRepo();
-      events.addAll(getHashtagsEvents(change, changeRepo, rw));
+    events.addAll(getHashtagsEvents(change, manager));
 
-      deleteRef(change, changeRepo);
+    // Delete ref only after hashtags have been read
+    deleteRef(change, changeMetaRepo, manager.getChangeCommands());
 
+    try (Repository codeRepo = repoManager.openRepository(change.getProject());
+        RevWalk codeRw = new RevWalk(codeRepo)) {
       for (PatchSet ps : db.patchSets().byChange(changeId)) {
-        events.add(new PatchSetEvent(change, ps, rw));
+        events.add(new PatchSetEvent(change, ps, codeRw));
         for (PatchLineComment c : db.patchComments().byPatchSet(ps.getId())) {
           PatchLineCommentEvent e =
               new PatchLineCommentEvent(c, change, ps, patchListCache);
@@ -156,63 +171,51 @@ public class ChangeRebuilder {
 
     Collections.sort(events);
     events.add(new FinalUpdatesEvent(change, notedbChange));
-    BatchMetaDataUpdate batch = null;
     ChangeUpdate update = null;
     for (Event e : events) {
       if (!sameUpdate(e, update)) {
-        writeToBatch(batch, update, changeRepo);
+        if (update != null) {
-        IdentifiedUser user = userFactory.create(dbProvider, e.who);
+          manager.add(update);
+        }
+        CurrentUser user = e.who != null
+            ? userFactory.create(Providers.of(db), e.who)
+            : internalUserFactory.create();
         update = updateFactory.create(
             controlFactory.controlFor(db, change, user), e.when);
         update.setPatchSetId(e.psId);
-        if (batch == null) {
-          batch = update.openUpdateInBatch(bru);
-        }
       }
       e.apply(update);
     }
-    if (batch != null) {
+    manager.add(update);
-      writeToBatch(batch, update, changeRepo);
-
-      // Since the BatchMetaDataUpdates generated by all ChangeRebuilders on a
-      // given project are backed by the same BatchRefUpdate, we need to
-      // synchronize on the BatchRefUpdate. Therefore, since commit on a
-      // BatchMetaDataUpdate is the only method that modifies a BatchRefUpdate,
-      // we can just synchronize this call.
-      synchronized (bru) {
-        batch.commit();
-      }
-    }
 
     for (Account.Id author : draftCommentEvents.keys()) {
-      IdentifiedUser user = userFactory.create(dbProvider, author);
+      IdentifiedUser user = userFactory.create(Providers.of(db), author);
       ChangeDraftUpdate draftUpdate = null;
-      BatchMetaDataUpdate batchForDrafts = null;
       for (PatchLineCommentEvent e : draftCommentEvents.get(author)) {
-        if (draftUpdate == null) {
+        if (!sameUpdate(e, draftUpdate)) {
+          if (draftUpdate != null) {
+            manager.add(draftUpdate);
+          }
           draftUpdate = draftUpdateFactory.create(
               controlFactory.controlFor(db, change, user), e.when);
           draftUpdate.setPatchSetId(e.psId);
-          batchForDrafts = draftUpdate.openUpdateInBatch(bruAllUsers);
         }
         e.applyDraft(draftUpdate);
       }
-      writeToBatch(batchForDrafts, draftUpdate, allUsersRepo);
+      manager.add(draftUpdate);
-      synchronized(bruAllUsers) {
-        batchForDrafts.commit();
-      }
     }
 
-    createStarredChangesRefs(changeId, bruAllUsers, allUsersRepo);
+    createStarredChangesRefs(db, changeId, manager.getAllUsersCommands(),
+        manager.getAllUsersRepo());
+    manager.execute();
   }
 
-  private void createStarredChangesRefs(Change.Id changeId,
+  private void createStarredChangesRefs(ReviewDb db, Change.Id changeId,
-      BatchRefUpdate bruAllUsers, Repository allUsersRepo)
+      ChainedReceiveCommands allUsersCmds, Repository allUsersRepo)
           throws IOException, OrmException {
     ObjectId emptyTree = emptyTree(allUsersRepo);
-    for (StarredChange starred : dbProvider.get().starredChanges()
+    for (StarredChange starred : db.starredChanges().byChange(changeId)) {
-        .byChange(changeId)) {
+      allUsersCmds.add(new ReceiveCommand(ObjectId.zeroId(), emptyTree,
-      bruAllUsers.addCommand(new ReceiveCommand(ObjectId.zeroId(), emptyTree,
           RefNames.refsStarredChanges(starred.getAccountId(), changeId)));
     }
   }
@@ -226,16 +229,18 @@ public class ChangeRebuilder {
   }
 
   private List<HashtagsEvent> getHashtagsEvents(Change change,
-      Repository changeRepo, RevWalk rw) throws IOException {
+      NoteDbUpdateManager manager) throws IOException {
     String refName = ChangeNoteUtil.changeRefName(change.getId());
-    Ref ref = changeRepo.exactRef(refName);
+    ObjectId old = manager.getChangeCommands()
-    if (ref == null) {
+        .getObjectId(manager.getChangeRepo(), refName);
+    if (old == null) {
       return Collections.emptyList();
     }
 
+    RevWalk rw = manager.getChangeRevWalk();
     List<HashtagsEvent> events = new ArrayList<>();
     rw.reset();
-    rw.markStart(rw.parseCommit(ref.getObjectId()));
+    rw.markStart(rw.parseCommit(old));
     for (RevCommit commit : rw) {
       Account.Id authorId =
           ChangeNoteUtil.parseIdent(commit.getAuthorIdent());
@@ -277,40 +282,12 @@ public class ChangeRebuilder {
     return new PatchSet.Id(change.getId(), psId);
   }
 
-  private void deleteRef(Change change, Repository changeRepo)
+  private void deleteRef(Change change, Repository repo,
-      throws IOException {
+      ChainedReceiveCommands cmds) throws IOException {
     String refName = ChangeNoteUtil.changeRefName(change.getId());
-    RefUpdate ru = changeRepo.updateRef(refName, true);
+    ObjectId old = cmds.getObjectId(repo, refName);
-    ru.setForceUpdate(true);
+    if (old != null) {
-    RefUpdate.Result result = ru.delete();
+      cmds.add(new ReceiveCommand(old, ObjectId.zeroId(), refName));
-    switch (result) {
-      case FORCED:
-      case NEW:
-      case NO_CHANGE:
-        break;
-      case FAST_FORWARD:
-      case IO_FAILURE:
-      case LOCK_FAILURE:
-      case NOT_ATTEMPTED:
-      case REJECTED:
-      case REJECTED_CURRENT_BRANCH:
-      case RENAMED:
-      default:
-        throw new IOException(
-            String.format("Failed to delete ref %s: %s", refName, result));
-    }
-  }
-
-  private void writeToBatch(BatchMetaDataUpdate batch,
-      AbstractChangeUpdate update, Repository repo) throws IOException,
-      OrmException {
-    if (update == null || update.isEmpty()) {
-      return;
-    }
-
-    try (ObjectInserter inserter = repo.newObjectInserter()) {
-      update.setInserter(inserter);
-      update.writeCommit(batch);
     }
   }
 
@@ -318,7 +295,7 @@ public class ChangeRebuilder {
     return when.getTime() / TS_WINDOW_MS;
   }
 
-  private static boolean sameUpdate(Event event, ChangeUpdate update) {
+  private static boolean sameUpdate(Event event, AbstractChangeUpdate update) {
     return update != null
         && round(event.when) == round(update.getWhen())
         && event.who.equals(update.getUser().getAccountId())
@@ -441,14 +418,14 @@ public class ChangeRebuilder {
       if (c.getRevId() == null) {
         setCommentRevId(c, cache, change, ps);
       }
-      update.insertComment(c);
+      update.putComment(c);
     }
 
     void applyDraft(ChangeDraftUpdate draftUpdate) throws OrmException {
       if (c.getRevId() == null) {
         setCommentRevId(c, cache, change, ps);
       }
-      draftUpdate.insertComment(c);
+      draftUpdate.putComment(c);
     }
   }
 

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

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.notedb;
 
+import static com.google.common.base.MoreObjects.firstNonNull;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
@@ -46,14 +47,12 @@ import com.google.gerrit.common.data.SubmitRecord;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchLineComment;
-import com.google.gerrit.reviewdb.client.PatchLineComment.Status;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.config.AnonymousCowardName;
 import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.git.MetaDataUpdate;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.util.LabelVote;
@@ -61,8 +60,10 @@ import com.google.gwtorm.server.OrmException;
 import com.google.inject.assistedinject.Assisted;
 import com.google.inject.assistedinject.AssistedInject;
 
+import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.CommitBuilder;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.notes.NoteMap;
 import org.eclipse.jgit.revwalk.FooterKey;
@@ -72,7 +73,7 @@ import org.eclipse.jgit.revwalk.RevWalk;
 import java.io.IOException;
 import java.util.Comparator;
 import java.util.Date;
-import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -99,27 +100,29 @@ public class ChangeUpdate extends AbstractChangeUpdate {
   }
 
   private final AccountCache accountCache;
-  private String commitSubject;
+  private final CommentsInNotesUtil commentsUtil;
-  private String subject;
+  private final ChangeDraftUpdate.Factory draftUpdateFactory;
+  private final NoteDbUpdateManager.Factory updateManagerFactory;
+
   private final Table<String, Account.Id, Optional<Short>> approvals;
   private final Map<Account.Id, ReviewerStateInternal> reviewers;
+
+  private String commitSubject;
+  private String subject;
   private String changeId;
   private String branch;
   private Change.Status status;
   private List<SubmitRecord> submitRecords;
   private String submissionId;
-  private final CommentsInNotesUtil commentsUtil;
   private List<PatchLineComment> comments;
   private String topic;
   private ObjectId commit;
   private Set<String> hashtags;
   private String changeMessage;
-  private ChangeNotes notes;
   private PatchSetState psState;
   private Iterable<String> groups;
   private String pushCert;
 
-  private final ChangeDraftUpdate.Factory draftUpdateFactory;
   private ChangeDraftUpdate draftUpdate;
 
   @AssistedInject
@@ -129,13 +132,13 @@ public class ChangeUpdate extends AbstractChangeUpdate {
       GitRepositoryManager repoManager,
       NotesMigration migration,
       AccountCache accountCache,
-      MetaDataUpdate.User updateFactory,
+      NoteDbUpdateManager.Factory updateManagerFactory,
       ChangeDraftUpdate.Factory draftUpdateFactory,
       ProjectCache projectCache,
       @Assisted ChangeControl ctl,
       CommentsInNotesUtil commentsUtil) {
     this(serverIdent, anonymousCowardName, repoManager, migration, accountCache,
-        updateFactory, draftUpdateFactory,
+        updateManagerFactory, draftUpdateFactory,
         projectCache, ctl, serverIdent.getWhen(), commentsUtil);
   }
 
@@ -146,14 +149,14 @@ public class ChangeUpdate extends AbstractChangeUpdate {
       GitRepositoryManager repoManager,
       NotesMigration migration,
       AccountCache accountCache,
-      MetaDataUpdate.User updateFactory,
+      NoteDbUpdateManager.Factory updateManagerFactory,
       ChangeDraftUpdate.Factory draftUpdateFactory,
       ProjectCache projectCache,
       @Assisted ChangeControl ctl,
       @Assisted Date when,
       CommentsInNotesUtil commentsUtil) {
     this(serverIdent, anonymousCowardName, repoManager, migration, accountCache,
-        updateFactory, draftUpdateFactory, ctl,
+        updateManagerFactory, draftUpdateFactory, ctl,
         when,
         projectCache.get(getProjectName(ctl)).getLabelTypes().nameComparator(),
         commentsUtil);
@@ -170,17 +173,19 @@ public class ChangeUpdate extends AbstractChangeUpdate {
       GitRepositoryManager repoManager,
       NotesMigration migration,
       AccountCache accountCache,
-      MetaDataUpdate.User updateFactory,
+      NoteDbUpdateManager.Factory updateManagerFactory,
       ChangeDraftUpdate.Factory draftUpdateFactory,
       @Assisted ChangeControl ctl,
       @Assisted Date when,
       @Assisted Comparator<String> labelNameComparator,
       CommentsInNotesUtil commentsUtil) {
-    super(migration, repoManager, updateFactory, ctl, serverIdent,
+    super(migration, repoManager, ctl, serverIdent,
         anonymousCowardName, when);
-    this.draftUpdateFactory = draftUpdateFactory;
     this.accountCache = accountCache;
     this.commentsUtil = commentsUtil;
+    this.draftUpdateFactory = draftUpdateFactory;
+    this.updateManagerFactory = updateManagerFactory;
+
     this.approvals = TreeBasedTable.create(
         labelNameComparator,
         Ordering.natural().onResultOf(new Function<Account.Id, Integer>() {
@@ -193,13 +198,19 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     this.comments = Lists.newArrayList();
   }
 
-  public void setChangeId(String changeId) throws OrmException {
+  public ObjectId commit() throws IOException, OrmException {
-    if (notes == null) {
+    NoteDbUpdateManager updateManager =
-      notes = getChangeNotes().load();
+        updateManagerFactory.create(getProjectName());
-    }
+    updateManager.add(this);
-    checkArgument(notes.getChange().getKey().get().equals(changeId),
+    updateManager.execute();
+    return getResult();
+  }
+
+  public void setChangeId(String changeId) {
+    String old = ctl.getChange().getKey().get();
+    checkArgument(old.equals(changeId),
         "The Change-Id was already set to %s, so we cannot set this Change-Id: %s",
-        notes.getChange().getKey(), changeId);
+        old, changeId);
     this.changeId = changeId;
   }
 
@@ -258,120 +269,41 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     this.changeMessage = changeMessage;
   }
 
-  public void insertComment(PatchLineComment comment) throws OrmException {
+  public void putComment(PatchLineComment c) {
-    if (comment.getStatus() == Status.DRAFT) {
-      insertDraftComment(comment);
-    } else {
-      insertPublishedComment(comment);
-    }
-  }
-
-  public void upsertComment(PatchLineComment comment) throws OrmException {
-    if (comment.getStatus() == Status.DRAFT) {
-      upsertDraftComment(comment);
-    } else {
-      deleteDraftCommentIfPresent(comment);
-      upsertPublishedComment(comment);
-    }
-  }
-
-  public void updateComment(PatchLineComment comment) throws OrmException {
-    if (comment.getStatus() == Status.DRAFT) {
-      updateDraftComment(comment);
-    } else {
-      deleteDraftCommentIfPresent(comment);
-      updatePublishedComment(comment);
-    }
-  }
-
-  public void deleteComment(PatchLineComment comment) throws OrmException {
-    if (comment.getStatus() == Status.DRAFT) {
-      deleteDraftComment(comment);
-    } else {
-      throw new IllegalArgumentException("Cannot delete a published comment.");
-    }
-  }
-
-  private void insertPublishedComment(PatchLineComment c) throws OrmException {
     verifyComment(c);
-    if (notes == null) {
-      notes = getChangeNotes().load();
-    }
-    if (migration.readChanges()) {
-      checkArgument(!notes.containsComment(c),
-          "A comment already exists with the same key as the following comment,"
-          + " so we cannot insert this comment: %s", c);
-    }
-    comments.add(c);
-  }
-
-  private void insertDraftComment(PatchLineComment c) throws OrmException {
     createDraftUpdateIfNull();
-    draftUpdate.insertComment(c);
+    if (c.getStatus() == PatchLineComment.Status.DRAFT) {
+      draftUpdate.putComment(c);
+    } else {
+      comments.add(c);
+      // Always delete the corresponding comment from drafts. Published comments
+      // are immutable, meaning in normal operation we only hit this path when
+      // publishing a comment. It's exactly in that case that we have to delete
+      // the draft.
+      draftUpdate.deleteComment(c);
+    }
   }
 
-  private void upsertPublishedComment(PatchLineComment c) throws OrmException {
+  public void deleteComment(PatchLineComment c) {
     verifyComment(c);
-    if (notes == null) {
+    if (c.getStatus() == PatchLineComment.Status.DRAFT) {
-      notes = getChangeNotes().load();
+      createDraftUpdateIfNull().deleteComment(c);
+    } else {
+      throw new IllegalArgumentException(
+          "Cannot delete published comment " + c);
     }
-    // This could allow callers to update a published comment if migration.write
-    // is on and migration.readComments is off because we will not be able to
-    // verify that the comment didn't already exist as a published comment
-    // since we don't have a ReviewDb.
-    if (migration.readChanges()) {
-      checkArgument(!notes.containsCommentPublished(c),
-          "Cannot update a comment that has already been published and saved");
-    }
-    comments.add(c);
   }
 
-  private void upsertDraftComment(PatchLineComment c) {
+  @VisibleForTesting
-    createDraftUpdateIfNull();
+  ChangeDraftUpdate createDraftUpdateIfNull() {
-    draftUpdate.upsertComment(c);
-  }
-
-  private void updatePublishedComment(PatchLineComment c) throws OrmException {
-    verifyComment(c);
-    if (notes == null) {
-      notes = getChangeNotes().load();
-    }
-    // See comment above in upsertPublishedComment() about potential risk with
-    // this check.
-    if (migration.readChanges()) {
-      checkArgument(!notes.containsCommentPublished(c),
-          "Cannot update a comment that has already been published and saved");
-    }
-    comments.add(c);
-  }
-
-  private void updateDraftComment(PatchLineComment c) throws OrmException {
-    createDraftUpdateIfNull();
-    draftUpdate.updateComment(c);
-  }
-
-  private void deleteDraftComment(PatchLineComment c) throws OrmException {
-    createDraftUpdateIfNull();
-    draftUpdate.deleteComment(c);
-  }
-
-  private void deleteDraftCommentIfPresent(PatchLineComment c)
-      throws OrmException {
-    createDraftUpdateIfNull();
-    draftUpdate.deleteCommentIfPresent(c);
-  }
-
-  private void createDraftUpdateIfNull() {
     if (draftUpdate == null) {
       draftUpdate = draftUpdateFactory.create(ctl, when);
     }
+    return draftUpdate;
   }
 
   private void verifyComment(PatchLineComment c) {
     checkArgument(c.getRevId() != null);
-    checkArgument(c.getStatus() == Status.PUBLISHED,
-        "Cannot add a draft comment to a ChangeUpdate. Use a ChangeDraftUpdate"
-        + " for draft comments");
     checkArgument(c.getAuthor().equals(getUser().getAccountId()),
         "The author for the following comment does not match the author of"
         + " this ChangeDraftUpdate (%s): %s", getUser().getAccountId(), c);
@@ -418,71 +350,92 @@ public class ChangeUpdate extends AbstractChangeUpdate {
   }
 
   /** @return the tree id for the updated tree */
-  private ObjectId storeRevisionNotes() throws OrmException, IOException {
+  private ObjectId storeRevisionNotes(RevWalk rw, ObjectInserter inserter,
-    ChangeNotes notes = ctl.getNotes().load();
+      ObjectId curr) throws ConfigInvalidException, OrmException, IOException {
-    NoteMap noteMap = notes.getNoteMap();
-    if (noteMap == null) {
-      noteMap = NoteMap.newEmptyMap();
-    }
     if (comments.isEmpty() && pushCert == null) {
       return null;
     }
+    RevisionNoteMap rnm = getRevisionNoteMap(rw, curr);
 
-    Map<RevId, RevisionNoteBuilder> builders = new HashMap<>();
+    RevisionNoteBuilder.Cache cache = new RevisionNoteBuilder.Cache(rnm);
     for (PatchLineComment c : comments) {
-      builder(builders, c.getRevId()).addComment(c);
+      cache.get(c.getRevId()).putComment(c);
     }
     if (pushCert != null) {
       checkState(commit != null);
-      builder(builders, new RevId(commit.name())).setPushCertificate(pushCert);
+      cache.get(new RevId(commit.name())).setPushCertificate(pushCert);
     }
+    Map<RevId, RevisionNoteBuilder> builders = cache.getBuilders();
+    checkComments(rnm.revisionNotes, builders);
 
     for (Map.Entry<RevId, RevisionNoteBuilder> e : builders.entrySet()) {
       ObjectId data = inserter.insert(
           OBJ_BLOB, e.getValue().build(commentsUtil));
-      noteMap.set(ObjectId.fromString(e.getKey().get()), data);
+      rnm.noteMap.set(ObjectId.fromString(e.getKey().get()), data);
     }
 
-    return noteMap.writeTree(inserter);
+    return rnm.noteMap.writeTree(inserter);
   }
 
-  private RevisionNoteBuilder builder(Map<RevId, RevisionNoteBuilder> builders,
+  private RevisionNoteMap getRevisionNoteMap(RevWalk rw, ObjectId curr)
-      RevId revId) {
+      throws ConfigInvalidException, OrmException, IOException {
-    RevisionNoteBuilder b = builders.get(revId);
+    if (migration.readChanges()) {
-    if (b == null) {
+      // If reading from changes is enabled, then the old ChangeNotes already
-      b = new RevisionNoteBuilder(
+      // parsed the revision notes. We can reuse them as long as the ref hasn't
-          getChangeNotes().getRevisionNotes().get(revId));
+      // advanced.
-      builders.put(revId, b);
+      ObjectId idFromNotes =
-    }
+          firstNonNull(ctl.getNotes().load().getRevision(), ObjectId.zeroId());
-    return b;
+      if (idFromNotes.equals(curr)) {
-  }
+        return checkNotNull(ctl.getNotes().revisionNoteMap);
-
-  public RevCommit commit() throws IOException {
-    BatchMetaDataUpdate batch = openUpdate();
-    try {
-      writeCommit(batch);
-      RevCommit c = batch.commit();
-      return c;
-    } catch (OrmException e) {
-      throw new IOException(e);
-    } finally {
-      batch.close();
-    }
-  }
-
-  @Override
-  public void writeCommit(BatchMetaDataUpdate batch) throws OrmException,
-      IOException {
-    CommitBuilder builder = new CommitBuilder();
-    if (migration.writeChanges()) {
-      ObjectId treeId = storeRevisionNotes();
-      if (treeId != null) {
-        builder.setTreeId(treeId);
       }
     }
-    batch.write(this, builder);
+    NoteMap noteMap;
-    if (draftUpdate != null) {
+    if (!curr.equals(ObjectId.zeroId())) {
-      draftUpdate.commit();
+      noteMap = NoteMap.read(rw.getObjectReader(), rw.parseCommit(curr));
+    } else {
+      noteMap = NoteMap.newEmptyMap();
+    }
+    // Even though reading from changes might not be enabled, we need to
+    // parse any existing revision notes so we can merge them.
+    return RevisionNoteMap.parse(
+        ctl.getId(), rw.getObjectReader(), noteMap, false);
+  }
+
+  private void checkComments(Map<RevId, RevisionNote> existingNotes,
+      Map<RevId, RevisionNoteBuilder> toUpdate) throws OrmException {
+    // Prohibit various kinds of illegal operations on comments.
+    Set<PatchLineComment.Key> existing = new HashSet<>();
+    for (RevisionNote rn : existingNotes.values()) {
+      for (PatchLineComment c : rn.comments) {
+        existing.add(c.getKey());
+        if (draftUpdate != null) {
+          // Take advantage of an existing update on All-Users to prune any
+          // published comments from drafts. NoteDbUpdateManager takes care of
+          // ensuring that this update is applied before its dependent draft
+          // update.
+          //
+          // Deleting aggressively in this way, combined with filtering out
+          // duplicate published/draft comments in ChangeNotes#getDraftComments,
+          // makes up for the fact that updates between the change repo and
+          // All-Users are not atomic.
+          //
+          // TODO(dborowitz): We might want to distinguish between deleted
+          // drafts that we're fixing up after the fact by putting them in a
+          // separate commit. But note that we don't care much about the commit
+          // graph of the draft ref, particularly because the ref is completely
+          // deleted when all drafts are gone.
+          draftUpdate.deleteComment(c.getRevId(), c.getKey());
+        }
+      }
+    }
+
+    for (RevisionNoteBuilder b : toUpdate.values()) {
+      for (PatchLineComment c : b.put.values()) {
+        if (existing.contains(c.getKey())) {
+          throw new OrmException(
+              "Cannot update existing published comment: " + c);
+        }
+      }
     }
   }
 
@@ -492,12 +445,9 @@ public class ChangeUpdate extends AbstractChangeUpdate {
   }
 
   @Override
-  protected boolean onSave(CommitBuilder cb) {
+  protected CommitBuilder applyImpl(RevWalk rw, ObjectInserter ins,
-    if (getRevision() != null && isEmpty()) {
+      ObjectId curr) throws OrmException, IOException {
-      return false;
+    CommitBuilder cb = new CommitBuilder();
-    }
-    cb.setAuthor(newIdent(getUser().getAccount(), when));
-    cb.setCommitter(new PersonIdent(serverIdent, when));
 
     int ps = psId != null ? psId.get() : getChange().currentPatchSetId().get();
     StringBuilder msg = new StringBuilder();
@@ -599,7 +549,15 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     }
 
     cb.setMessage(msg.toString());
-    return true;
+    try {
+      ObjectId treeId = storeRevisionNotes(rw, ins, curr);
+      if (treeId != null) {
+        cb.setTreeId(treeId);
+      }
+    } catch (ConfigInvalidException e) {
+      throw new OrmException(e);
+    }
+    return cb;
   }
 
   private void addPatchSetFooter(StringBuilder sb, int ps) {
@@ -634,6 +592,10 @@ public class ChangeUpdate extends AbstractChangeUpdate {
         && groups == null;
   }
 
+  ChangeDraftUpdate getDraftUpdate() {
+    return draftUpdate;
+  }
+
   private static StringBuilder addFooter(StringBuilder sb, FooterKey footer) {
     return sb.append(footer.getName()).append(": ");
   }

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

@@ -15,16 +15,12 @@
 package com.google.gerrit.server.notedb;
 
 import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.gerrit.server.PatchLineCommentsUtil.PLC_ORDER;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.GERRIT_PLACEHOLDER_HOST;
 import static com.google.gerrit.server.notedb.ChangeNotes.parseException;
-import static com.google.gerrit.server.notedb.RevisionNote.MAX_NOTE_SZ;
 import static java.nio.charset.StandardCharsets.UTF_8;
-import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
-import com.google.common.collect.Multimap;
 import com.google.common.primitives.Ints;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
@@ -42,16 +38,7 @@ import com.google.inject.Inject;
 import com.google.inject.Singleton;
 
 import org.eclipse.jgit.errors.ConfigInvalidException;
-import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.lib.ObjectInserter;
-import org.eclipse.jgit.lib.ObjectReader;
 import org.eclipse.jgit.lib.PersonIdent;
-import org.eclipse.jgit.lib.Ref;
-import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.notes.Note;
-import org.eclipse.jgit.notes.NoteMap;
-import org.eclipse.jgit.revwalk.RevCommit;
-import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.util.GitDateFormatter;
 import org.eclipse.jgit.util.GitDateFormatter.Format;
 import org.eclipse.jgit.util.GitDateParser;
@@ -60,18 +47,14 @@ import org.eclipse.jgit.util.QuotedString;
 import org.eclipse.jgit.util.RawParseUtils;
 
 import java.io.ByteArrayOutputStream;
-import java.io.IOException;
 import java.io.OutputStream;
 import java.io.OutputStreamWriter;
 import java.io.PrintWriter;
 import java.sql.Timestamp;
 import java.text.ParseException;
-import java.util.ArrayList;
-import java.util.Collections;
 import java.util.Date;
 import java.util.List;
 import java.util.Locale;
-import java.util.Map;
 
 /**
  * Utility functions to parse PatchLineComments out of a note byte array and
@@ -89,33 +72,6 @@ public class CommentsInNotesUtil {
   private static final String REVISION = "Revision";
   private static final String UUID = "UUID";
 
-  public static NoteMap parseCommentsFromNotes(Repository repo, String refName,
-      RevWalk walk, Change.Id changeId,
-      Multimap<RevId, PatchLineComment> comments,
-      Status status)
-      throws IOException, ConfigInvalidException {
-    Ref ref = repo.getRefDatabase().exactRef(refName);
-    if (ref == null) {
-      return null;
-    }
-
-    ObjectReader reader = walk.getObjectReader();
-    RevCommit commit = walk.parseCommit(ref.getObjectId());
-    NoteMap noteMap = NoteMap.read(reader, commit);
-
-    for (Note note : noteMap) {
-      byte[] bytes =
-          reader.open(note.getData(), OBJ_BLOB).getCachedBytes(MAX_NOTE_SZ);
-      List<PatchLineComment> result =
-          parseNote(bytes, new MutableInteger(), changeId, status);
-      if (result == null || result.isEmpty()) {
-        continue;
-      }
-      comments.putAll(new RevId(note.name()), result);
-    }
-    return noteMap;
-  }
-
   public static List<PatchLineComment> parseNote(byte[] note, MutableInteger p,
       Change.Id changeId, Status status) throws ConfigInvalidException {
     if (p.value >= note.length) {
@@ -432,7 +388,7 @@ public class CommentsInNotesUtil {
    *            same side and must share the same patch set ID.
    * @param out output stream to write to.
    */
-  public void buildNote(List<PatchLineComment> comments, OutputStream out) {
+  void buildNote(List<PatchLineComment> comments, OutputStream out) {
     if (comments.isEmpty()) {
       return;
     }
@@ -514,51 +470,4 @@ public class CommentsInNotesUtil {
       }
     }
   }
-
-  /**
-   * Write comments for multiple revisions to a note map.
-   * <p>
-   * Mutates the map in-place. only notes for SHA-1s found as keys in the map
-   * are modified; all other notes are left untouched.
-   *
-   * @param noteMap note map to modify.
-   * @param allComments map of revision to all comments for that revision;
-   *     callers are responsible for reading the original comments and applying
-   *     any changes. Differs from a multimap in that present-but-empty values
-   *     are significant, and indicate the note for that SHA-1 should be
-   *     deleted.
-   * @param inserter object inserter for writing notes.
-   * @throws IOException if an error occurred.
-   */
-  public void writeCommentsToNoteMap(NoteMap noteMap,
-      Map<RevId, List<PatchLineComment>> allComments, ObjectInserter inserter)
-      throws IOException {
-    for (Map.Entry<RevId, List<PatchLineComment>> e : allComments.entrySet()) {
-      List<PatchLineComment> comments = e.getValue();
-      ObjectId commit = ObjectId.fromString(e.getKey().get());
-      if (comments.isEmpty()) {
-        noteMap.remove(commit);
-        continue;
-      }
-      Collections.sort(comments, PLC_ORDER);
-      // We allow comments for multiple commits to be written in the same
-      // update, even though the rest of the metadata update is associated with
-      // a single patch set.
-      byte[] note = buildNote(comments);
-      if (note != null && note.length > 0) {
-        noteMap.set(commit, inserter.insert(OBJ_BLOB, note));
-      }
-    }
-  }
-
-  static void addCommentToMap(Map<RevId, List<PatchLineComment>> map,
-      PatchLineComment c) {
-    List<PatchLineComment> list = map.get(c.getRevId());
-    if (list == null) {
-      list = new ArrayList<>();
-      map.put(c.getRevId(), list);
-    }
-    list.add(c);
-  }
-
 }

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

@@ -15,7 +15,9 @@
 package com.google.gerrit.server.notedb;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ImmutableListMultimap;
+import com.google.common.collect.Multimap;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchLineComment;
@@ -31,6 +33,7 @@ import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.CommitBuilder;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.notes.NoteMap;
+import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
 
 import java.io.IOException;
@@ -66,7 +69,7 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
   private final Account.Id author;
 
   private ImmutableListMultimap<RevId, PatchLineComment> comments;
-  private NoteMap noteMap;
+  private RevisionNoteMap revisionNoteMap;
 
   DraftCommentNotes(GitRepositoryManager repoManager, NotesMigration migration,
       AllUsersName draftsProject, Change.Id changeId, Account.Id author) {
@@ -75,8 +78,8 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
     this.author = author;
   }
 
-  public NoteMap getNoteMap() {
+  RevisionNoteMap getRevisionNoteMap() {
-    return noteMap;
+    return revisionNoteMap;
   }
 
   public Account.Id getAuthor() {
@@ -110,13 +113,17 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
       return;
     }
 
-    try (RevWalk walk = new RevWalk(reader);
+    try (RevWalk walk = new RevWalk(reader)) {
-        DraftCommentNotesParser parser = new DraftCommentNotesParser(
+      RevCommit tipCommit = walk.parseCommit(rev);
-          getChangeId(), walk, rev, repoManager, draftsProject, author)) {
+      revisionNoteMap = RevisionNoteMap.parse(
-      parser.parseDraftComments();
+          getChangeId(), reader, NoteMap.read(reader, tipCommit), true);
-
+      Multimap<RevId, PatchLineComment> cs = ArrayListMultimap.create();
-      comments = ImmutableListMultimap.copyOf(parser.comments);
+      for (RevisionNote rn : revisionNoteMap.revisionNotes.values()) {
-      noteMap = parser.noteMap;
+        for (PatchLineComment c : rn.comments) {
+          cs.put(c.getRevId(), c);
+        }
+      }
+      comments = ImmutableListMultimap.copyOf(cs);
     }
   }
 
@@ -136,4 +143,9 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
   public Project.NameKey getProjectName() {
     return draftsProject;
   }
+
+  @VisibleForTesting
+  NoteMap getNoteMap() {
+    return revisionNoteMap != null ? revisionNoteMap.noteMap : null;
+  }
 }

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

@@ -1,69 +0,0 @@
-// Copyright (C) 2014 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.notedb;
-
-import com.google.common.collect.ArrayListMultimap;
-import com.google.common.collect.Multimap;
-import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.Change;
-import com.google.gerrit.reviewdb.client.PatchLineComment;
-import com.google.gerrit.reviewdb.client.RefNames;
-import com.google.gerrit.reviewdb.client.RevId;
-import com.google.gerrit.server.config.AllUsersName;
-import com.google.gerrit.server.git.GitRepositoryManager;
-
-import org.eclipse.jgit.errors.ConfigInvalidException;
-import org.eclipse.jgit.errors.RepositoryNotFoundException;
-import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.notes.NoteMap;
-import org.eclipse.jgit.revwalk.RevWalk;
-
-import java.io.IOException;
-
-class DraftCommentNotesParser implements AutoCloseable {
-  final Multimap<RevId, PatchLineComment> comments;
-  NoteMap noteMap;
-
-  private final Change.Id changeId;
-  private final ObjectId tip;
-  private final RevWalk walk;
-  private final Repository repo;
-  private final Account.Id author;
-
-  DraftCommentNotesParser(Change.Id changeId, RevWalk walk, ObjectId tip,
-      GitRepositoryManager repoManager, AllUsersName draftsProject,
-      Account.Id author) throws RepositoryNotFoundException, IOException {
-    this.changeId = changeId;
-    this.walk = walk;
-    this.tip = tip;
-    this.repo = repoManager.openMetadataRepository(draftsProject);
-    this.author = author;
-
-    comments = ArrayListMultimap.create();
-  }
-
-  @Override
-  public void close() {
-    repo.close();
-  }
-
-  void parseDraftComments() throws IOException, ConfigInvalidException {
-    walk.markStart(walk.parseCommit(tip));
-    noteMap = CommentsInNotesUtil.parseCommentsFromNotes(repo,
-        RefNames.refsDraftComments(author, changeId),
-        walk, changeId, comments, PatchLineComment.Status.DRAFT);
-  }
-}

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

@@ -21,5 +21,6 @@ public class NoteDbModule extends FactoryModule {
   public void configure() {
     factory(ChangeUpdate.Factory.class);
     factory(ChangeDraftUpdate.Factory.class);
+    factory(NoteDbUpdateManager.Factory.class);
   }
 }

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

@@ -0,0 +1,265 @@
+// Copyright (C) 2016 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.notedb;
+
+import static com.google.common.base.MoreObjects.firstNonNull;
+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 com.google.common.collect.ArrayListMultimap;
+import com.google.common.collect.ListMultimap;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.git.ChainedReceiveCommands;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.assistedinject.Assisted;
+import com.google.inject.assistedinject.AssistedInject;
+
+import org.eclipse.jgit.lib.BatchRefUpdate;
+import org.eclipse.jgit.lib.NullProgressMonitor;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectInserter;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.transport.ReceiveCommand;
+
+import java.io.IOException;
+
+public class NoteDbUpdateManager {
+  public interface Factory {
+    NoteDbUpdateManager create(Project.NameKey projectName);
+  }
+
+  private static class OpenRepo implements AutoCloseable {
+    final Repository repo;
+    final RevWalk rw;
+    final ObjectInserter ins;
+    final ChainedReceiveCommands cmds;
+    final boolean close;
+
+    OpenRepo(Repository repo, RevWalk rw, ObjectInserter ins,
+        ChainedReceiveCommands cmds, boolean close) {
+      this.repo = checkNotNull(repo);
+      this.rw = checkNotNull(rw);
+      this.ins = checkNotNull(ins);
+      this.cmds = checkNotNull(cmds);
+      this.close = close;
+    }
+
+    @Override
+    public void close() {
+      if (close) {
+        ins.close();
+        rw.close();
+        repo.close();
+      }
+    }
+  }
+
+  private final GitRepositoryManager repoManager;
+  private final NotesMigration migration;
+  private final AllUsersName allUsersName;
+  private final Project.NameKey projectName;
+  private final ListMultimap<String, ChangeUpdate> changeUpdates;
+  private final ListMultimap<String, ChangeDraftUpdate> draftUpdates;
+
+  private OpenRepo changeRepo;
+  private OpenRepo allUsersRepo;
+
+  @AssistedInject
+  NoteDbUpdateManager(GitRepositoryManager repoManager,
+      NotesMigration migration,
+      AllUsersName allUsersName,
+      @Assisted Project.NameKey projectName) {
+    this.repoManager = repoManager;
+    this.migration = migration;
+    this.allUsersName = allUsersName;
+    this.projectName = projectName;
+    changeUpdates = ArrayListMultimap.create();
+    draftUpdates = ArrayListMultimap.create();
+  }
+
+  public NoteDbUpdateManager setChangeRepo(Repository repo, RevWalk rw,
+      ObjectInserter ins, ChainedReceiveCommands cmds) {
+    checkState(changeRepo == null, "change repo already initialized");
+    changeRepo = new OpenRepo(repo, rw, ins, cmds, false);
+    return this;
+  }
+
+  Repository getChangeRepo() throws IOException {
+    initChangeRepo();
+    return changeRepo.repo;
+  }
+
+  RevWalk getChangeRevWalk() throws IOException {
+    initChangeRepo();
+    return changeRepo.rw;
+  }
+
+  ChainedReceiveCommands getChangeCommands() throws IOException {
+    initChangeRepo();
+    return changeRepo.cmds;
+  }
+
+  public NoteDbUpdateManager setAllUsersRepo(Repository repo, RevWalk rw,
+      ObjectInserter ins, ChainedReceiveCommands cmds) {
+    checkState(allUsersRepo == null, "allUsers repo already initialized");
+    allUsersRepo = new OpenRepo(repo, rw, ins, cmds, false);
+    return this;
+  }
+
+  Repository getAllUsersRepo() throws IOException {
+    initAllUsersRepo();
+    return allUsersRepo.repo;
+  }
+
+  ChainedReceiveCommands getAllUsersCommands() throws IOException {
+    initAllUsersRepo();
+    return allUsersRepo.cmds;
+  }
+
+  private void initChangeRepo() throws IOException {
+    if (changeRepo == null) {
+      changeRepo = openRepo(projectName);
+    }
+  }
+
+  private void initAllUsersRepo() throws IOException {
+    if (allUsersRepo == null) {
+      allUsersRepo = openRepo(allUsersName);
+    }
+  }
+
+  private OpenRepo openRepo(Project.NameKey p) throws IOException {
+    Repository repo = repoManager.openMetadataRepository(p);
+    ObjectInserter ins = repo.newObjectInserter();
+    return new OpenRepo(repo, new RevWalk(ins.newReader()), ins,
+        new ChainedReceiveCommands(), true);
+  }
+
+  private boolean isEmpty() {
+    if (!migration.writeChanges()) {
+      return true;
+    }
+    return changeUpdates.isEmpty()
+        && draftUpdates.isEmpty();
+  }
+
+  /**
+   * Add an update to the list of updates to execute.
+   * <p>
+   * Updates should only be added to the manager after all mutations have been
+   * made, as this method may eagerly access the update.
+   *
+   * @param update the update to add.
+   */
+  public void add(ChangeUpdate update) {
+    checkArgument(update.getProjectName().equals(projectName),
+      "update for project %s cannot be added to manager for project %s",
+      update.getProjectName(), projectName);
+    changeUpdates.put(update.getRefName(), update);
+    ChangeDraftUpdate du = update.getDraftUpdate();
+    if (du != null) {
+      draftUpdates.put(du.getRefName(), du);
+    }
+  }
+
+  public void add(ChangeDraftUpdate draftUpdate) {
+    draftUpdates.put(draftUpdate.getRefName(), draftUpdate);
+  }
+
+  public void execute() throws OrmException, IOException {
+    if (isEmpty()) {
+      return;
+    }
+    try {
+      initChangeRepo();
+      if (!draftUpdates.isEmpty()) {
+        initAllUsersRepo();
+      }
+      addCommands();
+
+      // ChangeUpdates must execute before ChangeDraftUpdates.
+      //
+      // ChangeUpdate will automatically delete draft comments for any published
+      // comments, but the updates to the two repos don't happen atomically.
+      // Thus if the change meta update succeeds and the All-Users update fails,
+      // we may have stale draft comments. Doing it in this order allows stale
+      // comments to be filtered out by ChangeNotes, reflecting the fact that
+      // comments can only go from DRAFT to PUBLISHED, not vice versa.
+      execute(changeRepo);
+      execute(allUsersRepo);
+    } finally {
+      if (allUsersRepo != null) {
+        allUsersRepo.close();
+      }
+      if (changeRepo != null) {
+        changeRepo.close();
+      }
+    }
+  }
+
+  private static void execute(OpenRepo or) throws IOException {
+    if (or == null || or.cmds.isEmpty()) {
+      return;
+    }
+    or.ins.flush();
+    BatchRefUpdate bru = or.repo.getRefDatabase().newBatchUpdate();
+    or.cmds.addTo(bru);
+    bru.execute(or.rw, NullProgressMonitor.INSTANCE);
+    for (ReceiveCommand cmd : bru.getCommands()) {
+      if (cmd.getResult() != ReceiveCommand.Result.OK) {
+        throw new IOException("Update failed: " + bru);
+      }
+    }
+  }
+
+  private void addCommands() throws OrmException, IOException {
+    if (isEmpty()) {
+      return;
+    }
+    checkState(changeRepo != null, "must set change repo");
+    if (!draftUpdates.isEmpty()) {
+      checkState(allUsersRepo != null, "must set all users repo");
+    }
+    addUpdates(changeUpdates, changeRepo);
+    if (!draftUpdates.isEmpty()) {
+      addUpdates(draftUpdates, allUsersRepo);
+    }
+  }
+
+  private static void addUpdates(
+      ListMultimap<String, ? extends AbstractChangeUpdate> updates, OpenRepo or)
+      throws OrmException, IOException {
+    for (String refName : updates.keySet()) {
+      ObjectId old = firstNonNull(
+          or.cmds.getObjectId(or.repo, refName), ObjectId.zeroId());
+      ObjectId curr = old;
+
+      for (AbstractChangeUpdate u : updates.get(refName)) {
+        ObjectId next = u.apply(or.rw, or.ins, curr);
+        if (next == null) {
+          continue;
+        }
+        curr = next;
+      }
+      if (!old.equals(curr)) {
+        or.cmds.add(new ReceiveCommand(old, curr, refName));
+      }
+    }
+  }
+}

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

@@ -63,14 +63,21 @@ class RevisionNote {
   final ImmutableList<PatchLineComment> comments;
   final String pushCert;
 
-  RevisionNote(Change.Id changeId, ObjectReader reader, ObjectId noteId)
+  RevisionNote(Change.Id changeId, ObjectReader reader, ObjectId noteId,
-      throws ConfigInvalidException, IOException {
+      boolean draftsOnly) throws ConfigInvalidException, IOException {
     byte[] bytes = reader.open(noteId, OBJ_BLOB).getCachedBytes(MAX_NOTE_SZ);
     MutableInteger p = new MutableInteger();
     trimLeadingEmptyLines(bytes, p);
-    pushCert = parsePushCert(changeId, bytes, p);
+    if (!draftsOnly) {
-    trimLeadingEmptyLines(bytes, p);
+      pushCert = parsePushCert(changeId, bytes, p);
-    comments = ImmutableList.copyOf(CommentsInNotesUtil.parseNote(
+      trimLeadingEmptyLines(bytes, p);
-        bytes, p, changeId, PatchLineComment.Status.PUBLISHED));
+    } else {
+      pushCert = null;
+    }
+    PatchLineComment.Status status = draftsOnly
+        ? PatchLineComment.Status.DRAFT
+        : PatchLineComment.Status.PUBLISHED;
+    comments = ImmutableList.copyOf(
+        CommentsInNotesUtil.parseNote(bytes, p, changeId, status));
   }
 }

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

@@ -14,35 +14,77 @@
 
 package com.google.gerrit.server.notedb;
 
+import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.gerrit.server.PatchLineCommentsUtil.PLC_ORDER;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
 import com.google.gerrit.reviewdb.client.PatchLineComment;
+import com.google.gerrit.reviewdb.client.RevId;
 
 import java.io.ByteArrayOutputStream;
+import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 class RevisionNoteBuilder {
-  private final Map<PatchLineComment.Key, PatchLineComment> comments;
+  static class Cache {
+    private final RevisionNoteMap revisionNoteMap;
+    private final Map<RevId, RevisionNoteBuilder> builders;
+
+    Cache(RevisionNoteMap revisionNoteMap) {
+      this.revisionNoteMap = revisionNoteMap;
+      this.builders = new HashMap<>();
+    }
+
+    RevisionNoteBuilder get(RevId revId) {
+      RevisionNoteBuilder b = builders.get(revId);
+      if (b == null) {
+        b = new RevisionNoteBuilder(
+            revisionNoteMap.revisionNotes.get(revId));
+        builders.put(revId, b);
+      }
+      return b;
+    }
+
+    Map<RevId, RevisionNoteBuilder> getBuilders() {
+      return Collections.unmodifiableMap(builders);
+    }
+  }
+
+  final List<PatchLineComment> baseComments;
+  final Map<PatchLineComment.Key, PatchLineComment> put;
+  final Set<PatchLineComment.Key> delete;
+
   private String pushCert;
 
   RevisionNoteBuilder(RevisionNote base) {
     if (base != null) {
-      comments = Maps.newHashMapWithExpectedSize(base.comments.size());
+      baseComments = base.comments;
-      for (PatchLineComment c : base.comments) {
+      put = Maps.newHashMapWithExpectedSize(base.comments.size());
-        addComment(c);
-      }
       pushCert = base.pushCert;
     } else {
-      comments = new HashMap<>();
+      baseComments = Collections.emptyList();
+      put = new HashMap<>();
       pushCert = null;
     }
+    delete = new HashSet<>();
   }
 
-  void addComment(PatchLineComment comment) {
+  void putComment(PatchLineComment comment) {
-    comments.put(comment.getKey(), comment);
+    checkArgument(!delete.contains(comment.getKey()),
+        "cannot both delete and put %s", comment.getKey());
+    put.put(comment.getKey(), comment);
+  }
+
+  void deleteComment(PatchLineComment.Key key) {
+    checkArgument(!put.containsKey(key), "cannot both delete and put %s", key);
+    delete.add(key);
   }
 
   void setPushCertificate(String pushCert) {
@@ -56,7 +98,16 @@ class RevisionNoteBuilder {
       out.write(certBytes, 0, trimTrailingNewlines(certBytes));
       out.write('\n');
     }
-    commentsUtil.buildNote(PLC_ORDER.sortedCopy(comments.values()), out);
+
+    List<PatchLineComment> all =
+        new ArrayList<>(baseComments.size() + put.size());
+    for (PatchLineComment c : Iterables.concat(baseComments, put.values())) {
+      if (!delete.contains(c.getKey())) {
+        all.add(c);
+      }
+    }
+    Collections.sort(all, PLC_ORDER);
+    commentsUtil.buildNote(all, out);
     return out.toByteArray();
   }
 

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

@@ -0,0 +1,51 @@
+// Copyright (C) 2016 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.notedb;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.RevId;
+
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.ObjectReader;
+import org.eclipse.jgit.notes.Note;
+import org.eclipse.jgit.notes.NoteMap;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+
+class RevisionNoteMap {
+  final NoteMap noteMap;
+  final ImmutableMap<RevId, RevisionNote> revisionNotes;
+
+  static RevisionNoteMap parse(Change.Id changeId, ObjectReader reader,
+      NoteMap noteMap, boolean draftsOnly)
+      throws ConfigInvalidException, IOException {
+    Map<RevId, RevisionNote> result = new HashMap<>();
+    for (Note note : noteMap) {
+      RevisionNote rn =
+          new RevisionNote(changeId, reader, note.getData(), draftsOnly);
+      result.put(new RevId(note.name()), rn);
+    }
+    return new RevisionNoteMap(noteMap, ImmutableMap.copyOf(result));
+  }
+
+  private RevisionNoteMap(NoteMap noteMap,
+      ImmutableMap<RevId, RevisionNote> revisionNotes) {
+    this.noteMap = noteMap;
+    this.revisionNotes = revisionNotes;
+  }
+}

gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java

@@ -29,8 +29,10 @@ import com.google.gerrit.reviewdb.client.PatchLineComment;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RevId;
+import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.InternalUser;
 import com.google.gerrit.server.StarredChangesUtil;
 import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.account.CapabilityControl;
@@ -67,7 +69,6 @@ import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.PersonIdent;
-import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.junit.After;
 import org.junit.Before;
@@ -91,17 +92,23 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests {
   protected InMemoryRepository repo;
   protected InMemoryRepositoryManager repoManager;
   protected PersonIdent serverIdent;
+  protected InternalUser internalUser;
   protected Project.NameKey project;
   protected RevWalk rw;
   protected TestRepository<InMemoryRepository> tr;
 
-  @Inject protected IdentifiedUser.GenericFactory userFactory;
+  @Inject
+  protected IdentifiedUser.GenericFactory userFactory;
+
+  @Inject
+  protected NoteDbUpdateManager.Factory updateManagerFactory;
+
+  @Inject
+  protected AllUsersName allUsers;
 
   private Injector injector;
   private String systemTimeZone;
 
-  @Inject private AllUsersName allUsers;
-
   @Before
   public void setUp() throws Exception {
     setTimeForTesting();
@@ -128,6 +135,7 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests {
       @Override
       public void configure() {
         install(new GitModule());
+        factory(NoteDbUpdateManager.Factory.class);
         bind(AllUsersName.class).toProvider(AllUsersNameProvider.class);
         bind(NotesMigration.class).toInstance(MIGRATION);
         bind(GitRepositoryManager.class).toInstance(repoManager);
@@ -159,6 +167,7 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests {
     changeOwner = userFactory.create(co.getId());
     otherUser = userFactory.create(ou.getId());
     otherUserId = otherUser.getAccountId();
+    internalUser = new InternalUser(null);
   }
 
   private void setTimeForTesting() {
@@ -181,13 +190,10 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests {
     return c;
   }
 
-  protected ChangeUpdate newUpdate(Change c, IdentifiedUser user)
+  protected ChangeUpdate newUpdate(Change c, CurrentUser user)
       throws Exception {
     ChangeUpdate update = TestChanges.newUpdate(
         injector, repoManager, MIGRATION, c, allUsers, user);
-    try (Repository repo = repoManager.openMetadataRepository(c.getProject())) {
-      update.load(repo);
-    }
     return update;
   }
 

gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java

@@ -381,6 +381,39 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
         + "Groups: d,e,f\n");
   }
 
+  @Test
+  public void parseServerIdent() throws Exception {
+    String msg = "Update change\n"
+        + "\n"
+        + "Patch-set: 1\n"
+        + "Branch: refs/heads/master\n"
+        + "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+        + "Subject: Change subject\n";
+    assertParseSucceeds(msg);
+    assertParseSucceeds(writeCommit(msg, serverIdent));
+
+    msg = "Update change\n"
+        + "\n"
+        + "With a message."
+        + "\n"
+        + "Patch-set: 1\n"
+        + "Branch: refs/heads/master\n"
+        + "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+        + "Subject: Change subject\n";
+    assertParseSucceeds(msg);
+    assertParseSucceeds(writeCommit(msg, serverIdent));
+
+    msg = "Update change\n"
+        + "\n"
+        + "Patch-set: 1\n"
+        + "Branch: refs/heads/master\n"
+        + "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+        + "Subject: Change subject\n"
+        + "Label: Label1=+1\n";
+    assertParseSucceeds(msg);
+    assertParseFails(writeCommit(msg, serverIdent));
+  }
+
   private RevCommit writeCommit(String body) throws Exception {
     return writeCommit(body, ChangeNoteUtil.newIdent(
         changeOwner.getAccount(), TimeUtil.nowTs(), serverIdent,
@@ -404,7 +437,11 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
   }
 
   private void assertParseSucceeds(String body) throws Exception {
-    try (ChangeNotesParser parser = newParser(writeCommit(body))) {
+    assertParseSucceeds(writeCommit(body));
+  }
+
+  private void assertParseSucceeds(RevCommit commit) throws Exception {
+    try (ChangeNotesParser parser = newParser(commit)) {
       parser.parseAll();
     }
   }

gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java

@@ -43,19 +43,19 @@ import com.google.gerrit.reviewdb.client.PatchLineComment;
 import com.google.gerrit.reviewdb.client.PatchLineComment.Status;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
+import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.client.RevId;
-import com.google.gerrit.server.git.VersionedMetaData.BatchMetaDataUpdate;
 import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
 
-import org.eclipse.jgit.lib.BatchRefUpdate;
 import org.eclipse.jgit.lib.Constants;
-import org.eclipse.jgit.lib.NullProgressMonitor;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.notes.Note;
 import org.eclipse.jgit.notes.NoteMap;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
-import org.eclipse.jgit.transport.ReceiveCommand;
 import org.junit.Test;
 
 import java.sql.Timestamp;
@@ -65,6 +65,9 @@ import java.util.List;
 import java.util.Map;
 
 public class ChangeNotesTest extends AbstractChangeNotesTest {
+  @Inject
+  private DraftCommentNotes.Factory draftNotesFactory;
+
   @Test
   public void approvalsOnePatchSet() throws Exception {
     Change c = newChange();
@@ -410,17 +413,16 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
   @Test
   public void emptyChangeUpdate() throws Exception {
     Change c = newChange();
+    Ref initial = repo.exactRef(ChangeNoteUtil.changeRefName(c.getId()));
+    assertThat(initial).isNotNull();
 
-    // The initial empty update creates a commit which is needed to track the
+    // Empty update doesn't create a new commit.
-    // creation time of the change.
     ChangeUpdate update = newUpdate(c, changeOwner);
-    ObjectId revision = update.getRevision();
-    assertThat(revision).isNotNull();
-
-    // Any further empty update doesn't create a new commit.
-    update = newUpdate(c, changeOwner);
     update.commit();
-    assertThat(update.getRevision()).isEqualTo(revision);
+    assertThat(update.getResult()).isNull();
+
+    Ref updated = repo.exactRef(ChangeNoteUtil.changeRefName(c.getId()));
+    assertThat(updated.getObjectId()).isEqualTo(initial.getObjectId());
   }
 
   @Test
@@ -433,7 +435,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     update.setHashtags(hashtags);
     update.commit();
     try (RevWalk walk = new RevWalk(repo)) {
-      RevCommit commit = walk.parseCommit(update.getRevision());
+      RevCommit commit = walk.parseCommit(update.getResult());
       walk.parseBody(commit);
       assertThat(commit.getFullMessage()).endsWith("Hashtags: tag1,tag2\n");
     }
@@ -712,7 +714,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     update.setPatchSetState(PatchSetState.DRAFT);
     update.putApproval("Code-Review", (short) 1);
     update.setChangeMessage("This is a message");
-    update.insertComment(newPublishedComment(c.currentPatchSetId(), "a.txt",
+    update.putComment(newPublishedComment(c.currentPatchSetId(), "a.txt",
         "uuid1", new CommentRange(1, 2, 3, 4), 1, changeOwner, null,
         TimeUtil.nowTs(), "Comment", (short) 1, commit.name()));
     update.commit();
@@ -810,7 +812,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     update = newUpdate(c, changeOwner);
     update.setPatchSetId(psId2);
     Timestamp ts = TimeUtil.nowTs();
-    update.insertComment(newPublishedComment(psId2, "a.txt",
+    update.putComment(newPublishedComment(psId2, "a.txt",
         "uuid1", new CommentRange(1, 2, 3, 4), 1, changeOwner, null, ts,
         "Comment", (short) 1, commit.name()));
     update.commit();
@@ -840,12 +842,11 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
   public void emptyExceptSubject() throws Exception {
     ChangeUpdate update = newUpdate(newChange(), changeOwner);
     update.setSubjectForCommit("Create change");
-    update.commit();
+    assertThat(update.commit()).isNotNull();
-    assertThat(update.getRevision()).isNotNull();
   }
 
   @Test
-  public void multipleUpdatesInBatch() throws Exception {
+  public void multipleUpdatesInManager() throws Exception {
     Change c = newChange();
     ChangeUpdate update1 = newUpdate(c, changeOwner);
     update1.putApproval("Verified", (short) 1);
@@ -853,14 +854,10 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     ChangeUpdate update2 = newUpdate(c, otherUser);
     update2.putApproval("Code-Review", (short) 2);
 
-    BatchMetaDataUpdate batch = update1.openUpdate();
+    NoteDbUpdateManager updateManager = updateManagerFactory.create(project);
-    try {
+    updateManager.add(update1);
-      update1.writeCommit(batch);
+    updateManager.add(update2);
-      update2.writeCommit(batch);
+    updateManager.execute();
-      batch.commit();
-    } finally {
-      batch.close();
-    }
 
     ChangeNotes notes = newNotes(c);
     List<PatchSetApproval> psas =
@@ -887,29 +884,24 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     CommentRange range1 = new CommentRange(1, 1, 2, 1);
     Timestamp time1 = TimeUtil.nowTs();
     PatchSet.Id psId = c.currentPatchSetId();
-    BatchRefUpdate bru = repo.getRefDatabase().newBatchUpdate();
+    NoteDbUpdateManager updateManager = updateManagerFactory.create(project);
-    BatchMetaDataUpdate batch = update1.openUpdateInBatch(bru);
     PatchLineComment comment1 = newPublishedComment(psId, "file1",
         uuid1, range1, range1.getEndLine(), otherUser, null, time1, message1,
         (short) 0, "abcd1234abcd1234abcd1234abcd1234abcd1234");
     update1.setPatchSetId(psId);
-    update1.upsertComment(comment1);
+    update1.putComment(comment1);
-    update1.writeCommit(batch);
+    updateManager.add(update1);
+
     ChangeUpdate update2 = newUpdate(c, otherUser);
     update2.putApproval("Code-Review", (short) 2);
-    update2.writeCommit(batch);
+    updateManager.add(update2);
 
     RevCommit tipCommit;
-    try (RevWalk rw = new RevWalk(repo)) {
+    updateManager.execute();
-      batch.commit();
-      bru.execute(rw, NullProgressMonitor.INSTANCE);
 
-      ChangeNotes notes = newNotes(c);
+    ChangeNotes notes = newNotes(c);
-      ObjectId tip = notes.getRevision();
+    ObjectId tip = notes.getRevision();
-      tipCommit = rw.parseCommit(tip);
+    tipCommit = rw.parseCommit(tip);
-    } finally {
-      batch.close();
-    }
 
     RevCommit commitWithApprovals = tipCommit;
     assertThat(commitWithApprovals).isNotNull();
@@ -949,43 +941,30 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     ChangeUpdate update2 = newUpdate(c2, otherUser);
     update2.putApproval("Code-Review", (short) 2);
 
-    BatchMetaDataUpdate batch1 = null;
+    Ref initial1 = repo.exactRef(update1.getRefName());
-    BatchMetaDataUpdate batch2 = null;
+    assertThat(initial1).isNotNull();
+    Ref initial2 = repo.exactRef(update2.getRefName());
+    assertThat(initial2).isNotNull();
 
-    BatchRefUpdate bru = repo.getRefDatabase().newBatchUpdate();
+    NoteDbUpdateManager updateManager = updateManagerFactory.create(project);
-    try {
+    updateManager.add(update1);
-      batch1 = update1.openUpdateInBatch(bru);
+    updateManager.add(update2);
-      update1.writeCommit(batch1);
+    updateManager.execute();
-      batch1.commit();
-      assertThat(repo.exactRef(update1.getRefName())).isNotNull();
 
-      batch2 = update2.openUpdateInBatch(bru);
+    Ref ref1 = repo.exactRef(update1.getRefName());
-      update2.writeCommit(batch2);
+    assertThat(ref1.getObjectId()).isEqualTo(update1.getResult());
-      batch2.commit();
+    assertThat(ref1.getObjectId()).isNotEqualTo(initial1.getObjectId());
-      assertThat(repo.exactRef(update2.getRefName())).isNotNull();
+    Ref ref2 = repo.exactRef(update2.getRefName());
-    } finally {
+    assertThat(ref2.getObjectId()).isEqualTo(update2.getResult());
-      if (batch1 != null) {
+    assertThat(ref2.getObjectId()).isNotEqualTo(initial2.getObjectId());
-        batch1.close();
-      }
-      if (batch2 != null) {
-        batch2.close();
-      }
-    }
 
-    List<ReceiveCommand> cmds = bru.getCommands();
+    PatchSetApproval approval1 = newNotes(c1).getApprovals()
-    assertThat(cmds).hasSize(2);
+        .get(c1.currentPatchSetId()).iterator().next();
-    assertThat(cmds.get(0).getRefName()).isEqualTo(update1.getRefName());
+    assertThat(approval1.getLabel()).isEqualTo("Verified");
-    assertThat(cmds.get(1).getRefName()).isEqualTo(update2.getRefName());
 
-    try (RevWalk rw = new RevWalk(repo)) {
+    PatchSetApproval approval2 = newNotes(c2).getApprovals()
-      bru.execute(rw, NullProgressMonitor.INSTANCE);
+        .get(c2.currentPatchSetId()).iterator().next();
-    }
+    assertThat(approval2.getLabel()).isEqualTo("Code-Review");
-
-    assertThat(cmds.get(0).getResult()).isEqualTo(ReceiveCommand.Result.OK);
-    assertThat(cmds.get(1).getResult()).isEqualTo(ReceiveCommand.Result.OK);
-
-    assertThat(repo.exactRef(update1.getRefName())).isNotNull();
-    assertThat(repo.exactRef(update2.getRefName())).isNotNull();
   }
 
   @Test
@@ -1150,7 +1129,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         uuid1, range1, range1.getEndLine(), otherUser, null, time1, message1,
         (short) 1, "abcd1234abcd1234abcd1234abcd1234abcd1234");
     update.setPatchSetId(psId);
-    update.upsertComment(comment1);
+    update.putComment(comment1);
     update.commit();
 
     update = newUpdate(c, otherUser);
@@ -1159,7 +1138,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         uuid2, range2, range2.getEndLine(), otherUser, null, time2, message2,
         (short) 1, "abcd1234abcd1234abcd1234abcd1234abcd1234");
     update.setPatchSetId(psId);
-    update.upsertComment(comment2);
+    update.putComment(comment2);
     update.commit();
 
     update = newUpdate(c, otherUser);
@@ -1168,14 +1147,14 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         uuid3, range3, range3.getEndLine(), otherUser, null, time3, message3,
         (short) 1, "abcd1234abcd1234abcd1234abcd1234abcd1234");
     update.setPatchSetId(psId);
-    update.upsertComment(comment3);
+    update.putComment(comment3);
     update.commit();
 
     ChangeNotes notes = newNotes(c);
 
     try (RevWalk walk = new RevWalk(repo)) {
       ArrayList<Note> notesInTree =
-          Lists.newArrayList(notes.getNoteMap().iterator());
+          Lists.newArrayList(notes.revisionNoteMap.noteMap.iterator());
       Note note = Iterables.getOnlyElement(notesInTree);
 
       byte[] bytes =
@@ -1229,7 +1208,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         uuid1, range1, range1.getEndLine(), otherUser, null, time1, message1,
         (short) 0, "abcd1234abcd1234abcd1234abcd1234abcd1234");
     update.setPatchSetId(psId);
-    update.upsertComment(comment1);
+    update.putComment(comment1);
     update.commit();
 
     update = newUpdate(c, otherUser);
@@ -1238,14 +1217,14 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         uuid2, range2, range2.getEndLine(), otherUser, null, time2, message2,
         (short) 0, "abcd1234abcd1234abcd1234abcd1234abcd1234");
     update.setPatchSetId(psId);
-    update.upsertComment(comment2);
+    update.putComment(comment2);
     update.commit();
 
     ChangeNotes notes = newNotes(c);
 
     try (RevWalk walk = new RevWalk(repo)) {
       ArrayList<Note> notesInTree =
-          Lists.newArrayList(notes.getNoteMap().iterator());
+          Lists.newArrayList(notes.revisionNoteMap.noteMap.iterator());
       Note note = Iterables.getOnlyElement(notesInTree);
 
       byte[] bytes =
@@ -1293,7 +1272,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         range, range.getEndLine(), otherUser, null, now, messageForBase,
         (short) 0, rev1);
     update.setPatchSetId(psId);
-    update.upsertComment(commentForBase);
+    update.putComment(commentForBase);
     update.commit();
 
     update = newUpdate(c, otherUser);
@@ -1302,7 +1281,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         range, range.getEndLine(), otherUser, null, now, messageForPS,
         (short) 1, rev2);
     update.setPatchSetId(psId);
-    update.upsertComment(commentForPS);
+    update.putComment(commentForPS);
     update.commit();
 
     assertThat(newNotes(c).getComments()).containsExactlyEntriesIn(
@@ -1329,7 +1308,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         uuid1, range, range.getEndLine(), otherUser, null, timeForComment1,
         "comment 1", side, rev);
     update.setPatchSetId(psId);
-    update.upsertComment(comment1);
+    update.putComment(comment1);
     update.commit();
 
     update = newUpdate(c, otherUser);
@@ -1337,7 +1316,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         uuid2, range, range.getEndLine(), otherUser, null, timeForComment2,
         "comment 2", side, rev);
     update.setPatchSetId(psId);
-    update.upsertComment(comment2);
+    update.putComment(comment2);
     update.commit();
 
     assertThat(newNotes(c).getComments()).containsExactlyEntriesIn(
@@ -1364,7 +1343,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         uuid, range, range.getEndLine(), otherUser, null, now, "comment 1",
         side, rev);
     update.setPatchSetId(psId);
-    update.upsertComment(comment1);
+    update.putComment(comment1);
     update.commit();
 
     update = newUpdate(c, otherUser);
@@ -1372,7 +1351,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         uuid, range, range.getEndLine(), otherUser, null, now, "comment 2",
         side, rev);
     update.setPatchSetId(psId);
-    update.upsertComment(comment2);
+    update.putComment(comment2);
     update.commit();
 
     assertThat(newNotes(c).getComments()).containsExactlyEntriesIn(
@@ -1398,7 +1377,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         uuid, range, range.getEndLine(), otherUser, null, now, "comment on ps1",
         side, rev1);
     update.setPatchSetId(ps1);
-    update.upsertComment(comment1);
+    update.putComment(comment1);
     update.commit();
 
     incrementPatchSet(c);
@@ -1410,7 +1389,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         uuid, range, range.getEndLine(), otherUser, null, now, "comment on ps2",
         side, rev2);
     update.setPatchSetId(ps2);
-    update.upsertComment(comment2);
+    update.putComment(comment2);
     update.commit();
 
     assertThat(newNotes(c).getComments()).containsExactlyEntriesIn(
@@ -1435,7 +1414,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         range.getEndLine(), otherUser, null, now, "comment on ps1", side,
         rev, Status.DRAFT);
     update.setPatchSetId(ps1);
-    update.insertComment(comment1);
+    update.putComment(comment1);
     update.commit();
 
     ChangeNotes notes = newNotes(c);
@@ -1446,7 +1425,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     comment1.setStatus(Status.PUBLISHED);
     update = newUpdate(c, otherUser);
     update.setPatchSetId(ps1);
-    update.updateComment(comment1);
+    update.putComment(comment1);
     update.commit();
 
     notes = newNotes(c);
@@ -1478,8 +1457,8 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     PatchLineComment comment2 = newComment(psId, filename, uuid2,
         range2, range2.getEndLine(), otherUser, null, now, "other on ps1",
         side, rev, Status.DRAFT);
-    update.insertComment(comment1);
+    update.putComment(comment1);
-    update.insertComment(comment2);
+    update.putComment(comment2);
     update.commit();
 
     ChangeNotes notes = newNotes(c);
@@ -1493,7 +1472,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     update = newUpdate(c, otherUser);
     update.setPatchSetId(psId);
     comment1.setStatus(Status.PUBLISHED);
-    update.updateComment(comment1);
+    update.putComment(comment1);
     update.commit();
 
     notes = newNotes(c);
@@ -1527,8 +1506,8 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         range2, range2.getEndLine(), otherUser, null, now, "comment on ps",
         (short) 1, rev2, Status.DRAFT);
 
-    update.insertComment(baseComment);
+    update.putComment(baseComment);
-    update.insertComment(psComment);
+    update.putComment(psComment);
     update.commit();
 
     ChangeNotes notes = newNotes(c);
@@ -1544,8 +1523,8 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
 
     baseComment.setStatus(Status.PUBLISHED);
     psComment.setStatus(Status.PUBLISHED);
-    update.updateComment(baseComment);
+    update.putComment(baseComment);
-    update.updateComment(psComment);
+    update.putComment(psComment);
     update.commit();
 
     notes = newNotes(c);
@@ -1573,7 +1552,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         range.getEndLine(), otherUser, null, now, "comment on ps1", side,
         rev, Status.DRAFT);
     update.setPatchSetId(psId);
-    update.upsertComment(comment);
+    update.putComment(comment);
     update.commit();
 
     ChangeNotes notes = newNotes(c);
@@ -1612,7 +1591,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         uuid, range, range.getEndLine(), otherUser, null, now, "comment on ps1",
         side, rev1, Status.DRAFT);
     update.setPatchSetId(ps1);
-    update.upsertComment(comment1);
+    update.putComment(comment1);
     update.commit();
 
     incrementPatchSet(c);
@@ -1624,7 +1603,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         uuid, range, range.getEndLine(), otherUser, null, now, "comment on ps2",
         side, rev2, Status.DRAFT);
     update.setPatchSetId(ps2);
-    update.upsertComment(comment2);
+    update.putComment(comment2);
     update.commit();
 
     ChangeNotes notes = newNotes(c);
@@ -1657,7 +1636,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         psId, "filename", uuid, null, 0, otherUser, null, now, messageForBase,
         (short) 0, rev);
     update.setPatchSetId(psId);
-    update.upsertComment(comment);
+    update.putComment(comment);
     update.commit();
 
     assertThat(newNotes(c).getComments()).containsExactlyEntriesIn(
@@ -1678,7 +1657,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         psId, "filename", uuid, null, 1, otherUser, null, now, messageForBase,
         (short) 0, rev);
     update.setPatchSetId(psId);
-    update.upsertComment(comment);
+    update.putComment(comment);
     update.commit();
 
     assertThat(newNotes(c).getComments()).containsExactlyEntriesIn(
@@ -1686,7 +1665,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
   }
 
   @Test
-  public void updateCommentsForMultipleRevisions() throws Exception {
+  public void putCommentsForMultipleRevisions() throws Exception {
     Change c = newChange();
     String uuid = "uuid";
     String rev1 = "abcd1234abcd1234abcd1234abcd1234abcd1234";
@@ -1708,8 +1687,8 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     PatchLineComment comment2 = newComment(ps2, filename,
         uuid, range, range.getEndLine(), otherUser, null, now, "comment on ps2",
         side, rev2, Status.DRAFT);
-    update.upsertComment(comment1);
+    update.putComment(comment1);
-    update.upsertComment(comment2);
+    update.putComment(comment2);
     update.commit();
 
     ChangeNotes notes = newNotes(c);
@@ -1720,8 +1699,8 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     update.setPatchSetId(ps2);
     comment1.setStatus(Status.PUBLISHED);
     comment2.setStatus(Status.PUBLISHED);
-    update.upsertComment(comment1);
+    update.putComment(comment1);
-    update.upsertComment(comment2);
+    update.putComment(comment2);
     update.commit();
 
     notes = newNotes(c);
@@ -1729,9 +1708,170 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     assertThat(notes.getComments()).hasSize(2);
   }
 
+  @Test
+  public void publishSubsetOfCommentsOnRevision() throws Exception {
+    Change c = newChange();
+    RevId rev1 = new RevId("abcd1234abcd1234abcd1234abcd1234abcd1234");
+    CommentRange range = new CommentRange(1, 1, 2, 1);
+    PatchSet.Id ps1 = c.currentPatchSetId();
+    short side = (short) 1;
+
+    ChangeUpdate update = newUpdate(c, otherUser);
+    update.setPatchSetId(ps1);
+    Timestamp now = TimeUtil.nowTs();
+    PatchLineComment comment1 = newComment(ps1, "file1",
+        "uuid1", range, range.getEndLine(), otherUser, null, now, "comment1",
+        side, rev1.get(), Status.DRAFT);
+    PatchLineComment comment2 = newComment(ps1, "file2",
+        "uuid2", range, range.getEndLine(), otherUser, null, now, "comment2",
+        side, rev1.get(), Status.DRAFT);
+    update.putComment(comment1);
+    update.putComment(comment2);
+    update.commit();
+
+    ChangeNotes notes = newNotes(c);
+    assertThat(notes.getDraftComments(otherUserId).get(rev1))
+        .containsExactly(comment1, comment2);
+    assertThat(notes.getComments()).isEmpty();
+
+    update = newUpdate(c, otherUser);
+    update.setPatchSetId(ps1);
+    comment2.setStatus(Status.PUBLISHED);
+    update.putComment(comment2);
+    update.commit();
+
+    notes = newNotes(c);
+    assertThat(notes.getDraftComments(otherUserId).get(rev1))
+        .containsExactly(comment1);
+    assertThat(notes.getComments().get(rev1)).containsExactly(comment2);
+  }
+
+  @Test
+  public void updateWithServerIdent() throws Exception {
+    Change c = newChange();
+    ChangeUpdate update = newUpdate(c, internalUser);
+    update.setChangeMessage("A message.");
+    update.commit();
+
+    ChangeMessage msg = Iterables.getLast(newNotes(c).getChangeMessages());
+    assertThat(msg.getMessage()).isEqualTo("A message.");
+    assertThat(msg.getAuthor()).isNull();
+
+    update = newUpdate(c, internalUser);
+    exception.expect(UnsupportedOperationException.class);
+    update.putApproval("Code-Review", (short) 1);
+  }
+
+  @Test
+  public void filterOutAndFixUpZombieDraftComments() throws Exception {
+    Change c = newChange();
+    RevId rev1 = new RevId("abcd1234abcd1234abcd1234abcd1234abcd1234");
+    CommentRange range = new CommentRange(1, 1, 2, 1);
+    PatchSet.Id ps1 = c.currentPatchSetId();
+    short side = (short) 1;
+
+    ChangeUpdate update = newUpdate(c, otherUser);
+    Timestamp now = TimeUtil.nowTs();
+    PatchLineComment comment1 = newComment(ps1, "file1",
+        "uuid1", range, range.getEndLine(), otherUser, null, now, "comment on ps1",
+        side, rev1.get(), Status.DRAFT);
+    PatchLineComment comment2 = newComment(ps1, "file2",
+        "uuid2", range, range.getEndLine(), otherUser, null, now, "another comment",
+        side, rev1.get(), Status.DRAFT);
+    update.putComment(comment1);
+    update.putComment(comment2);
+    update.commit();
+
+
+    String refName = RefNames.refsDraftComments(otherUserId, c.getId());
+    ObjectId oldDraftId = exactRefAllUsers(refName);
+
+    update = newUpdate(c, otherUser);
+    update.setPatchSetId(ps1);
+    comment2.setStatus(Status.PUBLISHED);
+    update.putComment(comment2);
+    update.commit();
+    assertThat(exactRefAllUsers(refName)).isNotNull();
+    assertThat(exactRefAllUsers(refName)).isNotEqualTo(oldDraftId);
+
+    // Re-add draft version of comment2 back to draft ref without updating
+    // change ref. Simulates the case where deleting the draft failed
+    // non-atomically after adding the published comment succeeded.
+    ChangeDraftUpdate draftUpdate =
+        newUpdate(c, otherUser).createDraftUpdateIfNull();
+    comment2.setStatus(Status.DRAFT);
+    draftUpdate.putComment(comment2);
+    NoteDbUpdateManager manager = updateManagerFactory.create(c.getProject());
+    manager.add(draftUpdate);
+    manager.execute();
+
+    // Looking at drafts directly shows the zombie comment.
+    DraftCommentNotes draftNotes =
+        draftNotesFactory.create(c.getId(), otherUserId);
+    assertThat(draftNotes.load().getComments().get(rev1))
+        .containsExactly(comment1, comment2);
+
+    comment2.setStatus(Status.PUBLISHED); // Reset for later assertions.
+
+    // Zombie comment is filtered out of drafts via ChangeNotes.
+    ChangeNotes notes = newNotes(c);
+    assertThat(notes.getDraftComments(otherUserId).get(rev1))
+        .containsExactly(comment1);
+    assertThat(notes.getComments().get(rev1))
+        .containsExactly(comment2);
+
+    update = newUpdate(c, otherUser);
+    update.setPatchSetId(ps1);
+    comment1.setStatus(Status.PUBLISHED);
+    update.putComment(comment1);
+    update.commit();
+
+    // Updating an unrelated comment causes the zombie comment to get fixed up.
+    assertThat(exactRefAllUsers(refName)).isNull();
+  }
+
+  @Test
+  public void updateCommentsInSequentialUpdates() throws Exception {
+    Change c = newChange();
+    CommentRange range = new CommentRange(1, 1, 2, 1);
+    String rev = "abcd1234abcd1234abcd1234abcd1234abcd1234";
+
+    ChangeUpdate update1 = newUpdate(c, otherUser);
+    PatchLineComment comment1 = newComment(c.currentPatchSetId(), "filename",
+        "uuid1", range, range.getEndLine(), otherUser, null,
+        new Timestamp(update1.getWhen().getTime()), "comment 1", (short) 1, rev,
+        Status.PUBLISHED);
+    update1.putComment(comment1);
+
+    ChangeUpdate update2 = newUpdate(c, otherUser);
+    PatchLineComment comment2 = newComment(c.currentPatchSetId(), "filename",
+        "uuid2", range, range.getEndLine(), otherUser, null,
+        new Timestamp(update2.getWhen().getTime()), "comment 2", (short) 1, rev,
+        Status.PUBLISHED);
+    update2.putComment(comment2);
+
+    NoteDbUpdateManager manager = updateManagerFactory.create(project);
+    manager.add(update1);
+    manager.add(update2);
+    manager.execute();
+
+    ChangeNotes notes = newNotes(c);
+    List<PatchLineComment> comments = notes.getComments().get(new RevId(rev));
+    assertThat(comments).hasSize(2);
+    assertThat(comments.get(0).getMessage()).isEqualTo("comment 1");
+    assertThat(comments.get(1).getMessage()).isEqualTo("comment 2");
+  }
+
   private String readNote(ChangeNotes notes, ObjectId noteId) throws Exception {
-    ObjectId dataId = notes.getNoteMap().getNote(noteId).getData();
+    ObjectId dataId = notes.revisionNoteMap.noteMap.getNote(noteId).getData();
     return new String(
         rw.getObjectReader().open(dataId, OBJ_BLOB).getCachedBytes(), UTF_8);
   }
+
+  private ObjectId exactRefAllUsers(String refName) throws Exception {
+    try (Repository allUsersRepo = repoManager.openRepository(allUsers)) {
+      Ref ref = allUsersRepo.exactRef(refName);
+      return ref != null ? ref.getObjectId() : null;
+    }
+  }
 }

gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java

@@ -45,7 +45,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
     update.commit();
     assertThat(update.getRefName()).isEqualTo("refs/changes/01/1/meta");
 
-    RevCommit commit = parseCommit(update.getRevision());
+    RevCommit commit = parseCommit(update.getResult());
     assertBodyEquals("Update patch set 1\n"
         + "\n"
         + "Patch-set: 1\n"
@@ -93,7 +93,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
         + "Subject: Change subject\n"
         + "Branch: refs/heads/master\n"
         + "Commit: " + update.getCommit().name() + "\n",
-        update.getRevision());
+        update.getResult());
   }
 
   @Test
@@ -115,7 +115,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
         + "Subject: Subject\n"
         + "Branch: refs/heads/master\n"
         + "Commit: " + commit.name() + "\n",
-        update.getRevision());
+        update.getResult());
   }
 
   @Test
@@ -129,7 +129,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
         + "\n"
         + "Patch-set: 1\n"
         + "Label: -Code-Review\n",
-        update.getRevision());
+        update.getResult());
   }
 
   @Test
@@ -147,7 +147,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
           submitLabel("Alternative-Code-Review", "NEED", null))));
     update.commit();
 
-    RevCommit commit = parseCommit(update.getRevision());
+    RevCommit commit = parseCommit(update.getResult());
     assertBodyEquals("Submit patch set 1\n"
         + "\n"
         + "Patch-set: 1\n"
@@ -185,7 +185,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
     update.setChangeMessage("Comment on the change.");
     update.commit();
 
-    RevCommit commit = parseCommit(update.getRevision());
+    RevCommit commit = parseCommit(update.getResult());
     assertBodyEquals("Update patch set 1\n"
         + "\n"
         + "Comment on the change.\n"
@@ -214,7 +214,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
         + "Status: merged\n"
         + "Submission-id: 1-1453387607626-96fabc25\n"
         + "Submitted-with: RULE_ERROR Problem with patch set: 1\n",
-        update.getRevision());
+        update.getResult());
   }
 
   @Test
@@ -228,7 +228,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
         + "\n"
         + "Patch-set: 1\n"
         + "Reviewer: Change Owner <1@gerrit>\n",
-        update.getRevision());
+        update.getResult());
   }
 
   @Test
@@ -246,7 +246,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
         + "\n"
         + "\n"
         + "Patch-set: 1\n",
-        update.getRevision());
+        update.getResult());
   }
 
   @Test
@@ -269,7 +269,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
         + "Testing paragraph 3\n"
         + "\n"
         + "Patch-set: 1\n",
-        update.getRevision());
+        update.getResult());
   }
 
   private RevCommit parseCommit(ObjectId id) throws Exception {

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

@@ -27,7 +27,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetInfo;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RevId;
-import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.notedb.ChangeDraftUpdate;
@@ -88,14 +88,14 @@ public class TestChanges {
 
   public static ChangeUpdate newUpdate(Injector injector,
       GitRepositoryManager repoManager, NotesMigration migration, Change c,
-      final AllUsersName allUsers, final IdentifiedUser user)
+      final AllUsersName allUsers, final CurrentUser user)
       throws Exception  {
     ChangeUpdate update = injector.createChildInjector(new FactoryModule() {
       @Override
       public void configure() {
         factory(ChangeUpdate.Factory.class);
         factory(ChangeDraftUpdate.Factory.class);
-        bind(IdentifiedUser.class).toInstance(user);
+        bind(CurrentUser.class).toInstance(user);
       }
     }).getInstance(ChangeUpdate.Factory.class).create(
         stubChangeControl(repoManager, migration, c, allUsers, user),
@@ -112,8 +112,8 @@ public class TestChanges {
     // first patch set, so create one.
     try (Repository repo = repoManager.openRepository(c.getProject())) {
       TestRepository<Repository> tr = new TestRepository<>(repo);
-      PersonIdent ident =
+      PersonIdent ident = user.asIdentifiedUser()
-          user.newCommitterIdent(update.getWhen(), TimeZone.getDefault());
+          .newCommitterIdent(update.getWhen(), TimeZone.getDefault());
       TestRepository<Repository>.CommitBuilder cb = tr.commit()
           .author(ident)
           .committer(ident)
@@ -132,7 +132,7 @@ public class TestChanges {
   private static ChangeControl stubChangeControl(
       GitRepositoryManager repoManager, NotesMigration migration,
       Change c, AllUsersName allUsers,
-      IdentifiedUser user) throws OrmException {
+      CurrentUser user) throws OrmException {
     ChangeControl ctl = EasyMock.createMock(ChangeControl.class);
     expect(ctl.getChange()).andStubReturn(c);
     expect(ctl.getProject()).andStubReturn(new Project(c.getProject()));