diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java index d1f2bfe497..b7a6b93817 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java @@ -52,6 +52,8 @@ import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.regex.Matcher; +import java.util.regex.Pattern; @NoHttpd public class CommentsIT extends AbstractDaemonTest { @@ -375,6 +377,8 @@ public class CommentsIT extends AbstractDaemonTest { addDraft(r1.getChangeId(), r1.getCommit().getName(), newDraft(FILE_NAME, Side.REVISION, 1, "nit: trailing whitespace")); + addDraft(r1.getChangeId(), r1.getCommit().getName(), + newDraft(FILE_NAME, Side.PARENT, 2, "what happened to this?")); addDraft(r2.getChangeId(), r2.getCommit().getName(), newDraft(FILE_NAME, Side.REVISION, 1, "join lines")); addDraft(r2.getChangeId(), r2.getCommit().getName(), @@ -410,8 +414,11 @@ public class CommentsIT extends AbstractDaemonTest { .comments(); assertThat(ps1Map.keySet()).containsExactly(FILE_NAME); List ps1List = ps1Map.get(FILE_NAME); - assertThat(ps1List).hasSize(1); - assertThat(ps1List.get(0).message).isEqualTo("nit: trailing whitespace"); + assertThat(ps1List).hasSize(2); + assertThat(ps1List.get(0).message).isEqualTo("what happened to this?"); + assertThat(ps1List.get(0).side).isEqualTo(Side.PARENT); + assertThat(ps1List.get(1).message).isEqualTo("nit: trailing whitespace"); + assertThat(ps1List.get(1).side).isNull(); assertThat(gApi.changes() .id(r2.getChangeId()) @@ -433,17 +440,20 @@ public class CommentsIT extends AbstractDaemonTest { assertThat(messages).hasSize(1); String url = canonicalWebUrl.get(); int c = r1.getChange().getId().get(); - assertThat(messages.get(0).body()).contains( - "\n" - + "Patch Set 2:\n" + assertThat(extractComments(messages.get(0).body())).isEqualTo( + "Patch Set 2:\n" + "\n" - + "(3 comments)\n" + + "(4 comments)\n" + "\n" + "comments\n" + "\n" + url + "#/c/" + c + "/1/a.txt\n" + "File a.txt:\n" + "\n" + + "PS1, Line 2: \n" + + "what happened to this?\n" + + "\n" + + "\n" + "PS1, Line 1: ew\n" + "nit: trailing whitespace\n" + "\n" @@ -457,9 +467,14 @@ public class CommentsIT extends AbstractDaemonTest { + "\n" + "PS2, Line 2: nten\n" + "typo: content\n" - + "\n" - + "\n" - + "-- \n"); + + "\n"); + } + + private static String extractComments(String msg) { + // Extract lines between start "....." and end "-- ". + Pattern p = Pattern.compile(".*[.]{5}\n+(.*)\\n+-- \n.*", Pattern.DOTALL); + Matcher m = p.matcher(msg); + return m.matches() ? m.group(1) : msg; } private void addComment(PushOneCommit.Result r, String message) diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java index bd7b7c1345..c04b729461 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java @@ -148,6 +148,10 @@ public final class PatchLineComment { return key; } + public PatchSet.Id getPatchSetId() { + return key.getParentKey().getParentKey(); + } + public int getLine() { return lineNbr; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java index e741594c93..ecd9027bfb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java @@ -15,6 +15,7 @@ package com.google.gerrit.server; import static com.google.common.base.MoreObjects.firstNonNull; +import static com.google.common.base.Preconditions.checkArgument; import com.google.common.base.Optional; import com.google.common.base.Predicate; @@ -353,13 +354,18 @@ public class PatchLineCommentsUtil { public static RevId setCommentRevId(PatchLineComment c, PatchListCache cache, Change change, PatchSet ps) throws OrmException { + checkArgument(c.getPatchSetId().equals(ps.getId()), + "cannot set RevId for patch set %s on comment %s", ps.getId(), c); if (c.getRevId() == null) { try { - // TODO(dborowitz): Bypass cache if side is REVISION. - PatchList patchList = cache.get(change, ps); - c.setRevId((c.getSide() == (short) 0) - ? new RevId(ObjectId.toString(patchList.getOldId())) - : new RevId(ObjectId.toString(patchList.getNewId()))); + if (Side.fromShort(c.getSide()) == Side.REVISION) { + c.setRevId(ps.getRevision()); + } else { + PatchList patchList = cache.get(change, ps); + c.setRevId((c.getSide() == (short) 0) + ? new RevId(ObjectId.toString(patchList.getOldId())) + : new RevId(ObjectId.toString(patchList.getNewId()))); + } } catch (PatchListNotAvailableException e) { throw new OrmException(e); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java index 134189ebd4..1bbd735f0d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java @@ -20,6 +20,7 @@ import static com.google.gerrit.server.notedb.PatchSetState.DRAFT; import static com.google.gerrit.server.notedb.PatchSetState.PUBLISHED; import com.google.common.collect.ImmutableCollection; +import com.google.common.collect.ImmutableMap; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.RevId; @@ -72,6 +73,20 @@ public class PatchSetUtil { return notes.load().getPatchSets().values(); } + public ImmutableMap byChangeAsMap(ReviewDb db, + ChangeNotes notes) throws OrmException { + if (!migration.readChanges()) { + ImmutableMap.Builder result = + ImmutableMap.builder(); + for (PatchSet ps : ChangeUtil.PS_ID_ORDER.sortedCopy( + db.patchSets().byChange(notes.getChangeId()))) { + result.put(ps.getId(), ps); + } + return result.build(); + } + return notes.load().getPatchSets(); + } + public PatchSet insert(ReviewDb db, RevWalk rw, ChangeUpdate update, PatchSet.Id psId, ObjectId commit, boolean draft, List groups, String pushCertificate) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java index 0cfe445059..74929698c2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java @@ -23,6 +23,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; import com.google.auto.value.AutoValue; import com.google.common.base.MoreObjects; import com.google.common.base.Strings; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.common.collect.Ordering; @@ -469,18 +470,15 @@ public class PostReview implements RestModifyView del.addAll(drafts.values()); break; case PUBLISH: - case PUBLISH_ALL_REVISIONS: for (PatchLineComment e : drafts.values()) { - e.setStatus(PatchLineComment.Status.PUBLISHED); - e.setWrittenOn(ctx.getWhen()); - setCommentRevId(e, patchListCache, ctx.getChange(), ps); - ups.add(e); + ups.add(publishComment(ctx, e, ps)); } break; + case PUBLISH_ALL_REVISIONS: + publishAllRevisions(ctx, drafts, ups); + break; } ChangeUpdate u = ctx.getUpdate(psId); - // TODO(dborowitz): Currently doesn't work for PUBLISH_ALL_REVISIONS with - // NoteDb. plcUtil.deleteComments(ctx.getDb(), u, del); plcUtil.putComments(ctx.getDb(), u, ups); comments.addAll(ups); @@ -526,6 +524,32 @@ public class PostReview implements RestModifyView return labels; } + private PatchLineComment publishComment(ChangeContext ctx, + PatchLineComment c, PatchSet ps) throws OrmException { + c.setStatus(PatchLineComment.Status.PUBLISHED); + c.setWrittenOn(ctx.getWhen()); + setCommentRevId(c, patchListCache, ctx.getChange(), checkNotNull(ps)); + return c; + } + + private void publishAllRevisions(ChangeContext ctx, + Map drafts, List ups) + throws OrmException { + boolean needOtherPatchSets = false; + for (PatchLineComment c : drafts.values()) { + if (!c.getPatchSetId().equals(psId)) { + needOtherPatchSets = true; + break; + } + } + Map patchSets = needOtherPatchSets + ? psUtil.byChangeAsMap(ctx.getDb(), ctx.getNotes()) + : ImmutableMap.of(psId, ps); + for (PatchLineComment e : drafts.values()) { + ups.add(publishComment(ctx, e, patchSets.get(e.getPatchSetId()))); + } + } + private boolean updateLabels(ChangeContext ctx) throws OrmException, ResourceConflictException { Map inLabels = MoreObjects.firstNonNull(in.labels, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java index d3084263bc..8741c3781c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java @@ -306,7 +306,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { } private void verifyComment(PatchLineComment c) { - checkArgument(c.getRevId() != null); + checkArgument(c.getRevId() != null, "RevId required for comment: %s", c); checkArgument(c.getAuthor().equals(getUser().getAccountId()), "The author for the following comment does not match the author of" + " this ChangeDraftUpdate (%s): %s", getUser().getAccountId(), c);