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

276: Merge style PRs should generate "merge" webrevs #501

Closed
wants to merge 3 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
@@ -38,6 +38,14 @@ private ArchiveItem(ArchiveItem parent, String id, ZonedDateTime created, ZonedD
this.footer = footer;
}

private static Optional<Commit> mergeCommit(Repository localRepo, Hash head) {
try {
return localRepo.lookup(head).filter(Commit::isMerge);
} catch (IOException e) {
return Optional.empty();
}
}

static ArchiveItem from(PullRequest pr, Repository localRepo, HostUserToEmailAuthor hostUserToEmailAuthor,
URI issueTracker, String issuePrefix, WebrevStorage.WebrevGenerator webrevGenerator,
WebrevNotification webrevNotification, ZonedDateTime created, ZonedDateTime updated,
@@ -47,9 +55,33 @@ static ArchiveItem from(PullRequest pr, Repository localRepo, HostUserToEmailAut
() -> "",
() -> ArchiveMessages.composeConversation(pr, localRepo, base, head),
() -> {
var fullWebrev = webrevGenerator.generate(base, head, "00");
webrevNotification.notify(0, fullWebrev, null);
return ArchiveMessages.composeConversationFooter(pr, issueTracker, issuePrefix, localRepo, fullWebrev, base, head);
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);
}
}
webrevNotification.notify(0, List.of(fullWebrev));
return ArchiveMessages.composeConversationFooter(pr, issueTracker, issuePrefix, localRepo, fullWebrev, base, head);
});
}

@@ -93,19 +125,19 @@ static ArchiveItem from(PullRequest pr, Repository localRepo, HostUserToEmailAut
}
},
() -> {
var fullWebrev = webrevGenerator.generate(base, head, String.format("%02d", index));
var fullWebrev = webrevGenerator.generate(base, head, String.format("%02d", index), WebrevDescription.Type.FULL);
if (lastBase.equals(base)) {
var incrementalWebrev = webrevGenerator.generate(lastHead, head, String.format("%02d-%02d", index - 1, index));
webrevNotification.notify(index, fullWebrev, incrementalWebrev);
var incrementalWebrev = webrevGenerator.generate(lastHead, head, String.format("%02d-%02d", index - 1, index), WebrevDescription.Type.INCREMENTAL);
webrevNotification.notify(index, List.of(fullWebrev, incrementalWebrev));
return ArchiveMessages.composeIncrementalFooter(pr, localRepo, fullWebrev, incrementalWebrev, head, lastHead);
} else {
var rebasedLastHead = rebasedLastHead(localRepo, base, lastHead);
if (rebasedLastHead.isPresent()) {
var incrementalWebrev = webrevGenerator.generate(rebasedLastHead.get(), head, String.format("%02d-%02d", index - 1, index));
webrevNotification.notify(index, fullWebrev, incrementalWebrev);
var incrementalWebrev = webrevGenerator.generate(rebasedLastHead.get(), head, String.format("%02d-%02d", index - 1, index), WebrevDescription.Type.INCREMENTAL);
webrevNotification.notify(index, List.of(fullWebrev, incrementalWebrev));
return ArchiveMessages.composeIncrementalFooter(pr, localRepo, fullWebrev, incrementalWebrev, head, lastHead);
} else {
webrevNotification.notify(index, fullWebrev, null);
webrevNotification.notify(index, List.of(fullWebrev));
return ArchiveMessages.composeRebasedFooter(pr, localRepo, fullWebrev, base, head);
}
}
@@ -238,36 +238,60 @@ static String composeReplyFooter(PullRequest pr) {
}

// When changing this, ensure that the PR pattern in the notifier still matches
static String composeConversationFooter(PullRequest pr, URI issueProject, String projectPrefix, Repository localRepo, URI webrev, Hash base, Hash head) {
static String composeConversationFooter(PullRequest pr, URI issueProject, String projectPrefix, Repository localRepo, WebrevDescription webrev, Hash base, Hash head) {
var commits = commits(localRepo, base, head);
var issueString = issueUrl(pr, issueProject, projectPrefix).map(url -> " Issue: " + url + "\n").orElse("");
return "Commit messages:\n" +
formatCommitMessagesBrief(commits).orElse("") + "\n\n" +
"Changes: " + pr.changeUrl() + "\n" +
" Webrev: " + webrev + "\n" +
" Webrev: " + webrev.uri().toString() + "\n" +
issueString +
" Stats: " + stats(localRepo, base, head) + "\n" +
" Patch: " + pr.diffUrl().toString() + "\n" +
" Fetch: " + fetchCommand(pr) + "\n\n" +
composeReplyFooter(pr);
}

static String composeRebasedFooter(PullRequest pr, Repository localRepo, URI fullWebrev, Hash base, Hash head) {
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" +
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";
}
return "Commit messages:\n" +
formatCommitMessagesBrief(commits).orElse("") + "\n\n" +
"Changes: " + pr.changeUrl() + "\n" +
webrevLinks +
" Stats: " + stats(localRepo, base, head) + "\n" +
" Patch: " + pr.diffUrl().toString() + "\n" +
" Fetch: " + fetchCommand(pr) + "\n\n" +
composeReplyFooter(pr);
}

static String composeRebasedFooter(PullRequest pr, Repository localRepo, WebrevDescription fullWebrev, Hash base, Hash head) {
return "Changes: " + pr.changeUrl() + "\n" +
" Webrev: " + fullWebrev.toString() + "\n" +
" Webrev: " + fullWebrev.uri().toString() + "\n" +
" Stats: " + stats(localRepo, base, head) + "\n" +
" Patch: " + pr.diffUrl().toString() + "\n" +
" Fetch: " + fetchCommand(pr) + "\n\n" +
composeReplyFooter(pr);
}

static String composeIncrementalFooter(PullRequest pr, Repository localRepo, URI fullWebrev, URI incrementalWebrev, Hash head, Hash lastHead) {
static String composeIncrementalFooter(PullRequest pr, Repository localRepo, WebrevDescription fullWebrev, WebrevDescription incrementalWebrev, Hash head, Hash lastHead) {
return "Changes:\n" +
" - all: " + pr.changeUrl() + "\n" +
" - new: " + pr.changeUrl(lastHead) + "\n\n" +
"Webrevs:\n" +
" - full: " + fullWebrev.toString() + "\n" +
" - incr: " + incrementalWebrev.toString() + "\n\n" +
" - full: " + fullWebrev.uri().toString() + "\n" +
" - incr: " + incrementalWebrev.uri().toString() + "\n\n" +
" Stats: " + stats(localRepo, lastHead, head) + "\n" +
" Patch: " + pr.diffUrl().toString() + "\n" +
" Fetch: " + fetchCommand(pr) + "\n\n" +
@@ -31,14 +31,13 @@
import org.openjdk.skara.vcs.Repository;

import java.io.*;
import java.net.URI;
import java.nio.file.Path;
import java.time.*;
import java.util.*;
import java.util.function.*;
import java.util.logging.Logger;
import java.util.regex.Pattern;
import java.util.stream.*;
import java.util.stream.Collectors;

class ArchiveWorkItem implements WorkItem {
private final PullRequest pr;
@@ -131,24 +130,24 @@ private boolean ignoreComment(HostUser author, String body) {
private static final String webrevHeaderMarker = "<!-- mlbridge webrev header -->";
private static final String webrevListMarker = "<!-- mlbridge webrev list -->";

private void updateWebrevComment(List<Comment> comments, int index, URI fullWebrev, URI incWebrev) {
private void updateWebrevComment(List<Comment> comments, int index, List<WebrevDescription> webrevs) {
var existing = comments.stream()
.filter(comment -> comment.author().equals(pr.repository().forge().currentUser()))
.filter(comment -> comment.body().contains(webrevCommentMarker))
.findAny();
var webrevDescriptions = webrevs.stream()
.map(d -> String.format("[%s](%s)", d.label(), d.uri()))
.collect(Collectors.joining(" - "));
var comment = webrevCommentMarker + "\n";
comment += webrevHeaderMarker + "\n";
comment += "### Webrevs" + "\n";
comment += webrevListMarker + "\n";
comment += " * " + String.format("%02d", index) + ": [Full](" + fullWebrev.toString() + ")";
if (incWebrev != null) {
comment += " - [Incremental](" + incWebrev.toString() + ")";
}
comment += " * " + String.format("%02d", index) + ": " + webrevDescriptions;
comment += " (" + pr.headHash() + ")\n";

if (existing.isPresent()) {
if (existing.get().body().contains(fullWebrev.toString())) {
log.fine("Webrev link already posted - skipping update");
if (existing.get().body().contains(webrevDescriptions)) {
log.fine("Webrev links already posted - skipping update");
return;
}
var previousListStart = existing.get().body().indexOf(webrevListMarker) + webrevListMarker.length() + 1;
@@ -328,7 +327,7 @@ public void run(Path scratchPath) {

var webrevGenerator = bot.webrevStorage().generator(pr, localRepo, webrevPath);
var newMails = archiver.generateNewEmails(sentMails, bot.cooldown(), localRepo, bot.issueTracker(), jbs.toUpperCase(), webrevGenerator,
(index, full, inc) -> updateWebrevComment(comments, index, full, inc),
(index, webrevs) -> updateWebrevComment(comments, index, webrevs),
user -> getAuthorAddress(census, user),
user -> getAuthorUserName(census, user),
user -> getAuthorRole(census, user),
@@ -0,0 +1,84 @@
/*
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
package org.openjdk.skara.bots.mlbridge;

import java.net.URI;

public class WebrevDescription {
public enum Type {
FULL,
INCREMENTAL,
MERGE_TARGET,
MERGE_SOURCE
}

private final URI uri;
private final Type type;
private final String description;

public WebrevDescription(URI uri, Type type, String description) {
this.uri = uri;
this.type = type;
this.description = description;
}

public WebrevDescription(URI uri, Type type) {
this.uri = uri;
this.type = type;
this.description = null;
}

public URI uri() {
return uri;
}

public String label() {
switch (type) {
case FULL:
return "Full";
case INCREMENTAL:
return "Incremental";
case MERGE_TARGET:
return "Merge target" + (description != null ? " (" + description + ")" : "");
case MERGE_SOURCE:
return "Merge source" + (description != null ? " (" + description + ")" : "");

}
throw new RuntimeException("Unknown type");
}

public String shortLabel() {
switch (type) {
case FULL:
return "full";
case INCREMENTAL:
return "incr";
case MERGE_TARGET:
return description != null ? description : "merge target";
case MERGE_SOURCE:
return description != null ? description : "merge source";

}
throw new RuntimeException("Unknown type");
}
}
@@ -22,9 +22,9 @@
*/
package org.openjdk.skara.bots.mlbridge;

import java.net.URI;
import java.util.List;

@FunctionalInterface
interface WebrevNotification {
void notify(int index, URI full, URI incremental);
void notify(int index, List<WebrevDescription> webrevDescriptions);
}