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

Guoxiong Li gli at openjdk.java.net
Wed Dec 15 12:39:58 UTC 2021


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

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

Commit messages:
 - 1288: Adjust the implementation of the 'reviewers' command to match the documentation

Changes: https://git.openjdk.java.net/skara/pull/1262/files
 Webrev: https://webrevs.openjdk.java.net/?repo=skara&pr=1262&range=00
  Issue: https://bugs.openjdk.java.net/browse/SKARA-1288
  Stats: 196 lines in 3 files changed: 169 ins; 7 del; 20 mod
  Patch: https://git.openjdk.java.net/skara/pull/1262.diff
  Fetch: git fetch https://git.openjdk.java.net/skara pull/1262/head:pull/1262

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


More information about the skara-dev mailing list