Add ReceiveCommits option to publish comments
To keep the options interface simple, the only possibility is
PUBLISH_ALL_REVISIONS.
Change-Id: Ia95974ef485c8c57ceae5b8bd4d9784bfee14e1f
diff --git a/Documentation/user-upload.txt b/Documentation/user-upload.txt
index 259540e..c12d60a 100644
--- a/Documentation/user-upload.txt
+++ b/Documentation/user-upload.txt
@@ -262,6 +262,16 @@
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
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index 4f4b6c6..5cd089e 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/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.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.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 @@
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);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
index 570842b..c398131 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
@@ -1242,6 +1242,9 @@
@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 =
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
index 0cee090..377cd44 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
+++ b/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.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 @@
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 @@
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 @@
ChangeData.Factory changeDataFactory,
ChangeKindCache changeKindCache,
ChangeMessagesUtil cmUtil,
+ CommentsUtil commentsUtil,
RevisionCreated revisionCreated,
CommentAdded commentAdded,
MergedByPushOp.Factory mergedByPushOpFactory,
@@ -164,6 +170,7 @@
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 @@
change.setWorkInProgress(true);
update.setWorkInProgress(true);
}
+ if (magicBranch.publishComments) {
+ comments = publishComments(ctx);
+ }
}
boolean draft = magicBranch != null && magicBranch.draft;
@@ -305,6 +315,20 @@
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 @@
} 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 =
- 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;
+ return ChangeMessagesUtil.newMessage(
+ patchSetId,
+ ctx.getUser(),
+ ctx.getWhen(),
+ message.toString(),
+ ChangeMessagesUtil.uploadedPatchSetTag(workInProgress));
}
private String changeKindMessage(ChangeKind changeKind) {
@@ -396,6 +415,13 @@
}
}
+ 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) {
