1076: Not possible to use 'Backport <sha>' if original bug doesn't ha…

…ve public sha

Reviewed-by: kcr

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

@@ -40,13 +40,13 @@
 import java.util.logging.Logger;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
-import java.util.stream.Stream;
 
 class CheckWorkItem extends PullRequestWorkItem {
     private final Pattern metadataComments = Pattern.compile("<!-- (?:(add|remove) (?:contributor|reviewer))|(?:summary: ')|(?:solves: ')|(?:additional required reviewers)");
     private final Logger log = Logger.getLogger("org.openjdk.skara.bots.pr");
     static final Pattern ISSUE_ID_PATTERN = Pattern.compile("^(?:(?<prefix>[A-Za-z][A-Za-z0-9]+)-)?(?<id>[0-9]+)(?::\\s+(?<title>.+))?$");
-    private static final Pattern BACKPORT_TITLE_PATTERN = Pattern.compile("^Backport\\s*([0-9a-z]{40})\\s*$");
+    private static final Pattern BACKPORT_HASH_TITLE_PATTERN = Pattern.compile("^Backport\\s*([0-9a-z]{40})\\s*$");
+    private static final Pattern BACKPORT_ISSUE_TITLE_PATTERN = Pattern.compile("^Backport\\s*(?:(?<prefix>[A-Za-z][A-Za-z0-9]+)-)?(?<id>[0-9]+)\\s*$");
     private static final String ELLIPSIS = "…";
 
     CheckWorkItem(PullRequestBot bot, PullRequest pr, Consumer<RuntimeException> errorHandler) {
@@ -227,9 +227,9 @@ public Collection<WorkItem> run(Path scratchPath) {
                 return List.of();
             }
 
-            var m = BACKPORT_TITLE_PATTERN.matcher(pr.title());
-            if (m.matches()) {
-                var hash = new Hash(m.group(1));
+            var backportHashMatcher = BACKPORT_HASH_TITLE_PATTERN.matcher(pr.title());
+            if (backportHashMatcher.matches()) {
+                var hash = new Hash(backportHashMatcher.group(1));
                 try {
                     var localRepo = materializeLocalRepo(scratchPath, hostedRepositoryPool);
                     if (localRepo.isAncestor(hash, pr.headHash())) {
@@ -299,6 +299,37 @@ public Collection<WorkItem> run(Path scratchPath) {
                 }
             }
 
+            // Check for a title of the form Backport <issueid>
+            var backportIssueMatcher = BACKPORT_ISSUE_TITLE_PATTERN.matcher(pr.title());
+            if (backportIssueMatcher.matches()) {
+                var prefix = getMatchGroup(backportIssueMatcher, "prefix");
+                var id = getMatchGroup(backportIssueMatcher, "id");
+                var project = bot.issueProject();
+
+                if (!prefix.isEmpty() && !prefix.equalsIgnoreCase(project.name())) {
+                    var text = "<!-- backport error -->\n" +
+                            ":warning: @" + pr.author().username() + " the issue prefix `" + prefix + "` does not" +
+                            " match project [" + project.name() + "](" + project.webUrl() + ").";
+                    addBackportErrorComment(text, comments);
+                    return List.of();
+                }
+                var issue = project.issue(id);
+                if (issue.isEmpty()) {
+                    var text = "<!-- backport error -->\n" +
+                            ":warning: @" + pr.author().username() + " the issue with id `" + id + "` " +
+                            "does not exist in project [" + project.name() + "](" + project.webUrl() + ").";
+                    addBackportErrorComment(text, comments);
+                    return List.of();
+                }
+                pr.setTitle(id + ": " + issue.get().title());
+                var text = "This backport pull request has now been updated with the original issue," +
+                        " but not the original commit. If you have the original commit hash, please update" +
+                        " the pull request title with `Backport <hash>`.";
+                pr.addComment(text);
+                pr.addLabel("backport");
+                return List.of(new CheckWorkItem(bot, pr.repository().pullRequest(pr.id()), errorHandler));
+            }
+
             // If the title needs updating, we run the check again
             if (updateTitle()) {
                 return List.of(new CheckWorkItem(bot, pr.repository().pullRequest(pr.id()), errorHandler));

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

@@ -266,6 +266,10 @@ Optional<Hash> mergeTarget(PrintWriter reply) {
     }
 
     Hash findOriginalBackportHash() {
+        return findOriginalBackportHash(pr);
+    }
+
+    static Hash findOriginalBackportHash(PullRequest pr) {
         var botUser = pr.repository().forge().currentUser();
         var backportLines = pr.comments()
                 .stream()

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

@@ -61,8 +61,10 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
             return;
         }
 
-        if (!pr.labelNames().contains("backport")) {
-            reply.println("@" + username + " can only mark [backport pull requests](https://wiki.openjdk.java.net/display/SKARA/Backports#Backports-BackportPullRequests) as clean");
+        if (!pr.labelNames().contains("backport") || CheckablePullRequest.findOriginalBackportHash(pr) == null) {
+            reply.println("@" + username + " can only mark [backport pull requests]" +
+                    "(https://wiki.openjdk.java.net/display/SKARA/Backports#Backports-BackportPullRequests)," +
+                    " with an original hash, as clean");
             return;
         }
 

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

@@ -1347,4 +1347,116 @@ void badIssueInOriginal(TestInfo testInfo) throws IOException {
             assertFalse(pr.labelNames().contains("backport"));
         }
     }
+
+    @Test
+    void noHashOnlyIssue(TestInfo testInfo) throws IOException {
+        try (var credentials = new HostCredentials(testInfo);
+             var tempFolder = new TemporaryDirectory()) {
+
+            var author = credentials.getHostedRepository();
+            var integrator = credentials.getHostedRepository();
+            var reviewer = credentials.getHostedRepository();
+            var issues = credentials.getIssueProject();
+            var censusBuilder = credentials.getCensusBuilder()
+                    .addCommitter(author.forge().currentUser().id())
+                    .addReviewer(integrator.forge().currentUser().id())
+                    .addReviewer(reviewer.forge().currentUser().id());
+            var bot = PullRequestBot.newBuilder()
+                    .repo(integrator)
+                    .censusRepo(censusBuilder.build())
+                    .issueProject(issues)
+                    .build();
+
+            // Populate the projects repository
+            var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType());
+            var masterHash = localRepo.resolve("master").orElseThrow();
+            localRepo.push(masterHash, author.url(), "master", true);
+
+            var issue1 = credentials.createIssue(issues, "An issue");
+            var issue1Number = issue1.id().split("-")[1];
+
+            // Create change
+            localRepo.checkout(localRepo.defaultBranch());
+            var editBranch = localRepo.branch(masterHash, "edit");
+            localRepo.checkout(editBranch);
+            var newFile2 = localRepo.root().resolve("a_new_file.txt");
+            Files.writeString(newFile2, "hello");
+            localRepo.add(newFile2);
+            var editHash = localRepo.commit("Backport", "duke", "duke@openjdk.java.net");
+            localRepo.push(editHash, author.url(), "refs/heads/edit", true);
+
+            // Create various kinds of bad pull request titles
+            // Use a bad project
+            var pr = credentials.createPullRequest(author, "master", "edit",
+                    "Backport " + "FOO-" + issue1.id().split("-")[1]);
+            TestBotRunner.runPeriodicItems(bot);
+            var backportComment = pr.comments().get(0).body();
+            assertTrue(backportComment.contains("does not match project"));
+            assertFalse(pr.labelNames().contains("backport"));
+
+            // Use bad issue ID
+            pr.setTitle("Backport TEST-4711");
+            TestBotRunner.runPeriodicItems(bot);
+            backportComment = pr.comments().get(1).body();
+            assertTrue(backportComment.contains("does not exist in project"));
+            assertFalse(pr.labelNames().contains("backport"));
+
+            // Use different kinds of good titles
+            // Use the full issue ID
+            pr.setTitle("Backport " + issue1.id());
+            TestBotRunner.runPeriodicItems(bot);
+            backportComment = pr.comments().get(2).body();
+            assertTrue(backportComment.contains("This backport pull request has now been updated with the original issue"));
+            assertEquals(issue1Number + ": An issue", pr.title());
+            assertTrue(pr.labelNames().contains("backport"));
+
+            // Set the title without project name
+            pr.setTitle("Backport " + issue1.id().split("-")[1]);
+            TestBotRunner.runPeriodicItems(bot);
+            backportComment = pr.comments().get(3).body();
+            assertTrue(backportComment.contains("This backport pull request has now been updated with the original issue"));
+            assertEquals(issue1Number + ": An issue", pr.title());
+            assertTrue(pr.labelNames().contains("backport"));
+
+            // Approve PR and re-run bot
+            var prAsReviewer = reviewer.pullRequest(pr.id());
+            prAsReviewer.addReview(Review.Verdict.APPROVED, "Looks good");
+            TestBotRunner.runPeriodicItems(bot);
+            assertLastCommentContains(pr, "This change now passes all *automated* pre-integration checks");
+
+            // Integrate
+            var prAsCommitter = author.pullRequest(pr.id());
+            prAsCommitter.addComment("/integrate");
+            TestBotRunner.runPeriodicItems(bot);
+
+            // Find the commit
+            assertLastCommentContains(pr, "Pushed as commit");
+
+            String hex = null;
+            var comment = pr.comments().get(pr.comments().size() - 1);
+            var lines = comment.body().split("\n");
+            var pattern = Pattern.compile(".* Pushed as commit ([0-9a-z]{40}).*");
+            for (var line : lines) {
+                var m = pattern.matcher(line);
+                if (m.matches()) {
+                    hex = m.group(1);
+                    break;
+                }
+            }
+            assertNotNull(hex);
+            assertEquals(40, hex.length());
+            localRepo.checkout(localRepo.defaultBranch());
+            localRepo.pull(author.url().toString(), "master", false);
+            var commit = localRepo.lookup(new Hash(hex)).orElseThrow();
+
+            var message = CommitMessageParsers.v1.parse(commit);
+            assertEquals(1, message.issues().size());
+            assertEquals("An issue", message.issues().get(0).description());
+            assertEquals(List.of("integrationreviewer3"), message.reviewers());
+            assertEquals(Optional.empty(), message.original());
+            assertEquals(List.of(), message.contributors());
+            assertEquals(List.of(), message.summaries());
+            assertEquals(List.of(), message.additional());
+        }
+    }
 }

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

@@ -73,7 +73,7 @@ void cleanCommandOnRegularPullRequestShouldNotWork(TestInfo testInfo) throws IOE
             assertFalse(pr.labelNames().contains("backport"));
             assertFalse(pr.labelNames().contains("clean"));
             assertLastCommentContains(pr, "can only mark [backport pull requests]");
-            assertLastCommentContains(pr, "as clean");
+            assertLastCommentContains(pr, ", with an original hash, as clean");
         }
     }
 
@@ -297,4 +297,49 @@ void authorShouldNotBeAllowed(TestInfo testInfo) throws IOException {
             assertLastCommentContains(pr, "can use the `/clean` command");
         }
     }
+
+    @Test
+    void missingBackportHash(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()
+                    .addCommitter(author.forge().currentUser().id())
+                    .addReviewer(integrator.forge().currentUser().id());
+            var issues = credentials.getIssueProject();
+            var prBot = PullRequestBot.newBuilder()
+                    .repo(integrator)
+                    .censusRepo(censusBuilder.build())
+                    .issueProject(issues)
+                    .build();
+
+            // Populate the projects repository
+            var localRepoFolder = tempFolder.path().resolve("localrepo");
+            var localRepo = CheckableRepository.init(localRepoFolder, author.repositoryType());
+            var masterHash = localRepo.resolve("master").orElseThrow();
+            assertFalse(CheckableRepository.hasBeenEdited(localRepo));
+            localRepo.push(masterHash, author.url(), "master", true);
+
+            var issue = credentials.createIssue(issues, "An issue");
+
+            // 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", "Backport " + issue.id());
+            TestBotRunner.runPeriodicItems(prBot);
+
+            assertTrue(pr.labelNames().contains("backport"));
+            assertFalse(pr.labelNames().contains("clean"));
+
+            // Try to issue the "/clean" PR command, should not work
+            pr.addComment("/clean");
+            TestBotRunner.runPeriodicItems(prBot);
+            assertTrue(pr.labelNames().contains("backport"));
+            assertFalse(pr.labelNames().contains("clean"));
+            assertLastCommentContains(pr, "can only mark [backport pull requests]");
+            assertLastCommentContains(pr, ", with an original hash, as clean");
+        }
+    }
 }