276: Merge style PRs should generate "merge" webrevs

Reviewed-by: ehelin

bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveItem.java

@@ -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);
                                        }
                                    }

bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveMessages.java

@@ -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" +

bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveWorkItem.java

@@ -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),

bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/WebrevDescription.java

@@ -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");
+    }
+}

bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/WebrevNotification.java

@@ -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);
 }