RFR: 1495: /backport on PRs instead of commits should inform of proper usage [v2]

[Erik Joelsson]

On Sat, 9 Jul 2022 16:29:24 GMT, Guoxiong Li <gli at openjdk.org> wrote:

>> Hi all,
>> 
>> This patch mainly solves the issue described at SKARA-1495 which makes the `backport` command be valid in a pull request which has been integrated.
>> 
>> But in detail, this patch has the following feature or solves the following issues:
>> 
>> - Currently, the commands of the pull request and the commands of the commit are not related and can't be known from each other. So when we use the commit commands, such as `tag`, in the pull request, the bot replies `Unknown command tag`. Actually, the bot should replies the message like `The command tag can not be used in pull requests.  Please try to use this command on the commit`. The change is mainly at method `CommandExtractor::extractCommands`.
>> - Many places of the code use a duplicated method, named `resultingCommitHash`, this method could be put into class `PullRequest` to avoid the duplication. I refactor it in this patch.
>> - Now the `backport` command can not be used in pull requests. I revise the logic to let it available in a pull request which has been integrated, mainly removing the overrided method `allowedInPullRequest` of the class `BackportCommand`.
>> - The reason that these issues are not found before is that we hadn't tested all the branches of the code, especially the branches in methods `PullRequestCommandWorkItem::processCommand` and `CommitCommandWorkItem::processCommand`. So I add more test cases to test almost all the branches. And if the branch can't be tested, I add a comment to explain it.
>> 
>> Although it seems that many features are not related to SKARA-1495, I put them together as an enhancement of the PullRequestBot.
>> 
>> Best Regards,
>> -- Guoxiong
>
> Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix NPE.

bots/notify/src/main/java/org/openjdk/skara/bots/notify/PullRequestWorkItem.java line 187:

> 185: 
> 186:         var issues = parseIssues();
> 187:         var commit = integratorId != null ? pr.findIntegratedCommitHash(List.of(integratorId)).orElse(null) : null;

Consider keeping the resultingCommitHash() method with the current signature and have it wrap the new pr.findIntegratedCommitHash(), doing the null checks and conversions there instead. I think that will make the code more readable.

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

> 56:     public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath,
> 57:                 CommandInvocation command, List<Comment> allComments, PrintWriter reply) {
> 58:         reply.println("The command `backport` can not be used in a pull request that has not yet been integrated.");

Shouldn't the message here be the one suggested in the Bug?

"(!) You cannot issue `/backport` on a pull request. Instead, give that command on the actual commit you want to backport."

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandExtractor.java line 55:

> 53:                 }
> 54:                 var command = commandMatcher.group(1).toLowerCase();
> 55:                 var handler = commitCommandFirst ? CommitCommandWorkItem.commandHandlers.get(command) : PullRequestCommandWorkItem.commandHandlers.get(command);

What are you trying to achieve with the commitCommandFirst boolean? In what situation would it be wrong to just use one unified list?

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestCommandWorkItem.java line 43:

> 41:     private static final Pattern commandReplyPattern = Pattern.compile("<!-- Jmerge command reply message \\((\\S+)\\) -->");
> 42: 
> 43:     static final Map<String, CommandHandler> commandHandlers = Map.ofEntries(

Instead of making these package private and adding the convoluted logic in extractCommands, can't we just add BackportCommand to the commandHandlers for PullRequestWorkItem?

It seems like the original author mixed two ways of separating commands between PRs and commits, both through these handler lists, and through the allowedIn* methods. If we really want to improve all the error messages, then all commands should be added to both lists, or maybe the lists should be unified into one.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestCommandWorkItem.java line 174:

> 172:             if (isCommit) {
> 173:                 if (handler.get().allowedInCommit()) {
> 174:                     var hash = pr.findIntegratedCommitHash();

It's worth noting that this will trigger a new `comments()` call to the forge instead of reusing the existing list of comments.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestCommandWorkItem.java line 205:

> 203:                     printer.print("The command `");
> 204:                     printer.print(command.name());
> 205:                     printer.println("` can not be used in pull requests. Please try to use this command on the commit.");

I don't think we need to suggest using the command on a commit in the general case.

Suggestion:

                    printer.println("` cannot be used in pull requests.");

forge/src/main/java/org/openjdk/skara/forge/PullRequest.java line 189:

> 187: 
> 188:     default Optional<Hash> findIntegratedCommitHash(List<String> userIds) {
> 189:         Pattern pushedPattern = Pattern.compile("Pushed as commit ([a-f0-9]{40})\\.");

If you are moving this pattern from the bots modules to this class, then I think you should also move the comment logic from IntegrateCommand here so that both the creation and parsing of this kind of comment is handled at the same level. It seems bad to have PullRequest know about what special comments the IntegrateCommand happens to add.

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

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