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

[Zhao Song]

On Mon, 8 Jul 2024 14:55:44 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.

The functionality seems good to me, just have some minor suggestions.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBotBuilder.java line 47:

> 45:     private IssueProject issueProject = null;
> 46:     private boolean ignoreStaleReviews = false;
> 47:     private boolean includeSimpleMerges = false;

I am not sure if `includeSimpleMerges` is a good name.
To me, if `includeSimpleMerges` is true, I think it means the bot **should** check the simple merges.
But actually, it means the bot should ignore simple merges?

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

> 57:             return true;
> 58:         if (!includeSimpleMerges)
> 59:             return false;

I think it's better to add `{}` after an if statement, even for single statements

vcs/src/main/java/org/openjdk/skara/vcs/git/GitCommits.java line 68:

> 66:         this.num = -1;
> 67:         from = reachableFrom;
> 68:         notFrom = unreachableFrom;

Add `this` before `from` and `notFrom` to align with surrounding code?

vcs/src/main/java/org/openjdk/skara/vcs/git/GitRepository.java line 1012:

> 1010:         }
> 1011:     }
> 1012: 

Maybe it's better to add an interface in `ReadOnlyRepository`

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

PR Review: https://git.openjdk.org/skara/pull/1672#pullrequestreview-2164487843
PR Review Comment: https://git.openjdk.org/skara/pull/1672#discussion_r1669369874
PR Review Comment: https://git.openjdk.org/skara/pull/1672#discussion_r1669387128
PR Review Comment: https://git.openjdk.org/skara/pull/1672#discussion_r1669405474
PR Review Comment: https://git.openjdk.org/skara/pull/1672#discussion_r1669403150