Merge "DeleteChangeMessage: use ID instead index of a change message"

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, String targetMessageId, String newMessage) {
-  public void replaceChangeMessage(ChangeUpdate update, int targetMessageIdx, String newMessage) {
+    update.deleteChangeMessageByRewritingHistory(targetMessageId, newMessage);
-    update.deleteChangeMessageByRewritingHistory(targetMessageIdx, 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)
+          isTargetCommit ? createNewCommitMessage(originalCommit) : originalCommit.getFullMessage();
-              ? 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) {
+    DeleteChangeMessageOp(String targetMessageIdx, String newMessage) {
-      this.targetMessageIdx = targetMessageIdx;
+      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(
+    assertMetaCommitsAfterDeletion(commitsBefore, changeNum, id, deletedBy, reason);
-        commitsBefore, changeNum, deletedMessageIndex, 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);