Notedb: Add 'Commit' footer

This makes it possible to map a patch set id to the commit object under review
particularly when no inline-comment has been made.

Change-Id: I01042c15399f7aa315eb9ab8ae0519cfae608f6a

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

@@ -28,6 +28,7 @@ import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 
+import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.revwalk.RevCommit;
 
 import java.sql.Timestamp;
@@ -36,6 +37,7 @@ import java.util.Collections;
 /** Utilities for manipulating patch sets. */
 @Singleton
 public class PatchSetUtil {
+  @SuppressWarnings("unused") // TODO(dborowitz): Read from notedb.
   private final NotesMigration migration;
 
   @Inject
@@ -83,13 +85,11 @@ public class PatchSetUtil {
     ps.setPushCertificate(pushCertificate);
     db.patchSets().insert(Collections.singleton(ps));
 
+    update.setCommit(ObjectId.fromString(ps.getRevision().get()));
     if (!update.getChange().getSubject().equals(commit.getShortMessage())) {
       update.setSubject(commit.getShortMessage());
     }
 
-    if (migration.writeChanges()) {
-      // TODO(dborowitz): Write to notedb.
-    }
     return ps;
   }
 }

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

@@ -29,6 +29,7 @@ public class ChangeNoteUtil {
   static final String GERRIT_PLACEHOLDER_HOST = "gerrit";
 
   static final FooterKey FOOTER_BRANCH = new FooterKey("Branch");
+  static final FooterKey FOOTER_COMMIT = new FooterKey("Commit");
   static final FooterKey FOOTER_HASHTAGS = new FooterKey("Hashtags");
   static final FooterKey FOOTER_LABEL = new FooterKey("Label");
   static final FooterKey FOOTER_PATCH_SET = new FooterKey("Patch-set");

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

@@ -126,6 +126,7 @@ 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;
@@ -246,6 +247,10 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     return ChangeNoteUtil.changeRefName(getChangeId());
   }
 
+  public PatchSet getCurrentPatchSet() {
+    return currentPatchSet;
+  }
+
   @Override
   protected void onLoad() throws IOException, ConfigInvalidException {
     ObjectId rev = getRevision();
@@ -274,6 +279,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
       change.setLastUpdatedOn(parser.lastUpdatedOn);
       change.setOwner(parser.ownerId);
       change.setSubmissionId(parser.submissionId);
+      currentPatchSet = parser.getCurrentPatchSet();
 
       if (parser.hashtags != null) {
         hashtags = ImmutableSet.copyOf(parser.hashtags);

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

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.notedb;
 
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_BRANCH;
+import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_COMMIT;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_HASHTAGS;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_LABEL;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_PATCH_SET;
@@ -58,6 +59,7 @@ import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.util.LabelVote;
 
 import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.errors.InvalidObjectIdException;
 import org.eclipse.jgit.errors.RepositoryNotFoundException;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.PersonIdent;
@@ -95,6 +97,7 @@ class ChangeNotesParser implements AutoCloseable {
   String subject;
   String originalSubject;
   String submissionId;
+  PatchSet.Id currentPatchSetId;
 
   private final Change.Id changeId;
   private final ObjectId tip;
@@ -104,6 +107,7 @@ 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,
@@ -119,6 +123,11 @@ class ChangeNotesParser implements AutoCloseable {
     allChangeMessages = Lists.newArrayList();
     changeMessagesByPatchSet = LinkedListMultimap.create();
     comments = ArrayListMultimap.create();
+    patchSets = Maps.newHashMap();
+  }
+
+  public PatchSet getCurrentPatchSet() {
+    return patchSets.get(currentPatchSetId);
   }
 
   @Override
@@ -177,7 +186,12 @@ class ChangeNotesParser implements AutoCloseable {
     if (status == null) {
       status = parseStatus(commit);
     }
+
     PatchSet.Id psId = parsePatchSetId(commit);
+    if (currentPatchSetId == null) {
+      currentPatchSetId = psId;
+    }
+
     Account.Id accountId = parseIdent(commit);
     ownerId = accountId;
     if (subject == null) {
@@ -193,6 +207,11 @@ class ChangeNotesParser implements AutoCloseable {
       submissionId = parseSubmissionId(commit);
     }
 
+    ObjectId currRev = parseRevision(commit);
+    if (currRev != null) {
+      parsePatchSet(psId, currRev);
+    }
+
     if (submitRecords.isEmpty()) {
       // Only parse the most recent set of submit records; any older ones are
       // still there, but not currently used.
@@ -239,6 +258,35 @@ class ChangeNotesParser implements AutoCloseable {
     return footerLines.get(0);
   }
 
+  private ObjectId parseRevision(RevCommit commit)
+      throws ConfigInvalidException {
+    String sha = parseOneFooter(commit, FOOTER_COMMIT);
+    if (sha == null) {
+      return null;
+    }
+    try {
+      return ObjectId.fromString(sha);
+    } catch (InvalidObjectIdException e) {
+      ConfigInvalidException cie = invalidFooter(FOOTER_COMMIT, sha);
+      cie.initCause(e);
+      throw cie;
+    }
+  }
+
+  private void parsePatchSet(PatchSet.Id psId, ObjectId rev)
+      throws ConfigInvalidException {
+    if (patchSets.containsKey(psId)) {
+      throw new ConfigInvalidException(
+          String.format(
+              "Multiple revisions parsed for patch set %s: %s and %s",
+              psId.get(), patchSets.get(psId).getRevision(), rev.name()));
+    } else {
+      PatchSet ps = new PatchSet(psId);
+      ps.setRevision(new RevId(rev.name()));
+      patchSets.put(psId, ps);
+    }
+  }
+
   private void parseHashtags(RevCommit commit) throws ConfigInvalidException {
     // Commits are parsed in reverse order and only the last set of hashtags should be used.
     if (hashtags != null) {

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

@@ -16,6 +16,7 @@ package com.google.gerrit.server.notedb;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_BRANCH;
+import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_COMMIT;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_HASHTAGS;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_LABEL;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_PATCH_SET;
@@ -105,6 +106,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
   private final CommentsInNotesUtil commentsUtil;
   private List<PatchLineComment> comments;
   private String topic;
+  private ObjectId commit;
   private Set<String> hashtags;
   private String changeMessage;
   private ChangeNotes notes;
@@ -357,6 +359,11 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     this.topic = Strings.nullToEmpty(topic);
   }
 
+  public void setCommit(ObjectId commit) {
+    checkArgument(commit != null);
+    this.commit = commit;
+  }
+
   public void setHashtags(Set<String> hashtags) {
     this.hashtags = hashtags;
   }
@@ -432,12 +439,12 @@ public class ChangeUpdate extends AbstractChangeUpdate {
   }
 
   @Override
-  protected boolean onSave(CommitBuilder commit) {
+  protected boolean onSave(CommitBuilder cb) {
     if (getRevision() != null && isEmpty()) {
       return false;
     }
-    commit.setAuthor(newIdent(getUser().getAccount(), when));
+    cb.setAuthor(newIdent(getUser().getAccount(), when));
-    commit.setCommitter(new PersonIdent(serverIdent, when));
+    cb.setCommitter(new PersonIdent(serverIdent, when));
 
     int ps = psId != null ? psId.get() : getChange().currentPatchSetId().get();
     StringBuilder msg = new StringBuilder();
@@ -472,6 +479,10 @@ public class ChangeUpdate extends AbstractChangeUpdate {
       addFooter(msg, FOOTER_TOPIC, topic);
     }
 
+    if (commit != null) {
+      addFooter(msg, FOOTER_COMMIT, commit.name());
+    }
+
     if (hashtags != null) {
       addFooter(msg, FOOTER_HASHTAGS, Joiner.on(",").join(hashtags));
     }
@@ -526,7 +537,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
       }
     }
 
-    commit.setMessage(msg.toString());
+    cb.setMessage(msg.toString());
     return true;
   }
 
@@ -547,7 +558,8 @@ public class ChangeUpdate extends AbstractChangeUpdate {
         && submissionId == null
         && submitRecords == null
         && hashtags == null
-        && topic == null;
+        && topic == null
+        && commit == null;
   }
 
   private static StringBuilder addFooter(StringBuilder sb, FooterKey footer) {

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

@@ -272,6 +272,29 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
         + "Subject: Some other subject\n");
   }
 
+  @Test
+  public void parseCommit() throws Exception {
+    assertParseSucceeds("Update change\n"
+        + "\n"
+        + "Patch-set: 1\n"
+        + "Branch: refs/heads/master\n"
+        + "Subject: Some subject of a change\n"
+        + "Commit: abcd1234abcd1234abcd1234abcd1234abcd1234");
+    assertParseFails("Update change\n"
+        + "\n"
+        + "Patch-set: 1\n"
+        + "Branch: refs/heads/master\n"
+        + "Subject: Some subject of a change\n"
+        + "Commit: abcd1234abcd1234abcd1234abcd1234abcd1234\n"
+        + "Commit: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
+    assertParseFails("Update patch set 1\n"
+        + "Uploaded patch set 1.\n"
+        + "Patch-set: 1\n"
+        + "Branch: refs/heads/master\n"
+        + "Subject: Some subject of a change\n"
+        + "Commit: beef");
+  }
+
   private RevCommit writeCommit(String body) throws Exception {
     return writeCommit(body, ChangeNoteUtil.newIdent(
         changeOwner.getAccount(), TimeUtil.nowTs(), serverIdent,

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

@@ -42,6 +42,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.server.git.VersionedMetaData.BatchMetaDataUpdate;
+import com.google.gwtorm.server.OrmException;
 
 import org.eclipse.jgit.lib.BatchRefUpdate;
 import org.eclipse.jgit.lib.Constants;
@@ -583,6 +584,62 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         .isGreaterThan(lastUpdatedOn);
   }
 
+  @Test
+  public void commitChangeNotesUnique() throws Exception {
+    // PatchSetId -> RevId must be a one to one mapping
+    Change c = newChange();
+
+    ChangeNotes notes = newNotes(c);
+    assertThat(notes.getCurrentPatchSet()).isNull();
+
+    // 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 =
+        ObjectId.fromString("badc0feebadc0feebadc0feebadc0feebadc0fee");
+    update.setCommit(commit);
+    update.commit();
+    exception.expect(OrmException.class);
+    exception.expectMessage("Multiple revisions parsed for patch set");
+    notes = newNotes(c);
+  }
+
+  @Test
+  public void commitChangeNotes() throws Exception {
+    Change c = newChange();
+
+    ChangeNotes notes = newNotes(c);
+    assertThat(notes.getCurrentPatchSet()).isNull();
+
+    // 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());
+
+    // ps2
+    incrementPatchSet(c);
+    commit =
+        ObjectId.fromString("badc0feebadc0feebadc0feebadc0feebadc0fee");
+    update.setCommit(commit);
+    update.commit();
+    notes = newNotes(c);
+    assertThat(notes.getCurrentPatchSet().getRevision().get())
+        .isEqualTo(commit.name());
+  }
+
   @Test
   public void emptyExceptSubject() throws Exception {
     ChangeUpdate update = newUpdate(newChange(), changeOwner);

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

@@ -88,6 +88,25 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
         update.getRevision());
   }
 
+  @Test
+  public void changeWithRevision() throws Exception {
+    Change c = TestChanges.newChange(project, changeOwner.getAccountId(), 1);
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.setChangeMessage("Foo");
+    update.setCommit(
+        ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
+    update.commit();
+    assertThat(update.getRefName()).isEqualTo("refs/changes/01/1/meta");
+
+    assertBodyEquals("Update patch set 1\n"
+        + "\n"
+        + "Foo\n"
+        + "\n"
+        + "Patch-set: 1\n"
+        + "Commit: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef\n",
+        update.getRevision());
+  }
+
   @Test
   public void approvalTombstoneCommitFormat() throws Exception {
     Change c = newChange();