DeleteChangeMessage: use ID instead index of a change message

When this API was implemented, we had to use the
index of a change message because at that time we had
change messages from both ReviewDb and NoteDb but with
different IDs. Thus we made the decision to use the
"index" of a change message as the bridge between NoteDb
and ReviewDb. IOW, we assumed that a change message with
"index = k" corresponds to the kth change meta commit.

However, this is not true because empty change messages
are skipped from the change message list in
ChangeNotesParser#parseChangeMessage. When a change
has empty change message, DeleteChangeMessage could
delete the wrong message.

Now, we don't have ReviewDb any more. It makes sense
to use the ID of the change message directly and thus
fix the above issue.

Change-Id: I09b0e16d9e014f674a7bee5e798eee8f635b6bdd

java/com/google/gerrit/server/ChangeMessagesUtil.java

@@ -109,12 +109,11 @@ public class ChangeMessagesUtil {
    * rather than an ID allowed us to delete the message from both NoteDb and ReviewDb.
    *
    * @param update change update.
-   * @param targetMessageIdx the index of the target change message.
+   * @param targetMessageId the id of the target change message.
    * @param newMessage the new message which is going to replace the old.
    */
-  // TODO(xchangcheng): Reconsider implementation now that there is only a single ID.
-  public void replaceChangeMessage(ChangeUpdate update, int targetMessageIdx, String newMessage) {
-    update.deleteChangeMessageByRewritingHistory(targetMessageIdx, newMessage);
+  public void replaceChangeMessage(ChangeUpdate update, String targetMessageId, String newMessage) {
+    update.deleteChangeMessageByRewritingHistory(targetMessageId, newMessage);
   }
 
   /**

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

@@ -368,9 +368,9 @@ public class ChangeUpdate extends AbstractChangeUpdate {
         deleteCommentRewriterFactory.create(getChange().getId(), uuid, newMessage);
   }
 
-  public void deleteChangeMessageByRewritingHistory(int targetMessageIdx, String newMessage) {
+  public void deleteChangeMessageByRewritingHistory(String targetMessageId, String newMessage) {
     deleteChangeMessageRewriter =
-        new DeleteChangeMessageRewriter(getChange().getId(), targetMessageIdx, newMessage);
+        new DeleteChangeMessageRewriter(getChange().getId(), targetMessageId, newMessage);
   }
 
   @VisibleForTesting

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

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.notedb;
 
 import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.parseCommitMessageRange;
 import static org.eclipse.jgit.util.RawParseUtils.decode;
@@ -40,12 +41,12 @@ import org.eclipse.jgit.util.RawParseUtils;
 public class DeleteChangeMessageRewriter implements NoteDbRewriter {
 
   private final Change.Id changeId;
-  private final int targetMessageIdx;
+  private final String targetMessageId;
   private final String newChangeMessage;
 
-  DeleteChangeMessageRewriter(Change.Id changeId, int targetMessageIdx, String newChangeMessage) {
+  DeleteChangeMessageRewriter(Change.Id changeId, String targetMessageId, String newChangeMessage) {
     this.changeId = changeId;
-    this.targetMessageIdx = targetMessageIdx;
+    this.targetMessageId = checkNotNull(targetMessageId);
     this.newChangeMessage = newChangeMessage;
   }
 
@@ -67,21 +68,18 @@ public class DeleteChangeMessageRewriter implements NoteDbRewriter {
 
     ObjectId newTipId = null;
     RevCommit originalCommit;
-    int idx = 0;
+    boolean startRewrite = false;
     while ((originalCommit = revWalk.next()) != null) {
-      if (idx < targetMessageIdx) {
+      boolean isTargetCommit = originalCommit.getId().getName().equals(targetMessageId);
+      if (!startRewrite && !isTargetCommit) {
         newTipId = originalCommit;
-        idx++;
         continue;
       }
 
+      startRewrite = true;
       String newCommitMessage =
-          (idx == targetMessageIdx)
-              ? createNewCommitMessage(originalCommit)
-              : originalCommit.getFullMessage();
+          isTargetCommit ? createNewCommitMessage(originalCommit) : originalCommit.getFullMessage();
       newTipId = rewriteOneCommit(originalCommit, newTipId, newCommitMessage, inserter);
-
-      idx++;
     }
     return newTipId;
   }

java/com/google/gerrit/server/restapi/change/DeleteChangeMessage.java

@@ -91,7 +91,7 @@ public class DeleteChangeMessage
     String newChangeMessage =
         createNewChangeMessage(user.asIdentifiedUser().getName(), input.reason);
     DeleteChangeMessageOp deleteChangeMessageOp =
-        new DeleteChangeMessageOp(resource.getChangeMessageIndex(), newChangeMessage);
+        new DeleteChangeMessageOp(resource.getChangeMessageId(), newChangeMessage);
     try (BatchUpdate batchUpdate =
         updateFactory.create(resource.getChangeResource().getProject(), user, TimeUtil.nowTs())) {
       batchUpdate.addOp(resource.getChangeId(), deleteChangeMessageOp).execute();
@@ -130,18 +130,18 @@ public class DeleteChangeMessage
   }
 
   private class DeleteChangeMessageOp implements BatchUpdateOp {
-    private final int targetMessageIdx;
+    private final String targetMessageId;
     private final String newMessage;
 
-    DeleteChangeMessageOp(int targetMessageIdx, String newMessage) {
-      this.targetMessageIdx = targetMessageIdx;
+    DeleteChangeMessageOp(String targetMessageIdx, String newMessage) {
+      this.targetMessageId = targetMessageIdx;
       this.newMessage = newMessage;
     }
 
     @Override
     public boolean updateChange(ChangeContext ctx) {
       PatchSet.Id psId = ctx.getChange().currentPatchSetId();
-      changeMessagesUtil.replaceChangeMessage(ctx.getUpdate(psId), targetMessageIdx, newMessage);
+      changeMessagesUtil.replaceChangeMessage(ctx.getUpdate(psId), targetMessageId, newMessage);
       return true;
     }
   }

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

@@ -130,6 +130,20 @@ public class ChangeMessagesIT extends AbstractDaemonTest {
     assertThat(messages1).containsExactlyElementsIn(messages2).inOrder();
   }
 
+  @Test
+  public void listChangeMessagesSkippedEmpty() throws Exception {
+    // Change message 1: create a change.
+    PushOneCommit.Result result = createChange();
+    String changeId = result.getChangeId();
+    // Will be a new commit with empty change message on the meta branch.
+    addOneReviewWithEmptyChangeMessage(changeId);
+    // Change Message 2: post a review with message "message 1".
+    addOneReview(changeId, "message");
+
+    List<ChangeMessageInfo> messages = gApi.changes().id(changeId).messages();
+    assertThat(messages).hasSize(2);
+  }
+
   @Test
   public void getOneChangeMessage() throws Exception {
     int changeNum = createOneChangeWithMultipleChangeMessagesInHistory();
@@ -217,22 +231,28 @@ public class ChangeMessagesIT extends AbstractDaemonTest {
     // Commit 1: create a change.
     PushOneCommit.Result result = createChange();
     String changeId = result.getChangeId();
-    // Commit 2: post a review with message "message 1".
+    // Commit 2: post an empty change message.
     requestScopeOperations.setApiUser(admin.getId());
+    addOneReviewWithEmptyChangeMessage(changeId);
+    // Commit 3: post a review with message "message 1".
     addOneReview(changeId, "message 1");
-    // Commit 3: amend a new patch set.
+    // Commit 4: amend a new patch set.
     requestScopeOperations.setApiUser(user.getId());
     amendChange(changeId);
-    // Commit 4: post a review with message "message 2".
+    // Commit 5: post a review with message "message 2".
     addOneReview(changeId, "message 2");
-    // Commit 5: amend a new patch set.
+    // Commit 6: amend a new patch set.
     amendChange(changeId);
-    // Commit 6: approve the change.
+    // Commit 7: approve the change.
     requestScopeOperations.setApiUser(admin.getId());
     gApi.changes().id(changeId).current().review(ReviewInput.approve());
-    // commit 7: submit the change.
+    // commit 8: submit the change.
     gApi.changes().id(changeId).current().submit();
 
+    // Verifies there is only 7 change messages although there are 8 commits.
+    List<ChangeMessageInfo> messages = gApi.changes().id(changeId).messages();
+    assertThat(messages).hasSize(7);
+
     return result.getChange().getId().get();
   }
 
@@ -249,6 +269,10 @@ public class ChangeMessagesIT extends AbstractDaemonTest {
     gApi.changes().id(changeId).current().review(reviewInput);
   }
 
+  private void addOneReviewWithEmptyChangeMessage(String changeId) throws Exception {
+    gApi.changes().id(changeId).current().review(new ReviewInput());
+  }
+
   private void deleteOneChangeMessage(
       int changeNum, int deletedMessageIndex, TestAccount deletedBy, String reason)
       throws Exception {
@@ -273,8 +297,7 @@ public class ChangeMessagesIT extends AbstractDaemonTest {
     assertThat(changes.stream().map(c -> c._number).collect(toSet())).contains(changeNum);
 
     // Verifies states of commits.
-    assertMetaCommitsAfterDeletion(
-        commitsBefore, changeNum, deletedMessageIndex, deletedBy, reason);
+    assertMetaCommitsAfterDeletion(commitsBefore, changeNum, id, deletedBy, reason);
   }
 
   private void assertMessagesAfterDeletion(
@@ -313,7 +336,7 @@ public class ChangeMessagesIT extends AbstractDaemonTest {
   private void assertMetaCommitsAfterDeletion(
       List<RevCommit> commitsBeforeDeletion,
       int changeNum,
-      int deletedMessageIndex,
+      String deletedMessageId,
       TestAccount deletedBy,
       String deleteReason)
       throws Exception {
@@ -324,7 +347,7 @@ public class ChangeMessagesIT extends AbstractDaemonTest {
     for (int i = 0; i < commitsBeforeDeletion.size(); i++) {
       RevCommit commitBefore = commitsBeforeDeletion.get(i);
       RevCommit commitAfter = commitsAfterDeletion.get(i);
-      if (i == deletedMessageIndex) {
+      if (commitBefore.getId().getName().equals(deletedMessageId)) {
         byte[] rawBefore = commitBefore.getRawBuffer();
         byte[] rawAfter = commitAfter.getRawBuffer();
         Charset encodingBefore = RawParseUtils.parseEncoding(rawBefore);