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 fc13a339c6..a739abefb1 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 @@ -18,6 +18,7 @@ import com.google.gwtorm.client.Column; import com.google.gwtorm.client.StringKey; import java.sql.Timestamp; +import java.util.Objects; /** A comment left by a user on a specific line of a {@link Patch}. */ public final class PatchLineComment { @@ -120,6 +121,15 @@ public final class PatchLineComment { @Column(id = 9, notNull = false) protected CommentRange range; + /** + * The RevId for the commit to which this comment is referring. + * + * Note that this field is not stored in the database. It is just provided + * for users of this class to avoid a lookup when they don't have easy access + * to a ReviewDb. + */ + protected RevId revId; + protected PatchLineComment() { } @@ -196,4 +206,51 @@ public final class PatchLineComment { public CommentRange getRange() { return range; } + + public void setRevId(RevId rev) { + revId = rev; + } + + public RevId getRevId() { + return revId; + } + + @Override + public boolean equals(Object o) { + if (o instanceof PatchLineComment) { + PatchLineComment c = (PatchLineComment) o; + return Objects.equals(key, c.getKey()) + && Objects.equals(lineNbr, c.getLine()) + && Objects.equals(author, c.getAuthor()) + && Objects.equals(writtenOn, c.getWrittenOn()) + && Objects.equals(status, c.getStatus().getCode()) + && Objects.equals(side, c.getSide()) + && Objects.equals(message, c.getMessage()) + && Objects.equals(parentUuid, c.getParentUuid()) + && Objects.equals(range, c.getRange()) + && Objects.equals(revId, c.getRevId()); + } + return false; + } + + @Override + public String toString() { + StringBuilder builder = new StringBuilder(); + builder.append("PatchLineComment{"); + builder.append("key=").append(key).append(','); + builder.append("lineNbr=").append(lineNbr).append(','); + builder.append("author=").append(author.get()).append(','); + builder.append("writtenOn=").append(writtenOn.toString()).append(','); + builder.append("status=").append(status).append(','); + builder.append("side=").append(side).append(','); + builder.append("message=").append(Objects.toString(message, "")) + .append(','); + builder.append("parentUuid=").append(Objects.toString(parentUuid, "")) + .append(','); + builder.append("range=").append(Objects.toString(range, "")) + .append(','); + builder.append("revId=").append(revId != null ? revId.get() : ""); + builder.append('}'); + return builder.toString(); + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/VersionedMetaData.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/VersionedMetaData.java index c665d67293..7828973311 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/VersionedMetaData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/VersionedMetaData.java @@ -222,13 +222,20 @@ public abstract class VersionedMetaData { return; } - final ObjectId res = newTree.writeTree(inserter); - if (res.equals(srcTree) && !update.allowEmpty()) { + ObjectId res = newTree.writeTree(inserter); + if (res.equals(srcTree) && !update.allowEmpty() + && (commit.getTreeId() == null)) { // If there are no changes to the content, don't create the commit. return; } - commit.setTreeId(res); + if (commit.getTreeId() == null) { + commit.setTreeId(res); + } else { + // In this case, the caller populated the tree without using DirCache. + res = commit.getTreeId(); + } + if (src != null) { commit.addParentId(src); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java index f8bfd94140..5a745f3bc2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java @@ -26,6 +26,7 @@ import com.google.common.base.Function; import com.google.common.base.Optional; import com.google.common.base.Supplier; import com.google.common.collect.ArrayListMultimap; +import com.google.common.collect.ComparisonChain; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableSetMultimap; @@ -41,7 +42,10 @@ import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; +import com.google.gerrit.reviewdb.client.CommentRange; +import com.google.gerrit.reviewdb.client.PatchLineComment; import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.client.PatchSet.Id; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.PatchSetApproval.LabelId; import com.google.gerrit.server.git.GitRepositoryManager; @@ -52,20 +56,27 @@ import com.google.inject.Inject; import com.google.inject.Singleton; import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.errors.RepositoryNotFoundException; import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.notes.Note; +import org.eclipse.jgit.notes.NoteMap; import org.eclipse.jgit.revwalk.FooterKey; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.util.RawParseUtils; +import org.eclipse.jgit.lib.Constants; import java.io.IOException; import java.nio.charset.Charset; import java.sql.Timestamp; +import java.text.ParseException; import java.util.Collection; import java.util.Collections; +import java.util.Comparator; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -90,6 +101,59 @@ public class ChangeNotes extends VersionedMetaData { } }); + public static Comparator PatchLineCommentComparator = + new Comparator() { + public int compare(PatchLineComment c1, PatchLineComment c2) { + String filename1 = c1.getKey().getParentKey().get(); + String filename2 = c2.getKey().getParentKey().get(); + CommentRange range1 = c1.getRange(); + CommentRange range2 = c2.getRange(); + + // The range field for a comment can be null. If a comment's range is null + // we just use the line field in the comment (which can't be null) for + // comparison and compare to the other comment's range's end line (if + // its range isn't null) or just the line field in the other comment. + int lineForComment1 = (range1 == null) + ? c1.getLine() : range1.getEndLine(); + int lineForComment2 = (range2 == null) + ? c2.getLine() : range2.getEndLine(); + + ComparisonChain chain = ComparisonChain.start(); + if ((range1 == null) || (range2 == null)) { + chain = chain.compare(lineForComment1, lineForComment2); + } else { + chain = chain.compare(range1.getStartLine(), range2.getStartLine()) + .compare(range1.getStartCharacter(), range2.getStartCharacter()) + .compare(range1.getEndLine(), range2.getEndLine()) + .compare(range1.getEndCharacter(), range2.getEndCharacter()); + } + return chain.compare(filename1, filename2) + .compare(c1.getWrittenOn(), c2.getWrittenOn()) + .result(); + } + }; + + public static ConfigInvalidException parseException(Change.Id changeId, + String fmt, Object... args) { + return new ConfigInvalidException("Change " + changeId + ": " + + String.format(fmt, args)); + } + + public static Account.Id parseIdent(PersonIdent ident, Change.Id changeId) + throws ConfigInvalidException { + String email = ident.getEmailAddress(); + int at = email.indexOf('@'); + if (at >= 0) { + String host = email.substring(at + 1, email.length()); + Integer id = Ints.tryParse(email.substring(0, at)); + if (id != null && host.equals(GERRIT_PLACEHOLDER_HOST)) { + return new Account.Id(id); + } + } + throw parseException(changeId, "invalid identity, expected @%s: %s", + GERRIT_PLACEHOLDER_HOST, email); + } + @Singleton public static class Factory { private final GitRepositoryManager repoManager; @@ -109,28 +173,38 @@ public class ChangeNotes extends VersionedMetaData { private final Change.Id changeId; private final ObjectId tip; private final RevWalk walk; + private final Repository repo; private final Map>> approvals; private final Map reviewers; private final List submitRecords; private final Multimap changeMessages; + private final Multimap commentsForPs; + private final Multimap commentsForBase; + private NoteMap commentNoteMap; private Change.Status status; - private Parser(Change.Id changeId, ObjectId tip, RevWalk walk) { - this.changeId = changeId; + private Parser(Change change, ObjectId tip, RevWalk walk, + GitRepositoryManager repoManager) throws RepositoryNotFoundException, + IOException { + this.changeId = change.getId(); this.tip = tip; this.walk = walk; + this.repo = repoManager.openRepository(change.getProject()); approvals = Maps.newHashMap(); reviewers = Maps.newLinkedHashMap(); submitRecords = Lists.newArrayListWithExpectedSize(1); changeMessages = LinkedListMultimap.create(); + commentsForPs = ArrayListMultimap.create(); + commentsForBase = ArrayListMultimap.create(); } - private void parseAll() throws ConfigInvalidException, IOException { + private void parseAll() throws ConfigInvalidException, IOException, ParseException { walk.markStart(walk.parseCommit(tip)); for (RevCommit commit : walk) { parse(commit); } + parseComments(walk.parseCommit(tip)); pruneReviewers(); } @@ -157,7 +231,7 @@ public class ChangeNotes extends VersionedMetaData { return ImmutableListMultimap.copyOf(changeMessages); } - private void parse(RevCommit commit) throws ConfigInvalidException { + private void parse(RevCommit commit) throws ConfigInvalidException, IOException { if (status == null) { status = parseStatus(commit); } @@ -269,6 +343,34 @@ public class ChangeNotes extends VersionedMetaData { changeMessages.put(psId, changeMessage); } + private void parseComments(RevCommit commit) + throws IOException, ConfigInvalidException, ParseException { + Ref sharedMeta = repo.getRef(ChangeNoteUtil.changeRefName(changeId)); + if (sharedMeta != null) { + RevCommit sharedBaseCommit = walk.parseCommit(sharedMeta.getObjectId()); + commentNoteMap = + NoteMap.read(walk.getObjectReader(), sharedBaseCommit); + } + Iterator notes = commentNoteMap.iterator(); + while (notes.hasNext()) { + Note next = notes.next(); + byte[] bytes = walk.getObjectReader().open( + next.getData(), Constants.OBJ_BLOB).getBytes(); + List result = + CommentsInNotesUtil.parseNote(bytes, changeId); + if ((result == null) || (result.isEmpty())) { + continue; + } + PatchSet.Id psId = result.get(0).getKey().getParentKey().getParentKey(); + short side = result.get(0).getSide(); + if (side == 0) { + commentsForBase.putAll(psId, result); + } else { + commentsForPs.putAll(psId, result); + } + } + } + private void parseApproval(PatchSet.Id psId, Account.Id accountId, RevCommit commit, String line) throws ConfigInvalidException { Table> curr = @@ -403,11 +505,6 @@ public class ChangeNotes extends VersionedMetaData { } } - private ConfigInvalidException parseException(String fmt, Object... args) { - return new ConfigInvalidException("Change " + changeId + ": " - + String.format(fmt, args)); - } - private ConfigInvalidException expectedOneFooter(FooterKey footer, List actual) { return parseException("missing or multiple %s: %s", @@ -425,6 +522,10 @@ public class ChangeNotes extends VersionedMetaData { throw invalidFooter(footer, actual); } } + + private ConfigInvalidException parseException(String fmt, Object... args) { + return ChangeNotes.parseException(changeId, fmt, args); + } } private final GitRepositoryManager repoManager; @@ -434,6 +535,9 @@ public class ChangeNotes extends VersionedMetaData { private ImmutableSetMultimap reviewers; private ImmutableList submitRecords; private ImmutableListMultimap changeMessages; + private ImmutableListMultimap commentsForBase; + private ImmutableListMultimap commentsForPS; + NoteMap noteMap; @VisibleForTesting ChangeNotes(GitRepositoryManager repoManager, Change change) { @@ -491,6 +595,23 @@ public class ChangeNotes extends VersionedMetaData { return changeMessages; } + /** @return inline comments on each patchset's base (side == 0). */ + public ImmutableListMultimap + getBaseComments() { + return commentsForBase; + } + + /** @return inline comments on each patchset (side == 1). */ + public ImmutableListMultimap + getPatchSetComments() { + return commentsForPS; + } + + /** @return the NoteMap */ + NoteMap getNoteMap() { + return noteMap; + } + @Override protected String getRefName() { return ChangeNoteUtil.changeRefName(change.getId()); @@ -505,7 +626,7 @@ public class ChangeNotes extends VersionedMetaData { } RevWalk walk = new RevWalk(reader); try { - Parser parser = new Parser(change.getId(), rev, walk); + Parser parser = new Parser(change, rev, walk, repoManager); parser.parseAll(); if (parser.status != null) { @@ -513,6 +634,9 @@ public class ChangeNotes extends VersionedMetaData { } approvals = parser.buildApprovals(); changeMessages = parser.buildMessages(); + commentsForBase = ImmutableListMultimap.copyOf(parser.commentsForBase); + commentsForPS = ImmutableListMultimap.copyOf(parser.commentsForPs); + noteMap = parser.commentNoteMap; ImmutableSetMultimap.Builder reviewers = ImmutableSetMultimap.builder(); @@ -523,6 +647,9 @@ public class ChangeNotes extends VersionedMetaData { this.reviewers = reviewers.build(); submitRecords = ImmutableList.copyOf(parser.submitRecords); + } catch (ParseException e1) { + // TODO(yyonas): figure out how to handle this exception + throw new IOException(e1); } finally { walk.release(); } @@ -533,6 +660,8 @@ public class ChangeNotes extends VersionedMetaData { reviewers = ImmutableSetMultimap.of(); submitRecords = ImmutableList.of(); changeMessages = ImmutableListMultimap.of(); + commentsForBase = ImmutableListMultimap.of(); + commentsForPS = ImmutableListMultimap.of(); } @Override 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 7933d7a4a2..7ebb4ad157 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 @@ -20,14 +20,18 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_PATCH_SET; import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_STATUS; import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_SUBMITTED_WITH; import static com.google.gerrit.server.notedb.ChangeNoteUtil.GERRIT_PLACEHOLDER_HOST; +import static com.google.gerrit.server.notedb.CommentsInNotesUtil.getCommentPsId; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Optional; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Lists; import com.google.common.collect.Maps; +import com.google.common.collect.Multimap; import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.PatchLineComment; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.GerritPersonIdent; @@ -39,18 +43,23 @@ import com.google.gerrit.server.git.VersionedMetaData; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.util.LabelVote; +import com.google.gwtorm.server.OrmException; import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.AssistedInject; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.CommitBuilder; +import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.notes.NoteMap; import org.eclipse.jgit.revwalk.FooterKey; import org.eclipse.jgit.revwalk.RevCommit; import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; import java.util.Comparator; import java.util.Date; import java.util.List; @@ -88,6 +97,9 @@ public class ChangeUpdate extends VersionedMetaData { private String subject; private PatchSet.Id psId; private List submitRecords; + private final CommentsInNotesUtil commentsUtil; + private List commentsForBase; + private List commentsForPs; private String changeMessage; @AssistedInject @@ -99,9 +111,10 @@ public class ChangeUpdate extends VersionedMetaData { MetaDataUpdate.User updateFactory, ProjectCache projectCache, IdentifiedUser user, - @Assisted ChangeControl ctl) { + @Assisted ChangeControl ctl, + CommentsInNotesUtil commentsUtil) { this(serverIdent, repoManager, migration, accountCache, updateFactory, - projectCache, ctl, serverIdent.getWhen()); + projectCache, ctl, serverIdent.getWhen(), commentsUtil); } @AssistedInject @@ -113,10 +126,12 @@ public class ChangeUpdate extends VersionedMetaData { MetaDataUpdate.User updateFactory, ProjectCache projectCache, @Assisted ChangeControl ctl, - @Assisted Date when) { - this(serverIdent, repoManager, migration, accountCache, updateFactory, - ctl, when, - projectCache.get(getProjectName(ctl)).getLabelTypes().nameComparator()); + @Assisted Date when, + CommentsInNotesUtil commentsUtil) { + this(serverIdent, repoManager, migration, accountCache, updateFactory, ctl, + when, + projectCache.get(getProjectName(ctl)).getLabelTypes().nameComparator(), + commentsUtil); } private static Project.NameKey getProjectName(ChangeControl ctl) { @@ -132,7 +147,8 @@ public class ChangeUpdate extends VersionedMetaData { MetaDataUpdate.User updateFactory, @Assisted ChangeControl ctl, @Assisted Date when, - @Assisted Comparator labelNameComparator) { + @Assisted Comparator labelNameComparator, + CommentsInNotesUtil commentsUtil) { this.repoManager = repoManager; this.migration = migration; this.accountCache = accountCache; @@ -140,8 +156,11 @@ public class ChangeUpdate extends VersionedMetaData { this.ctl = ctl; this.when = when; this.serverIdent = serverIdent; + this.commentsUtil = commentsUtil; this.approvals = Maps.newTreeMap(labelNameComparator); this.reviewers = Maps.newLinkedHashMap(); + this.commentsForPs = Lists.newArrayList(); + this.commentsForBase = Lists.newArrayList(); } public Change getChange() { @@ -191,6 +210,20 @@ public class ChangeUpdate extends VersionedMetaData { this.changeMessage = changeMessage; } + public void putComment(PatchLineComment comment) { + checkArgument(psId != null, + "setPatchSetId must be called before putComment"); + checkArgument(getCommentPsId(comment).equals(psId), + "Comment on %s doesn't match previous patch set %s", + getCommentPsId(comment), psId); + checkArgument(comment.getRevId() != null); + if (comment.getSide() == 0) { + commentsForBase.add(comment); + } else { + commentsForPs.add(comment); + } + } + public void putReviewer(Account.Id reviewer, ReviewerState type) { checkArgument(type != ReviewerState.REMOVED, "invalid ReviewerType"); reviewers.put(reviewer, type); @@ -213,6 +246,49 @@ public class ChangeUpdate extends VersionedMetaData { } } + /** @return the tree id for the updated tree */ + private ObjectId storeCommentsInNotes() throws OrmException, IOException { + ChangeNotes notes = ctl.getNotes(); + NoteMap noteMap = notes.getNoteMap(); + if (noteMap == null) { + noteMap = NoteMap.newEmptyMap(); + } + if (commentsForPs.isEmpty() && commentsForBase.isEmpty()) { + return null; + } + + Multimap allCommentsOnBases = + notes.getBaseComments(); + Multimap allCommentsOnPs = + notes.getPatchSetComments(); + + // This writes all comments for the base of this PS to the note map. + if (!commentsForBase.isEmpty()) { + writeCommentsToNoteMap(noteMap, allCommentsOnBases, commentsForBase); + } + + // This write all comments for this PS to the note map. + if (!commentsForPs.isEmpty()) { + writeCommentsToNoteMap(noteMap, allCommentsOnPs, commentsForPs); + } + return noteMap.writeTree(inserter); + } + + private void writeCommentsToNoteMap(NoteMap noteMap, + Multimap allComments, + List commentsToAdd) + throws OrmException, IOException { + ObjectId commitOID = + ObjectId.fromString(commentsToAdd.get(0).getRevId().get()); + List commentsForThisPS = + new ArrayList(allComments.get(psId)); + commentsForThisPS.addAll(commentsToAdd); + Collections.sort(commentsForThisPS, ChangeNotes.PatchLineCommentComparator); + byte[] note = commentsUtil.buildNote(commentsForThisPS); + ObjectId noteId = inserter.insert(Constants.OBJ_BLOB, note, 0, note.length); + noteMap.set(commitOID, noteId); + } + @Override public RevCommit commit(MetaDataUpdate md) throws IOException { throw new UnsupportedOperationException("use commit()"); @@ -221,8 +297,18 @@ public class ChangeUpdate extends VersionedMetaData { public RevCommit commit() throws IOException { BatchMetaDataUpdate batch = openUpdate(); try { - batch.write(new CommitBuilder()); - return batch.commit(); + CommitBuilder builder = new CommitBuilder(); + if (migration.write()) { + ObjectId treeId = storeCommentsInNotes(); + if (treeId != null) { + builder.setTreeId(treeId); + } + } + batch.write(builder); + RevCommit c = batch.commit(); + return c; + } catch (OrmException e) { + throw new IOException(e); } finally { batch.close(); } @@ -363,6 +449,8 @@ public class ChangeUpdate extends VersionedMetaData { private boolean isEmpty() { return approvals.isEmpty() && reviewers.isEmpty() + && commentsForBase.isEmpty() + && commentsForPs.isEmpty() && status == null && submitRecords == null && changeMessage == null; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/CommentsInNotesUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/CommentsInNotesUtil.java new file mode 100644 index 0000000000..1a66e5b5b8 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/CommentsInNotesUtil.java @@ -0,0 +1,475 @@ +// Copyright (C) 2014 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.notedb; + +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.gerrit.server.notedb.ChangeNoteUtil.GERRIT_PLACEHOLDER_HOST; +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.Lists; +import com.google.common.primitives.Ints; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.CommentRange; +import com.google.gerrit.reviewdb.client.Patch; +import com.google.gerrit.reviewdb.client.PatchLineComment; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.client.RevId; +import com.google.gerrit.server.GerritPersonIdent; +import com.google.gerrit.server.account.AccountCache; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; + +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.util.GitDateFormatter; +import org.eclipse.jgit.util.GitDateParser; +import org.eclipse.jgit.util.MutableInteger; +import org.eclipse.jgit.util.QuotedString; +import org.eclipse.jgit.util.RawParseUtils; +import org.eclipse.jgit.util.GitDateFormatter.Format; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.OutputStreamWriter; +import java.io.PrintWriter; +import java.nio.charset.Charset; +import java.sql.Timestamp; +import java.text.ParseException; +import java.util.Date; +import java.util.List; + +/** + * Utility functions to parse PatchLineComments out of a note byte array and + * store a list of PatchLineComments in the form of a note (in a byte array). + **/ +public class CommentsInNotesUtil { + private static final String AUTHOR = "Author"; + private static final String BASE_PATCH_SET = "Base-for-patch-set"; + private static final String COMMENT_RANGE = "Comment-range"; + private static final String FILE = "File"; + private static final String LENGTH = "Bytes"; + private static final String PARENT = "Parent"; + private static final String PATCH_SET = "Patch-set"; + private static final String REVISION = "Revision"; + private static final String UUID = "UUID"; + + public static List parseNote(byte[] note, + Change.Id changeId) throws ConfigInvalidException { + List result = Lists.newArrayList(); + int sizeOfNote = note.length; + Charset enc = RawParseUtils.parseEncoding(note); + MutableInteger curr = new MutableInteger(); + curr.value = 0; + + boolean isForBase = + (RawParseUtils.match(note, curr.value, PATCH_SET.getBytes(UTF_8))) < 0; + + PatchSet.Id psId = parsePsId(note, curr, changeId, enc, + isForBase ? BASE_PATCH_SET : PATCH_SET); + + RevId revId = + new RevId(parseStringField(note, curr, changeId, enc, REVISION)); + + PatchLineComment c = null; + while (curr.value < sizeOfNote) { + String previousFileName = c == null ? + null : c.getKey().getParentKey().getFileName(); + c = parseComment(note, curr, previousFileName, psId, revId, + isForBase, enc); + result.add(c); + } + return result; + } + + public static String formatTime(PersonIdent ident, Timestamp t) { + GitDateFormatter dateFormatter = new GitDateFormatter(Format.DEFAULT); + // TODO(dborowitz): Use a ThreadLocal or use Joda. + PersonIdent newIdent = new PersonIdent(ident, t); + return dateFormatter.formatDate(newIdent); + } + + public static PatchSet.Id getCommentPsId(PatchLineComment plc) { + return plc.getKey().getParentKey().getParentKey(); + } + + private static PatchLineComment parseComment(byte[] note, MutableInteger curr, + String currentFileName, PatchSet.Id psId, RevId revId, boolean isForBase, + Charset enc) + throws ConfigInvalidException { + Change.Id changeId = psId.getParentKey(); + + // Check if there is a new file. + boolean newFile = + (RawParseUtils.match(note, curr.value, FILE.getBytes(UTF_8))) != -1; + if (newFile) { + // If so, parse the new file name. + currentFileName = parseFilename(note, curr, changeId, enc); + } else if (currentFileName == null) { + throw parseException(changeId, "could not parse %s", FILE); + } + + CommentRange range = parseCommentRange(note, curr, changeId); + if (range == null) { + throw parseException(changeId, "could not parse %s", COMMENT_RANGE); + } + + Timestamp commentTime = parseTimestamp(note, curr, changeId, enc); + Account.Id aId = parseAuthor(note, curr, changeId, enc); + + boolean hasParent = + (RawParseUtils.match(note, curr.value, PARENT.getBytes(enc))) != -1; + String parentUUID = null; + if (hasParent) { + parentUUID = parseStringField(note, curr, changeId, enc, PARENT); + } + + String uuid = parseStringField(note, curr, changeId, enc, UUID); + int commentLength = parseCommentLength(note, curr, changeId, enc); + + String message = RawParseUtils.decode( + enc, note, curr.value, curr.value + commentLength); + checkResult(message, "message contents", changeId); + + PatchLineComment plc = new PatchLineComment( + new PatchLineComment.Key(new Patch.Key(psId, currentFileName), uuid), + range.getEndLine(), aId, parentUUID, commentTime); + plc.setMessage(message); + plc.setSide((short) (isForBase ? 0 : 1)); + if (range.getStartCharacter() != -1) { + plc.setRange(range); + } + plc.setRevId(revId); + + curr.value = RawParseUtils.nextLF(note, curr.value + commentLength); + curr.value = RawParseUtils.nextLF(note, curr.value); + return plc; + } + + private static String parseStringField(byte[] note, MutableInteger curr, + Change.Id changeId, Charset enc, String fieldName) + throws ConfigInvalidException { + int endOfLine = RawParseUtils.nextLF(note, curr.value); + checkHeaderLineFormat(note, curr, fieldName, enc, changeId); + int startOfField = RawParseUtils.endOfFooterLineKey(note, curr.value) + 2; + curr.value = endOfLine; + return RawParseUtils.decode(enc, note, startOfField, endOfLine - 1); + } + + /** + * @return a comment range. If the comment range line in the note only has + * one number, we return a CommentRange with that one number as the end + * line and the other fields as -1. If the comment range line in the note + * contains a whole comment range, then we return a CommentRange with all + * fields set. If the line is not correctly formatted, return null. + */ + private static CommentRange parseCommentRange(byte[] note, MutableInteger ptr, + Change.Id changeId) throws ConfigInvalidException { + CommentRange range = new CommentRange(-1, -1, -1, -1); + + int startLine = RawParseUtils.parseBase10(note, ptr.value, ptr); + if (startLine == 0) { + return null; + } + + if (note[ptr.value] == '\n') { + range.setEndLine(startLine); + return range; + } else if (note[ptr.value] == ':') { + range.setStartLine(startLine); + ptr.value += 1; + } else { + return null; + } + + int startChar = RawParseUtils.parseBase10(note, ptr.value, ptr); + if (startChar == 0) { + return null; + } + if (note[ptr.value] == '-') { + range.setStartCharacter(startChar); + ptr.value += 1; + } else { + return null; + } + + int endLine = RawParseUtils.parseBase10(note, ptr.value, ptr); + if (endLine == 0) { + return null; + } + if (note[ptr.value] == ':') { + range.setEndLine(endLine); + ptr.value += 1; + } else { + return null; + } + + int endChar = RawParseUtils.parseBase10(note, ptr.value, ptr); + if (endChar == 0) { + return null; + } + if (note[ptr.value] == '\n') { + range.setEndCharacter(endChar); + ptr.value += 1; + } else { + return null; + } + return range; + } + + private static PatchSet.Id parsePsId(byte[] note, MutableInteger curr, + Change.Id changeId, Charset enc, String fieldName) + throws ConfigInvalidException { + checkHeaderLineFormat(note, curr, fieldName, enc, changeId); + int startOfPsId = + RawParseUtils.endOfFooterLineKey(note, curr.value) + 1; + MutableInteger i = new MutableInteger(); + int patchSetId = + RawParseUtils.parseBase10(note, startOfPsId, i); + int endOfLine = RawParseUtils.nextLF(note, curr.value); + if (i.value != endOfLine - 1) { + throw parseException(changeId, "could not parse %s", fieldName); + } + checkResult(patchSetId, "patchset id", changeId); + curr.value = endOfLine; + return new PatchSet.Id(changeId, patchSetId); + } + + private static String parseFilename(byte[] note, MutableInteger curr, + Change.Id changeId, Charset enc) throws ConfigInvalidException { + checkHeaderLineFormat(note, curr, FILE, enc, changeId); + int startOfFileName = + RawParseUtils.endOfFooterLineKey(note, curr.value) + 2; + int endOfLine = RawParseUtils.nextLF(note, curr.value); + curr.value = endOfLine; + curr.value = RawParseUtils.nextLF(note, curr.value); + return QuotedString.GIT_PATH.dequote( + RawParseUtils.decode(enc, note, startOfFileName, endOfLine - 1)); + } + + private static Timestamp parseTimestamp(byte[] note, MutableInteger curr, + Change.Id changeId, Charset enc) + throws ConfigInvalidException { + int endOfLine = RawParseUtils.nextLF(note, curr.value); + Timestamp commentTime; + String dateString = + RawParseUtils.decode(enc, note, curr.value, endOfLine - 1); + try { + commentTime = + new Timestamp(GitDateParser.parse(dateString, null).getTime()); + } catch (ParseException e) { + throw new ConfigInvalidException("could not parse comment timestamp", e); + } + curr.value = endOfLine; + return checkResult(commentTime, "comment timestamp", changeId); + } + + private static Account.Id parseAuthor(byte[] note, MutableInteger curr, + Change.Id changeId, Charset enc) throws ConfigInvalidException { + checkHeaderLineFormat(note, curr, AUTHOR, enc, changeId); + int startOfAccountId = + RawParseUtils.endOfFooterLineKey(note, curr.value) + 2; + PersonIdent ident = + RawParseUtils.parsePersonIdent(note, startOfAccountId); + Account.Id aId = parseIdent(ident, changeId); + curr.value = RawParseUtils.nextLF(note, curr.value); + return checkResult(aId, "comment author", changeId); + } + + private static int parseCommentLength(byte[] note, MutableInteger curr, + Change.Id changeId, Charset enc) throws ConfigInvalidException { + checkHeaderLineFormat(note, curr, LENGTH, enc, changeId); + int startOfLength = + RawParseUtils.endOfFooterLineKey(note, curr.value) + 1; + MutableInteger i = new MutableInteger(); + int commentLength = + RawParseUtils.parseBase10(note, startOfLength, i); + int endOfLine = RawParseUtils.nextLF(note, curr.value); + if (i.value != endOfLine-1) { + throw parseException(changeId, "could not parse %s", PATCH_SET); + } + curr.value = endOfLine; + return checkResult(commentLength, "comment length", changeId); + } + + private static T checkResult(T o, String fieldName, + Change.Id changeId) throws ConfigInvalidException { + if (o == null) { + throw parseException(changeId, "could not parse %s", fieldName); + } + return o; + } + + private static int checkResult(int i, String fieldName, Change.Id changeId) + throws ConfigInvalidException { + if (i <= 0) { + throw parseException(changeId, "could not parse %s", fieldName); + } + return i; + } + + private PersonIdent newIdent(Account author, Date when) { + return new PersonIdent( + author.getFullName(), + author.getId().get() + "@" + GERRIT_PLACEHOLDER_HOST, + when, serverIdent.getTimeZone()); + } + + private static Account.Id parseIdent(PersonIdent ident, Change.Id changeId) + throws ConfigInvalidException { + String email = ident.getEmailAddress(); + int at = email.indexOf('@'); + if (at >= 0) { + String host = email.substring(at + 1, email.length()); + Integer id = Ints.tryParse(email.substring(0, at)); + if (id != null && host.equals(GERRIT_PLACEHOLDER_HOST)) { + return new Account.Id(id); + } + } + throw parseException(changeId, "invalid identity, expected @%s: %s", + GERRIT_PLACEHOLDER_HOST, email); + } + + private void appendHeaderField(PrintWriter writer, + String field, String value) throws IOException { + writer.print(field); + writer.print(": "); + writer.print(value); + writer.print('\n'); + } + + private static void checkHeaderLineFormat(byte[] note, MutableInteger curr, + String fieldName, Charset enc, Change.Id changeId) + throws ConfigInvalidException { + boolean correct = + RawParseUtils.match(note, curr.value, fieldName.getBytes(enc)) != -1; + correct &= (note[curr.value + fieldName.length()] == ':'); + correct &= (note[curr.value + fieldName.length() + 1] == ' '); + if (!correct) { + throw parseException(changeId, "could not parse %s", fieldName); + } + } + + private final AccountCache accountCache; + private final PersonIdent serverIdent; + + @VisibleForTesting + @Inject + public CommentsInNotesUtil(AccountCache accountCache, + @GerritPersonIdent PersonIdent serverIdent) { + this.accountCache = accountCache; + this.serverIdent = serverIdent; + } + + /** + * Build a note that contains the metadata for and the contents of all of the + * comments in the given list of comments. + * + * @param comments + * A list of the comments to be written to the returned note + * byte array. + * All of the comments in this list must have the same side and + * must share the same PatchSet.Id. + * @return the note. Null if there are no comments in the list. + */ + public byte[] buildNote(List comments) + throws OrmException, IOException { + if (comments.isEmpty()) { + return null; + } + + ByteArrayOutputStream buf = new ByteArrayOutputStream(); + OutputStreamWriter streamWriter = new OutputStreamWriter(buf, UTF_8); + PrintWriter writer = new PrintWriter(streamWriter); + PatchLineComment first = comments.get(0); + + short side = first.getSide(); + PatchSet.Id psId = getCommentPsId(first); + appendHeaderField(writer, side == 0 + ? BASE_PATCH_SET + : PATCH_SET, + Integer.toString(psId.get())); + appendHeaderField(writer, REVISION, first.getRevId().get()); + + String currentFilename = null; + + for (PatchLineComment c : comments) { + PatchSet.Id currentPsId = 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 " + + "(%d).\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 " + + "(%d).\n%s", side, c.toString()); + 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"); + } + + // 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()); + 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()); + + byte[] messageBytes = c.getMessage().getBytes(UTF_8); + appendHeaderField(writer, LENGTH, + Integer.toString(messageBytes.length)); + + writer.print(c.getMessage()); + writer.print("\n\n"); + } + writer.close(); + return buf.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 f912099c99..1f03dea183 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 @@ -17,6 +17,7 @@ package com.google.gerrit.server.notedb; import static com.google.gerrit.server.notedb.ReviewerState.CC; import static com.google.gerrit.server.notedb.ReviewerState.REVIEWER; import static com.google.inject.Scopes.SINGLETON; +import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.concurrent.TimeUnit.SECONDS; import static org.easymock.EasyMock.expect; @@ -26,17 +27,24 @@ import static org.junit.Assert.assertTrue; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSetMultimap; import com.google.common.collect.Iterables; +import com.google.common.collect.LinkedListMultimap; import com.google.common.collect.ListMultimap; +import com.google.common.collect.Lists; +import com.google.common.collect.Multimap; import com.google.common.collect.Ordering; import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.reviewdb.client.Account; 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.CommentRange; +import com.google.gerrit.reviewdb.client.Patch; +import com.google.gerrit.reviewdb.client.PatchLineComment; 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.reviewdb.client.Project; +import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountCache; @@ -53,6 +61,7 @@ import com.google.gerrit.server.git.GitModule; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.VersionedMetaData.BatchMetaDataUpdate; import com.google.gerrit.server.group.SystemGroupBackend; +import com.google.gerrit.server.notedb.CommentsInNotesUtil; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.util.TimeUtil; @@ -70,7 +79,9 @@ import org.easymock.EasyMock; import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.notes.Note; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; import org.joda.time.DateTime; @@ -81,6 +92,7 @@ import org.junit.Before; import org.junit.Test; import java.sql.Timestamp; +import java.util.ArrayList; import java.util.Date; import java.util.List; import java.util.TimeZone; @@ -831,6 +843,330 @@ public class ChangeNotesTest { assertEquals(ps1, cm.get(1).getPatchSetId()); } + @Test + public void patchLineCommentNotesFormatSide1() throws Exception { + Change c = newChange(); + ChangeUpdate update = newUpdate(c, otherUser); + String uuid = "uuid"; + String message1 = "comment 1"; + String message2 = "comment 2"; + String message3 = "comment 3"; + CommentRange range1 = new CommentRange(1, 1, 2, 1); + Timestamp time1 = TimeUtil.nowTs(); + Timestamp time2 = TimeUtil.nowTs(); + Timestamp time3 = TimeUtil.nowTs(); + PatchSet.Id psId = c.currentPatchSetId(); + + PatchLineComment comment1 = newPatchLineComment(psId, "file1", uuid, + range1, range1.getEndLine(), otherUser, null, time1, message1, + (short) 1, "abcd1234abcd1234abcd1234abcd1234abcd1234"); + update.setPatchSetId(psId); + update.putComment(comment1); + update.commit(); + + update = newUpdate(c, otherUser); + CommentRange range2 = new CommentRange(2, 1, 3, 1); + PatchLineComment comment2 = newPatchLineComment(psId, "file1", uuid, + range2, range2.getEndLine(), otherUser, null, time2, message2, + (short) 1, "abcd1234abcd1234abcd1234abcd1234abcd1234"); + update.setPatchSetId(psId); + update.putComment(comment2); + update.commit(); + + update = newUpdate(c, otherUser); + CommentRange range3 = new CommentRange(3, 1, 4, 1); + PatchLineComment comment3 = newPatchLineComment(psId, "file2", uuid, + range3, range3.getEndLine(), otherUser, null, time3, message3, + (short) 1, "abcd1234abcd1234abcd1234abcd1234abcd1234"); + update.setPatchSetId(psId); + update.putComment(comment3); + update.commit(); + + ChangeNotes notes = newNotes(c); + + RevWalk walk = new RevWalk(repo); + ArrayList notesInTree = + Lists.newArrayList(notes.getNoteMap().iterator()); + Note note = Iterables.getOnlyElement(notesInTree); + + byte[] bytes = + walk.getObjectReader().open( + note.getData(), Constants.OBJ_BLOB).getBytes(); + String noteString = new String(bytes, UTF_8); + assertEquals("Patch-set: 1\n" + + "Revision: abcd1234abcd1234abcd1234abcd1234abcd1234\n" + + "File: file1\n" + + "\n" + + "1:1-2:1\n" + + CommentsInNotesUtil.formatTime(serverIdent, time1) + "\n" + + "Author: Other Account <2@gerrit>\n" + + "UUID: uuid\n" + + "Bytes: 9\n" + + "comment 1\n" + + "\n" + + "2:1-3:1\n" + + CommentsInNotesUtil.formatTime(serverIdent, time2) + "\n" + + "Author: Other Account <2@gerrit>\n" + + "UUID: uuid\n" + + "Bytes: 9\n" + + "comment 2\n" + + "\n" + + "File: file2\n" + + "\n" + + "3:1-4:1\n" + + CommentsInNotesUtil.formatTime(serverIdent, time3) + "\n" + + "Author: Other Account <2@gerrit>\n" + + "UUID: uuid\n" + + "Bytes: 9\n" + + "comment 3\n" + + "\n", + noteString); + } + + @Test + public void patchLineCommentNotesFormatSide0() throws Exception { + Change c = newChange(); + ChangeUpdate update = newUpdate(c, otherUser); + String uuid = "uuid"; + String message1 = "comment 1"; + String message2 = "comment 2"; + CommentRange range1 = new CommentRange(1, 1, 2, 1); + Timestamp time1 = TimeUtil.nowTs(); + Timestamp time2 = TimeUtil.nowTs(); + PatchSet.Id psId = c.currentPatchSetId(); + + PatchLineComment comment1 = newPatchLineComment(psId, "file1", uuid, + range1, range1.getEndLine(), otherUser, null, time1, message1, + (short) 0, "abcd1234abcd1234abcd1234abcd1234abcd1234"); + update.setPatchSetId(psId); + update.putComment(comment1); + update.commit(); + + update = newUpdate(c, otherUser); + CommentRange range2 = new CommentRange(2, 1, 3, 1); + PatchLineComment comment2 = newPatchLineComment(psId, "file1", uuid, + range2, range2.getEndLine(), otherUser, null, time2, message2, + (short) 0, "abcd1234abcd1234abcd1234abcd1234abcd1234"); + update.setPatchSetId(psId); + update.putComment(comment2); + update.commit(); + + ChangeNotes notes = newNotes(c); + + RevWalk walk = new RevWalk(repo); + ArrayList notesInTree = + Lists.newArrayList(notes.getNoteMap().iterator()); + Note note = Iterables.getOnlyElement(notesInTree); + + byte[] bytes = + walk.getObjectReader().open( + note.getData(), Constants.OBJ_BLOB).getBytes(); + String noteString = new String(bytes, UTF_8); + assertEquals("Base-for-patch-set: 1\n" + + "Revision: abcd1234abcd1234abcd1234abcd1234abcd1234\n" + + "File: file1\n" + + "\n" + + "1:1-2:1\n" + + CommentsInNotesUtil.formatTime(serverIdent, time1) + "\n" + + "Author: Other Account <2@gerrit>\n" + + "UUID: uuid\n" + + "Bytes: 9\n" + + "comment 1\n" + + "\n" + + "2:1-3:1\n" + + CommentsInNotesUtil.formatTime(serverIdent, time2) + "\n" + + "Author: Other Account <2@gerrit>\n" + + "UUID: uuid\n" + + "Bytes: 9\n" + + "comment 2\n" + + "\n", + noteString); + } + + + @Test + public void patchLineCommentMultipleOnePatchsetOneFileBothSides() + throws Exception { + Change c = newChange(); + ChangeUpdate update = newUpdate(c, otherUser); + String uuid = "uuid"; + String messageForBase = "comment for base"; + String messageForPS = "comment for ps"; + CommentRange range = new CommentRange(1, 1, 2, 1); + Timestamp now = TimeUtil.nowTs(); + PatchSet.Id psId = c.currentPatchSetId(); + + PatchLineComment commentForBase = + newPatchLineComment(psId, "filename", uuid, + range, range.getEndLine(), otherUser, null, now, messageForBase, + (short) 0, "abcd1234abcd1234abcd1234abcd1234abcd1234"); + update.setPatchSetId(psId); + update.putComment(commentForBase); + update.commit(); + + update = newUpdate(c, otherUser); + PatchLineComment commentForPS = + newPatchLineComment(psId, "filename", uuid, + range, range.getEndLine(), otherUser, null, now, messageForPS, + (short) 1, "abcd4567abcd4567abcd4567abcd4567abcd4567"); + update.setPatchSetId(psId); + update.putComment(commentForPS); + update.commit(); + + ChangeNotes notes = newNotes(c); + Multimap commentsForBase = + notes.getBaseComments(); + Multimap commentsForPS = + notes.getPatchSetComments(); + assertEquals(commentsForBase.size(), 1); + assertEquals(commentsForPS.size(), 1); + + assertEquals(commentForBase, + Iterables.getOnlyElement(commentsForBase.get(psId))); + assertEquals(commentForPS, + Iterables.getOnlyElement(commentsForPS.get(psId))); + } + + @Test + public void patchLineCommentMultipleOnePatchsetOneFile() throws Exception { + Change c = newChange(); + String uuid = "uuid"; + CommentRange range = new CommentRange(1, 1, 2, 1); + PatchSet.Id psId = c.currentPatchSetId(); + String filename = "filename"; + short side = (short) 1; + + ChangeUpdate update = newUpdate(c, otherUser); + Timestamp timeForComment1 = TimeUtil.nowTs(); + Timestamp timeForComment2 = TimeUtil.nowTs(); + PatchLineComment comment1 = newPatchLineComment(psId, filename, uuid, range, + range.getEndLine(), otherUser, null, timeForComment1, "comment 1", side, + "abcd1234abcd1234abcd1234abcd1234abcd1234"); + update.setPatchSetId(psId); + update.putComment(comment1); + update.commit(); + + update = newUpdate(c, otherUser); + PatchLineComment comment2 = newPatchLineComment(psId, filename, uuid, range, + range.getEndLine(), otherUser, null, timeForComment2, "comment 2", side, + "abcd1234abcd1234abcd1234abcd1234abcd1234"); + update.setPatchSetId(psId); + update.putComment(comment2); + update.commit(); + + ChangeNotes notes = newNotes(c); + Multimap commentsForBase = + notes.getBaseComments(); + Multimap commentsForPS = + notes.getPatchSetComments(); + assertEquals(commentsForBase.size(), 0); + assertEquals(commentsForPS.size(), 2); + + ImmutableList commentsForThisPS = + (ImmutableList) commentsForPS.get(psId); + assertEquals(commentsForThisPS.size(), 2); + PatchLineComment commentFromNotes1 = commentsForThisPS.get(0); + PatchLineComment commentFromNotes2 = commentsForThisPS.get(1); + + assertEquals(comment1, commentFromNotes1); + assertEquals(comment2, commentFromNotes2); + } + + @Test + public void patchLineCommentMultipleOnePatchsetMultipleFiles() + throws Exception { + Change c = newChange(); + String uuid = "uuid"; + CommentRange range = new CommentRange(1, 1, 2, 1); + PatchSet.Id psId = c.currentPatchSetId(); + String filename1 = "filename1"; + String filename2 = "filename2"; + short side = (short) 1; + + ChangeUpdate update = newUpdate(c, otherUser); + Timestamp now = TimeUtil.nowTs(); + PatchLineComment comment1 = newPatchLineComment(psId, filename1, uuid, + range, range.getEndLine(), otherUser, null, now, "comment 1", side, + "abcd1234abcd1234abcd1234abcd1234abcd1234"); + update.setPatchSetId(psId); + update.putComment(comment1); + update.commit(); + + update = newUpdate(c, otherUser); + PatchLineComment comment2 = newPatchLineComment(psId, filename2, uuid, + range, range.getEndLine(), otherUser, null, now, "comment 2", side, + "abcd1234abcd1234abcd1234abcd1234abcd1234"); + update.setPatchSetId(psId); + update.putComment(comment2); + update.commit(); + + ChangeNotes notes = newNotes(c); + Multimap commentsForBase = + notes.getBaseComments(); + Multimap commentsForPS = + notes.getPatchSetComments(); + assertEquals(commentsForBase.size(), 0); + assertEquals(commentsForPS.size(), 2); + + ImmutableList commentsForThisPS = + (ImmutableList) commentsForPS.get(psId); + assertEquals(commentsForThisPS.size(), 2); + PatchLineComment commentFromNotes1 = commentsForThisPS.get(0); + PatchLineComment commentFromNotes2 = commentsForThisPS.get(1); + + assertEquals(comment1, commentFromNotes1); + assertEquals(comment2, commentFromNotes2); + } + + @Test + public void patchLineCommentMultiplePatchsets() throws Exception { + Change c = newChange(); + String uuid = "uuid"; + CommentRange range = new CommentRange(1, 1, 2, 1); + PatchSet.Id ps1 = c.currentPatchSetId(); + String filename = "filename1"; + short side = (short) 1; + + ChangeUpdate update = newUpdate(c, otherUser); + Timestamp now = TimeUtil.nowTs(); + PatchLineComment comment1 = newPatchLineComment(ps1, filename, uuid, + range, range.getEndLine(), otherUser, null, now, "comment on ps1", side, + "abcd1234abcd1234abcd1234abcd1234abcd1234"); + update.setPatchSetId(ps1); + update.putComment(comment1); + update.commit(); + + incrementPatchSet(c); + PatchSet.Id ps2 = c.currentPatchSetId(); + + update = newUpdate(c, otherUser); + now = TimeUtil.nowTs(); + PatchLineComment comment2 = newPatchLineComment(ps2, filename, uuid, + range, range.getEndLine(), otherUser, null, now, "comment on ps2", side, + "abcd4567abcd4567abcd4567abcd4567abcd4567"); + update.setPatchSetId(ps2); + update.putComment(comment2); + update.commit(); + + ChangeNotes notes = newNotes(c); + LinkedListMultimap commentsForBase = + LinkedListMultimap.create(notes.getBaseComments()); + LinkedListMultimap commentsForPS = + LinkedListMultimap.create(notes.getPatchSetComments()); + assertEquals(commentsForBase.keys().size(), 0); + assertEquals(commentsForPS.values().size(), 2); + + List commentsForPS1 = commentsForPS.get(ps1); + assertEquals(commentsForPS1.size(), 1); + PatchLineComment commentFromPs1 = commentsForPS1.get(0); + + List commentsForPS2 = commentsForPS.get(ps2); + assertEquals(commentsForPS2.size(), 1); + PatchLineComment commentFromPs2 = commentsForPS2.get(0); + + assertEquals(comment1, commentFromPs1); + assertEquals(comment2, commentFromPs2); + } private Change newChange() { Change.Id changeId = new Change.Id(1); @@ -844,6 +1180,21 @@ public class ChangeNotesTest { return c; } + private PatchLineComment newPatchLineComment(PatchSet.Id psId, + String filename, String UUID, CommentRange range, int line, + IdentifiedUser commenter, String parentUUID, Timestamp t, + String message, short side, String commitSHA1) { + PatchLineComment comment = new PatchLineComment( + new PatchLineComment.Key( + new Patch.Key(psId, filename), UUID), + line, commenter.getAccountId(), parentUUID, t); + comment.setSide(side); + comment.setMessage(message); + comment.setRange(range); + comment.setRevId(new RevId(commitSHA1)); + return comment; + } + private ChangeUpdate newUpdate(Change c, final IdentifiedUser user) throws Exception { return injector.createChildInjector(new FactoryModule() { @@ -877,10 +1228,13 @@ public class ChangeNotesTest { return new Timestamp(c.getCreatedOn().getTime() + millis); } - private ChangeControl stubChangeControl(Change c, IdentifiedUser user) { + private ChangeControl stubChangeControl(Change c, IdentifiedUser user) throws OrmException { ChangeControl ctl = EasyMock.createNiceMock(ChangeControl.class); expect(ctl.getChange()).andStubReturn(c); expect(ctl.getCurrentUser()).andStubReturn(user); + ChangeNotes notes = new ChangeNotes(repoManager, c); + notes = notes.load(); + expect(ctl.getNotes()).andStubReturn(notes); EasyMock.replay(ctl); return ctl; }