Split ChangeNoteUtil into JSON/Read/Write parts

The write part needs the AccountCache, which is problematic in the
schema migration.

Change-Id: I2b8a6a0d946c765588107fb10abd19b96aef274c

java/com/google/gerrit/server/notedb/AbstractChangeNotes.java

@@ -48,7 +48,8 @@ public abstract class AbstractChangeNotes<T> {
     final GitRepositoryManager repoManager;
     final NotesMigration migration;
     final AllUsersName allUsers;
-    final ChangeNoteUtil noteUtil;
+    final ChangeNoteJson changeNoteJson;
+    final LegacyChangeNoteRead legacyChangeNoteRead;
     final NoteDbMetrics metrics;
     final Provider<ReviewDb> db;
 
@@ -65,7 +66,8 @@ public abstract class AbstractChangeNotes<T> {
         GitRepositoryManager repoManager,
         NotesMigration migration,
         AllUsersName allUsers,
-        ChangeNoteUtil noteUtil,
+        ChangeNoteJson changeNoteJson,
+        LegacyChangeNoteRead legacyChangeNoteRead,
         NoteDbMetrics metrics,
         Provider<ReviewDb> db,
         Provider<ChangeRebuilder> rebuilder,
@@ -73,7 +75,8 @@ public abstract class AbstractChangeNotes<T> {
       this.repoManager = repoManager;
       this.migration = migration;
       this.allUsers = allUsers;
-      this.noteUtil = noteUtil;
+      this.legacyChangeNoteRead = legacyChangeNoteRead;
+      this.changeNoteJson = changeNoteJson;
       this.metrics = metrics;
       this.db = db;
       this.rebuilder = rebuilder;

java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java

@@ -120,7 +120,9 @@ public abstract class AbstractChangeUpdate {
       ChangeNoteUtil noteUtil, PersonIdent serverIdent, CurrentUser u, Date when) {
     checkUserType(u);
     if (u instanceof IdentifiedUser) {
-      return noteUtil.newIdent(u.asIdentifiedUser().getAccount(), when, serverIdent);
+      return noteUtil
+          .getLegacyChangeNoteWrite()
+          .newIdent(u.asIdentifiedUser().getAccount(), when, serverIdent);
     } else if (u instanceof InternalUser) {
       return serverIdent;
     }
@@ -175,7 +177,7 @@ public abstract class AbstractChangeUpdate {
   }
 
   protected PersonIdent newIdent(Account.Id authorId, Date when) {
-    return noteUtil.newIdent(authorId, when, serverIdent);
+    return noteUtil.getLegacyChangeNoteWrite().newIdent(authorId, when, serverIdent);
   }
 
   /** Whether no updates have been done. */

java/com/google/gerrit/server/notedb/ChangeBundle.java

@@ -505,11 +505,11 @@ public class ChangeBundle {
     if (rn >= 0) {
       s = s.substring(0, rn);
     }
-    return ChangeNoteUtil.sanitizeFooter(s);
+    return NoteDbUtil.sanitizeFooter(s);
   }
 
   private static String cleanNoteDbSubject(String s) {
-    return ChangeNoteUtil.sanitizeFooter(s);
+    return NoteDbUtil.sanitizeFooter(s);
   }
 
   /**

java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java

@@ -178,7 +178,12 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
     for (Map.Entry<RevId, RevisionNoteBuilder> e : builders.entrySet()) {
       updatedRevs.add(e.getKey());
       ObjectId id = ObjectId.fromString(e.getKey().get());
-      byte[] data = e.getValue().build(noteUtil, noteUtil.getWriteJson());
+      byte[] data =
+          e.getValue()
+              .build(
+                  noteUtil.getChangeNoteJson(),
+                  noteUtil.getLegacyChangeNoteWrite(),
+                  noteUtil.getChangeNoteJson().getWriteJson());
       if (!Arrays.equals(data, e.getValue().baseRaw)) {
         touchedAnyRevs = true;
       }
@@ -236,7 +241,12 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
     // 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(
-        noteUtil, getId(), rw.getObjectReader(), noteMap, PatchLineComment.Status.DRAFT);
+        noteUtil.getChangeNoteJson(),
+        noteUtil.getLegacyChangeNoteRead(),
+        getId(),
+        rw.getObjectReader(),
+        noteMap,
+        PatchLineComment.Status.DRAFT);
   }
 
   @Override

java/com/google/gerrit/server/notedb/ChangeNoteJson.java

@@ -0,0 +1,49 @@
+// Copyright (C) 2018 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.gerrit.server.config.GerritServerConfig;
+import com.google.gson.Gson;
+import com.google.gson.GsonBuilder;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.sql.Timestamp;
+import org.eclipse.jgit.lib.Config;
+
+@Singleton
+public class ChangeNoteJson {
+  private final Gson gson = newGson();
+  private final boolean writeJson;
+
+  static Gson newGson() {
+    return new GsonBuilder()
+        .registerTypeAdapter(Timestamp.class, new CommentTimestampAdapter().nullSafe())
+        .setPrettyPrinting()
+        .create();
+  }
+
+  public Gson getGson() {
+    return gson;
+  }
+
+  public boolean getWriteJson() {
+    return writeJson;
+  }
+
+  @Inject
+  ChangeNoteJson(@GerritServerConfig Config config) {
+    this.writeJson = config.getBoolean("notedb", "writeJson", true);
+  }
+}

java/com/google/gerrit/server/notedb/ChangeNoteUtil.java

@@ -14,52 +14,8 @@
 
 package com.google.gerrit.server.notedb;
 
-import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.gerrit.server.CommentsUtil.COMMENT_ORDER;
-import static com.google.gerrit.server.notedb.ChangeNotes.parseException;
-import static java.nio.charset.StandardCharsets.UTF_8;
-
-import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.CharMatcher;
-import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ListMultimap;
-import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.Change;
-import com.google.gerrit.reviewdb.client.Comment;
-import com.google.gerrit.reviewdb.client.CommentRange;
-import com.google.gerrit.reviewdb.client.PatchSet;
-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.account.AccountState;
-import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.gerrit.server.config.GerritServerId;
-import com.google.gson.Gson;
-import com.google.gson.GsonBuilder;
 import com.google.inject.Inject;
-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.HashSet;
-import java.util.List;
-import java.util.Locale;
-import java.util.Optional;
-import java.util.Set;
-import org.eclipse.jgit.errors.ConfigInvalidException;
-import org.eclipse.jgit.lib.Config;
-import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.revwalk.FooterKey;
-import org.eclipse.jgit.util.GitDateFormatter;
-import org.eclipse.jgit.util.GitDateFormatter.Format;
-import org.eclipse.jgit.util.GitDateParser;
-import org.eclipse.jgit.util.MutableInteger;
-import org.eclipse.jgit.util.QuotedString;
-import org.eclipse.jgit.util.RawParseUtils;
 
 public class ChangeNoteUtil {
   public static final FooterKey FOOTER_ASSIGNEE = new FooterKey("Assignee");
@@ -85,560 +41,43 @@ public class ChangeNoteUtil {
   public static final FooterKey FOOTER_WORK_IN_PROGRESS = new FooterKey("Work-in-progress");
   public static final FooterKey FOOTER_REVERT_OF = new FooterKey("Revert-of");
 
-  private static final String AUTHOR = "Author";
+  static final String AUTHOR = "Author";
-  private static final String BASE_PATCH_SET = "Base-for-patch-set";
+  static final String BASE_PATCH_SET = "Base-for-patch-set";
-  private static final String COMMENT_RANGE = "Comment-range";
+  static final String COMMENT_RANGE = "Comment-range";
-  private static final String FILE = "File";
+  static final String FILE = "File";
-  private static final String LENGTH = "Bytes";
+  static final String LENGTH = "Bytes";
-  private static final String PARENT = "Parent";
+  static final String PARENT = "Parent";
-  private static final String PARENT_NUMBER = "Parent-number";
+  static final String PARENT_NUMBER = "Parent-number";
-  private static final String PATCH_SET = "Patch-set";
+  static final String PATCH_SET = "Patch-set";
-  private static final String REAL_AUTHOR = "Real-author";
+  static final String REAL_AUTHOR = "Real-author";
-  private static final String REVISION = "Revision";
+  static final String REVISION = "Revision";
-  private static final String UUID = "UUID";
+  static final String UUID = "UUID";
-  private static final String UNRESOLVED = "Unresolved";
+  static final String UNRESOLVED = "Unresolved";
-  private static final String TAG = FOOTER_TAG.getName();
+  static final String TAG = FOOTER_TAG.getName();
 
-  public static String formatTime(PersonIdent ident, Timestamp t) {
+  private final LegacyChangeNoteRead legacyChangeNoteRead;
-    GitDateFormatter dateFormatter = new GitDateFormatter(Format.DEFAULT);
+  private final LegacyChangeNoteWrite legacyChangeNoteWrite;
-    // TODO(dborowitz): Use a ThreadLocal or use Joda.
+  private final ChangeNoteJson changeNoteJson;
-    PersonIdent newIdent = new PersonIdent(ident, t);
-    return dateFormatter.formatDate(newIdent);
-  }
-
-  static Gson newGson() {
-    return new GsonBuilder()
-        .registerTypeAdapter(Timestamp.class, new CommentTimestampAdapter().nullSafe())
-        .setPrettyPrinting()
-        .create();
-  }
-
-  private final AccountCache accountCache;
-  private final PersonIdent serverIdent;
-  private final String serverId;
-  private final Gson gson = newGson();
-  private final boolean writeJson;
 
   @Inject
   public ChangeNoteUtil(
-      AccountCache accountCache,
+      ChangeNoteJson changeNoteJson,
-      @GerritPersonIdent PersonIdent serverIdent,
+      LegacyChangeNoteRead legacyChangeNoteRead,
-      @GerritServerId String serverId,
+      LegacyChangeNoteWrite legacyChangeNoteWrite) {
-      @GerritServerConfig Config config) {
+    this.changeNoteJson = changeNoteJson;
-    this.accountCache = accountCache;
+    this.legacyChangeNoteRead = legacyChangeNoteRead;
-    this.serverIdent = serverIdent;
+    this.legacyChangeNoteWrite = legacyChangeNoteWrite;
-    this.serverId = serverId;
-    this.writeJson = config.getBoolean("notedb", "writeJson", true);
   }
 
-  public PersonIdent newIdent(Account.Id authorId, Date when, PersonIdent serverIdent) {
+  public LegacyChangeNoteRead getLegacyChangeNoteRead() {
-    Optional<Account> author = accountCache.get(authorId).map(AccountState::getAccount);
+    return legacyChangeNoteRead;
-    return new PersonIdent(
-        author.map(Account::getName).orElseGet(() -> Account.getName(authorId)),
-        authorId.get() + "@" + serverId,
-        when,
-        serverIdent.getTimeZone());
   }
 
-  @VisibleForTesting
+  public ChangeNoteJson getChangeNoteJson() {
-  public PersonIdent newIdent(Account author, Date when, PersonIdent serverIdent) {
+    return changeNoteJson;
-    return new PersonIdent(
-        author.getName(), author.getId().get() + "@" + serverId, when, serverIdent.getTimeZone());
   }
 
-  public boolean getWriteJson() {
+  public LegacyChangeNoteWrite getLegacyChangeNoteWrite() {
-    return writeJson;
+    return legacyChangeNoteWrite;
-  }
-
-  public Gson getGson() {
-    return gson;
-  }
-
-  public String getServerId() {
-    return serverId;
-  }
-
-  public Account.Id parseIdent(PersonIdent ident, Change.Id changeId)
-      throws ConfigInvalidException {
-    return NoteDbUtil.parseIdent(ident, serverId)
-        .orElseThrow(
-            () ->
-                parseException(
-                    changeId,
-                    "invalid identity, expected <id>@%s: %s",
-                    serverId,
-                    ident.getEmailAddress()));
-  }
-
-  private static boolean match(byte[] note, MutableInteger p, byte[] expected) {
-    int m = RawParseUtils.match(note, p.value, expected);
-    return m == p.value + expected.length;
-  }
-
-  public List<Comment> parseNote(byte[] note, MutableInteger p, Change.Id changeId)
-      throws ConfigInvalidException {
-    if (p.value >= note.length) {
-      return ImmutableList.of();
-    }
-    Set<Comment.Key> seen = new HashSet<>();
-    List<Comment> result = new ArrayList<>();
-    int sizeOfNote = note.length;
-    byte[] psb = PATCH_SET.getBytes(UTF_8);
-    byte[] bpsb = BASE_PATCH_SET.getBytes(UTF_8);
-    byte[] bpn = PARENT_NUMBER.getBytes(UTF_8);
-
-    RevId revId = new RevId(parseStringField(note, p, changeId, REVISION));
-    String fileName = null;
-    PatchSet.Id psId = null;
-    boolean isForBase = false;
-    Integer parentNumber = null;
-
-    while (p.value < sizeOfNote) {
-      boolean matchPs = match(note, p, psb);
-      boolean matchBase = match(note, p, bpsb);
-      if (matchPs) {
-        fileName = null;
-        psId = parsePsId(note, p, changeId, PATCH_SET);
-        isForBase = false;
-      } else if (matchBase) {
-        fileName = null;
-        psId = parsePsId(note, p, changeId, BASE_PATCH_SET);
-        isForBase = true;
-        if (match(note, p, bpn)) {
-          parentNumber = parseParentNumber(note, p, changeId);
-        }
-      } else if (psId == null) {
-        throw parseException(changeId, "missing %s or %s header", PATCH_SET, BASE_PATCH_SET);
-      }
-
-      Comment c = parseComment(note, p, fileName, psId, revId, isForBase, parentNumber);
-      fileName = c.key.filename;
-      if (!seen.add(c.key)) {
-        throw parseException(changeId, "multiple comments for %s in note", c.key);
-      }
-      result.add(c);
-    }
-    return result;
-  }
-
-  private Comment parseComment(
-      byte[] note,
-      MutableInteger curr,
-      String currentFileName,
-      PatchSet.Id psId,
-      RevId revId,
-      boolean isForBase,
-      Integer parentNumber)
-      throws ConfigInvalidException {
-    Change.Id changeId = psId.getParentKey();
-
-    // Check if there is a new file.
-    boolean newFile = (RawParseUtils.match(note, curr.value, FILE.getBytes(UTF_8))) != -1;
-    if (newFile) {
-      // If so, parse the new file name.
-      currentFileName = parseFilename(note, curr, changeId);
-    } else if (currentFileName == null) {
-      throw parseException(changeId, "could not parse %s", FILE);
-    }
-
-    CommentRange range = parseCommentRange(note, curr);
-    if (range == null) {
-      throw parseException(changeId, "could not parse %s", COMMENT_RANGE);
-    }
-
-    Timestamp commentTime = parseTimestamp(note, curr, changeId);
-    Account.Id aId = parseAuthor(note, curr, changeId, AUTHOR);
-    boolean hasRealAuthor =
-        (RawParseUtils.match(note, curr.value, REAL_AUTHOR.getBytes(UTF_8))) != -1;
-    Account.Id raId = null;
-    if (hasRealAuthor) {
-      raId = parseAuthor(note, curr, changeId, REAL_AUTHOR);
-    }
-
-    boolean hasParent = (RawParseUtils.match(note, curr.value, PARENT.getBytes(UTF_8))) != -1;
-    String parentUUID = null;
-    boolean unresolved = false;
-    if (hasParent) {
-      parentUUID = parseStringField(note, curr, changeId, PARENT);
-    }
-    boolean hasUnresolved =
-        (RawParseUtils.match(note, curr.value, UNRESOLVED.getBytes(UTF_8))) != -1;
-    if (hasUnresolved) {
-      unresolved = parseBooleanField(note, curr, changeId, UNRESOLVED);
-    }
-
-    String uuid = parseStringField(note, curr, changeId, UUID);
-
-    boolean hasTag = (RawParseUtils.match(note, curr.value, TAG.getBytes(UTF_8))) != -1;
-    String tag = null;
-    if (hasTag) {
-      tag = parseStringField(note, curr, changeId, TAG);
-    }
-
-    int commentLength = parseCommentLength(note, curr, changeId);
-
-    String message = RawParseUtils.decode(UTF_8, note, curr.value, curr.value + commentLength);
-    checkResult(message, "message contents", changeId);
-
-    Comment c =
-        new Comment(
-            new Comment.Key(uuid, currentFileName, psId.get()),
-            aId,
-            commentTime,
-            isForBase ? (short) (parentNumber == null ? 0 : -parentNumber) : (short) 1,
-            message,
-            serverId,
-            unresolved);
-    c.lineNbr = range.getEndLine();
-    c.parentUuid = parentUUID;
-    c.tag = tag;
-    c.setRevId(revId);
-    if (raId != null) {
-      c.setRealAuthor(raId);
-    }
-
-    if (range.getStartCharacter() != -1) {
-      c.setRange(range);
-    }
-
-    curr.value = RawParseUtils.nextLF(note, curr.value + commentLength);
-    curr.value = RawParseUtils.nextLF(note, curr.value);
-    return c;
-  }
-
-  private static String parseStringField(
-      byte[] note, MutableInteger curr, Change.Id changeId, String fieldName)
-      throws ConfigInvalidException {
-    int endOfLine = RawParseUtils.nextLF(note, curr.value);
-    checkHeaderLineFormat(note, curr, fieldName, changeId);
-    int startOfField = RawParseUtils.endOfFooterLineKey(note, curr.value) + 2;
-    curr.value = endOfLine;
-    return RawParseUtils.decode(UTF_8, note, startOfField, endOfLine - 1);
-  }
-
-  /**
-   * @return a comment range. If the comment range line in the note only has one number, we return a
-   *     CommentRange with that one number as the end line and the other fields as -1. If the
-   *     comment range line in the note contains a whole comment range, then we return a
-   *     CommentRange with all fields set. If the line is not correctly formatted, return null.
-   */
-  private static CommentRange parseCommentRange(byte[] note, MutableInteger ptr) {
-    CommentRange range = new CommentRange(-1, -1, -1, -1);
-
-    int last = ptr.value;
-    int startLine = RawParseUtils.parseBase10(note, ptr.value, ptr);
-    if (ptr.value == last) {
-      return null;
-    } else if (note[ptr.value] == '\n') {
-      range.setEndLine(startLine);
-      ptr.value += 1;
-      return range;
-    } else if (note[ptr.value] == ':') {
-      range.setStartLine(startLine);
-      ptr.value += 1;
-    } else {
-      return null;
-    }
-
-    last = ptr.value;
-    int startChar = RawParseUtils.parseBase10(note, ptr.value, ptr);
-    if (ptr.value == last) {
-      return null;
-    } else if (note[ptr.value] == '-') {
-      range.setStartCharacter(startChar);
-      ptr.value += 1;
-    } else {
-      return null;
-    }
-
-    last = ptr.value;
-    int endLine = RawParseUtils.parseBase10(note, ptr.value, ptr);
-    if (ptr.value == last) {
-      return null;
-    } else if (note[ptr.value] == ':') {
-      range.setEndLine(endLine);
-      ptr.value += 1;
-    } else {
-      return null;
-    }
-
-    last = ptr.value;
-    int endChar = RawParseUtils.parseBase10(note, ptr.value, ptr);
-    if (ptr.value == last) {
-      return null;
-    } else if (note[ptr.value] == '\n') {
-      range.setEndCharacter(endChar);
-      ptr.value += 1;
-    } else {
-      return null;
-    }
-    return range;
-  }
-
-  private static PatchSet.Id parsePsId(
-      byte[] note, MutableInteger curr, Change.Id changeId, String fieldName)
-      throws ConfigInvalidException {
-    checkHeaderLineFormat(note, curr, fieldName, changeId);
-    int startOfPsId = RawParseUtils.endOfFooterLineKey(note, curr.value) + 1;
-    MutableInteger i = new MutableInteger();
-    int patchSetId = RawParseUtils.parseBase10(note, startOfPsId, i);
-    int endOfLine = RawParseUtils.nextLF(note, curr.value);
-    if (i.value != endOfLine - 1) {
-      throw parseException(changeId, "could not parse %s", fieldName);
-    }
-    checkResult(patchSetId, "patchset id", changeId);
-    curr.value = endOfLine;
-    return new PatchSet.Id(changeId, patchSetId);
-  }
-
-  private static Integer parseParentNumber(byte[] note, MutableInteger curr, Change.Id changeId)
-      throws ConfigInvalidException {
-    checkHeaderLineFormat(note, curr, PARENT_NUMBER, changeId);
-
-    int start = RawParseUtils.endOfFooterLineKey(note, curr.value) + 1;
-    MutableInteger i = new MutableInteger();
-    int parentNumber = RawParseUtils.parseBase10(note, start, i);
-    int endOfLine = RawParseUtils.nextLF(note, curr.value);
-    if (i.value != endOfLine - 1) {
-      throw parseException(changeId, "could not parse %s", PARENT_NUMBER);
-    }
-    checkResult(parentNumber, "parent number", changeId);
-    curr.value = endOfLine;
-    return Integer.valueOf(parentNumber);
-  }
-
-  private static String parseFilename(byte[] note, MutableInteger curr, Change.Id changeId)
-      throws ConfigInvalidException {
-    checkHeaderLineFormat(note, curr, FILE, changeId);
-    int startOfFileName = RawParseUtils.endOfFooterLineKey(note, curr.value) + 2;
-    int endOfLine = RawParseUtils.nextLF(note, curr.value);
-    curr.value = endOfLine;
-    curr.value = RawParseUtils.nextLF(note, curr.value);
-    return QuotedString.GIT_PATH.dequote(
-        RawParseUtils.decode(UTF_8, note, startOfFileName, endOfLine - 1));
-  }
-
-  private static Timestamp parseTimestamp(byte[] note, MutableInteger curr, Change.Id changeId)
-      throws ConfigInvalidException {
-    int endOfLine = RawParseUtils.nextLF(note, curr.value);
-    Timestamp commentTime;
-    String dateString = RawParseUtils.decode(UTF_8, note, curr.value, endOfLine - 1);
-    try {
-      commentTime = new Timestamp(GitDateParser.parse(dateString, null, Locale.US).getTime());
-    } catch (ParseException e) {
-      throw new ConfigInvalidException("could not parse comment timestamp", e);
-    }
-    curr.value = endOfLine;
-    return checkResult(commentTime, "comment timestamp", changeId);
-  }
-
-  private Account.Id parseAuthor(
-      byte[] note, MutableInteger curr, Change.Id changeId, String fieldName)
-      throws ConfigInvalidException {
-    checkHeaderLineFormat(note, curr, fieldName, changeId);
-    int startOfAccountId = RawParseUtils.endOfFooterLineKey(note, curr.value) + 2;
-    PersonIdent ident = RawParseUtils.parsePersonIdent(note, startOfAccountId);
-    Account.Id aId = parseIdent(ident, changeId);
-    curr.value = RawParseUtils.nextLF(note, curr.value);
-    return checkResult(aId, fieldName, changeId);
-  }
-
-  private static int parseCommentLength(byte[] note, MutableInteger curr, Change.Id changeId)
-      throws ConfigInvalidException {
-    checkHeaderLineFormat(note, curr, LENGTH, changeId);
-    int startOfLength = RawParseUtils.endOfFooterLineKey(note, curr.value) + 1;
-    MutableInteger i = new MutableInteger();
-    i.value = startOfLength;
-    int commentLength = RawParseUtils.parseBase10(note, startOfLength, i);
-    if (i.value == startOfLength) {
-      throw parseException(changeId, "could not parse %s", LENGTH);
-    }
-    int endOfLine = RawParseUtils.nextLF(note, curr.value);
-    if (i.value != endOfLine - 1) {
-      throw parseException(changeId, "could not parse %s", LENGTH);
-    }
-    curr.value = endOfLine;
-    return checkResult(commentLength, "comment length", changeId);
-  }
-
-  private boolean parseBooleanField(
-      byte[] note, MutableInteger curr, Change.Id changeId, String fieldName)
-      throws ConfigInvalidException {
-    String str = parseStringField(note, curr, changeId, fieldName);
-    if ("true".equalsIgnoreCase(str)) {
-      return true;
-    } else if ("false".equalsIgnoreCase(str)) {
-      return false;
-    }
-    throw parseException(changeId, "invalid boolean for %s: %s", fieldName, str);
-  }
-
-  private static <T> T checkResult(T o, String fieldName, Change.Id changeId)
-      throws ConfigInvalidException {
-    if (o == null) {
-      throw parseException(changeId, "could not parse %s", fieldName);
-    }
-    return o;
-  }
-
-  private static int checkResult(int i, String fieldName, Change.Id changeId)
-      throws ConfigInvalidException {
-    if (i <= 0) {
-      throw parseException(changeId, "could not parse %s", fieldName);
-    }
-    return i;
-  }
-
-  private void appendHeaderField(PrintWriter writer, String field, String value) {
-    writer.print(field);
-    writer.print(": ");
-    writer.print(value);
-    writer.print('\n');
-  }
-
-  private static void checkHeaderLineFormat(
-      byte[] note, MutableInteger curr, String fieldName, Change.Id changeId)
-      throws ConfigInvalidException {
-    boolean correct = RawParseUtils.match(note, curr.value, fieldName.getBytes(UTF_8)) != -1;
-    int p = curr.value + fieldName.length();
-    correct &= (p < note.length && note[p] == ':');
-    p++;
-    correct &= (p < note.length && note[p] == ' ');
-    if (!correct) {
-      throw parseException(changeId, "could not parse %s", fieldName);
-    }
-  }
-
-  /**
-   * Build a note that contains the metadata for and the contents of all of the comments in the
-   * given comments.
-   *
-   * @param comments Comments to be written to the output stream, keyed by patch set ID; multiple
-   *     patch sets are allowed since base revisions may be shared across patch sets. All of the
-   *     comments must share the same RevId, and all the comments for a given patch set must have
-   *     the same side.
-   * @param out output stream to write to.
-   */
-  void buildNote(ListMultimap<Integer, Comment> comments, OutputStream out) {
-    if (comments.isEmpty()) {
-      return;
-    }
-
-    List<Integer> psIds = new ArrayList<>(comments.keySet());
-    Collections.sort(psIds);
-
-    OutputStreamWriter streamWriter = new OutputStreamWriter(out, UTF_8);
-    try (PrintWriter writer = new PrintWriter(streamWriter)) {
-      String revId = comments.values().iterator().next().revId;
-      appendHeaderField(writer, REVISION, revId);
-
-      for (int psId : psIds) {
-        List<Comment> psComments = COMMENT_ORDER.sortedCopy(comments.get(psId));
-        Comment first = psComments.get(0);
-
-        short side = first.side;
-        appendHeaderField(writer, side <= 0 ? BASE_PATCH_SET : PATCH_SET, Integer.toString(psId));
-        if (side < 0) {
-          appendHeaderField(writer, PARENT_NUMBER, Integer.toString(-side));
-        }
-
-        String currentFilename = null;
-
-        for (Comment c : psComments) {
-          checkArgument(
-              revId.equals(c.revId),
-              "All comments being added must have all the same RevId. The "
-                  + "comment below does not have the same RevId as the others "
-                  + "(%s).\n%s",
-              revId,
-              c);
-          checkArgument(
-              side == c.side,
-              "All comments being added must all have the same side. The "
-                  + "comment below does not have the same side as the others "
-                  + "(%s).\n%s",
-              side,
-              c);
-          String commentFilename = QuotedString.GIT_PATH.quote(c.key.filename);
-
-          if (!commentFilename.equals(currentFilename)) {
-            currentFilename = commentFilename;
-            writer.print("File: ");
-            writer.print(commentFilename);
-            writer.print("\n\n");
-          }
-
-          appendOneComment(writer, c);
-        }
-      }
-    }
-  }
-
-  private void appendOneComment(PrintWriter writer, Comment c) {
-    // The CommentRange field for a comment is allowed to be null. If it is
-    // null, then in the first line, we simply use the line number field for a
-    // comment instead. If it isn't null, we write the comment range itself.
-    Comment.Range range = c.range;
-    if (range != null) {
-      writer.print(range.startLine);
-      writer.print(':');
-      writer.print(range.startChar);
-      writer.print('-');
-      writer.print(range.endLine);
-      writer.print(':');
-      writer.print(range.endChar);
-    } else {
-      writer.print(c.lineNbr);
-    }
-    writer.print("\n");
-
-    writer.print(formatTime(serverIdent, c.writtenOn));
-    writer.print("\n");
-
-    appendIdent(writer, AUTHOR, c.author.getId(), c.writtenOn);
-    if (!c.getRealAuthor().equals(c.author)) {
-      appendIdent(writer, REAL_AUTHOR, c.getRealAuthor().getId(), c.writtenOn);
-    }
-
-    String parent = c.parentUuid;
-    if (parent != null) {
-      appendHeaderField(writer, PARENT, parent);
-    }
-
-    appendHeaderField(writer, UNRESOLVED, Boolean.toString(c.unresolved));
-    appendHeaderField(writer, UUID, c.key.uuid);
-
-    if (c.tag != null) {
-      appendHeaderField(writer, TAG, c.tag);
-    }
-
-    byte[] messageBytes = c.message.getBytes(UTF_8);
-    appendHeaderField(writer, LENGTH, Integer.toString(messageBytes.length));
-
-    writer.print(c.message);
-    writer.print("\n\n");
-  }
-
-  private void appendIdent(PrintWriter writer, String header, Account.Id id, Timestamp ts) {
-    PersonIdent ident = newIdent(id, ts, serverIdent);
-    StringBuilder name = new StringBuilder();
-    PersonIdent.appendSanitized(name, ident.getName());
-    name.append(" <");
-    PersonIdent.appendSanitized(name, ident.getEmailAddress());
-    name.append('>');
-    appendHeaderField(writer, header, name.toString());
-  }
-
-  private static final CharMatcher INVALID_FOOTER_CHARS = CharMatcher.anyOf("\r\n\0");
-
-  static String sanitizeFooter(String value) {
-    // Remove characters that would confuse JGit's footer parser if they were
-    // included in footer values, for example by splitting the footer block into
-    // multiple paragraphs.
-    //
-    // One painful example: RevCommit#getShorMessage() might return a message
-    // containing "\r\r", which RevCommit#getFooterLines() will treat as an
-    // empty paragraph for the purposes of footer parsing.
-    return INVALID_FOOTER_CHARS.trimAndCollapseFrom(value, ' ');
   }
 }

java/com/google/gerrit/server/notedb/ChangeNotesCache.java

@@ -346,7 +346,13 @@ public class ChangeNotesCache {
     @Override
     public ChangeNotesState call() throws ConfigInvalidException, IOException {
       ChangeNotesParser parser =
-          new ChangeNotesParser(key.changeId(), key.id(), rw, args.noteUtil, args.metrics);
+          new ChangeNotesParser(
+              key.changeId(),
+              key.id(),
+              rw,
+              args.changeNoteJson,
+              args.legacyChangeNoteRead,
+              args.metrics);
       ChangeNotesState result = parser.parseAll();
       // This assignment only happens if call() was actually called, which only
       // happens when Cache#get(K, Callable<V>) incurs a cache miss.

java/com/google/gerrit/server/notedb/ChangeNotesParser.java

@@ -124,7 +124,9 @@ class ChangeNotesParser {
   }
 
   // Private final members initialized in the constructor.
-  private final ChangeNoteUtil noteUtil;
+  private final ChangeNoteJson changeNoteJson;
+  private final LegacyChangeNoteRead legacyChangeNoteRead;
+
   private final NoteDbMetrics metrics;
   private final Change.Id id;
   private final ObjectId tip;
@@ -175,12 +177,14 @@ class ChangeNotesParser {
       Change.Id changeId,
       ObjectId tip,
       ChangeNotesRevWalk walk,
-      ChangeNoteUtil noteUtil,
+      ChangeNoteJson changeNoteJson,
+      LegacyChangeNoteRead legacyChangeNoteRead,
       NoteDbMetrics metrics) {
     this.id = changeId;
     this.tip = tip;
     this.walk = walk;
-    this.noteUtil = noteUtil;
+    this.changeNoteJson = changeNoteJson;
+    this.legacyChangeNoteRead = legacyChangeNoteRead;
     this.metrics = metrics;
     approvals = new LinkedHashMap<>();
     bufferedApprovals = new ArrayList<>();
@@ -446,7 +450,7 @@ class ChangeNotesParser {
       return effectiveAccountId;
     }
     PersonIdent ident = RawParseUtils.parsePersonIdent(realUser);
-    return noteUtil.parseIdent(ident, id);
+    return legacyChangeNoteRead.parseIdent(ident, id);
   }
 
   private String parseTopic(ChangeNotesCommit commit) throws ConfigInvalidException {
@@ -581,7 +585,7 @@ class ChangeNotesParser {
         parsedAssignee = Optional.empty();
       } else {
         PersonIdent ident = RawParseUtils.parsePersonIdent(assigneeValue);
-        parsedAssignee = Optional.ofNullable(noteUtil.parseIdent(ident, id));
+        parsedAssignee = Optional.ofNullable(legacyChangeNoteRead.parseIdent(ident, id));
       }
       if (assignee == null) {
         assignee = parsedAssignee;
@@ -749,7 +753,8 @@ class ChangeNotesParser {
     ChangeNotesCommit tipCommit = walk.parseCommit(tip);
     revisionNoteMap =
         RevisionNoteMap.parse(
-            noteUtil,
+            changeNoteJson,
+            legacyChangeNoteRead,
             id,
             reader,
             NoteMap.read(reader, tipCommit),
@@ -807,7 +812,7 @@ class ChangeNotesParser {
       labelVoteStr = line.substring(0, s);
       PersonIdent ident = RawParseUtils.parsePersonIdent(line.substring(s + 1));
       checkFooter(ident != null, FOOTER_LABEL, line);
-      effectiveAccountId = noteUtil.parseIdent(ident, id);
+      effectiveAccountId = legacyChangeNoteRead.parseIdent(ident, id);
     } else {
       labelVoteStr = line;
       effectiveAccountId = committerId;
@@ -849,7 +854,7 @@ class ChangeNotesParser {
       label = line.substring(1, s);
       PersonIdent ident = RawParseUtils.parsePersonIdent(line.substring(s + 1));
       checkFooter(ident != null, FOOTER_LABEL, line);
-      effectiveAccountId = noteUtil.parseIdent(ident, id);
+      effectiveAccountId = legacyChangeNoteRead.parseIdent(ident, id);
     } else {
       label = line.substring(1);
       effectiveAccountId = committerId;
@@ -913,7 +918,7 @@ class ChangeNotesParser {
           label.label = line.substring(c + 2, c2);
           PersonIdent ident = RawParseUtils.parsePersonIdent(line.substring(c2 + 2));
           checkFooter(ident != null, FOOTER_SUBMITTED_WITH, line);
-          label.appliedBy = noteUtil.parseIdent(ident, id);
+          label.appliedBy = legacyChangeNoteRead.parseIdent(ident, id);
         } else {
           label.label = line.substring(c + 2);
         }
@@ -929,7 +934,7 @@ class ChangeNotesParser {
     if (a.getName().equals(c.getName()) && a.getEmailAddress().equals(c.getEmailAddress())) {
       return null;
     }
-    return noteUtil.parseIdent(commit.getAuthorIdent(), id);
+    return legacyChangeNoteRead.parseIdent(commit.getAuthorIdent(), id);
   }
 
   private void parseReviewer(Timestamp ts, ReviewerStateInternal state, String line)
@@ -938,7 +943,7 @@ class ChangeNotesParser {
     if (ident == null) {
       throw invalidFooter(state.getFooterKey(), line);
     }
-    Account.Id accountId = noteUtil.parseIdent(ident, id);
+    Account.Id accountId = legacyChangeNoteRead.parseIdent(ident, id);
     reviewerUpdates.add(ReviewerStatusUpdate.create(ts, ownerId, accountId, state));
     if (!reviewers.containsRow(accountId)) {
       reviewers.put(accountId, state, ts);

java/com/google/gerrit/server/notedb/ChangeRevisionNote.java

@@ -37,19 +37,22 @@ class ChangeRevisionNote extends RevisionNote<Comment> {
   // See org.eclipse.jgit.transport.PushCertificateParser.END_SIGNATURE
   private static final byte[] END_SIGNATURE = "-----END PGP SIGNATURE-----\n".getBytes(UTF_8);
 
-  private final ChangeNoteUtil noteUtil;
+  private final ChangeNoteJson noteJson;
+  private final LegacyChangeNoteRead legacyChangeNoteRead;
   private final Change.Id changeId;
   private final PatchLineComment.Status status;
   private String pushCert;
 
   ChangeRevisionNote(
-      ChangeNoteUtil noteUtil,
+      ChangeNoteJson noteJson,
+      LegacyChangeNoteRead legacyChangeNoteRead,
       Change.Id changeId,
       ObjectReader reader,
       ObjectId noteId,
       PatchLineComment.Status status) {
     super(reader, noteId);
-    this.noteUtil = noteUtil;
+    this.legacyChangeNoteRead = legacyChangeNoteRead;
+    this.noteJson = noteJson;
     this.changeId = changeId;
     this.status = status;
   }
@@ -65,7 +68,7 @@ class ChangeRevisionNote extends RevisionNote<Comment> {
     p.value = offset;
 
     if (isJson(raw, p.value)) {
-      RevisionNoteData data = parseJson(noteUtil, raw, p.value);
+      RevisionNoteData data = parseJson(noteJson, raw, p.value);
       if (status == PatchLineComment.Status.PUBLISHED) {
         pushCert = data.pushCert;
       } else {
@@ -80,7 +83,7 @@ class ChangeRevisionNote extends RevisionNote<Comment> {
     } else {
       pushCert = null;
     }
-    List<Comment> comments = noteUtil.parseNote(raw, p, changeId);
+    List<Comment> comments = legacyChangeNoteRead.parseNote(raw, p, changeId);
     comments.forEach(c -> c.legacyFormat = true);
     return comments;
   }
@@ -89,7 +92,7 @@ class ChangeRevisionNote extends RevisionNote<Comment> {
     return raw[offset] == '{' || raw[offset] == '[';
   }
 
-  private RevisionNoteData parseJson(ChangeNoteUtil noteUtil, byte[] raw, int offset)
+  private RevisionNoteData parseJson(ChangeNoteJson noteUtil, byte[] raw, int offset)
       throws IOException {
     try (InputStream is = new ByteArrayInputStream(raw, offset, raw.length - offset);
         Reader r = new InputStreamReader(is, UTF_8)) {

java/com/google/gerrit/server/notedb/ChangeUpdate.java

@@ -40,7 +40,7 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_SUBMITTED_WI
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_TAG;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_TOPIC;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_WORK_IN_PROGRESS;
-import static com.google.gerrit.server.notedb.ChangeNoteUtil.sanitizeFooter;
+import static com.google.gerrit.server.notedb.NoteDbUtil.sanitizeFooter;
 import static java.util.Comparator.comparing;
 import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 
@@ -527,7 +527,13 @@ public class ChangeUpdate extends AbstractChangeUpdate {
 
     for (Map.Entry<RevId, RevisionNoteBuilder> e : builders.entrySet()) {
       ObjectId data =
-          inserter.insert(OBJ_BLOB, e.getValue().build(noteUtil, noteUtil.getWriteJson()));
+          inserter.insert(
+              OBJ_BLOB,
+              e.getValue()
+                  .build(
+                      noteUtil.getChangeNoteJson(),
+                      noteUtil.getLegacyChangeNoteWrite(),
+                      noteUtil.getChangeNoteJson().getWriteJson()));
       rnm.noteMap.set(ObjectId.fromString(e.getKey().get()), data);
     }
 
@@ -555,7 +561,12 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     // 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(
-        noteUtil, getId(), rw.getObjectReader(), noteMap, PatchLineComment.Status.PUBLISHED);
+        noteUtil.getChangeNoteJson(),
+        noteUtil.getLegacyChangeNoteRead(),
+        getId(),
+        rw.getObjectReader(),
+        noteMap,
+        PatchLineComment.Status.PUBLISHED);
   }
 
   private void checkComments(
@@ -738,7 +749,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     }
 
     if (readOnlyUntil != null) {
-      addFooter(msg, FOOTER_READ_ONLY_UNTIL, ChangeNoteUtil.formatTime(serverIdent, readOnlyUntil));
+      addFooter(msg, FOOTER_READ_ONLY_UNTIL, NoteDbUtil.formatTime(serverIdent, readOnlyUntil));
     }
 
     if (isPrivate != null) {

java/com/google/gerrit/server/notedb/DeleteCommentRewriter.java

@@ -133,9 +133,14 @@ public class DeleteCommentRewriter implements NoteDbRewriter {
    */
   @VisibleForTesting
   public static Map<String, Comment> getPublishedComments(
-      ChangeNoteUtil noteUtil, Change.Id changeId, ObjectReader reader, NoteMap noteMap)
+      ChangeNoteJson changeNoteJson,
+      LegacyChangeNoteRead legacyChangeNoteRead,
+      Change.Id changeId,
+      ObjectReader reader,
+      NoteMap noteMap)
       throws IOException, ConfigInvalidException {
-    return RevisionNoteMap.parse(noteUtil, changeId, reader, noteMap, PUBLISHED)
+    return RevisionNoteMap.parse(
+            changeNoteJson, legacyChangeNoteRead, changeId, reader, noteMap, PUBLISHED)
         .revisionNotes
         .values()
         .stream()
@@ -143,6 +148,16 @@ public class DeleteCommentRewriter implements NoteDbRewriter {
         .collect(toMap(c -> c.key.uuid, Function.identity()));
   }
 
+  public static Map<String, Comment> getPublishedComments(
+      ChangeNoteUtil noteUtil, Change.Id changeId, ObjectReader reader, NoteMap noteMap)
+      throws IOException, ConfigInvalidException {
+    return getPublishedComments(
+        noteUtil.getChangeNoteJson(),
+        noteUtil.getLegacyChangeNoteRead(),
+        changeId,
+        reader,
+        noteMap);
+  }
   /**
    * Gets the comments put in by the current commit. The message of the target comment will be
    * replaced by the new message.
@@ -205,7 +220,12 @@ public class DeleteCommentRewriter implements NoteDbRewriter {
       throws IOException, ConfigInvalidException {
     RevisionNoteMap<ChangeRevisionNote> revNotesMap =
         RevisionNoteMap.parse(
-            noteUtil, changeId, reader, NoteMap.read(reader, parentCommit), PUBLISHED);
+            noteUtil.getChangeNoteJson(),
+            noteUtil.getLegacyChangeNoteRead(),
+            changeId,
+            reader,
+            NoteMap.read(reader, parentCommit),
+            PUBLISHED);
     RevisionNoteBuilder.Cache cache = new RevisionNoteBuilder.Cache(revNotesMap);
 
     for (Comment c : putInComments) {
@@ -219,7 +239,13 @@ public class DeleteCommentRewriter implements NoteDbRewriter {
     Map<RevId, RevisionNoteBuilder> builders = cache.getBuilders();
     for (Map.Entry<RevId, RevisionNoteBuilder> entry : builders.entrySet()) {
       ObjectId objectId = ObjectId.fromString(entry.getKey().get());
-      byte[] data = entry.getValue().build(noteUtil, noteUtil.getWriteJson());
+      byte[] data =
+          entry
+              .getValue()
+              .build(
+                  noteUtil.getChangeNoteJson(),
+                  noteUtil.getLegacyChangeNoteWrite(),
+                  noteUtil.getChangeNoteJson().getWriteJson());
       if (data.length == 0) {
         revNotesMap.noteMap.remove(objectId);
       } else {

java/com/google/gerrit/server/notedb/DraftCommentNotes.java

@@ -155,7 +155,8 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
     ObjectReader reader = handle.walk().getObjectReader();
     revisionNoteMap =
         RevisionNoteMap.parse(
-            args.noteUtil,
+            args.changeNoteJson,
+            args.legacyChangeNoteRead,
             getChangeId(),
             reader,
             NoteMap.read(reader, tipCommit),

java/com/google/gerrit/server/notedb/LegacyChangeNoteRead.java

@@ -0,0 +1,402 @@
+// Copyright (C) 2018 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.gerrit.server.notedb.ChangeNotes.parseException;
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.Comment;
+import com.google.gerrit.reviewdb.client.Comment.Key;
+import com.google.gerrit.reviewdb.client.CommentRange;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.RevId;
+import com.google.gerrit.server.config.GerritServerId;
+import com.google.inject.Inject;
+import java.sql.Timestamp;
+import java.text.ParseException;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Locale;
+import java.util.Set;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.util.GitDateParser;
+import org.eclipse.jgit.util.MutableInteger;
+import org.eclipse.jgit.util.QuotedString;
+import org.eclipse.jgit.util.RawParseUtils;
+
+public class LegacyChangeNoteRead {
+  private final String serverId;
+
+  @Inject
+  public LegacyChangeNoteRead(@GerritServerId String serverId) {
+    this.serverId = serverId;
+  }
+
+  public Account.Id parseIdent(PersonIdent ident, Change.Id changeId)
+      throws ConfigInvalidException {
+    return NoteDbUtil.parseIdent(ident, serverId)
+        .orElseThrow(
+            () ->
+                parseException(
+                    changeId,
+                    "invalid identity, expected <id>@%s: %s",
+                    serverId,
+                    ident.getEmailAddress()));
+  }
+
+  private static boolean match(byte[] note, MutableInteger p, byte[] expected) {
+    int m = RawParseUtils.match(note, p.value, expected);
+    return m == p.value + expected.length;
+  }
+
+  public List<Comment> parseNote(byte[] note, MutableInteger p, Change.Id changeId)
+      throws ConfigInvalidException {
+    if (p.value >= note.length) {
+      return ImmutableList.of();
+    }
+    Set<Key> seen = new HashSet<>();
+    List<Comment> result = new ArrayList<>();
+    int sizeOfNote = note.length;
+    byte[] psb = ChangeNoteUtil.PATCH_SET.getBytes(UTF_8);
+    byte[] bpsb = ChangeNoteUtil.BASE_PATCH_SET.getBytes(UTF_8);
+    byte[] bpn = ChangeNoteUtil.PARENT_NUMBER.getBytes(UTF_8);
+
+    RevId revId = new RevId(parseStringField(note, p, changeId, ChangeNoteUtil.REVISION));
+    String fileName = null;
+    PatchSet.Id psId = null;
+    boolean isForBase = false;
+    Integer parentNumber = null;
+
+    while (p.value < sizeOfNote) {
+      boolean matchPs = match(note, p, psb);
+      boolean matchBase = match(note, p, bpsb);
+      if (matchPs) {
+        fileName = null;
+        psId = parsePsId(note, p, changeId, ChangeNoteUtil.PATCH_SET);
+        isForBase = false;
+      } else if (matchBase) {
+        fileName = null;
+        psId = parsePsId(note, p, changeId, ChangeNoteUtil.BASE_PATCH_SET);
+        isForBase = true;
+        if (match(note, p, bpn)) {
+          parentNumber = parseParentNumber(note, p, changeId);
+        }
+      } else if (psId == null) {
+        throw parseException(
+            changeId,
+            "missing %s or %s header",
+            ChangeNoteUtil.PATCH_SET,
+            ChangeNoteUtil.BASE_PATCH_SET);
+      }
+
+      Comment c = parseComment(note, p, fileName, psId, revId, isForBase, parentNumber);
+      fileName = c.key.filename;
+      if (!seen.add(c.key)) {
+        throw parseException(changeId, "multiple comments for %s in note", c.key);
+      }
+      result.add(c);
+    }
+    return result;
+  }
+
+  private Comment parseComment(
+      byte[] note,
+      MutableInteger curr,
+      String currentFileName,
+      PatchSet.Id psId,
+      RevId revId,
+      boolean isForBase,
+      Integer parentNumber)
+      throws ConfigInvalidException {
+    Change.Id changeId = psId.getParentKey();
+
+    // Check if there is a new file.
+    boolean newFile =
+        (RawParseUtils.match(note, curr.value, ChangeNoteUtil.FILE.getBytes(UTF_8))) != -1;
+    if (newFile) {
+      // If so, parse the new file name.
+      currentFileName = parseFilename(note, curr, changeId);
+    } else if (currentFileName == null) {
+      throw parseException(changeId, "could not parse %s", ChangeNoteUtil.FILE);
+    }
+
+    CommentRange range = parseCommentRange(note, curr);
+    if (range == null) {
+      throw parseException(changeId, "could not parse %s", ChangeNoteUtil.COMMENT_RANGE);
+    }
+
+    Timestamp commentTime = parseTimestamp(note, curr, changeId);
+    Account.Id aId = parseAuthor(note, curr, changeId, ChangeNoteUtil.AUTHOR);
+    boolean hasRealAuthor =
+        (RawParseUtils.match(note, curr.value, ChangeNoteUtil.REAL_AUTHOR.getBytes(UTF_8))) != -1;
+    Account.Id raId = null;
+    if (hasRealAuthor) {
+      raId = parseAuthor(note, curr, changeId, ChangeNoteUtil.REAL_AUTHOR);
+    }
+
+    boolean hasParent =
+        (RawParseUtils.match(note, curr.value, ChangeNoteUtil.PARENT.getBytes(UTF_8))) != -1;
+    String parentUUID = null;
+    boolean unresolved = false;
+    if (hasParent) {
+      parentUUID = parseStringField(note, curr, changeId, ChangeNoteUtil.PARENT);
+    }
+    boolean hasUnresolved =
+        (RawParseUtils.match(note, curr.value, ChangeNoteUtil.UNRESOLVED.getBytes(UTF_8))) != -1;
+    if (hasUnresolved) {
+      unresolved = parseBooleanField(note, curr, changeId, ChangeNoteUtil.UNRESOLVED);
+    }
+
+    String uuid = parseStringField(note, curr, changeId, ChangeNoteUtil.UUID);
+
+    boolean hasTag =
+        (RawParseUtils.match(note, curr.value, ChangeNoteUtil.TAG.getBytes(UTF_8))) != -1;
+    String tag = null;
+    if (hasTag) {
+      tag = parseStringField(note, curr, changeId, ChangeNoteUtil.TAG);
+    }
+
+    int commentLength = parseCommentLength(note, curr, changeId);
+
+    String message = RawParseUtils.decode(UTF_8, note, curr.value, curr.value + commentLength);
+    checkResult(message, "message contents", changeId);
+
+    Comment c =
+        new Comment(
+            new Comment.Key(uuid, currentFileName, psId.get()),
+            aId,
+            commentTime,
+            isForBase ? (short) (parentNumber == null ? 0 : -parentNumber) : (short) 1,
+            message,
+            serverId,
+            unresolved);
+    c.lineNbr = range.getEndLine();
+    c.parentUuid = parentUUID;
+    c.tag = tag;
+    c.setRevId(revId);
+    if (raId != null) {
+      c.setRealAuthor(raId);
+    }
+
+    if (range.getStartCharacter() != -1) {
+      c.setRange(range);
+    }
+
+    curr.value = RawParseUtils.nextLF(note, curr.value + commentLength);
+    curr.value = RawParseUtils.nextLF(note, curr.value);
+    return c;
+  }
+
+  private static String parseStringField(
+      byte[] note, MutableInteger curr, Change.Id changeId, String fieldName)
+      throws ConfigInvalidException {
+    int endOfLine = RawParseUtils.nextLF(note, curr.value);
+    checkHeaderLineFormat(note, curr, fieldName, changeId);
+    int startOfField = RawParseUtils.endOfFooterLineKey(note, curr.value) + 2;
+    curr.value = endOfLine;
+    return RawParseUtils.decode(UTF_8, note, startOfField, endOfLine - 1);
+  }
+
+  /**
+   * @return a comment range. If the comment range line in the note only has one number, we return a
+   *     CommentRange with that one number as the end line and the other fields as -1. If the
+   *     comment range line in the note contains a whole comment range, then we return a
+   *     CommentRange with all fields set. If the line is not correctly formatted, return null.
+   */
+  private static CommentRange parseCommentRange(byte[] note, MutableInteger ptr) {
+    CommentRange range = new CommentRange(-1, -1, -1, -1);
+
+    int last = ptr.value;
+    int startLine = RawParseUtils.parseBase10(note, ptr.value, ptr);
+    if (ptr.value == last) {
+      return null;
+    } else if (note[ptr.value] == '\n') {
+      range.setEndLine(startLine);
+      ptr.value += 1;
+      return range;
+    } else if (note[ptr.value] == ':') {
+      range.setStartLine(startLine);
+      ptr.value += 1;
+    } else {
+      return null;
+    }
+
+    last = ptr.value;
+    int startChar = RawParseUtils.parseBase10(note, ptr.value, ptr);
+    if (ptr.value == last) {
+      return null;
+    } else if (note[ptr.value] == '-') {
+      range.setStartCharacter(startChar);
+      ptr.value += 1;
+    } else {
+      return null;
+    }
+
+    last = ptr.value;
+    int endLine = RawParseUtils.parseBase10(note, ptr.value, ptr);
+    if (ptr.value == last) {
+      return null;
+    } else if (note[ptr.value] == ':') {
+      range.setEndLine(endLine);
+      ptr.value += 1;
+    } else {
+      return null;
+    }
+
+    last = ptr.value;
+    int endChar = RawParseUtils.parseBase10(note, ptr.value, ptr);
+    if (ptr.value == last) {
+      return null;
+    } else if (note[ptr.value] == '\n') {
+      range.setEndCharacter(endChar);
+      ptr.value += 1;
+    } else {
+      return null;
+    }
+    return range;
+  }
+
+  private static PatchSet.Id parsePsId(
+      byte[] note, MutableInteger curr, Change.Id changeId, String fieldName)
+      throws ConfigInvalidException {
+    checkHeaderLineFormat(note, curr, fieldName, changeId);
+    int startOfPsId = RawParseUtils.endOfFooterLineKey(note, curr.value) + 1;
+    MutableInteger i = new MutableInteger();
+    int patchSetId = RawParseUtils.parseBase10(note, startOfPsId, i);
+    int endOfLine = RawParseUtils.nextLF(note, curr.value);
+    if (i.value != endOfLine - 1) {
+      throw parseException(changeId, "could not parse %s", fieldName);
+    }
+    checkResult(patchSetId, "patchset id", changeId);
+    curr.value = endOfLine;
+    return new PatchSet.Id(changeId, patchSetId);
+  }
+
+  private static Integer parseParentNumber(byte[] note, MutableInteger curr, Change.Id changeId)
+      throws ConfigInvalidException {
+    checkHeaderLineFormat(note, curr, ChangeNoteUtil.PARENT_NUMBER, changeId);
+
+    int start = RawParseUtils.endOfFooterLineKey(note, curr.value) + 1;
+    MutableInteger i = new MutableInteger();
+    int parentNumber = RawParseUtils.parseBase10(note, start, i);
+    int endOfLine = RawParseUtils.nextLF(note, curr.value);
+    if (i.value != endOfLine - 1) {
+      throw parseException(changeId, "could not parse %s", ChangeNoteUtil.PARENT_NUMBER);
+    }
+    checkResult(parentNumber, "parent number", changeId);
+    curr.value = endOfLine;
+    return Integer.valueOf(parentNumber);
+  }
+
+  private static String parseFilename(byte[] note, MutableInteger curr, Change.Id changeId)
+      throws ConfigInvalidException {
+    checkHeaderLineFormat(note, curr, ChangeNoteUtil.FILE, changeId);
+    int startOfFileName = RawParseUtils.endOfFooterLineKey(note, curr.value) + 2;
+    int endOfLine = RawParseUtils.nextLF(note, curr.value);
+    curr.value = endOfLine;
+    curr.value = RawParseUtils.nextLF(note, curr.value);
+    return QuotedString.GIT_PATH.dequote(
+        RawParseUtils.decode(UTF_8, note, startOfFileName, endOfLine - 1));
+  }
+
+  private static Timestamp parseTimestamp(byte[] note, MutableInteger curr, Change.Id changeId)
+      throws ConfigInvalidException {
+    int endOfLine = RawParseUtils.nextLF(note, curr.value);
+    Timestamp commentTime;
+    String dateString = RawParseUtils.decode(UTF_8, note, curr.value, endOfLine - 1);
+    try {
+      commentTime = new Timestamp(GitDateParser.parse(dateString, null, Locale.US).getTime());
+    } catch (ParseException e) {
+      throw new ConfigInvalidException("could not parse comment timestamp", e);
+    }
+    curr.value = endOfLine;
+    return checkResult(commentTime, "comment timestamp", changeId);
+  }
+
+  private Account.Id parseAuthor(
+      byte[] note, MutableInteger curr, Change.Id changeId, String fieldName)
+      throws ConfigInvalidException {
+    checkHeaderLineFormat(note, curr, fieldName, changeId);
+    int startOfAccountId = RawParseUtils.endOfFooterLineKey(note, curr.value) + 2;
+    PersonIdent ident = RawParseUtils.parsePersonIdent(note, startOfAccountId);
+    Account.Id aId = parseIdent(ident, changeId);
+    curr.value = RawParseUtils.nextLF(note, curr.value);
+    return checkResult(aId, fieldName, changeId);
+  }
+
+  private static int parseCommentLength(byte[] note, MutableInteger curr, Change.Id changeId)
+      throws ConfigInvalidException {
+    checkHeaderLineFormat(note, curr, ChangeNoteUtil.LENGTH, changeId);
+    int startOfLength = RawParseUtils.endOfFooterLineKey(note, curr.value) + 1;
+    MutableInteger i = new MutableInteger();
+    i.value = startOfLength;
+    int commentLength = RawParseUtils.parseBase10(note, startOfLength, i);
+    if (i.value == startOfLength) {
+      throw parseException(changeId, "could not parse %s", ChangeNoteUtil.LENGTH);
+    }
+    int endOfLine = RawParseUtils.nextLF(note, curr.value);
+    if (i.value != endOfLine - 1) {
+      throw parseException(changeId, "could not parse %s", ChangeNoteUtil.LENGTH);
+    }
+    curr.value = endOfLine;
+    return checkResult(commentLength, "comment length", changeId);
+  }
+
+  private boolean parseBooleanField(
+      byte[] note, MutableInteger curr, Change.Id changeId, String fieldName)
+      throws ConfigInvalidException {
+    String str = parseStringField(note, curr, changeId, fieldName);
+    if ("true".equalsIgnoreCase(str)) {
+      return true;
+    } else if ("false".equalsIgnoreCase(str)) {
+      return false;
+    }
+    throw parseException(changeId, "invalid boolean for %s: %s", fieldName, str);
+  }
+
+  private static <T> T checkResult(T o, String fieldName, Change.Id changeId)
+      throws ConfigInvalidException {
+    if (o == null) {
+      throw parseException(changeId, "could not parse %s", fieldName);
+    }
+    return o;
+  }
+
+  private static int checkResult(int i, String fieldName, Change.Id changeId)
+      throws ConfigInvalidException {
+    if (i <= 0) {
+      throw parseException(changeId, "could not parse %s", fieldName);
+    }
+    return i;
+  }
+
+  private static void checkHeaderLineFormat(
+      byte[] note, MutableInteger curr, String fieldName, Change.Id changeId)
+      throws ConfigInvalidException {
+    boolean correct = RawParseUtils.match(note, curr.value, fieldName.getBytes(UTF_8)) != -1;
+    int p = curr.value + fieldName.length();
+    correct &= (p < note.length && note[p] == ':');
+    p++;
+    correct &= (p < note.length && note[p] == ' ');
+    if (!correct) {
+      throw parseException(changeId, "could not parse %s", fieldName);
+    }
+  }
+}

java/com/google/gerrit/server/notedb/LegacyChangeNoteWrite.java

@@ -0,0 +1,206 @@
+// Copyright (C) 2018 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.checkArgument;
+import static com.google.gerrit.server.CommentsUtil.COMMENT_ORDER;
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ListMultimap;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.Comment;
+import com.google.gerrit.server.GerritPersonIdent;
+import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.AccountState;
+import com.google.gerrit.server.config.GerritServerId;
+import com.google.inject.Inject;
+import java.io.OutputStream;
+import java.io.OutputStreamWriter;
+import java.io.PrintWriter;
+import java.sql.Timestamp;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Date;
+import java.util.List;
+import java.util.Optional;
+import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.util.QuotedString;
+
+public class LegacyChangeNoteWrite {
+
+  private final AccountCache accountCache;
+  private final PersonIdent serverIdent;
+  private final String serverId;
+
+  @Inject
+  public LegacyChangeNoteWrite(
+      AccountCache accountCache,
+      @GerritPersonIdent PersonIdent serverIdent,
+      @GerritServerId String serverId) {
+    this.accountCache = accountCache;
+    this.serverIdent = serverIdent;
+    this.serverId = serverId;
+  }
+
+  public PersonIdent newIdent(Account.Id authorId, Date when, PersonIdent serverIdent) {
+    Optional<Account> author = accountCache.get(authorId).map(AccountState::getAccount);
+    return new PersonIdent(
+        author.map(Account::getName).orElseGet(() -> Account.getName(authorId)),
+        authorId.get() + "@" + serverId,
+        when,
+        serverIdent.getTimeZone());
+  }
+
+  @VisibleForTesting
+  public PersonIdent newIdent(Account author, Date when, PersonIdent serverIdent) {
+    return new PersonIdent(
+        author.getName(), author.getId().get() + "@" + serverId, when, serverIdent.getTimeZone());
+  }
+
+  public String getServerId() {
+    return serverId;
+  }
+
+  private void appendHeaderField(PrintWriter writer, String field, String value) {
+    writer.print(field);
+    writer.print(": ");
+    writer.print(value);
+    writer.print('\n');
+  }
+
+  /**
+   * Build a note that contains the metadata for and the contents of all of the comments in the
+   * given comments.
+   *
+   * @param comments Comments to be written to the output stream, keyed by patch set ID; multiple
+   *     patch sets are allowed since base revisions may be shared across patch sets. All of the
+   *     comments must share the same RevId, and all the comments for a given patch set must have
+   *     the same side.
+   * @param out output stream to write to.
+   */
+  void buildNote(ListMultimap<Integer, Comment> comments, OutputStream out) {
+    if (comments.isEmpty()) {
+      return;
+    }
+
+    List<Integer> psIds = new ArrayList<>(comments.keySet());
+    Collections.sort(psIds);
+
+    OutputStreamWriter streamWriter = new OutputStreamWriter(out, UTF_8);
+    try (PrintWriter writer = new PrintWriter(streamWriter)) {
+      String revId = comments.values().iterator().next().revId;
+      appendHeaderField(writer, ChangeNoteUtil.REVISION, revId);
+
+      for (int psId : psIds) {
+        List<Comment> psComments = COMMENT_ORDER.sortedCopy(comments.get(psId));
+        Comment first = psComments.get(0);
+
+        short side = first.side;
+        appendHeaderField(
+            writer,
+            side <= 0 ? ChangeNoteUtil.BASE_PATCH_SET : ChangeNoteUtil.PATCH_SET,
+            Integer.toString(psId));
+        if (side < 0) {
+          appendHeaderField(writer, ChangeNoteUtil.PARENT_NUMBER, Integer.toString(-side));
+        }
+
+        String currentFilename = null;
+
+        for (Comment c : psComments) {
+          checkArgument(
+              revId.equals(c.revId),
+              "All comments being added must have all the same RevId. The "
+                  + "comment below does not have the same RevId as the others "
+                  + "(%s).\n%s",
+              revId,
+              c);
+          checkArgument(
+              side == c.side,
+              "All comments being added must all have the same side. The "
+                  + "comment below does not have the same side as the others "
+                  + "(%s).\n%s",
+              side,
+              c);
+          String commentFilename = QuotedString.GIT_PATH.quote(c.key.filename);
+
+          if (!commentFilename.equals(currentFilename)) {
+            currentFilename = commentFilename;
+            writer.print("File: ");
+            writer.print(commentFilename);
+            writer.print("\n\n");
+          }
+
+          appendOneComment(writer, c);
+        }
+      }
+    }
+  }
+
+  private void appendOneComment(PrintWriter writer, Comment c) {
+    // The CommentRange field for a comment is allowed to be null. If it is
+    // null, then in the first line, we simply use the line number field for a
+    // comment instead. If it isn't null, we write the comment range itself.
+    Comment.Range range = c.range;
+    if (range != null) {
+      writer.print(range.startLine);
+      writer.print(':');
+      writer.print(range.startChar);
+      writer.print('-');
+      writer.print(range.endLine);
+      writer.print(':');
+      writer.print(range.endChar);
+    } else {
+      writer.print(c.lineNbr);
+    }
+    writer.print("\n");
+
+    writer.print(NoteDbUtil.formatTime(serverIdent, c.writtenOn));
+    writer.print("\n");
+
+    appendIdent(writer, ChangeNoteUtil.AUTHOR, c.author.getId(), c.writtenOn);
+    if (!c.getRealAuthor().equals(c.author)) {
+      appendIdent(writer, ChangeNoteUtil.REAL_AUTHOR, c.getRealAuthor().getId(), c.writtenOn);
+    }
+
+    String parent = c.parentUuid;
+    if (parent != null) {
+      appendHeaderField(writer, ChangeNoteUtil.PARENT, parent);
+    }
+
+    appendHeaderField(writer, ChangeNoteUtil.UNRESOLVED, Boolean.toString(c.unresolved));
+    appendHeaderField(writer, ChangeNoteUtil.UUID, c.key.uuid);
+
+    if (c.tag != null) {
+      appendHeaderField(writer, ChangeNoteUtil.TAG, c.tag);
+    }
+
+    byte[] messageBytes = c.message.getBytes(UTF_8);
+    appendHeaderField(writer, ChangeNoteUtil.LENGTH, Integer.toString(messageBytes.length));
+
+    writer.print(c.message);
+    writer.print("\n\n");
+  }
+
+  private void appendIdent(PrintWriter writer, String header, Account.Id id, Timestamp ts) {
+    PersonIdent ident = newIdent(id, ts, serverIdent);
+    StringBuilder name = new StringBuilder();
+    PersonIdent.appendSanitized(name, ident.getName());
+    name.append(" <");
+    PersonIdent.appendSanitized(name, ident.getEmailAddress());
+    name.append('>');
+    appendHeaderField(writer, header, name.toString());
+  }
+}

java/com/google/gerrit/server/notedb/NoteDbUtil.java

@@ -14,12 +14,21 @@
 
 package com.google.gerrit.server.notedb;
 
+import com.google.common.base.CharMatcher;
 import com.google.common.primitives.Ints;
 import com.google.gerrit.reviewdb.client.Account;
+import java.sql.Timestamp;
 import java.util.Optional;
 import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.util.GitDateFormatter;
+import org.eclipse.jgit.util.GitDateFormatter.Format;
 
 public class NoteDbUtil {
+
+  /**
+   * Returns an AccountId for the given email address. Returns empty if the address isn't on this
+   * server.
+   */
   public static Optional<Account.Id> parseIdent(PersonIdent ident, String serverId) {
     String email = ident.getEmailAddress();
     int at = email.indexOf('@');
@@ -36,4 +45,24 @@ public class NoteDbUtil {
   }
 
   private NoteDbUtil() {}
+
+  public static String formatTime(PersonIdent ident, Timestamp t) {
+    GitDateFormatter dateFormatter = new GitDateFormatter(Format.DEFAULT);
+    // TODO(dborowitz): Use a ThreadLocal or use Joda.
+    PersonIdent newIdent = new PersonIdent(ident, t);
+    return dateFormatter.formatDate(newIdent);
+  }
+
+  private static final CharMatcher INVALID_FOOTER_CHARS = CharMatcher.anyOf("\r\n\0");
+
+  static String sanitizeFooter(String value) {
+    // Remove characters that would confuse JGit's footer parser if they were
+    // included in footer values, for example by splitting the footer block into
+    // multiple paragraphs.
+    //
+    // One painful example: RevCommit#getShorMessage() might return a message
+    // containing "\r\r", which RevCommit#getFooterLines() will treat as an
+    // empty paragraph for the purposes of footer parsing.
+    return INVALID_FOOTER_CHARS.trimAndCollapseFrom(value, ' ');
+  }
 }

java/com/google/gerrit/server/notedb/RevisionNoteBuilder.java

@@ -83,11 +83,17 @@ class RevisionNoteBuilder {
   }
 
   public byte[] build(ChangeNoteUtil noteUtil, boolean writeJson) throws IOException {
+    return build(noteUtil.getChangeNoteJson(), noteUtil.getLegacyChangeNoteWrite(), writeJson);
+  }
+
+  public byte[] build(
+      ChangeNoteJson changeNoteJson, LegacyChangeNoteWrite legacyChangeNoteWrite, boolean writeJson)
+      throws IOException {
     ByteArrayOutputStream out = new ByteArrayOutputStream();
     if (writeJson) {
-      buildNoteJson(noteUtil, out);
+      buildNoteJson(changeNoteJson, out);
     } else {
-      buildNoteLegacy(noteUtil, out);
+      buildNoteLegacy(legacyChangeNoteWrite, out);
     }
     return out.toByteArray();
   }
@@ -122,7 +128,7 @@ class RevisionNoteBuilder {
     return all;
   }
 
-  private void buildNoteJson(ChangeNoteUtil noteUtil, OutputStream out) throws IOException {
+  private void buildNoteJson(ChangeNoteJson noteUtil, OutputStream out) throws IOException {
     ListMultimap<Integer, Comment> comments = buildCommentMap();
     if (comments.isEmpty() && pushCert == null) {
       return;
@@ -137,7 +143,8 @@ class RevisionNoteBuilder {
     }
   }
 
-  private void buildNoteLegacy(ChangeNoteUtil noteUtil, OutputStream out) throws IOException {
+  private void buildNoteLegacy(LegacyChangeNoteWrite noteUtil, OutputStream out)
+      throws IOException {
     if (pushCert != null) {
       byte[] certBytes = pushCert.getBytes(UTF_8);
       out.write(certBytes, 0, trimTrailingNewlines(certBytes));

java/com/google/gerrit/server/notedb/RevisionNoteMap.java

@@ -32,7 +32,8 @@ class RevisionNoteMap<T extends RevisionNote<? extends Comment>> {
   final ImmutableMap<RevId, T> revisionNotes;
 
   static RevisionNoteMap<ChangeRevisionNote> parse(
-      ChangeNoteUtil noteUtil,
+      ChangeNoteJson noteJson,
+      LegacyChangeNoteRead legacyChangeNoteRead,
       Change.Id changeId,
       ObjectReader reader,
       NoteMap noteMap,
@@ -41,7 +42,8 @@ class RevisionNoteMap<T extends RevisionNote<? extends Comment>> {
     Map<RevId, ChangeRevisionNote> result = new HashMap<>();
     for (Note note : noteMap) {
       ChangeRevisionNote rn =
-          new ChangeRevisionNote(noteUtil, changeId, reader, note.getData(), status);
+          new ChangeRevisionNote(
+              noteJson, legacyChangeNoteRead, changeId, reader, note.getData(), status);
       rn.parse();
       result.put(new RevId(note.name()), rn);
     }
@@ -49,12 +51,12 @@ class RevisionNoteMap<T extends RevisionNote<? extends Comment>> {
   }
 
   static RevisionNoteMap<RobotCommentsRevisionNote> parseRobotComments(
-      ChangeNoteUtil noteUtil, ObjectReader reader, NoteMap noteMap)
+      ChangeNoteJson changeNoteJson, ObjectReader reader, NoteMap noteMap)
       throws ConfigInvalidException, IOException {
     Map<RevId, RobotCommentsRevisionNote> result = new HashMap<>();
     for (Note note : noteMap) {
       RobotCommentsRevisionNote rn =
-          new RobotCommentsRevisionNote(noteUtil, reader, note.getData());
+          new RobotCommentsRevisionNote(changeNoteJson, reader, note.getData());
       rn.parse();
       result.put(new RevId(note.name()), rn);
     }

java/com/google/gerrit/server/notedb/RobotCommentNotes.java

@@ -89,7 +89,8 @@ public class RobotCommentNotes extends AbstractChangeNotes<RobotCommentNotes> {
     RevCommit tipCommit = handle.walk().parseCommit(metaId);
     ObjectReader reader = handle.walk().getObjectReader();
     revisionNoteMap =
-        RevisionNoteMap.parseRobotComments(args.noteUtil, reader, NoteMap.read(reader, tipCommit));
+        RevisionNoteMap.parseRobotComments(
+            args.changeNoteJson, reader, NoteMap.read(reader, tipCommit));
     ListMultimap<RevId, RobotComment> cs = MultimapBuilder.hashKeys().arrayListValues().build();
     for (RobotCommentsRevisionNote rn : revisionNoteMap.revisionNotes.values()) {
       for (RobotComment c : rn.getComments()) {

java/com/google/gerrit/server/notedb/RobotCommentUpdate.java

@@ -202,7 +202,8 @@ public class RobotCommentUpdate extends AbstractChangeUpdate {
     }
     // Even though reading from changes might not be enabled, we need to
     // parse any existing revision notes so we can merge them.
-    return RevisionNoteMap.parseRobotComments(noteUtil, rw.getObjectReader(), noteMap);
+    return RevisionNoteMap.parseRobotComments(
+        noteUtil.getChangeNoteJson(), rw.getObjectReader(), noteMap);
   }
 
   @Override

java/com/google/gerrit/server/notedb/RobotCommentsRevisionNote.java

@@ -27,9 +27,9 @@ import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectReader;
 
 public class RobotCommentsRevisionNote extends RevisionNote<RobotComment> {
-  private final ChangeNoteUtil noteUtil;
+  private final ChangeNoteJson noteUtil;
 
-  RobotCommentsRevisionNote(ChangeNoteUtil noteUtil, ObjectReader reader, ObjectId noteId) {
+  RobotCommentsRevisionNote(ChangeNoteJson noteUtil, ObjectReader reader, ObjectId noteId) {
     super(reader, noteId);
     this.noteUtil = noteUtil;
   }

java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java

@@ -546,7 +546,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
     if (id == null) {
       return new PersonIdent(serverIdent, events.getWhen());
     }
-    return changeNoteUtil.newIdent(id, events.getWhen(), serverIdent);
+    return changeNoteUtil.getLegacyChangeNoteWrite().newIdent(id, events.getWhen(), serverIdent);
   }
 
   private List<HashtagsEvent> getHashtagsEvents(Change change, NoteDbUpdateManager manager)
@@ -564,7 +564,10 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
     for (RevCommit commit : rw) {
       Account.Id authorId;
       try {
-        authorId = changeNoteUtil.parseIdent(commit.getAuthorIdent(), change.getId());
+        authorId =
+            changeNoteUtil
+                .getLegacyChangeNoteRead()
+                .parseIdent(commit.getAuthorIdent(), change.getId());
       } catch (ConfigInvalidException e) {
         continue; // Corrupt data, no valid hashtags in this commit.
       }

javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -2676,7 +2676,9 @@ public class ChangeIT extends AbstractDaemonTest {
 
       assertThat(commitPatchSetCreation.getShortMessage()).isEqualTo("Create patch set 2");
       PersonIdent expectedAuthor =
-          changeNoteUtil.newIdent(getAccount(admin.id), c.updated, serverIdent.get());
+          changeNoteUtil
+              .getLegacyChangeNoteWrite()
+              .newIdent(getAccount(admin.id), c.updated, serverIdent.get());
       assertThat(commitPatchSetCreation.getAuthorIdent()).isEqualTo(expectedAuthor);
       assertThat(commitPatchSetCreation.getCommitterIdent())
           .isEqualTo(new PersonIdent(serverIdent.get(), c.updated));
@@ -2684,7 +2686,10 @@ public class ChangeIT extends AbstractDaemonTest {
 
       RevCommit commitChangeCreation = rw.parseCommit(commitPatchSetCreation.getParent(0));
       assertThat(commitChangeCreation.getShortMessage()).isEqualTo("Create change");
-      expectedAuthor = changeNoteUtil.newIdent(getAccount(admin.id), c.created, serverIdent.get());
+      expectedAuthor =
+          changeNoteUtil
+              .getLegacyChangeNoteWrite()
+              .newIdent(getAccount(admin.id), c.created, serverIdent.get());
       assertThat(commitChangeCreation.getAuthorIdent()).isEqualTo(expectedAuthor);
       assertThat(commitChangeCreation.getCommitterIdent())
           .isEqualTo(new PersonIdent(serverIdent.get(), c.created));

javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java

@@ -433,7 +433,9 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
       if (notesMigration.commitChangeWrites()) {
         PersonIdent committer = serverIdent.get();
         PersonIdent author =
-            noteUtil.newIdent(getAccount(admin.getId()), committer.getWhen(), committer);
+            noteUtil
+                .getLegacyChangeNoteWrite()
+                .newIdent(getAccount(admin.getId()), committer.getWhen(), committer);
         tr.branch(RefNames.changeMetaRef(c3.getId()))
             .commit()
             .author(author)

javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java

@@ -273,7 +273,9 @@ public class CreateChangeIT extends AbstractDaemonTest {
       assertThat(commit.getShortMessage()).isEqualTo("Create change");
 
       PersonIdent expectedAuthor =
-          changeNoteUtil.newIdent(getAccount(admin.id), c.created, serverIdent.get());
+          changeNoteUtil
+              .getLegacyChangeNoteWrite()
+              .newIdent(getAccount(admin.id), c.created, serverIdent.get());
       assertThat(commit.getAuthorIdent()).isEqualTo(expectedAuthor);
 
       assertThat(commit.getCommitterIdent())

javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java

@@ -950,7 +950,7 @@ public class CommentsIT extends AbstractDaemonTest {
   @Test
   public void jsonCommentHasLegacyFormatFalse() throws Exception {
     assume().that(notesMigration.readChanges()).isTrue();
-    assertThat(noteUtil.getWriteJson()).isTrue();
+    assertThat(noteUtil.getChangeNoteJson().getWriteJson()).isTrue();
 
     PushOneCommit.Result result = createChange();
     Change.Id changeId = result.getChange().getId();

javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java

@@ -902,7 +902,9 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
     }
     PersonIdent committer = serverIdent.get();
     PersonIdent author =
-        noteUtil.newIdent(getAccount(admin.getId()), committer.getWhen(), committer);
+        noteUtil
+            .getLegacyChangeNoteWrite()
+            .newIdent(getAccount(admin.getId()), committer.getWhen(), committer);
     serverSideTestRepo
         .branch(RefNames.changeMetaRef(id))
         .commit()

javatests/com/google/gerrit/acceptance/server/change/LegacyCommentsIT.java

@@ -53,7 +53,7 @@ public class LegacyCommentsIT extends AbstractDaemonTest {
   @Test
   public void legacyCommentHasLegacyFormatTrue() throws Exception {
     assume().that(notesMigration.readChanges()).isTrue();
-    assertThat(noteUtil.getWriteJson()).isFalse();
+    assertThat(noteUtil.getChangeNoteJson().getWriteJson()).isFalse();
 
     PushOneCommit.Result result = createChange();
     Change.Id changeId = result.getChange().getId();

javatests/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java

@@ -17,8 +17,8 @@ package com.google.gerrit.acceptance.server.notedb;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth8.assertThat;
 import static com.google.common.truth.TruthJUnit.assume;
-import static com.google.gerrit.server.notedb.ChangeNoteUtil.formatTime;
 import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB;
+import static com.google.gerrit.server.notedb.NoteDbUtil.formatTime;
 import static java.util.concurrent.TimeUnit.DAYS;
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import static java.util.concurrent.TimeUnit.SECONDS;

javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java

@@ -498,7 +498,11 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
   private RevCommit writeCommit(String body) throws Exception {
     ChangeNoteUtil noteUtil = injector.getInstance(ChangeNoteUtil.class);
     return writeCommit(
-        body, noteUtil.newIdent(changeOwner.getAccount(), TimeUtil.nowTs(), serverIdent), false);
+        body,
+        noteUtil
+            .getLegacyChangeNoteWrite()
+            .newIdent(changeOwner.getAccount(), TimeUtil.nowTs(), serverIdent),
+        false);
   }
 
   private RevCommit writeCommit(String body, PersonIdent author) throws Exception {
@@ -509,7 +513,9 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
     ChangeNoteUtil noteUtil = injector.getInstance(ChangeNoteUtil.class);
     return writeCommit(
         body,
-        noteUtil.newIdent(changeOwner.getAccount(), TimeUtil.nowTs(), serverIdent),
+        noteUtil
+            .getLegacyChangeNoteWrite()
+            .newIdent(changeOwner.getAccount(), TimeUtil.nowTs(), serverIdent),
         initWorkInProgress);
   }
 
@@ -555,7 +561,9 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
 
   private ChangeNotesParser newParser(ObjectId tip) throws Exception {
     walk.reset();
-    ChangeNoteUtil noteUtil = injector.getInstance(ChangeNoteUtil.class);
+    ChangeNoteJson changeNoteJson = injector.getInstance(ChangeNoteJson.class);
-    return new ChangeNotesParser(newChange().getId(), tip, walk, noteUtil, args.metrics);
+    LegacyChangeNoteRead reader = injector.getInstance(LegacyChangeNoteRead.class);
+    return new ChangeNotesParser(
+        newChange().getId(), tip, walk, changeNoteJson, reader, args.metrics);
   }
 }

javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java

@@ -77,6 +77,8 @@ import org.junit.Test;
 public class ChangeNotesTest extends AbstractChangeNotesTest {
   @Inject private DraftCommentNotes.Factory draftNotesFactory;
 
+  @Inject private ChangeNoteJson changeNoteJson;
+  @Inject private LegacyChangeNoteRead legacyChangeNoteRead;
   @Inject private ChangeNoteUtil noteUtil;
 
   @Inject private @GerritServerId String serverId;
@@ -1195,7 +1197,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
                   + "File: a.txt\n"
                   + "\n"
                   + "1:2-3:4\n"
-                  + ChangeNoteUtil.formatTime(serverIdent, ts)
+                  + NoteDbUtil.formatTime(serverIdent, ts)
                   + "\n"
                   + "Author: Change Owner <1@gerrit>\n"
                   + "Unresolved: false\n"
@@ -1288,7 +1290,13 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
 
     try (ChangeNotesRevWalk rw = ChangeNotesCommit.newRevWalk(repo)) {
       ChangeNotesParser notesWithComments =
-          new ChangeNotesParser(c.getId(), commitWithComments.copy(), rw, noteUtil, args.metrics);
+          new ChangeNotesParser(
+              c.getId(),
+              commitWithComments.copy(),
+              rw,
+              changeNoteJson,
+              legacyChangeNoteRead,
+              args.metrics);
       ChangeNotesState state = notesWithComments.parseAll();
       assertThat(state.approvals()).isEmpty();
       assertThat(state.publishedComments()).hasSize(1);
@@ -1296,7 +1304,14 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
 
     try (ChangeNotesRevWalk rw = ChangeNotesCommit.newRevWalk(repo)) {
       ChangeNotesParser notesWithApprovals =
-          new ChangeNotesParser(c.getId(), commitWithApprovals.copy(), rw, noteUtil, args.metrics);
+          new ChangeNotesParser(
+              c.getId(),
+              commitWithApprovals.copy(),
+              rw,
+              changeNoteJson,
+              legacyChangeNoteRead,
+              args.metrics);
+
       ChangeNotesState state = notesWithApprovals.parseAll();
       assertThat(state.approvals()).hasSize(1);
       assertThat(state.publishedComments()).hasSize(1);
@@ -1666,7 +1681,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
                     + "File: file1\n"
                     + "\n"
                     + "1:1-2:1\n"
-                    + ChangeNoteUtil.formatTime(serverIdent, time1)
+                    + NoteDbUtil.formatTime(serverIdent, time1)
                     + "\n"
                     + "Author: Other Account <2@gerrit>\n"
                     + "Unresolved: false\n"
@@ -1675,7 +1690,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
                     + "comment 1\n"
                     + "\n"
                     + "2:1-3:1\n"
-                    + ChangeNoteUtil.formatTime(serverIdent, time2)
+                    + NoteDbUtil.formatTime(serverIdent, time2)
                     + "\n"
                     + "Author: Other Account <2@gerrit>\n"
                     + "Unresolved: false\n"
@@ -1686,7 +1701,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
                     + "File: file2\n"
                     + "\n"
                     + "3:0-4:1\n"
-                    + ChangeNoteUtil.formatTime(serverIdent, time3)
+                    + NoteDbUtil.formatTime(serverIdent, time3)
                     + "\n"
                     + "Author: Other Account <2@gerrit>\n"
                     + "Unresolved: false\n"
@@ -1766,7 +1781,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
                     + "File: file1\n"
                     + "\n"
                     + "1:1-2:1\n"
-                    + ChangeNoteUtil.formatTime(serverIdent, time1)
+                    + NoteDbUtil.formatTime(serverIdent, time1)
                     + "\n"
                     + "Author: Other Account <2@gerrit>\n"
                     + "Unresolved: false\n"
@@ -1775,7 +1790,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
                     + "comment 1\n"
                     + "\n"
                     + "2:1-3:1\n"
-                    + ChangeNoteUtil.formatTime(serverIdent, time2)
+                    + NoteDbUtil.formatTime(serverIdent, time2)
                     + "\n"
                     + "Author: Other Account <2@gerrit>\n"
                     + "Unresolved: false\n"
@@ -1854,7 +1869,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
                     + "File: file1\n"
                     + "\n"
                     + "1:1-2:1\n"
-                    + ChangeNoteUtil.formatTime(serverIdent, time1)
+                    + NoteDbUtil.formatTime(serverIdent, time1)
                     + "\n"
                     + "Author: Other Account <2@gerrit>\n"
                     + "Unresolved: false\n"
@@ -1863,7 +1878,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
                     + "comment 1\n"
                     + "\n"
                     + "1:1-2:1\n"
-                    + ChangeNoteUtil.formatTime(serverIdent, time2)
+                    + NoteDbUtil.formatTime(serverIdent, time2)
                     + "\n"
                     + "Author: Other Account <2@gerrit>\n"
                     + "Parent: uuid1\n"
@@ -1951,7 +1966,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
 
       byte[] bytes = walk.getObjectReader().open(note.getData(), Constants.OBJ_BLOB).getBytes();
       String noteString = new String(bytes, UTF_8);
-      String timeStr = ChangeNoteUtil.formatTime(serverIdent, time);
+      String timeStr = NoteDbUtil.formatTime(serverIdent, time);
 
       if (!testJson()) {
         assertThat(noteString)
@@ -2048,7 +2063,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
                     + "File: file\n"
                     + "\n"
                     + "1:1-2:1\n"
-                    + ChangeNoteUtil.formatTime(serverIdent, time)
+                    + NoteDbUtil.formatTime(serverIdent, time)
                     + "\n"
                     + "Author: Other Account <2@gerrit>\n"
                     + "Real-author: Change Owner <1@gerrit>\n"
@@ -2103,7 +2118,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
 
       byte[] bytes = walk.getObjectReader().open(note.getData(), Constants.OBJ_BLOB).getBytes();
       String noteString = new String(bytes, UTF_8);
-      String timeStr = ChangeNoteUtil.formatTime(serverIdent, time);
+      String timeStr = NoteDbUtil.formatTime(serverIdent, time);
 
       if (!testJson()) {
         assertThat(noteString)
@@ -3511,7 +3526,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
   }
 
   private boolean testJson() {
-    return noteUtil.getWriteJson();
+    return noteUtil.getChangeNoteJson().getWriteJson();
   }
 
   private String readNote(ChangeNotes notes, ObjectId noteId) throws Exception {

javatests/com/google/gerrit/server/notedb/CommentTimestampAdapterTest.java

@@ -68,7 +68,7 @@ public class CommentTimestampAdapterTest {
 
     // Match ChangeNoteUtil#gson as of 4e1f02db913d91f2988f559048e513e6093a1bce
     legacyGson = new GsonBuilder().setPrettyPrinting().create();
-    gson = ChangeNoteUtil.newGson();
+    gson = ChangeNoteJson.newGson();
   }
 
   @After