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

[Andrew Hughes]

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.

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?

Thanks,
-- 
Andrew :)
Pronouns: he / him or they / them
Principal Free Java Software Engineer
OpenJDK Package Owner
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222

Please contact via e-mail, not proprietary chat networks
Available on Libera Chat & OFTC IRC networks as gnu_andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/jdk-dev/attachments/20240619/4415bf42/signature.asc>