RFR: 1288: Adjust the implementation of the 'reviewers' command to match the documentation

[Erik Joelsson]

On Wed, 15 Dec 2021 12:36:24 GMT, Guoxiong Li <gli at openjdk.org> wrote:

> Hi all,
> 
> The `reviewers` command [documentation](https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/reviewers) states:
> 
>> Description
>> Require that at least N users with given role (defaults to Author) review the pull request before it can be integrated. The requirements are in addition to the ones specified by the .jcheck/conf file.
> 
> But the implementation doesn't match the documentation now, especially the bug reported at [JDK-PULL-6752](https://github.com/openjdk/jdk/pull/6752) ([SKARA-1288](https://bugs.openjdk.java.net/browse/SKARA-1288) also recorded this bug).
> 
> This patch fixes the bug and revises the code to match the documentation.
> 
> The logic has small change(Please see the following code). In the past, the reviewer number of the role will be the sum of the `updatedLimits.get(r)` and `remainingAdditional`. But I don't know it meets our expectation. Because the higher roles can reduce the `remainingAdditional`, I think the remaining number should be only set to `updatedLimits` but not added to `updatedLimits`. This matches the documentation `Require that at least N users with given role (defaults to Author) review the pull request`, especially the key work `at least`.
> 
> 
> -                updatedLimits.replace(r, updatedLimits.get(r) + remainingAdditional);
> +                if (remainingAdditional > updatedLimits.get(r)) {
> +                    // Set the number for the lower roles to remove.
> +                    remainingRemovals = remainingAdditional - updatedLimits.get(r);
> +                   // The new number of the reviewers should larger or equal than the requirement of the '.jcheck/conf' file.
> +                    // Because the '.jcheck/conf' file means the minimal reviewer requirement.
> +                   updatedLimits.replace(r, remainingAdditional);
> +              }
> 
> 
> The other code changes are listed below:
> - add negative number check.
> - remove the updated limits check. Because it is not needed after adding the negative number check.
> - adjust the reply message, always output the concrete reviewers. Because the merged logic of jcheck config and user `reviewers` command is a little hard, we should let the user know the result directly instead of guessing the result.
> - `decrease lower roles` in the method `ReviewersTracker#updatedRoleLimits` doesn't work as expected. I fix it.
> - add more test cases.
> 
> Thanks for taking the time to review.
> 
> Best Regards,
> -- Guoxiong

Just a few nits on language in replies and comments. Copyright years need to be updated. Otherwise this looks really good!

bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersCommand.java line 136:

> 134:         var totalRequired = updatedLimits.values().stream().mapToInt(Integer::intValue).sum();
> 135:         reply.print("The total number of required reviews for this PR, " +
> 136:                 "which is combined by the configuration and the command, is now set to " + totalRequired);

I would suggest:

"The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to"

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

> 64:                     // Set the number for the lower roles to remove.
> 65:                     remainingRemovals = remainingAdditional - updatedLimits.get(r);
> 66:                     // The new number of the reviewers should larger or equal than the requirement of the '.jcheck/conf' file.

I would suggest moving this comment to before line 63 and express it something like this:

// The new value cannot the lower than the value in .jcheck/conf

bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersTracker.java line 86:

> 84:                 var removed = Math.max(0, originalVal - remainingRemovals);
> 85:                 updatedLimits.replace(r, removed);
> 86:                 remainingRemovals -= (originalVal - updatedLimits.get(r));

Maybe use the local variable `removed` instead of updatedLimits.get(r). I think that reads better.

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

PR: https://git.openjdk.java.net/skara/pull/1262