Factor out a value class for the output of ChangeNotesParser

The intent of this value class is to be in-process cacheable, so we
don't have to repeatedly read objects from the repo to parse and
re-parse the notes graph. Thus it contains the full set of Change
fields and associated Change entities.

Note that, even though it's somewhat unwieldy, we explicitly list
each field in Change that ChangeNotesParser knows how to parse, and
do *not* store a Change in ChangeNotesState. This is because the
rowVersion and noteDbState fields are used internally to test
consistency between ReviewDb and NoteDb, so we can't pretend that
these were populated from NoteDb and store them in any sort of cache.
Instead, we store the other fields, and provide a single method to
copy the field values to a Change object that we know was read from
ReviewDb. This is exactly what already happens in the
ChangeNotes.Factory path.

The main output of ChangeNotesParser is a ChangeNotesState, and each
ChangeNotes has a single one of these from which it can serve most of
its public methods. These methods were intentionally already
returning immutable collections in anticipation of this day.

Change-Id: I572cb1a8cb9ce8f68c2e4b744bdfab06a3383a1e

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

@@ -22,20 +22,17 @@ import static com.google.gerrit.reviewdb.client.RefNames.changeMetaRef;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Function;
 import com.google.common.base.Predicate;
-import com.google.common.base.Strings;
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
-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.collect.Tables;
 import com.google.common.util.concurrent.AsyncFunction;
 import com.google.common.util.concurrent.CheckedFuture;
 import com.google.common.util.concurrent.Futures;
@@ -44,7 +41,6 @@ import com.google.common.util.concurrent.ListeningExecutorService;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.data.SubmitRecord;
 import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.Branch;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.reviewdb.client.PatchLineComment;
@@ -54,7 +50,6 @@ import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.reviewdb.server.ReviewDbUtil;
 import com.google.gerrit.server.ReviewerSet;
 import com.google.gerrit.server.git.RefCache;
 import com.google.gerrit.server.git.RepoRefCache;
@@ -378,15 +373,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
   private final RefCache refs;
 
   private Change change;
-  private ImmutableSortedMap<PatchSet.Id, PatchSet> patchSets;
-  private ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals;
-  private ReviewerSet reviewers;
-  private ImmutableList<Account.Id> allPastReviewers;
-  private ImmutableList<SubmitRecord> submitRecords;
-  private ImmutableList<ChangeMessage> allChangeMessages;
-  private ImmutableListMultimap<PatchSet.Id, ChangeMessage> changeMessagesByPatchSet;
-  private ImmutableListMultimap<RevId, PatchLineComment> comments;
-  private ImmutableSet<String> hashtags;
+  private ChangeNotesState state;
 
   // Parsed note map state, used by ChangeUpdate to make in-place editing of
   // notes easier.
@@ -411,15 +398,15 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
   }
 
   public ImmutableMap<PatchSet.Id, PatchSet> getPatchSets() {
-    return patchSets;
+    return state.patchSets();
   }
 
   public ImmutableListMultimap<PatchSet.Id, PatchSetApproval> getApprovals() {
-    return approvals;
+    return state.approvals();
   }
 
   public ReviewerSet getReviewers() {
-    return reviewers;
+    return state.reviewers();
   }
 
   /**
@@ -427,14 +414,14 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
    * @return a ImmutableSet of all hashtags for this change sorted in alphabetical order.
    */
   public ImmutableSet<String> getHashtags() {
-    return ImmutableSortedSet.copyOf(hashtags);
+    return ImmutableSortedSet.copyOf(state.hashtags());
   }
 
   /**
    * @return a list of all users who have ever been a reviewer on this change.
    */
   public ImmutableList<Account.Id> getAllPastReviewers() {
-    return allPastReviewers;
+    return state.allPastReviewers();
   }
 
   /**
@@ -442,12 +429,12 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
    *     changes that were actually submitted.
    */
   public ImmutableList<SubmitRecord> getSubmitRecords() {
-    return submitRecords;
+    return state.submitRecords();
   }
 
   /** @return all change messages, in chronological order, oldest first. */
   public ImmutableList<ChangeMessage> getChangeMessages() {
-    return allChangeMessages;
+    return state.allChangeMessages();
   }
 
   /**
@@ -456,18 +443,19 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
    */
   public ImmutableListMultimap<PatchSet.Id, ChangeMessage>
       getChangeMessagesByPatchSet() {
-    return changeMessagesByPatchSet;
+    return state.changeMessagesByPatchSet();
   }
 
   /** @return inline comments on each revision. */
   public ImmutableListMultimap<RevId, PatchLineComment> getComments() {
-    return comments;
+    return state.publishedComments();
   }
 
   public ImmutableListMultimap<RevId, PatchLineComment> getDraftComments(
       Account.Id author) throws OrmException {
     loadDraftComments(author);
-    final Multimap<RevId, PatchLineComment> published = comments;
+    final Multimap<RevId, PatchLineComment> published =
+        state.publishedComments();
     // 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.
@@ -533,7 +521,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
 
   public PatchSet getCurrentPatchSet() {
     PatchSet.Id psId = change.currentPatchSetId();
-    return checkNotNull(patchSets.get(psId),
+    return checkNotNull(state.patchSets().get(psId),
         "missing current patch set %s", psId.get());
   }
 
@@ -547,57 +535,14 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     }
     ChangeNotesParser parser = new ChangeNotesParser(
         change.getId(), rev, handle.walk(), args.noteUtil, args.metrics);
-    parser.parseAll();
-
-    if (parser.status != null) {
-      change.setStatus(parser.status);
-    }
-    approvals = parser.buildApprovals();
-    changeMessagesByPatchSet = parser.buildMessagesByPatchSet();
-    allChangeMessages = parser.buildAllMessages();
-    comments = ImmutableListMultimap.copyOf(parser.comments);
-    revisionNoteMap = parser.revisionNoteMap;
-    change.setKey(new Change.Key(parser.changeId));
-    change.setDest(new Branch.NameKey(change.getProject(), parser.branch));
-    change.setTopic(Strings.emptyToNull(parser.topic));
-    change.setCreatedOn(parser.createdOn);
-    change.setLastUpdatedOn(parser.lastUpdatedOn);
-    change.setOwner(parser.ownerId);
-    change.setSubmissionId(parser.submissionId);
-    patchSets = ImmutableSortedMap.copyOf(
-        parser.patchSets, ReviewDbUtil.intKeyOrdering());
-
-    if (!patchSets.isEmpty()) {
-      change.setCurrentPatchSet(
-          parser.currentPatchSetId, parser.subject, parser.originalSubject);
-    } else {
-      // TODO(dborowitz): This should be an error, but for now it's required for
-      // some tests to pass.
-      change.clearCurrentPatchSet();
-    }
-
-    if (parser.hashtags != null) {
-      hashtags = ImmutableSet.copyOf(parser.hashtags);
-    } else {
-      hashtags = ImmutableSet.of();
-    }
-    this.reviewers = ReviewerSet.fromTable(Tables.transpose(parser.reviewers));
-    this.allPastReviewers = ImmutableList.copyOf(parser.allPastReviewers);
-
-    submitRecords = ImmutableList.copyOf(parser.submitRecords);
+    state = parser.parseAll();
+    state.copyColumnsTo(change);
+    revisionNoteMap = parser.getRevisionNoteMap();
   }
 
   @Override
   protected void loadDefaults() {
-    approvals = ImmutableListMultimap.of();
-    reviewers = ReviewerSet.empty();
-    submitRecords = ImmutableList.of();
-    allChangeMessages = ImmutableList.of();
-    changeMessagesByPatchSet = ImmutableListMultimap.of();
-    comments = ImmutableListMultimap.of();
-    hashtags = ImmutableSet.of();
-    patchSets = ImmutableSortedMap.of();
-    allPastReviewers = ImmutableList.of();
+    state = ChangeNotesState.empty(change);
   }
 
   @Override

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

@@ -37,8 +37,6 @@ import com.google.common.base.Splitter;
 import com.google.common.base.Supplier;
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.HashBasedTable;
-import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.LinkedListMultimap;
 import com.google.common.collect.Lists;
@@ -61,6 +59,7 @@ 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.reviewdb.server.ReviewDbUtil;
+import com.google.gerrit.server.ReviewerSet;
 import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
 import com.google.gerrit.server.util.LabelVote;
 
@@ -95,38 +94,42 @@ class ChangeNotesParser {
   private static final RevId PARTIAL_PATCH_SET =
       new RevId("INVALID PARTIAL PATCH SET");
 
-  final Table<Account.Id, ReviewerStateInternal, Timestamp> reviewers;
-  final List<Account.Id> allPastReviewers;
-  final List<SubmitRecord> submitRecords;
-  final Multimap<RevId, PatchLineComment> comments;
-  final TreeMap<PatchSet.Id, PatchSet> patchSets;
-  final Map<PatchSet.Id, PatchSetState> patchSetStates;
-
-  String branch;
-  Change.Status status;
-  String topic;
-  Set<String> hashtags;
-  Timestamp createdOn;
-  Timestamp lastUpdatedOn;
-  Account.Id ownerId;
-  String changeId;
-  String subject;
-  String originalSubject;
-  String submissionId;
-  String tag;
-  PatchSet.Id currentPatchSetId;
-  RevisionNoteMap revisionNoteMap;
-
+  // Private final members initialized in the constructor.
   private final ChangeNoteUtil noteUtil;
   private final NoteDbMetrics metrics;
   private final Change.Id id;
   private final ObjectId tip;
   private final ChangeNotesRevWalk walk;
+
+  // Private final but mutable members initialized in the constructor and filled
+  // in during the parsing process.
+  private final Table<Account.Id, ReviewerStateInternal, Timestamp> reviewers;
+  private final List<Account.Id> allPastReviewers;
+  private final List<SubmitRecord> submitRecords;
+  private final Multimap<RevId, PatchLineComment> comments;
+  private final TreeMap<PatchSet.Id, PatchSet> patchSets;
+  private final Map<PatchSet.Id, PatchSetState> patchSetStates;
   private final Map<PatchSet.Id,
       Table<Account.Id, Entry<String, String>, Optional<PatchSetApproval>>> approvals;
   private final List<ChangeMessage> allChangeMessages;
   private final Multimap<PatchSet.Id, ChangeMessage> changeMessagesByPatchSet;
 
+  // Non-final private members filled in during the parsing process.
+  private String branch;
+  private Change.Status status;
+  private String topic;
+  private Set<String> hashtags;
+  private Timestamp createdOn;
+  private Timestamp lastUpdatedOn;
+  private Account.Id ownerId;
+  private String changeId;
+  private String subject;
+  private String originalSubject;
+  private String submissionId;
+  private String tag;
+  private PatchSet.Id currentPatchSetId;
+  private RevisionNoteMap revisionNoteMap;
+
   ChangeNotesParser(Change.Id changeId, ObjectId tip, ChangeNotesRevWalk walk,
       ChangeNoteUtil noteUtil, NoteDbMetrics metrics) {
     this.id = changeId;
@@ -145,7 +148,8 @@ class ChangeNotesParser {
     patchSetStates = new HashMap<>();
   }
 
-  void parseAll() throws ConfigInvalidException, IOException {
+  ChangeNotesState parseAll()
+      throws ConfigInvalidException, IOException {
     // Don't include initial parse in timer, as this might do more I/O to page
     // in the block containing most commits. Later reads are not guaranteed to
     // avoid I/O, but often should.
@@ -162,10 +166,41 @@ class ChangeNotesParser {
       updatePatchSetStates();
       checkMandatoryFooters();
     }
+
+    return buildState();
   }
 
-  ImmutableListMultimap<PatchSet.Id, PatchSetApproval>
-      buildApprovals() {
+  RevisionNoteMap getRevisionNoteMap() {
+    return revisionNoteMap;
+  }
+
+  private ChangeNotesState buildState() {
+    return ChangeNotesState.create(
+        id,
+        new Change.Key(changeId),
+        createdOn,
+        lastUpdatedOn,
+        ownerId,
+        branch,
+        currentPatchSetId,
+        subject,
+        topic,
+        originalSubject,
+        submissionId,
+        status,
+
+        hashtags,
+        patchSets,
+        buildApprovals(),
+        ReviewerSet.fromTable(Tables.transpose(reviewers)),
+        allPastReviewers,
+        submitRecords,
+        buildAllMessages(),
+        buildMessagesByPatchSet(),
+        comments);
+  }
+
+  private Multimap<PatchSet.Id, PatchSetApproval> buildApprovals() {
     Multimap<PatchSet.Id, PatchSetApproval> result =
         ArrayListMultimap.create(approvals.keySet().size(), 3);
     for (Table<?, ?, Optional<PatchSetApproval>> curr : approvals.values()) {
@@ -178,19 +213,19 @@ class ChangeNotesParser {
     for (Collection<PatchSetApproval> v : result.asMap().values()) {
       Collections.sort((List<PatchSetApproval>) v, ChangeNotes.PSA_BY_TIME);
     }
-    return ImmutableListMultimap.copyOf(result);
+    return result;
   }
 
-  ImmutableList<ChangeMessage> buildAllMessages() {
-    return ImmutableList.copyOf(Lists.reverse(allChangeMessages));
+  private List<ChangeMessage> buildAllMessages() {
+    return Lists.reverse(allChangeMessages);
   }
 
-  ImmutableListMultimap<PatchSet.Id, ChangeMessage> buildMessagesByPatchSet() {
+  private Multimap<PatchSet.Id, ChangeMessage> buildMessagesByPatchSet() {
     for (Collection<ChangeMessage> v :
         changeMessagesByPatchSet.asMap().values()) {
       Collections.reverse((List<ChangeMessage>) v);
     }
-    return ImmutableListMultimap.copyOf(changeMessagesByPatchSet);
+    return changeMessagesByPatchSet;
   }
 
   private void parse(ChangeNotesCommit commit) throws ConfigInvalidException {

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

@@ -0,0 +1,185 @@
+// 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.Preconditions.checkNotNull;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableListMultimap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSortedMap;
+import com.google.common.collect.Multimap;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.common.data.SubmitRecord;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.Branch;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.ChangeMessage;
+import com.google.gerrit.reviewdb.client.PatchLineComment;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.PatchSetApproval;
+import com.google.gerrit.reviewdb.client.RevId;
+import com.google.gerrit.reviewdb.server.ReviewDbUtil;
+import com.google.gerrit.server.ReviewerSet;
+
+import java.sql.Timestamp;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * Immutable state associated with a change meta ref at a given commit.
+ * <p>
+ * One instance is the output of a single {@link ChangeNotesParser}, and
+ * contains types required to support public methods on {@link ChangeNotes}. It
+ * is intended to be cached in-process.
+ * <p>
+ * Note that {@link ChangeNotes} contains more than just a single {@code
+ * ChangeNoteState}, such as per-draft information, so that class is not cached
+ * directly.
+ */
+@AutoValue
+abstract class ChangeNotesState {
+  static ChangeNotesState empty(Change change) {
+    return new AutoValue_ChangeNotesState(
+        change.getId(),
+        null,
+        ImmutableSet.<String>of(),
+        ImmutableSortedMap.<PatchSet.Id, PatchSet>of(),
+        ImmutableListMultimap.<PatchSet.Id, PatchSetApproval>of(),
+        ReviewerSet.empty(),
+        ImmutableList.<Account.Id>of(),
+        ImmutableList.<SubmitRecord>of(),
+        ImmutableList.<ChangeMessage>of(),
+        ImmutableListMultimap.<PatchSet.Id, ChangeMessage>of(),
+        ImmutableListMultimap.<RevId, PatchLineComment>of());
+  }
+
+  static ChangeNotesState create(
+      Change.Id changeId,
+      Change.Key changeKey,
+      Timestamp createdOn,
+      Timestamp lastUpdatedOn,
+      Account.Id owner,
+      String branch,
+      @Nullable PatchSet.Id currentPatchSetId,
+      String subject,
+      @Nullable String topic,
+      @Nullable String originalSubject,
+      @Nullable String submissionId,
+      @Nullable Change.Status status,
+      @Nullable Set<String> hashtags,
+      Map<PatchSet.Id, PatchSet> patchSets,
+      Multimap<PatchSet.Id, PatchSetApproval> approvals,
+      ReviewerSet reviewers,
+      List<Account.Id> allPastReviewers,
+      List<SubmitRecord> submitRecords,
+      List<ChangeMessage> allChangeMessages,
+      Multimap<PatchSet.Id, ChangeMessage> changeMessagesByPatchSet,
+      Multimap<RevId, PatchLineComment> publishedComments) {
+    if (hashtags == null) {
+      hashtags = ImmutableSet.of();
+    }
+    return new AutoValue_ChangeNotesState(
+        changeId,
+        new AutoValue_ChangeNotesState_ChangeColumns(
+            changeKey,
+            createdOn,
+            lastUpdatedOn,
+            owner,
+            branch,
+            currentPatchSetId,
+            subject,
+            topic,
+            originalSubject,
+            submissionId,
+            status),
+        ImmutableSet.copyOf(hashtags),
+        ImmutableSortedMap.copyOf(patchSets, ReviewDbUtil.intKeyOrdering()),
+        ImmutableListMultimap.copyOf(approvals),
+        reviewers,
+        ImmutableList.copyOf(allPastReviewers),
+        ImmutableList.copyOf(submitRecords),
+        ImmutableList.copyOf(allChangeMessages),
+        ImmutableListMultimap.copyOf(changeMessagesByPatchSet),
+        ImmutableListMultimap.copyOf(publishedComments));
+  }
+
+
+  /**
+   * Subset of Change columns that can be represented in NoteDb.
+   * <p>
+   * Notable exceptions include rowVersion and noteDbState, which are only make
+   * sense when read from NoteDb, so they cannot be cached.
+   * <p>
+   * Fields are in listed column order.
+   */
+  @AutoValue
+  static abstract class ChangeColumns {
+    abstract Change.Key changeKey();
+    abstract Timestamp createdOn();
+    abstract Timestamp lastUpdatedOn();
+    abstract Account.Id owner();
+    abstract String branch(); // Project not included.
+    @Nullable abstract PatchSet.Id currentPatchSetId();
+    abstract String subject();
+    @Nullable abstract String topic();
+    @Nullable abstract String originalSubject();
+    @Nullable abstract String submissionId();
+    // TODO(dborowitz): Use a sensible default other than null
+    @Nullable abstract Change.Status status();
+  }
+
+  abstract Change.Id changeId();
+
+  @Nullable abstract ChangeColumns columns();
+
+  // Other related to this Change.
+  abstract ImmutableSet<String> hashtags();
+  abstract ImmutableSortedMap<PatchSet.Id, PatchSet> patchSets();
+  abstract ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals();
+  abstract ReviewerSet reviewers();
+  abstract ImmutableList<Account.Id> allPastReviewers();
+  abstract ImmutableList<SubmitRecord> submitRecords();
+  abstract ImmutableList<ChangeMessage> allChangeMessages();
+  abstract ImmutableListMultimap<PatchSet.Id, ChangeMessage>
+      changeMessagesByPatchSet();
+  abstract ImmutableListMultimap<RevId, PatchLineComment> publishedComments();
+
+  void copyColumnsTo(Change change) {
+    ChangeColumns c = checkNotNull(columns());
+    if (c.status() != null) {
+      change.setStatus(c.status());
+    }
+    change.setKey(c.changeKey());
+    change.setDest(new Branch.NameKey(change.getProject(), c.branch()));
+    change.setTopic(Strings.emptyToNull(c.topic()));
+    change.setCreatedOn(c.createdOn());
+    change.setLastUpdatedOn(c.lastUpdatedOn());
+    change.setOwner(c.owner());
+    change.setSubmissionId(c.submissionId());
+
+    if (!patchSets().isEmpty()) {
+      change.setCurrentPatchSet(
+          c.currentPatchSetId(), c.subject(), c.originalSubject());
+    } else {
+      // TODO(dborowitz): This should be an error, but for now it's required for
+      // some tests to pass.
+      change.clearCurrentPatchSet();
+    }
+  }
+}

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

@@ -17,6 +17,7 @@ package com.google.gerrit.server.notedb;
 import static org.junit.Assert.fail;
 
 import com.google.gerrit.common.TimeUtil;
+import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
 
 import org.eclipse.jgit.errors.ConfigInvalidException;
@@ -311,21 +312,21 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
   public void parseCommit() throws Exception {
     assertParseSucceeds("Update change\n"
         + "\n"
-        + "Patch-set: 1\n"
+        + "Patch-set: 2\n"
         + "Branch: refs/heads/master\n"
         + "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
         + "Subject: Some subject of a change\n"
         + "Commit: abcd1234abcd1234abcd1234abcd1234abcd1234");
     assertParseFails("Update change\n"
         + "\n"
-        + "Patch-set: 1\n"
+        + "Patch-set: 2\n"
         + "Branch: refs/heads/master\n"
         + "Subject: Some subject of a change\n"
         + "Commit: abcd1234abcd1234abcd1234abcd1234abcd1234\n"
         + "Commit: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
     assertParseFails("Update patch set 1\n"
         + "Uploaded patch set 1.\n"
-        + "Patch-set: 1\n"
+        + "Patch-set: 2\n"
         + "Branch: refs/heads/master\n"
         + "Subject: Some subject of a change\n"
         + "Commit: beef");
@@ -362,22 +363,15 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
   public void parsePatchSetGroups() throws Exception {
     assertParseSucceeds("Update change\n"
         + "\n"
-        + "Patch-set: 1\n"
+        + "Patch-set: 2\n"
         + "Branch: refs/heads/master\n"
         + "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
         + "Commit: abcd1234abcd1234abcd1234abcd1234abcd1234\n"
         + "Subject: Change subject\n"
         + "Groups: a,b,c\n");
-    // No patch set commit parsed on which we can set groups.
     assertParseFails("Update change\n"
         + "\n"
-        + "Patch-set: 1\n"
-        + "Branch: refs/heads/master\n"
-        + "Subject: Change subject\n"
-        + "Groups: a,b,c\n");
-    assertParseFails("Update change\n"
-        + "\n"
-        + "Patch-set: 1\n"
+        + "Patch-set: 2\n"
         + "Branch: refs/heads/master\n"
         + "Commit: abcd1234abcd1234abcd1234abcd1234abcd1234\n"
         + "Subject: Change subject\n"
@@ -462,8 +456,11 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
 
   private RevCommit writeCommit(String body, PersonIdent author)
       throws Exception {
+    Change change = newChange();
+    ChangeNotes notes = newNotes(change).load();
     try (ObjectInserter ins = testRepo.getRepository().newObjectInserter()) {
       CommitBuilder cb = new CommitBuilder();
+      cb.setParentId(notes.getRevision());
       cb.setAuthor(author);
       cb.setCommitter(new PersonIdent(serverIdent, author.getWhen()));
       cb.setTreeId(testRepo.tree());
@@ -498,6 +495,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
   }
 
   private ChangeNotesParser newParser(ObjectId tip) throws Exception {
+    walk.reset();
     return new ChangeNotesParser(
         newChange().getId(), tip, walk, noteUtil, args.metrics);
   }

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

@@ -1056,21 +1056,17 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     try (ChangeNotesRevWalk rw = ChangeNotesCommit.newRevWalk(repo)) {
       ChangeNotesParser notesWithComments = new ChangeNotesParser(
           c.getId(), commitWithComments.copy(), rw, noteUtil, args.metrics);
-      notesWithComments.parseAll();
-      ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals1 =
-          notesWithComments.buildApprovals();
-      assertThat(approvals1).isEmpty();
-      assertThat(notesWithComments.comments).hasSize(1);
+      ChangeNotesState state = notesWithComments.parseAll();
+      assertThat(state.approvals()).isEmpty();
+      assertThat(state.publishedComments()).hasSize(1);
     }
 
     try (ChangeNotesRevWalk rw = ChangeNotesCommit.newRevWalk(repo)) {
       ChangeNotesParser notesWithApprovals = new ChangeNotesParser(c.getId(),
           commitWithApprovals.copy(), rw, noteUtil, args.metrics);
-      notesWithApprovals.parseAll();
-      ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals2 =
-          notesWithApprovals.buildApprovals();
-      assertThat(approvals2).hasSize(1);
-      assertThat(notesWithApprovals.comments).hasSize(1);
+      ChangeNotesState state = notesWithApprovals.parseAll();
+      assertThat(state.approvals()).hasSize(1);
+      assertThat(state.publishedComments()).hasSize(1);
     }
   }