Skip to content
Permalink
Browse files
453: Merge commit not included in notification email 
Reviewed-by: ehelin
  • Loading branch information
@rwestberg
rwestberg committed Aug 6, 2020
1 parent 68d7387 commit c7890d6c3d09253e1521f0bc46d1e9f29a8bfb4e
40 bots/notify/src/main/java/org/openjdk/skara/bots/notify/mailinglist/MailingListNotifier.java
View file
@@ -34,6 +34,7 @@
import java.util.*;
import java.util.logging.Logger;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

class MailingListNotifier implements Notifier, RepositoryListener {
private final MailingList list;
@@ -137,17 +138,14 @@ private String tagToSubject(HostedRepository repository, Hash hash, Tag tag) {

private List<Commit> filterPrCommits(HostedRepository repository, Repository localRepository, List<Commit> commits, Branch branch) throws NonRetriableException {
var ret = new ArrayList<Commit>();
var mergedHashes = new HashSet<Hash>();
var mergedCommits = new HashSet<Hash>();

for (var commit : commits) {
if (mergedHashes.contains(commit.hash())) {
log.info("Commit " + commit.hash() + " belongs to a merge PR - skipping");
continue;
}

var candidates = repository.findPullRequestsWithComment(null, "Pushed as commit " + commit.hash() + ".");
if (candidates.size() != 1) {
log.warning("Commit " + commit.hash() + " matches " + candidates.size() + " pull requests - expected 1");
if (candidates.size() > 1) {
log.warning("Commit " + commit.hash() + " matches " + candidates.size() + " pull requests - expected 1");
}
ret.add(commit);
continue;
}
@@ -161,17 +159,29 @@ private List<Commit> filterPrCommits(HostedRepository repository, Repository loc
}

// For a merge PR, many other of these commits could belong here as well
try {
localRepository.fetch(repository.url(), candidate.fetchRef());
var baseHash = PullRequestUtils.baseHash(candidate, localRepository);
var prCommits = localRepository.commitMetadata(baseHash, candidate.headHash());
prCommits.forEach(prCommit -> mergedHashes.add(prCommit.hash()));
} catch (IOException e) {
log.warning("Could not fetch commits from " + prLink + " - cannot see if the belong to the PR");
if (commit.parents().size() > 1) {
if (!PullRequestUtils.isMerge(candidate)) {
log.warning("Merge commit from non-merge PR?");
ret.add(commit);
continue;
}

// For a merge PR, the first parent is always the target branch, so skip that one
for (int i = 1; i < commit.parents().size(); ++i) {
try {
localRepository.commitMetadata(commit.parents().get(0), commit.parents().get(i))
.forEach(c -> mergedCommits.add(c.hash()));
} catch (IOException e) {
log.warning("Unable to check if commits between " + commit.parents().get(0) + " and "
+ commit.parents().get(i) + " were brought in through merging in " + prLink);
}
}
}
}

return ret;
return ret.stream()
.filter(c -> !mergedCommits.contains(c.hash()))
.collect(Collectors.toList());
}

private void sendCombinedCommits(HostedRepository repository, List<Commit> commits, Branch branch) throws NonRetriableException {
175 ...ify/src/test/java/org/openjdk/skara/bots/notify/mailinglist/MailingListNotifierTests.java
View file
@@ -29,6 +29,7 @@
import org.openjdk.skara.test.*;

import java.io.IOException;
import java.nio.file.Files;
import java.time.Duration;
import java.util.*;
import java.util.regex.Pattern;
@@ -181,6 +182,84 @@ void testMailingListMultiple(TestInfo testInfo) throws IOException {
}
}

@Test
void testMailingListMerge(TestInfo testInfo) throws IOException {
try (var listServer = new TestMailmanServer();
var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory()) {
var repo = credentials.getHostedRepository();
var repoFolder = tempFolder.path().resolve("repo");
var localRepo = CheckableRepository.init(repoFolder, repo.repositoryType());
var masterHash = localRepo.resolve("master").orElseThrow();
credentials.commitLock(localRepo);
localRepo.pushAll(repo.url());

var listAddress = EmailAddress.parse(listServer.createList("test"));
var mailmanServer = MailingListServerFactory.createMailmanServer(listServer.getArchive(), listServer.getSMTP(), Duration.ZERO);
var mailmanList = mailmanServer.getList(listAddress.address());
var tagStorage = createTagStorage(repo);
var branchStorage = createBranchStorage(repo);
var prStateStorage = createPullRequestStateStorage(repo);
var storageFolder = tempFolder.path().resolve("storage");

var sender = EmailAddress.from("duke", "duke@duke.duke");
var notifyBot = NotifyBot.newBuilder()
.repository(repo)
.storagePath(storageFolder)
.branches(Pattern.compile("master"))
.tagStorageBuilder(tagStorage)
.branchStorageBuilder(branchStorage)
.prStateStorageBuilder(prStateStorage)
.build();
var updater = MailingListNotifier.newBuilder()
.list(mailmanList)
.recipient(listAddress)
.sender(sender)
.reportNewTags(false)
.reportNewBranches(false)
.reportNewBuilds(false)
.build();
updater.attachTo(notifyBot);

// No mail should be sent on the first run as there is no history
TestBotRunner.runPeriodicItems(notifyBot);
assertThrows(RuntimeException.class, () -> listServer.processIncoming(Duration.ofMillis(1)));

var editHash1 = CheckableRepository.appendAndCommit(localRepo, "Another line", "23456789: More fixes",
"first_author", "first@author.example.com");
localRepo.push(editHash1, repo.url(), "master");
var editHash2 = CheckableRepository.appendAndCommit(localRepo, "Yet another line", "3456789A: Even more fixes",
"another_author", "another@author.example.com");
localRepo.checkout(editHash1, true);
var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Something else");
localRepo.add(unrelated);
localRepo.commit("Unrelated", "unrelated_author", "unrelated@author.example.com");
localRepo.merge(editHash2);
var mergeHash = localRepo.commit("Merge", "merge_author", "merge@author.example.com");
localRepo.push(mergeHash, repo.url(), "master");

TestBotRunner.runPeriodicItems(notifyBot);
listServer.processIncoming();

var conversations = mailmanList.conversations(Duration.ofDays(1));
var email = conversations.get(0).first();
assertEquals(listAddress, email.sender());
assertEquals(EmailAddress.from("merge_author", "merge@author.example.com"), email.author());
assertEquals(email.recipients(), List.of(listAddress));
assertTrue(email.subject().contains(": 4 new changesets"));
assertFalse(email.subject().contains("master"));
assertTrue(email.body().contains("Changeset: " + editHash1.abbreviate()));
assertTrue(email.body().contains("23456789: More fixes"));
assertTrue(email.body().contains("Changeset: " + editHash2.abbreviate()));
assertTrue(email.body().contains("3456789A: Even more fixes"));
assertFalse(email.body().contains(masterHash.abbreviate()));
assertTrue(email.hasHeader("X-Git-URL"));
assertEquals(repo.webUrl().toString(), email.headerValue("X-Git-URL"));
assertTrue(email.hasHeader("X-Git-Changeset"));
assertEquals(editHash1.hex(), email.headerValue("X-Git-Changeset"));
}
}

@Test
void testMailingListSponsored(TestInfo testInfo) throws IOException {
try (var listServer = new TestMailmanServer();
@@ -520,6 +599,102 @@ void testMailingListPR(TestInfo testInfo) throws IOException {
}
}

@Test
void testMailingListMergePR(TestInfo testInfo) throws IOException {
try (var listServer = new TestMailmanServer();
var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory()) {
var repo = credentials.getHostedRepository();
var repoFolder = tempFolder.path().resolve("repo");
var localRepo = CheckableRepository.init(repoFolder, repo.repositoryType());
var masterHash = localRepo.resolve("master").orElseThrow();
credentials.commitLock(localRepo);
localRepo.pushAll(repo.url());

var listAddress = EmailAddress.parse(listServer.createList("test"));
var mailmanServer = MailingListServerFactory.createMailmanServer(listServer.getArchive(), listServer.getSMTP(), Duration.ZERO);
var mailmanList = mailmanServer.getList(listAddress.address());
var tagStorage = createTagStorage(repo);
var branchStorage = createBranchStorage(repo);
var prStateStorage = createPullRequestStateStorage(repo);
var storageFolder = tempFolder.path().resolve("storage");

var sender = EmailAddress.from("duke", "duke@duke.duke");
var notifyBot = NotifyBot.newBuilder()
.repository(repo)
.storagePath(storageFolder)
.branches(Pattern.compile("master"))
.tagStorageBuilder(tagStorage)
.branchStorageBuilder(branchStorage)
.prStateStorageBuilder(prStateStorage)
.build();
var updater = MailingListNotifier.newBuilder()
.list(mailmanList)
.recipient(listAddress)
.sender(sender)
.reportNewTags(false)
.reportNewBranches(false)
.reportNewBuilds(false)
.mode(MailingListNotifier.Mode.PR)
.build();
updater.attachTo(notifyBot);

// No mail should be sent on the first run as there is no history
TestBotRunner.runPeriodicItems(notifyBot);
assertThrows(RuntimeException.class, () -> listServer.processIncoming(Duration.ofMillis(1)));

var editHash1 = CheckableRepository.appendAndCommit(localRepo, "Another line", "23456789: More fixes",
"first_author", "first@author.example.com");
localRepo.push(editHash1, repo.url(), "edit");
CheckableRepository.appendAndCommit(localRepo, "And another line", "12345678: And more fixes",
"second_author", "second@author.example.com");
var editHash2 = CheckableRepository.appendAndCommit(localRepo, "Yet another line", "3456789A: Even more fixes",
"another_author", "another@author.example.com");
localRepo.checkout(editHash1, true);
var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Something else");
localRepo.add(unrelated);
localRepo.commit("Unrelated", "unrelated_author", "unrelated@author.example.com");
localRepo.merge(editHash2);
var mergeHash = localRepo.commit("Merge", "merge_author", "merge@author.example.com");
localRepo.push(mergeHash, repo.url(), "edit");

var pr = credentials.createPullRequest(repo, "master", "edit", "Merge test");

// PR hasn't been integrated yet, so there should be no mail
TestBotRunner.runPeriodicItems(notifyBot);
assertThrows(RuntimeException.class, () -> listServer.processIncoming(Duration.ofMillis(1)));

// Simulate an RFR email
var rfr = Email.create("[repo/branch] RFR: Merge test", "PR:\n" + pr.webUrl().toString())
.author(EmailAddress.from("duke", "duke@duke.duke"))
.recipient(listAddress)
.build();
mailmanList.post(rfr);
listServer.processIncoming();

// And an integration
pr.addComment("Pushed as commit " + mergeHash.hex() + ".");
localRepo.push(mergeHash, repo.url(), "master");

TestBotRunner.runPeriodicItems(notifyBot);
listServer.processIncoming();

var conversations = mailmanList.conversations(Duration.ofDays(1));
conversations.sort(Comparator.comparing(conversation -> conversation.first().subject()));
assertEquals(2, conversations.size());

var prConversation = conversations.get(0);
var pushConversation = conversations.get(1);
assertEquals(1, prConversation.allMessages().size());

var pushEmail = pushConversation.first();
assertEquals(listAddress, pushEmail.sender());
assertEquals(EmailAddress.from("unrelated_author", "unrelated@author.example.com"), pushEmail.author());
assertEquals(pushEmail.recipients(), List.of(listAddress));
assertTrue(pushEmail.subject().contains("2 new changesets"));
}
}

@Test
void testMailingListPROnce(TestInfo testInfo) throws IOException {
try (var listServer = new TestMailmanServer();

1 comment on commit c7890d6

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented on c7890d6 Aug 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Issues

Please sign in to comment.