RFR: 2312: Do not require re-review for a simple merge [v2]

[Erik Joelsson]

On Mon, 8 Jul 2024 23:28:30 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> If a repository is configured to ignore stale reviews, every commit to a PR made against that repo means that the PR needs to be re-reviewed. Re-reviewing is costly, and not all commits are worth that cost.
>> 
>> One example of such a commit is a simple merge. During a PR's lifetime, the PR's target branch (typically, repo's `master`) might be merged into the PR multiple times. Usually, such merges are trivial and would effectively be performed by Skara automatically if the PR didn't have them in the first place [^*]. For such commits, it makes sense to not require review.
>> 
>> This is my first contribution to Skara domain logic, which I've just started learning about. Also, while I tried to code this change according to Skara customs and idioms, I might have missed something. I guess what I'm saying is **take extra care** reviewing this PR. Thanks.
>> 
>> 
>> [^*]: This is the main assumption of this PR. Any simple merges present in a PR change the end result of that PR insignificantly, if at all.
>
> Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Respond to feedback

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBotFactory.java line 167:

> 165:                 botBuilder.ignoreStaleReviews(repo.value().get("ignorestale").asBoolean());
> 166:             }
> 167:             if (repo.value().contains("includemerge")) {

In newer Skara code we have already moved towards using camel case in the config options and having them match the boolean in the code, so I think we should do that here too, for both this new option and the old `ignorestale` if we agree to fix both.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewCoverage.java line 49:

> 47:     }
> 48: 
> 49:     public boolean covers(Review review, PullRequest pr) {

>From what I can tell, this method is called twice for each Review in a CheckRun, once when generating the commit and once when generating the body. It makes several invocations of `git` in each call, some of which are clearly repeating between calls. We need to look at how we can cache the results to avoid bloating CheckRuns too much. Since the class is instantiated once for each CheckRun (and command), it should be fine to implement caching in the class itself for most of these calls, or rather for the result of a particular review hash. The pr head and target hash won't change during one WorkItem, so the only input value that differs between calls to one instance is the review hash. With that in mind, the pr should be a field rather than a method parameter.

I would really like to solve such performance issues up front for new features and avoid adding more unnecessary bloat that will need cleanup later.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewCoverage.java line 69:

> 67:         boolean seenAtLeastOneCommit = false;
> 68:         try {
> 69:             var targetHash = PullRequestUtils.targetHash(gitRepo);

`PullRequestUtils::targetHash` is not caching the result, so will invoke `git` every time. There is a wrapper for this in CheckablePullRequest that does caching, which is used most of the time from the work items. Not sure how to best utilize that though. We could cache here, but that would still add one additional `git` invocation.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewCoverage.java line 74:

> 72:                     seenAtLeastOneCommit = true;
> 73:                     if (!c.isMerge() || c.numParents() != 2)
> 74:                         return false;

Nit: Please surround the if block with braces.

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

PR Review Comment: https://git.openjdk.org/skara/pull/1672#discussion_r1670150653
PR Review Comment: https://git.openjdk.org/skara/pull/1672#discussion_r1670193124
PR Review Comment: https://git.openjdk.org/skara/pull/1672#discussion_r1670184200
PR Review Comment: https://git.openjdk.org/skara/pull/1672#discussion_r1670173107