From fda8c4fc92d0ba7418abaad6ad0a94082f2a0e0a Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Fri, 14 Aug 2020 19:09:47 +0200 Subject: [PATCH] Add backend API and implementation for ported comments When a reviewer leaves comments on a patchset, those comments stay still attached to that patchset when the change owner uploads further patchsets. Replies to such comments (even when newer patchsets are available) are also attached to these older patchsets. This doesn't make a difference for views which simply list comments like the "Comments" tab or the "Change log". However, diff views only show comments of the chosen patchsets. Hence, changes with comments scattered all over different patchsets require quite some navigation. Sometimes, users might even miss some of these comments if they don't go to the right views. Ported comments represent a solution for the described situation. A ported comment is a comment which was left on an earlier patchset and is shown on a later patchset. The idea is that users can simply go to a diff view and see all comments which were left until that patchset. The plan is to make the ported comments distinguishable from the actual comments on the patchset. However, exact details will be left to the frontend implementation. To show the comments left until a patchset, the frontend will need to ask the backend both for the actual comments and the ported comments of that patchset (-> two separate REST API calls). The logic which comments are ported is contained in the backend. The backend always ports a complete comment thread. In the first version of ported comments, we decided to exclude resolved comment threads as showing all comments left on earlier patchsets might produce too much unrelated noise for users. We can improve on that in later versions of this feature. This change still also ports resolved comments as we'll only add the relevant filtering in a follow-up change. Comments on the parent commit of a patchset should also be ported. Those comments require additional considerations and hence are also deferred to a follow-up change. As for other comment endpoints, we distinguish between ported published comments and ported draft comments. The endpoint for the latter ones is added in a follow-up change. This change focuses on adding the new REST endpoint for ported published comments along with its implementation and extensive tests. Adding these detailed tests proved useful as we even noticed a bug in Gerrit's previous code (see I75a65bd57). We also know from past experience with diffs and edits due to rebase that only such extensive tests help to prevent regressions for corner cases. One major part of the implementation of the ported comments endpoint is the mapping of the comment positions from the original patchset to the target patchset. For edits due to rebase, we already have an algorithm to map a diff hunk between two commits to a diff hunk between two other commits. With I4062e6a22, we generalized this algorithm so that we can re-use it to map a comment position in one commit to the corresponding position in another commit. That algorithm currently drops any positions which run into a mapping conflict. Deleted files are also not properly supported. We'll adjust those aspects in follow-up changes. The position transformation algorithm also requires the diff between patchset commits as input. At the moment, we get that diff from the existing diff caches. However, the current structure of the diff caches makes this a very expensive operation as the computed diff encompasses all files between the patchsets. This should improve when we have a new diff cache structure in place which is currently in the making and which allows to only retrieve the diffs of necessary files (= those on which comments were left). Summary of open aspects addressed in follow-up changes: - Filtering of resolved comment threads - Handling of comments on parent commits (= "left side comments") including special considerations for merge commits - Separate endpoint for ported draft comments (implemented in If8802a4f9) - Correct handling for file deletions (implemented in If8ed1e511) - Adjustment of transformation logic to not exclude transformed positions on conflicts and to map those to file comments instead (implemented in I99f80bee3) - Making endpoint more robust by catching diff errors and omitting corresponding comments - REST API documentation (done last when the final state is available) For some of them, we already added tests which we can enable as soon as we have the correct implementation in place. Change-Id: I4e96af5acf428939a3a130c7ca6f6b74d40dab12 --- .../extensions/api/changes/RevisionApi.java | 7 + .../common/testing/CommentInfoSubject.java | 13 + .../server/api/changes/RevisionApiImpl.java | 13 + .../restapi/change/ListPortedComments.java | 189 ++ .../gerrit/server/restapi/change/Module.java | 2 + .../api/revision/PortedCommentsIT.java | 1547 +++++++++++++++++ 6 files changed, 1771 insertions(+) create mode 100644 java/com/google/gerrit/server/restapi/change/ListPortedComments.java create mode 100644 javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java diff --git a/java/com/google/gerrit/extensions/api/changes/RevisionApi.java b/java/com/google/gerrit/extensions/api/changes/RevisionApi.java index ff9fb3c559..9064b97a5a 100644 --- a/java/com/google/gerrit/extensions/api/changes/RevisionApi.java +++ b/java/com/google/gerrit/extensions/api/changes/RevisionApi.java @@ -106,6 +106,8 @@ public interface RevisionApi { List robotCommentsAsList() throws RestApiException; + Map> portedComments() throws RestApiException; + /** * Applies the indicated fix by creating a new change edit or integrating the fix with the * existing change edit. If no change edit exists before this call, the fix must refer to the @@ -293,6 +295,11 @@ public interface RevisionApi { throw new NotImplementedException(); } + @Override + public Map> portedComments() throws RestApiException { + throw new NotImplementedException(); + } + @Override public EditInfo applyFix(String fixId) throws RestApiException { throw new NotImplementedException(); diff --git a/java/com/google/gerrit/extensions/common/testing/CommentInfoSubject.java b/java/com/google/gerrit/extensions/common/testing/CommentInfoSubject.java index 923cfc31f7..c34e4398f1 100644 --- a/java/com/google/gerrit/extensions/common/testing/CommentInfoSubject.java +++ b/java/com/google/gerrit/extensions/common/testing/CommentInfoSubject.java @@ -27,6 +27,7 @@ import com.google.common.truth.Subject; import com.google.gerrit.extensions.client.Side; import com.google.gerrit.extensions.common.CommentInfo; import com.google.gerrit.truth.ListSubject; +import java.sql.Timestamp; import java.util.List; public class CommentInfoSubject extends Subject { @@ -91,6 +92,10 @@ public class CommentInfoSubject extends Subject { return check("inReplyTo").that(commentInfo().inReplyTo); } + public StringSubject commitId() { + return check("commitId").that(commentInfo().commitId); + } + public AccountInfoSubject author() { return check("author").about(accounts()).that(commentInfo().author); } @@ -99,6 +104,14 @@ public class CommentInfoSubject extends Subject { return check("tag").that(commentInfo().tag); } + public ComparableSubject updated() { + return check("updated").that(commentInfo().updated); + } + + public StringSubject changeMessageId() { + return check("changeMessageId").that(commentInfo().changeMessageId); + } + private CommentInfo commentInfo() { isNotNull(); return commentInfo; diff --git a/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java b/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java index b515dfe1bc..df01d8a92b 100644 --- a/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java +++ b/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java @@ -79,6 +79,7 @@ import com.google.gerrit.server.restapi.change.GetMergeList; import com.google.gerrit.server.restapi.change.GetPatch; import com.google.gerrit.server.restapi.change.GetRelated; import com.google.gerrit.server.restapi.change.GetRevisionActions; +import com.google.gerrit.server.restapi.change.ListPortedComments; import com.google.gerrit.server.restapi.change.ListRevisionComments; import com.google.gerrit.server.restapi.change.ListRevisionDrafts; import com.google.gerrit.server.restapi.change.ListRobotComments; @@ -130,6 +131,7 @@ class RevisionApiImpl implements RevisionApi { private final FileApiImpl.Factory fileApi; private final ListRevisionComments listComments; private final ListRobotComments listRobotComments; + private final ListPortedComments listPortedComments; private final ApplyFix applyFix; private final GetFixPreview getFixPreview; private final Fixes fixes; @@ -175,6 +177,7 @@ class RevisionApiImpl implements RevisionApi { FileApiImpl.Factory fileApi, ListRevisionComments listComments, ListRobotComments listRobotComments, + ListPortedComments listPortedComments, ApplyFix applyFix, GetFixPreview getFixPreview, Fixes fixes, @@ -219,6 +222,7 @@ class RevisionApiImpl implements RevisionApi { this.listComments = listComments; this.robotComments = robotComments; this.listRobotComments = listRobotComments; + this.listPortedComments = listPortedComments; this.applyFix = applyFix; this.getFixPreview = getFixPreview; this.fixes = fixes; @@ -453,6 +457,15 @@ class RevisionApiImpl implements RevisionApi { } } + @Override + public Map> portedComments() throws RestApiException { + try { + return listPortedComments.apply(revision).value(); + } catch (Exception e) { + throw asRestApiException("Cannot retrieve ported comments", e); + } + } + @Override public EditInfo applyFix(String fixId) throws RestApiException { try { diff --git a/java/com/google/gerrit/server/restapi/change/ListPortedComments.java b/java/com/google/gerrit/server/restapi/change/ListPortedComments.java new file mode 100644 index 0000000000..3f9e127e84 --- /dev/null +++ b/java/com/google/gerrit/server/restapi/change/ListPortedComments.java @@ -0,0 +1,189 @@ +// Copyright (C) 2020 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.restapi.change; + +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static java.util.stream.Collectors.groupingBy; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.google.gerrit.entities.Comment; +import com.google.gerrit.entities.Comment.Range; +import com.google.gerrit.entities.HumanComment; +import com.google.gerrit.entities.PatchSet; +import com.google.gerrit.entities.Project; +import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace; +import com.google.gerrit.extensions.common.CommentInfo; +import com.google.gerrit.extensions.restapi.Response; +import com.google.gerrit.extensions.restapi.RestReadView; +import com.google.gerrit.server.CommentsUtil; +import com.google.gerrit.server.change.RevisionResource; +import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.patch.DiffMappings; +import com.google.gerrit.server.patch.GitPositionTransformer; +import com.google.gerrit.server.patch.GitPositionTransformer.Mapping; +import com.google.gerrit.server.patch.GitPositionTransformer.Position; +import com.google.gerrit.server.patch.GitPositionTransformer.PositionedEntity; +import com.google.gerrit.server.patch.PatchList; +import com.google.gerrit.server.patch.PatchListCache; +import com.google.gerrit.server.patch.PatchListKey; +import com.google.gerrit.server.patch.PatchListNotAvailableException; +import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.Singleton; +import java.util.List; +import java.util.Map; + +@Singleton +public class ListPortedComments implements RestReadView { + + private final CommentsUtil commentsUtil; + private final Provider commentJson; + private final PatchListCache patchListCache; + + @Inject + public ListPortedComments( + Provider commentJson, CommentsUtil commentsUtil, PatchListCache patchListCache) { + this.commentJson = commentJson; + this.commentsUtil = commentsUtil; + this.patchListCache = patchListCache; + } + + @Override + public Response>> apply(RevisionResource revisionResource) + throws PermissionBackendException, PatchListNotAvailableException { + PatchSet targetPatchset = revisionResource.getPatchSet(); + + List allComments = loadAllPublishedComments(revisionResource); + ImmutableList relevantComments = filterToRelevant(allComments, targetPatchset); + ImmutableList portedComments = + portToTargetPatchset( + revisionResource.getChangeResource().getNotes(), targetPatchset, relevantComments); + return Response.ok(format(portedComments)); + } + + private List loadAllPublishedComments(RevisionResource revisionResource) { + return commentsUtil.publishedHumanCommentsByChange(revisionResource.getNotes()); + } + + private ImmutableList filterToRelevant( + List allComments, PatchSet targetPatchset) { + return allComments.stream() + .filter(comment -> comment.key.patchSetId < targetPatchset.number()) + // TODO(aliceks): Also support comments on parent revisions. + .filter(comment -> comment.side > 0) + .collect(toImmutableList()); + } + + private ImmutableList portToTargetPatchset( + ChangeNotes notes, PatchSet targetPatchset, List comments) + throws PatchListNotAvailableException { + Map> commentsPerPatchset = + comments.stream().collect(groupingBy(comment -> comment.key.patchSetId, toImmutableList())); + + ImmutableList.Builder portedComments = + ImmutableList.builderWithExpectedSize(comments.size()); + for (Integer originalPatchsetId : commentsPerPatchset.keySet()) { + ImmutableList patchsetComments = commentsPerPatchset.get(originalPatchsetId); + PatchSet originalPatchset = + notes.getPatchSets().get(PatchSet.id(notes.getChangeId(), originalPatchsetId)); + portedComments.addAll( + portToTargetPatchset( + notes.getProjectName(), originalPatchset, targetPatchset, patchsetComments)); + } + return portedComments.build(); + } + + private ImmutableList portToTargetPatchset( + Project.NameKey project, + PatchSet originalPatchset, + PatchSet targetPatchset, + ImmutableList comments) + throws PatchListNotAvailableException { + ImmutableSet mappings = + loadPatchsetMappings(project, originalPatchset, targetPatchset); + ImmutableList> positionedComments = + comments.stream().map(this::toPositionedEntity).collect(toImmutableList()); + return GitPositionTransformer.transform(positionedComments, mappings).stream() + .map(PositionedEntity::getEntityAtUpdatedPosition) + .collect(toImmutableList()); + } + + private ImmutableSet loadPatchsetMappings( + Project.NameKey project, PatchSet originalPatchset, PatchSet targetPatchset) + throws PatchListNotAvailableException { + PatchList patchList = + patchListCache.get( + PatchListKey.againstCommit( + originalPatchset.commitId(), targetPatchset.commitId(), Whitespace.IGNORE_NONE), + project); + return patchList.getPatches().stream().map(DiffMappings::toMapping).collect(toImmutableSet()); + } + + private PositionedEntity toPositionedEntity(HumanComment comment) { + return PositionedEntity.create( + comment, + ListPortedComments::extractPosition, + ListPortedComments::createCommentAtNewPosition); + } + + private static Position extractPosition(HumanComment comment) { + Position.Builder positionBuilder = Position.builder().filePath(comment.key.filename); + return positionBuilder.lineRange(extractLineRange(comment)).build(); + } + + private static GitPositionTransformer.Range extractLineRange(HumanComment comment) { + // Line specifications in comment are 1-based. Line specifications in Position are 0-based. + if (comment.range != null) { + // The combination of (line, charOffset) is exclusive and must be mapped to an exclusive line. + int exclusiveEndLine = + comment.range.endChar > 0 ? comment.range.endLine : comment.range.endLine - 1; + return GitPositionTransformer.Range.create(comment.range.startLine - 1, exclusiveEndLine); + } + return GitPositionTransformer.Range.create(comment.lineNbr - 1, comment.lineNbr); + } + + private static HumanComment createCommentAtNewPosition( + HumanComment originalComment, Position newPosition) { + HumanComment portedComment = new HumanComment(originalComment); + portedComment.key.filename = newPosition.filePath(); + portedComment.lineNbr = newPosition.lineRange().start() + 1; + if (portedComment.range != null) { + portedComment.range = + toRange( + newPosition.lineRange(), portedComment.range.startChar, portedComment.range.endChar); + portedComment.lineNbr = portedComment.range.endLine; + } + return portedComment; + } + + private static Comment.Range toRange( + GitPositionTransformer.Range lineRange, int originalStartChar, int originalEndChar) { + int adjustedEndLine = originalEndChar > 0 ? lineRange.end() : lineRange.end() + 1; + return new Range(lineRange.start() + 1, originalStartChar, adjustedEndLine, originalEndChar); + } + + private Map> format(List comments) + throws PermissionBackendException { + return commentJson + .get() + .setFillAccounts(true) + .setFillPatchSet(true) + .newHumanCommentFormatter() + .format(comments); + } +} diff --git a/java/com/google/gerrit/server/restapi/change/Module.java b/java/com/google/gerrit/server/restapi/change/Module.java index 466ea3c3bf..1a4f270057 100644 --- a/java/com/google/gerrit/server/restapi/change/Module.java +++ b/java/com/google/gerrit/server/restapi/change/Module.java @@ -172,6 +172,8 @@ public class Module extends RestApiModule { post(FIX_KIND, "apply").to(ApplyFix.class); get(FIX_KIND, "preview").to(GetFixPreview.class); + get(REVISION_KIND, "ported_comments").to(ListPortedComments.class); + child(REVISION_KIND, "files").to(Files.class); put(FILE_KIND, "reviewed").to(PutReviewed.class); delete(FILE_KIND, "reviewed").to(DeleteReviewed.class); diff --git a/javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java b/javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java new file mode 100644 index 0000000000..7e844b6d9d --- /dev/null +++ b/javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java @@ -0,0 +1,1547 @@ +// Copyright (C) 2020 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.acceptance.api.revision; + +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.Iterables.getOnlyElement; +import static com.google.common.collect.MoreCollectors.onlyElement; +import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.extensions.common.testing.CommentInfoSubject.assertThat; +import static com.google.gerrit.extensions.common.testing.CommentInfoSubject.assertThatList; +import static com.google.gerrit.truth.MapSubject.assertThatMap; + +import com.google.common.collect.ImmutableList; +import com.google.common.truth.Correspondence; +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.testsuite.account.AccountOperations; +import com.google.gerrit.acceptance.testsuite.change.ChangeOperations; +import com.google.gerrit.acceptance.testsuite.change.TestPatchset; +import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations; +import com.google.gerrit.entities.Account; +import com.google.gerrit.entities.Change; +import com.google.gerrit.entities.Patch; +import com.google.gerrit.entities.PatchSet; +import com.google.gerrit.extensions.common.CommentInfo; +import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.truth.NullAwareCorrespondence; +import com.google.inject.Inject; +import java.util.Collection; +import java.util.List; +import java.util.Map; +import org.junit.Ignore; +import org.junit.Test; + +public class PortedCommentsIT extends AbstractDaemonTest { + + @Inject private ChangeOperations changeOps; + @Inject private AccountOperations accountOps; + @Inject private RequestScopeOperations requestScopeOps; + + @Test + public void onlyCommentsBeforeTargetPatchsetArePorted() throws Exception { + // Set up change and patchsets. + Change.Id changeId = changeOps.newChange().create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create(); + PatchSet.Id patchset3Id = changeOps.change(changeId).newPatchset().create(); + // Add comments. + String comment1Uuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create(); + changeOps.change(changeId).patchset(patchset2Id).newComment().create(); + changeOps.change(changeId).patchset(patchset3Id).newComment().create(); + + List portedComments = flatten(getPortedComments(patchset2Id)); + + assertThatList(portedComments).comparingElementsUsing(hasUuid()).containsExactly(comment1Uuid); + } + + @Test + public void commentsOnAnyPatchsetBeforeTargetPatchsetArePorted() throws Exception { + // Set up change and patchsets. + Change.Id changeId = changeOps.newChange().create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + changeOps.change(changeId).newPatchset().create(); + PatchSet.Id patchset3Id = changeOps.change(changeId).newPatchset().create(); + PatchSet.Id patchset4Id = changeOps.change(changeId).newPatchset().create(); + // Add comments. + String comment1Uuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create(); + String comment3Uuid = changeOps.change(changeId).patchset(patchset3Id).newComment().create(); + + List portedComments = flatten(getPortedComments(patchset4Id)); + + assertThat(portedComments) + .comparingElementsUsing(hasUuid()) + .containsExactly(comment1Uuid, comment3Uuid); + } + + @Test + public void severalCommentsFromEarlierPatchsetArePorted() throws Exception { + // Set up change and patchsets. + Change.Id changeId = changeOps.newChange().create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create(); + // Add comments. + String comment1Uuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create(); + String comment2Uuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create(); + + List portedComments = flatten(getPortedComments(patchset2Id)); + + assertThat(portedComments) + .comparingElementsUsing(hasUuid()) + .containsExactly(comment1Uuid, comment2Uuid); + } + + @Test + public void completeCommentThreadIsPorted() throws Exception { + // Set up change and patchsets. + Change.Id changeId = changeOps.newChange().create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create(); + // Add comments. + String rootCommentUuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create(); + String child1CommentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .parentUuid(rootCommentUuid) + .create(); + String child2CommentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .parentUuid(child1CommentUuid) + .create(); + + List portedComments = flatten(getPortedComments(patchset2Id)); + + assertThat(portedComments) + .comparingElementsUsing(hasUuid()) + .containsExactly(rootCommentUuid, child1CommentUuid, child2CommentUuid); + } + + @Test + // TODO(aliceks): Filter out unresolved comment threads. + @Ignore + public void onlyUnresolvedCommentsArePorted() throws Exception { + // Set up change and patchsets. + Change.Id changeId = changeOps.newChange().create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create(); + // Add comments. + changeOps.change(changeId).patchset(patchset1Id).newComment().resolved().create(); + String comment2Uuid = + changeOps.change(changeId).patchset(patchset1Id).newComment().unresolved().create(); + + List portedComments = flatten(getPortedComments(patchset2Id)); + + assertThat(portedComments).comparingElementsUsing(hasUuid()).containsExactly(comment2Uuid); + } + + @Test + // TODO(aliceks): Filter out unresolved comment threads. + @Ignore + public void unresolvedStateOfLastCommentInThreadMatters() throws Exception { + // Set up change and patchsets. + Change.Id changeId = changeOps.newChange().create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create(); + // Add comments. + String rootComment1Uuid = + changeOps.change(changeId).patchset(patchset1Id).newComment().resolved().create(); + String childComment1Uuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .parentUuid(rootComment1Uuid) + .unresolved() + .create(); + String rootComment2Uuid = + changeOps.change(changeId).patchset(patchset1Id).newComment().unresolved().create(); + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .parentUuid(rootComment2Uuid) + .resolved() + .create(); + + List portedComments = flatten(getPortedComments(patchset2Id)); + + assertThat(portedComments) + .comparingElementsUsing(hasUuid()) + .containsExactly(rootComment1Uuid, childComment1Uuid); + } + + @Test + public void unresolvedStateOfLastCommentByDateMattersForBranchedThreads() throws Exception { + // Set up change and patchsets. + Change.Id changeId = changeOps.newChange().create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create(); + // Add comments. + String rootCommentUuid = + changeOps.change(changeId).patchset(patchset1Id).newComment().resolved().create(); + String childComment1Uuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .parentUuid(rootCommentUuid) + .resolved() + .create(); + String childComment2Uuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .parentUuid(rootCommentUuid) + .unresolved() + .create(); + + List portedComments = flatten(getPortedComments(patchset2Id)); + + assertThat(portedComments) + .comparingElementsUsing(hasUuid()) + .containsExactly(rootCommentUuid, childComment1Uuid, childComment2Uuid); + } + + @Test + public void draftCommentsAreNotPortedViaApiForPublishedComments() throws Exception { + Account.Id accountId = accountOps.newAccount().create(); + // Set up change and patchsets. + Change.Id changeId = changeOps.newChange().create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create(); + // Add draft comment. + changeOps.change(changeId).patchset(patchset1Id).newDraftComment().author(accountId).create(); + + // Draft comments are only visible to their author. + requestScopeOps.setApiUser(accountId); + List portedComments = flatten(getPortedComments(patchset2Id)); + + assertThatList(portedComments).isEmpty(); + } + + @Test + public void publishedCommentsOfAllTypesArePorted() throws Exception { + // Set up change and patchsets. + Change.Id changeId = changeOps.newChange().file("myFile").content("Line 1\nLine 2\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create(); + // Add comments. + String rangeCommentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .message("Range comment") + .fromLine(1) + .charOffset(2) + .toLine(2) + .charOffset(1) + .ofFile("myFile") + .create(); + String lineCommentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .message("Line comment") + .onLine(1) + .ofFile("myFile") + .create(); + String fileCommentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .message("File comment") + .onFileLevelOf("myFile") + .create(); + String patchsetLevelCommentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .message("Patchset-level comment") + .onPatchsetLevel() + .create(); + + List portedComments = flatten(getPortedComments(patchset2Id)); + + assertThat(portedComments) + .comparingElementsUsing(hasUuid()) + .containsExactly( + rangeCommentUuid, lineCommentUuid, fileCommentUuid, patchsetLevelCommentUuid); + } + + // This is not the desired behavior but at least a current state which doesn't throw exceptions + // or has wrong behavior. + @Test + public void commentOnParentCommitIsIgnored() throws Exception { + // Set up change and patchsets. + Change.Id changeId = changeOps.newChange().create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create(); + // Add comments. + changeOps.change(changeId).patchset(patchset1Id).newComment().onParentCommit().create(); + + List portedComments = flatten(getPortedComments(patchset2Id)); + + assertThat(portedComments).isEmpty(); + } + + @Test + public void portedCommentHasOriginalUuid() throws Exception { + // Set up change and patchsets. + Change.Id changeId = changeOps.newChange().create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create(); + // Add comment. + String commentUuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create(); + + List portedComments = flatten(getPortedComments(patchset2Id)); + + assertThatList(portedComments).onlyElement().uuid().isEqualTo(commentUuid); + } + + @Test + public void portedCommentHasOriginalPatchset() throws Exception { + // Set up change and patchsets. + Change.Id changeId = changeOps.newChange().create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create(); + // Add comment. + String commentUuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).patchSet().isEqualTo(patchset1Id.get()); + } + + @Test + public void portedCommentHasOriginalPatchsetCommitId() throws Exception { + // Set up change and patchsets. + Change.Id changeId = changeOps.newChange().create(); + TestPatchset patchset1 = changeOps.change(changeId).currentPatchset().get(); + PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create(); + // Add comment. + String commentUuid = + changeOps.change(changeId).patchset(patchset1.patchsetId()).newComment().create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).commitId().isEqualTo(patchset1.commitId().name()); + } + + @Test + public void portedCommentHasOriginalMessage() throws Exception { + // Set up change and patchsets. + Change.Id changeId = changeOps.newChange().create(); + TestPatchset patchset1 = changeOps.change(changeId).currentPatchset().get(); + PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1.patchsetId()) + .newComment() + .message("My comment text") + .create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).message().isEqualTo("My comment text"); + } + + @Test + public void portedReplyStillRefersToParentComment() throws Exception { + // Set up change and patchsets. + Change.Id changeId = changeOps.newChange().create(); + TestPatchset patchset1 = changeOps.change(changeId).currentPatchset().get(); + PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create(); + // Add comments. + String rootCommentUuid = + changeOps.change(changeId).patchset(patchset1.patchsetId()).newComment().create(); + String childCommentUuid = + changeOps + .change(changeId) + .patchset(patchset1.patchsetId()) + .newComment() + .parentUuid(rootCommentUuid) + .create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, childCommentUuid); + + assertThat(portedComment).inReplyTo().isEqualTo(rootCommentUuid); + } + + @Test + public void portedCommentHasOriginalAuthor() throws Exception { + // Set up change and patchsets. + Account.Id authorId = accountOps.newAccount().create(); + Change.Id changeId = changeOps.newChange().create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create(); + // Add comment. + String commentUuid = + changeOps.change(changeId).patchset(patchset1Id).newComment().author(authorId).create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).author().id().isEqualTo(authorId.get()); + } + + @Test + public void portedCommentHasOriginalTag() throws Exception { + // Set up change and patchsets. + Change.Id changeId = changeOps.newChange().create(); + TestPatchset patchset1 = changeOps.change(changeId).currentPatchset().get(); + PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1.patchsetId()) + .newComment() + .tag("My comment tag") + .create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).tag().isEqualTo("My comment tag"); + } + + @Test + public void portedCommentHasUpdatedTimestamp() throws Exception { + // Set up change and patchsets. + Change.Id changeId = changeOps.newChange().create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create(); + // Add comment. + String commentUuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).updated().isNotNull(); + } + + @Test + public void portedCommentDoesNotHaveChangeMessageId() throws Exception { + // Set up change and patchsets. + Change.Id changeId = changeOps.newChange().create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create(); + // Add comment. + String commentUuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + // There's currently no use case for linking ported comments to specific change messages. Hence, + // there's no reason to fill this field, which requires additional computations. + // Besides, we also don't fill this field for the comments requested for a specific patchset. + assertThat(portedComment).changeMessageId().isNull(); + } + + @Test + public void pathOfPortedCommentIsOnlyIndicatedInMap() throws Exception { + // Set up change and patchsets. + Change.Id changeId = changeOps.newChange().file("myFile").content("Line 1").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .onFileLevelOf("myFile") + .create(); + + Map> portedComments = getPortedComments(patchset2Id); + + assertThatMap(portedComments).keys().containsExactly("myFile"); + CommentInfo portedComment = extractSpecificComment(portedComments, commentUuid); + assertThat(portedComment).path().isNull(); + } + + @Test + public void portedRangeCommentCanHandleAddedLines() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .content("Line 1\nLine 1.1\nLine 1.2\nLine 2\nLine 3\nLine 4\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .fromLine(3) + .charOffset(2) + .toLine(4) + .charOffset(5) + .ofFile("myFile") + .create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).range().startLine().isEqualTo(5); + assertThat(portedComment).range().startCharacter().isEqualTo(2); + assertThat(portedComment).range().endLine().isEqualTo(6); + assertThat(portedComment).range().endCharacter().isEqualTo(5); + } + + @Test + public void portedRangeCommentCanHandleDeletedLines() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .content("Line 2\nLine 3\nLine 4\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .fromLine(3) + .charOffset(2) + .toLine(4) + .charOffset(5) + .ofFile("myFile") + .create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).range().startLine().isEqualTo(2); + assertThat(portedComment).range().startCharacter().isEqualTo(2); + assertThat(portedComment).range().endLine().isEqualTo(3); + assertThat(portedComment).range().endCharacter().isEqualTo(5); + } + + @Test + public void portedRangeCommentCanHandlePureRename() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps.change(changeId).newPatchset().file("myFile").renameTo("newFileName").create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .fromLine(3) + .charOffset(2) + .toLine(4) + .charOffset(5) + .ofFile("myFile") + .create(); + + Map> portedComments = getPortedComments(patchset2Id); + + assertThatMap(portedComments).keys().containsExactly("newFileName"); + CommentInfo portedComment = extractSpecificComment(portedComments, commentUuid); + assertThat(portedComment).range().startLine().isEqualTo(3); + assertThat(portedComment).range().startCharacter().isEqualTo(2); + assertThat(portedComment).range().endLine().isEqualTo(4); + assertThat(portedComment).range().endCharacter().isEqualTo(5); + } + + @Test + public void portedRangeCommentCanHandleRenameWithLineShift() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .delete() + .file("newFileName") + .content("Line 1\nLine 1.1\nLine 1.2\nLine 2\nLine 3\nLine 4\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .fromLine(3) + .charOffset(2) + .toLine(4) + .charOffset(5) + .ofFile("myFile") + .create(); + + Map> portedComments = getPortedComments(patchset2Id); + + assertThatMap(portedComments).keys().containsExactly("newFileName"); + CommentInfo portedComment = extractSpecificComment(portedComments, commentUuid); + assertThat(portedComment).range().startLine().isEqualTo(5); + assertThat(portedComment).range().startCharacter().isEqualTo(2); + assertThat(portedComment).range().endLine().isEqualTo(6); + assertThat(portedComment).range().endCharacter().isEqualTo(5); + } + + @Test + public void portedRangeCommentAdditionallyAppearsOnCopyAtIndependentPosition() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + // Gerrit currently only identifies a copy if a rename also happens at the same time. Modify the + // renamed file slightly different than the copied file so that the end location of the comment + // is different. Modify the renamed file less so that Gerrit/Git picks it as the renamed one. + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .delete() + .file("renamedFiled") + .content("Line 1\nLine 1.1\nLine 2\nLine 3\nLine 4\n") + .file("copiedFile") + .content("Line 1\nLine 1.1\nLine 1.2\nLine 2\nLine 3\nLine 4\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .fromLine(3) + .charOffset(2) + .toLine(4) + .charOffset(5) + .ofFile("myFile") + .create(); + + Map> portedComments = getPortedComments(patchset2Id); + + assertThatMap(portedComments).keys().containsExactly("renamedFiled", "copiedFile"); + CommentInfo portedCommentOnRename = getOnlyElement(portedComments.get("renamedFiled")); + assertThat(portedCommentOnRename).uuid().isEqualTo(commentUuid); + assertThat(portedCommentOnRename).range().startLine().isEqualTo(4); + assertThat(portedCommentOnRename).range().startCharacter().isEqualTo(2); + assertThat(portedCommentOnRename).range().endLine().isEqualTo(5); + assertThat(portedCommentOnRename).range().endCharacter().isEqualTo(5); + CommentInfo portedCommentOnCopy = getOnlyElement(portedComments.get("copiedFile")); + assertThat(portedCommentOnCopy).uuid().isEqualTo(commentUuid); + assertThat(portedCommentOnCopy).range().startLine().isEqualTo(5); + assertThat(portedCommentOnCopy).range().startCharacter().isEqualTo(2); + assertThat(portedCommentOnCopy).range().endLine().isEqualTo(6); + assertThat(portedCommentOnCopy).range().endCharacter().isEqualTo(5); + } + + @Test + public void lineOfPortedRangeCommentFollowsContract() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .content("Line 1\nLine 1.1\nLine 1.2\nLine 2\nLine 3\nLine 4\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .fromLine(3) + .charOffset(2) + .toLine(4) + .charOffset(5) + .ofFile("myFile") + .create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + // Line is equal to the end line, which is at 6 when ported. + assertThat(portedComment).line().isEqualTo(6); + } + + @Test + // TODO(aliceks): Adjust code to not ignore conflicts and add mapping to file comments. + @Ignore + public void portedRangeCommentBecomesFileCommentOnConflict() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .content("Line 1\nLine two\nLine three\nLine 4\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .fromLine(2) + .charOffset(2) + .toLine(3) + .charOffset(5) + .ofFile("myFile") + .create(); + + Map> portedComments = getPortedComments(patchset2Id); + + assertThatMap(portedComments).keys().containsExactly("myFile"); + CommentInfo portedComment = extractSpecificComment(portedComments, commentUuid); + assertThat(portedComment).range().isNull(); + assertThat(portedComment).line().isNull(); + } + + @Test + public void portedRangeCommentEndingOnLineJustBeforeModificationCanBePorted() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .content("Line 1\nLine 2\nLine three\nLine 4\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .fromLine(1) + .charOffset(2) + .toLine(2) + .charOffset(5) + .ofFile("myFile") + .create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).range().startLine().isEqualTo(1); + assertThat(portedComment).range().startCharacter().isEqualTo(2); + assertThat(portedComment).range().endLine().isEqualTo(2); + assertThat(portedComment).range().endCharacter().isEqualTo(5); + } + + @Test + public void portedRangeCommentEndingAtStartOfModifiedLineCanBePorted() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .content("Line 1\nLine 2\nLine three\nLine 4\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .fromLine(1) + .charOffset(2) + .toLine(3) + .charOffset(0) + .ofFile("myFile") + .create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).range().startLine().isEqualTo(1); + assertThat(portedComment).range().startCharacter().isEqualTo(2); + assertThat(portedComment).range().endLine().isEqualTo(3); + assertThat(portedComment).range().endCharacter().isEqualTo(0); + } + + @Test + // TODO(aliceks): Adjust code to not ignore conflicts and add mapping to file comments. + @Ignore + public void portedRangeCommentEndingWithinModifiedLineBecomesFileComment() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .content("Line 1\nLine 2\nLine three\nLine 4\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .fromLine(1) + .charOffset(2) + .toLine(3) + .charOffset(4) + .ofFile("myFile") + .create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).range().isNull(); + assertThat(portedComment).line().isNull(); + } + + @Test + // TODO(aliceks): Adjust code to not ignore conflicts and add mapping to file comments. + @Ignore + public void portedRangeCommentWithinModifiedLineBecomesFileComment() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .content("Line 1\nLine 2\nLine three\nLine 4\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .fromLine(3) + .charOffset(2) + .toLine(3) + .charOffset(5) + .ofFile("myFile") + .create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).range().isNull(); + assertThat(portedComment).line().isNull(); + } + + @Test + // TODO(aliceks): Adjust code to not ignore conflicts and add mapping to file comments. + @Ignore + public void portedRangeCommentStartingWithinLastModifiedLineBecomesFileComment() + throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .content("Line one\nLine two\nLine 3\nLine 4\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .fromLine(2) + .charOffset(2) + .toLine(4) + .charOffset(5) + .ofFile("myFile") + .create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).range().isNull(); + assertThat(portedComment).line().isNull(); + } + + @Test + public void portedRangeCommentStartingOnLineJustAfterModificationCanBePorted() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .content("Line 1\nLine two\nLine 3\nLine 4\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .fromLine(3) + .charOffset(2) + .toLine(4) + .charOffset(5) + .ofFile("myFile") + .create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).range().startLine().isEqualTo(3); + assertThat(portedComment).range().startCharacter().isEqualTo(2); + assertThat(portedComment).range().endLine().isEqualTo(4); + assertThat(portedComment).range().endCharacter().isEqualTo(5); + } + + @Test + // TODO(aliceks): Adjust code to not ignore conflicts and only map start/end conflicts to file + // comments. + @Ignore + public void portedRangeCommentStartingBeforeButEndingAfterModifiedLineCanBePorted() + throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .content("Line 1\nLine 2\nLine three\nLine 4\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .fromLine(2) + .charOffset(2) + .toLine(4) + .charOffset(5) + .ofFile("myFile") + .create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).range().startLine().isEqualTo(3); + assertThat(portedComment).range().startCharacter().isEqualTo(2); + assertThat(portedComment).range().endLine().isEqualTo(4); + assertThat(portedComment).range().endCharacter().isEqualTo(5); + } + + @Test + // TODO(aliceks): Correct handling of deleted files. + @Ignore + public void portedRangeCommentBecomesPatchsetLevelCommentOnFileDeletion() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps.change(changeId).newPatchset().file("myFile").delete().create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .fromLine(3) + .charOffset(2) + .toLine(4) + .charOffset(5) + .ofFile("myFile") + .create(); + + Map> portedComments = getPortedComments(patchset2Id); + assertThatMap(portedComments).keys().containsExactly(Patch.PATCHSET_LEVEL); + CommentInfo portedComment = extractSpecificComment(portedComments, commentUuid); + assertThat(portedComment).range().isNull(); + assertThat(portedComment).line().isNull(); + } + + @Test + public void portedLineCommentCanHandleAddedLines() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .content("Line 1\nLine 1.1\nLine 1.2\nLine 2\nLine 3\nLine 4\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .onLine(3) + .ofFile("myFile") + .create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).line().isEqualTo(5); + } + + @Test + public void portedLineCommentCanHandleDeletedLines() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .content("Line 2\nLine 3\nLine 4\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .onLine(3) + .ofFile("myFile") + .create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).line().isEqualTo(2); + } + + @Test + public void portedLineCommentCanHandlePureRename() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps.change(changeId).newPatchset().file("myFile").renameTo("newFileName").create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .onLine(3) + .ofFile("myFile") + .create(); + + Map> portedComments = getPortedComments(patchset2Id); + + assertThatMap(portedComments).keys().containsExactly("newFileName"); + CommentInfo portedComment = extractSpecificComment(portedComments, commentUuid); + assertThat(portedComment).line().isEqualTo(3); + } + + @Test + public void portedLineCommentCanHandleRenameWithLineShift() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .content("Line 1\nLine 1.1\nLine 1.2\nLine 2\nLine 3\nLine 4\n") + .create(); + PatchSet.Id patchset3Id = + changeOps.change(changeId).newPatchset().file("myFile").renameTo("newFileName").create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .onLine(3) + .ofFile("myFile") + .create(); + + Map> portedComments = getPortedComments(patchset3Id); + + assertThatMap(portedComments).keys().containsExactly("newFileName"); + CommentInfo portedComment = extractSpecificComment(portedComments, commentUuid); + assertThat(portedComment).line().isEqualTo(5); + } + + @Test + public void portedLineCommentAdditionallyAppearsOnCopyAtIndependentPosition() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + // Gerrit currently only identifies a copy if a rename also happens at the same time. Modify the + // renamed file slightly different than the copied file so that the end location of the comment + // is different. Modify the renamed file less so that Gerrit/Git picks it as the renamed one. + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .delete() + .file("renamedFiled") + .content("Line 1\nLine 1.1\nLine 2\nLine 3\nLine 4\n") + .file("copiedFile") + .content("Line 1\nLine 1.1\nLine 1.2\nLine 2\nLine 3\nLine 4\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .onLine(3) + .ofFile("myFile") + .create(); + + Map> portedComments = getPortedComments(patchset2Id); + + assertThatMap(portedComments).keys().containsExactly("renamedFiled", "copiedFile"); + CommentInfo portedCommentOnRename = getOnlyElement(portedComments.get("renamedFiled")); + assertThat(portedCommentOnRename).uuid().isEqualTo(commentUuid); + assertThat(portedCommentOnRename).line().isEqualTo(4); + CommentInfo portedCommentOnCopy = getOnlyElement(portedComments.get("copiedFile")); + assertThat(portedCommentOnCopy).uuid().isEqualTo(commentUuid); + assertThat(portedCommentOnCopy).line().isEqualTo(5); + } + + @Test + // TODO(aliceks): Adjust code to not ignore conflicts and add mapping to file comments. + @Ignore + public void portedLineCommentBecomesFileCommentOnConflict() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .content("Line 1\nLine two\nLine three\nLine 4\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .onLine(2) + .ofFile("myFile") + .create(); + + Map> portedComments = getPortedComments(patchset2Id); + + assertThatMap(portedComments).keys().containsExactly("myFile"); + CommentInfo portedComment = extractSpecificComment(portedComments, commentUuid); + assertThat(portedComment).line().isNull(); + } + + @Test + public void portedLineCommentOnLineJustBeforeModificationCanBePorted() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .content("Line 1\nLine 2\nLine three\nLine 4\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .onLine(2) + .ofFile("myFile") + .create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).line().isEqualTo(2); + } + + @Test + // TODO(aliceks): Adjust code to not ignore conflicts and add mapping to file comments. + @Ignore + public void portedLineCommentOnStartLineOfModificationBecomesFileComment() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .content("Line 1\nLine 2\nSome completely\ndifferent\ncontent\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .onLine(3) + .ofFile("myFile") + .create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).line().isNull(); + } + + @Test + // TODO(aliceks): Adjust code to not ignore conflicts and add mapping to file comments. + @Ignore + public void portedLineCommentOnLastLineOfModificationBecomesFileComment() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .content("Line 1\nLine 2\nSome completely\ndifferent\ncontent\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .onLine(4) + .ofFile("myFile") + .create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).line().isNull(); + } + + @Test + public void portedLineCommentOnLineJustAfterModificationCanBePorted() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .content("Line 1\nLine 2\nLine three\nLine 4\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .onLine(4) + .ofFile("myFile") + .create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).line().isEqualTo(4); + } + + @Test + // TODO(aliceks): Correct handling of deleted files. + @Ignore + public void portedLineCommentBecomesPatchsetLevelCommentOnFileDeletion() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps.change(changeId).newPatchset().file("myFile").delete().create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .onLine(3) + .ofFile("myFile") + .create(); + + Map> portedComments = getPortedComments(patchset2Id); + assertThatMap(portedComments).keys().containsExactly(Patch.PATCHSET_LEVEL); + CommentInfo portedComment = extractSpecificComment(portedComments, commentUuid); + assertThat(portedComment).range().isNull(); + assertThat(portedComment).line().isNull(); + } + + @Test + public void portedFileCommentIsObliviousToAdjustedFileContent() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .content("Line 1\nLine 1.1\nLine 1.2\nLine 2\nLine 3\nLine 4\n") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .onFileLevelOf("myFile") + .create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).line().isNull(); + } + + @Test + public void portedFileCommentCanHandleRename() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps.change(changeId).newPatchset().file("myFile").renameTo("newFileName").create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .onFileLevelOf("myFile") + .create(); + + Map> portedComments = getPortedComments(patchset2Id); + + assertThatMap(portedComments).keys().containsExactly("newFileName"); + CommentInfo portedComment = extractSpecificComment(portedComments, commentUuid); + assertThat(portedComment).line().isNull(); + } + + @Test + public void portedFileCommentAdditionallyAppearsOnCopy() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .renameTo("renamedFiled") + .file("copiedFile") + .content("Line 1\nLine 2\nLine 3\nLine 4\n") + .create(); + // Add comment. + changeOps.change(changeId).patchset(patchset1Id).newComment().onFileLevelOf("myFile").create(); + + Map> portedComments = getPortedComments(patchset2Id); + + assertThatMap(portedComments).keys().containsExactly("renamedFiled", "copiedFile"); + CommentInfo portedCommentOnCopy = getOnlyElement(portedComments.get("copiedFile")); + assertThat(portedCommentOnCopy).line().isNull(); + } + + @Test + // TODO(aliceks): Correct handling of deleted files. + @Ignore + public void portedFileCommentBecomesPatchsetLevelCommentOnFileDeletion() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps.change(changeId).newPatchset().file("myFile").delete().create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + .onFileLevelOf("myFile") + .create(); + + Map> portedComments = getPortedComments(patchset2Id); + assertThatMap(portedComments).keys().containsExactly(Patch.PATCHSET_LEVEL); + CommentInfo portedComment = extractSpecificComment(portedComments, commentUuid); + assertThat(portedComment).range().isNull(); + assertThat(portedComment).line().isNull(); + } + + @Test + public void portedPatchsetLevelCommentIsObliviousToAdjustedFileContent() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .file("myFile") + .content("Line 1\nLine 1.1\nLine 1.2\nLine 2\nLine 3\nLine 4\n") + .create(); + // Add comment. + changeOps.change(changeId).patchset(patchset1Id).newComment().onPatchsetLevel().create(); + + Map> portedComments = getPortedComments(patchset2Id); + + assertThatMap(portedComments).keys().containsExactly(Patch.PATCHSET_LEVEL); + } + + @Test + public void portedPatchsetLevelCommentIsObliviousToRename() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().file("myFile").content("Line 1\nLine 2\nLine 3\nLine 4\n").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps.change(changeId).newPatchset().file("myFile").renameTo("newFileName").create(); + // Add comment. + changeOps.change(changeId).patchset(patchset1Id).newComment().onPatchsetLevel().create(); + + Map> portedComments = getPortedComments(patchset2Id); + + assertThatMap(portedComments).keys().containsExactly(Patch.PATCHSET_LEVEL); + } + + @Test + public void lineCommentOnCommitMessageIsPortedToNewPosition() throws Exception { + // Set up change and patchsets. + Change.Id changeId = + changeOps.newChange().commitMessage("Summary line\n\nText 1\nText 2").create(); + PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId(); + PatchSet.Id patchset2Id = + changeOps + .change(changeId) + .newPatchset() + .commitMessage("Summary line\n\nText 1\nText 1.1\nText 2") + .create(); + // Add comment. + String commentUuid = + changeOps + .change(changeId) + .patchset(patchset1Id) + .newComment() + // The /COMMIT_MSG file has a header of 6 lines, so the summary line is in line 7. + // Place comment on 'Text 2' which is line 10. + .onLine(10) + .ofFile(Patch.COMMIT_MSG) + .create(); + + CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid); + + assertThat(portedComment).line().isEqualTo(11); + } + + private CommentInfo getPortedComment(PatchSet.Id patchsetId, String commentUuid) + throws RestApiException { + Map> portedComments = getPortedComments(patchsetId); + return extractSpecificComment(portedComments, commentUuid); + } + + private Map> getPortedComments(PatchSet.Id patchsetId) + throws RestApiException { + return gApi.changes() + .id(patchsetId.changeId().get()) + .revision(patchsetId.get()) + .portedComments(); + } + + private static CommentInfo extractSpecificComment( + Map> portedComments, String commentUuid) { + return portedComments.values().stream() + .flatMap(Collection::stream) + .filter(comment -> comment.id.equals(commentUuid)) + .collect(onlyElement()); + } + + /** + * Returns all comments in one list. The map keys (= file paths) are simply ignored. The returned + * comments won't have the file path attribute set for them as they came from a map with that + * attribute as key (= established Gerrit behavior). + */ + private static ImmutableList flatten( + Map> commentsPerFile) { + return commentsPerFile.values().stream() + .flatMap(Collection::stream) + .collect((toImmutableList())); + } + + // Unfortunately, we don't get an absolutely helpful error message when using this correspondence + // as CommentInfo doesn't have a toString() implementation. Even if we added it, the string + // representation would be quite unwieldy due to the huge number of comment attributes. + // Interestingly, using Correspondence#formattingDiffsUsing didn't improve anything. + private static Correspondence hasUuid() { + return NullAwareCorrespondence.transforming(comment -> comment.id, "hasUuid"); + } +}