From 8c1ebed5ed5a4a4df283aff6e01b4021fcfbe61c Mon Sep 17 00:00:00 2001 From: Changcheng Xiao Date: Thu, 28 Mar 2019 21:08:07 +0100 Subject: [PATCH] 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 --- .../gerrit/server/ChangeMessagesUtil.java | 7 ++- .../gerrit/server/notedb/ChangeUpdate.java | 4 +- .../notedb/DeleteChangeMessageRewriter.java | 20 ++++----- .../restapi/change/DeleteChangeMessage.java | 10 ++--- .../rest/change/ChangeMessagesIT.java | 43 ++++++++++++++----- 5 files changed, 52 insertions(+), 32 deletions(-) diff --git a/java/com/google/gerrit/server/ChangeMessagesUtil.java b/java/com/google/gerrit/server/ChangeMessagesUtil.java index 8b9a6f4aad..214c5d7fea 100644 --- a/java/com/google/gerrit/server/ChangeMessagesUtil.java +++ b/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); } /** diff --git a/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/java/com/google/gerrit/server/notedb/ChangeUpdate.java index 10f1f041e3..e1ef78ac20 100644 --- a/java/com/google/gerrit/server/notedb/ChangeUpdate.java +++ b/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 diff --git a/java/com/google/gerrit/server/notedb/DeleteChangeMessageRewriter.java b/java/com/google/gerrit/server/notedb/DeleteChangeMessageRewriter.java index 212aa3762e..8fb28e1bc9 100644 --- a/java/com/google/gerrit/server/notedb/DeleteChangeMessageRewriter.java +++ b/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; } diff --git a/java/com/google/gerrit/server/restapi/change/DeleteChangeMessage.java b/java/com/google/gerrit/server/restapi/change/DeleteChangeMessage.java index adbe0e664c..5dcd1fac2d 100644 --- a/java/com/google/gerrit/server/restapi/change/DeleteChangeMessage.java +++ b/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; } } diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeMessagesIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeMessagesIT.java index 3873c9d8df..dc765859b3 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeMessagesIT.java +++ b/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 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 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 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);