Merge "NoteDb: Support comments on multiple patch sets with same base"
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 b7a6b93..69cb05b 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
@@ -383,6 +383,10 @@
newDraft(FILE_NAME, Side.REVISION, 1, "join lines"));
addDraft(r2.getChangeId(), r2.getCommit().getName(),
newDraft(FILE_NAME, Side.REVISION, 2, "typo: content"));
+ addDraft(r2.getChangeId(), r2.getCommit().getName(),
+ newDraft(FILE_NAME, Side.PARENT, 1, "comment 1 on base"));
+ addDraft(r2.getChangeId(), r2.getCommit().getName(),
+ newDraft(FILE_NAME, Side.PARENT, 2, "comment 2 on base"));
PushOneCommit.Result other = createChange();
// Drafts on other changes aren't returned.
@@ -431,9 +435,11 @@
.comments();
assertThat(ps2Map.keySet()).containsExactly(FILE_NAME);
List<CommentInfo> ps2List = ps2Map.get(FILE_NAME);
- assertThat(ps2List).hasSize(2);
- assertThat(ps2List.get(0).message).isEqualTo("join lines");
- assertThat(ps2List.get(1).message).isEqualTo("typo: content");
+ assertThat(ps2List).hasSize(4);
+ assertThat(ps2List.get(0).message).isEqualTo("comment 1 on base");
+ assertThat(ps2List.get(1).message).isEqualTo("comment 2 on base");
+ assertThat(ps2List.get(2).message).isEqualTo("join lines");
+ assertThat(ps2List.get(3).message).isEqualTo("typo: content");
ImmutableList<Message> messages =
email.getMessages(r2.getChangeId(), "comment");
@@ -443,7 +449,7 @@
assertThat(extractComments(messages.get(0).body())).isEqualTo(
"Patch Set 2:\n"
+ "\n"
- + "(4 comments)\n"
+ + "(6 comments)\n"
+ "\n"
+ "comments\n"
+ "\n"
@@ -461,6 +467,14 @@
+ url + "#/c/" + c + "/2/a.txt\n"
+ "File a.txt:\n"
+ "\n"
+ + "PS2, Line 1: \n"
+ + "comment 1 on base\n"
+ + "\n"
+ + "\n"
+ + "PS2, Line 2: \n"
+ + "comment 2 on base\n"
+ + "\n"
+ + "\n"
+ "PS2, Line 1: ew\n"
+ "join lines\n"
+ "\n"
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java
index 7d3f363..8b112a3 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java
@@ -15,12 +15,14 @@
package com.google.gerrit.server.notedb;
import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.gerrit.server.PatchLineCommentsUtil.PLC_ORDER;
import static com.google.gerrit.server.notedb.ChangeNotes.parseException;
import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
+import com.google.common.collect.Multimap;
import com.google.common.primitives.Ints;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
@@ -31,8 +33,8 @@
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.client.RevId;
+import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.server.GerritPersonIdent;
-import com.google.gerrit.server.PatchLineCommentsUtil;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.config.AnonymousCowardName;
import com.google.gerrit.server.config.GerritServerId;
@@ -48,7 +50,6 @@
import org.eclipse.jgit.util.QuotedString;
import org.eclipse.jgit.util.RawParseUtils;
-import java.io.ByteArrayOutputStream;
import java.io.OutputStream;
import java.io.OutputStreamWriter;
import java.io.PrintWriter;
@@ -151,6 +152,11 @@
serverId, email);
}
+ private static boolean match(byte[] note, MutableInteger p, byte[] expected) {
+ int m = RawParseUtils.match(note, p.value, expected);
+ return m == p.value + expected.length;
+ }
+
public List<PatchLineComment> parseNote(byte[] note, MutableInteger p,
Change.Id changeId, Status status) throws ConfigInvalidException {
if (p.value >= note.length) {
@@ -159,21 +165,33 @@
Set<PatchLineComment.Key> seen = new HashSet<>();
List<PatchLineComment> result = Lists.newArrayList();
int sizeOfNote = note.length;
+ byte[] psb = PATCH_SET.getBytes(UTF_8);
+ byte[] bpsb = BASE_PATCH_SET.getBytes(UTF_8);
- boolean isForBase =
- (RawParseUtils.match(note, p.value, PATCH_SET.getBytes(UTF_8))) < 0;
+ RevId revId = new RevId(parseStringField(note, p, changeId, REVISION));
+ String fileName = null;
+ PatchSet.Id psId = null;
+ boolean isForBase = false;
- PatchSet.Id psId = parsePsId(note, p, changeId, isForBase ? BASE_PATCH_SET : PATCH_SET);
-
- RevId revId =
- new RevId(parseStringField(note, p, changeId, REVISION));
-
- PatchLineComment c = null;
while (p.value < sizeOfNote) {
- String previousFileName = c == null ?
- null : c.getKey().getParentKey().getFileName();
- c = parseComment(note, p, previousFileName, psId, revId,
- isForBase, status);
+ boolean matchPs = match(note, p, psb);
+ boolean matchBase = match(note, p, bpsb);
+ if (matchPs) {
+ fileName = null;
+ psId = parsePsId(note, p, changeId, PATCH_SET);
+ isForBase = false;
+ } else if (matchBase) {
+ fileName = null;
+ psId = parsePsId(note, p, changeId, BASE_PATCH_SET);
+ isForBase = true;
+ } else if (psId == null) {
+ throw parseException(changeId, "missing %s or %s header",
+ PATCH_SET, BASE_PATCH_SET);
+ }
+
+ PatchLineComment c =
+ parseComment(note, p, fileName, psId, revId, isForBase, status);
+ fileName = c.getKey().getParentKey().getFileName();
if (!seen.add(c.getKey())) {
throw parseException(
changeId, "multiple comments for %s in note", c.getKey());
@@ -434,105 +452,113 @@
}
}
- public byte[] buildNote(List<PatchLineComment> comments) {
- ByteArrayOutputStream out = new ByteArrayOutputStream();
- buildNote(comments, out);
- return out.toByteArray();
- }
-
/**
* Build a note that contains the metadata for and the contents of all of the
- * comments in the given list of comments.
+ * comments in the given comments.
*
- * @param comments A list of the comments to be written to the
- * output stream. All of the comments in this list must have the
- * same side and must share the same patch set ID.
+ * @param comments Comments to be written to the output stream, keyed by patch
+ * set ID; multiple patch sets are allowed since base revisions may be
+ * shared across patch sets. All of the comments must share the same
+ * RevId, and all the comments for a given patch set must have the same
+ * side.
* @param out output stream to write to.
*/
- void buildNote(List<PatchLineComment> comments, OutputStream out) {
+ void buildNote(Multimap<PatchSet.Id, PatchLineComment> comments,
+ OutputStream out) {
if (comments.isEmpty()) {
return;
}
+
+ List<PatchSet.Id> psIds =
+ ReviewDbUtil.intKeyOrdering().sortedCopy(comments.keySet());
+
OutputStreamWriter streamWriter = new OutputStreamWriter(out, UTF_8);
try (PrintWriter writer = new PrintWriter(streamWriter)) {
- PatchLineComment first = comments.get(0);
+ RevId revId = comments.values().iterator().next().getRevId();
+ appendHeaderField(writer, REVISION, revId.get());
- short side = first.getSide();
- PatchSet.Id psId = PatchLineCommentsUtil.getCommentPsId(first);
- appendHeaderField(writer, side == 0
- ? BASE_PATCH_SET
- : PATCH_SET,
- Integer.toString(psId.get()));
- appendHeaderField(writer, REVISION, first.getRevId().get());
+ for (PatchSet.Id psId : psIds) {
+ List<PatchLineComment> psComments =
+ PLC_ORDER.sortedCopy(comments.get(psId));
+ PatchLineComment first = psComments.get(0);
- String currentFilename = null;
+ short side = first.getSide();
+ appendHeaderField(writer, side == 0
+ ? BASE_PATCH_SET
+ : PATCH_SET,
+ Integer.toString(psId.get()));
- for (PatchLineComment c : comments) {
- PatchSet.Id currentPsId = PatchLineCommentsUtil.getCommentPsId(c);
- checkArgument(psId.equals(currentPsId),
- "All comments being added must all have the same PatchSet.Id. The "
- + "comment below does not have the same PatchSet.Id as the others "
- + "(%s).\n%s", psId.toString(), c.toString());
- checkArgument(side == c.getSide(),
- "All comments being added must all have the same side. The "
- + "comment below does not have the same side as the others "
- + "(%s).\n%s", side, c.toString());
- String commentFilename =
- QuotedString.GIT_PATH.quote(c.getKey().getParentKey().getFileName());
+ String currentFilename = null;
- if (!commentFilename.equals(currentFilename)) {
- currentFilename = commentFilename;
- writer.print("File: ");
- writer.print(commentFilename);
- writer.print("\n\n");
+ for (PatchLineComment c : psComments) {
+ checkArgument(revId.equals(c.getRevId()),
+ "All comments being added must have all the same RevId. The "
+ + "comment below does not have the same RevId as the others "
+ + "(%s).\n%s", revId, c);
+ checkArgument(side == c.getSide(),
+ "All comments being added must all have the same side. The "
+ + "comment below does not have the same side as the others "
+ + "(%s).\n%s", side, c);
+ String commentFilename = QuotedString.GIT_PATH.quote(
+ c.getKey().getParentKey().getFileName());
+
+ if (!commentFilename.equals(currentFilename)) {
+ currentFilename = commentFilename;
+ writer.print("File: ");
+ writer.print(commentFilename);
+ writer.print("\n\n");
+ }
+
+ appendOneComment(writer, c);
}
-
- // The CommentRange field for a comment is allowed to be null.
- // If it is indeed null, then in the first line, we simply use the line
- // number field for a comment instead. If it isn't null, we write the
- // comment range itself.
- CommentRange range = c.getRange();
- if (range != null) {
- writer.print(range.getStartLine());
- writer.print(':');
- writer.print(range.getStartCharacter());
- writer.print('-');
- writer.print(range.getEndLine());
- writer.print(':');
- writer.print(range.getEndCharacter());
- } else {
- writer.print(c.getLine());
- }
- writer.print("\n");
-
- writer.print(formatTime(serverIdent, c.getWrittenOn()));
- writer.print("\n");
-
- PersonIdent ident = newIdent(
- accountCache.get(c.getAuthor()).getAccount(),
- c.getWrittenOn(), serverIdent, anonymousCowardName);
- String nameString = ident.getName() + " <" + ident.getEmailAddress()
- + ">";
- appendHeaderField(writer, AUTHOR, nameString);
-
- String parent = c.getParentUuid();
- if (parent != null) {
- appendHeaderField(writer, PARENT, parent);
- }
-
- appendHeaderField(writer, UUID, c.getKey().get());
-
- if (c.getTag() != null) {
- appendHeaderField(writer, TAG, c.getTag());
- }
-
- byte[] messageBytes = c.getMessage().getBytes(UTF_8);
- appendHeaderField(writer, LENGTH,
- Integer.toString(messageBytes.length));
-
- writer.print(c.getMessage());
- writer.print("\n\n");
}
}
}
+
+ private void appendOneComment(PrintWriter writer, PatchLineComment c) {
+ // The CommentRange field for a comment is allowed to be null. If it is
+ // null, then in the first line, we simply use the line number field for a
+ // comment instead. If it isn't null, we write the comment range itself.
+ CommentRange range = c.getRange();
+ if (range != null) {
+ writer.print(range.getStartLine());
+ writer.print(':');
+ writer.print(range.getStartCharacter());
+ writer.print('-');
+ writer.print(range.getEndLine());
+ writer.print(':');
+ writer.print(range.getEndCharacter());
+ } else {
+ writer.print(c.getLine());
+ }
+ writer.print("\n");
+
+ writer.print(formatTime(serverIdent, c.getWrittenOn()));
+ writer.print("\n");
+
+ PersonIdent ident = newIdent(
+ accountCache.get(c.getAuthor()).getAccount(),
+ c.getWrittenOn(), serverIdent, anonymousCowardName);
+ String nameString = ident.getName() + " <" + ident.getEmailAddress()
+ + ">";
+ appendHeaderField(writer, AUTHOR, nameString);
+
+ String parent = c.getParentUuid();
+ if (parent != null) {
+ appendHeaderField(writer, PARENT, parent);
+ }
+
+ appendHeaderField(writer, UUID, c.getKey().get());
+
+ if (c.getTag() != null) {
+ appendHeaderField(writer, TAG, c.getTag());
+ }
+
+ byte[] messageBytes = c.getMessage().getBytes(UTF_8);
+ appendHeaderField(writer, LENGTH,
+ Integer.toString(messageBytes.length));
+
+ writer.print(c.getMessage());
+ writer.print("\n\n");
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNoteBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNoteBuilder.java
index 4345e8b..c8364d3 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNoteBuilder.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNoteBuilder.java
@@ -15,15 +15,16 @@
package com.google.gerrit.server.notedb;
import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.gerrit.server.PatchLineCommentsUtil.PLC_ORDER;
import static java.nio.charset.StandardCharsets.UTF_8;
+import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.Maps;
+import com.google.common.collect.Multimap;
import com.google.gerrit.reviewdb.client.PatchLineComment;
+import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.RevId;
import java.io.ByteArrayOutputStream;
-import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
@@ -101,19 +102,17 @@
out.write('\n');
}
- List<PatchLineComment> all =
- new ArrayList<>(baseComments.size() + put.size());
+ Multimap<PatchSet.Id, PatchLineComment> all = ArrayListMultimap.create();
for (PatchLineComment c : baseComments) {
if (!delete.contains(c.getKey()) && !put.containsKey(c.getKey())) {
- all.add(c);
+ all.put(c.getPatchSetId(), c);
}
}
for (PatchLineComment c : put.values()) {
if (!delete.contains(c.getKey())) {
- all.add(c);
+ all.put(c.getPatchSetId(), c);
}
}
- Collections.sort(all, PLC_ORDER);
noteUtil.buildNote(all, out);
return out.toByteArray();
}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
index 7cb74cb..44fa6f5 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -932,8 +932,8 @@
notes = newNotes(c);
assertThat(readNote(notes, commit)).isEqualTo(
pushCert
- + "Patch-set: 2\n"
+ "Revision: " + commit.name() + "\n"
+ + "Patch-set: 2\n"
+ "File: a.txt\n"
+ "\n"
+ "1:2-3:4\n"
@@ -1314,8 +1314,9 @@
walk.getObjectReader().open(
note.getData(), Constants.OBJ_BLOB).getBytes();
String noteString = new String(bytes, UTF_8);
- assertThat(noteString).isEqualTo("Patch-set: 1\n"
- + "Revision: abcd1234abcd1234abcd1234abcd1234abcd1234\n"
+ assertThat(noteString).isEqualTo(
+ "Revision: abcd1234abcd1234abcd1234abcd1234abcd1234\n"
+ + "Patch-set: 1\n"
+ "File: file1\n"
+ "\n"
+ "1:1-2:1\n"
@@ -1384,8 +1385,9 @@
walk.getObjectReader().open(
note.getData(), Constants.OBJ_BLOB).getBytes();
String noteString = new String(bytes, UTF_8);
- assertThat(noteString).isEqualTo("Base-for-patch-set: 1\n"
- + "Revision: abcd1234abcd1234abcd1234abcd1234abcd1234\n"
+ assertThat(noteString).isEqualTo(
+ "Revision: abcd1234abcd1234abcd1234abcd1234abcd1234\n"
+ + "Base-for-patch-set: 1\n"
+ "File: file1\n"
+ "\n"
+ "1:1-2:1\n"
@@ -1406,6 +1408,91 @@
}
@Test
+ public void patchLineCommentNotesFormatMultiplePatchSetsSameRevId()
+ throws Exception {
+ Change c = newChange();
+ String uuid1 = "uuid1";
+ String uuid2 = "uuid2";
+ String uuid3 = "uuid3";
+ String message1 = "comment 1";
+ String message2 = "comment 2";
+ String message3 = "comment 3";
+ CommentRange range1 = new CommentRange(1, 1, 2, 1);
+ CommentRange range2 = new CommentRange(2, 1, 3, 1);
+ Timestamp time = TimeUtil.nowTs();
+ RevId revId = new RevId("abcd1234abcd1234abcd1234abcd1234abcd1234");
+
+ PatchSet.Id psId1 = c.currentPatchSetId();
+ PatchSet.Id psId2 = new PatchSet.Id(c.getId(), psId1.get() + 1);
+
+ PatchLineComment comment1 = newPublishedComment(psId1, "file1",
+ uuid1, range1, range1.getEndLine(), otherUser, null, time, message1,
+ (short) 0, revId.get());
+ PatchLineComment comment2 = newPublishedComment(psId1, "file1",
+ uuid2, range2, range2.getEndLine(), otherUser, null, time, message2,
+ (short) 0, revId.get());
+ PatchLineComment comment3 = newPublishedComment(psId2, "file1",
+ uuid3, range1, range1.getEndLine(), otherUser, null, time, message3,
+ (short) 0, revId.get());
+
+ ChangeUpdate update = newUpdate(c, otherUser);
+ update.setPatchSetId(psId2);
+ update.putComment(comment3);
+ update.putComment(comment2);
+ update.putComment(comment1);
+ update.commit();
+
+ ChangeNotes notes = newNotes(c);
+
+ try (RevWalk walk = new RevWalk(repo)) {
+ ArrayList<Note> notesInTree =
+ Lists.newArrayList(notes.revisionNoteMap.noteMap.iterator());
+ Note note = Iterables.getOnlyElement(notesInTree);
+
+ byte[] bytes =
+ walk.getObjectReader().open(
+ note.getData(), Constants.OBJ_BLOB).getBytes();
+ String noteString = new String(bytes, UTF_8);
+ String timeStr = ChangeNoteUtil.formatTime(serverIdent, time);
+ assertThat(noteString).isEqualTo(
+ "Revision: abcd1234abcd1234abcd1234abcd1234abcd1234\n"
+ + "Base-for-patch-set: 1\n"
+ + "File: file1\n"
+ + "\n"
+ + "1:1-2:1\n"
+ + timeStr + "\n"
+ + "Author: Other Account <2@gerrit>\n"
+ + "UUID: uuid1\n"
+ + "Bytes: 9\n"
+ + "comment 1\n"
+ + "\n"
+ + "2:1-3:1\n"
+ + timeStr + "\n"
+ + "Author: Other Account <2@gerrit>\n"
+ + "UUID: uuid2\n"
+ + "Bytes: 9\n"
+ + "comment 2\n"
+ + "\n"
+ + "Base-for-patch-set: 2\n"
+ + "File: file1\n"
+ + "\n"
+ + "1:1-2:1\n"
+ + timeStr + "\n"
+ + "Author: Other Account <2@gerrit>\n"
+ + "UUID: uuid3\n"
+ + "Bytes: 9\n"
+ + "comment 3\n"
+ + "\n");
+ }
+
+ assertThat(notes.getComments()).isEqualTo(
+ ImmutableMultimap.of(
+ revId, comment1,
+ revId, comment2,
+ revId, comment3));
+ }
+
+ @Test
public void patchLineCommentMultipleOnePatchsetOneFileBothSides()
throws Exception {
Change c = newChange();
