Merge "Split comments and new PS info with new PS upload"

java/com/google/gerrit/server/PublishCommentUtil.java

@@ -28,6 +28,7 @@ import com.google.gerrit.extensions.validators.CommentForValidation;
 import com.google.gerrit.extensions.validators.CommentValidationFailure;
 import com.google.gerrit.extensions.validators.CommentValidator;
 import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.patch.PatchListCache;
 import com.google.gerrit.server.patch.PatchListNotAvailableException;
 import com.google.gerrit.server.plugincontext.PluginSetContext;
@@ -57,7 +58,7 @@ public class PublishCommentUtil {
 
   public void publish(
       ChangeContext ctx,
-      PatchSet.Id psId,
+      ChangeUpdate changeUpdate,
       Collection<Comment> draftComments,
       @Nullable String tag) {
     ChangeNotes notes = ctx.getNotes();
@@ -107,7 +108,7 @@ public class PublishCommentUtil {
       }
       commentsToPublish.add(draftComment);
     }
-    commentsUtil.putComments(ctx.getUpdate(psId), Status.PUBLISHED, commentsToPublish);
+    commentsUtil.putComments(changeUpdate, Status.PUBLISHED, commentsToPublish);
   }
 
   private static PatchSet.Id psId(ChangeNotes notes, Comment c) {

java/com/google/gerrit/server/PublishCommentsOp.java

@@ -0,0 +1,151 @@
+// Copyright (C) 2020 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;
+
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.entities.ChangeMessage;
+import com.google.gerrit.entities.Comment;
+import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
+import com.google.gerrit.server.change.EmailReviewComments;
+import com.google.gerrit.server.change.NotifyResolver;
+import com.google.gerrit.server.extensions.events.CommentAdded;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.notedb.ChangeUpdate;
+import com.google.gerrit.server.patch.PatchListNotAvailableException;
+import com.google.gerrit.server.update.BatchUpdateOp;
+import com.google.gerrit.server.update.ChangeContext;
+import com.google.gerrit.server.update.CommentsRejectedException;
+import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.util.LabelVote;
+import com.google.inject.Inject;
+import com.google.inject.assistedinject.Assisted;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * A {@link BatchUpdateOp} that can be used to publish draft comments
+ *
+ * <p>This class uses the {@link PublishCommentUtil} to publish draft comments and fires the
+ * necessary event for this.
+ */
+public class PublishCommentsOp implements BatchUpdateOp {
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+  private final PatchSetUtil psUtil;
+  private final ChangeNotes.Factory changeNotesFactory;
+  private final ChangeMessagesUtil cmUtil;
+  private final CommentAdded commentAdded;
+  private final CommentsUtil commentsUtil;
+  private final EmailReviewComments.Factory email;
+  private final List<LabelVote> labelDelta = new ArrayList<>();
+  private final Project.NameKey projectNameKey;
+  private final PatchSet.Id psId;
+  private final PublishCommentUtil publishCommentUtil;
+
+  private List<Comment> comments = new ArrayList<>();
+  private ChangeMessage message;
+  private IdentifiedUser user;
+
+  public interface Factory {
+    PublishCommentsOp create(PatchSet.Id psId, Project.NameKey projectNameKey);
+  }
+
+  @Inject
+  public PublishCommentsOp(
+      ChangeNotes.Factory changeNotesFactory,
+      ChangeMessagesUtil cmUtil,
+      CommentAdded commentAdded,
+      CommentsUtil commentsUtil,
+      EmailReviewComments.Factory email,
+      PatchSetUtil psUtil,
+      PublishCommentUtil publishCommentUtil,
+      @Assisted PatchSet.Id psId,
+      @Assisted Project.NameKey projectNameKey) {
+    this.cmUtil = cmUtil;
+    this.changeNotesFactory = changeNotesFactory;
+    this.commentAdded = commentAdded;
+    this.commentsUtil = commentsUtil;
+    this.email = email;
+    this.psId = psId;
+    this.publishCommentUtil = publishCommentUtil;
+    this.psUtil = psUtil;
+    this.projectNameKey = projectNameKey;
+  }
+
+  @Override
+  public boolean updateChange(ChangeContext ctx)
+      throws ResourceConflictException, UnprocessableEntityException, IOException,
+          PatchListNotAvailableException, CommentsRejectedException {
+    user = ctx.getIdentifiedUser();
+    comments = commentsUtil.draftByChangeAuthor(ctx.getNotes(), ctx.getUser().getAccountId());
+
+    // PublishCommentsOp should update a separate ChangeUpdate Object than the one used by other ops
+    // For example, with the "publish comments on PS upload" workflow,
+    // There are 2 ops: ReplaceOp & PublishCommentsOp, where each updates its own ChangeUpdate
+    // This is required since
+    //   1. a ChangeUpdate has only 1 change message
+    //   2. Each ChangeUpdate results in 1 commit in NoteDb
+    // We do it this way so that the execution results in 2 different commits in NoteDb
+    ChangeUpdate changeUpdate = ctx.getDistinctUpdate(psId);
+    publishCommentUtil.publish(ctx, changeUpdate, comments, null);
+    return insertMessage(ctx, changeUpdate);
+  }
+
+  @Override
+  public void postUpdate(Context ctx) {
+    if (message == null || comments.isEmpty()) {
+      return;
+    }
+    ChangeNotes changeNotes = changeNotesFactory.createChecked(projectNameKey, psId.changeId());
+    PatchSet ps = psUtil.get(changeNotes, psId);
+    NotifyResolver.Result notify = ctx.getNotify(changeNotes.getChangeId());
+    if (notify.shouldNotify()) {
+      email.create(notify, changeNotes, ps, user, message, comments, null, labelDelta).sendAsync();
+    }
+    try {
+      commentAdded.fire(
+          changeNotes.getChange(),
+          ps,
+          ctx.getAccount(),
+          message.getMessage(),
+          null,
+          null,
+          ctx.getWhen());
+    } catch (Exception e) {
+      logger.atWarning().withCause(e).log("comment-added event invocation failed");
+    }
+  }
+
+  private boolean insertMessage(ChangeContext ctx, ChangeUpdate changeUpdate) {
+    StringBuilder buf = new StringBuilder();
+    if (comments.size() == 1) {
+      buf.append("\n\n(1 comment)");
+    } else if (comments.size() > 1) {
+      buf.append(String.format("\n\n(%d comments)", comments.size()));
+    }
+    if (buf.length() == 0) {
+      return false;
+    }
+    message =
+        ChangeMessagesUtil.newMessage(
+            psId, user, ctx.getWhen(), "Patch Set " + psId.get() + ":" + buf, null);
+    cmUtil.addChangeMessage(changeUpdate, message);
+    return true;
+  }
+}

java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java

@@ -33,6 +33,7 @@ import com.google.gerrit.metrics.Histogram1;
 import com.google.gerrit.metrics.MetricMaker;
 import com.google.gerrit.metrics.Timer1;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.PublishCommentsOp;
 import com.google.gerrit.server.config.ConfigUtil;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.config.ReceiveCommitsExecutor;
@@ -103,6 +104,7 @@ public class AsyncReceiveCommits implements PreReceiveHook {
       // Don't expose the binding for ReceiveCommits.Factory. All callers should
       // be using AsyncReceiveCommits.Factory instead.
       install(new FactoryModuleBuilder().build(ReceiveCommits.Factory.class));
+      install(new FactoryModuleBuilder().build(PublishCommentsOp.Factory.class));
       install(new FactoryModuleBuilder().build(BranchCommitValidator.Factory.class));
     }
 

java/com/google/gerrit/server/git/receive/ReceiveCommits.java

@@ -102,6 +102,7 @@ import com.google.gerrit.server.CreateGroupPermissionSyncer;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.PublishCommentUtil;
+import com.google.gerrit.server.PublishCommentsOp;
 import com.google.gerrit.server.RequestInfo;
 import com.google.gerrit.server.RequestListener;
 import com.google.gerrit.server.account.AccountResolver;
@@ -331,6 +332,7 @@ class ReceiveCommits {
   private final RefOperationValidators.Factory refValidatorsFactory;
   private final ReplaceOp.Factory replaceOpFactory;
   private final PluginSetContext<RequestListener> requestListeners;
+  private final PublishCommentsOp.Factory publishCommentsOp;
   private final RetryHelper retryHelper;
   private final RequestScopePropagator requestScopePropagator;
   private final Sequences seq;
@@ -403,6 +405,7 @@ class ReceiveCommits {
       Provider<InternalChangeQuery> queryProvider,
       Provider<MergeOp> mergeOpProvider,
       Provider<MergeOpRepoManager> ormProvider,
+      PublishCommentsOp.Factory publishCommentsOp,
       ReceiveConfig receiveConfig,
       RefOperationValidators.Factory refValidatorsFactory,
       ReplaceOp.Factory replaceOpFactory,
@@ -449,6 +452,7 @@ class ReceiveCommits {
     this.projectCache = projectCache;
     this.psUtil = psUtil;
     this.performanceLoggers = performanceLoggers;
+    this.publishCommentsOp = publishCommentsOp;
     this.queryProvider = queryProvider;
     this.receiveConfig = receiveConfig;
     this.refValidatorsFactory = refValidatorsFactory;
@@ -905,10 +909,15 @@ class ReceiveCommits {
 
         logger.atFine().log("Adding %d replace requests", newChanges.size());
         for (ReplaceRequest replace : replaceByChange.values()) {
+          replace.addOps(bu, replaceProgress);
           if (magicBranch != null) {
             bu.setNotifyHandling(replace.ontoChange, magicBranch.getNotifyHandling(replace.notes));
+            if (magicBranch.shouldPublishComments()) {
+              bu.addOp(
+                  replace.notes.getChangeId(),
+                  publishCommentsOp.create(replace.psId, project.getNameKey()));
+            }
           }
-          replace.addOps(bu, replaceProgress);
         }
 
         logger.atFine().log("Adding %d create requests", newChanges.size());

java/com/google/gerrit/server/git/receive/ReplaceOp.java

@@ -32,7 +32,6 @@ import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.entities.BranchNameKey;
 import com.google.gerrit.entities.Change;
 import com.google.gerrit.entities.ChangeMessage;
-import com.google.gerrit.entities.Comment;
 import com.google.gerrit.entities.PatchSet;
 import com.google.gerrit.entities.PatchSetApproval;
 import com.google.gerrit.entities.PatchSetInfo;
@@ -45,13 +44,10 @@ import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.ChangeMessagesUtil;
-import com.google.gerrit.server.CommentsUtil;
 import com.google.gerrit.server.PatchSetUtil;
-import com.google.gerrit.server.PublishCommentUtil;
 import com.google.gerrit.server.account.AccountResolver;
 import com.google.gerrit.server.change.AddReviewersOp;
 import com.google.gerrit.server.change.ChangeKindCache;
-import com.google.gerrit.server.change.EmailReviewComments;
 import com.google.gerrit.server.change.NotifyResolver;
 import com.google.gerrit.server.change.ReviewerAdder;
 import com.google.gerrit.server.change.ReviewerAdder.InternalAddReviewerInput;
@@ -121,9 +117,6 @@ public class ReplaceOp implements BatchUpdateOp {
   private final ChangeData.Factory changeDataFactory;
   private final ChangeKindCache changeKindCache;
   private final ChangeMessagesUtil cmUtil;
-  private final CommentsUtil commentsUtil;
-  private final PublishCommentUtil publishCommentUtil;
-  private final EmailReviewComments.Factory emailCommentsFactory;
   private final ExecutorService sendEmailExecutor;
   private final RevisionCreated revisionCreated;
   private final CommentAdded commentAdded;
@@ -154,7 +147,6 @@ public class ReplaceOp implements BatchUpdateOp {
   private PatchSet newPatchSet;
   private ChangeKind changeKind;
   private ChangeMessage msg;
-  private List<Comment> comments = ImmutableList.of();
   private String rejectMessage;
   private MergedByPushOp mergedByPushOp;
   private RequestScopePropagator requestScopePropagator;
@@ -168,9 +160,6 @@ public class ReplaceOp implements BatchUpdateOp {
       ChangeData.Factory changeDataFactory,
       ChangeKindCache changeKindCache,
       ChangeMessagesUtil cmUtil,
-      CommentsUtil commentsUtil,
-      PublishCommentUtil publishCommentUtil,
-      EmailReviewComments.Factory emailCommentsFactory,
       RevisionCreated revisionCreated,
       CommentAdded commentAdded,
       MergedByPushOp.Factory mergedByPushOpFactory,
@@ -197,9 +186,6 @@ public class ReplaceOp implements BatchUpdateOp {
     this.changeDataFactory = changeDataFactory;
     this.changeKindCache = changeKindCache;
     this.cmUtil = cmUtil;
-    this.commentsUtil = commentsUtil;
-    this.publishCommentUtil = publishCommentUtil;
-    this.emailCommentsFactory = emailCommentsFactory;
     this.revisionCreated = revisionCreated;
     this.commentAdded = commentAdded;
     this.mergedByPushOpFactory = mergedByPushOpFactory;
@@ -302,13 +288,6 @@ public class ReplaceOp implements BatchUpdateOp {
         change.setWorkInProgress(true);
         update.setWorkInProgress(true);
       }
-      if (shouldPublishComments()) {
-        boolean workInProgress = change.isWorkInProgress();
-        if (magicBranch.workInProgress) {
-          workInProgress = true;
-        }
-        comments = publishComments(ctx, workInProgress);
-      }
     }
 
     newPatchSet =
@@ -416,11 +395,6 @@ public class ReplaceOp implements BatchUpdateOp {
     } else {
       message.append('.');
     }
-    if (comments.size() == 1) {
-      message.append("\n\n(1 comment)");
-    } else if (comments.size() > 1) {
-      message.append(String.format("\n\n(%d comments)", comments.size()));
-    }
     if (!Strings.isNullOrEmpty(reviewMessage)) {
       message.append("\n\n").append(reviewMessage);
     }
@@ -499,14 +473,6 @@ public class ReplaceOp implements BatchUpdateOp {
     change.setKey(Change.key(idList.get(idList.size() - 1).trim()));
   }
 
-  private List<Comment> publishComments(ChangeContext ctx, boolean workInProgress) {
-    List<Comment> comments =
-        commentsUtil.draftByChangeAuthor(ctx.getNotes(), ctx.getUser().getAccountId());
-    publishCommentUtil.publish(
-        ctx, patchSetId, comments, ChangeMessagesUtil.uploadedPatchSetTag(workInProgress));
-    return comments;
-  }
-
   @Override
   public void postUpdate(Context ctx) throws Exception {
     reviewerAdditions.postUpdate(ctx);
@@ -520,25 +486,10 @@ public class ReplaceOp implements BatchUpdateOp {
         e.run();
       }
     }
-
     NotifyResolver.Result notify = ctx.getNotify(notes.getChangeId());
-    if (shouldPublishComments()) {
-      emailCommentsFactory
-          .create(
-              notify,
-              notes,
-              newPatchSet,
-              ctx.getUser().asIdentifiedUser(),
-              msg,
-              comments,
-              msg.getMessage(),
-              ImmutableList.of()) // TODO(dborowitz): Include labels.
-          .sendAsync();
-    }
-
     revisionCreated.fire(notes.getChange(), newPatchSet, ctx.getAccount(), ctx.getWhen(), notify);
     try {
-      fireCommentAddedEvent(ctx);
+      fireApprovalsEvent(ctx);
     } catch (Exception e) {
       logger.atWarning().withCause(e).log("comment-added event invocation failed");
     }
@@ -588,11 +539,10 @@ public class ReplaceOp implements BatchUpdateOp {
     }
   }
 
-  private void fireCommentAddedEvent(Context ctx) throws IOException {
+  private void fireApprovalsEvent(Context ctx) throws IOException {
     if (approvals.isEmpty()) {
       return;
     }
-
     /* For labels that are not set in this operation, show the "current" value
      * of 0, and no oldValue as the value was not modified by this operation.
      * For labels that are set in this operation, the value was modified, so
@@ -612,7 +562,6 @@ public class ReplaceOp implements BatchUpdateOp {
         oldApprovals.put(entry.getKey(), (short) 0);
       }
     }
-
     commentAdded.fire(
         notes.getChange(),
         newPatchSet,
@@ -663,8 +612,4 @@ public class ReplaceOp implements BatchUpdateOp {
       return null;
     }
   }
-
-  private boolean shouldPublishComments() {
-    return magicBranch != null && magicBranch.shouldPublishComments();
-  }
 }

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

@@ -1003,7 +1003,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
         case PUBLISH:
         case PUBLISH_ALL_REVISIONS:
           validateComments(Streams.concat(drafts.values().stream(), toPublish.stream()));
-          publishCommentUtil.publish(ctx, psId, drafts.values(), in.tag);
+          publishCommentUtil.publish(ctx, ctx.getUpdate(psId), drafts.values(), in.tag);
           comments.addAll(drafts.values());
           break;
         case KEEP:

java/com/google/gerrit/server/update/BatchUpdate.java

@@ -23,6 +23,7 @@ import static java.util.Objects.requireNonNull;
 import static java.util.stream.Collectors.toSet;
 
 import com.google.common.base.Throwables;
+import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ListMultimap;
@@ -34,6 +35,7 @@ import com.google.common.util.concurrent.ListenableFuture;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.entities.Change;
 import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.entities.PatchSet.Id;
 import com.google.gerrit.entities.Project;
 import com.google.gerrit.extensions.api.changes.NotifyHandling;
 import com.google.gerrit.extensions.config.FactoryModule;
@@ -256,26 +258,51 @@ public class BatchUpdate implements AutoCloseable {
 
   private class ChangeContextImpl extends ContextImpl implements ChangeContext {
     private final ChangeNotes notes;
-    private final Map<PatchSet.Id, ChangeUpdate> updates;
+
+    /**
+     * Updates where the caller instructed us to create one NoteDb commit per update. Keyed by
+     * PatchSet.Id only for convenience.
+     */
+    private final Map<PatchSet.Id, ChangeUpdate> defaultUpdates;
+
+    /**
+     * Updates where the caller allowed us to combine potentially multiple adjustments into a single
+     * commit in NoteDb by re-using the same ChangeUpdate instance. Will still be one commit per
+     * patch set.
+     */
+    private final ListMultimap<Id, ChangeUpdate> distinctUpdates;
 
     private boolean deleted;
 
     ChangeContextImpl(ChangeNotes notes) {
       this.notes = requireNonNull(notes);
-      updates = new TreeMap<>(comparing(PatchSet.Id::get));
+      defaultUpdates = new TreeMap<>(comparing(PatchSet.Id::get));
+      distinctUpdates = ArrayListMultimap.create();
     }
 
     @Override
     public ChangeUpdate getUpdate(PatchSet.Id psId) {
-      ChangeUpdate u = updates.get(psId);
+      ChangeUpdate u = defaultUpdates.get(psId);
       if (u == null) {
-        u = changeUpdateFactory.create(notes, user, when);
+        u = getNewChangeUpdate(psId);
+        defaultUpdates.put(psId, u);
+      }
+      return u;
+    }
+
+    @Override
+    public ChangeUpdate getDistinctUpdate(PatchSet.Id psId) {
+      ChangeUpdate u = getNewChangeUpdate(psId);
+      distinctUpdates.put(psId, u);
+      return u;
+    }
+
+    private ChangeUpdate getNewChangeUpdate(PatchSet.Id psId) {
+      ChangeUpdate u = changeUpdateFactory.create(notes, user, when);
       if (newChanges.containsKey(notes.getChangeId())) {
         u.setAllowWriteToNewRef(true);
       }
       u.setPatchSetId(psId);
-        updates.put(psId, u);
-      }
       return u;
     }
 
@@ -571,7 +598,8 @@ public class BatchUpdate implements AutoCloseable {
         handle.setResult(id, ChangeResult.SKIPPED);
         continue;
       }
-      ctx.updates.values().forEach(handle.manager::add);
+      ctx.defaultUpdates.values().forEach(handle.manager::add);
+      ctx.distinctUpdates.values().forEach(handle.manager::add);
       if (ctx.deleted) {
         logDebug("Change %s was deleted", id);
         handle.manager.deleteChange(id);

java/com/google/gerrit/server/update/ChangeContext.java

@@ -30,7 +30,7 @@ import com.google.gerrit.server.notedb.ChangeUpdate;
  */
 public interface ChangeContext extends Context {
   /**
-   * Get an update for this change at a given patch set.
+   * Get the first update for this change at a given patch set.
    *
    * <p>A single operation can modify changes at different patch sets. Commits in the NoteDb graph
    * within this update are created in patch set order.
@@ -42,6 +42,16 @@ public interface ChangeContext extends Context {
    */
   ChangeUpdate getUpdate(PatchSet.Id psId);
 
+  /**
+   * Gets a new ChangeUpdate for this change at a given patch set.
+   *
+   * <p>To get the current patch set ID, use {@link com.google.gerrit.server.PatchSetUtil#current}.
+   *
+   * @param psId patch set ID.
+   * @return handle for change updates.
+   */
+  ChangeUpdate getDistinctUpdate(PatchSet.Id psId);
+
   /**
    * Get the up-to-date notes for this change.
    *

javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java

@@ -115,7 +115,6 @@ import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
-import java.util.regex.Pattern;
 import java.util.stream.Stream;
 import org.eclipse.jgit.api.errors.GitAPIException;
 import org.eclipse.jgit.junit.TestRepository;
@@ -2043,36 +2042,57 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
     assertThat(comments.stream().map(c -> c.id)).containsExactly(c1.id, c2.id, c3.id);
     assertThat(comments.stream().map(c -> c.message))
         .containsExactly("comment1", "comment2", "comment3");
-    assertThat(getLastMessage(r.getChangeId())).isEqualTo("Uploaded patch set 3.\n\n(3 comments)");
 
-    List<String> messages =
+    /* Assert the correctness of the API messages */
+    List<ChangeMessageInfo> allMessages = getMessages(r.getChangeId());
+    List<String> messagesText = allMessages.stream().map(m -> m.message).collect(toList());
+    assertThat(messagesText)
+        .containsExactly(
+            "Uploaded patch set 1.",
+            "Uploaded patch set 2.",
+            "Uploaded patch set 3.",
+            "Patch Set 3:\n\n(3 comments)")
+        .inOrder();
+
+    /* Assert the tags - PS#2 comments do not have tags, PS#3 upload is autogenerated */
+    List<String> messagesTags = allMessages.stream().map(m -> m.tag).collect(toList());
+    ;
+    assertThat(messagesTags.get(2)).isEqualTo("autogenerated:gerrit:newPatchSet");
+    assertThat(messagesTags.get(3)).isNull();
+
+    /* Assert the correctness of the emails sent */
+    List<String> emailMessages =
         sender.getMessages().stream()
             .map(Message::body)
             .sorted(Comparator.comparingInt(m -> m.contains("reexamine") ? 0 : 1))
             .collect(toList());
-    assertThat(messages).hasSize(2);
+    assertThat(emailMessages).hasSize(2);
 
-    assertThat(messages.get(0)).contains("Gerrit-MessageType: newpatchset");
+    assertThat(emailMessages.get(0)).contains("Gerrit-MessageType: newpatchset");
-    assertThat(messages.get(0)).contains("I'd like you to reexamine a change");
+    assertThat(emailMessages.get(0)).contains("I'd like you to reexamine a change");
-    assertThat(messages.get(0)).doesNotContain("Uploaded patch set 3");
+    assertThat(emailMessages.get(0)).doesNotContain("Uploaded patch set 3");
 
-    assertThat(messages.get(1)).contains("Gerrit-MessageType: comment");
+    assertThat(emailMessages.get(1)).contains("Gerrit-MessageType: comment");
-    assertThat(messages.get(1))
+    assertThat(emailMessages.get(1)).contains("Patch Set 3:\n\n(3 comments)");
-        .containsMatch(
+    assertThat(emailMessages.get(1)).contains("PS1, Line 1:");
-            Pattern.compile(
+    assertThat(emailMessages.get(1)).contains("PS2, Line 1:");
-                // A little weird that the comment email contains this text, but it's actually
+
-                // what's in the ChangeMessage. Really we should fuse the emails into one, but until
+    /* Assert the correctness of the NoteDb change meta commits */
-                // then, this test documents the current behavior.
+    List<RevCommit> commitMessages = getChangeMetaCommitsInReverseOrder(r.getChange().getId());
-                "Uploaded patch set 3\\.\n"
+    assertThat(commitMessages).hasSize(5);
+    assertThat(commitMessages.get(0).getShortMessage()).isEqualTo("Create change");
+    assertThat(commitMessages.get(1).getShortMessage()).isEqualTo("Create patch set 2");
+    assertThat(commitMessages.get(2).getShortMessage()).isEqualTo("Update patch set 2");
+    assertThat(commitMessages.get(3).getShortMessage()).isEqualTo("Create patch set 3");
+    assertThat(commitMessages.get(4).getFullMessage())
+        .isEqualTo(
+            "Update patch set 3\n"
                 + "\n"
-                    + "\\(3 comments\\)\\n.*"
+                + "Patch Set 3:\n"
-                    + "PS1, Line 1:.*"
+                + "\n"
-                    + "comment1\\n.*"
+                + "(3 comments)\n"
-                    + "PS1, Line 1:.*"
+                + "\n"
-                    + "comment2\\n.*"
+                + "Patch-set: 3\n");
-                    + "PS2, Line 1:.*"
-                    + "comment3\\n",
-                Pattern.DOTALL));
   }
 
   @Test
@@ -2085,8 +2105,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
 
     Collection<CommentInfo> comments = getPublishedComments(r.getChangeId());
     assertThat(comments.stream().map(c -> c.message)).containsExactly("comment1");
-    assertThat(getLastMessage(r.getChangeId()))
+    assertThat(getLastMessage(r.getChangeId())).isEqualTo("Patch Set 2:\n" + "\n" + "(1 comment)");
-        .isEqualTo("Uploaded patch set 2.\n\n(1 comment)\n\nThe message");
   }
 
   @Test
@@ -2104,16 +2123,22 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
     amendChanges(initialHead, commits, "refs/for/master%publish-comments");
 
     Collection<CommentInfo> cs1 = getPublishedComments(id1);
+    List<ChangeMessageInfo> messages1 = getMessages(id1);
     assertThat(cs1.stream().map(c -> c.message)).containsExactly("comment1");
     assertThat(cs1.stream().map(c -> c.id)).containsExactly(c1.id);
-    assertThat(getLastMessage(id1))
+    assertThat(messages1.get(0).message).isEqualTo("Uploaded patch set 1.");
-        .isEqualTo("Uploaded patch set 2: Commit message was updated.\n\n(1 comment)");
+    assertThat(messages1.get(1).message)
+        .isEqualTo("Uploaded patch set 2: Commit message was updated.");
+    assertThat(messages1.get(2).message).isEqualTo("Patch Set 2:\n\n(1 comment)");
 
     Collection<CommentInfo> cs2 = getPublishedComments(id2);
+    List<ChangeMessageInfo> messages2 = getMessages(id2);
     assertThat(cs2.stream().map(c -> c.message)).containsExactly("comment2");
     assertThat(cs2.stream().map(c -> c.id)).containsExactly(c2.id);
-    assertThat(getLastMessage(id2))
+    assertThat(messages2.get(0).message).isEqualTo("Uploaded patch set 1.");
-        .isEqualTo("Uploaded patch set 2: Commit message was updated.\n\n(1 comment)");
+    assertThat(messages2.get(1).message)
+        .isEqualTo("Uploaded patch set 2: Commit message was updated.");
+    assertThat(messages2.get(2).message).isEqualTo("Patch Set 2:\n\n(1 comment)");
   }
 
   @Test
@@ -2138,7 +2163,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
     assertThat(cs2.stream().map(c -> c.id)).containsExactly(c2.id);
 
     assertThat(getLastMessage(id1)).doesNotMatch("[Cc]omment");
-    assertThat(getLastMessage(id2)).isEqualTo("Uploaded patch set 2.\n\n(1 comment)");
+    assertThat(getLastMessage(id2)).isEqualTo("Patch Set 2:\n\n(1 comment)");
   }
 
   @Test
@@ -2625,6 +2650,10 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
         .get();
   }
 
+  private List<ChangeMessageInfo> getMessages(String changeId) throws Exception {
+    return gApi.changes().id(changeId).get(MESSAGES).messages.stream().collect(toList());
+  }
+
   private void assertThatUserIsOnlyReviewer(ChangeInfo ci, TestAccount reviewer) {
     assertThat(ci.reviewers).isNotNull();
     assertThat(ci.reviewers.keySet()).containsExactly(ReviewerState.REVIEWER);

javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java

@@ -125,8 +125,10 @@ public class ReceiveCommitsCommentValidationIT extends AbstractDaemonTest {
     assertThat(testCommentHelper.getPublishedComments(result.getChangeId())).isEmpty();
     amendChange(changeId, "refs/for/master%publish-comments", admin, testRepo);
     assertThat(testCommentHelper.getPublishedComments(result.getChangeId())).hasSize(2);
+
     assertThat(capture.getAllValues()).hasSize(1);
-    assertThat(capture.getValue())
+
+    assertThat(capture.getAllValues().get(0))
         .containsExactly(
             CommentForValidation.create(
                 CommentForValidation.CommentType.INLINE_COMMENT, draftInline.message),