Add support for deleting change message
This commit adds a new REST endpoint to support deletion of
a change message, which is similar to Icaeb3c24f.
The deletion for NoteDb is implemented by rewriting the change
meta branch, which will replace the whole change message with
a sentence like "Change message removed by:... ". Messages
like "(N comments)" and quotes will also be replaced as
they are part of a change message.
To be able to delete a change message, a user must hold the
"Administrate Server" global capability.
Change-Id: I439b00fcf03a629d9a33c401cf4d8cdd49705ace
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index cb3b88b..9b3fb52 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -2475,7 +2475,66 @@
"username": "jdoe"
},
"date": "2013-03-23 21:34:02.419000000",
- "message": "Change message removed by: Administrator; Reason: spam",
+ "message": "a change message",
+ "_revision_number": 1
+ }
+----
+
+[[delete-change-message]]
+=== Delete Change Message
+--
+'DELETE /changes/link:#change-id[\{change-id\}]/message/link:#change-message-id[\{change-message-id\}' +
+'POST /changes/link:#change-id[\{change-id\}]//message/link:#change-message-id[\{change-message-id\}/delete'
+--
+
+Deletes a change message by replacing the change message with a new message,
+which contains the name of the user who deleted the change message and the
+reason why it was deleted. The reason can be provided in the request body as a
+link:#delete-change-message-input[DeleteChangeMessageInput] entity.
+
+Note that only users with the
+link:access-control.html#capability_administrateServer[Administrate Server]
+global capability are permitted to delete a change message.
+
+To delete a change message, send a DELETE request:
+
+.Request
+----
+ DELETE /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/message/aaee04dcb46bafc8be24d8aa70b3b1beb7df5780 HTTP/1.0
+----
+
+To provide a reason for the deletion, use a POST request:
+
+.Request
+----
+ POST /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/message/aaee04dcb46bafc8be24d8aa70b3b1beb7df5780/delete HTTP/1.0
+ Content-Type: application/json; charset=UTF-8
+
+ {
+ "reason": "spam"
+ }
+----
+
+As response a link:#change-message-info[ChangeMessageInfo] entity is returned that
+describes the updated change message.
+
+.Response
+----
+ HTTP/1.1 200 OK
+ Content-Disposition: attachment
+ Content-Type: application/json; charset=UTF-8
+
+ )]}'
+ {
+ "id": "aaee04dcb46bafc8be24d8aa70b3b1beb7df5780",
+ "author": {
+ "_account_id": 1000096,
+ "name": "John Doe",
+ "email": "john.doe@example.com",
+ "username": "jdoe"
+ },
+ "date": "2013-03-23 21:34:02.419000000",
+ "message": "Change message removed by: Administrator\nReason: spam",
"_revision_number": 1
}
----
@@ -6049,6 +6108,20 @@
of recipient type to link:#notify-info[NotifyInfo] entity.
|=============================
+[[delete-change-message-input]]
+=== DeleteChangeMessageInput
+The `DeleteChangeMessageInput` entity contains the options for deleting a change message.
+
+[options="header",cols="1,^1,5"]
+|=============================
+|Field Name ||Description
+|`reason` |optional|
+The reason why the change message should be deleted. +
+If set, the change message will be replaced with
+"Change message removed by: `name`\nReason: `reason`",
+or just "Change message removed by: `name`." if not set.
+|=============================
+
[[delete-comment-input]]
=== DeleteCommentInput
The `DeleteCommentInput` entity contains the option for deleting a comment.
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 960b558..1d87880 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -33,6 +33,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
import com.google.common.jimfs.Jimfs;
import com.google.common.primitives.Chars;
import com.google.gerrit.acceptance.AcceptanceTestRequestScope.Context;
@@ -63,6 +64,7 @@
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeType;
+import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.extensions.common.DiffInfo;
import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.extensions.restapi.BinaryResult;
@@ -76,6 +78,7 @@
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.BooleanProjectConfig;
import com.google.gerrit.reviewdb.client.Branch;
+import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
@@ -148,6 +151,7 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
+import java.util.Comparator;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.List;
@@ -167,6 +171,7 @@
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevSort;
import org.eclipse.jgit.revwalk.RevTree;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.FetchResult;
@@ -1695,4 +1700,29 @@
}
}
}
+
+ protected List<RevCommit> getChangeMetaCommitsInReverseOrder(Change.Id changeId)
+ throws IOException {
+ try (Repository repo = repoManager.openRepository(project);
+ RevWalk revWalk = new RevWalk(repo)) {
+ revWalk.sort(RevSort.TOPO);
+ revWalk.sort(RevSort.REVERSE);
+ Ref metaRef = repo.exactRef(RefNames.changeMetaRef(changeId));
+ revWalk.markStart(revWalk.parseCommit(metaRef.getObjectId()));
+ return Lists.newArrayList(revWalk);
+ }
+ }
+
+ protected List<CommentInfo> getChangeSortedComments(int changeNum) throws Exception {
+ List<CommentInfo> comments = new ArrayList<>();
+ Map<String, List<CommentInfo>> commentsMap = gApi.changes().id(changeNum).comments();
+ for (Map.Entry<String, List<CommentInfo>> e : commentsMap.entrySet()) {
+ for (CommentInfo c : e.getValue()) {
+ c.path = e.getKey(); // Set the comment's path field.
+ comments.add(c);
+ }
+ }
+ comments.sort(Comparator.comparing(c -> c.id));
+ return comments;
+ }
}
diff --git a/java/com/google/gerrit/extensions/api/changes/ChangeMessageApi.java b/java/com/google/gerrit/extensions/api/changes/ChangeMessageApi.java
index eb2d2b9..66356f1 100644
--- a/java/com/google/gerrit/extensions/api/changes/ChangeMessageApi.java
+++ b/java/com/google/gerrit/extensions/api/changes/ChangeMessageApi.java
@@ -24,6 +24,14 @@
ChangeMessageInfo get() throws RestApiException;
/**
+ * Deletes a change message by replacing its message. For NoteDb, it's implemented by rewriting
+ * the commit history of change meta branch.
+ *
+ * @return the change message with its message updated.
+ */
+ ChangeMessageInfo delete(DeleteChangeMessageInput input) throws RestApiException;
+
+ /**
* A default implementation which allows source compatibility when adding new methods to the
* interface.
*/
@@ -32,5 +40,10 @@
public ChangeMessageInfo get() throws RestApiException {
throw new NotImplementedException();
}
+
+ @Override
+ public ChangeMessageInfo delete(DeleteChangeMessageInput input) throws RestApiException {
+ throw new NotImplementedException();
+ }
}
}
diff --git a/java/com/google/gerrit/extensions/api/changes/DeleteChangeMessageInput.java b/java/com/google/gerrit/extensions/api/changes/DeleteChangeMessageInput.java
new file mode 100644
index 0000000..ece5a61
--- /dev/null
+++ b/java/com/google/gerrit/extensions/api/changes/DeleteChangeMessageInput.java
@@ -0,0 +1,31 @@
+// Copyright (C) 2018 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.extensions.api.changes;
+
+import com.google.gerrit.extensions.restapi.DefaultInput;
+
+/** Input for deleting a change message. */
+public class DeleteChangeMessageInput {
+ /** An optional reason for deleting the change message. */
+ @DefaultInput public String reason;
+
+ public DeleteChangeMessageInput() {
+ this.reason = "";
+ }
+
+ public DeleteChangeMessageInput(String reason) {
+ this.reason = reason;
+ }
+}
diff --git a/java/com/google/gerrit/extensions/common/ChangeMessageInfo.java b/java/com/google/gerrit/extensions/common/ChangeMessageInfo.java
index a2e61b8..07ad71b 100644
--- a/java/com/google/gerrit/extensions/common/ChangeMessageInfo.java
+++ b/java/com/google/gerrit/extensions/common/ChangeMessageInfo.java
@@ -45,4 +45,24 @@
public int hashCode() {
return Objects.hash(id, tag, author, realAuthor, date, message, _revisionNumber);
}
+
+ @Override
+ public String toString() {
+ return "ChangeMessageInfo{"
+ + "id="
+ + id
+ + ", tag="
+ + tag
+ + ", author="
+ + author
+ + ", realAuthor="
+ + realAuthor
+ + ", date="
+ + date
+ + ", _revisionNumber"
+ + _revisionNumber
+ + ", message=["
+ + message
+ + "]}";
+ }
}
diff --git a/java/com/google/gerrit/server/ChangeMessagesUtil.java b/java/com/google/gerrit/server/ChangeMessagesUtil.java
index e635072..a6dbb53 100644
--- a/java/com/google/gerrit/server/ChangeMessagesUtil.java
+++ b/java/com/google/gerrit/server/ChangeMessagesUtil.java
@@ -16,6 +16,7 @@
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.gerrit.reviewdb.server.ReviewDbUtil.unwrapDb;
import com.google.common.annotations.VisibleForTesting;
import com.google.gerrit.common.Nullable;
@@ -27,7 +28,9 @@
import com.google.gerrit.server.account.AccountLoader;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.notedb.NotesMigration;
+import com.google.gerrit.server.update.BatchUpdateReviewDb;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -129,6 +132,71 @@
}
/**
+ * Replace an existing change message with the provided new message.
+ *
+ * <p>The ID of a change message is different between NoteDb and ReviewDb. In NoteDb, it's the
+ * commit SHA-1, but in ReviewDb it's generated randomly. To make sure the change message can be
+ * deleted from both NoteDb and ReviewDb, the index of the change message must be used rather than
+ * its ID.
+ *
+ * @param db the {@code ReviewDb} instance to update.
+ * @param update change update.
+ * @param targetMessageIdx the index of the target change message.
+ * @param newMessage the new message which is going to replace the old.
+ * @throws OrmException
+ */
+ public void replaceChangeMessage(
+ ReviewDb db, ChangeUpdate update, int targetMessageIdx, String newMessage)
+ throws OrmException {
+ if (PrimaryStorage.of(update.getChange()).equals(PrimaryStorage.REVIEW_DB)) {
+ if (db instanceof BatchUpdateReviewDb) {
+ db = ((BatchUpdateReviewDb) db).unsafeGetDelegate();
+ }
+ db = unwrapDb(db);
+
+ List<ChangeMessage> messagesInReviewDb =
+ sortChangeMessages(db.changeMessages().byChange(update.getId()));
+ if (migration.readChanges()) {
+ sanityCheckForChangeMessages(messagesInReviewDb, update.getNotes().getChangeMessages());
+ }
+ ChangeMessage targetMessage = messagesInReviewDb.get(targetMessageIdx);
+ targetMessage.setMessage(newMessage);
+ db.changeMessages().upsert(Collections.singleton(targetMessage));
+ }
+
+ update.deleteChangeMessageByRewritingHistory(targetMessageIdx, newMessage);
+ }
+
+ private static void sanityCheckForChangeMessages(
+ List<ChangeMessage> messagesInReviewDb, List<ChangeMessage> messagesInNoteDb) {
+ String message =
+ String.format(
+ "Change messages in ReivewDb and NoteDb don't match: NoteDb %s; ReviewDb %s",
+ messagesInNoteDb, messagesInReviewDb);
+ if (messagesInReviewDb.size() != messagesInNoteDb.size()) {
+ throw new IllegalStateException(message);
+ }
+
+ for (int i = 0; i < messagesInReviewDb.size(); i++) {
+ ChangeMessage messageInReviewDb = messagesInReviewDb.get(i);
+ ChangeMessage messageInNoteDb = messagesInNoteDb.get(i);
+
+ // Don't compare the keys because they are different for the same change message in NoteDb and
+ // ReviewDb.
+ boolean isEqual =
+ Objects.equals(messageInReviewDb.getAuthor(), messageInNoteDb.getAuthor())
+ && Objects.equals(messageInReviewDb.getWrittenOn(), messageInNoteDb.getWrittenOn())
+ && Objects.equals(messageInReviewDb.getMessage(), messageInNoteDb.getMessage())
+ && Objects.equals(messageInReviewDb.getPatchSetId(), messageInNoteDb.getPatchSetId())
+ && Objects.equals(messageInReviewDb.getTag(), messageInNoteDb.getTag())
+ && Objects.equals(messageInReviewDb.getRealAuthor(), messageInNoteDb.getRealAuthor());
+ if (!isEqual) {
+ throw new IllegalStateException(message);
+ }
+ }
+ }
+
+ /**
* @param tag value of a tag, or null.
* @return whether the tag starts with the autogenerated prefix.
*/
diff --git a/java/com/google/gerrit/server/api/changes/ChangeMessageApiImpl.java b/java/com/google/gerrit/server/api/changes/ChangeMessageApiImpl.java
index 988ac91..14310e8 100644
--- a/java/com/google/gerrit/server/api/changes/ChangeMessageApiImpl.java
+++ b/java/com/google/gerrit/server/api/changes/ChangeMessageApiImpl.java
@@ -17,9 +17,11 @@
import static com.google.gerrit.server.api.ApiUtil.asRestApiException;
import com.google.gerrit.extensions.api.changes.ChangeMessageApi;
+import com.google.gerrit.extensions.api.changes.DeleteChangeMessageInput;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.change.ChangeMessageResource;
+import com.google.gerrit.server.restapi.change.DeleteChangeMessage;
import com.google.gerrit.server.restapi.change.GetChangeMessage;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@@ -30,12 +32,16 @@
}
private final GetChangeMessage getChangeMessage;
+ private final DeleteChangeMessage deleteChangeMessage;
private final ChangeMessageResource changeMessageResource;
@Inject
ChangeMessageApiImpl(
- GetChangeMessage getChangeMessage, @Assisted ChangeMessageResource changeMessageResource) {
+ GetChangeMessage getChangeMessage,
+ DeleteChangeMessage deleteChangeMessage,
+ @Assisted ChangeMessageResource changeMessageResource) {
this.getChangeMessage = getChangeMessage;
+ this.deleteChangeMessage = deleteChangeMessage;
this.changeMessageResource = changeMessageResource;
}
@@ -47,4 +53,13 @@
throw asRestApiException("Cannot retrieve change message", e);
}
}
+
+ @Override
+ public ChangeMessageInfo delete(DeleteChangeMessageInput input) throws RestApiException {
+ try {
+ return deleteChangeMessage.apply(changeMessageResource, input).value();
+ } catch (Exception e) {
+ throw asRestApiException("Cannot delete change message", e);
+ }
+ }
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/java/com/google/gerrit/server/notedb/ChangeUpdate.java
index 903a982..bd7f8fc 100644
--- a/java/com/google/gerrit/server/notedb/ChangeUpdate.java
+++ b/java/com/google/gerrit/server/notedb/ChangeUpdate.java
@@ -160,6 +160,7 @@
private ChangeDraftUpdate draftUpdate;
private RobotCommentUpdate robotCommentUpdate;
private DeleteCommentRewriter deleteCommentRewriter;
+ private DeleteChangeMessageRewriter deleteChangeMessageRewriter;
@AssistedInject
private ChangeUpdate(
@@ -395,6 +396,11 @@
deleteCommentRewriterFactory.create(getChange().getId(), uuid, newMessage);
}
+ public void deleteChangeMessageByRewritingHistory(int targetMessageIdx, String newMessage) {
+ deleteChangeMessageRewriter =
+ new DeleteChangeMessageRewriter(getChange().getId(), targetMessageIdx, newMessage);
+ }
+
@VisibleForTesting
ChangeDraftUpdate createDraftUpdateIfNull() {
if (draftUpdate == null) {
@@ -609,7 +615,9 @@
@Override
protected CommitBuilder applyImpl(RevWalk rw, ObjectInserter ins, ObjectId curr)
throws OrmException, IOException {
- checkState(deleteCommentRewriter == null, "cannot update and rewrite ref in one BatchUpdate");
+ checkState(
+ deleteCommentRewriter == null && deleteChangeMessageRewriter == null,
+ "cannot update and rewrite ref in one BatchUpdate");
CommitBuilder cb = new CommitBuilder();
@@ -823,6 +831,10 @@
return deleteCommentRewriter;
}
+ public DeleteChangeMessageRewriter getDeleteChangeMessageRewriter() {
+ return deleteChangeMessageRewriter;
+ }
+
public void setAllowWriteToNewRef(boolean allow) {
isAllowWriteToNewtRef = allow;
}
diff --git a/java/com/google/gerrit/server/notedb/DeleteChangeMessageRewriter.java b/java/com/google/gerrit/server/notedb/DeleteChangeMessageRewriter.java
new file mode 100644
index 0000000..212aa37
--- /dev/null
+++ b/java/com/google/gerrit/server/notedb/DeleteChangeMessageRewriter.java
@@ -0,0 +1,133 @@
+// Copyright (C) 2018 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.notedb;
+
+import static com.google.common.base.Preconditions.checkArgument;
+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;
+
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.RefNames;
+import java.io.IOException;
+import java.nio.charset.Charset;
+import java.util.Optional;
+import org.eclipse.jgit.lib.CommitBuilder;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectInserter;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevSort;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.util.RawParseUtils;
+
+/**
+ * Deletes a change message from NoteDb by rewriting the commit history. After deletion, the whole
+ * change message will be replaced by a new message indicating the original change message has been
+ * deleted for the given reason.
+ */
+public class DeleteChangeMessageRewriter implements NoteDbRewriter {
+
+ private final Change.Id changeId;
+ private final int targetMessageIdx;
+ private final String newChangeMessage;
+
+ DeleteChangeMessageRewriter(Change.Id changeId, int targetMessageIdx, String newChangeMessage) {
+ this.changeId = changeId;
+ this.targetMessageIdx = targetMessageIdx;
+ this.newChangeMessage = newChangeMessage;
+ }
+
+ @Override
+ public String getRefName() {
+ return RefNames.changeMetaRef(changeId);
+ }
+
+ @Override
+ public ObjectId rewriteCommitHistory(RevWalk revWalk, ObjectInserter inserter, ObjectId currTip)
+ throws IOException {
+ checkArgument(!currTip.equals(ObjectId.zeroId()));
+
+ // Walk from the first commit of the branch.
+ revWalk.reset();
+ revWalk.markStart(revWalk.parseCommit(currTip));
+ revWalk.sort(RevSort.TOPO);
+ revWalk.sort(RevSort.REVERSE);
+
+ ObjectId newTipId = null;
+ RevCommit originalCommit;
+ int idx = 0;
+ while ((originalCommit = revWalk.next()) != null) {
+ if (idx < targetMessageIdx) {
+ newTipId = originalCommit;
+ idx++;
+ continue;
+ }
+
+ String newCommitMessage =
+ (idx == targetMessageIdx)
+ ? createNewCommitMessage(originalCommit)
+ : originalCommit.getFullMessage();
+ newTipId = rewriteOneCommit(originalCommit, newTipId, newCommitMessage, inserter);
+
+ idx++;
+ }
+ return newTipId;
+ }
+
+ private String createNewCommitMessage(RevCommit commit) {
+ byte[] raw = commit.getRawBuffer();
+
+ Optional<ChangeNoteUtil.CommitMessageRange> range = parseCommitMessageRange(commit);
+ checkState(range.isPresent(), "failed to parse commit message");
+
+ // Only replace the commit message body, which is the user-provided message. The subject and
+ // footers are NoteDb metadata.
+ Charset encoding = RawParseUtils.parseEncoding(raw);
+ String prefix =
+ decode(encoding, raw, range.get().subjectStart(), range.get().changeMessageStart());
+ String postfix = decode(encoding, raw, range.get().changeMessageEnd() + 1, raw.length);
+ return prefix + newChangeMessage + postfix;
+ }
+
+ /**
+ * Rewrites one commit.
+ *
+ * @param originalCommit the original commit to be rewritten.
+ * @param parentCommitId the parent of the new commit. For the first rewritten commit, it's the
+ * parent of 'originalCommit'. For the latter rewritten commits, it's the commit rewritten
+ * just before it.
+ * @param commitMessage the full commit message of the new commit.
+ * @param inserter the {@code ObjectInserter} for the rewrite process.
+ * @return the {@code objectId} of the new commit.
+ * @throws IOException
+ */
+ private ObjectId rewriteOneCommit(
+ RevCommit originalCommit,
+ ObjectId parentCommitId,
+ String commitMessage,
+ ObjectInserter inserter)
+ throws IOException {
+ CommitBuilder cb = new CommitBuilder();
+ if (parentCommitId != null) {
+ cb.setParentId(parentCommitId);
+ }
+ cb.setTreeId(originalCommit.getTree());
+ cb.setMessage(commitMessage);
+ cb.setCommitter(originalCommit.getCommitterIdent());
+ cb.setAuthor(originalCommit.getAuthorIdent());
+ cb.setEncoding(originalCommit.getEncoding());
+ return inserter.insert(cb);
+ }
+}
diff --git a/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java b/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
index b20bc73..fd25059 100644
--- a/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
+++ b/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
@@ -467,14 +467,33 @@
if (rcu != null) {
robotCommentUpdates.put(rcu.getRefName(), rcu);
}
- DeleteCommentRewriter rwt = update.getDeleteCommentRewriter();
- if (rwt != null) {
- // Checks whether there is any ChangeUpdate added earlier trying to update the same ref.
+ DeleteCommentRewriter deleteCommentRewriter = update.getDeleteCommentRewriter();
+ if (deleteCommentRewriter != null) {
+ // Checks whether there is any ChangeUpdate or rewriter added earlier for the same ref.
checkArgument(
- !changeUpdates.containsKey(rwt.getRefName()),
+ !changeUpdates.containsKey(deleteCommentRewriter.getRefName()),
"cannot update & rewrite ref %s in one BatchUpdate",
- rwt.getRefName());
- rewriters.put(rwt.getRefName(), rwt);
+ deleteCommentRewriter.getRefName());
+ checkArgument(
+ !rewriters.containsKey(deleteCommentRewriter.getRefName()),
+ "cannot rewrite the same ref %s in one BatchUpdate",
+ deleteCommentRewriter.getRefName());
+ rewriters.put(deleteCommentRewriter.getRefName(), deleteCommentRewriter);
+ }
+
+ DeleteChangeMessageRewriter deleteChangeMessageRewriter =
+ update.getDeleteChangeMessageRewriter();
+ if (deleteChangeMessageRewriter != null) {
+ // Checks whether there is any ChangeUpdate or rewriter added earlier for the same ref.
+ checkArgument(
+ !changeUpdates.containsKey(deleteChangeMessageRewriter.getRefName()),
+ "cannot update & rewrite ref %s in one BatchUpdate",
+ deleteChangeMessageRewriter.getRefName());
+ checkArgument(
+ !rewriters.containsKey(deleteChangeMessageRewriter.getRefName()),
+ "cannot rewrite the same ref %s in one BatchUpdate",
+ deleteChangeMessageRewriter.getRefName());
+ rewriters.put(deleteChangeMessageRewriter.getRefName(), deleteChangeMessageRewriter);
}
changeUpdates.put(update.getRefName(), update);
diff --git a/java/com/google/gerrit/server/restapi/change/DeleteChangeMessage.java b/java/com/google/gerrit/server/restapi/change/DeleteChangeMessage.java
new file mode 100644
index 0000000..67c54c2
--- /dev/null
+++ b/java/com/google/gerrit/server/restapi/change/DeleteChangeMessage.java
@@ -0,0 +1,175 @@
+// Copyright (C) 2018 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.restapi.change;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.gerrit.server.ChangeMessagesUtil.createChangeMessageInfo;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Strings;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.common.TimeUtil;
+import com.google.gerrit.extensions.api.changes.DeleteChangeMessageInput;
+import com.google.gerrit.extensions.common.ChangeMessageInfo;
+import com.google.gerrit.extensions.common.Input;
+import com.google.gerrit.extensions.restapi.Response;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.ChangeMessage;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.ChangeMessagesUtil;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.account.AccountLoader;
+import com.google.gerrit.server.change.ChangeMessageResource;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.permissions.GlobalPermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.update.BatchUpdate;
+import com.google.gerrit.server.update.BatchUpdateOp;
+import com.google.gerrit.server.update.ChangeContext;
+import com.google.gerrit.server.update.RetryHelper;
+import com.google.gerrit.server.update.RetryingRestModifyView;
+import com.google.gerrit.server.update.UpdateException;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+import java.io.IOException;
+import java.util.List;
+
+/** Deletes a change message by rewriting history. */
+@Singleton
+public class DeleteChangeMessage
+ extends RetryingRestModifyView<
+ ChangeMessageResource, DeleteChangeMessageInput, Response<ChangeMessageInfo>> {
+
+ private final Provider<CurrentUser> userProvider;
+ private final Provider<ReviewDb> dbProvider;
+ private final PermissionBackend permissionBackend;
+ private final ChangeMessagesUtil changeMessagesUtil;
+ private final AccountLoader.Factory accountLoaderFactory;
+ private final ChangeNotes.Factory notesFactory;
+
+ @Inject
+ public DeleteChangeMessage(
+ Provider<CurrentUser> userProvider,
+ Provider<ReviewDb> dbProvider,
+ PermissionBackend permissionBackend,
+ ChangeMessagesUtil changeMessagesUtil,
+ AccountLoader.Factory accountLoaderFactory,
+ ChangeNotes.Factory notesFactory,
+ RetryHelper retryHelper) {
+ super(retryHelper);
+ this.userProvider = userProvider;
+ this.dbProvider = dbProvider;
+ this.permissionBackend = permissionBackend;
+ this.changeMessagesUtil = changeMessagesUtil;
+ this.accountLoaderFactory = accountLoaderFactory;
+ this.notesFactory = notesFactory;
+ }
+
+ @Override
+ public Response<ChangeMessageInfo> applyImpl(
+ BatchUpdate.Factory updateFactory,
+ ChangeMessageResource resource,
+ DeleteChangeMessageInput input)
+ throws RestApiException, PermissionBackendException, OrmException, UpdateException,
+ IOException {
+ CurrentUser user = userProvider.get();
+ permissionBackend.user(user).check(GlobalPermission.ADMINISTRATE_SERVER);
+
+ String newChangeMessage =
+ createNewChangeMessage(user.asIdentifiedUser().getName(), input.reason);
+ DeleteChangeMessageOp deleteChangeMessageOp =
+ new DeleteChangeMessageOp(resource.getChangeMessageIndex(), newChangeMessage);
+ try (BatchUpdate batchUpdate =
+ updateFactory.create(
+ dbProvider.get(), resource.getChangeResource().getProject(), user, TimeUtil.nowTs())) {
+ batchUpdate.addOp(resource.getChangeId(), deleteChangeMessageOp).execute();
+ }
+
+ ChangeMessageInfo updatedMessageInfo =
+ createUpdatedChangeMessageInfo(resource.getChangeId(), resource.getChangeMessageIndex());
+ return Response.created(updatedMessageInfo);
+ }
+
+ private ChangeMessageInfo createUpdatedChangeMessageInfo(Change.Id id, int targetIdx)
+ throws OrmException, PermissionBackendException {
+ List<ChangeMessage> messages =
+ changeMessagesUtil.byChange(dbProvider.get(), notesFactory.createChecked(id));
+ ChangeMessage updatedChangeMessage = messages.get(targetIdx);
+ AccountLoader accountLoader = accountLoaderFactory.create(true);
+ ChangeMessageInfo info = createChangeMessageInfo(updatedChangeMessage, accountLoader);
+ accountLoader.fill();
+ return info;
+ }
+
+ @VisibleForTesting
+ public static String createNewChangeMessage(String deletedBy, @Nullable String deletedReason) {
+ checkNotNull(deletedBy, "user name must not be null");
+
+ if (Strings.isNullOrEmpty(deletedReason)) {
+ return createNewChangeMessage(deletedBy);
+ }
+ return String.format("Change message removed by: %s\nReason: %s", deletedBy, deletedReason);
+ }
+
+ @VisibleForTesting
+ public static String createNewChangeMessage(String deletedBy) {
+ checkNotNull(deletedBy, "user name must not be null");
+
+ return "Change message removed by: " + deletedBy;
+ }
+
+ private class DeleteChangeMessageOp implements BatchUpdateOp {
+ private final int targetMessageIdx;
+ private final String newMessage;
+
+ DeleteChangeMessageOp(int targetMessageIdx, String newMessage) {
+ this.targetMessageIdx = targetMessageIdx;
+ this.newMessage = newMessage;
+ }
+
+ @Override
+ public boolean updateChange(ChangeContext ctx) throws OrmException {
+ PatchSet.Id psId = ctx.getChange().currentPatchSetId();
+ changeMessagesUtil.replaceChangeMessage(
+ ctx.getDb(), ctx.getUpdate(psId), targetMessageIdx, newMessage);
+ return true;
+ }
+ }
+
+ @Singleton
+ public static class DefaultDeleteChangeMessage
+ extends RetryingRestModifyView<ChangeMessageResource, Input, Response<ChangeMessageInfo>> {
+ private final DeleteChangeMessage deleteChangeMessage;
+
+ @Inject
+ public DefaultDeleteChangeMessage(
+ DeleteChangeMessage deleteChangeMessage, RetryHelper retryHelper) {
+ super(retryHelper);
+ this.deleteChangeMessage = deleteChangeMessage;
+ }
+
+ @Override
+ protected Response<ChangeMessageInfo> applyImpl(
+ BatchUpdate.Factory updateFactory, ChangeMessageResource resource, Input input)
+ throws Exception {
+ return deleteChangeMessage.applyImpl(updateFactory, resource, new DeleteChangeMessageInput());
+ }
+ }
+}
diff --git a/java/com/google/gerrit/server/restapi/change/Module.java b/java/com/google/gerrit/server/restapi/change/Module.java
index 5b28971..5a5d2bb 100644
--- a/java/com/google/gerrit/server/restapi/change/Module.java
+++ b/java/com/google/gerrit/server/restapi/change/Module.java
@@ -182,6 +182,8 @@
child(CHANGE_KIND, "messages").to(ChangeMessages.class);
get(CHANGE_MESSAGE_KIND).to(GetChangeMessage.class);
+ delete(CHANGE_MESSAGE_KIND).to(DeleteChangeMessage.DefaultDeleteChangeMessage.class);
+ post(CHANGE_MESSAGE_KIND, "delete").to(DeleteChangeMessage.class);
factory(AccountLoader.Factory.class);
factory(ChangeInserter.Factory.class);
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeMessagesIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeMessagesIT.java
index d0b01b7..790b884 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeMessagesIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeMessagesIT.java
@@ -11,26 +11,42 @@
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
-
package com.google.gerrit.acceptance.rest.change;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.PushOneCommit.FILE_NAME;
import static com.google.gerrit.extensions.client.ListChangesOption.MESSAGES;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+import static com.google.gerrit.server.notedb.ChangeNoteUtil.parseCommitMessageRange;
+import static com.google.gerrit.server.restapi.change.DeleteChangeMessage.createNewChangeMessage;
import static java.util.concurrent.TimeUnit.SECONDS;
+import static java.util.stream.Collectors.toSet;
+import static org.eclipse.jgit.util.RawParseUtils.decode;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.common.data.GlobalCapability;
+import com.google.gerrit.extensions.api.changes.DeleteChangeMessageInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
+import com.google.gerrit.extensions.common.CommentInfo;
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.server.notedb.ChangeNoteUtil;
import com.google.gerrit.testing.ConfigSuite;
import com.google.gerrit.testing.TestTimeUtil;
+import java.nio.charset.Charset;
import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;
+import java.util.Optional;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.util.RawParseUtils;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
@@ -114,14 +130,85 @@
public void getOneChangeMessage() throws Exception {
int changeNum = createOneChangeWithMultipleChangeMessagesInHistory();
List<ChangeMessageInfo> messages = new ArrayList<>(gApi.changes().id(changeNum).get().messages);
-
for (ChangeMessageInfo messageInfo : messages) {
String id = messageInfo.id;
assertThat(gApi.changes().id(changeNum).message(id).get()).isEqualTo(messageInfo);
}
}
+ @Test
+ public void deleteCannotBeAppliedWithoutAdministrateServerCapability() throws Exception {
+ int changeNum = createOneChangeWithMultipleChangeMessagesInHistory();
+ setApiUser(user);
+
+ try {
+ deleteOneChangeMessage(changeNum, 0, user, "spam");
+ fail("expected AuthException");
+ } catch (AuthException e) {
+ assertThat(e.getMessage()).isEqualTo("administrate server not permitted");
+ }
+ }
+
+ @Test
+ public void deleteCanBeAppliedWithAdministrateServerCapability() throws Exception {
+ allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ADMINISTRATE_SERVER);
+ int changeNum = createOneChangeWithMultipleChangeMessagesInHistory();
+ setApiUser(user);
+ deleteOneChangeMessage(changeNum, 0, user, "spam");
+ }
+
+ @Test
+ public void deleteCannotBeAppliedWithEmptyChangeMessageUuid() throws Exception {
+ String changeId = createChange().getChangeId();
+
+ try {
+ gApi.changes().id(changeId).message("").delete(new DeleteChangeMessageInput("spam"));
+ fail("expected ResourceNotFoundException");
+ } catch (ResourceNotFoundException e) {
+ assertThat(e.getMessage()).isEqualTo("change message not found");
+ }
+ }
+
+ @Test
+ public void deleteCannotBeAppliedWithNonExistingChangeMessageUuid() throws Exception {
+ String changeId = createChange().getChangeId();
+ DeleteChangeMessageInput input = new DeleteChangeMessageInput();
+ String id = "8473b95934b5732ac55d26311a706c9c2bde9941";
+ input.reason = "spam";
+
+ try {
+ gApi.changes().id(changeId).message(id).delete(input);
+ fail("expected ResourceNotFoundException");
+ } catch (ResourceNotFoundException e) {
+ assertThat(e.getMessage()).isEqualTo(String.format("change message %s not found", id));
+ }
+ }
+
+ @Test
+ public void deleteCanBeAppliedWithoutProvidingReason() throws Exception {
+ int changeNum = createOneChangeWithMultipleChangeMessagesInHistory();
+ deleteOneChangeMessage(changeNum, 2, admin, "");
+ }
+
+ @Test
+ public void deleteOneChangeMessageTwice() throws Exception {
+ int changeNum = createOneChangeWithMultipleChangeMessagesInHistory();
+ // Deletes the second change message twice.
+ deleteOneChangeMessage(changeNum, 1, admin, "reason 1");
+ deleteOneChangeMessage(changeNum, 1, admin, "reason 2");
+ }
+
+ @Test
+ public void deleteMultipleChangeMessages() throws Exception {
+ int changeNum = createOneChangeWithMultipleChangeMessagesInHistory();
+ for (int i = 0; i < 7; ++i) {
+ deleteOneChangeMessage(changeNum, i, admin, "reason " + i);
+ }
+ }
+
private int createOneChangeWithMultipleChangeMessagesInHistory() throws Exception {
+ // Creates the following commit history on the meta branch of the test change.
+
setApiUser(user);
// Commit 1: create a change.
PushOneCommit.Result result = createChange();
@@ -158,6 +245,150 @@
gApi.changes().id(changeId).current().review(reviewInput);
}
+ private void deleteOneChangeMessage(
+ int changeNum, int deletedMessageIndex, TestAccount deletedBy, String reason)
+ throws Exception {
+ List<ChangeMessageInfo> messagesBeforeDeletion = gApi.changes().id(changeNum).messages();
+
+ List<CommentInfo> commentsBefore = getChangeSortedComments(changeNum);
+ List<RevCommit> commitsBefore = new ArrayList<>();
+ if (notesMigration.readChanges()) {
+ commitsBefore = getChangeMetaCommitsInReverseOrder(new Change.Id(changeNum));
+ }
+
+ String id = messagesBeforeDeletion.get(deletedMessageIndex).id;
+ DeleteChangeMessageInput input = new DeleteChangeMessageInput(reason);
+ ChangeMessageInfo info = gApi.changes().id(changeNum).message(id).delete(input);
+
+ // Verify the return change message info is as expect.
+ assertThat(info.message).isEqualTo(createNewChangeMessage(deletedBy.fullName, reason));
+ List<ChangeMessageInfo> messagesAfterDeletion = gApi.changes().id(changeNum).messages();
+ assertMessagesAfterDeletion(
+ messagesBeforeDeletion, messagesAfterDeletion, deletedMessageIndex, deletedBy, reason);
+ assertCommentsAfterDeletion(changeNum, commentsBefore);
+
+ // Verify change index is updated after deletion.
+ List<ChangeInfo> changes = gApi.changes().query("message removed").get();
+ assertThat(changes.stream().map(c -> c._number).collect(toSet())).contains(changeNum);
+
+ // Verifies states of commits if NoteDb is on.
+ if (notesMigration.readChanges()) {
+ assertMetaCommitsAfterDeletion(
+ commitsBefore, changeNum, deletedMessageIndex, deletedBy, reason);
+ }
+ }
+
+ private void assertMessagesAfterDeletion(
+ List<ChangeMessageInfo> messagesBeforeDeletion,
+ List<ChangeMessageInfo> messagesAfterDeletion,
+ int deletedMessageIndex,
+ TestAccount deletedBy,
+ String deleteReason) {
+ assertThat(messagesAfterDeletion)
+ .named("after: %s; before: %s", messagesAfterDeletion, messagesBeforeDeletion)
+ .hasSize(messagesBeforeDeletion.size());
+
+ for (int i = 0; i < messagesAfterDeletion.size(); ++i) {
+ ChangeMessageInfo before = messagesBeforeDeletion.get(i);
+ ChangeMessageInfo after = messagesAfterDeletion.get(i);
+
+ if (i < deletedMessageIndex) {
+ // The uuid of a commit message will be updated after rewriting.
+ assertThat(after.id).isEqualTo(before.id);
+ }
+
+ assertThat(after.tag).isEqualTo(before.tag);
+ assertThat(after.author).isEqualTo(before.author);
+ assertThat(after.realAuthor).isEqualTo(before.realAuthor);
+ assertThat(after._revisionNumber).isEqualTo(before._revisionNumber);
+
+ if (i == deletedMessageIndex) {
+ assertThat(after.message)
+ .isEqualTo(createNewChangeMessage(deletedBy.fullName, deleteReason));
+ } else {
+ assertThat(after.message).isEqualTo(before.message);
+ }
+ }
+ }
+
+ private void assertMetaCommitsAfterDeletion(
+ List<RevCommit> commitsBeforeDeletion,
+ int changeNum,
+ int deletedMessageIndex,
+ TestAccount deletedBy,
+ String deleteReason)
+ throws Exception {
+ List<RevCommit> commitsAfterDeletion =
+ getChangeMetaCommitsInReverseOrder(new Change.Id(changeNum));
+ assertThat(commitsAfterDeletion).hasSize(commitsBeforeDeletion.size());
+
+ for (int i = 0; i < commitsBeforeDeletion.size(); i++) {
+ RevCommit commitBefore = commitsBeforeDeletion.get(i);
+ RevCommit commitAfter = commitsAfterDeletion.get(i);
+ if (i == deletedMessageIndex) {
+ byte[] rawBefore = commitBefore.getRawBuffer();
+ byte[] rawAfter = commitAfter.getRawBuffer();
+ Charset encodingBefore = RawParseUtils.parseEncoding(rawBefore);
+ Charset encodingAfter = RawParseUtils.parseEncoding(rawAfter);
+ Optional<ChangeNoteUtil.CommitMessageRange> rangeBefore =
+ parseCommitMessageRange(commitBefore);
+ Optional<ChangeNoteUtil.CommitMessageRange> rangeAfter =
+ parseCommitMessageRange(commitAfter);
+ assertThat(rangeBefore.isPresent()).isTrue();
+ assertThat(rangeAfter.isPresent()).isTrue();
+
+ String subjectBefore =
+ decode(
+ encodingBefore,
+ rawBefore,
+ rangeBefore.get().subjectStart(),
+ rangeBefore.get().subjectEnd());
+ String subjectAfter =
+ decode(
+ encodingAfter,
+ rawAfter,
+ rangeAfter.get().subjectStart(),
+ rangeAfter.get().subjectEnd());
+ assertThat(subjectBefore).isEqualTo(subjectAfter);
+
+ String footersBefore =
+ decode(
+ encodingBefore,
+ rawBefore,
+ rangeBefore.get().changeMessageEnd() + 1,
+ rawBefore.length);
+ String footersAfter =
+ decode(
+ encodingAfter, rawAfter, rangeAfter.get().changeMessageEnd() + 1, rawAfter.length);
+ assertThat(footersBefore).isEqualTo(footersAfter);
+
+ String message =
+ decode(
+ encodingAfter,
+ rawAfter,
+ rangeAfter.get().changeMessageStart(),
+ rangeAfter.get().changeMessageEnd() + 1);
+ assertThat(message).isEqualTo(createNewChangeMessage(deletedBy.fullName, deleteReason));
+ } else {
+ assertThat(commitAfter.getFullMessage()).isEqualTo(commitBefore.getFullMessage());
+ }
+
+ assertThat(commitAfter.getCommitterIdent().getName())
+ .isEqualTo(commitBefore.getCommitterIdent().getName());
+ assertThat(commitAfter.getAuthorIdent().getName())
+ .isEqualTo(commitBefore.getAuthorIdent().getName());
+ assertThat(commitAfter.getEncoding()).isEqualTo(commitBefore.getEncoding());
+ assertThat(commitAfter.getEncodingName()).isEqualTo(commitBefore.getEncodingName());
+ }
+ }
+
+ /** Verifies comments are not changed after deleting change message(s). */
+ private void assertCommentsAfterDeletion(int changeNum, List<CommentInfo> commentsBeforeDeletion)
+ throws Exception {
+ List<CommentInfo> commentsAfterDeletion = getChangeSortedComments(changeNum);
+ assertThat(commentsAfterDeletion).containsExactlyElementsIn(commentsBeforeDeletion).inOrder();
+ }
+
private static void assertMessage(String expected, String actual) {
assertThat(actual).isEqualTo("Patch Set 1:\n\n" + expected);
}
diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
index 211af59..8b1f4bc 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
@@ -47,7 +47,6 @@
import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Patch;
-import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.notedb.ChangeNoteUtil;
@@ -58,26 +57,21 @@
import com.google.gerrit.testing.FakeEmailSender.Message;
import com.google.inject.Inject;
import com.google.inject.Provider;
-import java.io.IOException;
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
-import java.util.Comparator;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
-import java.util.Map.Entry;
import java.util.Optional;
import java.util.function.Supplier;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import org.eclipse.jgit.lib.ObjectReader;
-import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.notes.NoteMap;
import org.eclipse.jgit.revwalk.RevCommit;
-import org.eclipse.jgit.revwalk.RevWalk;
import org.junit.Before;
import org.junit.Test;
@@ -838,7 +832,7 @@
CommentInput c9 = newComment("b.txt", "comment 9");
addComments(changeId, ps2, c9);
- List<CommentInfo> commentsBeforeDelete = getChangeSortedComments(changeId);
+ List<CommentInfo> commentsBeforeDelete = getChangeSortedComments(id.get());
assertThat(commentsBeforeDelete).hasSize(9);
// PS1 has comments [c1, c2, c3, c4, c5].
assertThat(getRevisionComments(changeId, ps1)).hasSize(5);
@@ -853,7 +847,7 @@
for (int i = 0; i < commentsBeforeDelete.size(); i++) {
List<RevCommit> commitsBeforeDelete = new ArrayList<>();
if (notesMigration.commitChangeWrites()) {
- commitsBeforeDelete = getCommits(id);
+ commitsBeforeDelete = getChangeMetaCommitsInReverseOrder(id);
}
CommentInfo comment = commentsBeforeDelete.get(i);
@@ -879,7 +873,7 @@
comment.message = expectedMsg;
commentsBeforeDelete.set(i, comment);
- List<CommentInfo> commentsAfterDelete = getChangeSortedComments(changeId);
+ List<CommentInfo> commentsAfterDelete = getChangeSortedComments(id.get());
assertThat(commentsAfterDelete).isEqualTo(commentsBeforeDelete);
}
@@ -893,7 +887,7 @@
addComments(changeId, ps3, c12);
addComments(changeId, ps4, c13);
- assertThat(getChangeSortedComments(changeId)).hasSize(13);
+ assertThat(getChangeSortedComments(id.get())).hasSize(13);
assertThat(getRevisionComments(changeId, ps1)).hasSize(6);
assertThat(getRevisionComments(changeId, ps2)).hasSize(3);
assertThat(getRevisionComments(changeId, ps3)).hasSize(1);
@@ -914,7 +908,7 @@
addComments(changeId, ps1, c2);
addComments(changeId, ps1, c3);
- List<CommentInfo> commentsBeforeDelete = getChangeSortedComments(changeId);
+ List<CommentInfo> commentsBeforeDelete = getChangeSortedComments(id.get());
assertThat(commentsBeforeDelete).hasSize(3);
Optional<CommentInfo> targetComment =
commentsBeforeDelete.stream().filter(c -> c.message.equals("comment 2")).findFirst();
@@ -924,7 +918,7 @@
List<RevCommit> commitsBeforeDelete = new ArrayList<>();
if (notesMigration.commitChangeWrites()) {
- commitsBeforeDelete = getCommits(id);
+ commitsBeforeDelete = getChangeMetaCommitsInReverseOrder(id);
}
setApiUser(admin);
@@ -944,7 +938,7 @@
if (notesMigration.commitChangeWrites()) {
assertMetaBranchCommitsAfterRewriting(commitsBeforeDelete, id, uuid, expectedMsg);
}
- assertThat(getChangeSortedComments(changeId)).hasSize(3);
+ assertThat(getChangeSortedComments(id.get())).hasSize(3);
}
@Test
@@ -962,19 +956,6 @@
assertThat(comment.legacyFormat).isFalse();
}
- private List<CommentInfo> getChangeSortedComments(String changeId) throws Exception {
- List<CommentInfo> comments = new ArrayList<>();
- Map<String, List<CommentInfo>> commentsMap = getPublishedComments(changeId);
- for (Entry<String, List<CommentInfo>> e : commentsMap.entrySet()) {
- for (CommentInfo c : e.getValue()) {
- c.path = e.getKey(); // Set the comment's path field.
- comments.add(c);
- }
- }
- comments.sort(Comparator.comparing(c -> c.id));
- return comments;
- }
-
private List<CommentInfo> getRevisionComments(String changeId, String revId) throws Exception {
return getPublishedComments(changeId, revId)
.values()
@@ -998,15 +979,6 @@
gApi.changes().id(changeId).revision(revision).review(input);
}
- private List<RevCommit> getCommits(Change.Id changeId) throws IOException {
- try (Repository repo = repoManager.openRepository(project);
- RevWalk revWalk = new RevWalk(repo)) {
- Ref metaRef = repo.exactRef(RefNames.changeMetaRef(changeId));
- revWalk.markStart(revWalk.parseCommit(metaRef.getObjectId()));
- return Lists.newArrayList(revWalk);
- }
- }
-
/**
* All the commits, which contain the target comment before, should still contain the comment with
* the updated message. All the other metas of the commits should be exactly the same.
@@ -1017,7 +989,7 @@
String targetCommentUuid,
String expectedMessage)
throws Exception {
- List<RevCommit> afterDelete = getCommits(changeId);
+ List<RevCommit> afterDelete = getChangeMetaCommitsInReverseOrder(changeId);
assertThat(afterDelete).hasSize(beforeDelete.size());
try (Repository repo = repoManager.openRepository(project);
@@ -1119,10 +1091,6 @@
return gApi.changes().id(changeId).revision(revId).drafts();
}
- private Map<String, List<CommentInfo>> getPublishedComments(String changeId) throws Exception {
- return gApi.changes().id(changeId).comments();
- }
-
private CommentInfo getDraftComment(String changeId, String revId, String uuid) throws Exception {
return gApi.changes().id(changeId).revision(revId).draft(uuid).get();
}
