Rewrite updating comments in NoteDb

The high-level API is simplified: we no longer ask callers to
distinguish between insert/update/upsert, and instead use upsert
everywhere, calling it "put". These distinctions were not
particularly important from the perspective of e.g. the REST API.

Although API simplification is a good thing regardless, this change
was primarily driven by the fact that we do not want to use the
eagerly-read ChangeNotes from within ChangeUpdate internals prior to
actually applying the update. The various checkArgument calls in
insert/update/upsert were trying to enforce, for example, that we only
insert changes that don't exist at the time the setter was called.

But in the context of NoteDbUpdateManager that executes multiple
ChangeUpdates in sequence, it doesn't make sense to look up a
comment in the old comment map based on the current branch tip when
the ChangeUpdate instance was created. Instead we should be looking at
the comment map that is the immediate output of the previous update in
the sequence. This distinction is precisely what was causing the test
in I064f004e to fail.

To solve this problem at the storage layer, instead of depending on
the NoteMap from the original ChangeNotes, read the RevisionNotes
from the current revision passed in from the NoteDbUpdateManager
(which may not have been flushed yet, but is still available to the
reader). Don't bother reading the whole ChangeNotes again, as that is
unnecessary. Encapsulate the RevisionNotes and the mutable NoteMap
instance itself in a small POJO, RevisionNoteMap. In the common case
of a single ChangeUpdate, we can even reuse the previously parsed
RevisionNoteMap from the ChangeNotes that was passed in; we only have
to re-read when doing multiple updates.

Mutations are done to the RevisionNoteMap using the same
RevisionNoteMapBuilder idiom we were already using for merging push
certificates with inline comments. Moreover, we can use the same
logic within ChangeDraftUpdate, obviating the need for
DraftCommentNotesParser and simplifying the parse codepath somewhat.

Finally, this change improves reliability and robustness around the
fact that we are writing ref updates to two distinct repositories
non-atomically, the change meta repo and All-Users. Order the updates
so that the write to the change repo happens first, taking advantage
of the fact that comments can only go from DRAFT to PUBLISHED, not
vice versa. If the change update succeeds and the All-Users update
fails, then we will have a zombie DRAFT comment left in All-Users
where there is a PUBLISHED comment in the change repo. We fix this up
in two ways: at read time, by explicitly filtering out any comments
from the draft list that are also published; and at write time, by
deleting all zombie draft comments any time we write any other draft
comment.

Add test cases for a few more combinations of comment updates,
including one that simulates a failed All-Users write.

Change-Id: I69c46ab4b55d4e40ad80754ee8528e99ae6ad4c5

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() {
-    return noteMap;
+  RevisionNoteMap getRevisionNoteMap() {
+    return revisionNoteMap;
   }
 
   public Account.Id getAuthor() {
@@ -110,13 +113,17 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
       return;
     }
 
-    try (RevWalk walk = new RevWalk(reader);
-        DraftCommentNotesParser parser = new DraftCommentNotesParser(
-          getChangeId(), walk, rev, repoManager, draftsProject, author)) {
-      parser.parseDraftComments();
-
-      comments = ImmutableListMultimap.copyOf(parser.comments);
-      noteMap = parser.noteMap;
+    try (RevWalk walk = new RevWalk(reader)) {
+      RevCommit tipCommit = walk.parseCommit(rev);
+      revisionNoteMap = RevisionNoteMap.parse(
+          getChangeId(), reader, NoteMap.read(reader, tipCommit), true);
+      Multimap<RevId, PatchLineComment> cs = ArrayListMultimap.create();
+      for (RevisionNote rn : revisionNoteMap.revisionNotes.values()) {
+        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;
+  }
 }