RFR: 1797: Add support for /backport pull request command

[Erik Joelsson]

On Wed, 1 Feb 2023 20:38:53 GMT, Zhao Song <zsong at openjdk.org> wrote:

> This patch introduces support for the /backport command in pull requests. (ErikJ and ErikH proposed the idea and came up with this implementation.)
> 
> Usage of /backport in pull request:
> 
> Syntax: `/backport <repo> [<branch>] ` ` /backport disable <repo> [<branch>]` (default branch is master)
> 
> 1. If the `/backport` command is used in an **open** pull request, it adds a label `Backport=repo:branch` to the PR. If the PR is later integrated, the PR bot scans for backport labels and begins the backporting process. To cancel the backport, the user can use `/backport disable repo master` to remove the label before the PR is integrated.
> 2.  If the `/backport` command is used in an **integrated** PR, it creates the backport branch and comments on the PR. This process is similar to using the /backport command in commits. (This feature was implemented in [SKARA-1495](https://bugs.openjdk.org/browse/SKARA-1495) by guoxiong li, but the usage was not documented on the[ wiki page](https://wiki.openjdk.org/display/SKARA/Pull+Request+Commands), so seems like few people use this command)
> 3.  If the `/backport` command is used in a **closed** (not integrated) PR, the user will receive an error message stating that the command cannot be used in a closed but not integrated PR.

I had forgotten about the support for /backport command on closed PRs, good that you found that. I'm also happily surprised at how easy it was to add processing of the label in the `IntegrateCommand`. It looks like that will work robustly as commands are always processed in order. I added some comments and suggestions.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/BackportCommand.java line 47:

> 45: 
> 46:     private void showHelpInPR(PrintWriter reply) {
> 47:         reply.println("Usage: `/backport <repository> [<branch>]` `/backport disable <repository> [<branch>]`");

Suggestion:

        reply.println("Usage: `/backport [disable] <repository> [<branch>]`");

bots/pr/src/main/java/org/openjdk/skara/bots/pr/BackportCommand.java line 66:

> 64: 
> 65:     @Override
> 66:     public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List<Comment> allComments, PrintWriter reply, List<String> labelsToAdd, List<String> labelsToRemove) {

Please break up this line.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/BackportCommand.java line 73:

> 71: 
> 72:         if (pr.isClosed() && !pr.labelNames().contains("integrated")) {
> 73:             reply.println("`/backport` command can not be used in closed but not integrated PR");

Suggestion:

            reply.println("`/backport` command can not be used in a closed but not integrated pull request");

bots/pr/src/main/java/org/openjdk/skara/bots/pr/BackportCommand.java line 99:

> 97:                 reply.println("Backport for repo `" + targetRepoName + "` on branch `" + targetBranchName + "` was successfully disabled.");
> 98:             } else {
> 99:                 reply.println("Backport for repo `" + targetRepoName + "` on branch `" + targetBranchName + "` was not enabled, so you can not disable it.");

Suggestion:

                reply.println("Backport for repo `" + targetRepoName + "` on branch `" + targetBranchName + "` was already disabled.");

bots/pr/src/main/java/org/openjdk/skara/bots/pr/BackportCommand.java line 133:

> 131:                 reply.println("The target branch `" + targetBranchName + "` does not exist");
> 132:                 return;
> 133:             }

This looks like it has been copied from the other method. Would be better to break it out into methods and reused.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/BackportCommand.java line 140:

> 138:                 labelsToAdd.add(backportLabel);
> 139:                 reply.println("Backport for repo `" + targetRepoName + "` on branch `" + targetBranchName + "` was successfully enabled.");
> 140:                 reply.println("Please note that instructions for creating backports will be provided once this PR is integrated.");

Suggestion:

                reply.println("Backport for repo `" + targetRepoName + "` on branch `" + targetBranchName + "` was successfully enabled and will be performed once this pull request has been integrated.");
                reply.println(" Further instructions will be provided at that time.");

bots/pr/src/main/java/org/openjdk/skara/bots/pr/BackportCommand.java line 208:

> 206:             var botComment = allComments.stream()
> 207:                     .filter(comment -> comment.author().equals(bot.repo().forge().currentUser()))
> 208:                     .filter(comment -> comment.body().contains("Backport for repo `" + repoName + "` on branch `" + targetBranchName + "` was successfully enabled."))

Instead of parsing the human readable message, I would recommend encoding a more structured data entry in an html comment. That will be easier to parse and less sensitive to future changes of this code. There should be several similar examples of that throughout the bots.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java line 246:

> 244:                 addPrePushComment(pr, amendedHash, rebaseMessage.toString());
> 245:                 localRepo.push(amendedHash, pr.repository().url(), pr.targetRef());
> 246:                 processBackportLabel(pr, allComments);

I think this call should be moved to the top of `markIntegratedAndClosed`. Otherwise, if the bot is interrupted just after line 245, then the next attempt at running `IntegrateCommand` will jump straight into `markIntegratedAndClosed` and skip the backport processing.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java line 340:

> 338:                 var text = "Creating backport for repo " + repoName + " on branch " + branchName
> 339:                         + "\n<!--\n /backport " + repoName + " " + branchName + "\n-->" + "\n"
> 340:                         + PullRequestCommandWorkItem.VALID_BOT_COMMAND_MARKER;

In `CheckWorkItem`, when adding `/integrate` based on an earlier `/integrate auto`, we don't hide the command. I think we should be consistent and not hide it here either.

-------------

PR: https://git.openjdk.org/skara/pull/1466