RFR: 2331: Improve review notes [v3]

[Pavel Rappo]

On Wed, 24 Jul 2024 13:54:03 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Don't special-case wrong target or missing commit
>>   
>>   After out-of-band conversation with @zhaosongzs, we decided to drop
>>   useStaleReviews check.
>
> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 618:
> 
>> 616:                             if (!hash.get().equals(pr.headHash())) {
>> 617:                                 if (reviewCoverage.covers(review) || !tooFewReviewers) {
>> 618:                                     entry += (tooFewReviewers ? " ⚠️ " : " ")
> 
> Why are you using `tooFewReviewers` to decide whether or not to show the warning emoji. Shouldn't this check be `useStaleReviews`?

`useStaleReviews` is baked into `reviewCoverage.covers(review)`:

    public boolean covers(Review review) {
        return cachedCoverage.computeIfAbsent(review, this::covers0);
    }

    private boolean covers0(Review review) {
        var r = review.hash();
        // Reviews without a hash are never valid as they referred to no longer
        // existing commits.
        if (r.isEmpty() || review.verdict() != Review.Verdict.APPROVED
                || !review.targetRef().equals(pr.targetRef())) {
            return false;
        }
        if (useStaleReviews || r.get().equals(pr.headHash())) {
            return true;
        }
        if (!acceptSimpleMerges) {
            return false;
        }

Here and elsewhere, `tooFewReviewers` is checked because it's decisive: if there are enough reviewers, no emoji or the word "required" should be used.

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

PR Review Comment: https://git.openjdk.org/skara/pull/1679#discussion_r1689986242