opendev/gerrit
Code Issues Proposed changes

NoteDb: Support comments on multiple patch sets with same base

Browse Source 
It is possible for multiple patch sets to share the base revision,
contrary to the previous assumption in the notes format. We were
assuming that a single note contained only comments for a single patch
set ID, with the format:

  (Base-for-)Patch-set: 1
  Revision: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef
  ...

Tweak the format slightly to reflect the fact that there may be
multiple patch sets represented in a single note:

  Revision: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef
  (Base-for-)Patch-set: 1
  ...

  (Base-for-)Patch-set: 2
  ...

Support this both in the parser and when building RevisionNotes, as
we can publish multiple comments on the same base with different patch
set IDs when using PUBLISH_ALL_REVISIONS.

Change-Id: I8d2995b04a926bc7c39bab7de863ba8217b0e350
This commit is contained in:
Dave Borowitz
2016-04-21 12:24:55 -04:00
parent bcbf1f8324
commit ddb2c60c63
4 changed files with 239 additions and 113 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java
View File

@@ -383,6 +383,10 @@ public class CommentsIT extends AbstractDaemonTest {
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 @@ public class CommentsIT extends AbstractDaemonTest {
.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 @@ public class CommentsIT extends AbstractDaemonTest {
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 @@ public class CommentsIT extends AbstractDaemonTest {
+ 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"

220
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java
View File

@@ -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.PatchLineComment.Status;
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.MutableInteger;
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 @@ public class ChangeNoteUtil {
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 @@ public class ChangeNoteUtil {
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 class ChangeNoteUtil {
}
}
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");
}
}

13
gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNoteBuilder.java
View File

@@ -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 @@ class RevisionNoteBuilder {
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();
}

97
gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
View File

@@ -932,8 +932,8 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
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 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
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 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
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"
@@ -1405,6 +1407,91 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
}
}
@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 {