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

[Pavel Rappo]

On Mon, 8 Jul 2024 21:46:31 GMT, Zhao Song <zsong 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.
>
> 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?

Naming is hard. Consistent naming in an established project is harder still.

`includeSimpleMerges` (or whatever it ends up being named) is supposed to work in pair with `ignoreStaleReviews`. In that pair, "include" is the opposite to "ignore": do not ignore reviews for simple merges.

That said, I'm open to better naming.

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

PR Review Comment: https://git.openjdk.org/skara/pull/1672#discussion_r1669426440