opendev/gerrit
Code Issues Proposed changes

CommentsUtil: Don't use the diff cache to look up the parent commit

Browse Source 
It's a bit strange that we used the diff caches to lookup a specific
parent commit (e.g. first parent, second parent, auto-merge) of a
patchset even though we weren't interested in the diff at all.

One reason might be that auto-merge commits are only reliably available
within the diff cache. Furthermore, the diff result is cached and hence
quickly available on subsequent calls.

Besides being confusing and possibly doing more than necessary (e.g.
computing actual diffs), this approach has another major disadvantage:
It returns the wrong commit SHA-1 if the specified 'side' doesn't exist
for the commit (e.g. if the second parent is specified but the patchset
isn't a merge commit). In such a case, the diff cache falls back to the
first parent commit without further notice. That's problematic if you're
relying on the returned commit SHA-1 to be correct as we do for ported
comments.

A better approach than using the diff cache is to directly lookup the
desired parent commit. One possible disadvantage of this approach is
that we always need to open the repository to do so.

The new approach also lets callers decide how to react to a non-existent
parent commit as it returns an Optional.empty() instead of directly
throwing an exception. Callers may choose to ignore such a parent or
throw an exception of a type and message of their choice.

For auto-merge commits, we still need to use the old approach. With the
redesigned diff caches, we should be able to switch to a different
approach, too.

Change-Id: Iad263b16c45b7c0d826b98aeb8f8865296326bf7
This commit is contained in:
Alice Kober-Sotzek
2020-09-14 17:28:28 +02:00
parent 6f34d00523
commit 31f07a5e9f
12 changed files with 226 additions and 130 deletions
Download Patch File Download Diff File Expand all files Collapse all files

31
java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java
View File

@@ -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.IdentifiedUser.GenericFactory;
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 @@ public class PerPatchsetOperationsImpl implements PerPatchsetOperations {
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 @@ public class PerPatchsetOperationsImpl implements PerPatchsetOperations {
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 @@ public class PerPatchsetOperationsImpl implements PerPatchsetOperations {
}
@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 @@ public class PerPatchsetOperationsImpl implements PerPatchsetOperations {
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 @@ public class PerPatchsetOperationsImpl implements PerPatchsetOperations {
.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 @@ public class PerPatchsetOperationsImpl implements PerPatchsetOperations {
}
@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 @@ public class PerPatchsetOperationsImpl implements PerPatchsetOperations {
}
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 @@ public class PerPatchsetOperationsImpl implements PerPatchsetOperations {
newRobotComment.properties = robotCommentCreation.properties();
}
setCommentCommitId(
newRobotComment,
patchListCache,
context.getChange(),
changeNotes.getPatchSets().get(patchsetId));
commentsUtil.setCommentCommitId(
newRobotComment, context.getChange(), changeNotes.getPatchSets().get(patchsetId));
return newRobotComment;
}
}

59
java/com/google/gerrit/server/CommentsUtil.java
View File

@@ -31,6 +31,7 @@ import com.google.gerrit.entities.Comment;
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 java.util.Optional;
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 @@ public class CommentsUtil {
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 @@ public class CommentsUtil {
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);
}
}

14
java/com/google/gerrit/server/PublishCommentUtil.java
View File

@@ -22,15 +22,12 @@ import com.google.common.flogger.FluentLogger;
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 @@ import java.util.Set;
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 @@ public class PublishCommentUtil {
// 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);

13
java/com/google/gerrit/server/mail/receive/MailProcessor.java
View File

@@ -59,8 +59,6 @@ import com.google.gerrit.server.mail.MailFilter;
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;
@@ -107,7 +105,6 @@ public class MailProcessor {
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;
@@ -127,7 +124,6 @@ public class MailProcessor {
ChangeMessagesUtil changeMessagesUtil,
CommentsUtil commentsUtil,
OneOffRequestContext oneOffRequestContext,
PatchListCache patchListCache,
PatchSetUtil psUtil,
Provider<InternalChangeQuery> queryProvider,
DynamicMap<MailFilter> mailFilters,
@@ -144,7 +140,6 @@ public class MailProcessor {
this.changeMessagesUtil = changeMessagesUtil;
this.commentsUtil = commentsUtil;
this.oneOffRequestContext = oneOffRequestContext;
this.patchListCache = patchListCache;
this.psUtil = psUtil;
this.queryProvider = queryProvider;
this.mailFilters = mailFilters;
@@ -331,8 +326,7 @@ public class MailProcessor {
}
@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) {
@@ -424,8 +418,7 @@ public class MailProcessor {
}
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.
@@ -457,7 +450,7 @@ public class MailProcessor {
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;
}
}

13
java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java
View File

@@ -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.CurrentUser;
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 @@ public class DeleteDraftComments
private final Provider<CommentJson> commentJsonProvider;
private final CommentsUtil commentsUtil;
private final PatchSetUtil psUtil;
private final PatchListCache patchListCache;
@Inject
DeleteDraftComments(
@@ -92,8 +88,7 @@ public class DeleteDraftComments
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 @@ public class DeleteDraftComments
this.commentJsonProvider = commentJsonProvider;
this.commentsUtil = commentsUtil;
this.psUtil = psUtil;
this.patchListCache = patchListCache;
}
@Override
@@ -176,14 +170,13 @@ public class DeleteDraftComments
}
@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));
}

26
java/com/google/gerrit/server/restapi/change/CommentPorter.java
View File

@@ -67,10 +67,12 @@ public class CommentPorter {
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;
}
/**
@@ -208,13 +210,22 @@ public class CommentPorter {
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 {
@@ -285,6 +296,11 @@ public class CommentPorter {
// 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;
}

13
java/com/google/gerrit/server/restapi/change/CreateDraftComment.java
View File

@@ -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.extensions.restapi.Url;
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 @@ public class CreateDraftComment implements RestModifyView<RevisionResource, Draf
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 @@ public class CreateDraftComment implements RestModifyView<RevisionResource, Draf
@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 @@ public class CreateDraftComment implements RestModifyView<RevisionResource, Draf
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));

16
java/com/google/gerrit/server/restapi/change/DeleteDraftComment.java
View File

@@ -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.extensions.restapi.RestModifyView;
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 @@ public class DeleteDraftComment implements RestModifyView<DraftCommentResource,
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 @@ public class DeleteDraftComment implements RestModifyView<DraftCommentResource,
}
@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 @@ public class DeleteDraftComment implements RestModifyView<DraftCommentResource,
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;
}

15
java/com/google/gerrit/server/restapi/change/PostReview.java
View File

@@ -18,7 +18,6 @@ import static com.google.common.base.MoreObjects.firstNonNull;
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;
@@ -894,7 +893,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
@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);
@@ -960,7 +959,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
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();
@@ -1012,7 +1011,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
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;
@@ -1091,8 +1090,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
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 =
@@ -1112,8 +1110,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
private RobotComment createRobotCommentFromInput(
ChangeContext ctx, String path, RobotCommentInput robotCommentInput)
throws PatchListNotAvailableException {
ChangeContext ctx, String path, RobotCommentInput robotCommentInput) {
RobotComment robotComment =
commentsUtil.newRobotComment(
ctx,
@@ -1128,7 +1125,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
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;
}

13
java/com/google/gerrit/server/restapi/change/PutDraftComment.java
View File

@@ -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.CommentsUtil;
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 @@ public class PutDraftComment implements RestModifyView<DraftCommentResource, Dra
private final CommentsUtil commentsUtil;
private final PatchSetUtil psUtil;
private final Provider<CommentJson> commentJson;
private final PatchListCache patchListCache;
@Inject
PutDraftComment(
@@ -62,14 +58,12 @@ public class PutDraftComment implements RestModifyView<DraftCommentResource, Dra
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 @@ public class PutDraftComment implements RestModifyView<DraftCommentResource, Dra
}
@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 @@ public class PutDraftComment implements RestModifyView<DraftCommentResource, Dra
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,

107
javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java
View File

@@ -35,6 +35,7 @@ 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;
@@ -44,6 +45,7 @@ import java.time.ZoneOffset;
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 {
@@ -1657,6 +1659,111 @@ public class PortedCommentsIT extends AbstractDaemonTest {
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.

36
javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java
View File

@@ -17,6 +17,7 @@ package com.google.gerrit.server.restapi.change;
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.HumanComment;
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.server.patch.PatchListNotAvailableException;
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 @@ public class CommentPorterTest {
@Rule public final MockitoRule mockito = MockitoJUnit.rule();
@Mock private PatchListCache patchListCache;
@Mock private CommentsUtil commentsUtil;
private int uuidCounter = 0;
@@ -68,9 +72,10 @@ public class CommentPorterTest {
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 @@ public class CommentPorterTest {
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 @@ public class CommentPorterTest {
}
@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 @@ public class CommentPorterTest {
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 @@ public class CommentPorterTest {
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,11 +157,12 @@ public class CommentPorterTest {
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);
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)))
@@ -177,9 +186,10 @@ public class CommentPorterTest {
// Leave out patchset 1 (e.g. reserved for draft patchsets in the past).
ChangeNotes changeNotes = mockChangeNotes(project, change, 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));
PatchList emptyDiff = getEmptyDiff();
when(patchListCache.get(any(PatchListKey.class), any(Project.NameKey.class)))
.thenReturn(emptyDiff);