Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

375: Improve Merge PR RFR email #591

Closed
wants to merge 8 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -5,7 +5,7 @@
import org.openjdk.skara.issuetracker.Comment;
import org.openjdk.skara.vcs.*;

import java.io.IOException;
import java.io.*;
import java.net.URI;
import java.time.ZonedDateTime;
import java.util.*;
@@ -38,10 +38,12 @@ private ArchiveItem(ArchiveItem parent, String id, ZonedDateTime created, ZonedD
this.footer = footer;
}

private static Optional<Commit> mergeCommit(Repository localRepo, Hash head) {
private static Optional<Commit> mergeCommit(PullRequest pr, Repository localRepo, Hash head) {
try {
return localRepo.lookup(head).filter(Commit::isMerge);
} catch (IOException e) {
var author = new Author("duke", "duke@openjdk.org");
var hash = PullRequestUtils.createCommit(pr, localRepo, head, author, author, pr.title());
return localRepo.lookup(hash);
} catch (IOException | CommitFailure e) {
return Optional.empty();
}
}
@@ -51,39 +53,45 @@ static ArchiveItem from(PullRequest pr, Repository localRepo, HostUserToEmailAut
WebrevNotification webrevNotification, ZonedDateTime created, ZonedDateTime updated,
Hash base, Hash head, String subjectPrefix, String threadPrefix) {
return new ArchiveItem(null, "fc", created, updated, pr.author(), Map.of("PR-Head-Hash", head.hex(),
"PR-Base-Hash", base.hex(),
"PR-Thread-Prefix", threadPrefix),
"PR-Base-Hash", base.hex(),
"PR-Thread-Prefix", threadPrefix),
() -> subjectPrefix + threadPrefix + (threadPrefix.isEmpty() ? "" : ": ") + pr.title(),
() -> "",
() -> ArchiveMessages.composeConversation(pr, localRepo, base, head),
() -> ArchiveMessages.composeConversation(pr),
() -> {
var fullWebrev = webrevGenerator.generate(base, head, "00", WebrevDescription.Type.FULL);
if (pr.title().startsWith("Merge")) {
var mergeCommit = mergeCommit(localRepo, head);
if (mergeCommit.isPresent()) {
var mergeWebrevs = new ArrayList<WebrevDescription>();
mergeWebrevs.add(fullWebrev);
for (int i = 0; i < mergeCommit.get().parentDiffs().size(); ++i) {
var diff = mergeCommit.get().parentDiffs().get(i);
switch (i) {
case 0:
mergeWebrevs.add(webrevGenerator.generate(diff, String.format("00.%d", i), WebrevDescription.Type.MERGE_TARGET, pr.targetRef()));
break;
case 1:
var mergeSource = pr.title().length() > 6 ? pr.title().substring(6) : null;
mergeWebrevs.add(webrevGenerator.generate(diff, String.format("00.%d", i), WebrevDescription.Type.MERGE_SOURCE, mergeSource));
break;
default:
mergeWebrevs.add(webrevGenerator.generate(diff, String.format("00.%d", i), WebrevDescription.Type.MERGE_SOURCE, null));
break;
}
}
webrevNotification.notify(0, mergeWebrevs);
return ArchiveMessages.composeMergeConversationFooter(pr, localRepo, mergeWebrevs, base, head);
}
if (PullRequestUtils.isMerge(pr)) {
//TODO: Try to merge in target - generate possible conflict webrev
var mergeCommit = mergeCommit(pr, localRepo, head);
var mergeWebrevs = new ArrayList<WebrevDescription>();
if (mergeCommit.isPresent()) {
for (int i = 0; i < mergeCommit.get().parentDiffs().size(); ++i) {
var diff = mergeCommit.get().parentDiffs().get(i);
if (diff.patches().size() == 0) {
continue;
}
switch (i) {
case 0:
mergeWebrevs.add(webrevGenerator.generate(diff, String.format("00.%d", i), WebrevDescription.Type.MERGE_TARGET, pr.targetRef()));
break;
case 1:
var mergeSource = pr.title().length() > 6 ? pr.title().substring(6) : null;
mergeWebrevs.add(webrevGenerator.generate(diff, String.format("00.%d", i), WebrevDescription.Type.MERGE_SOURCE, mergeSource));
break;
default:
mergeWebrevs.add(webrevGenerator.generate(diff, String.format("00.%d", i), WebrevDescription.Type.MERGE_SOURCE, null));
break;
}
}
if (!mergeWebrevs.isEmpty()) {
webrevNotification.notify(0, mergeWebrevs);
}
}
return ArchiveMessages.composeMergeConversationFooter(pr, localRepo, mergeWebrevs, base, head);
} else {
var fullWebrev = webrevGenerator.generate(base, head, "00", WebrevDescription.Type.FULL);
webrevNotification.notify(0, List.of(fullWebrev));
return ArchiveMessages.composeConversationFooter(pr, issueTracker, issuePrefix, localRepo, fullWebrev, base, head);
}
webrevNotification.notify(0, List.of(fullWebrev));
return ArchiveMessages.composeConversationFooter(pr, issueTracker, issuePrefix, localRepo, fullWebrev, base, head);
});
}

@@ -111,12 +111,21 @@ private static Optional<String> formatCommitMessagesFull(List<CommitMetadata> co
}

private static Optional<String> formatCommitMessagesBrief(List<CommitMetadata> commits) {
return formatCommitMessagesBrief(commits, 100);
}

private static Optional<String> formatCommitMessagesBrief(List<CommitMetadata> commits, int maxEntries) {
if (commits.size() == 0) {
return Optional.empty();
} else {
return Optional.of(commits.stream()
.map(ArchiveMessages::formatCommitBrief)
.collect(Collectors.joining("\n")));
var commitSummary = commits.stream()
.limit(maxEntries)
.map(ArchiveMessages::formatCommitBrief)
.collect(Collectors.joining("\n"));
if (commits.size() > maxEntries) {
commitSummary += "\n - ...omitting " + (commits.size() - maxEntries) + " further commits.";
}
return Optional.of(commitSummary);
}
}

@@ -151,7 +160,7 @@ private static String fetchCommand(PullRequest pr) {
return "git fetch " + repoUrl + " " + pr.fetchRef() + ":pull/" + pr.id();
}

static String composeConversation(PullRequest pr, Repository localRepo, Hash base, Hash head) {
static String composeConversation(PullRequest pr) {
var filteredBody = filterComments(pr.body());
if (filteredBody.isEmpty()) {
filteredBody = pr.title().strip();
@@ -254,22 +263,20 @@ static String composeConversationFooter(PullRequest pr, URI issueProject, String

static String composeMergeConversationFooter(PullRequest pr, Repository localRepo, List<WebrevDescription> webrevs, Hash base, Hash head) {
var commits = commits(localRepo, base, head);
var webrevLinks = "";
if (webrevs.size() > 1) {
webrevLinks = " Webrev: " + webrevs.get(0).uri() + "\n\n" +
"The following webrevs contain only the adjustments done while merging with regards to each parent branch:\n" +
String webrevLinks;
if (webrevs.size() > 0) {
webrevLinks = "The webrev" + (webrevs.size() > 1 ? "s" : "") + " contain" + (webrevs.size() == 1 ? "s" : "") +
" only the adjustments done while merging with regards to each parent branch:\n" +
webrevs.stream()
.skip(1)
.map(d -> String.format(" - %s: %s", d.shortLabel(), d.uri()))
.collect(Collectors.joining("\n")) + "\n\n";
} else {
webrevLinks = " Webrev: " + webrevs.get(0).uri() + "\n\n" +
"The merge commit only contains trivial merges, so no merge-specific webrevs have been generated.\n\n";
webrevLinks = "The merge commit only contains trivial merges, so no merge-specific webrevs have been generated.\n\n";
}
return "Commit messages:\n" +
formatCommitMessagesBrief(commits).orElse("") + "\n\n" +
"Changes: " + pr.changeUrl() + "\n" +
formatCommitMessagesBrief(commits, 10).orElse("") + "\n\n" +
webrevLinks +
"Changes: " + pr.changeUrl() + "\n" +
" Stats: " + stats(localRepo, base, head) + "\n" +
" Patch: " + pr.diffUrl().toString() + "\n" +
" Fetch: " + fetchCommand(pr) + "\n\n" +
@@ -292,20 +292,14 @@ public void run(Path scratchPath) {
// Materialize the PR's source and target ref
var seedPath = bot.seedStorage().orElse(scratchPath.resolve("seeds"));
var hostedRepositoryPool = new HostedRepositoryPool(seedPath);
var repository = pr.repository();
var localRepoPath = scratchPath.resolve("mlbridge-mergebase");
var localRepo = hostedRepositoryPool.checkout(pr, localRepoPath.resolve(repository.name()));
localRepo.fetch(repository.url(), "+" + pr.targetRef() + ":mlbridge_prinstance", false);

var targetHash = pr.targetHash();
var headHash = pr.headHash();
var baseHash = localRepo.mergeBase(targetHash, headHash);
var localRepoPath = scratchPath.resolve("mlbridge-mergebase").resolve(pr.repository().name());
var localRepo = PullRequestUtils.materialize(hostedRepositoryPool, pr, localRepoPath);

var webrevPath = scratchPath.resolve("mlbridge-webrevs");
var listServer = MailingListServerFactory.createMailmanServer(bot.listArchive(), bot.smtpServer(), bot.sendInterval());
var list = listServer.getList(bot.listAddress().address());

var archiver = new ReviewArchive(pr, bot.emailAddress(), baseHash, headHash);
var archiver = new ReviewArchive(pr, bot.emailAddress());

// Regular comments
for (var comment : comments) {
@@ -6,6 +6,7 @@
import org.openjdk.skara.issuetracker.*;
import org.openjdk.skara.vcs.*;

import java.io.IOException;
import java.net.URI;
import java.nio.charset.StandardCharsets;
import java.security.*;
@@ -18,20 +19,16 @@
class ReviewArchive {
private final PullRequest pr;
private final EmailAddress sender;
private final Hash base;
private final Hash head;

private final List<Comment> comments = new ArrayList<>();
private final List<Review> reviews = new ArrayList<>();
private final List<ReviewComment> reviewComments = new ArrayList<>();

private final Logger log = Logger.getLogger("org.openjdk.skara.bots.mlbridge");

ReviewArchive(PullRequest pr, EmailAddress sender, Hash base, Hash head) {
ReviewArchive(PullRequest pr, EmailAddress sender) {
this.pr = pr;
this.sender = sender;
this.base = base;
this.head = head;
}

void addComment(Comment comment) {
@@ -55,7 +52,7 @@ private Optional<ArchiveItem> findPreviousReplyBy(List<ArchiveItem> generated, H
.findAny();
}

private List<ArchiveItem> generateArchiveItems(List<Email> sentEmails, Repository localRepo, URI issueTracker, String issuePrefix, HostUserToEmailAuthor hostUserToEmailAuthor, HostUserToUserName hostUserToUserName, HostUserToRole hostUserToRole, WebrevStorage.WebrevGenerator webrevGenerator, WebrevNotification webrevNotification, String subjectPrefix) {
private List<ArchiveItem> generateArchiveItems(List<Email> sentEmails, Repository localRepo, URI issueTracker, String issuePrefix, HostUserToEmailAuthor hostUserToEmailAuthor, HostUserToUserName hostUserToUserName, HostUserToRole hostUserToRole, WebrevStorage.WebrevGenerator webrevGenerator, WebrevNotification webrevNotification, String subjectPrefix) throws IOException {
var generated = new ArrayList<ArchiveItem>();
Hash lastBase = null;
Hash lastHead = null;
@@ -96,12 +93,13 @@ private List<ArchiveItem> generateArchiveItems(List<Email> sentEmails, Repositor
}

// Check if we're at a revision not previously reported
if (!base.equals(lastBase) || !head.equals(lastHead)) {
var baseHash = PullRequestUtils.baseHash(pr, localRepo);
if (!baseHash.equals(lastBase) || !pr.headHash().equals(lastHead)) {
if (generated.isEmpty()) {
var first = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, issueTracker, issuePrefix, webrevGenerator, webrevNotification, pr.createdAt(), pr.updatedAt(), base, head, subjectPrefix, threadPrefix);
var first = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, issueTracker, issuePrefix, webrevGenerator, webrevNotification, pr.createdAt(), pr.updatedAt(), baseHash, pr.headHash(), subjectPrefix, threadPrefix);
generated.add(first);
} else {
var revision = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, webrevGenerator, webrevNotification, pr.updatedAt(), pr.updatedAt(), lastBase, lastHead, base, head, ++revisionIndex, generated.get(0), subjectPrefix, threadPrefix);
var revision = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, webrevGenerator, webrevNotification, pr.updatedAt(), pr.updatedAt(), lastBase, lastHead, baseHash, pr.headHash(), ++revisionIndex, generated.get(0), subjectPrefix, threadPrefix);
generated.add(revision);
}
}
@@ -230,7 +228,7 @@ private String getStableMessageId(EmailAddress uniqueMessageId) {
return uniqueMessageId.localPart().split("\\.")[0];
}

List<Email> generateNewEmails(List<Email> sentEmails, Duration cooldown, Repository localRepo, URI issueTracker, String issuePrefix, WebrevStorage.WebrevGenerator webrevGenerator, WebrevNotification webrevNotification, HostUserToEmailAuthor hostUserToEmailAuthor, HostUserToUserName hostUserToUserName, HostUserToRole hostUserToRole, String subjectPrefix, Consumer<Instant> retryConsumer) {
List<Email> generateNewEmails(List<Email> sentEmails, Duration cooldown, Repository localRepo, URI issueTracker, String issuePrefix, WebrevStorage.WebrevGenerator webrevGenerator, WebrevNotification webrevNotification, HostUserToEmailAuthor hostUserToEmailAuthor, HostUserToUserName hostUserToUserName, HostUserToRole hostUserToRole, String subjectPrefix, Consumer<Instant> retryConsumer) throws IOException {
var ret = new ArrayList<Email>();
var allItems = generateArchiveItems(sentEmails, localRepo, issueTracker, issuePrefix, hostUserToEmailAuthor, hostUserToUserName, hostUserToRole, webrevGenerator, webrevNotification, subjectPrefix);
var sentItemIds = sentItemIds(sentEmails);