Notedb: Unify subject and patch set handling

When inserting patch sets, pass in a RevWalk along with the commit
SHA-1 so we can ensure the body is parsed. Set the subject implicitly
when we add a new patch set; always do this, even if the subject
matches the previous subject.

When parsing patch sets, we have to still parse the subject separately
from the patch set, since there is no subject field on PatchSet. But
we can use a single setter on Change to set the current patch set ID,
subject, and original subject together as a batch, rather than going
through PatchSetInfo.

Change-Id: If37c4990e2e5888d5e87bd5c34a821e59186ab6d

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java

@@ -588,7 +588,6 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
 
     ChangeUpdate u = changeUpdateFactory.create(
         changeControlFactory.controlFor(c, userFactory.create(adminId)));
-    u.setSubject(c.getSubject());
     u.setBranch(c.getDest().get());
     u.commit();
 

gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java

@@ -563,18 +563,10 @@ public final class Change {
     return subject;
   }
 
-  public void setSubject(String subject) {
-    this.subject = subject;
-  }
-
   public String getOriginalSubject() {
     return originalSubject != null ? originalSubject : subject;
   }
 
-  public void setOriginalSubject(String originalSubject) {
-    this.originalSubject = originalSubject;
-  }
-
   /** Get the id of the most current {@link PatchSet} in this change. */
   public PatchSet.Id currentPatchSetId() {
     if (currentPatchSetId > 0) {
@@ -600,6 +592,23 @@ public final class Change {
     }
   }
 
+  public void setCurrentPatchSet(PatchSet.Id psId, String subject,
+      String originalSubject) {
+    if (!psId.getParentKey().equals(changeId)) {
+      throw new IllegalArgumentException(
+          "patch set ID " + psId + " is not for change " + changeId);
+    }
+    currentPatchSetId = psId.get();
+    this.subject = subject;
+    this.originalSubject = originalSubject;
+  }
+
+  public void clearCurrentPatchSet() {
+    currentPatchSetId = 0;
+    subject = null;
+    originalSubject = null;
+  }
+
   public String getSubmissionId() {
     return submissionId;
   }

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

@@ -29,8 +29,9 @@ import com.google.inject.Inject;
 import com.google.inject.Singleton;
 
 import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
 
+import java.io.IOException;
 import java.sql.Timestamp;
 import java.util.Collections;
 
@@ -62,9 +63,10 @@ public class PatchSetUtil {
         db.patchSets().byChange(notes.getChangeId()));
   }
 
-  public PatchSet insert(ReviewDb db, ChangeUpdate update, PatchSet.Id psId,
+  public PatchSet insert(ReviewDb db, RevWalk rw, ChangeUpdate update,
-      RevCommit commit, boolean draft, Iterable<String> groups,
+      PatchSet.Id psId, ObjectId commit, boolean draft,
-      String pushCertificate) throws OrmException {
+      Iterable<String> groups, String pushCertificate)
+      throws OrmException, IOException {
     Change.Id changeId = update.getChange().getId();
     checkArgument(psId.getParentKey().equals(changeId),
         "cannot insert patch set %s on change %s", psId, changeId);
@@ -85,10 +87,7 @@ public class PatchSetUtil {
     ps.setPushCertificate(pushCertificate);
     db.patchSets().insert(Collections.singleton(ps));
 
-    update.setCommit(ObjectId.fromString(ps.getRevision().get()));
+    update.setCommit(rw, commit);
-    if (!update.getChange().getSubject().equals(commit.getShortMessage())) {
-      update.setSubject(commit.getShortMessage());
-    }
 
     return ps;
   }

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

@@ -297,7 +297,6 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
 
     ChangeUpdate update = ctx.getUpdate(psId);
     update.setSubjectForCommit("Create change");
-    update.setSubject(change.getSubject());
     update.setBranch(change.getDest().get());
     update.setTopic(change.getTopic());
 
@@ -306,8 +305,8 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
     if (newGroups == null) {
       newGroups = GroupCollector.getDefaultGroups(commit);
     }
-    patchSet = psUtil.insert(
+    patchSet = psUtil.insert(ctx.getDb(), ctx.getRevWalk(), update, psId,
-        ctx.getDb(), update, psId, commit, draft, newGroups, null);
+        commit, draft, newGroups, null);
     ctx.saveChange();
 
     /* TODO: fixStatus is used here because the tests

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

@@ -222,9 +222,8 @@ public class PatchSetInserter extends BatchUpdate.Op {
       PatchSet prevPs = psUtil.current(ctx.getDb(), ctx.getNotes());
       newGroups = prevPs != null ? prevPs.getGroups() : null;
     }
-    ctx.getRevWalk().parseBody(commit);
+    patchSet = psUtil.insert(ctx.getDb(), ctx.getRevWalk(), ctx.getUpdate(psId),
-    patchSet = psUtil.insert(ctx.getDb(), ctx.getUpdate(psId), psId, commit,
+        psId, commit, draft, newGroups, null);
-        draft, newGroups, null);
 
     if (sendMail) {
       oldReviewers = approvalsUtil.getReviewers(db, ctl.getNotes());

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

@@ -2273,7 +2273,7 @@ public class ReceiveCommits {
 
         boolean draft = magicBranch != null && magicBranch.draft;
         newPatchSet = psUtil.insert(
-            db, update, psId, newCommit, draft, newGroups,
+            db, state.rw, update, psId, newCommit, draft, newGroups,
             rp.getPushCertificate() != null
               ? rp.getPushCertificate().toTextWithSignature()
               : null);

gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java

@@ -144,13 +144,13 @@ public class CherryPick extends SubmitStrategy {
 
     @Override
     public PatchSet updateChangeImpl(ChangeContext ctx) throws OrmException,
-         NoSuchChangeException {
+         NoSuchChangeException, IOException {
       checkState(newCommit != null,
           "no new commit produced by CherryPick of %s, expected to fail fast",
           toMerge.change().getId());
       PatchSet prevPs = args.psUtil.current(ctx.getDb(), ctx.getNotes());
-      PatchSet newPs = args.psUtil.insert(ctx.getDb(), ctx.getUpdate(psId),
+      PatchSet newPs = args.psUtil.insert(ctx.getDb(), ctx.getRevWalk(),
-          psId, newCommit, false,
+          ctx.getUpdate(psId), psId, newCommit, false,
           prevPs != null ? prevPs.getGroups() : null, null);
       ctx.getChange().setCurrentPatchSet(patchSetInfo);
       ctx.saveChange();

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

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.notedb;
 
+import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.GERRIT_PLACEHOLDER_HOST;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -21,8 +22,10 @@ import com.google.common.base.Function;
 import com.google.common.base.Strings;
 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.ImmutableSetMultimap;
+import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Ordering;
 import com.google.common.primitives.Ints;
@@ -36,6 +39,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RevId;
+import com.google.gerrit.reviewdb.server.ReviewDbUtil;
 import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.config.AllUsersNameProvider;
 import com.google.gerrit.server.git.GitRepositoryManager;
@@ -117,6 +121,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
   }
 
   private final Change change;
+  private ImmutableSortedMap<PatchSet.Id, PatchSet> patchSets;
   private ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals;
   private ImmutableSetMultimap<ReviewerStateInternal, Account.Id> reviewers;
   private ImmutableList<Account.Id> allPastReviewers;
@@ -126,7 +131,6 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
   private ImmutableListMultimap<RevId, PatchLineComment> comments;
   private ImmutableSet<String> hashtags;
   NoteMap noteMap;
-  private PatchSet currentPatchSet;
 
   private final AllUsersName allUsers;
   private DraftCommentNotes draftCommentNotes;
@@ -143,6 +147,10 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     return change;
   }
 
+  public ImmutableMap<PatchSet.Id, PatchSet> getPatchSets() {
+    return patchSets;
+  }
+
   public ImmutableListMultimap<PatchSet.Id, PatchSetApproval> getApprovals() {
     return approvals;
   }
@@ -248,7 +256,9 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
   }
 
   public PatchSet getCurrentPatchSet() {
-    return currentPatchSet;
+    PatchSet.Id psId = change.currentPatchSetId();
+    return checkNotNull(patchSets.get(psId),
+        "missing current patch set %s", psId.get());
   }
 
   @Override
@@ -271,15 +281,23 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
       allChangeMessages = parser.buildAllMessages();
       comments = ImmutableListMultimap.copyOf(parser.comments);
       noteMap = parser.commentNoteMap;
-      change.setOriginalSubject(parser.originalSubject);
-      change.setSubject(parser.subject);
       change.setDest(new Branch.NameKey(getProjectName(), parser.branch));
       change.setTopic(Strings.emptyToNull(parser.topic));
       change.setCreatedOn(parser.createdOn);
       change.setLastUpdatedOn(parser.lastUpdatedOn);
       change.setOwner(parser.ownerId);
       change.setSubmissionId(parser.submissionId);
-      currentPatchSet = parser.getCurrentPatchSet();
+      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);

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

@@ -55,6 +55,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.client.RevId;
+import com.google.gerrit.reviewdb.server.ReviewDbUtil;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.util.LabelVote;
 
@@ -86,6 +87,7 @@ class ChangeNotesParser implements AutoCloseable {
   final List<Account.Id> allPastReviewers;
   final List<SubmitRecord> submitRecords;
   final Multimap<RevId, PatchLineComment> comments;
+  final Map<PatchSet.Id, PatchSet> patchSets;
   NoteMap commentNoteMap;
   String branch;
   Change.Status status;
@@ -107,7 +109,6 @@ class ChangeNotesParser implements AutoCloseable {
       Table<Account.Id, String, Optional<PatchSetApproval>>> approvals;
   private final List<ChangeMessage> allChangeMessages;
   private final Multimap<PatchSet.Id, ChangeMessage> changeMessagesByPatchSet;
-  private final Map<PatchSet.Id, PatchSet> patchSets;
 
   ChangeNotesParser(Change change, ObjectId tip, RevWalk walk,
       GitRepositoryManager repoManager) throws RepositoryNotFoundException,
@@ -123,11 +124,7 @@ class ChangeNotesParser implements AutoCloseable {
     allChangeMessages = Lists.newArrayList();
     changeMessagesByPatchSet = LinkedListMultimap.create();
     comments = ArrayListMultimap.create();
-    patchSets = Maps.newHashMap();
+    patchSets = Maps.newTreeMap(ReviewDbUtil.intKeyOrdering());
-  }
-
-  public PatchSet getCurrentPatchSet() {
-    return patchSets.get(currentPatchSetId);
   }
 
   @Override
@@ -197,14 +194,20 @@ class ChangeNotesParser implements AutoCloseable {
 
     Account.Id accountId = parseIdent(commit);
     ownerId = accountId;
-    if (subject == null) {
+
-      subject = parseSubject(commit);
+    String currSubject = parseSubject(commit);
+    if (currSubject != null) {
+      if (subject == null) {
+        subject = currSubject;
+      }
+      originalSubject = currSubject;
     }
-    originalSubject = parseSubject(commit);
+
     parseChangeMessage(psId, accountId, commit, ts);
     if (topic == null) {
       topic = parseTopic(commit);
     }
+
     parseHashtags(commit);
     if (submissionId == null) {
       submissionId = parseSubmissionId(commit);

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

@@ -63,6 +63,7 @@ import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.notes.NoteMap;
 import org.eclipse.jgit.revwalk.FooterKey;
 import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
 
 import java.io.IOException;
 import java.util.ArrayList;
@@ -227,10 +228,15 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     this.commitSubject = commitSubject;
   }
 
-  public void setSubject(String subject) {
+  void setSubject(String subject) {
     this.subject = subject;
   }
 
+  @VisibleForTesting
+  ObjectId getCommit() {
+    return commit;
+  }
+
   public void setChangeMessage(String changeMessage) {
     this.changeMessage = changeMessage;
   }
@@ -359,9 +365,11 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     this.topic = Strings.nullToEmpty(topic);
   }
 
-  public void setCommit(ObjectId commit) {
+  public void setCommit(RevWalk rw, ObjectId id) throws IOException {
-    checkArgument(commit != null);
+    RevCommit commit = rw.parseCommit(id);
+    rw.parseBody(commit);
     this.commit = commit;
+    subject = commit.getShortMessage();
   }
 
   public void setHashtags(Set<String> hashtags) {
@@ -554,7 +562,6 @@ public class ChangeUpdate extends AbstractChangeUpdate {
         && reviewers.isEmpty()
         && branch == null
         && status == null
-        && subject == null
         && submissionId == null
         && submitRecords == null
         && hashtags == null

gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java

@@ -322,7 +322,6 @@ public class CommentsTest extends GerritServerTests {
   private Change newChange() throws Exception {
     Change c = TestChanges.newChange(project, changeOwner.getAccountId());
     ChangeUpdate u = newUpdate(c, changeOwner);
-    u.setSubject(c.getSubject());
     u.setBranch(c.getDest().get());
     u.commit();
     return c;

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

@@ -61,15 +61,15 @@ import com.google.inject.Inject;
 import com.google.inject.Injector;
 import com.google.inject.util.Providers;
 
-import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevWalk;
 import org.junit.After;
 import org.junit.Before;
 
-import java.io.IOException;
 import java.sql.Timestamp;
 import java.util.TimeZone;
 
@@ -87,6 +87,8 @@ public class AbstractChangeNotesTest extends GerritBaseTests {
   protected InMemoryRepositoryManager repoManager;
   protected PersonIdent serverIdent;
   protected Project.NameKey project;
+  protected RevWalk rw;
+  protected TestRepository<InMemoryRepository> tr;
 
   @Inject protected IdentifiedUser.GenericFactory userFactory;
 
@@ -105,6 +107,8 @@ public class AbstractChangeNotesTest extends GerritBaseTests {
     project = new Project.NameKey("test-project");
     repoManager = new InMemoryRepositoryManager();
     repo = repoManager.createRepository(project);
+    tr = new TestRepository<>(repo);
+    rw = tr.getRevWalk();
     accountCache = new FakeAccountCache();
     Account co = new Account(new Account.Id(1), TimeUtil.nowTs());
     co.setFullName("Change Owner");
@@ -162,18 +166,16 @@ public class AbstractChangeNotesTest extends GerritBaseTests {
     System.setProperty("user.timezone", systemTimeZone);
   }
 
-  protected Change newChange()
+  protected Change newChange() throws Exception {
-      throws IOException, OrmException, ConfigInvalidException {
     Change c = TestChanges.newChange(project, changeOwner.getAccountId());
     ChangeUpdate u = newUpdate(c, changeOwner);
-    u.setSubject(c.getSubject());
     u.setBranch(c.getDest().get());
     u.commit();
     return c;
   }
 
   protected ChangeUpdate newUpdate(Change c, IdentifiedUser user)
-      throws OrmException, IOException, ConfigInvalidException {
+      throws Exception {
     ChangeUpdate update = TestChanges.newUpdate(
         injector, repoManager, MIGRATION, c, allUsers, user);
     try (Repository repo = repoManager.openMetadataRepository(c.getProject())) {

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

@@ -512,32 +512,6 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         new Branch.NameKey(project, otherBranch));
   }
 
-  @Test
-  public void subjectChangeNotes() throws Exception {
-    Change c = newChange();
-
-    ChangeNotes notes = newNotes(c);
-    assertThat(notes.getChange().getSubject()).isEqualTo(c.getSubject());
-    assertThat(notes.getChange().getOriginalSubject()).isEqualTo(c.getSubject());
-
-    // An unrelated update doesn't affect the subject
-    ChangeUpdate update = newUpdate(c, changeOwner);
-    update.setTopic("topic"); // Change something to get a new commit.
-    update.commit();
-    notes = newNotes(c);
-    assertThat(notes.getChange().getSubject()).isEqualTo(c.getSubject());
-    assertThat(notes.getChange().getOriginalSubject()).isEqualTo(c.getSubject());
-
-    // An update of the subject doesn't affect the original subject
-    update = newUpdate(c, changeOwner);
-    String newSubject = "other subject";
-    update.setSubject(newSubject);
-    update.commit();
-    notes = newNotes(c);
-    assertThat(notes.getChange().getSubject()).isEqualTo(newSubject);
-    assertThat(notes.getChange().getOriginalSubject()).isEqualTo(c.getSubject());
-  }
-
   @Test
   public void ownerChangeNotes() throws Exception {
     Change c = newChange();
@@ -590,22 +564,13 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     Change c = newChange();
 
     ChangeNotes notes = newNotes(c);
-    assertThat(notes.getCurrentPatchSet()).isNull();
+    PatchSet ps = notes.getCurrentPatchSet();
-
+    assertThat(ps).isNotNull();
-    // ps1
-    ObjectId commit =
-        ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
-    ChangeUpdate update = newUpdate(c, changeOwner);
-    update.setCommit(commit);
-    update.commit();
-    notes = newNotes(c);
-    assertThat(notes.getCurrentPatchSet().getRevision().get())
-        .isEqualTo(commit.name());
 
     // new revId for the same patch set, ps1
-    commit =
+    ChangeUpdate update = newUpdate(c, changeOwner);
-        ObjectId.fromString("badc0feebadc0feebadc0feebadc0feebadc0fee");
+    RevCommit commit = tr.commit().message("PS1 again").create();
-    update.setCommit(commit);
+    update.setCommit(rw, commit);
     update.commit();
     exception.expect(OrmException.class);
     exception.expectMessage("Multiple revisions parsed for patch set");
@@ -616,37 +581,33 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
   public void patchSetChangeNotes() throws Exception {
     Change c = newChange();
 
+    // ps1 created by newChange()
     ChangeNotes notes = newNotes(c);
-    assertThat(notes.getCurrentPatchSet()).isNull();
+    PatchSet ps1 = notes.getCurrentPatchSet();
-
+    assertThat(notes.getChange().currentPatchSetId()).isEqualTo(ps1.getId());
-    // ps1
+    assertThat(notes.getChange().getSubject()).isEqualTo("Change subject");
-    ObjectId commit =
+    assertThat(notes.getChange().getOriginalSubject())
-        ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
+        .isEqualTo("Change subject");
-    ChangeUpdate update = newUpdate(c, changeOwner);
+    assertThat(ps1.getId()).isEqualTo(new PatchSet.Id(c.getId(), 1));
-    update.setCommit(commit);
+    assertThat(ps1.getUploader()).isEqualTo(changeOwner.getAccountId());
-    update.commit();
-    notes = newNotes(c);
-    PatchSet ps = notes.getCurrentPatchSet();
-    assertThat(ps.getId()).isEqualTo(new PatchSet.Id(c.getId(), 1));
-    assertThat(notes.getChange().currentPatchSetId()).isEqualTo(ps.getId());
-    assertThat(ps.getRevision().get()).isEqualTo(commit.name());
-    assertThat(ps.getUploader()).isEqualTo(changeOwner.getAccountId());
-    assertThat(ps.getCreatedOn()).isEqualTo(update.getWhen());
 
     // ps2 by other user
     incrementPatchSet(c);
-    commit =
+    RevCommit commit = tr.commit().message("PS2").create();
-        ObjectId.fromString("badc0feebadc0feebadc0feebadc0feebadc0fee");
+    ChangeUpdate update = newUpdate(c, otherUser);
-    update = newUpdate(c, otherUser);
+    update.setCommit(rw, commit);
-    update.setCommit(commit);
     update.commit();
     notes = newNotes(c);
-    ps = notes.getCurrentPatchSet();
+    PatchSet ps2 = notes.getCurrentPatchSet();
-    assertThat(ps.getId()).isEqualTo(new PatchSet.Id(c.getId(), 2));
+    assertThat(ps2.getId()).isEqualTo(new PatchSet.Id(c.getId(), 2));
-    assertThat(notes.getChange().currentPatchSetId()).isEqualTo(ps.getId());
+    assertThat(notes.getChange().getSubject()).isEqualTo("PS2");
-    assertThat(ps.getRevision().get()).isEqualTo(commit.name());
+    assertThat(notes.getChange().getOriginalSubject())
-    assertThat(ps.getUploader()).isEqualTo(otherUser.getAccountId());
+        .isEqualTo("Change subject");
-    assertThat(ps.getCreatedOn()).isEqualTo(update.getWhen());
+    assertThat(notes.getChange().currentPatchSetId()).isEqualTo(ps2.getId());
+    assertThat(ps2.getRevision().get()).isNotEqualTo(ps1.getRevision());
+    assertThat(ps2.getRevision().get()).isEqualTo(commit.name());
+    assertThat(ps2.getUploader()).isEqualTo(otherUser.getAccountId());
+    assertThat(ps2.getCreatedOn()).isEqualTo(update.getWhen());
   }
 
   @Test

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

@@ -49,6 +49,9 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
     assertBodyEquals("Update patch set 1\n"
         + "\n"
         + "Patch-set: 1\n"
+        + "Subject: Change subject\n"
+        + "Branch: refs/heads/master\n"
+        + "Commit: " + update.getCommit().name() + "\n"
         + "Reviewer: Change Owner <1@gerrit>\n"
         + "CC: Other Account <2@gerrit>\n"
         + "Label: Code-Review=-1\n"
@@ -84,7 +87,10 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
         + "Just a little code change.\n"
         + "How about a new line\n"
         + "\n"
-        + "Patch-set: 1\n",
+        + "Patch-set: 1\n"
+        + "Subject: Change subject\n"
+        + "Branch: refs/heads/master\n"
+        + "Commit: " + update.getCommit().name() + "\n",
         update.getRevision());
   }
 
@@ -93,8 +99,8 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
     Change c = TestChanges.newChange(project, changeOwner.getAccountId(), 1);
     ChangeUpdate update = newUpdate(c, changeOwner);
     update.setChangeMessage("Foo");
-    update.setCommit(
+    RevCommit commit = tr.commit().message("Subject").create();
-        ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
+    update.setCommit(rw, commit);
     update.commit();
     assertThat(update.getRefName()).isEqualTo("refs/changes/01/1/meta");
 
@@ -103,7 +109,9 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
         + "Foo\n"
         + "\n"
         + "Patch-set: 1\n"
-        + "Commit: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef\n",
+        + "Subject: Subject\n"
+        + "Branch: refs/heads/master\n"
+        + "Commit: " + commit.name() + "\n",
         update.getRevision());
   }
 

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

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.testutil;
 
+import static com.google.common.base.MoreObjects.firstNonNull;
 import static org.easymock.EasyMock.expect;
 
 import com.google.common.collect.Ordering;
@@ -39,8 +40,13 @@ import com.google.gwtorm.server.OrmException;
 import com.google.inject.Injector;
 
 import org.easymock.EasyMock;
+import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
 
+import java.util.TimeZone;
 import java.util.concurrent.atomic.AtomicInteger;
 
 /**
@@ -84,8 +90,8 @@ public class TestChanges {
   public static ChangeUpdate newUpdate(Injector injector,
       GitRepositoryManager repoManager, NotesMigration migration, Change c,
       final AllUsersNameProvider allUsers, final IdentifiedUser user)
-      throws OrmException {
+      throws Exception  {
-    return injector.createChildInjector(new FactoryModule() {
+    ChangeUpdate update = injector.createChildInjector(new FactoryModule() {
       @Override
       public void configure() {
         factory(ChangeUpdate.Factory.class);
@@ -96,6 +102,32 @@ public class TestChanges {
     }).getInstance(ChangeUpdate.Factory.class).create(
         stubChangeControl(repoManager, migration, c, allUsers, user),
         TimeUtil.nowTs(), Ordering.<String> natural());
+
+    ChangeNotes notes = update.getChangeNotes();
+    boolean hasPatchSets = notes.getPatchSets() != null
+        && !notes.getPatchSets().isEmpty();
+    if (hasPatchSets || !migration.readChanges()) {
+      return update;
+    }
+
+    // Change doesn't exist yet. Notedb requires that there be a commit for the
+    // first patch set, so create one.
+    try (Repository repo = repoManager.openRepository(c.getProject())) {
+      TestRepository<Repository> tr = new TestRepository<>(repo);
+      PersonIdent ident =
+          user.newCommitterIdent(update.getWhen(), TimeZone.getDefault());
+      TestRepository<Repository>.CommitBuilder cb = tr.commit()
+          .author(ident)
+          .committer(ident)
+          .message(firstNonNull(c.getSubject(), "Test change"));
+      Ref parent = repo.exactRef(c.getDest().get());
+      if (parent != null) {
+        cb.parent(tr.getRevWalk().parseCommit(parent.getObjectId()));
+      }
+      update.setBranch(c.getDest().get());
+      update.setCommit(tr.getRevWalk(), cb.create());
+      return update;
+    }
   }
 
   public static ChangeControl stubChangeControl(