Merge changes Iad263b16,If74c346e,I5682bf9b
* changes:
CommentsUtil: Don't use the diff cache to look up the parent commit
Cover some more behaviors of ported comments with tests
Ignore comments on non-existing patchsets when porting
diff --git a/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java b/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java
index 465c419..b8c841c 100644
--- a/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java
+++ b/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java
@@ -14,8 +14,6 @@
package com.google.gerrit.acceptance.testsuite.change;
-import static com.google.gerrit.server.CommentsUtil.setCommentCommitId;
-
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Comment.Status;
import com.google.gerrit.entities.HumanComment;
@@ -32,8 +30,6 @@
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
-import com.google.gerrit.server.patch.PatchListCache;
-import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -58,7 +54,6 @@
private final IdentifiedUser.GenericFactory userFactory;
private final BatchUpdate.Factory batchUpdateFactory;
private final CommentsUtil commentsUtil;
- private final PatchListCache patchListCache;
private final ChangeNotes changeNotes;
private final PatchSet.Id patchsetId;
@@ -73,14 +68,12 @@
GenericFactory userFactory,
BatchUpdate.Factory batchUpdateFactory,
CommentsUtil commentsUtil,
- PatchListCache patchListCache,
@Assisted ChangeNotes changeNotes,
@Assisted PatchSet.Id patchsetId) {
this.repositoryManager = repositoryManager;
this.userFactory = userFactory;
this.batchUpdateFactory = batchUpdateFactory;
this.commentsUtil = commentsUtil;
- this.patchListCache = patchListCache;
this.changeNotes = changeNotes;
this.patchsetId = patchsetId;
}
@@ -154,7 +147,7 @@
}
@Override
- public boolean updateChange(ChangeContext context) throws Exception {
+ public boolean updateChange(ChangeContext context) {
HumanComment comment = toNewComment(context, commentCreation);
ChangeUpdate changeUpdate = context.getUpdate(patchsetId);
changeUpdate.putComment(commentCreation.status(), comment);
@@ -165,8 +158,7 @@
return true;
}
- private HumanComment toNewComment(ChangeContext context, TestCommentCreation commentCreation)
- throws PatchListNotAvailableException {
+ private HumanComment toNewComment(ChangeContext context, TestCommentCreation commentCreation) {
String message = commentCreation.message().orElse("The text of a test comment.");
String filePath = commentCreation.file().orElse(Patch.PATCHSET_LEVEL);
@@ -197,11 +189,8 @@
.map(PerPatchsetOperationsImpl::toCommentRange)
.ifPresent(range -> newComment.setLineNbrAndRange(null, range));
- setCommentCommitId(
- newComment,
- patchListCache,
- context.getChange(),
- changeNotes.getPatchSets().get(patchsetId));
+ commentsUtil.setCommentCommitId(
+ newComment, context.getChange(), changeNotes.getPatchSets().get(patchsetId));
return newComment;
}
}
@@ -236,7 +225,7 @@
}
@Override
- public boolean updateChange(ChangeContext context) throws Exception {
+ public boolean updateChange(ChangeContext context) {
RobotComment robotComment = toNewRobotComment(context, robotCommentCreation);
ChangeUpdate changeUpdate = context.getUpdate(patchsetId);
changeUpdate.putRobotComment(robotComment);
@@ -248,8 +237,7 @@
}
private RobotComment toNewRobotComment(
- ChangeContext context, TestRobotCommentCreation robotCommentCreation)
- throws PatchListNotAvailableException {
+ ChangeContext context, TestRobotCommentCreation robotCommentCreation) {
String message = robotCommentCreation.message().orElse("The text of a test robot comment.");
String filePath = robotCommentCreation.file().orElse(Patch.PATCHSET_LEVEL);
@@ -281,11 +269,8 @@
newRobotComment.properties = robotCommentCreation.properties();
}
- setCommentCommitId(
- newRobotComment,
- patchListCache,
- context.getChange(),
- changeNotes.getPatchSets().get(patchsetId));
+ commentsUtil.setCommentCommitId(
+ newRobotComment, context.getChange(), changeNotes.getPatchSets().get(patchsetId));
return newRobotComment;
}
}
diff --git a/java/com/google/gerrit/server/CommentsUtil.java b/java/com/google/gerrit/server/CommentsUtil.java
index 4d5778a..b752791 100644
--- a/java/com/google/gerrit/server/CommentsUtil.java
+++ b/java/com/google/gerrit/server/CommentsUtil.java
@@ -31,6 +31,7 @@
import com.google.gerrit.entities.HumanComment;
import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.entities.RobotComment;
import com.google.gerrit.exceptions.StorageException;
@@ -55,6 +56,7 @@
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
/** Utility functions to manipulate Comments. */
@Singleton
@@ -109,13 +111,18 @@
private final GitRepositoryManager repoManager;
private final AllUsersName allUsers;
private final String serverId;
+ private final PatchListCache patchListCache;
@Inject
CommentsUtil(
- GitRepositoryManager repoManager, AllUsersName allUsers, @GerritServerId String serverId) {
+ GitRepositoryManager repoManager,
+ AllUsersName allUsers,
+ @GerritServerId String serverId,
+ PatchListCache patchListCache) {
this.repoManager = repoManager;
this.allUsers = allUsers;
this.serverId = serverId;
+ this.patchListCache = patchListCache;
}
public HumanComment newHumanComment(
@@ -372,41 +379,63 @@
return sort(result);
}
- public static void setCommentCommitId(Comment c, PatchListCache cache, Change change, PatchSet ps)
- throws PatchListNotAvailableException {
+ public void setCommentCommitId(Comment c, Change change, PatchSet ps) {
checkArgument(
c.key.patchSetId == ps.id().get(),
"cannot set commit ID for patch set %s on comment %s",
ps.id(),
c);
if (c.getCommitId() == null) {
- c.setCommitId(determineCommitId(cache, change, ps, c.side));
+ // This code is very much down into our stack and shouldn't be used for validation. Hence,
+ // don't throw an exception here if we can't find a commit for the indicated side but
+ // simply use the all-null ObjectId.
+ c.setCommitId(determineCommitId(change, ps, c.side).orElseGet(ObjectId::zeroId));
}
}
/**
* Determines the SHA-1 of the commit referenced by the (change, patchset, side) triple.
*
- * @param patchListCache the cache to use for SHA-1 lookups
* @param change the change to which the commit belongs
* @param patchset the patchset to which the commit belongs
* @param side the side indicating which commit of the patchset to take. 1 is the patchset commit,
* 0 the parent commit (or auto-merge for changes representing merge commits); -x the xth
* parent commit of a merge commit
- * @return the commit SHA-1
- * @throws PatchListNotAvailableException if the SHA-1 is unavailable for an unknown reason
+ * @return the commit SHA-1 or an empty {@link Optional} if the side isn't available for the given
+ * change/patchset
+ * @throws StorageException if the SHA-1 is unavailable for an unknown reason
*/
- public static ObjectId determineCommitId(
- PatchListCache patchListCache, Change change, PatchSet patchset, short side)
- throws PatchListNotAvailableException {
+ public Optional<ObjectId> determineCommitId(Change change, PatchSet patchset, short side) {
if (Side.fromShort(side) == Side.PARENT) {
if (side < 0) {
- return patchListCache.getOldId(change, patchset, -side);
- } else {
- return patchListCache.getOldId(change, patchset, null);
+ int parentNumber = Math.abs(side);
+ return resolveParentCommit(change.getProject(), patchset, parentNumber);
}
- } else {
- return patchset.commitId();
+ return Optional.of(resolveAutoMergeCommit(change, patchset));
+ }
+ return Optional.of(patchset.commitId());
+ }
+
+ private Optional<ObjectId> resolveParentCommit(
+ Project.NameKey project, PatchSet patchset, int parentNumber) {
+ try (Repository repository = repoManager.openRepository(project)) {
+ RevCommit commit = repository.parseCommit(patchset.commitId());
+ if (commit.getParentCount() < parentNumber) {
+ return Optional.empty();
+ }
+ return Optional.of(commit.getParent(parentNumber - 1));
+ } catch (IOException e) {
+ throw new StorageException(e);
+ }
+ }
+
+ private ObjectId resolveAutoMergeCommit(Change change, PatchSet patchset) {
+ try {
+ // TODO(ghareeb): Adjust after the auto-merge code was moved out of the diff caches. Also
+ // unignore the test in PortedCommentsIT.
+ return patchListCache.getOldId(change, patchset, null);
+ } catch (PatchListNotAvailableException e) {
+ throw new StorageException(e);
}
}
diff --git a/java/com/google/gerrit/server/PublishCommentUtil.java b/java/com/google/gerrit/server/PublishCommentUtil.java
index 658af15..4d19dd0 100644
--- a/java/com/google/gerrit/server/PublishCommentUtil.java
+++ b/java/com/google/gerrit/server/PublishCommentUtil.java
@@ -22,15 +22,12 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.HumanComment;
import com.google.gerrit.entities.PatchSet;
-import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.validators.CommentForValidation;
import com.google.gerrit.extensions.validators.CommentValidationContext;
import com.google.gerrit.extensions.validators.CommentValidationFailure;
import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
-import com.google.gerrit.server.patch.PatchListCache;
-import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.plugincontext.PluginSetContext;
import com.google.gerrit.server.update.ChangeContext;
import com.google.inject.Inject;
@@ -44,16 +41,13 @@
public class PublishCommentUtil {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
- private final PatchListCache patchListCache;
private final PatchSetUtil psUtil;
private final CommentsUtil commentsUtil;
@Inject
- PublishCommentUtil(
- CommentsUtil commentsUtil, PatchListCache patchListCache, PatchSetUtil psUtil) {
+ PublishCommentUtil(CommentsUtil commentsUtil, PatchSetUtil psUtil) {
this.commentsUtil = commentsUtil;
this.psUtil = psUtil;
- this.patchListCache = patchListCache;
}
public void publish(
@@ -101,11 +95,7 @@
// Draft may have been created by a different real user; copy the current real user. (Only
// applies to X-Gerrit-RunAs, since modifying drafts via on_behalf_of is not allowed.)
ctx.getUser().updateRealAccountId(draftComment::setRealAuthor);
- try {
- CommentsUtil.setCommentCommitId(draftComment, patchListCache, notes.getChange(), ps);
- } catch (PatchListNotAvailableException e) {
- throw new StorageException(e);
- }
+ commentsUtil.setCommentCommitId(draftComment, notes.getChange(), ps);
commentsToPublish.add(draftComment);
}
commentsUtil.putHumanComments(changeUpdate, HumanComment.Status.PUBLISHED, commentsToPublish);
diff --git a/java/com/google/gerrit/server/mail/receive/MailProcessor.java b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
index aea5308..df38118 100644
--- a/java/com/google/gerrit/server/mail/receive/MailProcessor.java
+++ b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
@@ -59,8 +59,6 @@
import com.google.gerrit.server.mail.send.InboundEmailRejectionSender;
import com.google.gerrit.server.mail.send.MessageIdGenerator;
import com.google.gerrit.server.notedb.ChangeNotes;
-import com.google.gerrit.server.patch.PatchListCache;
-import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.plugincontext.PluginSetContext;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
@@ -106,7 +104,6 @@
private final ChangeMessagesUtil changeMessagesUtil;
private final CommentsUtil commentsUtil;
private final OneOffRequestContext oneOffRequestContext;
- private final PatchListCache patchListCache;
private final PatchSetUtil psUtil;
private final Provider<InternalChangeQuery> queryProvider;
private final DynamicMap<MailFilter> mailFilters;
@@ -126,7 +123,6 @@
ChangeMessagesUtil changeMessagesUtil,
CommentsUtil commentsUtil,
OneOffRequestContext oneOffRequestContext,
- PatchListCache patchListCache,
PatchSetUtil psUtil,
Provider<InternalChangeQuery> queryProvider,
DynamicMap<MailFilter> mailFilters,
@@ -143,7 +139,6 @@
this.changeMessagesUtil = changeMessagesUtil;
this.commentsUtil = commentsUtil;
this.oneOffRequestContext = oneOffRequestContext;
- this.patchListCache = patchListCache;
this.psUtil = psUtil;
this.queryProvider = queryProvider;
this.mailFilters = mailFilters;
@@ -330,8 +325,7 @@
}
@Override
- public boolean updateChange(ChangeContext ctx)
- throws UnprocessableEntityException, PatchListNotAvailableException {
+ public boolean updateChange(ChangeContext ctx) throws UnprocessableEntityException {
patchSet = psUtil.get(ctx.getNotes(), psId);
notes = ctx.getNotes();
if (patchSet == null) {
@@ -419,8 +413,7 @@
}
private HumanComment persistentCommentFromMailComment(
- ChangeContext ctx, MailComment mailComment, PatchSet patchSetForComment)
- throws PatchListNotAvailableException {
+ ChangeContext ctx, MailComment mailComment, PatchSet patchSetForComment) {
String fileName;
// The patch set that this comment is based on is different if this
// comment was sent in reply to a comment on a previous patch set.
@@ -452,7 +445,7 @@
comment.range = mailComment.getInReplyTo().range;
comment.unresolved = mailComment.getInReplyTo().unresolved;
}
- CommentsUtil.setCommentCommitId(comment, patchListCache, ctx.getChange(), patchSetForComment);
+ commentsUtil.setCommentCommitId(comment, ctx.getChange(), patchSetForComment);
return comment;
}
}
diff --git a/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java b/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java
index f29c6e6..70d1eaf 100644
--- a/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java
+++ b/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java
@@ -15,7 +15,6 @@
package com.google.gerrit.server.restapi.account;
import static com.google.common.collect.ImmutableList.toImmutableList;
-import static com.google.gerrit.server.CommentsUtil.setCommentCommitId;
import com.google.common.base.CharMatcher;
import com.google.common.base.Strings;
@@ -40,8 +39,6 @@
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.account.AccountResource;
import com.google.gerrit.server.change.ChangeJson;
-import com.google.gerrit.server.patch.PatchListCache;
-import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
@@ -80,7 +77,6 @@
private final Provider<CommentJson> commentJsonProvider;
private final CommentsUtil commentsUtil;
private final PatchSetUtil psUtil;
- private final PatchListCache patchListCache;
@Inject
DeleteDraftComments(
@@ -92,8 +88,7 @@
ChangeJson.Factory changeJsonFactory,
Provider<CommentJson> commentJsonProvider,
CommentsUtil commentsUtil,
- PatchSetUtil psUtil,
- PatchListCache patchListCache) {
+ PatchSetUtil psUtil) {
this.userProvider = userProvider;
this.batchUpdateFactory = batchUpdateFactory;
this.queryBuilderProvider = queryBuilderProvider;
@@ -103,7 +98,6 @@
this.commentJsonProvider = commentJsonProvider;
this.commentsUtil = commentsUtil;
this.psUtil = psUtil;
- this.patchListCache = patchListCache;
}
@Override
@@ -176,14 +170,13 @@
}
@Override
- public boolean updateChange(ChangeContext ctx)
- throws PatchListNotAvailableException, PermissionBackendException {
+ public boolean updateChange(ChangeContext ctx) throws PermissionBackendException {
ImmutableList.Builder<CommentInfo> comments = ImmutableList.builder();
boolean dirty = false;
for (HumanComment c : commentsUtil.draftByChangeAuthor(ctx.getNotes(), accountId)) {
dirty = true;
PatchSet.Id psId = PatchSet.id(ctx.getChange().getId(), c.key.patchSetId);
- setCommentCommitId(c, patchListCache, ctx.getChange(), psUtil.get(ctx.getNotes(), psId));
+ commentsUtil.setCommentCommitId(c, ctx.getChange(), psUtil.get(ctx.getNotes(), psId));
commentsUtil.deleteHumanComments(ctx.getUpdate(psId), Collections.singleton(c));
comments.add(humanCommentFormatter.format(c));
}
diff --git a/java/com/google/gerrit/server/restapi/change/CommentPorter.java b/java/com/google/gerrit/server/restapi/change/CommentPorter.java
index 05241e2..e2abf29 100644
--- a/java/com/google/gerrit/server/restapi/change/CommentPorter.java
+++ b/java/com/google/gerrit/server/restapi/change/CommentPorter.java
@@ -67,10 +67,12 @@
private final GitPositionTransformer positionTransformer =
new GitPositionTransformer(BestPositionOnConflict.INSTANCE);
private final PatchListCache patchListCache;
+ private final CommentsUtil commentsUtil;
@Inject
- public CommentPorter(PatchListCache patchListCache) {
+ public CommentPorter(PatchListCache patchListCache, CommentsUtil commentsUtil) {
this.patchListCache = patchListCache;
+ this.commentsUtil = commentsUtil;
}
/**
@@ -131,13 +133,21 @@
ImmutableList<HumanComment> patchsetComments = commentsPerPatchset.get(originalPatchsetId);
PatchSet originalPatchset =
notes.getPatchSets().get(PatchSet.id(notes.getChangeId(), originalPatchsetId));
- portedComments.addAll(
- portSamePatchset(
- notes.getProjectName(),
- notes.getChange(),
- originalPatchset,
- targetPatchset,
- patchsetComments));
+ if (originalPatchset != null) {
+ portedComments.addAll(
+ portSamePatchset(
+ notes.getProjectName(),
+ notes.getChange(),
+ originalPatchset,
+ targetPatchset,
+ patchsetComments));
+ } else {
+ logger.atWarning().log(
+ String.format(
+ "Some comments which should be ported refer to the non-existent patchset %s of"
+ + " change %d. Omitting %d affected comments.",
+ originalPatchsetId, notes.getChangeId().get(), patchsetComments.size()));
+ }
}
return portedComments.build();
}
@@ -200,13 +210,22 @@
PatchSet targetPatchset,
short side)
throws PatchListNotAvailableException {
- ObjectId originalCommit =
- CommentsUtil.determineCommitId(patchListCache, change, originalPatchset, side);
- ObjectId targetCommit =
- CommentsUtil.determineCommitId(patchListCache, change, targetPatchset, side);
+ ObjectId originalCommit = determineCommitId(change, originalPatchset, side);
+ ObjectId targetCommit = determineCommitId(change, targetPatchset, side);
return loadCommitMappings(project, originalCommit, targetCommit);
}
+ private ObjectId determineCommitId(Change change, PatchSet patchset, short side) {
+ return commentsUtil
+ .determineCommitId(change, patchset, side)
+ .orElseThrow(
+ () ->
+ new IllegalStateException(
+ String.format(
+ "Commit indicated by change %d, patchset %d, side %d doesn't exist.",
+ change.getId().get(), patchset.id().get(), side)));
+ }
+
private ImmutableSet<Mapping> loadCommitMappings(
Project.NameKey project, ObjectId originalCommit, ObjectId targetCommit)
throws PatchListNotAvailableException {
@@ -277,6 +296,11 @@
// No line -> use 0 = file comment or any other comment type without an explicit line.
portedComment.lineNbr = newPosition.lineRange().map(range -> range.start() + 1).orElse(0);
}
+ if (Patch.PATCHSET_LEVEL.equals(portedComment.key.filename)) {
+ // Correct the side of the comment to Side.REVISION (= 1) if the comment was changed to
+ // patchset level.
+ portedComment.side = 1;
+ }
return portedComment;
}
diff --git a/java/com/google/gerrit/server/restapi/change/CreateDraftComment.java b/java/com/google/gerrit/server/restapi/change/CreateDraftComment.java
index 399da8f..8476767 100644
--- a/java/com/google/gerrit/server/restapi/change/CreateDraftComment.java
+++ b/java/com/google/gerrit/server/restapi/change/CreateDraftComment.java
@@ -15,7 +15,6 @@
package com.google.gerrit.server.restapi.change;
import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL;
-import static com.google.gerrit.server.CommentsUtil.setCommentCommitId;
import com.google.common.base.Strings;
import com.google.gerrit.entities.HumanComment;
@@ -32,8 +31,6 @@
import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.change.RevisionResource;
-import com.google.gerrit.server.patch.PatchListCache;
-import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
@@ -51,20 +48,17 @@
private final Provider<CommentJson> commentJson;
private final CommentsUtil commentsUtil;
private final PatchSetUtil psUtil;
- private final PatchListCache patchListCache;
@Inject
CreateDraftComment(
BatchUpdate.Factory updateFactory,
Provider<CommentJson> commentJson,
CommentsUtil commentsUtil,
- PatchSetUtil psUtil,
- PatchListCache patchListCache) {
+ PatchSetUtil psUtil) {
this.updateFactory = updateFactory;
this.commentJson = commentJson;
this.commentsUtil = commentsUtil;
this.psUtil = psUtil;
- this.patchListCache = patchListCache;
}
@Override
@@ -111,8 +105,7 @@
@Override
public boolean updateChange(ChangeContext ctx)
- throws ResourceNotFoundException, UnprocessableEntityException,
- PatchListNotAvailableException {
+ throws ResourceNotFoundException, UnprocessableEntityException {
PatchSet ps = psUtil.get(ctx.getNotes(), psId);
if (ps == null) {
throw new ResourceNotFoundException("patch set not found: " + psId);
@@ -133,7 +126,7 @@
comment.setLineNbrAndRange(in.line, in.range);
comment.tag = in.tag;
- setCommentCommitId(comment, patchListCache, ctx.getChange(), ps);
+ commentsUtil.setCommentCommitId(comment, ctx.getChange(), ps);
commentsUtil.putHumanComments(
ctx.getUpdate(psId), HumanComment.Status.DRAFT, Collections.singleton(comment));
diff --git a/java/com/google/gerrit/server/restapi/change/DeleteDraftComment.java b/java/com/google/gerrit/server/restapi/change/DeleteDraftComment.java
index 71fd4d2..51a0b8e 100644
--- a/java/com/google/gerrit/server/restapi/change/DeleteDraftComment.java
+++ b/java/com/google/gerrit/server/restapi/change/DeleteDraftComment.java
@@ -14,8 +14,6 @@
package com.google.gerrit.server.restapi.change;
-import static com.google.gerrit.server.CommentsUtil.setCommentCommitId;
-
import com.google.gerrit.entities.Comment;
import com.google.gerrit.entities.HumanComment;
import com.google.gerrit.entities.PatchSet;
@@ -28,8 +26,6 @@
import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.change.DraftCommentResource;
-import com.google.gerrit.server.patch.PatchListCache;
-import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -45,18 +41,13 @@
private final BatchUpdate.Factory updateFactory;
private final CommentsUtil commentsUtil;
private final PatchSetUtil psUtil;
- private final PatchListCache patchListCache;
@Inject
DeleteDraftComment(
- BatchUpdate.Factory updateFactory,
- CommentsUtil commentsUtil,
- PatchSetUtil psUtil,
- PatchListCache patchListCache) {
+ BatchUpdate.Factory updateFactory, CommentsUtil commentsUtil, PatchSetUtil psUtil) {
this.updateFactory = updateFactory;
this.commentsUtil = commentsUtil;
this.psUtil = psUtil;
- this.patchListCache = patchListCache;
}
@Override
@@ -79,8 +70,7 @@
}
@Override
- public boolean updateChange(ChangeContext ctx)
- throws ResourceNotFoundException, PatchListNotAvailableException {
+ public boolean updateChange(ChangeContext ctx) throws ResourceNotFoundException {
Optional<HumanComment> maybeComment =
commentsUtil.getDraft(ctx.getNotes(), ctx.getIdentifiedUser(), key);
if (!maybeComment.isPresent()) {
@@ -92,7 +82,7 @@
throw new ResourceNotFoundException("patch set not found: " + psId);
}
HumanComment c = maybeComment.get();
- setCommentCommitId(c, patchListCache, ctx.getChange(), ps);
+ commentsUtil.setCommentCommitId(c, ctx.getChange(), ps);
commentsUtil.deleteHumanComments(ctx.getUpdate(psId), Collections.singleton(c));
return true;
}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index 7a4a891..3367ca6 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -18,7 +18,6 @@
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL;
-import static com.google.gerrit.server.CommentsUtil.setCommentCommitId;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static com.google.gerrit.server.permissions.LabelPermission.ForUser.ON_BEHALF_OF;
import static com.google.gerrit.server.project.ProjectCache.illegalState;
@@ -893,7 +892,7 @@
@Override
public boolean updateChange(ChangeContext ctx)
throws ResourceConflictException, UnprocessableEntityException, IOException,
- PatchListNotAvailableException, CommentsRejectedException {
+ CommentsRejectedException {
user = ctx.getIdentifiedUser();
notes = ctx.getNotes();
ps = psUtil.get(ctx.getNotes(), psId);
@@ -951,7 +950,7 @@
}
private boolean insertComments(ChangeContext ctx, List<RobotComment> newRobotComments)
- throws PatchListNotAvailableException, CommentsRejectedException {
+ throws CommentsRejectedException {
Map<String, List<CommentInput>> inputComments = in.comments;
if (inputComments == null) {
inputComments = Collections.emptyMap();
@@ -1003,7 +1002,7 @@
comment.message = inputComment.message;
}
- setCommentCommitId(comment, patchListCache, ctx.getChange(), ps);
+ commentsUtil.setCommentCommitId(comment, ctx.getChange(), ps);
comment.setLineNbrAndRange(inputComment.line, inputComment.range);
comment.tag = in.tag;
@@ -1082,8 +1081,7 @@
return !newRobotComments.isEmpty();
}
- private List<RobotComment> getNewRobotComments(ChangeContext ctx)
- throws PatchListNotAvailableException {
+ private List<RobotComment> getNewRobotComments(ChangeContext ctx) {
List<RobotComment> toAdd = new ArrayList<>(in.robotComments.size());
Set<CommentSetEntry> existingIds =
@@ -1103,8 +1101,7 @@
}
private RobotComment createRobotCommentFromInput(
- ChangeContext ctx, String path, RobotCommentInput robotCommentInput)
- throws PatchListNotAvailableException {
+ ChangeContext ctx, String path, RobotCommentInput robotCommentInput) {
RobotComment robotComment =
commentsUtil.newRobotComment(
ctx,
@@ -1119,7 +1116,7 @@
robotComment.properties = robotCommentInput.properties;
robotComment.setLineNbrAndRange(robotCommentInput.line, robotCommentInput.range);
robotComment.tag = in.tag;
- setCommentCommitId(robotComment, patchListCache, ctx.getChange(), ps);
+ commentsUtil.setCommentCommitId(robotComment, ctx.getChange(), ps);
robotComment.fixSuggestions = createFixSuggestionsFromInput(robotCommentInput.fixSuggestions);
return robotComment;
}
diff --git a/java/com/google/gerrit/server/restapi/change/PutDraftComment.java b/java/com/google/gerrit/server/restapi/change/PutDraftComment.java
index 39de43d..84a3d89 100644
--- a/java/com/google/gerrit/server/restapi/change/PutDraftComment.java
+++ b/java/com/google/gerrit/server/restapi/change/PutDraftComment.java
@@ -15,7 +15,6 @@
package com.google.gerrit.server.restapi.change;
import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL;
-import static com.google.gerrit.server.CommentsUtil.setCommentCommitId;
import com.google.gerrit.entities.Comment;
import com.google.gerrit.entities.HumanComment;
@@ -32,8 +31,6 @@
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.change.DraftCommentResource;
import com.google.gerrit.server.notedb.ChangeUpdate;
-import com.google.gerrit.server.patch.PatchListCache;
-import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
@@ -54,7 +51,6 @@
private final CommentsUtil commentsUtil;
private final PatchSetUtil psUtil;
private final Provider<CommentJson> commentJson;
- private final PatchListCache patchListCache;
@Inject
PutDraftComment(
@@ -62,14 +58,12 @@
DeleteDraftComment delete,
CommentsUtil commentsUtil,
PatchSetUtil psUtil,
- Provider<CommentJson> commentJson,
- PatchListCache patchListCache) {
+ Provider<CommentJson> commentJson) {
this.updateFactory = updateFactory;
this.delete = delete;
this.commentsUtil = commentsUtil;
this.psUtil = psUtil;
this.commentJson = commentJson;
- this.patchListCache = patchListCache;
}
@Override
@@ -114,8 +108,7 @@
}
@Override
- public boolean updateChange(ChangeContext ctx)
- throws ResourceNotFoundException, PatchListNotAvailableException {
+ public boolean updateChange(ChangeContext ctx) throws ResourceNotFoundException {
Optional<HumanComment> maybeComment =
commentsUtil.getDraft(ctx.getNotes(), ctx.getIdentifiedUser(), key);
if (!maybeComment.isPresent()) {
@@ -143,7 +136,7 @@
commentsUtil.deleteHumanComments(update, Collections.singleton(origComment));
comment.key.filename = in.path;
}
- setCommentCommitId(comment, patchListCache, ctx.getChange(), ps);
+ commentsUtil.setCommentCommitId(comment, ctx.getChange(), ps);
commentsUtil.putHumanComments(
update,
HumanComment.Status.DRAFT,
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java b/javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java
index 783fa49..d76d114 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java
@@ -34,6 +34,8 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.extensions.api.changes.DeleteCommentInput;
+import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.truth.NullAwareCorrespondence;
@@ -43,6 +45,7 @@
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 {
@@ -339,6 +342,51 @@
}
@Test
+ public void commentOnInvalidParentIsPorted() 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 commentUuid = newComment(patchset1Id).onSecondParentCommit().create();
+
+ List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
+
+ assertThat(portedComments).comparingElementsUsing(hasUuid()).containsExactly(commentUuid);
+ }
+
+ @Test
+ public void commentsOnInvalidPositionArePorted() throws Exception {
+ // Set up change and patchsets.
+ Change.Id changeId = changeOps.newChange().file("myFile").content("Line 1\n").create();
+ PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
+ PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
+ // Add comments.
+ String commentUuid1 = newComment(patchset1Id).onFileLevelOf("not-existing file").create();
+ String commentUuid2 = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
+
+ List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
+
+ assertThat(portedComments)
+ .comparingElementsUsing(hasUuid())
+ .containsExactly(commentUuid1, commentUuid2);
+ }
+
+ @Test
+ public void commentsOnInvalidPositionKeepTheirInvalidPosition() 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.
+ newComment(patchset1Id).onFileLevelOf("not-existing file").create();
+
+ Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
+
+ assertThatMap(portedComments).keys().containsExactly("not-existing file");
+ }
+
+ @Test
public void portedCommentHasOriginalUuid() throws Exception {
// Set up change and patchsets.
Change.Id changeId = changeOps.newChange().create();
@@ -998,6 +1046,42 @@
}
@Test
+ public void overlappingRangeCommentsArePortedToNewPosition() 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()
+ .file("myFile")
+ .content("Line 1\nLine 1.1\nLine 2\n")
+ .create();
+ // Add comment.
+ String commentUuid1 =
+ newComment(patchset1Id)
+ .fromLine(2)
+ .charOffset(0)
+ .toLine(2)
+ .charOffset(3)
+ .ofFile("myFile")
+ .create();
+ String commentUuid2 =
+ newComment(patchset1Id)
+ .fromLine(2)
+ .charOffset(1)
+ .toLine(2)
+ .charOffset(4)
+ .ofFile("myFile")
+ .create();
+
+ CommentInfo portedComment1 = getPortedComment(patchset2Id, commentUuid1);
+ assertThat(portedComment1).range().startLine().isEqualTo(3);
+ CommentInfo portedComment2 = getPortedComment(patchset2Id, commentUuid2);
+ assertThat(portedComment2).range().startLine().isEqualTo(3);
+ }
+
+ @Test
public void portedLineCommentCanHandleAddedLines() throws Exception {
// Set up change and patchsets.
Change.Id changeId =
@@ -1241,6 +1325,28 @@
}
@Test
+ public void overlappingLineCommentsArePortedToNewPosition() 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()
+ .file("myFile")
+ .content("Line 1\nLine 1.1\nLine 2\n")
+ .create();
+ // Add comment.
+ String commentUuid1 = newComment(patchset1Id).onLine(2).ofFile("myFile").create();
+ String commentUuid2 = newComment(patchset1Id).onLine(2).ofFile("myFile").create();
+
+ CommentInfo portedComment1 = getPortedComment(patchset2Id, commentUuid1);
+ assertThat(portedComment1).line().isEqualTo(3);
+ CommentInfo portedComment2 = getPortedComment(patchset2Id, commentUuid2);
+ assertThat(portedComment2).line().isEqualTo(3);
+ }
+
+ @Test
public void portedFileCommentIsObliviousToAdjustedFileContent() throws Exception {
// Set up change and patchsets.
Change.Id changeId =
@@ -1553,6 +1659,169 @@
assertThat(portedComment).line().isGreaterThan(2);
}
+ @Test
+ public void commentOnFirstParentIsPortedToSingleParentWhenPatchsetChangedToNonMergeCommit()
+ throws Exception {
+ // Set up change and patchsets.
+ Change.Id parent1ChangeId = changeOps.newChange().file("file1").content("Line 1\n").create();
+ Change.Id parent2ChangeId = changeOps.newChange().file("file2").content("Line 1\n").create();
+ Change.Id childChangeId =
+ changeOps
+ .newChange()
+ .mergeOf()
+ .change(parent1ChangeId)
+ .and()
+ .change(parent2ChangeId)
+ .create();
+ PatchSet.Id childPatchset1Id =
+ changeOps.change(childChangeId).currentPatchset().get().patchsetId();
+ PatchSet.Id parent1PatchsetId2 =
+ changeOps
+ .change(parent1ChangeId)
+ .newPatchset()
+ .file("file1")
+ .content("Line 0\nLine 1\n")
+ .create();
+ PatchSet.Id childPatchset2Id =
+ changeOps
+ .change(childChangeId)
+ .newPatchset()
+ .parent()
+ .patchset(parent1PatchsetId2)
+ .create();
+ // Add comment.
+ String commentUuid =
+ newComment(childPatchset1Id).onParentCommit().onLine(1).ofFile("file1").create();
+
+ CommentInfo portedComment = getPortedComment(childPatchset2Id, commentUuid);
+
+ assertThat(portedComment).line().isEqualTo(2);
+ assertThat(portedComment).side().isEqualTo(Side.PARENT);
+ assertThat(portedComment).parent().isEqualTo(1);
+ }
+
+ @Test
+ public void commentOnSecondParentBecomesPatchsetLevelCommentWhenPatchsetChangedToNonMergeCommit()
+ throws Exception {
+ // Set up change and patchsets.
+ Change.Id parent1ChangeId = changeOps.newChange().file("file1").content("Line 1\n").create();
+ Change.Id parent2ChangeId = changeOps.newChange().file("file2").content("Line 1\n").create();
+ Change.Id childChangeId =
+ changeOps
+ .newChange()
+ .mergeOf()
+ .change(parent1ChangeId)
+ .and()
+ .change(parent2ChangeId)
+ .create();
+ PatchSet.Id childPatchset1Id =
+ changeOps.change(childChangeId).currentPatchset().get().patchsetId();
+ PatchSet.Id childPatchset2Id =
+ changeOps.change(childChangeId).newPatchset().parent().change(parent1ChangeId).create();
+ // Add comment.
+ String commentUuid =
+ newComment(childPatchset1Id).onSecondParentCommit().onLine(1).ofFile("file2").create();
+
+ Map<String, List<CommentInfo>> portedComments = getPortedComments(childPatchset2Id);
+ assertThatMap(portedComments).keys().containsExactly(Patch.PATCHSET_LEVEL);
+ CommentInfo portedComment = extractSpecificComment(portedComments, commentUuid);
+ assertThat(portedComment).line().isNull();
+ assertThat(portedComment).side().isNull();
+ assertThat(portedComment).parent().isNull();
+ }
+
+ @Test
+ // TODO(ghareeb): Adjust implementation in CommentsUtil to use the new auto-merge code instead of
+ // PatchListCache#getOldId which returns the wrong result if a change isn't a merge commit.
+ @Ignore
+ public void
+ commentOnAutoMergeCommitBecomesPatchsetLevelCommentWhenPatchsetChangedToNonMergeCommit()
+ throws Exception {
+ // Set up change and patchsets.
+ Change.Id parent1ChangeId = changeOps.newChange().file("file1").content("Line 1\n").create();
+ Change.Id parent2ChangeId = changeOps.newChange().file("file1").content("Line 1\n").create();
+ Change.Id childChangeId =
+ changeOps
+ .newChange()
+ .mergeOf()
+ .change(parent1ChangeId)
+ .and()
+ .change(parent2ChangeId)
+ .create();
+ PatchSet.Id childPatchset1Id =
+ changeOps.change(childChangeId).currentPatchset().get().patchsetId();
+ PatchSet.Id childPatchset2Id =
+ changeOps.change(childChangeId).newPatchset().parent().change(parent1ChangeId).create();
+ // Add comment.
+ String commentUuid =
+ newComment(childPatchset1Id).onAutoMergeCommit().onLine(1).ofFile("file1").create();
+
+ Map<String, List<CommentInfo>> portedComments = getPortedComments(childPatchset2Id);
+ assertThatMap(portedComments).keys().containsExactly(Patch.PATCHSET_LEVEL);
+ CommentInfo portedComment = extractSpecificComment(portedComments, commentUuid);
+ assertThat(portedComment).line().isNull();
+ assertThat(portedComment).side().isNull();
+ assertThat(portedComment).parent().isNull();
+ }
+
+ @Test
+ public void whitespaceOnlyModificationsAreAlsoConsideredWhenPorting() throws Exception {
+ // Set up change and patchsets.
+ Change.Id changeId = changeOps.newChange().file("myFile").content("Line 1\n").create();
+ PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
+ PatchSet.Id patchset2Id =
+ changeOps.change(changeId).newPatchset().file("myFile").content("\nLine 1\n").create();
+ // Add comment.
+ String commentUuid = newComment(patchset1Id).onLine(1).ofFile("myFile").create();
+
+ CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
+
+ assertThat(portedComment).line().isEqualTo(2);
+ }
+
+ @Test
+ public void deletedCommentContentIsNotCachedInPortedComments() throws Exception {
+ // Set up change and patchsets.
+ Change.Id changeId = changeOps.newChange().create();
+ PatchSet.Id patchsetId1 = changeOps.change(changeId).currentPatchset().get().patchsetId();
+ PatchSet.Id patchsetId2 = changeOps.change(changeId).newPatchset().create();
+ // Add comment.
+ String commentUuid = newComment(patchsetId1).message("Confidential content").create();
+
+ getPortedComment(patchsetId2, commentUuid);
+ gApi.changes()
+ .id(changeId.get())
+ .revision(patchsetId1.get())
+ .comment(commentUuid)
+ .delete(new DeleteCommentInput());
+ CommentInfo portedComment = getPortedComment(patchsetId2, commentUuid);
+
+ assertThat(portedComment).message().doesNotContain("Confidential content");
+ }
+
+ @Test
+ public void setOfPortedCommentsCanChangeOnRepeatedCalls() throws Exception {
+ // Set up change and patchsets.
+ Change.Id changeId = changeOps.newChange().create();
+ PatchSet.Id patchsetId1 = changeOps.change(changeId).currentPatchset().get().patchsetId();
+ PatchSet.Id patchsetId2 = changeOps.change(changeId).newPatchset().create();
+ // Add comment.
+ String commentUuid1 = newComment(patchsetId1).unresolved().create();
+
+ ImmutableList<CommentInfo> pastPortedComments = flatten(getPortedComments(patchsetId2));
+ // Set the existing comment thread to resolved, so it won't be ported anymore.
+ newComment(patchsetId1).parentUuid(commentUuid1).resolved().create();
+ // Create a new comment which should show up as ported comment.
+ String commentUuid2 = newComment(patchsetId1).create();
+ ImmutableList<CommentInfo> portedComments = flatten(getPortedComments(patchsetId2));
+
+ // Ensure that results are not cached between calls. This should not be necessary as the diffs
+ // are already cached. If we need to also cache the ported comments in the future, we'll need to
+ // identify ALL situations when the set of ported comments changes.
+ assertThat(portedComments).isNotEqualTo(pastPortedComments);
+ assertThat(portedComments).comparingElementsUsing(hasUuid()).containsExactly(commentUuid2);
+ }
+
private TestCommentCreation.Builder newComment(PatchSet.Id patchsetId) {
// Create unresolved comments by default as only those are ported. Tests get override the
// unresolved state by explicitly setting it.
diff --git a/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java b/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java
index 9c30fc9..8891cd2 100644
--- a/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java
+++ b/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java
@@ -17,6 +17,7 @@
import static com.google.common.truth.Truth.assertThat;
import static java.util.Comparator.comparing;
import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyShort;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
@@ -31,6 +32,7 @@
import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.patch.ComparisonType;
import com.google.gerrit.server.patch.PatchList;
@@ -41,6 +43,7 @@
import com.google.gerrit.truth.NullAwareCorrespondence;
import java.sql.Timestamp;
import java.util.Arrays;
+import java.util.Optional;
import org.eclipse.jgit.lib.ObjectId;
import org.junit.Rule;
import org.junit.Test;
@@ -56,6 +59,7 @@
@Rule public final MockitoRule mockito = MockitoJUnit.rule();
@Mock private PatchListCache patchListCache;
+ @Mock private CommentsUtil commentsUtil;
private int uuidCounter = 0;
@@ -68,9 +72,10 @@
PatchSet patchset2 = createPatchset(PatchSet.id(changeId, 2));
ChangeNotes changeNotes = mockChangeNotes(project, change, patchset1, patchset2);
- CommentPorter commentPorter = new CommentPorter(patchListCache);
+ CommentPorter commentPorter = new CommentPorter(patchListCache, commentsUtil);
HumanComment comment = createComment(patchset1.id(), "myFile");
- when(patchListCache.getOldId(any(), any(), any())).thenReturn(dummyObjectId);
+ when(commentsUtil.determineCommitId(any(), any(), anyShort()))
+ .thenReturn(Optional.of(dummyObjectId));
when(patchListCache.get(any(PatchListKey.class), any(Project.NameKey.class)))
.thenThrow(PatchListNotAvailableException.class);
ImmutableList<HumanComment> portedComments =
@@ -88,9 +93,10 @@
PatchSet patchset2 = createPatchset(PatchSet.id(changeId, 2));
ChangeNotes changeNotes = mockChangeNotes(project, change, patchset1, patchset2);
- CommentPorter commentPorter = new CommentPorter(patchListCache);
+ CommentPorter commentPorter = new CommentPorter(patchListCache, commentsUtil);
HumanComment comment = createComment(patchset1.id(), "myFile");
- when(patchListCache.getOldId(any(), any(), any())).thenReturn(dummyObjectId);
+ when(commentsUtil.determineCommitId(any(), any(), anyShort()))
+ .thenReturn(Optional.of(dummyObjectId));
when(patchListCache.get(any(PatchListKey.class), any(Project.NameKey.class)))
.thenThrow(IllegalStateException.class);
ImmutableList<HumanComment> portedComments =
@@ -100,7 +106,7 @@
}
@Test
- public void commentsAreNotDroppedWhenRetrievingCommitSha1sHasUnexpectedError() throws Exception {
+ public void commentsAreNotDroppedWhenRetrievingCommitSha1sHasUnexpectedError() {
Project.NameKey project = Project.nameKey("myProject");
Change.Id changeId = Change.id(1);
Change change = createChange(project, changeId);
@@ -108,9 +114,10 @@
PatchSet patchset2 = createPatchset(PatchSet.id(changeId, 2));
ChangeNotes changeNotes = mockChangeNotes(project, change, patchset1, patchset2);
- CommentPorter commentPorter = new CommentPorter(patchListCache);
+ CommentPorter commentPorter = new CommentPorter(patchListCache, commentsUtil);
HumanComment comment = createComment(patchset1.id(), "myFile");
- when(patchListCache.getOldId(any(), any(), any())).thenThrow(IllegalStateException.class);
+ when(commentsUtil.determineCommitId(any(), any(), anyShort()))
+ .thenThrow(IllegalStateException.class);
ImmutableList<HumanComment> portedComments =
commentPorter.portComments(changeNotes, patchset2, ImmutableList.of(comment));
@@ -126,9 +133,10 @@
PatchSet patchset2 = createPatchset(PatchSet.id(changeId, 2));
ChangeNotes changeNotes = mockChangeNotes(project, change, patchset1, patchset2);
- CommentPorter commentPorter = new CommentPorter(patchListCache);
+ CommentPorter commentPorter = new CommentPorter(patchListCache, commentsUtil);
HumanComment comment = createComment(patchset1.id(), "myFile");
- when(patchListCache.getOldId(any(), any(), any())).thenReturn(dummyObjectId);
+ when(commentsUtil.determineCommitId(any(), any(), anyShort()))
+ .thenReturn(Optional.of(dummyObjectId));
when(patchListCache.get(any(PatchListKey.class), any(Project.NameKey.class)))
.thenThrow(IllegalStateException.class);
ImmutableList<HumanComment> portedComments =
@@ -149,18 +157,13 @@
PatchSet patchset3 = createPatchset(PatchSet.id(changeId, 3));
ChangeNotes changeNotes = mockChangeNotes(project, change, patchset1, patchset2, patchset3);
- CommentPorter commentPorter = new CommentPorter(patchListCache);
+ CommentPorter commentPorter = new CommentPorter(patchListCache, commentsUtil);
// Place the comments on different patchsets to have two different diff requests.
HumanComment comment1 = createComment(patchset1.id(), "myFile");
HumanComment comment2 = createComment(patchset2.id(), "myFile");
- when(patchListCache.getOldId(any(), any(), any())).thenReturn(dummyObjectId);
- PatchList emptyDiff =
- new PatchList(
- dummyObjectId,
- dummyObjectId,
- false,
- ComparisonType.againstOtherPatchSet(),
- new PatchListEntry[0]);
+ when(commentsUtil.determineCommitId(any(), any(), anyShort()))
+ .thenReturn(Optional.of(dummyObjectId));
+ PatchList emptyDiff = getEmptyDiff();
// Throw an exception on the first diff request but return an actual value on the second.
when(patchListCache.get(any(PatchListKey.class), any(Project.NameKey.class)))
.thenThrow(IllegalStateException.class)
@@ -173,6 +176,29 @@
assertThat(portedComments).comparingElementsUsing(hasFilePath()).contains("myFile");
}
+ @Test
+ public void commentsWithInvalidPatchsetsAreIgnored() throws Exception {
+ Project.NameKey project = Project.nameKey("myProject");
+ Change.Id changeId = Change.id(1);
+ Change change = createChange(project, changeId);
+ PatchSet patchset1 = createPatchset(PatchSet.id(changeId, 1));
+ PatchSet patchset2 = createPatchset(PatchSet.id(changeId, 2));
+ // Leave out patchset 1 (e.g. reserved for draft patchsets in the past).
+ ChangeNotes changeNotes = mockChangeNotes(project, change, patchset2);
+
+ CommentPorter commentPorter = new CommentPorter(patchListCache, commentsUtil);
+ HumanComment comment = createComment(patchset1.id(), "myFile");
+ when(commentsUtil.determineCommitId(any(), any(), anyShort()))
+ .thenReturn(Optional.of(dummyObjectId));
+ PatchList emptyDiff = getEmptyDiff();
+ when(patchListCache.get(any(PatchListKey.class), any(Project.NameKey.class)))
+ .thenReturn(emptyDiff);
+ ImmutableList<HumanComment> portedComments =
+ commentPorter.portComments(changeNotes, patchset2, ImmutableList.of(comment));
+
+ assertThat(portedComments).isEmpty();
+ }
+
private Change createChange(Project.NameKey project, Change.Id changeId) {
return new Change(
Change.key("changeKey"),
@@ -224,4 +250,13 @@
private Correspondence<HumanComment, String> hasFilePath() {
return NullAwareCorrespondence.transforming(comment -> comment.key.filename, "hasFilePath");
}
+
+ private PatchList getEmptyDiff() {
+ return new PatchList(
+ dummyObjectId,
+ dummyObjectId,
+ false,
+ ComparisonType.againstOtherPatchSet(),
+ new PatchListEntry[0]);
+ }
}
