Proposal: Require re-review before integration if the PR is modified

[Kevin Rushforth]

inline comments below

On 6/19/2024 8:53 AM, Andrew Hughes wrote:
> On 16:31 Wed 19 Jun 2024, Severin Gehwolf wrote:
>> On Wed, 2024-06-19 at 15:00 +0100, Andrew Haley wrote:
>>> Maybe this policy will result in some otherwise unnecessary
>>> re-reviewing. But as long as it's only a single reviewer to do a final
>>> pass, rather than a tick from everyone who has approved, I agree with
>>> this policy.
>> I like this idea of a single reviewer only (not all of them) needing to
>> approve the final version.
>>
>> FWIW, I tend to merge latest master before integrating so as to reduce
>> chance of breakages caused by rebases at integration time. This process
>> change seems to me then needing to chase up all reviewers again to
>> approve the final version.
>>
>> Thanks,
>> Severin
>>
> Only the number of reviewers required by Skara need to re-review. The
> default in .jcheck/conf for jdk is currently one, but this can be
> raised by the /reviewers command.

Correct. In the default case, where the number of required reviewers for 
a PR was not raised via the "/reviewers N" command, only one of the 
reviewers who approved the PR needs to review it.

For PRs where "/reviewers N" was used to increase the minimum number of 
reviewers, "N" must re-review. Worth noting is that a Reviewer can lower 
the number of reviewers needed back to 1, so that a single re-review is 
needed. I can imagine this being done by a Reviewer who re-reviews and 
notices that the only change was to fix a typo, update the Copyright 
year, or do a clean merge from master.

> At present, if a Reviewer approves the first draft of a change, there
> can be innumerable subsequent changes to the change, even to the point
> of being completely different to the original, and the change can
> still be committed. I think it would be good to close this loophole in
> the process.
>
> I've had situations in the past where I've been surprised when I've
> requested changes and not had chance to review them before the patch
> has been pushed on the basis of an older stale review. At least one
> Reviewer should ack the version that is finally committed to the
> repository, so I think this is a good proposal.  We also tended to do
> this informally back in the Mercurial days, so I don't think it's a
> completely new idea either.
>
> There is a trade-off against expediency here, but I think it is
> warranted. As Kevin and Aleksei mentioned, this re-review need
> only be checking any changes since the previous review, not a
> review from scratch.
>
> I think there is some confusion in that the 'Reviewed-by' line on the
> commit already seems to have become detached from the set of Reviewers
> who approved the commit. With this change, not only will it include
> Authors and Committers who reviewed the change, but also Reviewers
> whose review was stale and did not count towards Skara's approval
> assessment. Maybe a separate topic is whether we want to lock down
> Reviewed-by to just being the Reviewers Skara counted as valid, rather
> than anyone who reviewed it on GitHub?

That would be a bigger change and would require additional (and 
separate) discussion. I'm not in favor of such a change myself, 
especially given the default case where only one of multiple Reviewers 
re-reviews a minor change to the PR.

-- Kevin


>
> Thanks,
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/jdk-dev/attachments/20240624/b25e7b00/attachment-0001.htm>