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

[Erik Joelsson]

On Wed, 10 Jul 2024 13:35:54 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
> 
>  - Rename properties as suggested
>    
>    IMPORTANT
>    ---------
>    
>    The ignoreStale JSON property is now called useStaleReviews. The
>    semantics of useStaleReviews is the opposite (negated) that of
>    ignoreStale.
>    
>    That means that all repos that are configured with ignoreStale set to
>    true should instead be configured with useStaleReviews set to false or
>    useStaleReviews absent. And the repositories whose configuration doesn't
>    contain ignoreStale or whose ignoreStale is set to false should instead
>    be configured with useStaleReviews set to true.
>    
>    In any case, if present, ignoreStale should be removed from a repo
>    configuration as that property is now unrecognised.
>  - Merge branch 'master' into 2312
>  - Respond to feedback
>  - Respond to feedback
>    
>    Pulls up new methods from GitRepository.
>  - Respond to feedback
>  - Initial commit

> Correction to the commit message of [18ecf69](https://github.com/openjdk/skara/commit/18ecf690e3a404993ac6c8b3d27aa5a0051abdb8). It should read as follows:
> 
> ## IMPORTANT
> The ignoreStale JSON property is now called useStaleReviews. The semantics of useStaleReviews is the opposite (negated) of that of ignoreStale. The default value of useStaleReviews is true.
> 
> That means that all repos that are configured with ignoreStale set to true should instead be configured with useStaleReviews set to false. And all repos that are configured with ignoreStale implicitly set to false should instead be configured to useStaleReviews set to true, or ignoreStale removed from a repo configuration.

Thanks for renaming the properties. This should be straightforward enough to update for. I like the new property names much better already when reading the code.

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

PR Comment: https://git.openjdk.org/skara/pull/1672#issuecomment-2220616011