RFR: 2331: Improve review notes

[Kevin Rushforth]

On Fri, 19 Jul 2024 10:24:34 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> Rest assured, I will not integrate this PR until we figure out what to do with the issue you raised, Chen. That issue should be fixed either in this PR or a follow-up one.
>> 
>> Personally, I think if a review is stale but otherwise good, we don't need to add any emoji. It suffices to add the commit hash the review applies to, and maybe indicate how far away that commit is from HEAD, to quantify staleness: for example, "3 commits behind". Any extra commits added since that review may only be simple merges anyway.
>> 
>> However, if a review is stale and restricted (i.e. does not count in required reviews), we may add "⚠️". In a sense, it would provide a similar indication to that of a repo configured with:
>> 
>>     useStaleReviews=true
>>     acceptSimpleMerges=false
>> 
>> In this picture, the "🔄" emoji goes away completely.
>
> To add to my previous message.
> 
> We should keep in mind that any indication in question is primarily for a PR _author_. From my own experience as a _reviewer_, I don't normally follow a particular PR that I have reviewed to see if my review has become stale and needs an update. If the author feels I should re-review their PR, they ping me directly.

I was thinking along similar lines. Basically there are two main cases, `useStaleReviews=(true|false)`. Here is logic that I think does roughly what we want:


if (useStaleReviews) {
    display the "⚠️" emoji and "Review applies to HASH" message always for stale reviews
} else {
    display the "🔄" emoji and "Re-review required" message for all stale reviews
    if (acceptSimpleMerges) {
        display the "review applies to HASH" message without any emoji for non-HEAD simple merge reviews [1]
    }
}

[1] By this I mean a review of a commits that isn't the HEAD, but that differs
from the head only by a simple merge


This ensures that the same PR doesn't show both the "⚠️" and "🔄" emojis.

Another possible incremental improvement we could consider is to only display the re-review needed emoji + message if the review criteria isn't satisfied. This would mean that as soon the minimum number of Reviewers re-review, the rest of the stale reviews lose the emoji (but retain a message indicating that they are out of date).


if (useStaleReviews) {
    display the "⚠️" emoji and "Review applies to HASH" message always for stale reviews
} else {
    if (review criteria not satisfied) {
        display the "🔄" emoji and "Re-review required" message for all stale reviews
        if (acceptSimpleMerges) {
            display the "review applies to HASH" message without an emoji for non-HEAD simple merge reviews [1]
        }
    } else {
        display the "review applies to HASH" message without an emoji for stale reviews and non-HEAD simple merge reviews [1]
    }
}

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

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