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

[Erik Joelsson]

On Tue, 29 Mar 2022 10:39:44 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
>
> Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix comments, names and copyrights.

Marked as reviewed by erikj (Lead).

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

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