<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<br>
<blockquote type="cite">Thanks, Jorn. Yeah, okay, but this seems
rather cumbersome. And it leaves the reviewers guessing. As a
Reviewer, I want to see a PR branch that is kept in sync with
master, especially for longer-running PRs.</blockquote>
<br>
An enhancement was filed to not require re-review for a clean merge:<br>
<br>
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.org/browse/SKARA-2312">https://bugs.openjdk.org/browse/SKARA-2312</a><br>
<br>
It is being worked on, although there is no timeline for when it
might be implemented.<br>
<br>
<blockquote type="cite">Another thought, would it be possible for
Skara to disregard changes that just fix the copyrights?</blockquote>
<br>
It might be possible, since Skara does this when comparing whether a
backport is clean. I'm not sure how likely it is.<br>
<br>
-- Kevin<br>
<br>
<br>
<div class="moz-cite-prefix">On 7/4/2024 3:43 AM, Thomas Stüfe
wrote:<br>
</div>
<blockquote type="cite" cite="mid:CAA-vtUwVWh7i8XB0OdFysJ9A=YfBsfLrV=qumjmXuRQ88O7Jtw@mail.gmail.com">
<div dir="ltr">Thanks, Jorn. Yeah, okay, but this seems rather
cumbersome. And it leaves the reviewers guessing. As a Reviewer,
I want to see a PR branch that is kept in sync with master,
especially for longer-running PRs.
<div><br>
</div>
<div>Another thought, would it be possible for Skara to
disregard changes that just fix the copyrights? I have many
reviews ending with "LGTM, please fix copyrights before
pushing". It would be nice not to need another review for
those.</div>
<div><br>
</div>
<div>Thanks, Thomas</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Tue, Jun 25, 2024 at
1:14 PM Jorn Vernee <<a href="mailto:jorn.vernee@oracle.com" moz-do-not-send="true" class="moz-txt-link-freetext">jorn.vernee@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div>
<p>I'd like to also point out that: you are not required to
push the merge to your PR branch in order to have github
actions run for it. The github actions run for any branch
that gets pushed to your fork (if you have them enabled),
the Skara bots just fetch that information when the branch
is used for a PR.<br>
<br>
So, in other words, you could create a new local branch
from your PR branch, do the merge on the new branch, and
push that to your fork, creating a new branch in your fork
with the merge. Github actions would run for the new
branch, but this wouldn't modify the PR branch, so it
wouldn't require a re-review under the proposed change.<br>
</p>
<p>Jorn<br>
</p>
<div>On 25-6-2024 11:19, <a href="mailto:erik.joelsson@oracle.com" target="_blank" moz-do-not-send="true" class="moz-txt-link-freetext">erik.joelsson@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite">
<div>On 6/24/24 06:38, Thomas Stüfe wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div>One question, do merges with master cause the
Skara reviewer check to fail? </div>
<div><br>
</div>
<div>We try to encourage authors to merge often,
especially before pushing. Patches should be based
on a reasonably fresh copy of master. I believe the
contribution guidelines state that too, and it is
just good practice.</div>
<div><br>
</div>
</div>
</blockquote>
<p>Skara does not have a way to distinguish clean merges
from the target branch from other changes when
determining if a review is stale. It simple compares the
hash the review was performed at with the current head
hash of the PR source branch.</p>
<p>Implementing such a check may be possible in the
future, but it's not something we can promise to happen.<br>
</p>
<p>/Erik<br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div>Cheers, Thomas</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Tue, Jun 18, 2024
at 6:00 PM Iris Clark <<a href="mailto:iris.clark@oracle.com" target="_blank" moz-do-not-send="true" class="moz-txt-link-freetext">iris.clark@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">Hi.<br>
<br>
Every PR must be reviewed by at least one Reviewer
[0] before it's integrated<br>
into the main-line repository [1]. While we
recommend that the PR contain the<br>
final version of the change at creation time, this
is not always feasible,<br>
particularly for large or complex changes. The
integrated version of 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 complete set<br>
of changes has been reviewed before it was
integrated into the repository.<br>
This could, in the worst case, allow an adversary to
commit malicious 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 do not<br>
count towards the minimum number of reviews required
for 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. When a PR is<br>
updated, instead of simply noting that the PR
applies to a particular 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 effect for<br>
the OpenJFX repos [2] since October 2019, when
OpenJFX moved to 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 href="https://openjdk.org/guide/#fixing-a-bug" rel="noreferrer" target="_blank" moz-do-not-send="true" class="moz-txt-link-freetext">https://openjdk.org/guide/#fixing-a-bug</a>
(step 12)<br>
[1] <a href="https://github.com/openjdk/jdk" rel="noreferrer" target="_blank" moz-do-not-send="true" class="moz-txt-link-freetext">https://github.com/openjdk/jdk</a><br>
[2] <a href="https://github.com/openjdk/jfx" rel="noreferrer" target="_blank" moz-do-not-send="true" class="moz-txt-link-freetext">https://github.com/openjdk/jfx</a><br>
</blockquote>
</div>
</blockquote>
</blockquote>
</div>
</blockquote>
</div>
</blockquote>
<br>
</body>
</html>