542: Adjust label (cc) command logic

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandWorkItem.java

@@ -213,7 +213,8 @@ public Collection<WorkItem> run(Path scratchPath) {
         var nextCommand = nextCommand(pr, comments);
         if (nextCommand.isEmpty()) {
             log.info("No new non-external PR commands found, stopping further processing");
-            return List.of();
+            // When all commands are processed, it's time to check labels
+            return List.of(new LabelerWorkItem(bot, pr, errorHandler));
         }
 
         var census = CensusInstance.create(bot.censusRepo(), bot.censusRef(), scratchPath.resolve("census"), pr);

bots/pr/src/main/java/org/openjdk/skara/bots/pr/LabelCommand.java

@@ -49,15 +49,6 @@ private void showHelp(LabelConfiguration labelConfiguration, PrintWriter reply)
         labelConfiguration.allowed().forEach(label -> reply.println(" * `" + label + "`"));
     }
 
-    private Set<String> automaticLabels(PullRequestBot bot, PullRequest pr, Path scratchPath) throws IOException {
-        var path = scratchPath.resolve("pr").resolve("labelcommand").resolve(pr.repository().name());
-        var seedPath = bot.seedStorage().orElse(scratchPath.resolve("seeds"));
-        var hostedRepositoryPool = new HostedRepositoryPool(seedPath);
-        var localRepo = PullRequestUtils.materialize(hostedRepositoryPool, pr, path);
-        var files = PullRequestUtils.changedFiles(pr, localRepo);
-        return bot.labelConfiguration().fromChanges(files);
-    }
-
     @Override
     public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List<Comment> allComments, PrintWriter reply) {
         if (!command.user().equals(pr.author()) && (!censusInstance.isCommitter(command.user()))) {
@@ -88,28 +79,21 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
             for (var label : labels) {
                 if (!currentLabels.contains(label)) {
                     pr.addLabel(label);
+                    reply.println(LabelTracker.addLabelMarker(label));
                     reply.println("The `" + label + "` label was successfully added.");
                 } else {
                     reply.println("The `" + label + "` label was already applied.");
                 }
             }
         } else if (argumentMatcher.group(1).equals("remove")) {
-            try {
-                var automaticLabels = automaticLabels(bot, pr, scratchPath);
-                for (var label : labels) {
-                    if (currentLabels.contains(label)) {
-                        if (automaticLabels.contains(label)) {
-                            reply.println("The `" + label + "` label was automatically added and cannot be removed.");
-                        } else {
-                            pr.removeLabel(label);
-                            reply.println("The `" + label + "` label was successfully removed.");
-                        }
-                    } else {
-                        reply.println("The `" + label + "` label was not set.");
-                    }
+            for (var label : labels) {
+                if (currentLabels.contains(label)) {
+                    pr.removeLabel(label);
+                    reply.println(LabelTracker.removeLabelMarker(label));
+                    reply.println("The `" + label + "` label was successfully removed.");
+                } else {
+                    reply.println("The `" + label + "` label was not set.");
                 }
-            } catch (IOException e) {
-                reply.println("An error occurred when trying to check automatically added labels");
             }
         }
     }

bots/pr/src/main/java/org/openjdk/skara/bots/pr/LabelConfiguration.java

@@ -87,21 +87,22 @@ public Set<String> fromChanges(Set<Path> changes) {
             }
         }
 
-        // If the current labels matches at least two members of a group, the group is also included
+        var ret = new HashSet<>(labels);
+        // If the current labels matches at least two members of a group, use the group instead
         for (var group : groups.entrySet()) {
             var count = 0;
             for (var groupEntry : group.getValue()) {
-                if (labels.contains(groupEntry)) {
+                if (ret.contains(groupEntry)) {
                     count++;
                     if (count == 2) {
-                        labels.add(group.getKey());
+                        ret.add(group.getKey());
+                        ret.removeAll(group.getValue());
                         break;
                     }
                 }
             }
         }
-
-        return labels;
+        return ret;
     }
 
     public Set<String> allowed() {

bots/pr/src/main/java/org/openjdk/skara/bots/pr/LabelTracker.java

@@ -0,0 +1,88 @@
+/*
+ * 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.pr;
+
+import org.openjdk.skara.host.HostUser;
+import org.openjdk.skara.issuetracker.Comment;
+
+import java.util.*;
+import java.util.regex.*;
+import java.util.stream.Collectors;
+
+public class LabelTracker {
+    private final static String addMarker = "<!-- added label: '%s' -->";
+    private final static String removeMarker = "<!-- removed label: '%s' -->";
+    private final static Pattern labelMarkerPattern = Pattern.compile("<!-- (added|removed) label: '(.*?)' -->");
+
+    static String addLabelMarker(String label) {
+        return String.format(addMarker, label);
+    }
+
+    static String removeLabelMarker(String label) {
+        return String.format(removeMarker, label);
+    }
+
+    // Return all manually added labels, but filter any explicitly removed ones
+    static Set<String> currentAdded(HostUser botUser, List<Comment> comments) {
+        var labelActions = comments.stream()
+                .filter(comment -> comment.author().equals(botUser))
+                .flatMap(comment -> comment.body().lines())
+                .map(labelMarkerPattern::matcher)
+                .filter(Matcher::find)
+                .collect(Collectors.toList());
+
+        var ret = new HashSet<String>();
+        for (var actionMatch : labelActions) {
+            var action = actionMatch.group(1);
+            if (action.equals("added")) {
+                ret.add(actionMatch.group(2));
+            } else {
+                ret.remove(actionMatch.group(2));
+            }
+        }
+
+        return Collections.unmodifiableSet(ret);
+    }
+
+    // Return all manually removed labels, but filter any explicitly added ones
+    static Set<String> currentRemoved(HostUser botUser, List<Comment> comments) {
+        var labelActions = comments.stream()
+                                   .filter(comment -> comment.author().equals(botUser))
+                                   .flatMap(comment -> comment.body().lines())
+                                   .map(labelMarkerPattern::matcher)
+                                   .filter(Matcher::find)
+                                   .collect(Collectors.toList());
+
+        var ret = new HashSet<String>();
+        for (var actionMatch : labelActions) {
+            var action = actionMatch.group(1);
+            if (action.equals("removed")) {
+                ret.add(actionMatch.group(2));
+            } else {
+                ret.remove(actionMatch.group(2));
+            }
+        }
+
+        return Collections.unmodifiableSet(ret);
+    }
+}

bots/pr/src/main/java/org/openjdk/skara/bots/pr/LabelerWorkItem.java

@@ -62,14 +62,26 @@ public Collection<WorkItem> run(Path scratchPath) {
                                   .filter(key -> bot.labelConfiguration().allowed().contains(key))
                                   .collect(Collectors.toSet());
 
-            // Add all labels not already set
+            var comments = pr.comments();
+            var manuallyAdded = LabelTracker.currentAdded(pr.repository().forge().currentUser(), comments);
+            var manuallyRemoved = LabelTracker.currentRemoved(pr.repository().forge().currentUser(), comments);
+
+            // If a manual label command has been issued before we have done any labeling,
+            // that is considered to be a request to override any automatic labelling
+            if (bot.currentLabels().isEmpty() && !(manuallyAdded.isEmpty() && manuallyRemoved.isEmpty())) {
+                return List.of();
+            }
+
+            // Add all labels not already set that are not manually removed
             newLabels.stream()
                      .filter(label -> !currentLabels.contains(label))
+                     .filter(label -> !manuallyRemoved.contains(label))
                      .forEach(pr::addLabel);
 
-            // Remove set labels no longer present
+            // Remove set labels no longer present unless it has been manually added
             currentLabels.stream()
                          .filter(label -> !newLabels.contains(label))
+                         .filter(label -> !manuallyAdded.contains(label))
                          .forEach(pr::removeLabel);
 
             bot.currentLabels().put(pr.headHash(), Boolean.TRUE);

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBot.java

@@ -119,7 +119,6 @@ private List<WorkItem> getWorkItems(List<PullRequest> pullRequests) {
                 }
 
                 ret.add(new CheckWorkItem(this, pr, e -> updateCache.invalidate(pr)));
-                ret.add(new LabelerWorkItem(this, pr, e -> updateCache.invalidate(pr)));
             }
         }
 

bots/pr/src/test/java/org/openjdk/skara/bots/pr/LabelConfigurationTests.java

@@ -57,6 +57,6 @@ void group() {
 
         assertEquals(Set.of("1"), config.fromChanges(Set.of(Path.of("a.cpp"))));
         assertEquals(Set.of("2"), config.fromChanges(Set.of(Path.of("a.hpp"))));
-        assertEquals(Set.of("1", "2", "both"), config.fromChanges(Set.of(Path.of("a.cpp"), Path.of("a.hpp"))));
+        assertEquals(Set.of("both"), config.fromChanges(Set.of(Path.of("a.cpp"), Path.of("a.hpp"))));
     }
 }

bots/pr/src/test/java/org/openjdk/skara/bots/pr/LabelTests.java

@@ -125,7 +125,7 @@ void simple(TestInfo testInfo) throws IOException {
     }
 
     @Test
-    void removeAutoApplied(TestInfo testInfo) throws IOException {
+    void adjustAutoApplied(TestInfo testInfo) throws IOException {
         try (var credentials = new HostCredentials(testInfo);
              var tempFolder = new TemporaryDirectory()) {
             var author = credentials.getHostedRepository();
@@ -165,23 +165,88 @@ void removeAutoApplied(TestInfo testInfo) throws IOException {
             // It will refuse to remove it
             pr.addComment("/label remove 2");
             TestBotRunner.runPeriodicItems(prBot);
-            assertLastCommentContains(pr, "The `2` label was automatically added and cannot be removed.");
+            assertLastCommentContains(pr, "The `2` label was successfully removed.");
 
             // Add another file to trigger a group match
             Files.writeString(localRepoFolder.resolve("test.cpp"), "Hello there");
             localRepo.add(Path.of("test.cpp"));
             editHash = localRepo.commit("Another one", "duke", "duke@openjdk.org");
             localRepo.push(editHash, author.url(), "edit");
 
-            // The bot should have applied more labels automatically
+            // The bot should have applied more labels automatically (but not the manually removed)
             TestBotRunner.runPeriodicItems(prBot);
-            assertEquals(Set.of("1", "2", "group", "rfr"), new HashSet<>(pr.labels()));
+            assertEquals(Set.of("group", "rfr"), new HashSet<>(pr.labels()));
+
+            // Remove one of these as well
+            pr.addComment("/label remove group");
+            TestBotRunner.runPeriodicItems(prBot);
+            assertLastCommentContains(pr, "The `group` label was successfully removed.");
+            assertEquals(Set.of("rfr"), new HashSet<>(pr.labels()));
 
-            // It will refuse to remove these as well
-            pr.addComment("/label remove group, 1");
+            // Adding them back again is fine
+            pr.addComment("/label add group 1");
             TestBotRunner.runPeriodicItems(prBot);
-            assertLastCommentContains(pr, "The `1` label was automatically added and cannot be removed.");
-            assertLastCommentContains(pr, "The `group` label was automatically added and cannot be removed.");
+            assertLastCommentContains(pr, "The `group` label was successfully added.");
+            assertLastCommentContains(pr, "The `1` label was successfully added.");
+            assertEquals(Set.of("1", "group", "rfr"), new HashSet<>(pr.labels()));
+        }
+    }
+
+    @Test
+    void overrideAutoApplied(TestInfo testInfo) throws IOException {
+        try (var credentials = new HostCredentials(testInfo);
+             var tempFolder = new TemporaryDirectory()) {
+            var author = credentials.getHostedRepository();
+            var integrator = credentials.getHostedRepository();
+
+            var censusBuilder = credentials.getCensusBuilder()
+                                           .addReviewer(integrator.forge().currentUser().id())
+                                           .addCommitter(author.forge().currentUser().id());
+            var labelConfiguration = LabelConfiguration.newBuilder()
+                                                       .addMatchers("1", List.of(Pattern.compile("cpp$")))
+                                                       .addMatchers("2", List.of(Pattern.compile("hpp$")))
+                                                       .addGroup("group", List.of("1", "2"))
+                                                       .addExtra("extra")
+                                                       .build();
+            var prBot = PullRequestBot.newBuilder()
+                                      .repo(integrator)
+                                      .censusRepo(censusBuilder.build())
+                                      .labelConfiguration(labelConfiguration)
+                                      .build();
+
+            // Populate the projects repository
+            var localRepoFolder = tempFolder.path().resolve("localrepo");
+            var localRepo = CheckableRepository.init(localRepoFolder, author.repositoryType(), Path.of("test.hpp"));
+            var masterHash = localRepo.resolve("master").orElseThrow();
+            assertFalse(CheckableRepository.hasBeenEdited(localRepo));
+            localRepo.push(masterHash, author.url(), "master", true);
+
+            // Make a change with a corresponding PR
+            var editHash = CheckableRepository.appendAndCommit(localRepo);
+            localRepo.push(editHash, author.url(), "edit", true);
+            var pr = credentials.createPullRequest(author, "master", "edit", "123: This is a pull request");
+            pr.setBody("/cc 1");
+
+            // The bot will not add any label automatically
+            TestBotRunner.runPeriodicItems(prBot);
+            assertEquals(Set.of("1", "rfr"), new HashSet<>(pr.labels()));
+
+            // Add another file to trigger a group match
+            Files.writeString(localRepoFolder.resolve("test.cpp"), "Hello there");
+            localRepo.add(Path.of("test.cpp"));
+            editHash = localRepo.commit("Another one", "duke", "duke@openjdk.org");
+            localRepo.push(editHash, author.url(), "edit");
+
+            // The bot will still not do any automatic labelling
+            TestBotRunner.runPeriodicItems(prBot);
+            assertEquals(Set.of("1", "rfr"), new HashSet<>(pr.labels()));
+
+            // Adding manually is still fine
+            pr.addComment("/label add group 2");
+            TestBotRunner.runPeriodicItems(prBot);
+            assertLastCommentContains(pr, "The `group` label was successfully added.");
+            assertLastCommentContains(pr, "The `2` label was successfully added.");
+            assertEquals(Set.of("1", "2", "group", "rfr"), new HashSet<>(pr.labels()));
         }
     }