Merge "Merge branch 'stable-3.2'"
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 134af9a..f02d89a 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -182,20 +182,6 @@
+
By default 1.
-[[asyncPostUpdate]]
-=== Section asyncPostUpdate
-
-[[asyncPostUpdate.threadPoolSize]]asyncPostUpdate.threadPoolSize::
-+
-Maximum size of thread pool in which async post updates are sent out.
-+
-When set to 0, a direct executor is used.
-+
-When unset, use link:#sendemail[sendemail.threadPoolSize] threadPoolSize. When both
-are unset, use the default.
-+
-By default, 1.
-
[[auth]]
=== Section auth
@@ -884,8 +870,6 @@
The check if they are is cheap and always happens on the thread that
inquires for a cached value.
+
-When set to 0, a direct executor is used.
-+
Defaults to 2.
==== [[cache_names]]Standard Caches
@@ -4490,7 +4474,11 @@
an error occurs.
[[sendemail.threadPoolSize]]sendemail.threadPoolSize::
-Deprecated. Replaced with link:#asyncPostUpdate.threadPoolSize[asyncPostUpdate.threadPoolSize]
++
+Maximum size of thread pool in which the review comments
+notifications are sent out asynchronously.
++
+By default, 1.
[[sendemail.from]]sendemail.from::
+
diff --git a/java/com/google/gerrit/acceptance/AbstractNotificationTest.java b/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
index 32bc992..452df67 100644
--- a/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
@@ -48,7 +48,6 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
-import java.util.Optional;
import java.util.Set;
import java.util.function.Function;
import org.eclipse.jgit.junit.TestRepository;
@@ -98,7 +97,7 @@
protected static class FakeEmailSenderSubject extends Subject {
private final FakeEmailSender fakeEmailSender;
- private Optional<Message> message;
+ private Message message;
private StagedUsers users;
private Map<RecipientType, List<String>> recipients = new HashMap<>();
private Set<String> accountedFor = new HashSet<>();
@@ -117,29 +116,35 @@
}
public FakeEmailSenderSubject sent(String messageType, StagedUsers users) {
- fakeEmailSender.readOneMessage();
- message =
- fakeEmailSender.getMessages().stream()
- .filter(
- m ->
- m.headers()
- .get("X-Gerrit-MessageType")
- .equals(new EmailHeader.String(messageType)))
- .findFirst();
- if (!message.isPresent()) {
- failWithoutActual(
- fact(String.format("expected message of type %s", messageType), "not sent"));
+ message = fakeEmailSender.nextMessage();
+ if (message == null) {
+ failWithoutActual(fact("expected message", "not sent"));
}
recipients = new HashMap<>();
- recipients.put(TO, parseAddresses(message.get(), "To"));
- recipients.put(CC, parseAddresses(message.get(), "Cc"));
+ recipients.put(TO, parseAddresses(message, "To"));
+ recipients.put(CC, parseAddresses(message, "Cc"));
recipients.put(
BCC,
- message.get().rcpt().stream()
+ message.rcpt().stream()
.map(Address::email)
.filter(e -> !recipients.get(TO).contains(e) && !recipients.get(CC).contains(e))
.collect(toList()));
this.users = users;
+ if (!message.headers().containsKey("X-Gerrit-MessageType")) {
+ failWithoutActual(
+ fact("expected to have message sent with", "X-Gerrit-MessageType header"));
+ }
+ EmailHeader header = message.headers().get("X-Gerrit-MessageType");
+ if (!header.equals(new EmailHeader.String(messageType))) {
+ failWithoutActual(
+ fact("expected message of type", messageType),
+ fact(
+ "actual",
+ header instanceof EmailHeader.String
+ ? ((EmailHeader.String) header).getString()
+ : header));
+ }
+
return this;
}
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/PublishCommentsOp.java b/java/com/google/gerrit/server/PublishCommentsOp.java
index e03c244..358ce92 100644
--- a/java/com/google/gerrit/server/PublishCommentsOp.java
+++ b/java/com/google/gerrit/server/PublishCommentsOp.java
@@ -28,7 +28,6 @@
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.CommentsRejectedException;
@@ -47,7 +46,7 @@
* <p>This class uses the {@link PublishCommentUtil} to publish draft comments and fires the
* necessary event for this.
*/
-public class PublishCommentsOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class PublishCommentsOp implements BatchUpdateOp {
private final PatchSetUtil psUtil;
private final ChangeNotes.Factory changeNotesFactory;
private final ChangeMessagesUtil cmUtil;
@@ -62,8 +61,6 @@
private List<HumanComment> comments = new ArrayList<>();
private ChangeMessage message;
private IdentifiedUser user;
- private ChangeNotes changeNotes;
- private PatchSet patchset;
public interface Factory {
PublishCommentsOp create(PatchSet.Id psId, Project.NameKey projectNameKey);
@@ -112,24 +109,11 @@
@Override
public void postUpdate(Context ctx) {
- changeNotes = changeNotesFactory.createChecked(projectNameKey, psId.changeId());
- patchset = psUtil.get(changeNotes, psId);
-
- commentAdded.fire(
- changeNotes.getChange(),
- patchset,
- ctx.getAccount(),
- message.getMessage(),
- ImmutableMap.of(),
- ImmutableMap.of(),
- ctx.getWhen());
- }
-
- @Override
- public void asyncPostUpdate(Context ctx) {
if (message == null || comments.isEmpty()) {
return;
}
+ ChangeNotes changeNotes = changeNotesFactory.createChecked(projectNameKey, psId.changeId());
+ PatchSet ps = psUtil.get(changeNotes, psId);
NotifyResolver.Result notify = ctx.getNotify(changeNotes.getChangeId());
if (notify.shouldNotify()) {
RepoView repoView;
@@ -140,10 +124,17 @@
String.format("Repository %s not found", ctx.getProject().get()), ex);
}
email
- .create(
- notify, changeNotes, patchset, user, message, comments, null, labelDelta, repoView)
- .send();
+ .create(notify, changeNotes, ps, user, message, comments, null, labelDelta, repoView)
+ .sendAsync();
}
+ commentAdded.fire(
+ changeNotes.getChange(),
+ ps,
+ ctx.getAccount(),
+ message.getMessage(),
+ ImmutableMap.of(),
+ ImmutableMap.of(),
+ ctx.getWhen());
}
private boolean insertMessage(ChangeContext ctx, ChangeUpdate changeUpdate) {
diff --git a/java/com/google/gerrit/server/change/AbandonOp.java b/java/com/google/gerrit/server/change/AbandonOp.java
index e0e489b..6c39ed0 100644
--- a/java/com/google/gerrit/server/change/AbandonOp.java
+++ b/java/com/google/gerrit/server/change/AbandonOp.java
@@ -30,14 +30,13 @@
import com.google.gerrit.server.mail.send.MessageIdGenerator;
import com.google.gerrit.server.mail.send.ReplyToChangeSender;
import com.google.gerrit.server.notedb.ChangeUpdate;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
-public class AbandonOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class AbandonOp implements BatchUpdateOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final AbandonedSender.Factory abandonedSenderFactory;
@@ -52,7 +51,6 @@
private Change change;
private PatchSet patchSet;
private ChangeMessage message;
- private NotifyResolver.Result notify;
public interface Factory {
AbandonOp create(
@@ -98,7 +96,6 @@
update.setStatus(change.getStatus());
message = newMessage(ctx);
cmUtil.addChangeMessage(update, message);
- notify = ctx.getNotify(change.getId());
return true;
}
@@ -115,11 +112,7 @@
@Override
public void postUpdate(Context ctx) {
- changeAbandoned.fire(change, patchSet, accountState, msgTxt, ctx.getWhen(), notify.handling());
- }
-
- @Override
- public void asyncPostUpdate(Context ctx) {
+ NotifyResolver.Result notify = ctx.getNotify(change.getId());
try {
ReplyToChangeSender emailSender =
abandonedSenderFactory.create(ctx.getProject(), change.getId());
@@ -134,5 +127,6 @@
} catch (Exception e) {
logger.atSevere().withCause(e).log("Cannot email update for change %s", change.getId());
}
+ changeAbandoned.fire(change, patchSet, accountState, msgTxt, ctx.getWhen(), notify.handling());
}
}
diff --git a/java/com/google/gerrit/server/change/AddReviewersEmail.java b/java/com/google/gerrit/server/change/AddReviewersEmail.java
index 5e48353..4a3f638 100644
--- a/java/com/google/gerrit/server/change/AddReviewersEmail.java
+++ b/java/com/google/gerrit/server/change/AddReviewersEmail.java
@@ -23,27 +23,34 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.mail.send.AddReviewerSender;
import com.google.gerrit.server.mail.send.MessageIdGenerator;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.util.Collection;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
@Singleton
public class AddReviewersEmail {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final AddReviewerSender.Factory addReviewerSenderFactory;
+ private final ExecutorService sendEmailsExecutor;
private final MessageIdGenerator messageIdGenerator;
@Inject
AddReviewersEmail(
- AddReviewerSender.Factory addReviewerSenderFactory, MessageIdGenerator messageIdGenerator) {
+ AddReviewerSender.Factory addReviewerSenderFactory,
+ @SendEmailExecutor ExecutorService sendEmailsExecutor,
+ MessageIdGenerator messageIdGenerator) {
this.addReviewerSenderFactory = addReviewerSenderFactory;
+ this.sendEmailsExecutor = sendEmailsExecutor;
this.messageIdGenerator = messageIdGenerator;
}
- public void emailReviewers(
+ public void emailReviewersAsync(
IdentifiedUser user,
Change change,
Collection<Account.Id> added,
@@ -71,20 +78,27 @@
ImmutableList<Address> immutableAddedByEmail = ImmutableList.copyOf(addedByEmail);
ImmutableList<Address> immutableCopiedByEmail = ImmutableList.copyOf(copiedByEmail);
- try {
- AddReviewerSender emailSender = addReviewerSenderFactory.create(projectNameKey, cId);
- emailSender.setNotify(notify);
- emailSender.setFrom(userId);
- emailSender.addReviewers(immutableToMail);
- emailSender.addReviewersByEmail(immutableAddedByEmail);
- emailSender.addExtraCC(immutableToCopy);
- emailSender.addExtraCCByEmail(immutableCopiedByEmail);
- emailSender.setMessageId(
- messageIdGenerator.fromChangeUpdate(change.getProject(), change.currentPatchSetId()));
- emailSender.send();
- } catch (Exception err) {
- logger.atSevere().withCause(err).log(
- "Cannot send email to new reviewers of change %s", change.getId());
- }
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError =
+ sendEmailsExecutor.submit(
+ () -> {
+ try {
+ AddReviewerSender emailSender =
+ addReviewerSenderFactory.create(projectNameKey, cId);
+ emailSender.setNotify(notify);
+ emailSender.setFrom(userId);
+ emailSender.addReviewers(immutableToMail);
+ emailSender.addReviewersByEmail(immutableAddedByEmail);
+ emailSender.addExtraCC(immutableToCopy);
+ emailSender.addExtraCCByEmail(immutableCopiedByEmail);
+ emailSender.setMessageId(
+ messageIdGenerator.fromChangeUpdate(
+ change.getProject(), change.currentPatchSetId()));
+ emailSender.send();
+ } catch (Exception err) {
+ logger.atSevere().withCause(err).log(
+ "Cannot send email to new reviewers of change %s", change.getId());
+ }
+ });
}
}
diff --git a/java/com/google/gerrit/server/change/AddReviewersOp.java b/java/com/google/gerrit/server/change/AddReviewersOp.java
index fd57722..ff8e5c6 100644
--- a/java/com/google/gerrit/server/change/AddReviewersOp.java
+++ b/java/com/google/gerrit/server/change/AddReviewersOp.java
@@ -42,7 +42,6 @@
import com.google.gerrit.server.extensions.events.ReviewerAdded;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.project.ProjectCache;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -53,7 +52,7 @@
import java.util.List;
import java.util.Set;
-public class AddReviewersOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class AddReviewersOp implements BatchUpdateOp {
public interface Factory {
/**
@@ -239,7 +238,7 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(Context ctx) throws Exception {
opResult =
Result.builder()
.setAddedReviewers(addedReviewers)
@@ -247,20 +246,8 @@
.setAddedCCs(addedCCs)
.setAddedCCsByEmail(addedCCsByEmail)
.build();
- if (!addedReviewers.isEmpty()) {
- List<AccountState> reviewers =
- addedReviewers.stream()
- .map(r -> accountCache.get(r.accountId()))
- .flatMap(Streams::stream)
- .collect(toList());
- reviewerAdded.fire(change, patchSet, reviewers, ctx.getAccount(), ctx.getWhen());
- }
- }
-
- @Override
- public void asyncPostUpdate(Context ctx) {
if (sendEmail) {
- addReviewersEmail.emailReviewers(
+ addReviewersEmail.emailReviewersAsync(
ctx.getUser().asIdentifiedUser(),
change,
Lists.transform(addedReviewers, PatchSetApproval::accountId),
@@ -269,6 +256,14 @@
addedCCsByEmail,
ctx.getNotify(change.getId()));
}
+ if (!addedReviewers.isEmpty()) {
+ List<AccountState> reviewers =
+ addedReviewers.stream()
+ .map(r -> accountCache.get(r.accountId()))
+ .flatMap(Streams::stream)
+ .collect(toList());
+ reviewerAdded.fire(change, patchSet, reviewers, ctx.getAccount(), ctx.getWhen());
+ }
}
public Result getResult() {
diff --git a/java/com/google/gerrit/server/change/AddToAttentionSetOp.java b/java/com/google/gerrit/server/change/AddToAttentionSetOp.java
index 31147fc..8053b30 100644
--- a/java/com/google/gerrit/server/change/AddToAttentionSetOp.java
+++ b/java/com/google/gerrit/server/change/AddToAttentionSetOp.java
@@ -25,7 +25,6 @@
import com.google.gerrit.server.mail.send.MessageIdGenerator;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -35,7 +34,7 @@
import java.io.IOException;
/** Add a specified user to the attention set. */
-public class AddToAttentionSetOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class AddToAttentionSetOp implements BatchUpdateOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -102,7 +101,7 @@
}
@Override
- public void asyncPostUpdate(Context ctx) {
+ public void postUpdate(Context ctx) {
if (!notify) {
return;
}
@@ -115,7 +114,7 @@
reason,
messageIdGenerator.fromChangeUpdate(ctx.getRepoView(), change.currentPatchSetId()),
attentionUserId)
- .send();
+ .sendAsync();
} catch (IOException e) {
logger.atSevere().withCause(e).log(e.getMessage(), change.getId());
}
diff --git a/java/com/google/gerrit/server/change/ChangeInserter.java b/java/com/google/gerrit/server/change/ChangeInserter.java
index 2f95a5c..a086cb1 100644
--- a/java/com/google/gerrit/server/change/ChangeInserter.java
+++ b/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -52,6 +52,7 @@
import com.google.gerrit.server.change.ReviewerAdder.InternalAddReviewerInput;
import com.google.gerrit.server.change.ReviewerAdder.ReviewerAddition;
import com.google.gerrit.server.change.ReviewerAdder.ReviewerAdditionList;
+import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.config.UrlFormatter;
import com.google.gerrit.server.events.CommitReceivedEvent;
import com.google.gerrit.server.extensions.events.CommentAdded;
@@ -68,12 +69,12 @@
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.ssh.NoSshInfo;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
import com.google.gerrit.server.update.InsertChangeOp;
import com.google.gerrit.server.update.RepoContext;
import com.google.gerrit.server.util.CommitMessageUtil;
+import com.google.gerrit.server.util.RequestScopePropagator;
import com.google.gerrit.server.validators.ValidationException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@@ -83,13 +84,15 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.ReceiveCommand;
-public class ChangeInserter implements InsertChangeOp, AsyncPostUpdateOp {
+public class ChangeInserter implements InsertChangeOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -103,6 +106,7 @@
private final ApprovalsUtil approvalsUtil;
private final ChangeMessagesUtil cmUtil;
private final CreateChangeSender.Factory createChangeSenderFactory;
+ private final ExecutorService sendEmailExecutor;
private final CommitValidators.Factory commitValidatorsFactory;
private final RevisionCreated revisionCreated;
private final CommentAdded commentAdded;
@@ -126,6 +130,7 @@
private List<String> groups = Collections.emptyList();
private boolean validate = true;
private Map<String, Short> approvals;
+ private RequestScopePropagator requestScopePropagator;
private boolean fireRevisionCreated;
private boolean sendMail;
private boolean updateRef;
@@ -141,7 +146,6 @@
private String pushCert;
private ProjectState projectState;
private ReviewerAdditionList reviewerAdditions;
- private NotifyResolver.Result notify;
@Inject
ChangeInserter(
@@ -152,6 +156,7 @@
ApprovalsUtil approvalsUtil,
ChangeMessagesUtil cmUtil,
CreateChangeSender.Factory createChangeSenderFactory,
+ @SendEmailExecutor ExecutorService sendEmailExecutor,
CommitValidators.Factory commitValidatorsFactory,
CommentAdded commentAdded,
RevisionCreated revisionCreated,
@@ -168,6 +173,7 @@
this.approvalsUtil = approvalsUtil;
this.cmUtil = cmUtil;
this.createChangeSenderFactory = createChangeSenderFactory;
+ this.sendEmailExecutor = sendEmailExecutor;
this.commitValidatorsFactory = commitValidatorsFactory;
this.revisionCreated = revisionCreated;
this.commentAdded = commentAdded;
@@ -309,6 +315,11 @@
return this;
}
+ public ChangeInserter setRequestScopePropagator(RequestScopePropagator r) {
+ this.requestScopePropagator = r;
+ return this;
+ }
+
public ChangeInserter setRevertOf(Change.Id revertOf) {
this.revertOf = revertOf;
return this;
@@ -446,13 +457,57 @@
ChangeMessagesUtil.uploadedPatchSetTag(workInProgress));
cmUtil.addChangeMessage(update, changeMessage);
}
- notify = ctx.getNotify(change.getId());
return true;
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(Context ctx) throws Exception {
reviewerAdditions.postUpdate(ctx);
+ NotifyResolver.Result notify = ctx.getNotify(change.getId());
+ if (sendMail && notify.shouldNotify()) {
+ Runnable sender =
+ new Runnable() {
+ @Override
+ public void run() {
+ try {
+ CreateChangeSender emailSender =
+ createChangeSenderFactory.create(change.getProject(), change.getId());
+ emailSender.setFrom(change.getOwner());
+ emailSender.setPatchSet(patchSet, patchSetInfo);
+ emailSender.setNotify(notify);
+ emailSender.addReviewers(
+ reviewerAdditions.flattenResults(AddReviewersOp.Result::addedReviewers).stream()
+ .map(PatchSetApproval::accountId)
+ .collect(toImmutableSet()));
+ emailSender.addReviewersByEmail(
+ reviewerAdditions.flattenResults(AddReviewersOp.Result::addedReviewersByEmail));
+ emailSender.addExtraCC(
+ reviewerAdditions.flattenResults(AddReviewersOp.Result::addedCCs));
+ emailSender.addExtraCCByEmail(
+ reviewerAdditions.flattenResults(AddReviewersOp.Result::addedCCsByEmail));
+ emailSender.setMessageId(
+ messageIdGenerator.fromChangeUpdate(ctx.getRepoView(), patchSet.id()));
+ emailSender.send();
+ } catch (Exception e) {
+ logger.atSevere().withCause(e).log(
+ "Cannot send email for new change %s", change.getId());
+ }
+ }
+
+ @Override
+ public String toString() {
+ return "send-email newchange";
+ }
+ };
+ if (requestScopePropagator != null) {
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError =
+ sendEmailExecutor.submit(requestScopePropagator.wrap(sender));
+ } else {
+ sender.run();
+ }
+ }
+
/* For labels that are not set in this operation, show the "current" value
* of 0, and no oldValue as the value was not modified by this operation.
* For labels that are set in this operation, the value was modified, so
@@ -480,34 +535,6 @@
}
}
- @Override
- public void asyncPostUpdate(Context ctx) {
- reviewerAdditions.asyncPostUpdate(ctx);
- if (sendMail && notify.shouldNotify()) {
- try {
- CreateChangeSender emailSender =
- createChangeSenderFactory.create(change.getProject(), change.getId());
- emailSender.setFrom(change.getOwner());
- emailSender.setPatchSet(patchSet, patchSetInfo);
- emailSender.setNotify(notify);
- emailSender.addReviewers(
- reviewerAdditions.flattenResults(AddReviewersOp.Result::addedReviewers).stream()
- .map(PatchSetApproval::accountId)
- .collect(toImmutableSet()));
- emailSender.addReviewersByEmail(
- reviewerAdditions.flattenResults(AddReviewersOp.Result::addedReviewersByEmail));
- emailSender.addExtraCC(reviewerAdditions.flattenResults(AddReviewersOp.Result::addedCCs));
- emailSender.addExtraCCByEmail(
- reviewerAdditions.flattenResults(AddReviewersOp.Result::addedCCsByEmail));
- emailSender.setMessageId(
- messageIdGenerator.fromChangeUpdate(ctx.getRepoView(), patchSet.id()));
- emailSender.send();
- } catch (Exception e) {
- logger.atSevere().withCause(e).log("Cannot send email for new change %s", change.getId());
- }
- }
- }
-
private void validate(RepoContext ctx) throws IOException, ResourceConflictException {
if (!validate) {
return;
diff --git a/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java b/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java
index d9ede58..255e13a 100644
--- a/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java
+++ b/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java
@@ -22,7 +22,6 @@
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.mail.send.DeleteReviewerSender;
import com.google.gerrit.server.mail.send.MessageIdGenerator;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -30,7 +29,7 @@
import com.google.inject.assistedinject.Assisted;
import java.util.Collections;
-public class DeleteReviewerByEmailOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class DeleteReviewerByEmailOp implements BatchUpdateOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -74,7 +73,7 @@
}
@Override
- public void asyncPostUpdate(Context ctx) {
+ public void postUpdate(Context ctx) {
try {
NotifyResolver.Result notify = ctx.getNotify(change.getId());
if (!notify.shouldNotify()) {
diff --git a/java/com/google/gerrit/server/change/DeleteReviewerOp.java b/java/com/google/gerrit/server/change/DeleteReviewerOp.java
index de11e07..07cb04f 100644
--- a/java/com/google/gerrit/server/change/DeleteReviewerOp.java
+++ b/java/com/google/gerrit/server/change/DeleteReviewerOp.java
@@ -43,7 +43,6 @@
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.RemoveReviewerControl;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -56,7 +55,7 @@
import java.util.HashMap;
import java.util.Map;
-public class DeleteReviewerOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class DeleteReviewerOp implements BatchUpdateOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -77,12 +76,11 @@
private final AccountState reviewer;
private final DeleteReviewerInput input;
- private ChangeMessage changeMessage;
- private Change currChange;
- private PatchSet currPs;
- private Map<String, Short> newApprovals = new HashMap<>();
- private Map<String, Short> oldApprovals = new HashMap<>();
- private NotifyResolver.Result notify;
+ ChangeMessage changeMessage;
+ Change currChange;
+ PatchSet currPs;
+ Map<String, Short> newApprovals = new HashMap<>();
+ Map<String, Short> oldApprovals = new HashMap<>();
@Inject
DeleteReviewerOp(
@@ -167,27 +165,13 @@
changeMessage =
ChangeMessagesUtil.newMessage(ctx, msg.toString(), ChangeMessagesUtil.TAG_DELETE_REVIEWER);
cmUtil.addChangeMessage(update, changeMessage);
- notify = ctx.getNotify(currChange.getId());
return true;
}
@Override
public void postUpdate(Context ctx) {
- reviewerDeleted.fire(
- currChange,
- currPs,
- reviewer,
- ctx.getAccount(),
- changeMessage.getMessage(),
- newApprovals,
- oldApprovals,
- notify.handling(),
- ctx.getWhen());
- }
-
- @Override
- public void asyncPostUpdate(Context ctx) {
+ NotifyResolver.Result notify = ctx.getNotify(currChange.getId());
if (input.notify == null
&& currChange.isWorkInProgress()
&& !oldApprovals.isEmpty()
@@ -203,6 +187,16 @@
} catch (Exception err) {
logger.atSevere().withCause(err).log("Cannot email update for change %s", currChange.getId());
}
+ reviewerDeleted.fire(
+ currChange,
+ currPs,
+ reviewer,
+ ctx.getAccount(),
+ changeMessage.getMessage(),
+ newApprovals,
+ oldApprovals,
+ notify.handling(),
+ ctx.getWhen());
}
private Iterable<PatchSetApproval> approvals(ChangeContext ctx, Account.Id accountId) {
diff --git a/java/com/google/gerrit/server/change/EmailReviewComments.java b/java/com/google/gerrit/server/change/EmailReviewComments.java
index 309b041..cacfbe7 100644
--- a/java/com/google/gerrit/server/change/EmailReviewComments.java
+++ b/java/com/google/gerrit/server/change/EmailReviewComments.java
@@ -21,18 +21,24 @@
import com.google.gerrit.entities.ChangeMessage;
import com.google.gerrit.entities.Comment;
import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.mail.send.CommentSender;
import com.google.gerrit.server.mail.send.MessageIdGenerator;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.update.RepoView;
import com.google.gerrit.server.util.LabelVote;
+import com.google.gerrit.server.util.RequestContext;
+import com.google.gerrit.server.util.ThreadLocalRequestContext;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.util.List;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
-public class EmailReviewComments {
+public class EmailReviewComments implements Runnable, RequestContext {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -65,8 +71,10 @@
RepoView repoView);
}
+ private final ExecutorService sendEmailsExecutor;
private final PatchSetInfoFactory patchSetInfoFactory;
private final CommentSender.Factory commentSenderFactory;
+ private final ThreadLocalRequestContext requestContext;
private final MessageIdGenerator messageIdGenerator;
private final NotifyResolver.Result notify;
@@ -81,8 +89,10 @@
@Inject
EmailReviewComments(
+ @SendEmailExecutor ExecutorService executor,
PatchSetInfoFactory patchSetInfoFactory,
CommentSender.Factory commentSenderFactory,
+ ThreadLocalRequestContext requestContext,
MessageIdGenerator messageIdGenerator,
@Assisted NotifyResolver.Result notify,
@Assisted ChangeNotes notes,
@@ -93,8 +103,10 @@
@Nullable @Assisted String patchSetComment,
@Assisted List<LabelVote> labels,
@Assisted RepoView repoView) {
+ this.sendEmailsExecutor = executor;
this.patchSetInfoFactory = patchSetInfoFactory;
this.commentSenderFactory = commentSenderFactory;
+ this.requestContext = requestContext;
this.messageIdGenerator = messageIdGenerator;
this.notify = notify;
this.notes = notes;
@@ -107,7 +119,14 @@
this.repoView = repoView;
}
- public void send() {
+ public void sendAsync() {
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError = sendEmailsExecutor.submit(this);
+ }
+
+ @Override
+ public void run() {
+ RequestContext old = requestContext.setContext(this);
try {
CommentSender emailSender =
commentSenderFactory.create(notes.getProjectName(), notes.getChangeId());
@@ -124,6 +143,18 @@
emailSender.send();
} catch (Exception e) {
logger.atSevere().withCause(e).log("Cannot email comments for %s", patchSet.id());
+ } finally {
+ requestContext.setContext(old);
}
}
+
+ @Override
+ public String toString() {
+ return "send-email comments";
+ }
+
+ @Override
+ public CurrentUser getUser() {
+ return user.getRealUser();
+ }
}
diff --git a/java/com/google/gerrit/server/change/PatchSetInserter.java b/java/com/google/gerrit/server/change/PatchSetInserter.java
index 4502408..882352d 100644
--- a/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -49,7 +49,6 @@
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.ssh.NoSshInfo;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -63,7 +62,7 @@
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.transport.ReceiveCommand;
-public class PatchSetInserter implements BatchUpdateOp, AsyncPostUpdateOp {
+public class PatchSetInserter implements BatchUpdateOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -110,7 +109,6 @@
private ChangeMessage changeMessage;
private ReviewerSet oldReviewers;
private boolean oldWorkInProgressState;
- private NotifyResolver.Result notify;
@Inject
public PatchSetInserter(
@@ -281,12 +279,12 @@
throw new BadRequestException(ex.getMessage());
}
}
- notify = ctx.getNotify(change.getId());
return true;
}
@Override
- public void asyncPostUpdate(Context ctx) {
+ public void postUpdate(Context ctx) {
+ NotifyResolver.Result notify = ctx.getNotify(change.getId());
if (notify.shouldNotify() && sendEmail) {
requireNonNull(changeMessage);
try {
@@ -306,10 +304,7 @@
"Cannot send email for new patch set on change %s", change.getId());
}
}
- }
- @Override
- public void postUpdate(Context ctx) {
if (fireRevisionCreated) {
revisionCreated.fire(change, patchSet, ctx.getAccount(), ctx.getWhen(), notify);
}
diff --git a/java/com/google/gerrit/server/change/RebaseChangeOp.java b/java/com/google/gerrit/server/change/RebaseChangeOp.java
index a8cc75a..231359b 100644
--- a/java/com/google/gerrit/server/change/RebaseChangeOp.java
+++ b/java/com/google/gerrit/server/change/RebaseChangeOp.java
@@ -34,7 +34,6 @@
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -49,7 +48,7 @@
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
-public class RebaseChangeOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class RebaseChangeOp implements BatchUpdateOp {
public interface Factory {
RebaseChangeOp create(ChangeNotes notes, PatchSet originalPatchSet, ObjectId baseCommitId);
}
@@ -222,11 +221,6 @@
patchSetInserter.postUpdate(ctx);
}
- @Override
- public void asyncPostUpdate(Context ctx) {
- patchSetInserter.asyncPostUpdate(ctx);
- }
-
public RevCommit getRebasedCommit() {
checkState(rebasedCommit != null, "getRebasedCommit() only valid after updateRepo");
return rebasedCommit;
diff --git a/java/com/google/gerrit/server/change/RemoveFromAttentionSetOp.java b/java/com/google/gerrit/server/change/RemoveFromAttentionSetOp.java
index 7f4e0e3..e532409 100644
--- a/java/com/google/gerrit/server/change/RemoveFromAttentionSetOp.java
+++ b/java/com/google/gerrit/server/change/RemoveFromAttentionSetOp.java
@@ -26,7 +26,6 @@
import com.google.gerrit.server.mail.send.RemoveFromAttentionSetSender;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -37,7 +36,7 @@
import java.util.Optional;
/** Remove a specified user from the attention set. */
-public class RemoveFromAttentionSetOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class RemoveFromAttentionSetOp implements BatchUpdateOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -102,7 +101,7 @@
}
@Override
- public void asyncPostUpdate(Context ctx) {
+ public void postUpdate(Context ctx) {
if (!notify) {
return;
}
@@ -115,7 +114,7 @@
reason,
messageIdGenerator.fromChangeUpdate(ctx.getRepoView(), change.currentPatchSetId()),
attentionUserId)
- .send();
+ .sendAsync();
} catch (IOException e) {
logger.atSevere().withCause(e).log(e.getMessage(), change.getId());
}
diff --git a/java/com/google/gerrit/server/change/ReviewerAdder.java b/java/com/google/gerrit/server/change/ReviewerAdder.java
index 3638bf2..3d986d2 100644
--- a/java/com/google/gerrit/server/change/ReviewerAdder.java
+++ b/java/com/google/gerrit/server/change/ReviewerAdder.java
@@ -587,7 +587,7 @@
}
}
- public void postUpdate(Context ctx) {
+ public void postUpdate(Context ctx) throws Exception {
for (ReviewerAddition addition : additions()) {
if (addition.op != null) {
addition.op.postUpdate(ctx);
@@ -595,14 +595,6 @@
}
}
- public void asyncPostUpdate(Context ctx) {
- for (ReviewerAddition addition : additions()) {
- if (addition.op != null) {
- addition.op.asyncPostUpdate(ctx);
- }
- }
- }
-
public <T> ImmutableSet<T> flattenResults(
Function<AddReviewersOp.Result, ? extends Collection<T>> func) {
additions()
diff --git a/java/com/google/gerrit/server/change/SetAssigneeOp.java b/java/com/google/gerrit/server/change/SetAssigneeOp.java
index c2e36f5..411c9b6 100644
--- a/java/com/google/gerrit/server/change/SetAssigneeOp.java
+++ b/java/com/google/gerrit/server/change/SetAssigneeOp.java
@@ -28,7 +28,6 @@
import com.google.gerrit.server.mail.send.SetAssigneeSender;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.plugincontext.PluginSetContext;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -38,7 +37,7 @@
import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
-public class SetAssigneeOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class SetAssigneeOp implements BatchUpdateOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -121,7 +120,7 @@
}
@Override
- public void asyncPostUpdate(Context ctx) {
+ public void postUpdate(Context ctx) {
try {
SetAssigneeSender emailSender =
setAssigneeSenderFactory.create(
@@ -134,10 +133,6 @@
logger.atSevere().withCause(err).log(
"Cannot send email to new assignee of change %s", change.getId());
}
- }
-
- @Override
- public void postUpdate(Context ctx) {
assigneeChanged.fire(
change, ctx.getAccount(), oldAssignee != null ? oldAssignee.state() : null, ctx.getWhen());
}
diff --git a/java/com/google/gerrit/server/change/WorkInProgressOp.java b/java/com/google/gerrit/server/change/WorkInProgressOp.java
index 99f7adb..f0ebb80 100644
--- a/java/com/google/gerrit/server/change/WorkInProgressOp.java
+++ b/java/com/google/gerrit/server/change/WorkInProgressOp.java
@@ -28,7 +28,6 @@
import com.google.gerrit.server.extensions.events.WorkInProgressStateChanged;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -38,7 +37,7 @@
import java.io.IOException;
/* Set work in progress or ready for review state on a change */
-public class WorkInProgressOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class WorkInProgressOp implements BatchUpdateOp {
public static class Input extends InputWithMessage {
@Nullable public NotifyHandling notify;
@@ -127,7 +126,8 @@
}
@Override
- public void asyncPostUpdate(Context ctx) {
+ public void postUpdate(Context ctx) {
+ stateChanged.fire(change, ps, ctx.getAccount(), ctx.getWhen());
NotifyResolver.Result notify = ctx.getNotify(change.getId());
if (workInProgress
|| notify.handling().compareTo(NotifyHandling.OWNER_REVIEWERS) < 0
@@ -152,11 +152,6 @@
cmsg.getMessage(),
ImmutableList.of(),
repoView)
- .send();
- }
-
- @Override
- public void postUpdate(Context ctx) {
- stateChanged.fire(change, ps, ctx.getAccount(), ctx.getWhen());
+ .sendAsync();
}
}
diff --git a/java/com/google/gerrit/server/config/AsyncPostUpdateExecutor.java b/java/com/google/gerrit/server/config/SendEmailExecutor.java
similarity index 90%
rename from java/com/google/gerrit/server/config/AsyncPostUpdateExecutor.java
rename to java/com/google/gerrit/server/config/SendEmailExecutor.java
index 7951b8c..cf90cbf 100644
--- a/java/com/google/gerrit/server/config/AsyncPostUpdateExecutor.java
+++ b/java/com/google/gerrit/server/config/SendEmailExecutor.java
@@ -1,4 +1,4 @@
-// Copyright (C) 2020 The Android Open Source Project
+// Copyright (C) 2014 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
@@ -23,4 +23,4 @@
/** Marker on the global {@link ScheduledThreadPoolExecutor} used to send email. */
@Retention(RUNTIME)
@BindingAnnotation
-public @interface AsyncPostUpdateExecutor {}
+public @interface SendEmailExecutor {}
diff --git a/java/com/google/gerrit/server/config/SysExecutorModule.java b/java/com/google/gerrit/server/config/SysExecutorModule.java
index 5caf6fe..ea45b12 100644
--- a/java/com/google/gerrit/server/config/SysExecutorModule.java
+++ b/java/com/google/gerrit/server/config/SysExecutorModule.java
@@ -51,20 +51,14 @@
@Provides
@Singleton
- @AsyncPostUpdateExecutor
+ @SendEmailExecutor
public ExecutorService provideSendEmailExecutor(
@GerritServerConfig Config config, WorkQueue queues) {
- // sendemail.threadPoolSize is deprecated and overridden by asyncPostUpdate.threadPoolSize.
- int poolSize =
- config.getInt(
- "asyncPostUpdate",
- null,
- "threadPoolSize",
- config.getInt("sendemail", null, "threadPoolSize", 1));
+ int poolSize = config.getInt("sendemail", null, "threadPoolSize", 1);
if (poolSize == 0) {
return newDirectExecutorService();
}
- return queues.createQueue(poolSize, "AsyncPostUpdate", true);
+ return queues.createQueue(poolSize, "SendEmail", true);
}
@Provides
diff --git a/java/com/google/gerrit/server/git/CommitUtil.java b/java/com/google/gerrit/server/git/CommitUtil.java
index 67c7f01..47cbd60 100644
--- a/java/com/google/gerrit/server/git/CommitUtil.java
+++ b/java/com/google/gerrit/server/git/CommitUtil.java
@@ -45,7 +45,6 @@
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.notedb.Sequences;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -301,7 +300,7 @@
return changeId;
}
- private class NotifyOp implements BatchUpdateOp, AsyncPostUpdateOp {
+ private class NotifyOp implements BatchUpdateOp {
private final Change change;
private final ChangeInserter ins;
@@ -311,12 +310,8 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(Context ctx) throws Exception {
changeReverted.fire(change, ins.getChange(), ctx.getWhen());
- }
-
- @Override
- public void asyncPostUpdate(Context ctx) {
try {
RevertedSender emailSender = revertedSenderFactory.create(ctx.getProject(), change.getId());
emailSender.setFrom(ctx.getAccountId());
diff --git a/java/com/google/gerrit/server/git/MergedByPushOp.java b/java/com/google/gerrit/server/git/MergedByPushOp.java
index 13625bc..40e2730 100644
--- a/java/com/google/gerrit/server/git/MergedByPushOp.java
+++ b/java/com/google/gerrit/server/git/MergedByPushOp.java
@@ -25,19 +25,22 @@
import com.google.gerrit.entities.SubmissionId;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.PatchSetUtil;
+import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.extensions.events.ChangeMerged;
import com.google.gerrit.server.mail.send.MergedSender;
import com.google.gerrit.server.mail.send.MessageIdGenerator;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.util.RequestScopePropagator;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
@@ -49,21 +52,24 @@
* <p>When we find a change corresponding to a commit that is pushed to a branch directly, we close
* the change. This class marks the change as merged, and sends out the email notification.
*/
-public class MergedByPushOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class MergedByPushOp implements BatchUpdateOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
MergedByPushOp create(
+ RequestScopePropagator requestScopePropagator,
PatchSet.Id psId,
@Assisted SubmissionId submissionId,
@Assisted("refName") String refName,
@Assisted("mergeResultRevId") String mergeResultRevId);
}
+ private final RequestScopePropagator requestScopePropagator;
private final PatchSetInfoFactory patchSetInfoFactory;
private final ChangeMessagesUtil cmUtil;
private final MergedSender.Factory mergedSenderFactory;
private final PatchSetUtil psUtil;
+ private final ExecutorService sendEmailExecutor;
private final ChangeMerged changeMerged;
private final MessageIdGenerator messageIdGenerator;
@@ -84,8 +90,10 @@
ChangeMessagesUtil cmUtil,
MergedSender.Factory mergedSenderFactory,
PatchSetUtil psUtil,
+ @SendEmailExecutor ExecutorService sendEmailExecutor,
ChangeMerged changeMerged,
MessageIdGenerator messageIdGenerator,
+ @Assisted RequestScopePropagator requestScopePropagator,
@Assisted PatchSet.Id psId,
@Assisted SubmissionId submissionId,
@Assisted("refName") String refName,
@@ -94,8 +102,10 @@
this.cmUtil = cmUtil;
this.mergedSenderFactory = mergedSenderFactory;
this.psUtil = psUtil;
+ this.sendEmailExecutor = sendEmailExecutor;
this.changeMerged = changeMerged;
this.messageIdGenerator = messageIdGenerator;
+ this.requestScopePropagator = requestScopePropagator;
this.submissionId = submissionId;
this.psId = psId;
this.refName = refName;
@@ -171,27 +181,36 @@
if (!correctBranch) {
return;
}
+ @SuppressWarnings("unused") // Runnable already handles errors
+ Future<?> possiblyIgnoredError =
+ sendEmailExecutor.submit(
+ requestScopePropagator.wrap(
+ new Runnable() {
+ @Override
+ public void run() {
+ try {
+ MergedSender emailSender =
+ mergedSenderFactory.create(ctx.getProject(), psId.changeId());
+ emailSender.setFrom(ctx.getAccountId());
+ emailSender.setPatchSet(patchSet, info);
+ emailSender.setMessageId(
+ messageIdGenerator.fromChangeUpdate(ctx.getRepoView(), patchSet.id()));
+ emailSender.send();
+ } catch (Exception e) {
+ logger.atSevere().withCause(e).log(
+ "Cannot send email for submitted patch set %s", psId);
+ }
+ }
+
+ @Override
+ public String toString() {
+ return "send-email merged";
+ }
+ }));
+
changeMerged.fire(change, patchSet, ctx.getAccount(), mergeResultRevId, ctx.getWhen());
}
- @Override
- public void asyncPostUpdate(Context ctx) {
- if (!correctBranch) {
- return;
- }
-
- try {
- MergedSender emailSender = mergedSenderFactory.create(ctx.getProject(), psId.changeId());
- emailSender.setFrom(ctx.getAccountId());
- emailSender.setPatchSet(patchSet, info);
- emailSender.setMessageId(
- messageIdGenerator.fromChangeUpdate(ctx.getRepoView(), patchSet.id()));
- emailSender.send();
- } catch (Exception e) {
- logger.atSevere().withCause(e).log("Cannot send email for submitted patch set %s", psId);
- }
- }
-
private PatchSetInfo getPatchSetInfo(ChangeContext ctx) throws IOException {
RevWalk rw = ctx.getRevWalk();
RevCommit commit = rw.parseCommit(requireNonNull(patchSet).commitId());
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index e44f422..015ed0b 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -175,6 +175,7 @@
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.LabelVote;
import com.google.gerrit.server.util.MagicBranch;
+import com.google.gerrit.server.util.RequestScopePropagator;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.gerrit.server.validators.ValidationException;
import com.google.gerrit.util.cli.CmdLineParser;
@@ -338,6 +339,7 @@
private final PluginSetContext<RequestListener> requestListeners;
private final PublishCommentsOp.Factory publishCommentsOp;
private final RetryHelper retryHelper;
+ private final RequestScopePropagator requestScopePropagator;
private final Sequences seq;
private final SetHashtagsOp.Factory hashtagsFactory;
private final SubmoduleOp.Factory subOpFactory;
@@ -419,6 +421,7 @@
ReplaceOp.Factory replaceOpFactory,
PluginSetContext<RequestListener> requestListeners,
RetryHelper retryHelper,
+ RequestScopePropagator requestScopePropagator,
Sequences seq,
SetHashtagsOp.Factory hashtagsFactory,
SubmoduleOp.Factory subOpFactory,
@@ -467,6 +470,7 @@
this.replaceOpFactory = replaceOpFactory;
this.requestListeners = requestListeners;
this.retryHelper = retryHelper;
+ this.requestScopePropagator = requestScopePropagator;
this.seq = seq;
this.subOpFactory = subOpFactory;
this.tagCache = tagCache;
@@ -2582,6 +2586,7 @@
magicBranch.getCombinedCcs(fromFooters))
.setApprovals(approvals)
.setMessage(msg.toString())
+ .setRequestScopePropagator(requestScopePropagator)
.setSendMail(true)
.setPatchSetDescription(magicBranch.message));
if (!magicBranch.hashtags.isEmpty()) {
@@ -3008,20 +3013,22 @@
RevCommit priorCommit = revisions.inverse().get(priorPatchSet);
replaceOp =
- replaceOpFactory.create(
- projectState,
- notes.getChange().getDest(),
- checkMergedInto,
- checkMergedInto ? inputCommand.getNewId().name() : null,
- priorPatchSet,
- priorCommit,
- psId,
- newCommit,
- info,
- groups,
- magicBranch,
- receivePack.getPushCertificate(),
- notes.getChange());
+ replaceOpFactory
+ .create(
+ projectState,
+ notes.getChange().getDest(),
+ checkMergedInto,
+ checkMergedInto ? inputCommand.getNewId().name() : null,
+ priorPatchSet,
+ priorCommit,
+ psId,
+ newCommit,
+ info,
+ groups,
+ magicBranch,
+ receivePack.getPushCertificate(),
+ notes.getChange())
+ .setRequestScopePropagator(requestScopePropagator);
bu.addOp(notes.getChangeId(), replaceOp);
if (progress != null) {
bu.addOp(notes.getChangeId(), new ChangeProgressOp(progress));
@@ -3302,7 +3309,11 @@
bu.addOp(
psId.changeId(),
mergedByPushOpFactory.create(
- psId, submissionId, refName, newTip.getId().getName()));
+ requestScopePropagator,
+ psId,
+ submissionId,
+ refName,
+ newTip.getId().getName()));
continue COMMIT;
}
}
@@ -3346,7 +3357,12 @@
bu.addOp(
id,
mergedByPushOpFactory
- .create(req.psId, submissionId, refName, newTip.getId().getName())
+ .create(
+ requestScopePropagator,
+ req.psId,
+ submissionId,
+ refName,
+ newTip.getId().getName())
.setPatchSetProvider(req.replaceOp::getPatchSet));
bu.addOp(id, new ChangeProgressOp(progress));
ids.add(id);
diff --git a/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
index 87266f3..ce62d7a 100644
--- a/java/com/google/gerrit/server/git/receive/ReplaceOp.java
+++ b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
@@ -56,6 +56,7 @@
import com.google.gerrit.server.change.ReviewerAdder.InternalAddReviewerInput;
import com.google.gerrit.server.change.ReviewerAdder.ReviewerAddition;
import com.google.gerrit.server.change.ReviewerAdder.ReviewerAdditionList;
+import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.config.UrlFormatter;
import com.google.gerrit.server.extensions.events.CommentAdded;
import com.google.gerrit.server.extensions.events.RevisionCreated;
@@ -70,11 +71,11 @@
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
import com.google.gerrit.server.update.RepoContext;
+import com.google.gerrit.server.util.RequestScopePropagator;
import com.google.gerrit.server.validators.ValidationException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@@ -85,6 +86,8 @@
import java.util.Map;
import java.util.Optional;
import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
import java.util.stream.Stream;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
@@ -93,7 +96,7 @@
import org.eclipse.jgit.transport.PushCertificate;
import org.eclipse.jgit.transport.ReceiveCommand;
-public class ReplaceOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class ReplaceOp implements BatchUpdateOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -120,6 +123,7 @@
private final ChangeData.Factory changeDataFactory;
private final ChangeKindCache changeKindCache;
private final ChangeMessagesUtil cmUtil;
+ private final ExecutorService sendEmailExecutor;
private final RevisionCreated revisionCreated;
private final CommentAdded commentAdded;
private final MergedByPushOp.Factory mergedByPushOpFactory;
@@ -153,6 +157,7 @@
private ChangeMessage msg;
private String rejectMessage;
private MergedByPushOp mergedByPushOp;
+ private RequestScopePropagator requestScopePropagator;
private ReviewerAdditionList reviewerAdditions;
private MailRecipients oldRecipients;
@@ -169,6 +174,7 @@
PatchSetUtil psUtil,
ReplacePatchSetSender.Factory replacePatchSetFactory,
ProjectCache projectCache,
+ @SendEmailExecutor ExecutorService sendEmailExecutor,
ReviewerAdder reviewerAdder,
Change change,
MessageIdGenerator messageIdGenerator,
@@ -196,6 +202,7 @@
this.psUtil = psUtil;
this.replacePatchSetFactory = replacePatchSetFactory;
this.projectCache = projectCache;
+ this.sendEmailExecutor = sendEmailExecutor;
this.reviewerAdder = reviewerAdder;
this.change = change;
this.messageIdGenerator = messageIdGenerator;
@@ -232,7 +239,11 @@
if (mergedInto != null) {
mergedByPushOp =
mergedByPushOpFactory.create(
- patchSetId, new SubmissionId(change), mergedInto, mergeResultRevId);
+ requestScopePropagator,
+ patchSetId,
+ new SubmissionId(change),
+ mergedInto,
+ mergeResultRevId);
}
}
@@ -482,8 +493,18 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(Context ctx) throws Exception {
reviewerAdditions.postUpdate(ctx);
+ if (changeKind != ChangeKind.TRIVIAL_REBASE) {
+ // TODO(dborowitz): Merge email templates so we only have to send one.
+ Runnable e = new ReplaceEmailTask(ctx);
+ if (requestScopePropagator != null) {
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError = sendEmailExecutor.submit(requestScopePropagator.wrap(e));
+ } else {
+ e.run();
+ }
+ }
NotifyResolver.Result notify = ctx.getNotify(notes.getChangeId());
revisionCreated.fire(notes.getChange(), newPatchSet, ctx.getAccount(), ctx.getWhen(), notify);
try {
@@ -496,11 +517,15 @@
}
}
- @Override
- public void asyncPostUpdate(Context ctx) {
- reviewerAdditions.asyncPostUpdate(ctx);
- if (changeKind != ChangeKind.TRIVIAL_REBASE) {
- // TODO(dborowitz): Merge email templates so we only have to send one.
+ private class ReplaceEmailTask implements Runnable {
+ private final Context ctx;
+
+ private ReplaceEmailTask(Context ctx) {
+ this.ctx = ctx;
+ }
+
+ @Override
+ public void run() {
try {
ReplacePatchSetSender emailSender =
replacePatchSetFactory.create(projectState.getNameKey(), notes.getChangeId());
@@ -528,8 +553,10 @@
"Cannot send email for new patch set %s", newPatchSet.id());
}
}
- if (mergedByPushOp != null) {
- mergedByPushOp.asyncPostUpdate(ctx);
+
+ @Override
+ public String toString() {
+ return "send-email newpatchset";
}
}
@@ -586,6 +613,11 @@
return cmd;
}
+ public ReplaceOp setRequestScopePropagator(RequestScopePropagator requestScopePropagator) {
+ this.requestScopePropagator = requestScopePropagator;
+ return this;
+ }
+
private static String findMergedInto(Context ctx, String first, RevCommit commit) {
try {
RevWalk rw = ctx.getRevWalk();
diff --git a/java/com/google/gerrit/server/mail/receive/MailProcessor.java b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
index b8c6bf0..df38118 100644
--- a/java/com/google/gerrit/server/mail/receive/MailProcessor.java
+++ b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
@@ -59,12 +59,9 @@
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;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -107,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;
@@ -127,7 +123,6 @@
ChangeMessagesUtil changeMessagesUtil,
CommentsUtil commentsUtil,
OneOffRequestContext oneOffRequestContext,
- PatchListCache patchListCache,
PatchSetUtil psUtil,
Provider<InternalChangeQuery> queryProvider,
DynamicMap<MailFilter> mailFilters,
@@ -144,7 +139,6 @@
this.changeMessagesUtil = changeMessagesUtil;
this.commentsUtil = commentsUtil;
this.oneOffRequestContext = oneOffRequestContext;
- this.patchListCache = patchListCache;
this.psUtil = psUtil;
this.queryProvider = queryProvider;
this.mailFilters = mailFilters;
@@ -315,7 +309,7 @@
}
}
- private class Op implements BatchUpdateOp, AsyncPostUpdateOp {
+ private class Op implements BatchUpdateOp {
private final PatchSet.Id psId;
private final List<MailComment> parsedComments;
private final String tag;
@@ -331,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) {
@@ -360,26 +353,6 @@
@Override
public void postUpdate(Context ctx) throws Exception {
- // Get previous approvals from this user
- Map<String, Short> approvals = new HashMap<>();
- approvalsUtil
- .byPatchSetUser(
- notes, psId, ctx.getAccountId(), ctx.getRevWalk(), ctx.getRepoView().getConfig())
- .forEach(a -> approvals.put(a.label(), a.value()));
- // Fire Gerrit event. Note that approvals can't be granted via email, so old and new approvals
- // are always the same here.
- commentAdded.fire(
- notes.getChange(),
- patchSet,
- ctx.getAccount(),
- changeMessage.getMessage(),
- approvals,
- approvals,
- ctx.getWhen());
- }
-
- @Override
- public void asyncPostUpdate(Context ctx) throws Exception {
String patchSetComment = null;
if (parsedComments.get(0).getType() == MailComment.CommentType.CHANGE_MESSAGE) {
patchSetComment = parsedComments.get(0).getMessage();
@@ -396,7 +369,23 @@
patchSetComment,
ImmutableList.of(),
ctx.getRepoView())
- .send();
+ .sendAsync();
+ // Get previous approvals from this user
+ Map<String, Short> approvals = new HashMap<>();
+ approvalsUtil
+ .byPatchSetUser(
+ notes, psId, ctx.getAccountId(), ctx.getRevWalk(), ctx.getRepoView().getConfig())
+ .forEach(a -> approvals.put(a.label(), a.value()));
+ // Fire Gerrit event. Note that approvals can't be granted via email, so old and new approvals
+ // are always the same here.
+ commentAdded.fire(
+ notes.getChange(),
+ patchSet,
+ ctx.getAccount(),
+ changeMessage.getMessage(),
+ approvals,
+ approvals,
+ ctx.getWhen());
}
private ChangeMessage generateChangeMessage(ChangeContext ctx) {
@@ -424,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.
@@ -457,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/DeleteVote.java b/java/com/google/gerrit/server/restapi/change/DeleteVote.java
index 22c3567..4b813df 100644
--- a/java/com/google/gerrit/server/restapi/change/DeleteVote.java
+++ b/java/com/google/gerrit/server/restapi/change/DeleteVote.java
@@ -50,7 +50,6 @@
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.project.RemoveReviewerControl;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -147,7 +146,7 @@
return Response.none();
}
- private class Op implements BatchUpdateOp, AsyncPostUpdateOp {
+ private class Op implements BatchUpdateOp {
private final ProjectState projectState;
private final AccountState accountState;
private final String label;
@@ -222,30 +221,17 @@
@Override
public void postUpdate(Context ctx) {
- voteDeleted.fire(
- change,
- ps,
- accountState,
- newApprovals,
- oldApprovals,
- input.notify,
- changeMessage.getMessage(),
- ctx.getIdentifiedUser().state(),
- ctx.getWhen());
- }
-
- @Override
- public void asyncPostUpdate(Context ctx) {
if (changeMessage == null) {
return;
}
+ IdentifiedUser user = ctx.getIdentifiedUser();
try {
NotifyResolver.Result notify = ctx.getNotify(change.getId());
if (notify.shouldNotify()) {
ReplyToChangeSender emailSender =
deleteVoteSenderFactory.create(ctx.getProject(), change.getId());
- emailSender.setFrom(ctx.getIdentifiedUser().getAccountId());
+ emailSender.setFrom(user.getAccountId());
emailSender.setChangeMessage(changeMessage.getMessage(), ctx.getWhen());
emailSender.setNotify(notify);
emailSender.setMessageId(
@@ -255,6 +241,17 @@
} catch (Exception e) {
logger.atSevere().withCause(e).log("Cannot email update for change %s", change.getId());
}
+
+ voteDeleted.fire(
+ change,
+ ps,
+ accountState,
+ newApprovals,
+ oldApprovals,
+ input.notify,
+ changeMessage.getMessage(),
+ user.state(),
+ ctx.getWhen());
}
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index 32c1656..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;
@@ -120,7 +119,6 @@
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -449,7 +447,7 @@
ccByEmail.addAll(addition.reviewersByEmail);
}
}
- addReviewersEmail.emailReviewers(
+ addReviewersEmail.emailReviewersAsync(
user.asIdentifiedUser(), change, to, cc, toByEmail, ccByEmail, notify);
}
}
@@ -871,7 +869,7 @@
abstract Comment.Range range();
}
- private class Op implements BatchUpdateOp, AsyncPostUpdateOp {
+ private class Op implements BatchUpdateOp {
private final ProjectState projectState;
private final PatchSet.Id psId;
private final ReviewInput in;
@@ -894,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);
@@ -917,7 +915,7 @@
}
@Override
- public void asyncPostUpdate(Context ctx) {
+ public void postUpdate(Context ctx) {
if (message == null) {
return;
}
@@ -935,20 +933,12 @@
in.message,
labelDelta,
ctx.getRepoView())
- .send();
+ .sendAsync();
} catch (IOException ex) {
throw new StorageException(
String.format("Repository %s not found", ctx.getProject().get()), ex);
}
}
- }
-
- @Override
- public void postUpdate(Context ctx) {
- if (message == null) {
- return;
- }
-
commentAdded.fire(
notes.getChange(),
ps,
@@ -960,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();
@@ -1012,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;
@@ -1091,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 =
@@ -1112,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,
@@ -1128,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/java/com/google/gerrit/server/restapi/change/Restore.java b/java/com/google/gerrit/server/restapi/change/Restore.java
index 26b9d4c..7faf8e0 100644
--- a/java/com/google/gerrit/server/restapi/change/Restore.java
+++ b/java/com/google/gerrit/server/restapi/change/Restore.java
@@ -44,7 +44,6 @@
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -109,7 +108,7 @@
return Response.ok(json.noOptions().format(op.change));
}
- private class Op implements BatchUpdateOp, AsyncPostUpdateOp {
+ private class Op implements BatchUpdateOp {
private final RestoreInput input;
private Change change;
@@ -150,12 +149,6 @@
@Override
public void postUpdate(Context ctx) {
- changeRestored.fire(
- change, patchSet, ctx.getAccount(), Strings.emptyToNull(input.message), ctx.getWhen());
- }
-
- @Override
- public void asyncPostUpdate(Context ctx) {
try {
ReplyToChangeSender emailSender =
restoredSenderFactory.create(ctx.getProject(), change.getId());
@@ -167,6 +160,8 @@
} catch (Exception e) {
logger.atSevere().withCause(e).log("Cannot email update for change %s", change.getId());
}
+ changeRestored.fire(
+ change, patchSet, ctx.getAccount(), Strings.emptyToNull(input.message), ctx.getWhen());
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
index d128186..ca39a57 100644
--- a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
+++ b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
@@ -72,7 +72,6 @@
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.restapi.change.CherryPickChange.Result;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -601,7 +600,7 @@
}
}
- private class NotifyOp implements BatchUpdateOp, AsyncPostUpdateOp {
+ private class NotifyOp implements BatchUpdateOp {
private final Change change;
private final Change.Id revertChangeId;
@@ -611,13 +610,9 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(Context ctx) throws Exception {
changeReverted.fire(
change, changeNotesFactory.createChecked(revertChangeId).getChange(), ctx.getWhen());
- }
-
- @Override
- public void asyncPostUpdate(Context ctx) {
try {
RevertedSender emailSender = revertedSenderFactory.create(ctx.getProject(), change.getId());
emailSender.setFrom(ctx.getAccountId());
diff --git a/java/com/google/gerrit/server/submit/EmailMerge.java b/java/com/google/gerrit/server/submit/EmailMerge.java
index c7671b0..4efa4c8 100644
--- a/java/com/google/gerrit/server/submit/EmailMerge.java
+++ b/java/com/google/gerrit/server/submit/EmailMerge.java
@@ -19,14 +19,22 @@
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.change.NotifyResolver;
+import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.mail.send.MergedSender;
import com.google.gerrit.server.mail.send.MessageIdGenerator;
import com.google.gerrit.server.update.RepoView;
+import com.google.gerrit.server.util.RequestContext;
+import com.google.gerrit.server.util.ThreadLocalRequestContext;
import com.google.inject.Inject;
+import com.google.inject.OutOfScopeException;
import com.google.inject.assistedinject.Assisted;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
-class EmailMerge {
+class EmailMerge implements Runnable, RequestContext {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
interface Factory {
@@ -38,7 +46,10 @@
RepoView repoView);
}
+ private final ExecutorService sendEmailsExecutor;
private final MergedSender.Factory mergedSenderFactory;
+ private final ThreadLocalRequestContext requestContext;
+ private final IdentifiedUser.GenericFactory identifiedUserFactory;
private final MessageIdGenerator messageIdGenerator;
private final Project.NameKey project;
@@ -49,14 +60,20 @@
@Inject
EmailMerge(
+ @SendEmailExecutor ExecutorService executor,
MergedSender.Factory mergedSenderFactory,
+ ThreadLocalRequestContext requestContext,
+ IdentifiedUser.GenericFactory identifiedUserFactory,
MessageIdGenerator messageIdGenerator,
@Assisted Project.NameKey project,
@Assisted Change change,
@Assisted @Nullable Account.Id submitter,
@Assisted NotifyResolver.Result notify,
@Assisted RepoView repoView) {
+ this.sendEmailsExecutor = executor;
this.mergedSenderFactory = mergedSenderFactory;
+ this.requestContext = requestContext;
+ this.identifiedUserFactory = identifiedUserFactory;
this.messageIdGenerator = messageIdGenerator;
this.project = project;
this.change = change;
@@ -65,7 +82,14 @@
this.repoView = repoView;
}
- public void send() {
+ void sendAsync() {
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError = sendEmailsExecutor.submit(this);
+ }
+
+ @Override
+ public void run() {
+ RequestContext old = requestContext.setContext(this);
try {
MergedSender emailSender = mergedSenderFactory.create(project, change.getId());
if (submitter != null) {
@@ -77,6 +101,21 @@
emailSender.send();
} catch (Exception e) {
logger.atSevere().withCause(e).log("Cannot email merged notification for %s", change.getId());
+ } finally {
+ requestContext.setContext(old);
}
}
+
+ @Override
+ public String toString() {
+ return "send-email merged";
+ }
+
+ @Override
+ public CurrentUser getUser() {
+ if (submitter != null) {
+ return identifiedUserFactory.create(submitter).getRealUser();
+ }
+ throw new OutOfScopeException("No user on email thread");
+ }
}
diff --git a/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java b/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
index 3629640..edc3725 100644
--- a/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
+++ b/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
@@ -275,13 +275,6 @@
rebaseOp.postUpdate(ctx);
}
}
-
- @Override
- public void asyncPostUpdateImpl(Context ctx) {
- if (rebaseOp != null) {
- rebaseOp.asyncPostUpdate(ctx);
- }
- }
}
private class MergeIfNecessaryOp extends SubmitStrategyOp {
diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
index 201eba8..3cc566b 100644
--- a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
+++ b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
@@ -43,7 +43,6 @@
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.project.ProjectConfig;
import com.google.gerrit.server.project.ProjectState;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -61,7 +60,7 @@
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.transport.ReceiveCommand;
-abstract class SubmitStrategyOp implements BatchUpdateOp, AsyncPostUpdateOp {
+abstract class SubmitStrategyOp implements BatchUpdateOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
protected final SubmitStrategy.Arguments args;
@@ -484,22 +483,8 @@
}
}
- if (mergeResultRev != null && !args.dryrun) {
- args.changeMerged.fire(
- updatedChange,
- mergedPatchSet,
- args.accountCache.get(submitter.accountId()).orElse(null),
- args.mergeTip.getCurrentTip().name(),
- ctx.getWhen());
- }
- }
-
- /**
- * Assume the change must have been merged at this point, otherwise we would have failed in one of
- * the other steps in postUpdate (which is done prior to this method).
- */
- @Override
- public final void asyncPostUpdate(Context ctx) {
+ // Assume the change must have been merged at this point, otherwise we would
+ // have failed fast in one of the other steps.
try {
args.mergedSenderFactory
.create(
@@ -508,11 +493,18 @@
submitter.accountId(),
ctx.getNotify(getId()),
ctx.getRepoView())
- .send();
+ .sendAsync();
} catch (Exception e) {
logger.atSevere().withCause(e).log("Cannot email merged notification for %s", getId());
}
- asyncPostUpdateImpl(ctx);
+ if (mergeResultRev != null && !args.dryrun) {
+ args.changeMerged.fire(
+ updatedChange,
+ mergedPatchSet,
+ args.accountCache.get(submitter.accountId()).orElse(null),
+ args.mergeTip.getCurrentTip().name(),
+ ctx.getWhen());
+ }
}
/**
@@ -537,12 +529,6 @@
protected void postUpdateImpl(Context ctx) throws Exception {}
/**
- * @see #asyncPostUpdate(Context)
- * @param ctx
- */
- protected void asyncPostUpdateImpl(Context ctx) {}
-
- /**
* Amend the commit with gitlink update
*
* @param commit
diff --git a/java/com/google/gerrit/server/update/AsyncPostUpdateOp.java b/java/com/google/gerrit/server/update/AsyncPostUpdateOp.java
deleted file mode 100644
index 9a989b7..0000000
--- a/java/com/google/gerrit/server/update/AsyncPostUpdateOp.java
+++ /dev/null
@@ -1,38 +0,0 @@
-// Copyright (C) 2020 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.server.update;
-
-import com.google.gerrit.server.config.SysExecutorModule;
-
-/** Base interface for operations performed asynchronously as part of a {@link BatchUpdate}. */
-public interface AsyncPostUpdateOp {
-
- /**
- * Override this method to do something after the update e.g. send emails. This method will be
- * invoked asynchronously, and when invoked, the invoking method will not wait for the async
- * updates to finish. This method will be called after {@link BatchUpdateOp} operations and {@link
- * RepoOnlyOp} are finished.
- *
- * <p>The maximum amount of threads in the thread pool is decided by
- * asyncPostUpdate.threadPoolSize. When asyncPostUpdate.threadPoolSize is not specified, the
- * deprecated sendemail.threadPoolSize is used (see {@link
- * SysExecutorModule#provideSendEmailExecutor}). This is the case for legacy reasons, since in the
- * past only some emails were sent async (and sendemail.threadPoolSize) was used, and now all
- * emails (and possibly others) are done async, so asyncPostUpdate.threadPoolSize is used.
- *
- * @param ctx context
- */
- default void asyncPostUpdate(Context ctx) throws Exception {}
-}
diff --git a/java/com/google/gerrit/server/update/BatchUpdate.java b/java/com/google/gerrit/server/update/BatchUpdate.java
index 2fdc124..166e88d 100644
--- a/java/com/google/gerrit/server/update/BatchUpdate.java
+++ b/java/com/google/gerrit/server/update/BatchUpdate.java
@@ -47,7 +47,6 @@
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.change.NotifyResolver;
-import com.google.gerrit.server.config.AsyncPostUpdateExecutor;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.validators.OnSubmitValidators;
@@ -63,7 +62,6 @@
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.NoSuchRefException;
-import com.google.gerrit.server.util.ThreadLocalRequestContext;
import com.google.inject.Inject;
import com.google.inject.Module;
import com.google.inject.assistedinject.Assisted;
@@ -77,7 +75,6 @@
import java.util.Optional;
import java.util.TimeZone;
import java.util.TreeMap;
-import java.util.concurrent.ExecutorService;
import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.PersonIdent;
@@ -335,8 +332,6 @@
private final NoteDbUpdateManager.Factory updateManagerFactory;
private final ChangeIndexer indexer;
private final GitReferenceUpdated gitRefUpdated;
- private final ThreadLocalRequestContext requestContext;
- private final ExecutorService executorService;
private final Project.NameKey project;
private final CurrentUser user;
@@ -365,8 +360,6 @@
NoteDbUpdateManager.Factory updateManagerFactory,
ChangeIndexer indexer,
GitReferenceUpdated gitRefUpdated,
- ThreadLocalRequestContext requestContext,
- @AsyncPostUpdateExecutor ExecutorService executorService,
@Assisted Project.NameKey project,
@Assisted CurrentUser user,
@Assisted Timestamp when) {
@@ -376,8 +369,6 @@
this.updateManagerFactory = updateManagerFactory;
this.indexer = indexer;
this.gitRefUpdated = gitRefUpdated;
- this.requestContext = requestContext;
- this.executorService = executorService;
this.project = project;
this.user = user;
this.when = when;
@@ -646,40 +637,23 @@
return new ChangeContextImpl(notes);
}
- private void executePostOps() {
+ private void executePostOps() throws Exception {
ContextImpl ctx = new ContextImpl();
for (BatchUpdateOp op : ops.values()) {
- postUpdate(ctx, op);
- if (op instanceof AsyncPostUpdateOp) {
- asyncPostUpdate(ctx, ((AsyncPostUpdateOp) op));
+ try (TraceContext.TraceTimer ignored =
+ TraceContext.newTimer(op.getClass().getSimpleName() + "#postUpdate", Metadata.empty())) {
+ op.postUpdate(ctx);
}
}
for (RepoOnlyOp op : repoOnlyOps) {
- postUpdate(ctx, op);
- if (op instanceof AsyncPostUpdateOp) {
- asyncPostUpdate(ctx, ((AsyncPostUpdateOp) op));
+ try (TraceContext.TraceTimer ignored =
+ TraceContext.newTimer(op.getClass().getSimpleName() + "#postUpdate", Metadata.empty())) {
+ op.postUpdate(ctx);
}
}
}
- /** Invoke the postUpdate methods synchronously. */
- private void postUpdate(ContextImpl ctx, RepoOnlyOp op) {
- try (TraceContext.TraceTimer ignored =
- TraceContext.newTimer(op.getClass().getSimpleName() + "#postUpdate", Metadata.empty())) {
- op.postUpdate(ctx);
- } catch (Exception ex) {
- logDebug(
- String.format(
- "postUpdate for project %s failed for user %s", ctx.getProject(), ctx.getUser()));
- }
- }
-
- /** Invoke the asyncPostUpdate methods asynchronously. */
- private void asyncPostUpdate(ContextImpl ctx, AsyncPostUpdateOp op) {
- executorService.execute(new ExecuteAsyncPostUpdate(op, ctx, user, requestContext));
- }
-
private static void logDebug(String msg) {
// Only log if there is a requestId assigned, since those are the
// expensive/complicated requests like MergeOp. Doing it every time would be
diff --git a/java/com/google/gerrit/server/update/ExecuteAsyncPostUpdate.java b/java/com/google/gerrit/server/update/ExecuteAsyncPostUpdate.java
deleted file mode 100644
index e640ab1..0000000
--- a/java/com/google/gerrit/server/update/ExecuteAsyncPostUpdate.java
+++ /dev/null
@@ -1,65 +0,0 @@
-// Copyright (C) 2020 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.server.update;
-
-import com.google.common.flogger.FluentLogger;
-import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.util.RequestContext;
-import com.google.gerrit.server.util.ThreadLocalRequestContext;
-
-/** Executes {@link AsyncPostUpdateOp#asyncPostUpdate(Context)} on a specific op, asynchronously. */
-public class ExecuteAsyncPostUpdate implements Runnable, RequestContext {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
- private final AsyncPostUpdateOp op;
- private final Context ctx;
- private final CurrentUser user;
- private final ThreadLocalRequestContext threadLocalRequestContext;
-
- ExecuteAsyncPostUpdate(
- AsyncPostUpdateOp op,
- Context ctx,
- CurrentUser user,
- ThreadLocalRequestContext threadLocalRequestContext) {
- this.op = op;
- this.ctx = ctx;
- this.user = user;
- this.threadLocalRequestContext = threadLocalRequestContext;
- }
-
- @Override
- public void run() {
- RequestContext old = threadLocalRequestContext.setContext(this);
- try {
- op.asyncPostUpdate(ctx);
- } catch (Exception e) {
- logger.atSevere().withCause(e).log(
- "Cannot perform async post update for repo %s and user %s",
- ctx.getProject(), ctx.getAccount().account().getName());
- } finally {
- threadLocalRequestContext.setContext(old);
- }
- }
-
- @Override
- public String toString() {
- return "async-post-update";
- }
-
- @Override
- public CurrentUser getUser() {
- return user;
- }
-}
diff --git a/java/com/google/gerrit/server/update/RepoOnlyOp.java b/java/com/google/gerrit/server/update/RepoOnlyOp.java
index f9b41c4..7e9c47e 100644
--- a/java/com/google/gerrit/server/update/RepoOnlyOp.java
+++ b/java/com/google/gerrit/server/update/RepoOnlyOp.java
@@ -30,11 +30,10 @@
default void updateRepo(RepoContext ctx) throws Exception {}
/**
- * Override this method to do something after the update e.g. run hooks. This method will
- * <strong>NOT</strong> be invoked asynchronously. This method will be finished before {@link
- * AsyncPostUpdateOp#asyncPostUpdate} is called.
+ * Override this method to do something after the update e.g. send email or run hooks
*
* @param ctx context
*/
+ // TODO(dborowitz): Support async operations?
default void postUpdate(Context ctx) throws Exception {}
}
diff --git a/java/com/google/gerrit/server/util/AttentionSetEmail.java b/java/com/google/gerrit/server/util/AttentionSetEmail.java
index 2b09c49..56b1dda 100644
--- a/java/com/google/gerrit/server/util/AttentionSetEmail.java
+++ b/java/com/google/gerrit/server/util/AttentionSetEmail.java
@@ -17,7 +17,9 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.AccountState;
+import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.mail.send.AddToAttentionSetSender;
import com.google.gerrit.server.mail.send.AttentionSetSender;
import com.google.gerrit.server.mail.send.MessageIdGenerator;
@@ -25,8 +27,10 @@
import com.google.gerrit.server.update.Context;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
-public class AttentionSetEmail {
+public class AttentionSetEmail implements Runnable, RequestContext {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -51,6 +55,7 @@
Account.Id attentionUserId);
}
+ private ExecutorService sendEmailsExecutor;
private AttentionSetSender sender;
private Context ctx;
private Change change;
@@ -61,12 +66,14 @@
@Inject
AttentionSetEmail(
+ @SendEmailExecutor ExecutorService executor,
@Assisted AttentionSetSender sender,
@Assisted Context ctx,
@Assisted Change change,
@Assisted String reason,
@Assisted MessageIdGenerator.MessageId messageId,
@Assisted Account.Id attentionUserId) {
+ this.sendEmailsExecutor = executor;
this.sender = sender;
this.ctx = ctx;
this.change = change;
@@ -75,7 +82,13 @@
this.attentionUserId = attentionUserId;
}
- public void send() {
+ public void sendAsync() {
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError = sendEmailsExecutor.submit(this);
+ }
+
+ @Override
+ public void run() {
try {
AccountState accountState =
ctx.getUser().isIdentifiedUser() ? ctx.getUser().asIdentifiedUser().state() : null;
@@ -91,4 +104,14 @@
logger.atSevere().withCause(e).log("Cannot email update for change %s", change.getId());
}
}
+
+ @Override
+ public String toString() {
+ return "send-email comments";
+ }
+
+ @Override
+ public CurrentUser getUser() {
+ return ctx.getUser();
+ }
}
diff --git a/java/com/google/gerrit/testing/FakeEmailSender.java b/java/com/google/gerrit/testing/FakeEmailSender.java
index f73020a..fec9b27 100644
--- a/java/com/google/gerrit/testing/FakeEmailSender.java
+++ b/java/com/google/gerrit/testing/FakeEmailSender.java
@@ -128,7 +128,6 @@
}
public synchronized @Nullable Message peekMessage() {
- waitForEmails();
if (messagesRead >= messages.size()) {
return null;
}
@@ -136,14 +135,9 @@
}
public synchronized @Nullable Message nextMessage() {
- waitForEmails();
Message msg = peekMessage();
- readOneMessage();
- return msg;
- }
-
- public synchronized void readOneMessage() {
messagesRead++;
+ return msg;
}
public ImmutableList<Message> getMessages() {
@@ -166,7 +160,7 @@
// a single thread in tests (tricky because most callers just use the
// default executor).
for (WorkQueue.Task<?> task : workQueue.getTasks()) {
- if (task.toString().contains("async-post-update")) {
+ if (task.toString().contains("send-email")) {
try {
task.get();
} catch (ExecutionException | InterruptedException e) {
diff --git a/java/com/google/gerrit/testing/InMemoryModule.java b/java/com/google/gerrit/testing/InMemoryModule.java
index 693fd90..6c9fbed 100644
--- a/java/com/google/gerrit/testing/InMemoryModule.java
+++ b/java/com/google/gerrit/testing/InMemoryModule.java
@@ -47,7 +47,6 @@
import com.google.gerrit.server.config.AllUsersNameProvider;
import com.google.gerrit.server.config.AnonymousCowardName;
import com.google.gerrit.server.config.AnonymousCowardNameProvider;
-import com.google.gerrit.server.config.AsyncPostUpdateExecutor;
import com.google.gerrit.server.config.CanonicalWebUrlModule;
import com.google.gerrit.server.config.CanonicalWebUrlProvider;
import com.google.gerrit.server.config.DefaultUrlFormatter;
@@ -59,6 +58,7 @@
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.GerritServerId;
import com.google.gerrit.server.config.GerritServerIdProvider;
+import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.config.SitePath;
import com.google.gerrit.server.config.TrackingFooters;
import com.google.gerrit.server.config.TrackingFootersProvider;
@@ -279,7 +279,7 @@
@Provides
@Singleton
- @AsyncPostUpdateExecutor
+ @SendEmailExecutor
public ExecutorService createSendEmailExecutor() {
return newDirectExecutorService();
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 0bfd525..d4affb7 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -284,11 +284,6 @@
String fileName = "a_new_file.txt";
String fileContent = "First line\nSecond line\n";
PushOneCommit.Result result = createChange("Add a file", fileName, fileContent);
-
- // Emails are sent here async which triggers cache hits, so we must wait until those email are
- // actually sent.
- sender.clear();
-
String triplet = project.get() + "~master~" + result.getChangeId();
CacheStats startPatch = cloneStats(fileCache.stats());
CacheStats startIntra = cloneStats(intraCache.stats());
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/acceptance/git/AbstractSubmitOnPush.java b/javatests/com/google/gerrit/acceptance/git/AbstractSubmitOnPush.java
index aafe9b9..23bcdec 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractSubmitOnPush.java
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractSubmitOnPush.java
@@ -412,17 +412,17 @@
sender.clear();
result = pushTo(pushSpec + ",submit,notify=" + NotifyHandling.OWNER_REVIEWERS);
result.assertOkStatus();
- assertThatEmailsForChangeCreationAndSubmitWereSent(result.getChangeId(), user, null);
+ assertThatEmailsForChangeCreationAndSubmitWereSent(user, null);
sender.clear();
result = pushTo(pushSpec + ",submit,notify=" + NotifyHandling.ALL);
result.assertOkStatus();
- assertThatEmailsForChangeCreationAndSubmitWereSent(result.getChangeId(), user, null);
+ assertThatEmailsForChangeCreationAndSubmitWereSent(user, null);
sender.clear();
result = pushTo(pushSpec + ",submit"); // default is notify = ALL
result.assertOkStatus();
- assertThatEmailsForChangeCreationAndSubmitWereSent(result.getChangeId(), user, null);
+ assertThatEmailsForChangeCreationAndSubmitWereSent(user, null);
}
@Test
@@ -442,22 +442,19 @@
PushOneCommit.Result result =
pushTo(pushSpec + ",submit,notify=" + NotifyHandling.NONE + ",notify-to=" + user2.email());
result.assertOkStatus();
- assertThatEmailsForChangeCreationAndSubmitWereSent(
- result.getChangeId(), user2, RecipientType.TO);
+ assertThatEmailsForChangeCreationAndSubmitWereSent(user2, RecipientType.TO);
sender.clear();
result =
pushTo(pushSpec + ",submit,notify=" + NotifyHandling.NONE + ",notify-cc=" + user2.email());
result.assertOkStatus();
- assertThatEmailsForChangeCreationAndSubmitWereSent(
- result.getChangeId(), user2, RecipientType.CC);
+ assertThatEmailsForChangeCreationAndSubmitWereSent(user2, RecipientType.CC);
sender.clear();
result =
pushTo(pushSpec + ",submit,notify=" + NotifyHandling.NONE + ",notify-bcc=" + user2.email());
result.assertOkStatus();
- assertThatEmailsForChangeCreationAndSubmitWereSent(
- result.getChangeId(), user2, RecipientType.BCC);
+ assertThatEmailsForChangeCreationAndSubmitWereSent(user2, RecipientType.BCC);
}
private PatchSetApproval getSubmitter(PatchSet.Id patchSetId) throws Exception {
@@ -518,15 +515,15 @@
* sent as "To" and sometimes can be sent as "Cc".
*/
private void assertThatEmailsForChangeCreationAndSubmitWereSent(
- String changeId, TestAccount expected, @Nullable RecipientType expectedRecipientType) {
+ TestAccount expected, @Nullable RecipientType expectedRecipientType) {
String expectedEmail = expected.email();
String expectedFullName = expected.fullName();
Address expectedAddress = Address.create(expectedFullName, expectedEmail);
assertThat(sender.getMessages()).hasSize(2);
- Message message = Iterables.getOnlyElement(sender.getMessages(changeId, "newchange"));
+ Message message = sender.getMessages().get(0);
assertThat(message.body().contains("review")).isTrue();
assertAddress(message, expectedAddress, expectedRecipientType);
- message = Iterables.getOnlyElement(sender.getMessages(changeId, "merged"));
+ message = sender.getMessages().get(1);
assertThat(message.rcpt()).containsExactly(expectedAddress);
assertAddress(message, expectedAddress, expectedRecipientType);
assertThat(message.body().contains("submitted")).isTrue();
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
index e040860..a6bd5eb 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
@@ -28,7 +28,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.RestResponse;
@@ -399,13 +398,13 @@
List<Message> messages = sender.getMessages();
assertThat(messages).hasSize(2);
- Message m = Iterables.getOnlyElement(sender.getMessages(r.getChangeId(), "comment"));
+ Message m = messages.get(0);
assertThat(m.rcpt()).containsExactly(user.getNameEmail(), observer.getNameEmail());
assertThat(m.body()).contains(admin.fullName() + " has posted comments on this change.");
assertThat(m.body()).contains("Change subject: " + PushOneCommit.SUBJECT + "\n");
assertThat(m.body()).contains("Patch Set 1: Code-Review+2");
- m = Iterables.getOnlyElement(sender.getMessages(r.getChangeId(), "newchange"));
+ m = messages.get(1);
assertThat(m.rcpt()).containsExactly(user.getNameEmail(), observer.getNameEmail());
assertThat(m.body()).contains("Hello " + user.fullName() + ",\n");
assertThat(m.body()).contains("I'd like you to do a code review.");
diff --git a/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java b/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java
index 7570ce9..1a01184 100644
--- a/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java
@@ -124,14 +124,12 @@
List<FakeEmailSender.Message> messages = sender.getMessages();
assertThat(messages).hasSize(2);
- FakeEmailSender.Message newPatchsetMessage =
- Iterables.getOnlyElement(sender.getMessages(changeId, "newpatchset"));
+ FakeEmailSender.Message newPatchsetMessage = messages.get(0);
assertThat(newPatchsetMessage.body()).contains("new patch set");
assertThat(newPatchsetMessage.headers().get("Message-ID").toString())
.doesNotContain("EmailReviewComments");
- FakeEmailSender.Message newCommentsMessage =
- Iterables.getOnlyElement(sender.getMessages(changeId, "comment"));
+ FakeEmailSender.Message newCommentsMessage = messages.get(1);
assertThat(newCommentsMessage.body()).contains("has posted comments on this change");
assertThat(newCommentsMessage.headers().get("Message-ID").toString())
.contains("EmailReviewComments");
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]);
+ }
}
diff --git a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts
index 7bcd833..b75c26d 100644
--- a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts
+++ b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts
@@ -127,9 +127,9 @@
/** @override */
created() {
super.created();
- this.addEventListener('content-change', e =>
- this._handleContentChange(e as CustomEvent<{value: string}>)
- );
+ this.addEventListener('content-change', e => {
+ this._handleContentChange(e as CustomEvent<{value: string}>);
+ });
}
/** @override */
diff --git a/polygerrit-ui/app/styles/shared-styles.ts b/polygerrit-ui/app/styles/shared-styles.ts
index 3b738a4..fbc62e2 100644
--- a/polygerrit-ui/app/styles/shared-styles.ts
+++ b/polygerrit-ui/app/styles/shared-styles.ts
@@ -135,7 +135,7 @@
/* Stopgap solution until we remove hidden$ attributes. */
- :host[hidden],
+ :host([hidden]),
[hidden] {
display: none !important;
}
