Merge "Diff against root commits: use ObjectId#zeroId instead of EMPTY_TREE_ID"
diff --git a/java/com/google/gerrit/server/change/FileInfoJsonModule.java b/java/com/google/gerrit/server/change/FileInfoJsonModule.java
index f90bd14..de116bb 100644
--- a/java/com/google/gerrit/server/change/FileInfoJsonModule.java
+++ b/java/com/google/gerrit/server/change/FileInfoJsonModule.java
@@ -14,12 +14,31 @@
package com.google.gerrit.server.change;
+import com.google.gerrit.server.config.GerritServerConfig;
import com.google.inject.AbstractModule;
+import org.eclipse.jgit.lib.Config;
public class FileInfoJsonModule extends AbstractModule {
+ /** Use the new diff cache implementation {@link FileInfoJsonNewImpl}. */
+ private final boolean useNewDiffCache;
+
+ /** Used to dark launch the new diff cache with the list files endpoint. */
+ private final boolean runNewDiffCacheAsync;
+
+ public FileInfoJsonModule(@GerritServerConfig Config cfg) {
+ this.useNewDiffCache =
+ cfg.getBoolean("cache", "diff_cache", "runNewDiffCache_ListFiles", false);
+ this.runNewDiffCacheAsync =
+ cfg.getBoolean("cache", "diff_cache", "runNewDiffCacheAsync_listFiles", false);
+ }
@Override
public void configure() {
- bind(FileInfoJson.class).to(FileInfoJsonComparingImpl.class);
+ if (runNewDiffCacheAsync) {
+ bind(FileInfoJson.class).to(FileInfoJsonComparingImpl.class);
+ return;
+ }
+ bind(FileInfoJson.class)
+ .to(useNewDiffCache ? FileInfoJsonNewImpl.class : FileInfoJsonOldImpl.class);
}
}
diff --git a/java/com/google/gerrit/server/comment/CommentContextCacheImpl.java b/java/com/google/gerrit/server/comment/CommentContextCacheImpl.java
index e12b538..80b4fee 100644
--- a/java/com/google/gerrit/server/comment/CommentContextCacheImpl.java
+++ b/java/com/google/gerrit/server/comment/CommentContextCacheImpl.java
@@ -99,25 +99,26 @@
Iterable<CommentContextKey> inputKeys) {
ImmutableMap.Builder<CommentContextKey, CommentContext> result = ImmutableMap.builder();
- List<CommentContextKey> adjustedKeys =
+ // We do two transformations to the input keys: first we adjust the max context padding, and
+ // second we hash the file path. The transformed keys are used to request context from the
+ // cache. Keeping a map of the original inputKeys to the transformed keys
+ Map<CommentContextKey, CommentContextKey> inputKeysToCacheKeys =
Streams.stream(inputKeys)
- .map(CommentContextCacheImpl::adjustMaxContextPadding)
- .collect(ImmutableList.toImmutableList());
-
- // Convert the input keys to the same keys but with their file paths hashed
- Map<CommentContextKey, CommentContextKey> keysToCacheKeys =
- adjustedKeys.stream()
.collect(
Collectors.toMap(
Function.identity(),
- k -> k.toBuilder().path(Loader.hashPath(k.path())).build()));
+ k ->
+ adjustMaxContextPadding(k)
+ .toBuilder()
+ .path(Loader.hashPath(k.path()))
+ .build()));
try {
ImmutableMap<CommentContextKey, CommentContext> allContext =
- contextCache.getAll(keysToCacheKeys.values());
+ contextCache.getAll(inputKeysToCacheKeys.values());
for (CommentContextKey inputKey : inputKeys) {
- CommentContextKey cacheKey = keysToCacheKeys.get(adjustMaxContextPadding(inputKey));
+ CommentContextKey cacheKey = inputKeysToCacheKeys.get(inputKey);
result.put(inputKey, allContext.get(cacheKey));
}
return result.build();
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 076ba46..5a74c78 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -271,7 +271,7 @@
install(new IgnoreSelfApprovalRule.Module());
install(new ReceiveCommitsModule());
install(new SshAddressesModule());
- install(new FileInfoJsonModule());
+ install(new FileInfoJsonModule(cfg));
install(ThreadLocalRequestContext.module());
install(new ApprovalModule());
diff --git a/java/com/google/gerrit/server/mail/send/EmailArguments.java b/java/com/google/gerrit/server/mail/send/EmailArguments.java
index 7fff232..258c9af 100644
--- a/java/com/google/gerrit/server/mail/send/EmailArguments.java
+++ b/java/com/google/gerrit/server/mail/send/EmailArguments.java
@@ -18,6 +18,7 @@
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.server.AnonymousUser;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.GerritPersonIdentProvider;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.IdentifiedUser.GenericFactory;
@@ -95,6 +96,7 @@
final OutgoingEmailValidator validator;
final boolean addInstanceNameInSubject;
final Provider<String> instanceNameProvider;
+ final Provider<CurrentUser> currentUserProvider;
@Inject
EmailArguments(
@@ -126,7 +128,8 @@
Provider<InternalAccountQuery> accountQueryProvider,
OutgoingEmailValidator validator,
@GerritInstanceName Provider<String> instanceNameProvider,
- @GerritServerConfig Config cfg) {
+ @GerritServerConfig Config cfg,
+ Provider<CurrentUser> currentUserProvider) {
this.server = server;
this.projectCache = projectCache;
this.permissionBackend = permissionBackend;
@@ -157,5 +160,6 @@
this.instanceNameProvider = instanceNameProvider;
this.addInstanceNameInSubject = cfg.getBoolean("sendemail", "addInstanceNameInSubject", false);
+ this.currentUserProvider = currentUserProvider;
}
}
diff --git a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
index f5fb6b0..ddcc0cf 100644
--- a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
+++ b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
@@ -30,6 +30,7 @@
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo.EmailFormat;
import com.google.gerrit.mail.MailHeader;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.change.NotifyResolver;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -127,18 +128,40 @@
Optional<AccountState> fromUser = args.accountCache.get(fromId);
if (fromUser.isPresent()) {
GeneralPreferencesInfo senderPrefs = fromUser.get().generalPreferences();
+ CurrentUser user = args.currentUserProvider.get();
+ boolean isImpersonating = user.isIdentifiedUser() && user.isImpersonating();
+ if (isImpersonating && user.getAccountId() != fromId) {
+ // This should not be possible, if this is the case it means the RequestContext is not
+ // set up correctly.
+ throw new EmailException(
+ String.format(
+ "User %s is sending email from %s, while acting on behalf of %s",
+ user.asIdentifiedUser().getRealUser().getAccountId(),
+ fromId,
+ user.getAccountId()));
+ }
if (senderPrefs != null && senderPrefs.getEmailStrategy() == CC_ON_OWN_COMMENTS) {
- // If we are impersonating a user, make sure they receive a CC of
- // this message so they can always review and audit what we sent
- // on their behalf to others.
+ // Include the sender in email if they enabled email notifications on their own
+ // comments.
//
logger.atFine().log(
"CC email sender %s because the email strategy of this user is %s",
fromUser.get().account().id(), CC_ON_OWN_COMMENTS);
add(RecipientType.CC, fromId);
+ } else if (isImpersonating) {
+ // If we are impersonating a user, make sure they receive a CC of
+ // this message regardless of email strategy, unless email notifications are explicitly
+ // disabled for this user. This way they can always review and audit what we sent
+ // on their behalf to others.
+ logger.atFine().log(
+ "CC email sender %s because the email is sent on behalf of and email notifications"
+ + " are enabled for this user.",
+ fromUser.get().account().id());
+ add(RecipientType.CC, fromId);
+
} else if (!notify.accounts().containsValue(fromId) && rcptTo.remove(fromId)) {
// If they don't want a copy, but we queued one up anyway,
- // drop them from the recipient lists.
+ // drop them from the recipient lists, but only if the user is not being impersonated.
//
logger.atFine().log(
"Not CCing email sender %s because the email strategy of this user is not %s but %s",
diff --git a/java/com/google/gerrit/server/submit/EmailMerge.java b/java/com/google/gerrit/server/submit/EmailMerge.java
index 109c9c3..3d38f6c 100644
--- a/java/com/google/gerrit/server/submit/EmailMerge.java
+++ b/java/com/google/gerrit/server/submit/EmailMerge.java
@@ -16,7 +16,6 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
-import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
import com.google.gerrit.server.CurrentUser;
@@ -42,7 +41,7 @@
EmailMerge create(
Project.NameKey project,
Change change,
- Account.Id submitter,
+ IdentifiedUser submitter,
NotifyResolver.Result notify,
RepoView repoView,
String stickyApprovalDiff);
@@ -51,12 +50,11 @@
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;
private final Change change;
- private final Account.Id submitter;
+ private final IdentifiedUser submitter;
private final NotifyResolver.Result notify;
private final RepoView repoView;
private final String stickyApprovalDiff;
@@ -66,18 +64,16 @@
@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 @Nullable IdentifiedUser submitter,
@Assisted NotifyResolver.Result notify,
@Assisted RepoView repoView,
@Assisted String stickyApprovalDiff) {
this.sendEmailsExecutor = executor;
this.mergedSenderFactory = mergedSenderFactory;
this.requestContext = requestContext;
- this.identifiedUserFactory = identifiedUserFactory;
this.messageIdGenerator = messageIdGenerator;
this.project = project;
this.change = change;
@@ -99,7 +95,7 @@
MergedSender emailSender =
mergedSenderFactory.create(project, change.getId(), Optional.of(stickyApprovalDiff));
if (submitter != null) {
- emailSender.setFrom(submitter);
+ emailSender.setFrom(submitter.getAccountId());
}
emailSender.setNotify(notify);
emailSender.setMessageId(
@@ -120,7 +116,7 @@
@Override
public CurrentUser getUser() {
if (submitter != null) {
- return identifiedUserFactory.create(submitter).getRealUser();
+ return submitter;
}
throw new OutOfScopeException("No user on email thread");
}
diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
index 09470d4..f181c36 100644
--- a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
+++ b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
@@ -503,7 +503,7 @@
.create(
ctx.getProject(),
toMerge.change(),
- submitter.accountId(),
+ args.caller,
ctx.getNotify(getId()),
ctx.getRepoView(),
stickyApprovalDiff)
diff --git a/java/com/google/gerrit/testing/InMemoryModule.java b/java/com/google/gerrit/testing/InMemoryModule.java
index cd5032a..7ca763a1 100644
--- a/java/com/google/gerrit/testing/InMemoryModule.java
+++ b/java/com/google/gerrit/testing/InMemoryModule.java
@@ -250,7 +250,7 @@
install(new RestApiModule());
install(new OAuthRestModule());
install(new DefaultProjectNameLockManager.Module());
- install(new FileInfoJsonModule());
+ install(new FileInfoJsonModule(cfg));
bind(ProjectOperations.class).to(ProjectOperationsImpl.class);
}
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
index 4a8c376..0f50797 100644
--- a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
@@ -89,6 +89,7 @@
.forUpdate()
.add(allow(Permission.FORGE_COMMITTER).ref("refs/*").group(REGISTERED_USERS))
.add(allow(Permission.SUBMIT).ref("refs/*").group(REGISTERED_USERS))
+ .add(allow(Permission.SUBMIT_AS).ref("refs/*").group(REGISTERED_USERS))
.add(allow(Permission.ABANDON).ref("refs/*").group(REGISTERED_USERS))
.add(allowLabel(LabelId.CODE_REVIEW).ref("refs/*").group(REGISTERED_USERS).range(-2, +2))
.update();
@@ -1623,6 +1624,75 @@
assertThat(sender).didNotSend();
}
+ @Test
+ public void mergeOnBehalfOfEmailEnabled_impersonatedOwnerNotified() throws Exception {
+ StagedChange sc = stageChangeReadyForMerge();
+ // If notification is enabled, onBehalfOfUser is always notified.
+ setEmailStrategy(sc.owner, ENABLED);
+ merge(sc.changeId, other, sc.owner, ALL);
+ assertThat(sender)
+ .sent("merged", sc)
+ .to(sc.owner)
+ .cc(sc.reviewer, sc.ccer)
+ .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL)
+ .bcc(sc.starrer)
+ .bcc(ALL_COMMENTS, SUBMITTED_CHANGES)
+ .noOneElse();
+ assertThat(sender).didNotSend();
+ }
+
+ @Test
+ public void mergeOnBehalfOfEmailEnabled_impersonatedReviewerNotified() throws Exception {
+ StagedChange sc = stageChangeReadyForMerge();
+ // If notification is enabled, onBehalfOfUser is always notified.
+ setEmailStrategy(sc.reviewer, ENABLED);
+ merge(sc.changeId, other, sc.reviewer, ALL);
+ assertThat(sender)
+ .sent("merged", sc)
+ .to(sc.owner)
+ .cc(sc.reviewer, sc.ccer)
+ .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL)
+ .bcc(sc.starrer)
+ .bcc(ALL_COMMENTS, SUBMITTED_CHANGES)
+ .noOneElse();
+ assertThat(sender).didNotSend();
+ }
+
+ @Test
+ public void mergeOnBehalfOfReviewerNotifyOwner_impersonatedReviewerInCC() throws Exception {
+ StagedChange sc = stageChangeReadyForMerge();
+ setEmailStrategy(sc.reviewer, ENABLED);
+ // Even though Submit strategy is OWNER, impersonated reviewer is added to CC.
+ merge(sc.changeId, other, sc.reviewer, OWNER);
+ assertThat(sender).sent("merged", sc).to(sc.owner).cc(sc.reviewer).noOneElse();
+ assertThat(sender).didNotSend();
+ }
+
+ @Test
+ public void mergeOnBehalfOfOtherNotifyOwner_impersonatedOtherInCC() throws Exception {
+ StagedChange sc = stageChangeReadyForMerge();
+ // Unrelated impersonated user is added to CC.
+ merge(sc.changeId, sc.reviewer, other, OWNER);
+ assertThat(sender).sent("merged", sc).to(sc.owner).cc(other).noOneElse();
+ assertThat(sender).didNotSend();
+ }
+
+ @Test
+ public void mergeOnBehalfOfEmailDisabled_doesNotNotify() throws Exception {
+ StagedChange sc = stageChangeReadyForMerge();
+ setEmailStrategy(other, EmailStrategy.DISABLED);
+ merge(sc.changeId, sc.reviewer, other, OWNER);
+ assertThat(sender).sent("merged", sc).to(sc.owner).noOneElse();
+ assertThat(sender).didNotSend();
+ }
+
+ @Test
+ public void mergeOnBehalfOfNotifyNone() throws Exception {
+ StagedChange sc = stageChangeReadyForMerge();
+ merge(sc.changeId, other, sc.owner, NONE);
+ assertThat(sender).didNotSend();
+ }
+
private void merge(String changeId, TestAccount by) throws Exception {
merge(changeId, by, ENABLED);
}
@@ -1648,6 +1718,20 @@
gApi.changes().id(changeId).current().submit(in);
}
+ private void merge(String changeId, TestAccount by, TestAccount onBehalfOf) throws Exception {
+ merge(changeId, by, onBehalfOf, /*notify=*/ null);
+ }
+
+ private void merge(
+ String changeId, TestAccount by, TestAccount onBehalfOf, @Nullable NotifyHandling notify)
+ throws Exception {
+ requestScopeOperations.setApiUser(by.id());
+ SubmitInput in = new SubmitInput();
+ in.notify = notify;
+ in.onBehalfOf = onBehalfOf.id().toString();
+ gApi.changes().id(changeId).current().submit(in);
+ }
+
private StagedChange stageChangeReadyForMerge() throws Exception {
StagedChange sc = stageReviewableChange();
requestScopeOperations.setApiUser(sc.reviewer.id());
