Merge branch 'stable-3.5' into stable-3.6
* stable-3.5:
Make MailSenderIT.outgoingMailWithACommentsInUnchangedFile less fragile
Fix the email notification for comments unchanged files
Fix timeouts calculation in SshDaemon
Add IT test for email notifications with comments on unchanged files
Revert "ApprovalInference: Ignore approvals given on a non-current patchset"
ApprovalInference: Ignore approvals given on a non-current patchset
Update git submodules
Release-Notes: skip
Change-Id: Ie9c7278de7e282a59f9bf5c78f1ed31db6cfd3ff
diff --git a/java/com/google/gerrit/server/mail/send/CommentSender.java b/java/com/google/gerrit/server/mail/send/CommentSender.java
index ce5438b..0006fa0 100644
--- a/java/com/google/gerrit/server/mail/send/CommentSender.java
+++ b/java/com/google/gerrit/server/mail/send/CommentSender.java
@@ -54,7 +54,9 @@
import java.util.Map;
import java.util.Optional;
import org.apache.james.mime4j.dom.field.FieldName;
+import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
/** Send comments, after the author of them hit used Publish Comments in the UI. */
@@ -206,7 +208,8 @@
groups.add(currentGroup);
if (modifiedFiles != null && !modifiedFiles.isEmpty()) {
try {
- currentGroup.fileData = new PatchFile(repo, modifiedFiles, c.key.filename);
+ currentGroup.fileData =
+ loadPatchFile(repo, modifiedFiles, c.key.filename, patchSet.commitId());
} catch (IOException e) {
logger.atWarning().withCause(e).log(
"Cannot load %s from %s in %s",
@@ -227,6 +230,28 @@
return groups;
}
+ private PatchFile loadPatchFile(
+ Repository repo,
+ Map<String, FileDiffOutput> modifiedFiles,
+ String fileName,
+ ObjectId commitId)
+ throws IOException {
+ try {
+ return new PatchFile(repo, modifiedFiles, fileName);
+ } catch (MissingObjectException e) {
+ // check if the file has not been modified then is an unchanged file
+ if (!isModifiedFile(modifiedFiles, fileName)) {
+ return new PatchFile(repo, fileName, commitId);
+ }
+ throw e;
+ }
+ }
+
+ private boolean isModifiedFile(Map<String, FileDiffOutput> modifiedFiles, String fileName) {
+ return modifiedFiles.values().stream()
+ .anyMatch(f -> f.newPath().map(v -> v.equals(fileName)).orElse(false));
+ }
+
/** Get the set of accounts whose comments have been replied to in this email. */
private HashSet<Account.Id> getReplyAccounts() {
HashSet<Account.Id> replyAccounts = new HashSet<>();
diff --git a/java/com/google/gerrit/server/patch/PatchFile.java b/java/com/google/gerrit/server/patch/PatchFile.java
index 7a8180bd..6c7da25 100644
--- a/java/com/google/gerrit/server/patch/PatchFile.java
+++ b/java/com/google/gerrit/server/patch/PatchFile.java
@@ -114,6 +114,17 @@
}
}
+ public PatchFile(Repository repo, String fileName, ObjectId patchSetCommitId) throws IOException {
+ this.repo = repo;
+ this.diff = FileDiffOutput.empty(fileName, patchSetCommitId, patchSetCommitId);
+ try (ObjectReader reader = repo.newObjectReader();
+ RevWalk rw = new RevWalk(reader)) {
+ final RevCommit bCommit = rw.parseCommit(diff.newCommitId());
+ this.aTree = bCommit.getTree();
+ this.bTree = bCommit.getTree();
+ }
+ }
+
private String getOldName() {
String name = FilePathAdapter.getOldPath(diff.oldPath(), diff.changeType());
if (name != null) {
diff --git a/java/com/google/gerrit/sshd/SshDaemon.java b/java/com/google/gerrit/sshd/SshDaemon.java
index cc35a32..01b5ee8 100644
--- a/java/com/google/gerrit/sshd/SshDaemon.java
+++ b/java/com/google/gerrit/sshd/SshDaemon.java
@@ -15,7 +15,6 @@
package com.google.gerrit.sshd;
import static com.google.gerrit.server.ssh.SshAddressesModule.IANA_SSH_PORT;
-import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static java.util.concurrent.TimeUnit.SECONDS;
import static org.apache.sshd.core.CoreModuleProperties.AUTH_TIMEOUT;
import static org.apache.sshd.core.CoreModuleProperties.IDLE_TIMEOUT;
@@ -184,23 +183,21 @@
AUTH_TIMEOUT.set(
this,
Duration.ofSeconds(
- MILLISECONDS.convert(
- ConfigUtil.getTimeUnit(cfg, "sshd", null, "loginGraceTime", 120, SECONDS),
- SECONDS)));
+ ConfigUtil.getTimeUnit(cfg, "sshd", null, "loginGraceTime", 120, SECONDS)));
long idleTimeoutSeconds = ConfigUtil.getTimeUnit(cfg, "sshd", null, "idleTimeout", 0, SECONDS);
- IDLE_TIMEOUT.set(this, Duration.ofSeconds(SECONDS.toMillis(idleTimeoutSeconds)));
- NIO2_READ_TIMEOUT.set(this, Duration.ofSeconds(SECONDS.toMillis(idleTimeoutSeconds)));
+ IDLE_TIMEOUT.set(this, Duration.ofSeconds(idleTimeoutSeconds));
+ NIO2_READ_TIMEOUT.set(this, Duration.ofSeconds(idleTimeoutSeconds));
long rekeyTimeLimit =
ConfigUtil.getTimeUnit(cfg, "sshd", null, "rekeyTimeLimit", 3600, SECONDS);
- REKEY_TIME_LIMIT.set(this, Duration.ofSeconds(SECONDS.toMillis(rekeyTimeLimit)));
+ REKEY_TIME_LIMIT.set(this, Duration.ofSeconds(rekeyTimeLimit));
REKEY_BYTES_LIMIT.set(
this, cfg.getLong("sshd", "rekeyBytesLimit", 1024 * 1024 * 1024 /* 1GB */));
long waitTimeoutSeconds = ConfigUtil.getTimeUnit(cfg, "sshd", null, "waitTimeout", 30, SECONDS);
- WAIT_FOR_SPACE_TIMEOUT.set(this, Duration.ofSeconds(SECONDS.toMillis(waitTimeoutSeconds)));
+ WAIT_FOR_SPACE_TIMEOUT.set(this, Duration.ofSeconds(waitTimeoutSeconds));
final int maxConnectionsPerUser = cfg.getInt("sshd", "maxConnectionsPerUser", 64);
if (0 < maxConnectionsPerUser) {
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java b/javatests/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java
index cb1a679..97d3cd8 100644
--- a/javatests/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java
@@ -14,19 +14,27 @@
package com.google.gerrit.acceptance.server.mail;
+import static com.google.common.truth.Truth.assertThat;
+
import com.google.common.collect.ImmutableList;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.extensions.api.changes.DraftInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
+import com.google.gerrit.extensions.api.changes.RevisionApi;
import com.google.gerrit.extensions.client.Comment;
import com.google.gerrit.extensions.client.Side;
+import com.google.gerrit.extensions.common.ChangeInput;
+import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.mail.MailMessage;
import com.google.inject.Inject;
import java.time.Instant;
import java.util.HashMap;
+import java.util.List;
+import org.eclipse.jgit.lib.Constants;
import org.junit.Ignore;
@Ignore
@@ -69,6 +77,64 @@
return changeId;
}
+ String createChangeWithUnchangedFileReviewed(
+ TestAccount reviewer, String adminInlineComment, String userInlineComment) throws Exception {
+
+ // Create a change with a file and merge it
+ String fileContent = "this is line 1 \nthis is line 2 \nthis is line 3 \nthis is line 4";
+ PushOneCommit.Result firstChangeResult = createChange("First Change", FILE_NAME, fileContent);
+ firstChangeResult.assertOkStatus();
+ merge(firstChangeResult);
+
+ // Create an second empty change
+ String secondChangeId =
+ gApi.changes()
+ .create(new ChangeInput(project.get(), Constants.MASTER, "Second Change"))
+ .get()
+ .id;
+ RevisionApi secondChangeRevision = gApi.changes().id(secondChangeId).current();
+
+ // Add draft
+ DraftInput draftInputFromAdmin =
+ createInlineDraftInLine1(adminInlineComment, Side.REVISION, null);
+ secondChangeRevision.createDraft(draftInputFromAdmin);
+
+ // Review change
+ ReviewInput adminInput = new ReviewInput();
+ adminInput.drafts = ReviewInput.DraftHandling.PUBLISH_ALL_REVISIONS;
+ secondChangeRevision.review(adminInput);
+
+ requestScopeOperations.setApiUser(reviewer.id());
+
+ // Add draft
+ List<CommentInfo> comments = gApi.changes().id(secondChangeId).commentsRequest().getAsList();
+ assertThat(comments).hasSize(1);
+ CommentInfo adminCommentInfo = comments.get(0);
+ DraftInput draftInputFromUser =
+ createInlineDraftInLine1(userInlineComment, Side.REVISION, adminCommentInfo.id);
+ gApi.changes().id(secondChangeId).current().createDraft(draftInputFromUser);
+
+ // Review change
+ ReviewInput reviewerInput = new ReviewInput();
+ reviewerInput.drafts = ReviewInput.DraftHandling.PUBLISH_ALL_REVISIONS;
+ gApi.changes().id(secondChangeId).current().review(reviewerInput);
+
+ return secondChangeId;
+ }
+
+ private DraftInput createInlineDraftInLine1(String comment, Side side, String inReplyTo) {
+ DraftInput draftInput = new DraftInput();
+ draftInput.line = 1;
+ draftInput.message = comment;
+ draftInput.path = FILE_NAME;
+ draftInput.side = side;
+ draftInput.unresolved = true;
+ draftInput.patchSet = 1;
+ draftInput.inReplyTo = inReplyTo;
+
+ return draftInput;
+ }
+
protected static CommentInput newComment(String path, Side side, int line, String message) {
CommentInput c = new CommentInput();
c.path = path;
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/MailSenderIT.java b/javatests/com/google/gerrit/acceptance/server/mail/MailSenderIT.java
index 45a471b..9322502 100644
--- a/javatests/com/google/gerrit/acceptance/server/mail/MailSenderIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/mail/MailSenderIT.java
@@ -16,10 +16,14 @@
import static com.google.common.truth.Truth.assertThat;
+import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.entities.EmailHeader;
import com.google.gerrit.entities.EmailHeader.StringEmailHeader;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.CommentInfo;
import java.net.URI;
+import java.util.List;
import java.util.Map;
import org.junit.Test;
@@ -63,9 +67,33 @@
assertThat(headerString(headers, "In-Reply-To")).isEqualTo(threadId);
}
+ @Test
+ public void outgoingMailWithACommentsInUnchangedFile() throws Exception {
+ String adminInlineComment = "comment from admin in Line 1";
+ String userInlineComment =
+ String.format("reply from user %s to comment made by admin in Line 1", user.fullName());
+ String changeId =
+ createChangeWithUnchangedFileReviewed(user, adminInlineComment, userInlineComment);
+ List<CommentInfo> comments = gApi.changes().id(changeId).commentsRequest().getAsList();
+ assertThat(comments).hasSize(2);
+ String lastCommentId = Iterables.getLast(comments).id;
+ ChangeInfo changeInfo = gApi.changes().id(changeId).get();
+ String expectedBodyAsString =
+ "PS1, Line 1: this is line 1 \n" + "> " + adminInlineComment + "\n" + userInlineComment;
+ assertThat(sender.getMessages()).hasSize(1);
+ String bodyAsString = sender.getMessages().iterator().next().body();
+ assertThat(bodyAsString).contains(expectedBodyAsString);
+ assertThat(bodyAsString).contains(getChangeUrl(changeInfo) + "/comment/" + lastCommentId);
+ assertThat(bodyAsString).contains("File gerrit-server/test.txt:\n");
+ }
+
private String headerString(Map<String, EmailHeader> headers, String name) {
EmailHeader header = headers.get(name);
assertThat(header).isInstanceOf(StringEmailHeader.class);
return ((StringEmailHeader) header).getString();
}
+
+ private String getChangeUrl(ChangeInfo changeInfo) {
+ return canonicalWebUrl.get() + "c/" + changeInfo.project + "/+/" + changeInfo._number;
+ }
}
