Add ReceiveCommits option to publish comments

To keep the options interface simple, the only possibility is
PUBLISH_ALL_REVISIONS.

Change-Id: Ia95974ef485c8c57ceae5b8bd4d9784bfee14e1f

Documentation/user-upload.txt

@@ -262,6 +262,16 @@ option:
 git push refs parameter does not allow spaces.  Use the '_' character instead,
 it will then be applied as "This is a rebase on master".
 
+[[publish-comments]]
+==== Publish Draft Comments
+
+If you have draft comments on the change(s) that are updated by the push, the
+`publish-comments` option will cause them to be published:
+
+----
+  git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/experimental%publish-comments
+----
+
 [[review_labels]]
 ==== Review Labels
 

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java

@@ -15,21 +15,25 @@
 package com.google.gerrit.acceptance.git;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
 import static com.google.common.truth.TruthJUnit.assume;
 import static com.google.gerrit.acceptance.GitUtil.assertPushOk;
 import static com.google.gerrit.acceptance.GitUtil.assertPushRejected;
 import static com.google.gerrit.acceptance.GitUtil.pushHead;
+import static com.google.gerrit.acceptance.PushOneCommit.FILE_NAME;
 import static com.google.gerrit.common.FooterConstants.CHANGE_ID;
 import static com.google.gerrit.extensions.common.EditInfoSubject.assertThat;
 import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
 import static com.google.gerrit.server.project.Util.category;
 import static com.google.gerrit.server.project.Util.value;
 import static java.util.concurrent.TimeUnit.SECONDS;
+import static java.util.stream.Collectors.toList;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Streams;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.GitUtil;
 import com.google.gerrit.acceptance.PushOneCommit;
@@ -37,6 +41,7 @@ import com.google.gerrit.acceptance.TestAccount;
 import com.google.gerrit.acceptance.TestProjectInput;
 import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.common.data.Permission;
+import com.google.gerrit.extensions.api.changes.DraftInput;
 import com.google.gerrit.extensions.api.changes.NotifyHandling;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
 import com.google.gerrit.extensions.api.projects.BranchInput;
@@ -45,8 +50,10 @@ import com.google.gerrit.extensions.client.InheritableBoolean;
 import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.client.ProjectWatchInfo;
 import com.google.gerrit.extensions.client.ReviewerState;
+import com.google.gerrit.extensions.client.Side;
 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.common.EditInfo;
 import com.google.gerrit.extensions.common.LabelInfo;
 import com.google.gerrit.extensions.common.RevisionInfo;
@@ -1469,6 +1476,137 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
     assertThat(info.status).isEqualTo(ChangeStatus.NEW);
   }
 
+  @Test
+  public void publishCommentsOnPushPublishesDraftsOnAllRevisions() throws Exception {
+    PushOneCommit.Result r = createChange();
+    String rev1 = r.getCommit().name();
+    CommentInfo c1 = addDraft(r.getChangeId(), rev1, newDraft(FILE_NAME, 1, "comment1"));
+    CommentInfo c2 = addDraft(r.getChangeId(), rev1, newDraft(FILE_NAME, 1, "comment2"));
+
+    r = amendChange(r.getChangeId());
+    String rev2 = r.getCommit().name();
+    CommentInfo c3 = addDraft(r.getChangeId(), rev2, newDraft(FILE_NAME, 1, "comment3"));
+
+    assertThat(getPublishedComments(r.getChangeId())).isEmpty();
+
+    gApi.changes().id(r.getChangeId()).addReviewer(user.email);
+    sender.clear();
+    amendChange(r.getChangeId(), "refs/for/master%publish-comments");
+
+    Collection<CommentInfo> comments = getPublishedComments(r.getChangeId());
+    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)");
+
+    Message m = Iterables.getLast(sender.getMessages());
+    assertThat(m.body()).contains("I'd like you to reexamine a change");
+    // TODO(dborowitz): The template currently doesn't include the ChangeMessage (aka cover letter)
+    // text at all. Update this to look for the (3 commnets) text once that's fixed.
+    assertThat(m.body()).doesNotMatch("[Cc]omment");
+  }
+
+  @Test
+  public void publishCommentsOnPushWithMessage() throws Exception {
+    PushOneCommit.Result r = createChange();
+    String rev = r.getCommit().name();
+    addDraft(r.getChangeId(), rev, newDraft(FILE_NAME, 1, "comment1"));
+
+    r = amendChange(r.getChangeId(), "refs/for/master%publish-comments,m=The_message");
+
+    Collection<CommentInfo> comments = getPublishedComments(r.getChangeId());
+    assertThat(comments.stream().map(c -> c.message)).containsExactly("comment1");
+    assertThat(getLastMessage(r.getChangeId()))
+        .isEqualTo("Uploaded patch set 2.\n\n(1 comment)\n\nThe message");
+  }
+
+  @Test
+  public void publishCommentsOnPushPublishesDraftsOnMultipleChanges() throws Exception {
+    ObjectId initialHead = testRepo.getRepository().resolve("HEAD");
+    List<RevCommit> commits = createChanges(2, "refs/for/master");
+    String id1 = byCommit(commits.get(0)).change().getKey().get();
+    String id2 = byCommit(commits.get(1)).change().getKey().get();
+    CommentInfo c1 = addDraft(id1, commits.get(0).name(), newDraft(FILE_NAME, 1, "comment1"));
+    CommentInfo c2 = addDraft(id2, commits.get(1).name(), newDraft(FILE_NAME, 1, "comment2"));
+
+    assertThat(getPublishedComments(id1)).isEmpty();
+    assertThat(getPublishedComments(id2)).isEmpty();
+
+    amendChanges(initialHead, commits, "refs/for/master%publish-comments");
+
+    Collection<CommentInfo> cs1 = getPublishedComments(id1);
+    assertThat(cs1.stream().map(c -> c.message)).containsExactly("comment1");
+    assertThat(cs1.stream().map(c -> c.id)).containsExactly(c1.id);
+    assertThat(getLastMessage(id1))
+        .isEqualTo("Uploaded patch set 2: Commit message was updated.\n\n(1 comment)");
+
+    Collection<CommentInfo> cs2 = getPublishedComments(id2);
+    assertThat(cs2.stream().map(c -> c.message)).containsExactly("comment2");
+    assertThat(cs2.stream().map(c -> c.id)).containsExactly(c2.id);
+    assertThat(getLastMessage(id2))
+        .isEqualTo("Uploaded patch set 2: Commit message was updated.\n\n(1 comment)");
+  }
+
+  @Test
+  public void publishCommentsOnPushOnlyPublishesDraftsOnUpdatedChanges() throws Exception {
+    PushOneCommit.Result r1 = createChange();
+    PushOneCommit.Result r2 = createChange();
+    String id1 = r1.getChangeId();
+    String id2 = r2.getChangeId();
+    addDraft(id1, r1.getCommit().name(), newDraft(FILE_NAME, 1, "comment1"));
+    CommentInfo c2 = addDraft(id2, r2.getCommit().name(), newDraft(FILE_NAME, 1, "comment2"));
+
+    assertThat(getPublishedComments(id1)).isEmpty();
+    assertThat(getPublishedComments(id2)).isEmpty();
+
+    r2 = amendChange(id2, "refs/for/master%publish-comments");
+
+    assertThat(getPublishedComments(id1)).isEmpty();
+    assertThat(gApi.changes().id(id1).drafts()).hasSize(1);
+
+    Collection<CommentInfo> cs2 = getPublishedComments(id2);
+    assertThat(cs2.stream().map(c -> c.message)).containsExactly("comment2");
+    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)");
+  }
+
+  private DraftInput newDraft(String path, int line, String message) {
+    DraftInput d = new DraftInput();
+    d.path = path;
+    d.side = Side.REVISION;
+    d.line = line;
+    d.message = message;
+    d.unresolved = true;
+    return d;
+  }
+
+  private CommentInfo addDraft(String changeId, String revId, DraftInput in) throws Exception {
+    return gApi.changes().id(changeId).revision(revId).createDraft(in).get();
+  }
+
+  private Collection<CommentInfo> getPublishedComments(String changeId) throws Exception {
+    return gApi.changes()
+        .id(changeId)
+        .comments()
+        .values()
+        .stream()
+        .flatMap(cs -> cs.stream())
+        .collect(toList());
+  }
+
+  private String getLastMessage(String changeId) throws Exception {
+    return Streams.findLast(
+            gApi.changes()
+                .id(changeId)
+                .get(EnumSet.of(ListChangesOption.MESSAGES))
+                .messages
+                .stream()
+                .map(m -> m.message))
+        .get();
+  }
+
   private void assertThatUserIsOnlyReviewer(ChangeInfo ci, TestAccount reviewer) {
     assertThat(ci.reviewers).isNotNull();
     assertThat(ci.reviewers.keySet()).containsExactly(ReviewerState.REVIEWER);

gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java

@@ -1242,6 +1242,9 @@ public class ReceiveCommits {
     @Option(name = "--merged", usage = "create single change for a merged commit")
     boolean merged;
 
+    @Option(name = "--publish-comments", usage = "publish all draft comments on updated changes")
+    boolean publishComments;
+
     @Option(
       name = "--notify",
       usage =

gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.git;
 
 import static com.google.gerrit.common.FooterConstants.CHANGE_ID;
+import static com.google.gerrit.server.ChangeMessagesUtil.TAG_UPLOADED_PATCH_SET;
 import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromFooters;
 import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromReviewers;
 import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
@@ -30,12 +31,14 @@ import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.reviewdb.client.Branch;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
+import com.google.gerrit.reviewdb.client.Comment;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.PatchSetInfo;
 import com.google.gerrit.server.ApprovalCopier;
 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.account.AccountResolver;
 import com.google.gerrit.server.change.ChangeKindCache;
@@ -100,6 +103,7 @@ public class ReplaceOp implements BatchUpdateOp {
   private final ChangeData.Factory changeDataFactory;
   private final ChangeKindCache changeKindCache;
   private final ChangeMessagesUtil cmUtil;
+  private final CommentsUtil commentsUtil;
   private final ExecutorService sendEmailExecutor;
   private final RevisionCreated revisionCreated;
   private final CommentAdded commentAdded;
@@ -127,6 +131,7 @@ 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;
@@ -140,6 +145,7 @@ public class ReplaceOp implements BatchUpdateOp {
       ChangeData.Factory changeDataFactory,
       ChangeKindCache changeKindCache,
       ChangeMessagesUtil cmUtil,
+      CommentsUtil commentsUtil,
       RevisionCreated revisionCreated,
       CommentAdded commentAdded,
       MergedByPushOp.Factory mergedByPushOpFactory,
@@ -164,6 +170,7 @@ public class ReplaceOp implements BatchUpdateOp {
     this.changeDataFactory = changeDataFactory;
     this.changeKindCache = changeKindCache;
     this.cmUtil = cmUtil;
+    this.commentsUtil = commentsUtil;
     this.revisionCreated = revisionCreated;
     this.commentAdded = commentAdded;
     this.mergedByPushOpFactory = mergedByPushOpFactory;
@@ -253,6 +260,9 @@ public class ReplaceOp implements BatchUpdateOp {
         change.setWorkInProgress(true);
         update.setWorkInProgress(true);
       }
+      if (magicBranch.publishComments) {
+        comments = publishComments(ctx);
+      }
     }
 
     boolean draft = magicBranch != null && magicBranch.draft;
@@ -305,6 +315,20 @@ public class ReplaceOp implements BatchUpdateOp {
 
     recipients.add(oldRecipients);
 
+    msg = createChangeMessage(ctx, reviewMessage);
+    cmUtil.addChangeMessage(ctx.getDb(), update, msg);
+
+    if (mergedByPushOp == null) {
+      resetChange(ctx);
+    } else {
+      mergedByPushOp.setPatchSetProvider(Providers.of(newPatchSet)).updateChange(ctx);
+    }
+
+    return true;
+  }
+
+  private ChangeMessage createChangeMessage(ChangeContext ctx, String reviewMessage)
+      throws OrmException {
     String approvalMessage =
         ApprovalsUtil.renderMessageWithApprovals(
             patchSetId.get(), approvals, scanLabels(ctx, approvals));
@@ -315,26 +339,21 @@ 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").append(reviewMessage);
+      message.append("\n\n").append(reviewMessage);
     }
     boolean workInProgress = magicBranch != null && magicBranch.workInProgress;
-    msg =
+    return ChangeMessagesUtil.newMessage(
-        ChangeMessagesUtil.newMessage(
         patchSetId,
         ctx.getUser(),
         ctx.getWhen(),
         message.toString(),
         ChangeMessagesUtil.uploadedPatchSetTag(workInProgress));
-    cmUtil.addChangeMessage(ctx.getDb(), update, msg);
-
-    if (mergedByPushOp == null) {
-      resetChange(ctx);
-    } else {
-      mergedByPushOp.setPatchSetProvider(Providers.of(newPatchSet)).updateChange(ctx);
-    }
-
-    return true;
   }
 
   private String changeKindMessage(ChangeKind changeKind) {
@@ -396,6 +415,13 @@ public class ReplaceOp implements BatchUpdateOp {
     }
   }
 
+  private List<Comment> publishComments(ChangeContext ctx) throws OrmException {
+    List<Comment> comments =
+        commentsUtil.draftByChangeAuthor(ctx.getDb(), ctx.getNotes(), ctx.getUser().getAccountId());
+    commentsUtil.publish(ctx, patchSetId, comments, TAG_UPLOADED_PATCH_SET);
+    return comments;
+  }
+
   @Override
   public void postUpdate(Context ctx) throws Exception {
     if (changeKind != ChangeKind.TRIVIAL_REBASE) {