Merge branch 'stable-3.7' into stable-3.8 * stable-3.7: 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: Id0ab08341040a25d01f06b8f983c40c999132766
diff --git a/java/com/google/gerrit/server/mail/send/CommentSender.java b/java/com/google/gerrit/server/mail/send/CommentSender.java index 3711ca2..7eef00f 100644 --- a/java/com/google/gerrit/server/mail/send/CommentSender.java +++ b/java/com/google/gerrit/server/mail/send/CommentSender.java
@@ -61,6 +61,7 @@ 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; @@ -235,7 +236,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", @@ -256,6 +258,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 da59148..aed3f76 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) { @@ -153,9 +164,7 @@ } private Text load(ObjectId tree, String path) - throws MissingObjectException, - IncorrectObjectTypeException, - CorruptObjectException, + throws MissingObjectException, IncorrectObjectTypeException, CorruptObjectException, IOException { if (path == null || Patch.PATCHSET_LEVEL.equals(path)) { return Text.EMPTY;
diff --git a/java/com/google/gerrit/sshd/SshDaemon.java b/java/com/google/gerrit/sshd/SshDaemon.java index af7d22b..572ccfb 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 f728995..552f591 100644 --- a/javatests/com/google/gerrit/acceptance/server/mail/MailSenderIT.java +++ b/javatests/com/google/gerrit/acceptance/server/mail/MailSenderIT.java
@@ -17,14 +17,18 @@ import static com.google.common.truth.Truth.assertThat; import static java.nio.charset.StandardCharsets.UTF_8; +import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.Sandboxed; import com.google.gerrit.acceptance.UseLocalDisk; 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 com.google.gerrit.server.config.SitePaths; import java.net.URI; import java.nio.file.Files; +import java.util.List; import java.util.Map; import javax.inject.Inject; import org.junit.Test; @@ -86,9 +90,33 @@ assertThat(subject).isEqualTo("CUSTOM-TEMPLATE"); } + @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; + } }