RFR: 1997: Allow the creator of a PR to set the commit author

[Erik Joelsson]

On Thu, 31 Aug 2023 22:42:46 GMT, Zhao Song <zsong at openjdk.org> wrote:

> In this pr, we will introduce a new pull request command `/author`.
> 
> If an author wants to contribute a change to a repository, but he doesn't have access, then someone with the access will be able to help him with this command.
> 
> Usage: `/author [set|remove] [@user | openjdk-user | Full Name <email at address>]`
> Examples: 
> `/author set @openjdk-bot`
> `/author @openjdk-bot`
> `/author set J. Duke <duke at openjdk.org>`
> `/author remove @openjdk-bot`
> `/author remove`
> 
> Note: only Committers of the repository would be allowed to issue /author command.

Looks pretty good. I have some suggestions for language to make it clearer to users and a minor optimization.

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

> 53:     public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, ScratchArea scratchArea, CommandInvocation command, List<Comment> allComments, PrintWriter reply) {
> 54:         if (!command.user().equals(pr.author())) {
> 55:             reply.println("Only the author (@" + pr.author().username() + ") is allowed to issue the `author` command.");

Suggestion:

            reply.println("Only the pull request author (@" + pr.author().username() + ") is allowed to issue the `author` command.");

bots/pr/src/main/java/org/openjdk/skara/bots/pr/AuthorCommand.java line 60:

> 58: 
> 59:         if (!censusInstance.isCommitter(pr.author())) {
> 60:             reply.println("Only committers in this [project](https://openjdk.org/census#" + censusInstance.project().name() + ") are allowed to issue the `author` command.");

I think we should link to the definition of Committer instead (like we do for Reviewers in the BackportCommand). A census link would have to be conditional on which census this repository is using.
Suggestion:

            reply.println("Only [Committers](https://openjdk.org/bylaws#committer) are allowed to issue the `author` command.");

bots/pr/src/main/java/org/openjdk/skara/bots/pr/AuthorCommand.java line 91:

> 89:                 }
> 90:                 reply.println(Authors.setAuthorMarker(author.get()));
> 91:                 reply.println("Author of this pull request has been set to `" + author.get() + "` successfully.");

I think we need to change the language a bit. Claiming that the author of the pull request has changed can be confusing, because the author isn't actually changing in the forge. I think "pull request author" and "overriding author" are better terms. 
Suggestion:

                reply.println("Setting overriding author to `" + author.get() + "`. When this pull request is integrated, the overriding author will be used in the commit.");

bots/pr/src/main/java/org/openjdk/skara/bots/pr/AuthorCommand.java line 112:

> 110:                     if (currAuthor.get().equals(author.get())) {
> 111:                         reply.println(Authors.removeAuthorMarker(author.get()));
> 112:                         reply.println("Author `" + author.get() + "` successfully removed.");

Suggestion:

                        reply.println("Overriding author `" + author.get() + "` was successfully removed. When this pull request is integrated, the pull request author will be the author of the commit.");

bots/pr/src/main/java/org/openjdk/skara/bots/pr/AuthorCommand.java line 115:

> 113:                     } else {
> 114:                         reply.println("`" + author.get() + "` was not set to this pull request's author.");
> 115:                         reply.println("Current author of this pull request is set to: `" + currAuthor.get() + "`");

Suggestion:

                        reply.println("Cannot remove `" + author.get() + "`, the overriding author is currently set to: `" + currAuthor.get() + "`");

bots/pr/src/main/java/org/openjdk/skara/bots/pr/AuthorCommand.java line 125:

> 123:     @Override
> 124:     public String description() {
> 125:         return "sets or removes author for a PR";

Suggestion:

        return "sets an overriding author used in the final commit when the PR is integrated";

bots/pr/src/main/java/org/openjdk/skara/bots/pr/Authors.java line 32:

> 30: import java.util.regex.*;
> 31: 
> 32: class Authors {

I think this class should be just `Author` since we can never set more than one , or perhaps `OverridingAuthor` to make it even clearer what it is.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckablePullRequest.java line 181:

> 179:         }
> 180: 
> 181:         var authorSet = Authors.author(pr.repository().forge().currentUser(), pr.comments());

Calling `pr.comments()` here is a remote call so rather expensive. It's already done in `commitMessage`. We could at least make sure we only call it once. Looking a bit further, I see that in all places where a `CheckablePullRequest` is created, we already have a list of comments, so I think an even better solution here is to add a `comments` field and supply it to the constructor. 

I think the variable name can be clearer.
Suggestion:

        var overridingAuthor = Authors.author(pr.repository().forge().currentUser(), pr.comments());

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

PR Review: https://git.openjdk.org/skara/pull/1553#pullrequestreview-1606986111
PR Review Comment: https://git.openjdk.org/skara/pull/1553#discussion_r1313064434
PR Review Comment: https://git.openjdk.org/skara/pull/1553#discussion_r1313058552
PR Review Comment: https://git.openjdk.org/skara/pull/1553#discussion_r1313063169
PR Review Comment: https://git.openjdk.org/skara/pull/1553#discussion_r1313066662
PR Review Comment: https://git.openjdk.org/skara/pull/1553#discussion_r1313072027
PR Review Comment: https://git.openjdk.org/skara/pull/1553#discussion_r1313077939
PR Review Comment: https://git.openjdk.org/skara/pull/1553#discussion_r1313049494
PR Review Comment: https://git.openjdk.org/skara/pull/1553#discussion_r1313078472