<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
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.<br>
<br>
And if it isn't a trivial change, I'd want a closer look anyway.<br>
<br>
-- Kevin<br>
<br>
<br>
<div class="moz-cite-prefix">On 6/18/2024 1:11 PM, Gibbons, Scott
wrote:<br>
</div>
<blockquote type="cite" cite="mid:DS0PR11MB7734D86EF2710984E247A0F3E2CE2@DS0PR11MB7734.namprd11.prod.outlook.com">
<style type="text/css" style="display:none;">P {margin-top:0;margin-bottom:0;}</style>
<div class="elementToProof" style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 11pt; color: rgb(0, 0, 0);">
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?</div>
<div class="elementToProof" style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 11pt; color: rgb(0, 0, 0);">
<br>
</div>
<div id="Signature">
<p style="margin: 0in; font-family: Calibri, sans-serif; font-size: 11pt;"><span style="font-family: "Intel Clear", sans-serif; font-size: 10pt; color: rgb(31, 56, 100);">Thanks,</span></p>
<p style="margin: 0in; font-family: Calibri, sans-serif; font-size: 11pt;"><span style="font-family: "Intel Clear", sans-serif; font-size: 10pt; color: rgb(31, 56, 100);"><b>--Scott
Gibbons</b></span></p>
<p style="margin: 0in; font-family: Calibri, sans-serif; font-size: 11pt;"><span style="color: black;">Software Development Engineer, Runtime
Engineering</span></p>
<p style="margin: 0in; font-family: Calibri, sans-serif; font-size: 11pt;"><span><img style="width: 67px; height: 49px; margin-top: 0px; margin-bottom: 0px;" data-outlook-trace="F:1|T:1" src="cid:part1.beAHX2JN.p6LKuKoN@oracle.com" class="" width="67" height="49"></span><span style="font-family: "Intel Clear", sans-serif; font-size: 8pt; color: rgb(0, 112, 192);">
DEVELOPER SOFTWARE ENGINEERING</span><span style="color: black;"><br>
Ph: 1-503-456-7756</span></p>
<p style="margin: 0in; font-family: Calibri, sans-serif; font-size: 11pt;"><span style="color: black;">Cell: 1-469-450-8390</span></p>
<p style="margin: 0in;"><span style="font-family: Calibri, sans-serif; font-size: 11pt; color: rgb(34, 34, 34); background-color: white;">Intel
JF1, 2111 NE 25th Ave</span></p>
<p style="margin: 0in; font-family: Calibri, sans-serif; font-size: 11pt;"><span style="color: black;">Hillsboro, OR 97124<br>
Intel Corporation | </span><span style="color: blue;"><a href="https://webmail.intel.com/owa/redir.aspx?SURL=WYr7qZDpIv3m1SKFmeHJuzsfCBuGN-jwkBYQUSRR6yrupkscpgzUCGgAdAB0AHAAOgAvAC8AdwB3AHcALgBpAG4AdABlAGwALgBjAG8AbQA.&URL=http%3a%2f%2fwww.intel.com" target="_blank" style="color: blue; margin-top: 0px; margin-bottom: 0px;" moz-do-not-send="true">www.intel.com</a></span></p>
<p style="margin: 0in; font-family: Calibri, sans-serif; font-size: 11pt;"> </p>
</div>
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font style="font-size:11pt" face="Calibri, sans-serif" color="#000000"><b>From:</b>
jdk-dev <a class="moz-txt-link-rfc2396E" href="mailto:jdk-dev-retn@openjdk.org"><jdk-dev-retn@openjdk.org></a> on behalf of Kevin
Rushforth <a class="moz-txt-link-rfc2396E" href="mailto:kevin.rushforth@oracle.com"><kevin.rushforth@oracle.com></a><br>
<b>Sent:</b> Tuesday, June 18, 2024 10:31 AM<br>
<b>To:</b> <a class="moz-txt-link-abbreviated" href="mailto:jdk-dev@openjdk.org">jdk-dev@openjdk.org</a> <a class="moz-txt-link-rfc2396E" href="mailto:jdk-dev@openjdk.org"><jdk-dev@openjdk.org></a><br>
<b>Subject:</b> Re: Proposal: Require re-review before
integration if the PR is modified</font>
<div> </div>
</div>
<div class="BodyFragment"><font size="2"><span style="font-size:11pt;">
<div class="PlainText">Yes, it will. At least the minimum
number of reviewers will need to
<br>
re-review.<br>
<br>
-- Kevin<br>
<br>
<br>
On 6/18/2024 10:27 AM, Daniel Fuchs wrote:<br>
> Hi Iris,<br>
><br>
> I would welcome this change. But would it block
integration<br>
> if the new changeset that was pushed is only a merge
changeset,<br>
> merging changes from the mainline into the PR for a
last round<br>
> of testing?<br>
><br>
> best regards,<br>
><br>
> -- daniel<br>
><br>
> On 18/06/2024 17:00, Iris Clark wrote:<br>
>> Hi.<br>
>><br>
>> Every PR must be reviewed by at least one
Reviewer [0] before it's <br>
>> integrated<br>
>> into the main-line repository [1]. While we
recommend that the PR <br>
>> contain the<br>
>> final version of the change at creation time,
this is not always <br>
>> feasible,<br>
>> particularly for large or complex changes. The
integrated version of <br>
>> the PR<br>
>> may evolve significantly from when the review was
done. Thus the final<br>
>> commit's "Reviewed-by" field may give false
confidence that the <br>
>> complete set<br>
>> of changes has been reviewed before it was
integrated into the <br>
>> repository.<br>
>> This could, in the worst case, allow an adversary
to commit malicious <br>
>> code.<br>
>><br>
>> I propose that reviews be automatically marked as
stale when the PR is<br>
>> updated, and re-review be required before
integration. Stale reviews <br>
>> do not<br>
>> count towards the minimum number of reviews
required for <br>
>> integration. The<br>
>> final commit's "Reviewed-by" field will continue
to list all reviewers,<br>
>> regardless of whether they evaluated an older
version of the PR. <br>
>> When a PR is<br>
>> updated, instead of simply noting that the PR
applies to a particular <br>
>> version,<br>
>> the review will be noted as stale, indicating
that the PR does not meet<br>
>> integration requirements. This re-review
requirement has been in <br>
>> effect for<br>
>> the OpenJFX repos [2] since October 2019, when
OpenJFX moved to <br>
>> GitHub using<br>
>> the Skara tooling.<br>
>><br>
>> I suggest that we adopt this policy two weeks
hence, on Tue 3 July.<br>
>><br>
>> Concerns?<br>
>><br>
>> [0]<a class="moz-txt-link-freetext" href="https://openjdk.org/guide/#fixing-a-bug">https://openjdk.org/guide/#fixing-a-bug</a> (step
12)<br>
>> [1]<a class="moz-txt-link-freetext" href="https://github.com/openjdk/jdk">https://github.com/openjdk/jdk</a><br>
>> [2]<a class="moz-txt-link-freetext" href="https://github.com/openjdk/jfx">https://github.com/openjdk/jfx</a><br>
><br>
<br>
</div>
</span></font></div>
</blockquote>
<br>
</body>
</html>