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

[Aleksei Ivanov]

That's what I do too. Even if I didn't approve yet, I look at the diffs 
since my last review first; before approving, I quickly look through the 
entire changeset just in case.

Yet it will add an overhead. However, I always try to look at the 
updates to PRs which I reviewed.

-- 
Regards,
Alexey

On 2024-06-19 00:46, Kevin Rushforth wrote:
> When I do a re-review of a PR I've already approved, I only looks at 
> the diffs since my last review. So the amount of additional effort for 
> a trivial change (e.g., fixing a typo or updating copyright years) 
> that is done after I've approved it is very small.
>
> And if it isn't a trivial change, I'd want a closer look anyway.
>
> -- Kevin
>
>
> On 6/18/2024 1:11 PM, Gibbons, Scott wrote:
>> Just a thought - won't this essentially double the load on 
>> reviewers?  I understand the idea, but are the reviewers able to 
>> handle such an increase in workload?
>>
>> Thanks,
>>
>> *--Scott Gibbons*
>>
>> Software Development Engineer, Runtime Engineering
>>
>> DEVELOPER SOFTWARE ENGINEERING
>> Ph: 1-503-456-7756
>>
>> Cell: 1-469-450-8390
>>
>> Intel JF1, 2111 NE 25th Ave
>>
>> Hillsboro, OR 97124
>> Intel Corporation | www.intel.com 
>> <https://webmail.intel.com/owa/redir.aspx?SURL=WYr7qZDpIv3m1SKFmeHJuzsfCBuGN-jwkBYQUSRR6yrupkscpgzUCGgAdAB0AHAAOgAvAC8AdwB3AHcALgBpAG4AdABlAGwALgBjAG8AbQA.&URL=http%3a%2f%2fwww.intel.com>
>>
>> ------------------------------------------------------------------------
>> *From:* jdk-dev <jdk-dev-retn at openjdk.org> on behalf of Kevin 
>> Rushforth <kevin.rushforth at oracle.com>
>> *Sent:* Tuesday, June 18, 2024 10:31 AM
>> *To:* jdk-dev at openjdk.org <jdk-dev at openjdk.org>
>> *Subject:* Re: Proposal: Require re-review before integration if the 
>> PR is modified
>> Yes, it will. At least the minimum number of reviewers will need to
>> re-review.
>>
>> -- Kevin
>>
>>
>> On 6/18/2024 10:27 AM, Daniel Fuchs wrote:
>> > Hi Iris,
>> >
>> > I would welcome this change. But would it block integration
>> > if the new changeset that was pushed is only a merge changeset,
>> > merging changes from the mainline into the PR for a last round
>> > of testing?
>> >
>> > best regards,
>> >
>> > -- daniel
>> >
>> > On 18/06/2024 17:00, Iris Clark wrote:
>> >> Hi.
>> >>
>> >> Every PR must be reviewed by at least one Reviewer [0] before it's
>> >> integrated
>> >> into the main-line repository [1].  While we recommend that the PR
>> >> contain the
>> >> final version of the change at creation time, this is not always
>> >> feasible,
>> >> particularly for large or complex changes.  The integrated version of
>> >> the PR
>> >> may evolve significantly from when the review was done.  Thus the 
>> final
>> >> commit's "Reviewed-by" field may give false confidence that the
>> >> complete set
>> >> of changes has been reviewed before it was integrated into the
>> >> repository.
>> >> This could, in the worst case, allow an adversary to commit malicious
>> >> code.
>> >>
>> >> I propose that reviews be automatically marked as stale when the PR is
>> >> updated, and re-review be required before integration.  Stale reviews
>> >> do not
>> >> count towards the minimum number of reviews required for
>> >> integration.  The
>> >> final commit's "Reviewed-by" field will continue to list all 
>> reviewers,
>> >> regardless of whether they evaluated an older version of the PR.
>> >> When a PR is
>> >> updated, instead of simply noting that the PR applies to a particular
>> >> version,
>> >> the review will be noted as stale, indicating that the PR does not 
>> meet
>> >> integration requirements.  This re-review requirement has been in
>> >> effect for
>> >> the OpenJFX repos [2] since October 2019, when OpenJFX moved to
>> >> GitHub using
>> >> the Skara tooling.
>> >>
>> >> I suggest that we adopt this policy two weeks hence, on Tue 3 July.
>> >>
>> >> Concerns?
>> >>
>> >> [0]https://openjdk.org/guide/#fixing-a-bug (step 12)
>> >> [1]https://github.com/openjdk/jdk
>> >> [2]https://github.com/openjdk/jfx
>> >
>>
>