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

[Kevin Rushforth]

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

Unless there is a corner case that I haven't hit, the implementation is working as expected now. I will need to look at your proposed change more closely, but I don't see the need to change the existing logic.

What problem are you trying to solve?

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

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