From bf2133884b4a1babd9949018fbf724c7742056e5 Mon Sep 17 00:00:00 2001 From: Robin Westberg Date: Wed, 5 Aug 2020 11:07:33 +0200 Subject: [PATCH] Filter out commits brought in through a merge PR --- .../mailinglist/MailingListNotifier.java | 40 ++-- .../mailinglist/MailingListNotifierTests.java | 175 ++++++++++++++++++ 2 files changed, 200 insertions(+), 15 deletions(-) diff --git a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/mailinglist/MailingListNotifier.java b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/mailinglist/MailingListNotifier.java index 2a092e8fa..a520d8227 100644 --- a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/mailinglist/MailingListNotifier.java +++ b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/mailinglist/MailingListNotifier.java @@ -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 filterPrCommits(HostedRepository repository, Repository localRepository, List commits, Branch branch) throws NonRetriableException { var ret = new ArrayList(); - var mergedHashes = new HashSet(); + var mergedCommits = new HashSet(); 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 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 commits, Branch branch) throws NonRetriableException { diff --git a/bots/notify/src/test/java/org/openjdk/skara/bots/notify/mailinglist/MailingListNotifierTests.java b/bots/notify/src/test/java/org/openjdk/skara/bots/notify/mailinglist/MailingListNotifierTests.java index 32aa77eb1..46652b999 100644 --- a/bots/notify/src/test/java/org/openjdk/skara/bots/notify/mailinglist/MailingListNotifierTests.java +++ b/bots/notify/src/test/java/org/openjdk/skara/bots/notify/mailinglist/MailingListNotifierTests.java @@ -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();